Fix/booking repository booker#29790
Open
Ashid332 wants to merge 4 commits into
Open
Conversation
- Replaces flawed containment logic with strict interval intersection math - Centralizes Prisma where-clause generation into prismaIntervalOverlap.ts - Migrates getBusyTimes, BookingRepository, and HolidayRepository to new utility - Adds comprehensive behavioral and unit test coverage for overlapping scenarios
fix: resolve busy time overlap logic and centralize interval queries
Contributor
|
Welcome to Cal.diy, @Ashid332! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
Contributor
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a shared Prisma helper for strict and inclusive interval-overlap filters with validation tests. Booking, busy-time, and holiday queries now use the helper, with new booking-limit tests covering overlap and boundary cases. The booking lookup also explicitly handles missing records and derives the booker from the first ordered attendee. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/features/busyTimes/services/getBusyTimes.test.ts`:
- Around line 462-474: Update the test setup around createMockBookingResult and
the getBusyTimesForLimitChecks request to replace dayjs date construction and
formatting with native Date parsing and Date.toISOString(). Preserve the
existing start and end boundary values while emitting standard UTC strings for
startDate and endDate.
- Around line 402-422: Update the requestedStart/requestedEnd test setup and
setupPrismaMock to use native Date comparisons instead of dayjs, including
Date-based UTC bounds and interval checks. Replace existingBookings: any[] with
the appropriate booking type, and type the mocked Prisma findMany arguments
using its inferred or generated parameter type rather than casting args?.where
as any.
In `@packages/lib/prismaIntervalOverlap.ts`:
- Around line 1-3: Remove the redundant type-description comment in
packages/lib/prismaIntervalOverlap.ts lines 1-3, the assertion-instruction
comment in packages/lib/prismaIntervalOverlap.test.ts lines 13-14, and the
assertion-narration comment in packages/lib/prismaIntervalOverlap.test.ts lines
59-64; leave the surrounding implementation and assertions unchanged.
- Around line 31-40: Update the validation failures in prismaIntervalOverlap to
throw ErrorWithCode instead of plain Error, preserving the existing messages for
invalid start dates, invalid end dates, and non-chronological intervals while
assigning stable error codes according to the project’s conventions.
- Around line 4-6: The getOverlappingIntervalWhereClause API currently exposes a
broad union regardless of inclusive. Add boolean-specific overloads or an
equivalent conditional return type so inclusive=true returns the strict lt/gt
clause and inclusive=false or the default returns the lte/gte clause, allowing
callers to access the exact properties safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4cacf23-f46a-4ee7-8d4a-0dfb6cf61ef1
📒 Files selected for processing (6)
packages/features/bookings/repositories/BookingRepository.tspackages/features/busyTimes/services/getBusyTimes.test.tspackages/features/busyTimes/services/getBusyTimes.tspackages/features/holidays/repositories/HolidayRepository.tspackages/lib/prismaIntervalOverlap.test.tspackages/lib/prismaIntervalOverlap.ts
Comment on lines
+402
to
+422
| const requestedStart = dayjs("2026-07-15T10:00:00Z"); | ||
| const requestedEnd = dayjs("2026-07-15T11:00:00Z"); | ||
|
|
||
| const setupPrismaMock = (existingBookings: any[]) => { | ||
| vi.mocked(prisma.booking.findMany).mockImplementation(async (args) => { | ||
| const where = args?.where as any; | ||
| const startTimeCondition = where?.startTime; | ||
| const endTimeCondition = where?.endTime; | ||
|
|
||
| return existingBookings.filter((booking) => { | ||
| // Emulate Prisma's lt/gt logic | ||
| const startsBeforeEnd = startTimeCondition?.lt | ||
| ? dayjs(booking.startTime).isBefore(dayjs(startTimeCondition.lt)) | ||
| : true; | ||
| const endsAfterStart = endTimeCondition?.gt | ||
| ? dayjs(booking.endTime).isAfter(dayjs(endTimeCondition.gt)) | ||
| : true; | ||
| return startsBeforeEnd && endsAfterStart; | ||
| }); | ||
| }); | ||
| }; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace dayjs with native Date and eliminate any types.
As per coding guidelines:
- "Use
date-fnsor nativeDateinstead of Day.js when timezone awareness isn't needed" - "Never use
as any- use proper type-safe solutions instead"
Timezone awareness is not needed here as we are dealing with straight UTC time bounds and primitive interval math. Additionally, avoid any[] for the mock list and as any for the Prisma arguments.
♻️ Proposed refactor for native dates and type safety
- const requestedStart = dayjs("2026-07-15T10:00:00Z");
- const requestedEnd = dayjs("2026-07-15T11:00:00Z");
+ const requestedStart = new Date("2026-07-15T10:00:00Z");
+ const requestedEnd = new Date("2026-07-15T11:00:00Z");
- const setupPrismaMock = (existingBookings: any[]) => {
+ const setupPrismaMock = (existingBookings: ReturnType<typeof createMockBookingResult>[]) => {
vi.mocked(prisma.booking.findMany).mockImplementation(async (args) => {
- const where = args?.where as any;
+ const where = args?.where as { startTime?: { lt?: Date }; endTime?: { gt?: Date } } | undefined;
const startTimeCondition = where?.startTime;
const endTimeCondition = where?.endTime;
return existingBookings.filter((booking) => {
// Emulate Prisma's lt/gt logic
const startsBeforeEnd = startTimeCondition?.lt
- ? dayjs(booking.startTime).isBefore(dayjs(startTimeCondition.lt))
+ ? booking.startTime.getTime() < startTimeCondition.lt.getTime()
: true;
const endsAfterStart = endTimeCondition?.gt
- ? dayjs(booking.endTime).isAfter(dayjs(endTimeCondition.gt))
+ ? booking.endTime.getTime() > endTimeCondition.gt.getTime()
: true;
return startsBeforeEnd && endsAfterStart;
});
});
};📝 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.
Suggested change
| const requestedStart = dayjs("2026-07-15T10:00:00Z"); | |
| const requestedEnd = dayjs("2026-07-15T11:00:00Z"); | |
| const setupPrismaMock = (existingBookings: any[]) => { | |
| vi.mocked(prisma.booking.findMany).mockImplementation(async (args) => { | |
| const where = args?.where as any; | |
| const startTimeCondition = where?.startTime; | |
| const endTimeCondition = where?.endTime; | |
| return existingBookings.filter((booking) => { | |
| // Emulate Prisma's lt/gt logic | |
| const startsBeforeEnd = startTimeCondition?.lt | |
| ? dayjs(booking.startTime).isBefore(dayjs(startTimeCondition.lt)) | |
| : true; | |
| const endsAfterStart = endTimeCondition?.gt | |
| ? dayjs(booking.endTime).isAfter(dayjs(endTimeCondition.gt)) | |
| : true; | |
| return startsBeforeEnd && endsAfterStart; | |
| }); | |
| }); | |
| }; | |
| const requestedStart = new Date("2026-07-15T10:00:00Z"); | |
| const requestedEnd = new Date("2026-07-15T11:00:00Z"); | |
| const setupPrismaMock = (existingBookings: ReturnType<typeof createMockBookingResult>[]) => { | |
| vi.mocked(prisma.booking.findMany).mockImplementation(async (args) => { | |
| const where = args?.where as { startTime?: { lt?: Date }; endTime?: { gt?: Date } } | undefined; | |
| const startTimeCondition = where?.startTime; | |
| const endTimeCondition = where?.endTime; | |
| return existingBookings.filter((booking) => { | |
| // Emulate Prisma's lt/gt logic | |
| const startsBeforeEnd = startTimeCondition?.lt | |
| ? booking.startTime.getTime() < startTimeCondition.lt.getTime() | |
| : true; | |
| const endsAfterStart = endTimeCondition?.gt | |
| ? booking.endTime.getTime() > endTimeCondition.gt.getTime() | |
| : true; | |
| return startsBeforeEnd && endsAfterStart; | |
| }); | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/features/busyTimes/services/getBusyTimes.test.ts` around lines 402 -
422, Update the requestedStart/requestedEnd test setup and setupPrismaMock to
use native Date comparisons instead of dayjs, including Date-based UTC bounds
and interval checks. Replace existingBookings: any[] with the appropriate
booking type, and type the mocked Prisma findMany arguments using its inferred
or generated parameter type rather than casting args?.where as any.
Source: Coding guidelines
Comment on lines
+462
to
+474
| const mockBooking = createMockBookingResult({ | ||
| startTime: dayjs(existingStart).toDate(), | ||
| endTime: dayjs(existingEnd).toDate(), | ||
| }); | ||
| setupPrismaMock([mockBooking]); | ||
|
|
||
| const busyTimesService = getBusyTimesService(); | ||
| const busyTimes = await busyTimesService.getBusyTimesForLimitChecks({ | ||
| userIds: [1], | ||
| eventTypeId: 1, | ||
| startDate: requestedStart.format(), | ||
| endDate: requestedEnd.format(), | ||
| // PER_YEAR is explicitly excluded from date range expansion in getStartEndDateforLimitCheck |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use native Date construction and .toISOString().
Continuing the removal of dayjs as per the coding guidelines, use native Date parsing and rely on .toISOString() to emit standard UTC strings for the service boundaries.
♻️ Proposed refactor
- const mockBooking = createMockBookingResult({
- startTime: dayjs(existingStart).toDate(),
- endTime: dayjs(existingEnd).toDate(),
- });
+ const mockBooking = createMockBookingResult({
+ startTime: new Date(existingStart),
+ endTime: new Date(existingEnd),
+ });
setupPrismaMock([mockBooking]);
const busyTimesService = getBusyTimesService();
const busyTimes = await busyTimesService.getBusyTimesForLimitChecks({
userIds: [1],
eventTypeId: 1,
- startDate: requestedStart.format(),
- endDate: requestedEnd.format(),
+ startDate: requestedStart.toISOString(),
+ endDate: requestedEnd.toISOString(),
// PER_YEAR is explicitly excluded from date range expansion in getStartEndDateforLimitCheck📝 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.
Suggested change
| const mockBooking = createMockBookingResult({ | |
| startTime: dayjs(existingStart).toDate(), | |
| endTime: dayjs(existingEnd).toDate(), | |
| }); | |
| setupPrismaMock([mockBooking]); | |
| const busyTimesService = getBusyTimesService(); | |
| const busyTimes = await busyTimesService.getBusyTimesForLimitChecks({ | |
| userIds: [1], | |
| eventTypeId: 1, | |
| startDate: requestedStart.format(), | |
| endDate: requestedEnd.format(), | |
| // PER_YEAR is explicitly excluded from date range expansion in getStartEndDateforLimitCheck | |
| const mockBooking = createMockBookingResult({ | |
| startTime: new Date(existingStart), | |
| endTime: new Date(existingEnd), | |
| }); | |
| setupPrismaMock([mockBooking]); | |
| const busyTimesService = getBusyTimesService(); | |
| const busyTimes = await busyTimesService.getBusyTimesForLimitChecks({ | |
| userIds: [1], | |
| eventTypeId: 1, | |
| startDate: requestedStart.toISOString(), | |
| endDate: requestedEnd.toISOString(), | |
| // PER_YEAR is explicitly excluded from date range expansion in getStartEndDateforLimitCheck |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/features/busyTimes/services/getBusyTimes.test.ts` around lines 462 -
474, Update the test setup around createMockBookingResult and the
getBusyTimesForLimitChecks request to replace dayjs date construction and
formatting with native Date parsing and Date.toISOString(). Preserve the
existing start and end boundary values while emitting standard UTC strings for
startDate and endDate.
Source: Coding guidelines
Comment on lines
+1
to
+3
| /** | ||
| * Represents the Prisma `where` clause used to filter overlapping interval boundaries. | ||
| */ |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove comments that only restate adjacent code.
packages/lib/prismaIntervalOverlap.ts#L1-L3: remove the redundant type description.packages/lib/prismaIntervalOverlap.test.ts#L13-L14: remove the assertion instructions.packages/lib/prismaIntervalOverlap.test.ts#L59-L64: remove the assertion narration.
As per coding guidelines, comments must explain why, not what.
📍 Affects 2 files
packages/lib/prismaIntervalOverlap.ts#L1-L3(this comment)packages/lib/prismaIntervalOverlap.test.ts#L13-L14packages/lib/prismaIntervalOverlap.test.ts#L59-L64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/lib/prismaIntervalOverlap.ts` around lines 1 - 3, Remove the
redundant type-description comment in packages/lib/prismaIntervalOverlap.ts
lines 1-3, the assertion-instruction comment in
packages/lib/prismaIntervalOverlap.test.ts lines 13-14, and the
assertion-narration comment in packages/lib/prismaIntervalOverlap.test.ts lines
59-64; leave the surrounding implementation and assertions unchanged.
Source: Coding guidelines
Comment on lines
+4
to
+6
| export type OverlappingIntervalWhereClause = | ||
| | { startTime: { lte: Date }; endTime: { gte: Date } } | ||
| | { startTime: { lt: Date }; endTime: { gt: Date } }; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline packages/lib/prismaIntervalOverlap.ts --view expanded
rg -n -C3 '\.(lt|gt|lte|gte)\b' packages/lib/prismaIntervalOverlap.test.ts
fd -a package.json --exec sh -c 'jq -r ".scripts | to_entries[]? | select(.key | test(\"type|check\"))" "$1"' _ {}Repository: calcom/cal.diy
Length of output: 11632
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== packages/lib/prismaIntervalOverlap.ts ==\n'
cat -n packages/lib/prismaIntervalOverlap.ts
printf '\n== packages/lib/prismaIntervalOverlap.test.ts ==\n'
cat -n packages/lib/prismaIntervalOverlap.test.tsRepository: calcom/cal.diy
Length of output: 6544
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' packages/lib/prismaIntervalOverlap.ts | cat -n
printf '\n---\n'
sed -n '1,220p' packages/lib/prismaIntervalOverlap.test.ts | cat -nRepository: calcom/cal.diy
Length of output: 6454
Narrow the return type by inclusive in packages/lib/prismaIntervalOverlap.ts
getOverlappingIntervalWhereClause(start, end) still exposes the full union, so the default path can't safely access .lt/.gt. Add true/false overloads (or a conditional return type) so strict and inclusive calls get the exact clause shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/lib/prismaIntervalOverlap.ts` around lines 4 - 6, The
getOverlappingIntervalWhereClause API currently exposes a broad union regardless
of inclusive. Add boolean-specific overloads or an equivalent conditional return
type so inclusive=true returns the strict lt/gt clause and inclusive=false or
the default returns the lte/gte clause, allowing callers to access the exact
properties safely.
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Visual Demo (For contributors especially)
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