fix: incorrect slot duration when rescheduling bookings#23290
fix: incorrect slot duration when rescheduling bookings#23290anikdhabal merged 19 commits intocalcom:mainfrom
Conversation
|
@sahitya-chandra is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe PR modifies packages/features/bookings/Booker/store.ts. It removes logic that, during rescheduling (when rescheduleUid and bookingData are present), computed the original booking duration from bookingData.startTime and bookingData.endTime and then set selectedDuration and the duration query parameter. Now, in that branch, only selectedTimeslot is cleared (set to null). No public API changes are introduced. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(no out-of-scope changes identified) Possibly related PRs
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
github-actions
bot
added
booking-page
dosubot
bot
added
the
bookings
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (08/22/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (08/22/25)1 label was added to this PR based on Keith Williams's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/features/bookings/Booker/utils/event.ts (3)
100-116: Reschedule duration fix is directionally correct; preserve non-reschedule behavior with a conditional fallback.Passing
durationdirectly fixes the stale slot-length on reschedule, but it also drops the previous fallback to the store (selectedDuration). That can regress non-reschedule flows where we relied on the store to persist the user’s last-picked duration.Recommend: Only bypass the store when rescheduling. Else, keep the original fallback to preserve UX.
Apply:
@@ - const rescheduleUid = searchParams?.get("rescheduleUid"); + const rescheduleUid = searchParams?.get("rescheduleUid"); + // During reschedule, prefer the latest event-type duration (prop). + // Otherwise, preserve user's selected duration from the store. + const effectiveDuration = rescheduleUid ? duration : (durationFromStore ?? duration); @@ - month: monthFromStore ?? month, - duration, + month: monthFromStore ?? month, + duration: effectiveDuration,Verification (manual):
- Create a 15-min event type, book it, then change to 60 min.
- Start reschedule flow (URL contains
rescheduleUid): available slots should be 60 min.- New booking flow (no
rescheduleUid), after manually picking a non-default duration once: navigating months should continue honoringselectedDurationfrom the store.If your intent is to always ignore store duration even outside reschedule, please add a code comment documenting the new precedence and add an e2e to lock it. I can help add either path with tests.
115-115: Use object shorthand for clarity.
useApiV2: useApiV2can be shortened touseApiV2.- useApiV2: useApiV2, + useApiV2,
75-90: Remove unused prop or plumb it through.
fromRedirectOfNonOrgLink?: boolean;is declared in the args type but not used. Either remove it or wire it into downstream calls if intended.Option A — remove now:
teamMemberEmail?: string | null; - fromRedirectOfNonOrgLink?: boolean; isTeamEvent?: boolean; useApiV2?: boolean;Option B — if needed later, add a TODO with reasoning to avoid confusion.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/bookings/Booker/utils/event.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/bookings/Booker/utils/event.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/bookings/Booker/utils/event.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/bookings/Booker/utils/event.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
- GitHub Check: Linters / lint
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/features/bookings/components/event-meta/Duration.tsx (2)
115-121: Use stable keys: prefer the duration value as the React key (avoid index).Index keys can cause unnecessary re-mounts if the list changes. Durations are unique and stable, so use
durationas the key.Apply this diff:
- key={index} + key={duration}
17-26: Avoid mutating theminsparameter ingetDurationFormattedfor clarity.Reassigning function parameters (
mins %= 60) is harder to read and can be error-prone. Use a local variable.Apply this diff:
- const hours = Math.floor(mins / 60); - mins %= 60; + const hours = Math.floor(mins / 60); + const remainMins = mins % 60; @@ - if (mins > 0) { + if (remainMins > 0) { @@ - : t("multiple_duration_timeUnit_short", { count: mins, unit: "minute" }); + : t("multiple_duration_timeUnit_short", { count: remainMins, unit: "minute" });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/bookings/components/event-meta/Duration.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/bookings/components/event-meta/Duration.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/bookings/components/event-meta/Duration.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/bookings/components/event-meta/Duration.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/features/bookings/components/event-meta/Duration.tsx (1)
72-82: LGTM: Fix correctly gates syncing to single-duration and preserves user picks for multi/dynamic.The early-return plus hasMultiple guard prevents snap-backs on multi-duration/dynamic events, while single-duration events now track event.length changes (reschedule case). This directly addresses CAL-6307/#23284 without introducing UX regressions. Nice work.
🧹 Nitpick comments (1)
packages/features/bookings/components/event-meta/Duration.tsx (1)
72-82: Prefer nullish check over truthy for selectedDuration.Using
!selectedDurationconflates undefined with any falsy value. If 0 ever becomes a valid duration, this will misbehave. Use a nullish check to be precise.Apply this minimal diff:
- if (!selectedDuration && (hasMultiple || isDynamicEvent)) { + if (selectedDuration == null && (hasMultiple || isDynamicEvent)) {Note: If you adopt 0-minute as a valid value in the future, review other truthy checks (e.g., the conditional render at Line 104 and the scroll effect at Line 87).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/bookings/components/event-meta/Duration.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/bookings/components/event-meta/Duration.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/bookings/components/event-meta/Duration.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/bookings/components/event-meta/Duration.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Tests / Unit
- GitHub Check: Type check / check-types
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/features/bookings/components/event-meta/Duration.tsx (5)
73-74: Make hasMultiple robust to empty arraysIf multipleDuration is present but empty (e.g., []), the current truthy check treats it as “has multiple,” which can misclassify single-duration events and inhibit the reschedule sync path. Use a non-empty check.
- const hasMultiple = !!event.metadata?.multipleDuration; + const hasMultiple = (event.metadata?.multipleDuration?.length ?? 0) > 0;
87-91: Null-check selectedDuration to future-proof zero-valued durationsUse an explicit null/undefined check instead of a truthy check in the scroll-into-view effect to avoid breaking if 0 ever becomes a valid duration.
- if (selectedDuration && itemRefs.current[selectedDuration]) { + if (selectedDuration != null && itemRefs.current[selectedDuration]) {
104-105: Render guard should also use explicit null/undefined checkThis keeps behavior consistent with the effect logic and avoids hiding the UI if 0 is ever valid.
- return selectedDuration ? ( + return selectedDuration != null ? (
102-103: Fallback durations should apply only when metadata is missing or emptyIf multipleDuration is an empty array, the current code renders nothing. Prefer falling back to the default set when empty.
- const durations = event?.metadata?.multipleDuration || [15, 30, 60, 90]; + const durationsMeta = event?.metadata?.multipleDuration; + const durations = durationsMeta && durationsMeta.length > 0 ? durationsMeta : [15, 30, 60, 90];
121-126: Use stable keys for list itemsUsing index as a key can cause incorrect item identity if durations change order. Duration values are unique and stable—prefer them for keys.
- key={index} + key={duration}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/bookings/components/event-meta/Duration.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/features/bookings/components/event-meta/Duration.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/bookings/components/event-meta/Duration.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/bookings/components/event-meta/Duration.tsx
🔇 Additional comments (1)
packages/features/bookings/components/event-meta/Duration.tsx (1)
72-81: Reschedule bug fix is correct and avoids overriding user picksGating the sync to single-duration events and early-returning for multi/dynamic events is the right approach. This should make reschedules reflect updated event.length while preserving manual selections for multi/dynamic types.
Quick sanity checks:
- Single-duration: create 15m event, book, change to 60m, open reschedule. Expect selectedDuration = 60.
- Multi-duration: event.length = 60; pick 30; ensure selection doesn’t snap back.
- Dynamic: no forced overrides after initial set when unset.
|
@supalarry sir, I have found that So I have pushed the required code, can you have a look at the PR. |
E2E results are ready! |
| // For multi-duration or dynamic events, set only once on mount and preserve user picks. | ||
| const hasMultiple = !!event.metadata?.multipleDuration; | ||
| if (selectedDuration == null && (hasMultiple || isDynamicEvent)) { | ||
| setSelectedDuration(event.length); | ||
| return; | ||
| } | ||
| // For single-duration events, always sync to current event.length (reschedule case). | ||
| if (!hasMultiple && !isDynamicEvent && selectedDuration !== event.length) { | ||
| setSelectedDuration(event.length); | ||
| } |
There was a problem hiding this comment.
Since we’re calling setSelectedDuration(event.length) in both cases, we can simplify this into a single if statement.
|
@anikdhabal sir is this code not working ?? |
4 participants
What does this PR do?
Visual Demo (For contributors especially)
Screencast.from.2025-08-23.02-59-22.webm
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Image Demo (if applicable):
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist