fix: Reservation-to-Booking flow allows double-booking reserved slots#23988
fix: Reservation-to-Booking flow allows double-booking reserved slots#23988Anshumancanrock wants to merge 13 commits intocalcom:mainfrom
Conversation
|
@Anshumancanrock is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
bookings
WalkthroughThis change threads an optional Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
packages/features/bookings/lib/handleNewBooking/createBooking.ts (1)
142-147: Minor: Prisma guideline — prefer select over include.Not blocking, but our repo guideline is “only select what you need; avoid include.” Consider switching to select in a follow-up.
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts (8)
88-91: Fix brittle array assertion on responses
toContainwith an object checks by reference. Use arrayContaining + objectContaining.Apply:
- expect(createdBooking.responses).toContain({ - email: booker.email, - name: booker.name, - }); + expect(createdBooking.responses).toEqual( + expect.arrayContaining([expect.objectContaining({ email: booker.email, name: booker.name })]) + );
96-98: Loosen Prisma call assertion to avoid over‑specificationImplementation may pass additional keys or use transactions. Match on uid only.
Apply:
- expect(prismaMock.selectedSlots.delete).toHaveBeenCalledWith({ - where: { uid: "reserved-slot-123" }, - }); + expect(prismaMock.selectedSlots.delete).toHaveBeenCalledWith( + expect.objectContaining({ where: { uid: "reserved-slot-123" } }) + );
197-199: Same Day.js object bug in expired-reservation mockUse
.dateStringwith explicit UTC time.Apply:
- slotUtcStartDate: dayjs(getDate({ dateIncrement: 1 })).utc().toDate(), - slotUtcEndDate: dayjs(getDate({ dateIncrement: 1 })).utc().add(15, "minutes").toDate(), + slotUtcStartDate: dayjs(`${getDate({ dateIncrement: 1 }).dateString}T12:00:00.000Z`).toDate(), + slotUtcEndDate: dayjs(`${getDate({ dateIncrement: 1 }).dateString}T12:15:00.000Z`).toDate(),
223-225: Loosen Prisma delete assertion for expired cleanupAlign with objectContaining.
Apply:
- expect(prismaMock.selectedSlots.delete).toHaveBeenCalledWith({ - where: { uid: "expired-reservation" }, - }); + expect(prismaMock.selectedSlots.delete).toHaveBeenCalledWith( + expect.objectContaining({ where: { uid: "expired-reservation" } }) + );
265-267: Fix mismatched‑time test inputs (invalid dates today)Use
.dateStringand explicit UTC times to guarantee the mismatch is tested (not date parsing).Apply:
- slotUtcStartDate: dayjs(getDate({ dateIncrement: 2 })).utc().toDate(), // Different day - slotUtcEndDate: dayjs(getDate({ dateIncrement: 2 })).utc().add(15, "minutes").toDate(), + slotUtcStartDate: dayjs(`${getDate({ dateIncrement: 2 }).dateString}T12:00:00.000Z`).toDate(), // Different day + slotUtcEndDate: dayjs(`${getDate({ dateIncrement: 2 }).dateString}T12:15:00.000Z`).toDate(), @@ - start: dayjs(getDate({ dateIncrement: 1 })).toISOString(), // Different from reservation - end: dayjs(getDate({ dateIncrement: 1 })).add(15, "minutes").toISOString(), + start: `${getDate({ dateIncrement: 1 }).dateString}T12:00:00.000Z`, // Different from reservation + end: `${getDate({ dateIncrement: 1 }).dateString}T12:15:00.000Z`,Also applies to: 276-277
42-42: Remove unused destructured variable
reservedSlotUidis not used.Apply:
- const { eventType, reservedSlotUid } = await createBookingScenario({ + const { eventType } = await createBookingScenario({
85-105: Optionally assert atomicity via transactionIf the handler wraps reservation validation + booking + deletion in a single DB transaction, assert it.
Apply:
const createdBooking = await handleNewBooking(req); + expect(prismaMock.$transaction).toHaveBeenCalledTimes(1);If
$transactionisn’t exposed onprismaMock, I can help wire that mock.
1-6: Nice coverage and scenariosGood breadth: success, not found, expired (with cleanup), mismatch, and no-reservation path.
Consider adding a “parallel double-consume” test (two concurrent handleNewBooking calls with the same UID) to prove the transaction prevents double-booking. I can sketch this if helpful.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsx(3 hunks)packages/features/bookings/lib/bookingCreateBodySchema.ts(1 hunks)packages/features/bookings/lib/handleNewBooking.ts(2 hunks)packages/features/bookings/lib/handleNewBooking/createBooking.ts(5 hunks)packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts(1 hunks)packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/lib/book-event-form/booking-to-mutation-input-mapper.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/lib/book-event-form/booking-to-mutation-input-mapper.tsxpackages/features/bookings/lib/bookingCreateBodySchema.tspackages/features/bookings/lib/handleNewBooking/createBooking.tspackages/platform/atoms/hooks/bookings/useHandleBookEvent.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.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/lib/book-event-form/booking-to-mutation-input-mapper.tsxpackages/features/bookings/lib/bookingCreateBodySchema.tspackages/features/bookings/lib/handleNewBooking/createBooking.tspackages/platform/atoms/hooks/bookings/useHandleBookEvent.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
**/*.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/lib/bookingCreateBodySchema.tspackages/features/bookings/lib/handleNewBooking/createBooking.tspackages/platform/atoms/hooks/bookings/useHandleBookEvent.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: In the Cal.com codebase, the forceRescheduleForCancelledBooking flag historically affects both CANCELLED and REJECTED booking statuses, despite its name suggesting it should only affect cancelled bookings. This behavior existed before PR #23736 and was preserved during the refactoring.
📚 Learning: 2025-07-15T12:59:34.389Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsxpackages/features/bookings/lib/handleNewBooking.ts
📚 Learning: 2025-09-12T11:23:34.158Z
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: The test file packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.test.ts explicitly documents on line 236 that the current behavior of forceRescheduleForCancelledBooking affecting both CANCELLED and REJECTED bookings is known to be incorrect, but is preserved as "Current Behavior" for backward compatibility. The test comment states the expected behavior should be that REJECTED bookings redirect to booking details even when forceRescheduleForCancelledBooking=true.
Applied to files:
packages/features/bookings/lib/handleNewBooking/createBooking.tspackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
📚 Learning: 2025-08-29T22:57:31.407Z
Learnt from: bandhan-majumder
PR: calcom/cal.com#23454
File: packages/features/bookings/Booker/components/EventMeta.tsx:16-16
Timestamp: 2025-08-29T22:57:31.407Z
Learning: In Cal.com's Booker architecture, components become client-side through BookerStoreProvider.tsx which has "use client". Any component using useBookerStoreContext automatically runs on the client and should use client-appropriate utilities like markdownToSafeHTMLClient, regardless of whether they have explicit "use client" directives.
Applied to files:
packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts
🧬 Code graph analysis (1)
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts (3)
packages/features/bookings/lib/handleNewBooking/test/getNewBookingHandler.ts (1)
getNewBookingHandler(6-8)apps/web/test/utils/bookingScenario/bookingScenario.ts (5)
getBooker(2220-2234)getOrganizer(1520-1576)TestData(1239-1511)createBookingScenario(978-1009)getDate(1093-1140)apps/web/test/utils/bookingScenario/expects.ts (1)
expectBookingToBeInDatabase(418-435)
🔇 Additional comments (4)
packages/features/bookings/lib/bookingCreateBodySchema.ts (1)
33-37: Schema addition LGTM.Optional string with doc comment fits the API surface.
Please confirm the API docs and client typings are regenerated where needed.
packages/features/bookings/lib/handleNewBooking.ts (1)
482-484: Propagation is correct; ensure user-friendly error mapping for reservation failures.createBooking now validates/consumes reservations. Make sure errors from that path surface as HttpError with stable, translatable codes (e.g., reserved_slot_not_found/expired/mismatch) rather than raw Prisma errors, so UI can display t(code). See suggested throws in createBooking.ts comment.
Also applies to: 1530-1531
packages/features/bookings/lib/handleNewBooking/createBooking.ts (2)
28-29: Type surface change LGTM.Optional reservedSlotUid on CreateBookingParams is appropriate.
90-117: Threading reservedSlotUid through _createBooking/saveBooking is fine.No issues with param plumbing.
Please ensure all callers pass it only when present.
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsx
Outdated
Show resolved
Hide resolved
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
Outdated
Show resolved
Hide resolved
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
Outdated
Show resolved
Hide resolved
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsx (1)
131-145: Avoid carrying reservedSlotUid in base input to prevent accidental propagation.Build the base input without reservation and inject it per occurrence. This reduces chances of leaks if more fields get spread later.
- const input = mapBookingToMutationInput({ ...booking, bookingUid: undefined }); + // Exclude reservation from the shared base input + const { reservedSlotUid, ...bookingWithoutReservation } = booking; + const input = mapBookingToMutationInput({ ...bookingWithoutReservation, bookingUid: undefined }); - return recurringDates.map((recurringDate, index) => ({ + return recurringDates.map((recurringDate, index) => ({ ...input, @@ - // Only include reservedSlotUid for the first occurrence - reservedSlotUid: index === 0 ? input.reservedSlotUid : undefined, + // Only first occurrence consumes the reservation + reservedSlotUid: index === 0 ? reservedSlotUid : undefined, }));packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts (1)
129-149: Clear reservation with undefined and clear on any success to avoid stale state.
- Store “no reservation” as undefined (not null) to align with typical hook semantics.
- Clearing on all successful bookings avoids cross‑event bleed if users switch event types.
- const clearReservationOnSuccess = () => { - if (reservedSlotUid && - event.data.schedulingType !== "ROUND_ROBIN" && - event.data.schedulingType !== "COLLECTIVE") { - setReservedSlotUid(null); - } - }; + const clearReservationOnSuccess = () => { + if (reservedSlotUid) { + setReservedSlotUid(undefined); + } + };packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts (3)
105-109: Use objectContaining for responses (it’s an object, not an array).[toContain] on objects is unreliable here.
- expect(createdBooking.responses).toContain({ - email: booker.email, - name: booker.name, - }); + expect(createdBooking.responses).toEqual( + expect.objectContaining({ email: booker.email, name: booker.name }) + );
156-158: Mock the correct Prisma call (findFirst vs findUnique).Code uses findFirst; mocking findUnique is inert.
- prismaMock.selectedSlots.findUnique.mockResolvedValue(null); + prismaMock.selectedSlots.findFirst.mockResolvedValue(null);
382-383: Assert no findFirst calls instead of findUnique.- expect(prismaMock.selectedSlots.findUnique).not.toHaveBeenCalled(); + expect(prismaMock.selectedSlots.findFirst).not.toHaveBeenCalled();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsx(5 hunks)packages/features/bookings/lib/handleNewBooking/createBooking.ts(6 hunks)packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts(1 hunks)packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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/lib/handleNewBooking/createBooking.tspackages/platform/atoms/hooks/bookings/useHandleBookEvent.tspackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.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/lib/handleNewBooking/createBooking.tspackages/platform/atoms/hooks/bookings/useHandleBookEvent.tspackages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsxpackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.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/lib/handleNewBooking/createBooking.tspackages/platform/atoms/hooks/bookings/useHandleBookEvent.tspackages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsxpackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
**/*.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/lib/book-event-form/booking-to-mutation-input-mapper.tsx
🧠 Learnings (9)
📓 Common learnings
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: In the Cal.com codebase, the forceRescheduleForCancelledBooking flag historically affects both CANCELLED and REJECTED booking statuses, despite its name suggesting it should only affect cancelled bookings. This behavior existed before PR #23736 and was preserved during the refactoring.
Learnt from: eunjae-lee
PR: calcom/cal.com#23166
File: packages/prisma/migrations/20250818151914_routing_form_response_denormalized_backfill2/migration.sql:65-66
Timestamp: 2025-08-19T09:47:49.478Z
Learning: The Booking table has a unique constraint and index on the uid column (defined as `uid String unique` in schema.prisma), so JOINs on Booking.uid have optimal performance and don't require additional indexing.
📚 Learning: 2025-09-12T11:23:34.158Z
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: In the Cal.com codebase, the forceRescheduleForCancelledBooking flag historically affects both CANCELLED and REJECTED booking statuses, despite its name suggesting it should only affect cancelled bookings. This behavior existed before PR #23736 and was preserved during the refactoring.
Applied to files:
packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts
📚 Learning: 2025-09-12T11:23:34.158Z
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: The test file packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.test.ts explicitly documents on line 236 that the current behavior of forceRescheduleForCancelledBooking affecting both CANCELLED and REJECTED bookings is known to be incorrect, but is preserved as "Current Behavior" for backward compatibility. The test comment states the expected behavior should be that REJECTED bookings redirect to booking details even when forceRescheduleForCancelledBooking=true.
Applied to files:
packages/platform/atoms/hooks/bookings/useHandleBookEvent.tspackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
📚 Learning: 2025-08-29T22:57:31.407Z
Learnt from: bandhan-majumder
PR: calcom/cal.com#23454
File: packages/features/bookings/Booker/components/EventMeta.tsx:16-16
Timestamp: 2025-08-29T22:57:31.407Z
Learning: In Cal.com's Booker architecture, components become client-side through BookerStoreProvider.tsx which has "use client". Any component using useBookerStoreContext automatically runs on the client and should use client-appropriate utilities like markdownToSafeHTMLClient, regardless of whether they have explicit "use client" directives.
Applied to files:
packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts
📚 Learning: 2025-07-15T12:59:34.389Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsx
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.{ts,tsx} : Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js `.utc()` in hot paths like loops
Applied to files:
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
📚 Learning: 2025-08-21T13:44:06.805Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.805Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
Applied to files:
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
📚 Learning: 2025-09-18T08:50:49.906Z
Learnt from: eunjae-lee
PR: calcom/cal.com#23752
File: packages/lib/server/service/InsightsBookingBaseService.ts:340-371
Timestamp: 2025-09-18T08:50:49.906Z
Learning: In the InsightsBookingBaseService (packages/lib/server/service/InsightsBookingBaseService.ts), the startTime date range filter intentionally uses different fields for the lower and upper bounds: `startDate <= "startTime" AND "endTime" <= endDate`. This ensures bookings start within the selected range but don't extend beyond the end date, which is the intended business logic and not an inconsistency.
Applied to files:
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
🧬 Code graph analysis (2)
packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts (1)
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsx (2)
mapBookingToMutationInput(38-108)mapRecurringBookingToMutationInput(114-146)
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts (3)
packages/features/bookings/lib/handleNewBooking/test/getNewBookingHandler.ts (1)
getNewBookingHandler(6-8)apps/web/test/utils/bookingScenario/bookingScenario.ts (5)
getBooker(2220-2234)getOrganizer(1520-1576)TestData(1239-1511)createBookingScenario(978-1009)getDate(1093-1140)apps/web/test/utils/bookingScenario/expects.ts (1)
expectBookingToBeInDatabase(418-435)
⏰ 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: Type check / check-types
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
🔇 Additional comments (8)
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsx (2)
35-35: Good: BookingOptions threads reservedSlotUid end-to-end.
106-107: Good: reservedSlotUid included in mutation payload.packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts (2)
120-124: Correct: gate reservedSlotUid by scheduling type.
151-160: Good: wrap callbacks so success clears reservation across all flows (instant/recurring/standard).packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts (1)
226-227: Same as above: expired path should mock findFirst.- prismaMock.selectedSlots.findUnique.mockResolvedValue(expiredReservation); + prismaMock.selectedSlots.findFirst.mockResolvedValue(expiredReservation);packages/features/bookings/lib/handleNewBooking/createBooking.ts (3)
184-187: Prefer findUnique when uid is unique — verify Prisma schemaIf the Prisma model backing tx.selectedSlots declares uid as @unique (or in an @@unique), replace findFirst with findUnique({ where: { uid: reservedSlotUid } }) to use the unique index; otherwise keep findFirst.
Location: packages/features/bookings/lib/handleNewBooking/createBooking.ts:184-187. I couldn't find prisma/schema.prisma in the repo — confirm the model's uid uniqueness and update accordingly.
182-216: Optional hardening — validate reservation belongs to the organizer/event before consumingCouldn't verify Prisma schema (prisma/ not found). Confirm selectedSlots includes owner/event columns and enforce one of the checks below before deleting the reservation.
- Assert reservation.userId === organizerUser.id and/or reservation.eventTypeId === newBookingData.eventTypeId.
- If those columns are absent, enforce ownership via a join (selectedSlots → slot/event → organizer) or add explicit userId/eventTypeId to selectedSlots.
File: packages/features/bookings/lib/handleNewBooking/createBooking.ts (lines 182–216)
182-216: Expired-reservation cleanup uses deleteMany; tests expect delete — pick one and keep consistent.To match current tests and keep consumption path race‑proof (deleteMany), switch only the expired‑cleanup to delete.
- if (reservation.releaseAt && dayjs().isAfter(reservation.releaseAt)) { - // Clean up expired reservation and throw error - await tx.selectedSlots.deleteMany({ - where: { uid: reservedSlotUid }, - }); - throw new HttpError({ statusCode: 410, message: "reserved_slot_expired" }); - } + if (reservation.releaseAt && dayjs().isAfter(reservation.releaseAt)) { + // Clean up expired reservation (single row) and throw + await tx.selectedSlots.delete({ where: { uid: reservedSlotUid } }); + throw new HttpError({ statusCode: 410, message: "reserved_slot_expired" }); + }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsx (2)
35-35: Document the semantics of reservedSlotUid.Add a brief JSDoc clarifying when this should be set (single booking or first recurrence only) and that RR/Collective flows should not include it. Helps future callers avoid double-consume bugs.
Apply:
verificationCode?: string; - reservedSlotUid?: string; + /** UID of a reserved slot to be consumed atomically during booking creation. + * Use only for single bookings or the first occurrence of a recurring series. + * Not applicable to ROUND_ROBIN or COLLECTIVE scheduling. */ + reservedSlotUid?: string;
135-147: Only first recurrence consumes the reservation; consider omitting undefined property.Logic is correct. Small polish: avoid emitting reservedSlotUid: undefined by conditionally spreading.
Apply:
- // Only first occurrence should consume the reservation - reservedSlotUid: idx === 0 ? reservedSlotUid : undefined, + // Only first occurrence should consume the reservation + ...(idx === 0 && reservedSlotUid ? { reservedSlotUid } : {}),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- 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/lib/book-event-form/booking-to-mutation-input-mapper.tsx(5 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/lib/book-event-form/booking-to-mutation-input-mapper.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/lib/book-event-form/booking-to-mutation-input-mapper.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/lib/book-event-form/booking-to-mutation-input-mapper.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: In the Cal.com codebase, the forceRescheduleForCancelledBooking flag historically affects both CANCELLED and REJECTED booking statuses, despite its name suggesting it should only affect cancelled bookings. This behavior existed before PR #23736 and was preserved during the refactoring.
Learnt from: eunjae-lee
PR: calcom/cal.com#23166
File: packages/prisma/migrations/20250818151914_routing_form_response_denormalized_backfill2/migration.sql:65-66
Timestamp: 2025-08-19T09:47:49.478Z
Learning: The Booking table has a unique constraint and index on the uid column (defined as `uid String unique` in schema.prisma), so JOINs on Booking.uid have optimal performance and don't require additional indexing.
📚 Learning: 2025-07-15T12:59:34.389Z
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Applied to files:
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsx
🔇 Additional comments (3)
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsx (3)
60-61: Prop wiring looks correct.Including reservedSlotUid in the mapper’s args is correct and keeps the API surface consistent.
131-134: Good fix: base input excludes reservation to avoid multi-consume.This addresses the earlier concern about copying the reservation into every recurrence.
106-107: Forwarding reservedSlotUid is correct — schemas/handlers already accept it.bookingCreateBodySchema defines reservedSlotUid as optional (packages/features/bookings/lib/bookingCreateBodySchema.ts:36); createBooking handler/DTOs handle it (packages/features/bookings/lib/handleNewBooking/createBooking.ts) and there are reservation tests — no changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts (1)
120-124: Gate reservedSlotUid with a ternary for type-safety (avoid spreading boolean).The current pattern spreads
falsewhen unsupported; TS can complain in some configs. Prefer a ternary.- ...(event.data.schedulingType !== "ROUND_ROBIN" && - event.data.schedulingType !== "COLLECTIVE" && { - reservedSlotUid: reservedSlotUid || undefined, - }), + ...(event.data.schedulingType !== "ROUND_ROBIN" && + event.data.schedulingType !== "COLLECTIVE" + ? { reservedSlotUid: reservedSlotUid || undefined } + : {}),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts(1 hunks)packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
🧰 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/platform/atoms/hooks/bookings/useHandleBookEvent.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/platform/atoms/hooks/bookings/useHandleBookEvent.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/platform/atoms/hooks/bookings/useHandleBookEvent.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: In the Cal.com codebase, the forceRescheduleForCancelledBooking flag historically affects both CANCELLED and REJECTED booking statuses, despite its name suggesting it should only affect cancelled bookings. This behavior existed before PR #23736 and was preserved during the refactoring.
Learnt from: eunjae-lee
PR: calcom/cal.com#23166
File: packages/prisma/migrations/20250818151914_routing_form_response_denormalized_backfill2/migration.sql:65-66
Timestamp: 2025-08-19T09:47:49.478Z
Learning: The Booking table has a unique constraint and index on the uid column (defined as `uid String unique` in schema.prisma), so JOINs on Booking.uid have optimal performance and don't require additional indexing.
📚 Learning: 2025-09-12T11:23:34.158Z
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: In the Cal.com codebase, the forceRescheduleForCancelledBooking flag historically affects both CANCELLED and REJECTED booking statuses, despite its name suggesting it should only affect cancelled bookings. This behavior existed before PR #23736 and was preserved during the refactoring.
Applied to files:
packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts
📚 Learning: 2025-09-12T11:23:34.158Z
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: The test file packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.test.ts explicitly documents on line 236 that the current behavior of forceRescheduleForCancelledBooking affecting both CANCELLED and REJECTED bookings is known to be incorrect, but is preserved as "Current Behavior" for backward compatibility. The test comment states the expected behavior should be that REJECTED bookings redirect to booking details even when forceRescheduleForCancelledBooking=true.
Applied to files:
packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts
📚 Learning: 2025-08-29T22:57:31.407Z
Learnt from: bandhan-majumder
PR: calcom/cal.com#23454
File: packages/features/bookings/Booker/components/EventMeta.tsx:16-16
Timestamp: 2025-08-29T22:57:31.407Z
Learning: In Cal.com's Booker architecture, components become client-side through BookerStoreProvider.tsx which has "use client". Any component using useBookerStoreContext automatically runs on the client and should use client-appropriate utilities like markdownToSafeHTMLClient, regardless of whether they have explicit "use client" directives.
Applied to files:
packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts
🧬 Code graph analysis (1)
packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts (1)
packages/features/bookings/lib/book-event-form/booking-to-mutation-input-mapper.tsx (2)
mapBookingToMutationInput(38-108)mapRecurringBookingToMutationInput(114-148)
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (5)
packages/platform/atoms/hooks/bookings/useHandleBookEvent.ts (5)
7-7: LGTM: Threading reservation ID via hook.Importing useSlotReservationId here is correct and aligns the hook with the new reservation lifecycle.
78-78: Behavior change: onError now always provided (toast shown).enhancedCallbacks supplies onError when callbacks is undefined, which may surface toasts in flows that previously suppressed them (e.g., platform/no inputTimeSlot). Confirm this is intended.
129-136: Clear reservation by removing the value, not setting an empty string.Use
undefined(or a dedicated remover) so the storage/key is actually cleared; empty string may linger in storage/state and is semantically different from “absent”.- setReservedSlotUid(""); + setReservedSlotUid(undefined);#!/bin/bash # Check for other places setting empty reservation values rg -n -C2 --type=ts 'setReservedSlotUid\('
139-150: LGTM: Enhanced callbacks safely compose onSuccess.Good preservation of caller onSuccess while injecting clearReservationOnSuccess; onError passthrough also maintained.
153-161: LGTM: Pass enhancedCallbacks across all booking paths.Ensures consistent reservation clearing for instant, recurring, and standard bookings.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/bookings/lib/handleNewBooking/createBooking.ts (1)
4-13: Import HttpError for structured errors.import type { routingFormResponseInDbSchema } from "@calcom/app-store/routing-forms/zod"; import dayjs from "@calcom/dayjs"; +import { HttpError } from "@calcom/lib/http-error"; import { isPrismaObjOrUndefined } from "@calcom/lib/isPrismaObj";
🧹 Nitpick comments (1)
packages/features/bookings/lib/handleNewBooking/createBooking.ts (1)
138-144: Export saveBooking for testing — do not add| nullto the parameterExporting saveBooking is fine (makes it testable); OriginalRescheduledBooking is already nullable (defined as ... | null in packages/features/bookings/lib/handleNewBooking/originalRescheduledBookingUtils.ts), so keep the parameter type as OriginalRescheduledBooking.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/features/bookings/lib/handleNewBooking/createBooking.ts(5 hunks)packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/test/reservation-simple.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
🧰 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/lib/handleNewBooking/createBooking.tspackages/features/bookings/lib/handleNewBooking/test/reservation-simple.test.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/lib/handleNewBooking/createBooking.tspackages/features/bookings/lib/handleNewBooking/test/reservation-simple.test.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/lib/handleNewBooking/createBooking.tspackages/features/bookings/lib/handleNewBooking/test/reservation-simple.test.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: In the Cal.com codebase, the forceRescheduleForCancelledBooking flag historically affects both CANCELLED and REJECTED booking statuses, despite its name suggesting it should only affect cancelled bookings. This behavior existed before PR #23736 and was preserved during the refactoring.
Learnt from: eunjae-lee
PR: calcom/cal.com#23166
File: packages/prisma/migrations/20250818151914_routing_form_response_denormalized_backfill2/migration.sql:65-66
Timestamp: 2025-08-19T09:47:49.478Z
Learning: The Booking table has a unique constraint and index on the uid column (defined as `uid String unique` in schema.prisma), so JOINs on Booking.uid have optimal performance and don't require additional indexing.
📚 Learning: 2025-09-12T11:23:34.158Z
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: The test file packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.test.ts explicitly documents on line 236 that the current behavior of forceRescheduleForCancelledBooking affecting both CANCELLED and REJECTED bookings is known to be incorrect, but is preserved as "Current Behavior" for backward compatibility. The test comment states the expected behavior should be that REJECTED bookings redirect to booking details even when forceRescheduleForCancelledBooking=true.
Applied to files:
packages/features/bookings/lib/handleNewBooking/createBooking.tspackages/features/bookings/lib/handleNewBooking/test/reservation-simple.test.ts
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/features/bookings/lib/handleNewBooking/test/reservation-simple.test.ts
📚 Learning: 2025-09-03T09:52:51.182Z
Learnt from: hariombalhara
PR: calcom/cal.com#23541
File: packages/features/bookings/lib/di/modules/RegularBookingServiceModule.ts:22-28
Timestamp: 2025-09-03T09:52:51.182Z
Learning: The IBookingServiceDependencies interface in packages/features/bookings/lib/handleNewBooking.ts contains 6 properties: cacheService, checkBookingAndDurationLimitsService, prismaClient, bookingRepository, featuresRepository, and checkBookingLimitsService. This interface is used by RegularBookingService and matches the depsMap structure in RegularBookingServiceModule.
Applied to files:
packages/features/bookings/lib/handleNewBooking/test/reservation-simple.test.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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (3)
packages/features/bookings/lib/handleNewBooking/createBooking.ts (3)
28-29: LGTM: reservedSlotUid threading added to params.
90-90: LGTM: reservedSlotUid plumbed through to saveBooking.Also applies to: 115-116, 142-144
176-214: Reservation validation: use findFirst + deleteMany, throw HttpError, and compare to minute precision (race-proof + matches tests).Current code throws generic Error, uses findUnique/delete (racy), and exact time equality. This will fail the new tests and is vulnerable under contention.
return prisma.$transaction(async (tx) => { - // If we have a reservedSlotUid, validate that the reservation exists and delete it + // If we have a reservedSlotUid, validate and consume the reservation atomically if (reservedSlotUid) { - const reservation = await tx.selectedSlots.findUnique({ - where: { uid: reservedSlotUid }, - }); + const reservation = await tx.selectedSlots.findFirst({ + where: { uid: reservedSlotUid }, + }); if (!reservation) { - throw new Error("Reserved slot not found or already consumed"); + throw new HttpError({ statusCode: 409, message: "reserved_slot_not_found" }); } // Check if reservation is still valid (not expired) if (reservation.releaseAt && dayjs().isAfter(reservation.releaseAt)) { - // Clean up expired reservation and throw error - await tx.selectedSlots.delete({ - where: { uid: reservedSlotUid }, - }); - throw new Error("Reserved slot has expired"); + // Clean up expired reservation and throw error + await tx.selectedSlots.deleteMany({ where: { uid: reservedSlotUid } }); + throw new HttpError({ statusCode: 410, message: "reserved_slot_expired" }); } // Validate that the reservation matches the booking time slot const bookingStart = dayjs(newBookingData.startTime); const bookingEnd = dayjs(newBookingData.endTime); const reservationStart = dayjs(reservation.slotUtcStartDate); const reservationEnd = dayjs(reservation.slotUtcEndDate); - if (!bookingStart.isSame(reservationStart) || !bookingEnd.isSame(reservationEnd)) { - throw new Error("Reserved slot time does not match booking time"); + if (!bookingStart.isSame(reservationStart, "minute") || !bookingEnd.isSame(reservationEnd, "minute")) { + throw new HttpError({ statusCode: 400, message: "reserved_slot_time_mismatch" }); } - // Delete the reservation to consume it - await tx.selectedSlots.delete({ - where: { uid: reservedSlotUid }, - }); + // Delete the reservation to consume it (race-proof) + const { count } = await tx.selectedSlots.deleteMany({ where: { uid: reservedSlotUid } }); + if (count !== 1) { + throw new HttpError({ statusCode: 409, message: "reserved_slot_not_found" }); + } }
packages/features/bookings/lib/handleNewBooking/test/reservation-simple.test.ts
Outdated
Show resolved
Hide resolved
packages/features/bookings/lib/handleNewBooking/test/reservation-simple.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/features/bookings/lib/handleNewBooking/test/reservation-logic.test.ts (1)
1-1: Inconsistent test function usageThis file uses
itwhile the other test files usetest. While both are valid in Vitest, consistency across the test suite improves readability.-import { describe, it, expect, vi, beforeEach } from "vitest"; +import { describe, test, expect, vi, beforeEach } from "vitest";Then update all
itcalls totestthroughout the file for consistency with other test files.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
packages/features/bookings/lib/handleNewBooking/createBooking.ts(5 hunks)packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/test/reservation-logic.test.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/test/reservation-simple.test.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/test/reservation-system.test.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/test/reservation-validation.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/features/bookings/lib/handleNewBooking/createBooking.ts
- packages/features/bookings/lib/handleNewBooking/test/reservation-simple.test.ts
🧰 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/lib/handleNewBooking/test/reservation-logic.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-validation.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-system.test.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/lib/handleNewBooking/test/reservation-logic.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-validation.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-system.test.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/lib/handleNewBooking/test/reservation-logic.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-validation.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-system.test.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: In the Cal.com codebase, the forceRescheduleForCancelledBooking flag historically affects both CANCELLED and REJECTED booking statuses, despite its name suggesting it should only affect cancelled bookings. This behavior existed before PR #23736 and was preserved during the refactoring.
Learnt from: eunjae-lee
PR: calcom/cal.com#23166
File: packages/prisma/migrations/20250818151914_routing_form_response_denormalized_backfill2/migration.sql:65-66
Timestamp: 2025-08-19T09:47:49.478Z
Learning: The Booking table has a unique constraint and index on the uid column (defined as `uid String unique` in schema.prisma), so JOINs on Booking.uid have optimal performance and don't require additional indexing.
📚 Learning: 2025-09-12T11:23:34.158Z
Learnt from: hariombalhara
PR: calcom/cal.com#23736
File: packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.ts:73-84
Timestamp: 2025-09-12T11:23:34.158Z
Learning: The test file packages/features/bookings/lib/reschedule/determineReschedulePreventionRedirect.test.ts explicitly documents on line 236 that the current behavior of forceRescheduleForCancelledBooking affecting both CANCELLED and REJECTED bookings is known to be incorrect, but is preserved as "Current Behavior" for backward compatibility. The test comment states the expected behavior should be that REJECTED bookings redirect to booking details even when forceRescheduleForCancelledBooking=true.
Applied to files:
packages/features/bookings/lib/handleNewBooking/test/reservation-logic.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-validation.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-system.test.ts
📚 Learning: 2025-08-27T13:32:46.887Z
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
Applied to files:
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-validation.test.tspackages/features/bookings/lib/handleNewBooking/test/reservation-system.test.ts
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.{ts,tsx} : Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js `.utc()` in hot paths like loops
Applied to files:
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
📚 Learning: 2025-08-21T13:44:06.805Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.805Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
Applied to files:
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
📚 Learning: 2025-09-18T08:50:49.906Z
Learnt from: eunjae-lee
PR: calcom/cal.com#23752
File: packages/lib/server/service/InsightsBookingBaseService.ts:340-371
Timestamp: 2025-09-18T08:50:49.906Z
Learning: In the InsightsBookingBaseService (packages/lib/server/service/InsightsBookingBaseService.ts), the startTime date range filter intentionally uses different fields for the lower and upper bounds: `startDate <= "startTime" AND "endTime" <= endDate`. This ensures bookings start within the selected range but don't extend beyond the end date, which is the intended business logic and not an inconsistency.
Applied to files:
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
🧬 Code graph analysis (1)
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts (3)
packages/features/bookings/lib/handleNewBooking/test/getNewBookingHandler.ts (1)
getNewBookingHandler(6-8)apps/web/test/utils/bookingScenario/bookingScenario.ts (5)
getBooker(2220-2234)getOrganizer(1520-1576)TestData(1239-1511)createBookingScenario(978-1009)getDate(1093-1140)apps/web/test/utils/bookingScenario/expects.ts (1)
expectBookingToBeInDatabase(418-435)
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (7)
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts (3)
52-78: Test setup has incorrect API usage and missing time specificationsMultiple issues in the test setup:
createBookingScenariodoesn't supportreservedSlotsparameter - should useselectedSlots- The function returns
{ eventTypes, workflows }not{ eventType, reservedSlotUid }- Invalid dayjs usage:
dayjs(getDate(...))passes an object instead of a date string- Missing explicit start/end times in the booking request
- const { eventType, reservedSlotUid } = await createBookingScenario({ + const { eventTypes } = await createBookingScenario({ eventTypes: [ { id: 1, slotInterval: 15, length: 15, users: [ { id: 101, }, ], }, ], users: [organizer], // Create a reserved slot for the test - reservedSlots: [ + selectedSlots: [ { uid: "reserved-slot-123", eventTypeId: 1, userId: 101, - slotUtcStartDate: dayjs(getDate({ dateIncrement: 1 })).utc().toDate(), - slotUtcEndDate: dayjs(getDate({ dateIncrement: 1 })).utc().add(15, "minutes").toDate(), + slotUtcStartDate: dayjs(`${getDate({ dateIncrement: 1 }).dateString}T10:00:00Z`).toDate(), + slotUtcEndDate: dayjs(`${getDate({ dateIncrement: 1 }).dateString}T10:15:00Z`).toDate(), - releaseAt: dayjs().add(15, "minutes").toDate(), + releaseAt: dayjs(`${getDate({ dateIncrement: 1 }).dateString}T10:00:00Z`).add(15, "minutes").toDate(), isSeat: false, }, ], }); const mockRequestData = getMockRequestDataForBooking({ data: { eventTypeId: 1, + start: `${getDate({ dateIncrement: 1 }).dateString}T10:00:00Z`, + end: `${getDate({ dateIncrement: 1 }).dateString}T10:15:00Z`, responses: { email: booker.email, name: booker.name, location: { optionValue: "", value: "integrations:daily" }, }, reservedSlotUid: "reserved-slot-123", }, });Also applies to: 133-147, 186-200, 254-268, 319-333
207-209: Invalid dayjs usage for date constructionSimilar to the previous issue,
dayjs(getDate(...))is incorrect asgetDate()returns an object with{ date, month, year, dateString }properties, not a Date object.- slotUtcStartDate: dayjs(getDate({ dateIncrement: 1 })).utc().toDate(), - slotUtcEndDate: dayjs(getDate({ dateIncrement: 1 })).utc().add(15, "minutes").toDate(), + slotUtcStartDate: dayjs(`${getDate({ dateIncrement: 1 }).dateString}T10:00:00Z`).toDate(), + slotUtcEndDate: dayjs(`${getDate({ dateIncrement: 1 }).dateString}T10:15:00Z`).toDate(),Also applies to: 275-276, 286-287
110-114: Incorrect assertion - wrong field referenceThe test uses
eventType.idon Line 112 buteventTypeis not defined. Based on the corrected factory return value, this should beeventTypes[0].id.await expectBookingToBeInDatabase({ uid: createdBooking.uid!, - eventTypeId: eventType.id, + eventTypeId: 1, // Or eventTypes[0].id if available status: BookingStatus.ACCEPTED, });Also applies to: 359-362
packages/features/bookings/lib/handleNewBooking/test/reservation-system.test.ts (2)
1-200: Duplicate test file with nearly identical contentThis entire test file is a near-duplicate of
reservation-validation.test.tswith only minor differences in test names and descriptions. The test logic, mocked functions, and assertions are identical.This duplication reduces maintainability and increases the risk of tests diverging over time. Please consolidate these test files.
19-19: Non-deterministic time calculationsUsing
Date.now()makes tests non-deterministic and potentially flaky.- expirationTime: new Date(Date.now() + 5 * 60 * 1000), // 5 minutes from now + expirationTime: new Date("2024-01-01T10:05:00Z"), // Fixed future time for deterministic testsAlso applies to: 96-96
packages/features/bookings/lib/handleNewBooking/test/reservation-logic.test.ts (1)
1-166: Third duplicate test file with identical contentThis is the third file with essentially the same test content as
reservation-validation.test.tsandreservation-system.test.ts. The only difference is usingitinstead oftestfor test definitions.Having three files testing the same functionality with duplicated code significantly impacts maintainability. Please consolidate all reservation tests into a single, well-organized test file.
packages/features/bookings/lib/handleNewBooking/test/reservation-validation.test.ts (1)
51-71: Inline validation logic should match actual implementationThe test duplicates the reservation validation logic inline rather than testing against the actual implementation. This creates a risk of tests passing even if the production code behavior diverges.
Consider extracting the validation logic to a testable module or mocking at a higher level to test the actual implementation.
packages/features/bookings/lib/handleNewBooking/test/reservation-handling.test.ts
Outdated
Show resolved
Hide resolved
packages/features/bookings/lib/handleNewBooking/test/reservation-validation.test.ts
Outdated
Show resolved
Hide resolved
packages/features/bookings/lib/handleNewBooking/test/reservation-validation.test.ts
Outdated
Show resolved
Hide resolved
|
This PR is being marked as stale due to inactivity. |
hariombalhara
left a comment
hariombalhara
left a comment
There was a problem hiding this comment.
Why are there binary files in this PR ?
|
Closing due to binary files in the PR and staleness |
3 participants
What does this PR do?
This PR fixes a critical S1 issue where the booking system allowed creating bookings on reserved slots without proper validation, leading to possible double-bookings. This was escalated by Udemy and affects enterprise clients.
Before fix:
After Fix:
User A: Creates reservation for 2:00 PM slot
User B: Calls handleNewBooking with reservedSlotUid → Atomic transaction:
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist