fix: Prevent double bookings with recurring booking links#22803
fix: Prevent double bookings with recurring booking links#22803naaa760 wants to merge 3 commits intocalcom:mainfrom naaa760:fix/prevent-duplicate-recurring-bookings
Conversation
…base transaction Added imports for HttpError and ErrorCode
Added imports for HttpError, ErrorCode, prisma, and BookingStatus
WalkthroughThe changes introduce explicit overlap checks for both single and recurring bookings. In Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ 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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@naaa760 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
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/30/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/30/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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/features/bookings/lib/handleNewBooking/createBooking.ts(3 hunks)packages/features/bookings/lib/handleNewRecurringBooking.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/handleNewRecurringBooking.tspackages/features/bookings/lib/handleNewBooking/createBooking.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/handleNewRecurringBooking.tspackages/features/bookings/lib/handleNewBooking/createBooking.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CarinaWolli
PR: calcom/cal.com#22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/prisma/schema.prisma:1068-1071
Timestamp: 2025-07-16T05:10:22.891Z
Learning: In PR #22304 for Cal.com private link expiration features, the `maxUsageCount` field was intentionally set to default to 1 (non-nullable) as a breaking change, making all existing private links single-use after migration. This was a deliberate design decision by alishaz-polymath.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
Learnt from: eunjae-lee
PR: calcom/cal.com#22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
packages/features/bookings/lib/handleNewRecurringBooking.ts (3)
Learnt from: CarinaWolli
PR: #22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Learnt from: vijayraghav-io
PR: #21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: vijayraghav-io
PR: #21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The upsertSelectedCalendarsForEventTypeIds method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like @@unique([outlookSubscriptionId, eventTypeId]) should not be applied as they would prevent this intended functionality.
packages/features/bookings/lib/handleNewBooking/createBooking.ts (5)
Learnt from: eunjae-lee
PR: #22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Learnt from: CarinaWolli
PR: #22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
Learnt from: vijayraghav-io
PR: #21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
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
Learnt from: eunjae-lee
PR: #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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (4)
packages/features/bookings/lib/handleNewBooking/createBooking.ts (2)
9-10: LGTM!The new imports for
HttpErrorandErrorCodeare correctly added and necessary for the overlap checking functionality.
77-138: LGTM! Comprehensive overlap detection with proper error handling.The function correctly implements all four overlap scenarios:
- New booking starts during existing booking
- New booking ends during existing booking
- New booking completely contains existing booking
- New booking is completely contained within existing booking
The logic properly excludes rescheduled bookings and uses appropriate Prisma
selectto fetch only required fields, following the coding guidelines.packages/features/bookings/lib/handleNewRecurringBooking.ts (2)
5-8: LGTM!The new imports are consistent with the single booking implementation and necessary for the overlap checking functionality.
124-129: LGTM! Proper integration of overlap checking.The overlap check is correctly placed before processing any recurring bookings, ensuring conflicts are detected early. Using
firstBookingdata foreventTypeIdandrescheduleUidis appropriate since all recurring bookings share the same event type and reschedule context.
| // Check for overlapping bookings before creating the new booking | ||
| await checkForOverlappingBookings({ | ||
| eventTypeId: newBookingData.eventTypeId!, | ||
| startTime: newBookingData.startTime, | ||
| endTime: newBookingData.endTime, | ||
| rescheduleUid: originalRescheduledBooking?.uid, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Proper integration into the transaction flow.
The overlap check is correctly placed at the beginning of the transaction before any database modifications. The parameters are properly extracted from the booking data.
Verify that eventTypeId is always non-null at this point to ensure the non-null assertion is safe:
🏁 Script executed:
#!/bin/bash
# Description: Check if eventTypeId can be null in buildNewBookingData function
# Expected: eventTypeId should always be set when this function is called
ast-grep --pattern $'function buildNewBookingData($$$) {
$$$
eventType: eventTypeRel,
$$$
}'Length of output: 99
🏁 Script executed:
#!/bin/bash
set -e
echo "🔍 Locating newBookingData definition in createBooking.ts"
rg -n -C5 "newBookingData" packages/features/bookings/lib/handleNewBooking/createBooking.ts
echo ""
echo "🔍 Searching for the NewBookingData type/interface declaration in the repo"
rg -n -C3 "NewBookingData" -g "*.ts"Length of output: 6345
🏁 Script executed:
#!/bin/bash
# Extract buildNewBookingData including eventTypeId assignments
sed -n '280,360p' packages/features/bookings/lib/handleNewBooking/createBooking.tsLength of output: 2792
Ensure the correct eventTypeId is used for overlap checking
The current call uses newBookingData.eventTypeId!, but buildNewBookingData never assigns an eventTypeId property—so it will always be undefined at runtime. Please update the overlap check to use the actual eventType.id from the function parameters (or explicitly add eventTypeId to newBookingData).
Suggested change:
--- a/packages/features/bookings/lib/handleNewBooking/createBooking.ts
+++ b/packages/features/bookings/lib/handleNewBooking/createBooking.ts
@@ 241,247c241,245
- await checkForOverlappingBookings({
- eventTypeId: newBookingData.eventTypeId!,
+ await checkForOverlappingBookings({
+ eventTypeId: eventType.id,
startTime: newBookingData.startTime,
endTime: newBookingData.endTime,
rescheduleUid: originalRescheduledBooking?.uid,• File: packages/features/bookings/lib/handleNewBooking/createBooking.ts
• Lines: 241–247
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check for overlapping bookings before creating the new booking | |
| await checkForOverlappingBookings({ | |
| eventTypeId: newBookingData.eventTypeId!, | |
| startTime: newBookingData.startTime, | |
| endTime: newBookingData.endTime, | |
| rescheduleUid: originalRescheduledBooking?.uid, | |
| }); | |
| // Check for overlapping bookings before creating the new booking | |
| await checkForOverlappingBookings({ | |
| eventTypeId: eventType.id, | |
| startTime: newBookingData.startTime, | |
| endTime: newBookingData.endTime, | |
| rescheduleUid: originalRescheduledBooking?.uid, | |
| }); |
🤖 Prompt for AI Agents
In packages/features/bookings/lib/handleNewBooking/createBooking.ts around lines
241 to 247, the overlap check uses newBookingData.eventTypeId which is never set
and thus undefined. Fix this by replacing newBookingData.eventTypeId with the
correct eventType.id parameter value when calling checkForOverlappingBookings,
ensuring the actual event type ID is used for the overlap check.
| /** | ||
| * Check for overlapping bookings across all recurring dates | ||
| */ | ||
| async function checkForOverlappingRecurringBookings({ | ||
| eventTypeId, | ||
| recurringDates, | ||
| rescheduleUid, | ||
| }: { | ||
| eventTypeId: number; | ||
| recurringDates: { start: string | undefined; end: string | undefined }[]; | ||
| rescheduleUid?: string; | ||
| }) { | ||
| for (const date of recurringDates) { | ||
| if (!date.start || !date.end) continue; | ||
|
|
||
| const startTime = new Date(date.start); | ||
| const endTime = new Date(date.end); | ||
|
|
||
| const overlappingBookings = await prisma.booking.findFirst({ | ||
| where: { | ||
| eventTypeId, | ||
| status: { | ||
| in: [BookingStatus.ACCEPTED, BookingStatus.PENDING], | ||
| }, | ||
| // Check for overlapping time ranges | ||
| OR: [ | ||
| // New booking starts during an existing booking | ||
| { | ||
| startTime: { lte: startTime }, | ||
| endTime: { gt: startTime }, | ||
| }, | ||
| // New booking ends during an existing booking | ||
| { | ||
| startTime: { lt: endTime }, | ||
| endTime: { gte: endTime }, | ||
| }, | ||
| // New booking completely contains an existing booking | ||
| { | ||
| startTime: { gte: startTime }, | ||
| endTime: { lte: endTime }, | ||
| }, | ||
| // New booking is completely contained within an existing booking | ||
| { | ||
| startTime: { lte: startTime }, | ||
| endTime: { gte: endTime }, | ||
| }, | ||
| ], | ||
| // Exclude the booking being rescheduled | ||
| ...(rescheduleUid && { uid: { not: rescheduleUid } }), | ||
| }, | ||
| select: { | ||
| id: true, | ||
| uid: true, | ||
| startTime: true, | ||
| endTime: true, | ||
| status: true, | ||
| }, | ||
| }); | ||
|
|
||
| if (overlappingBookings) { | ||
| throw new HttpError({ | ||
| statusCode: 409, | ||
| message: ErrorCode.BookingConflict, | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize database queries for better performance.
While the logic is correct, the current implementation makes a separate database query for each recurring date, which could be inefficient for bookings with many recurrences.
Consider optimizing with a single query that checks all dates at once:
-async function checkForOverlappingRecurringBookings({
- eventTypeId,
- recurringDates,
- rescheduleUid,
-}: {
- eventTypeId: number;
- recurringDates: { start: string | undefined; end: string | undefined }[];
- rescheduleUid?: string;
-}) {
- for (const date of recurringDates) {
- if (!date.start || !date.end) continue;
-
- const startTime = new Date(date.start);
- const endTime = new Date(date.end);
-
- const overlappingBookings = await prisma.booking.findFirst({
- where: {
- eventTypeId,
- status: {
- in: [BookingStatus.ACCEPTED, BookingStatus.PENDING],
- },
- // Check for overlapping time ranges
- OR: [
- // ... overlap conditions
- ],
- // Exclude the booking being rescheduled
- ...(rescheduleUid && { uid: { not: rescheduleUid } }),
- },
- select: {
- id: true,
- uid: true,
- startTime: true,
- endTime: true,
- status: true,
- },
- });
-
- if (overlappingBookings) {
- throw new HttpError({
- statusCode: 409,
- message: ErrorCode.BookingConflict,
- });
- }
- }
-}
+async function checkForOverlappingRecurringBookings({
+ eventTypeId,
+ recurringDates,
+ rescheduleUid,
+}: {
+ eventTypeId: number;
+ recurringDates: { start: string | undefined; end: string | undefined }[];
+ rescheduleUid?: string;
+}) {
+ const validDates = recurringDates
+ .filter(date => date.start && date.end)
+ .map(date => ({
+ startTime: new Date(date.start!),
+ endTime: new Date(date.end!)
+ }));
+
+ if (validDates.length === 0) return;
+
+ const dateConditions = validDates.flatMap(({ startTime, endTime }) => [
+ { startTime: { lte: startTime }, endTime: { gt: startTime } },
+ { startTime: { lt: endTime }, endTime: { gte: endTime } },
+ { startTime: { gte: startTime }, endTime: { lte: endTime } },
+ { startTime: { lte: startTime }, endTime: { gte: endTime } },
+ ]);
+
+ const overlappingBookings = await prisma.booking.findFirst({
+ where: {
+ eventTypeId,
+ status: {
+ in: [BookingStatus.ACCEPTED, BookingStatus.PENDING],
+ },
+ OR: dateConditions,
+ ...(rescheduleUid && { uid: { not: rescheduleUid } }),
+ },
+ select: {
+ id: true,
+ uid: true,
+ startTime: true,
+ endTime: true,
+ status: true,
+ },
+ });
+
+ if (overlappingBookings) {
+ throw new HttpError({
+ statusCode: 409,
+ message: ErrorCode.BookingConflict,
+ });
+ }
+}🤖 Prompt for AI Agents
In packages/features/bookings/lib/handleNewRecurringBooking.ts between lines 28
and 94, the function currently performs a separate database query for each
recurring date to check for overlapping bookings, which is inefficient. Refactor
the code to consolidate all recurring date checks into a single Prisma query by
constructing a combined OR condition that includes all date ranges at once. This
will reduce the number of database calls and improve performance for bookings
with many recurrences.
Devanshusharma2005
left a comment
Devanshusharma2005
left a comment
There was a problem hiding this comment.
@naaa760 can you please address the type-checks and unit tests failing.
Please add a loom also to show the changes.
Marking it draft until then .
Feel free to rfr.
2 participants
What does this PR do?
#CLOSES: #22801
Visual Demo
Before the Fix:
After the Fix:
How should this be tested?
Environment Setup:
Test Steps:
Test Regular Booking Conflict:
Test Recurring Booking Conflict:
Test Race Condition:
Expected Behavior:
Files Modified
packages/features/bookings/lib/handleNewBooking/createBooking.tspackages/features/bookings/lib/handleNewRecurringBooking.ts