fix(api-v2): respect requested slot range for rolling windows#28939
fix(api-v2): respect requested slot range for rolling windows#28939gfernandez-me wants to merge 3 commits intocalcom:mainfrom
Conversation
… unit tests - Added `disableRollingWindowAdjustment` option to `SlotsInputService_2024_09_04` to control rolling window adjustments. - Updated `transformGetSlotsQuery` method to respect the new option. - Created unit tests for `SlotsInputService_2024_09_04` to verify behavior with and without rolling window adjustments. - Enhanced `getStartTimeForRollingWindowComputation` utility to handle the new option. - Updated relevant types in `trpc` to include the new option.
|
Welcome to Cal.diy, @gfernandez-me! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
github-actions
bot
added
api
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7be46a449c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| orgSlug, | ||
| rescheduleUid, | ||
| rrHostSubsetIds: query.rrHostSubsetIds, | ||
| disableRollingWindowAdjustment: true, |
There was a problem hiding this comment.
Setting disableRollingWindowAdjustment: true on every v2 request causes rolling-window period checks to use only the caller’s requested range, not the near-term dates that calculatePeriodLimits depends on to compute the true window end. For rolling-window event types, a request that starts in the future (for example, a 14-day window queried from day 20 onward) can now return slots outside the configured booking window because missing early dates are treated as non-bookable and push the computed end date forward. Previously, the one-month lookback mitigated this by including near-term dates in the computation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed by removing disableRollingWindowAdjustment from the v2 slots path and shared slots input schema. The rolling-window one-month lookback is still enabled for bounds calculation, while v2 requests still default timeZone to UTC, so the existing final requested-range filter trims the response to the caller’s start/end range.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR adds unit tests and implements a helper function for handling timezone and rolling window computations in the slots module. A new test file validates the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.ts (1)
61-77:⚠️ Potential issue | 🟡 MinorConfirm the timezone default change behavior and type inconsistency before merge.
The new
"UTC"default fortimeZoneintroduces a real behavioral change: requests without an explicit timezone now go throughAvailableSlotsService._filterSlotsByRequestedDateRange(previously they'd short-circuit at line 305 when timeZone was undefined). This filters slots by UTC day boundaries, potentially dropping/keeping edge-day slots differently than the prior behavior.Additionally,
InternalGetSlotsQuery.timeZoneis typedstring | undefinedbut the service always assigns a string value (line 61:const timeZone = query.timeZone ?? "UTC"). Tightening this type tostringwould better reflect the invariant and catch any call site still assuming undefined is possible.The
disableRollingWindowAdjustment: truehardcoding on line 77 prevents v2 callers from opting into the one-month lookback, which is intentional per issue#25405but worth documenting if future per-caller overrides are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.ts` around lines 61 - 77, The change introduces a new default timeZone = query.timeZone ?? "UTC" which changes runtime behavior of AvailableSlotsService._filterSlotsByRequestedDateRange (previously short-circuited when timeZone was undefined); either revert to preserving undefined when not provided or intentionally keep "UTC" but then update the type of InternalGetSlotsQuery.timeZone to string (and update all call sites/tests) to reflect the invariant and add a comment documenting the behavioral change; also add a short note near disableRollingWindowAdjustment in the returned payload (and/or its callers) to document the intentional hardcode (issue `#25405`) and consider exposing a flag if per-caller overrides will be needed.
🧹 Nitpick comments (2)
packages/trpc/server/routers/viewer/slots/types.ts (1)
39-39: LGTM.Optional boolean added consistently with sibling
_bypass*/_silent*internal flags. Note this one is not prefixed with_like the other internal-only flags — consider_disableRollingWindowAdjustmentif the intent is that it's an internal/undocumented knob rather than part of the public tRPC input surface. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/server/routers/viewer/slots/types.ts` at line 39, The new optional flag disableRollingWindowAdjustment is inconsistent with other internal flags that use a leading underscore (e.g., sibling `_bypass*` / `_silent*`), so if this property is intended as an internal/undocumented knob, rename it to `_disableRollingWindowAdjustment` in the type definition (types.ts) and update any consumers that read or validate this field (search for usages of `disableRollingWindowAdjustment`) to use the underscored name; otherwise, leave it as-is if it is intended to be part of the public tRPC input surface.packages/trpc/server/routers/viewer/slots/util.ts (1)
89-105: Helper extraction looks correct; behavior preserved.
getStartTimeForRollingWindowComputationfaithfully replicates the prior inline logic (past-start bypass, non-rolling bypass, subtract-one-month), and the newdisableRollingWindowAdjustmentshort-circuit is placed alongside the other bypass conditions, soinput.startTimeis returned verbatim when the v2 path sets the flag — exactly what issue#25405requires.Minor nit:
isStartTimeInPastis computed even whendisableRollingWindowAdjustmentis true. You can reorder to short-circuit first and skip thedayjscall, but it's micro-optimization.- const isStartTimeInPast = dayjs(startTime).isBefore(dayjs().subtract(1, "day").startOf("day")); - - if (isStartTimeInPast || !isRollingWindowPeriodType || disableRollingWindowAdjustment) { + if (disableRollingWindowAdjustment || !isRollingWindowPeriodType) { + return startTime; + } + const isStartTimeInPast = dayjs(startTime).isBefore(dayjs().subtract(1, "day").startOf("day")); + if (isStartTimeInPast) { return startTime; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/server/routers/viewer/slots/util.ts` around lines 89 - 105, The function getStartTimeForRollingWindowComputation currently computes isStartTimeInPast (using dayjs(startTime)) even when disableRollingWindowAdjustment is true; reorder the checks to short-circuit on disableRollingWindowAdjustment and non-rolling cases before calling dayjs so you skip the unnecessary date parse. Concretely, check disableRollingWindowAdjustment and !isRollingWindowPeriodType first and return startTime immediately if either is true, then compute isStartTimeInPast and proceed with the existing logic (return startTime if past, otherwise subtract one month).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.ts`:
- Around line 61-77: The change introduces a new default timeZone =
query.timeZone ?? "UTC" which changes runtime behavior of
AvailableSlotsService._filterSlotsByRequestedDateRange (previously
short-circuited when timeZone was undefined); either revert to preserving
undefined when not provided or intentionally keep "UTC" but then update the type
of InternalGetSlotsQuery.timeZone to string (and update all call sites/tests) to
reflect the invariant and add a comment documenting the behavioral change; also
add a short note near disableRollingWindowAdjustment in the returned payload
(and/or its callers) to document the intentional hardcode (issue `#25405`) and
consider exposing a flag if per-caller overrides will be needed.
---
Nitpick comments:
In `@packages/trpc/server/routers/viewer/slots/types.ts`:
- Line 39: The new optional flag disableRollingWindowAdjustment is inconsistent
with other internal flags that use a leading underscore (e.g., sibling
`_bypass*` / `_silent*`), so if this property is intended as an
internal/undocumented knob, rename it to `_disableRollingWindowAdjustment` in
the type definition (types.ts) and update any consumers that read or validate
this field (search for usages of `disableRollingWindowAdjustment`) to use the
underscored name; otherwise, leave it as-is if it is intended to be part of the
public tRPC input surface.
In `@packages/trpc/server/routers/viewer/slots/util.ts`:
- Around line 89-105: The function getStartTimeForRollingWindowComputation
currently computes isStartTimeInPast (using dayjs(startTime)) even when
disableRollingWindowAdjustment is true; reorder the checks to short-circuit on
disableRollingWindowAdjustment and non-rolling cases before calling dayjs so you
skip the unnecessary date parse. Concretely, check
disableRollingWindowAdjustment and !isRollingWindowPeriodType first and return
startTime immediately if either is true, then compute isStartTimeInPast and
proceed with the existing logic (return startTime if past, otherwise subtract
one month).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a8cf7f2-7b70-4960-bf92-c7940b101474
📒 Files selected for processing (5)
apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.spec.tsapps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.tspackages/trpc/server/routers/viewer/slots/types.tspackages/trpc/server/routers/viewer/slots/util.test.tspackages/trpc/server/routers/viewer/slots/util.ts
…ut service - Updated `SlotsInputService_2024_09_04` to no longer set `disableRollingWindowAdjustment` by default. - Adjusted unit tests to reflect the change, ensuring that rolling window bounds checks are preserved. - Removed references to `disableRollingWindowAdjustment` from relevant types and utility functions.
1 participant
What changed
Fixes #25405.
This updates the v2 slots flow so
GET /v2/slotsrespects the requestedstartandendrange when an event type uses a rolling booking window.disableRollingWindowAdjustmentoption to the shared slots calculation path.timeZonevalues toUTC, matching the endpoint docs.Testing
TZ=UTC yarn test packages/trpc/server/routers/viewer/slots/util.test.tsyarn workspace @calcom/api-v2 jest src/modules/slots/slots-2024-09-04/services/slots-input.service.spec.ts --runInBandyarn workspace @calcom/trpc build:serverNODE_OPTIONS='--max_old_space_size=8192' yarn workspace @calcom/api-v2 buildyarn biome format apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.ts apps/api/v2/src/modules/slots/slots-2024-09-04/services/slots-input.service.spec.ts packages/trpc/server/routers/viewer/slots/types.ts packages/trpc/server/routers/viewer/slots/util.ts packages/trpc/server/routers/viewer/slots/util.test.tsgit diff --check