Refactor/participant display name#29788
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
|
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! |
|
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: |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds shared helpers for participant display-name resolution and Prisma interval-overlap predicates. The display-name helper is applied to calendar events, notifications, CRM payloads, and booking audit actors. The overlap helper supports strict and inclusive comparisons with input validation, and is used by booking, busy-time, and holiday queries. Tests cover fallback names, predicate shapes, validation, reference handling, and booking-limit overlap scenarios. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Line 407: Replace the as any cast in the test mock’s where extraction with an
inline type describing the expected Prisma where-clause structure, preserving
type-safe access to args?.where without weakening types. Keep the surrounding
mock behavior unchanged.
- Around line 402-478: Replace Day.js usage in the busy-times overlap tests with
native Date operations: initialize requestedStart/requestedEnd and mock booking
timestamps using Date, compare booking and query boundaries via Date timestamps
in setupPrismaMock, and preserve the existing UTC interval and expected overlap
results without introducing timezone-specific logic.
In `@packages/lib/prismaIntervalOverlap.ts`:
- Around line 31-41: Update the validation failures in the interval validation
block to import and use ErrorWithCode.Factory.BadRequest from `@calcom/lib/errors`
instead of plain Error, covering invalid start dates, invalid end dates, and
non-increasing intervals while preserving the existing messages.
🪄 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: 2d09e595-cffa-4db2-9293-d2d2da471dd5
📒 Files selected for processing (15)
packages/app-store/hubspot/lib/CrmService.tspackages/app-store/pipedrive-crm/lib/CrmService.tspackages/features/booking-audit/lib/service/ActorStrategies.tspackages/features/bookings/lib/handleCancelBooking.tspackages/features/bookings/lib/tasker/BookingEmailAndSmsTaskService.tspackages/features/bookings/repositories/BookingRepository.tspackages/features/busyTimes/services/getBusyTimes.test.tspackages/features/busyTimes/services/getBusyTimes.tspackages/features/holidays/repositories/HolidayRepository.tspackages/lib/buildCalEventFromBooking.tspackages/lib/participant/getParticipantDisplayName.test.tspackages/lib/participant/getParticipantDisplayName.tspackages/lib/prismaIntervalOverlap.test.tspackages/lib/prismaIntervalOverlap.tspackages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
| 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; | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| it.each([ | ||
| { | ||
| scenario: "1. Existing booking starts before the requested interval and ends inside it.", | ||
| existingStart: "2026-07-15T09:30:00Z", | ||
| existingEnd: "2026-07-15T10:15:00Z", | ||
| expectedCount: 1, | ||
| }, | ||
| { | ||
| scenario: "2. Existing booking starts inside the requested interval and ends after it.", | ||
| existingStart: "2026-07-15T10:45:00Z", | ||
| existingEnd: "2026-07-15T12:00:00Z", | ||
| expectedCount: 1, | ||
| }, | ||
| { | ||
| scenario: "3. Existing booking completely surrounds the requested interval.", | ||
| existingStart: "2026-07-15T09:00:00Z", | ||
| existingEnd: "2026-07-15T12:00:00Z", | ||
| expectedCount: 1, | ||
| }, | ||
| { | ||
| scenario: "4. Existing booking is fully contained within the requested interval.", | ||
| existingStart: "2026-07-15T10:15:00Z", | ||
| existingEnd: "2026-07-15T10:45:00Z", | ||
| expectedCount: 1, | ||
| }, | ||
| { | ||
| scenario: "5. Existing booking ends exactly when the requested interval starts.", | ||
| existingStart: "2026-07-15T09:00:00Z", | ||
| existingEnd: "2026-07-15T10:00:00Z", | ||
| expectedCount: 0, | ||
| }, | ||
| { | ||
| scenario: "6. Existing booking starts exactly when the requested interval ends.", | ||
| existingStart: "2026-07-15T11:00:00Z", | ||
| existingEnd: "2026-07-15T12:00:00Z", | ||
| expectedCount: 0, | ||
| }, | ||
| ])("$scenario", async ({ existingStart, existingEnd, expectedCount }) => { | ||
| 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 | ||
| // This allows us to test the exact requested interval [10:00 - 11:00] | ||
| bookingLimits: { PER_YEAR: 5 }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use native Date instead of Day.js.
The timestamp comparisons and ISO string formatting in this test only use UTC boundaries and don't require external timezone logic, meaning they can easily be handled by the native Date API without the overhead of dayjs.
As per coding guidelines, use date-fns or native Date instead of Day.js when timezone awareness isn't needed.
♻️ Proposed refactor
- 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[]) => {
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;
+ // Emulate Prisma's lt/gt logic
+ const startsBeforeEnd = startTimeCondition?.lt
+ ? new Date(booking.startTime).getTime() < new Date(startTimeCondition.lt).getTime()
+ : true;
+ const endsAfterStart = endTimeCondition?.gt
+ ? new Date(booking.endTime).getTime() > new Date(endTimeCondition.gt).getTime()
+ : true;
return startsBeforeEnd && endsAfterStart;
});
});
};
// ... (unchanged array blocks)
])("$scenario", async ({ existingStart, existingEnd, expectedCount }) => {
const mockBooking = createMockBookingResult({
- startTime: dayjs(existingStart).toDate(),
- endTime: dayjs(existingEnd).toDate(),
+ 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
// This allows us to test the exact requested interval [10:00 - 11:00]
bookingLimits: { PER_YEAR: 5 },
});📝 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.
| 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; | |
| }); | |
| }); | |
| }; | |
| it.each([ | |
| { | |
| scenario: "1. Existing booking starts before the requested interval and ends inside it.", | |
| existingStart: "2026-07-15T09:30:00Z", | |
| existingEnd: "2026-07-15T10:15:00Z", | |
| expectedCount: 1, | |
| }, | |
| { | |
| scenario: "2. Existing booking starts inside the requested interval and ends after it.", | |
| existingStart: "2026-07-15T10:45:00Z", | |
| existingEnd: "2026-07-15T12:00:00Z", | |
| expectedCount: 1, | |
| }, | |
| { | |
| scenario: "3. Existing booking completely surrounds the requested interval.", | |
| existingStart: "2026-07-15T09:00:00Z", | |
| existingEnd: "2026-07-15T12:00:00Z", | |
| expectedCount: 1, | |
| }, | |
| { | |
| scenario: "4. Existing booking is fully contained within the requested interval.", | |
| existingStart: "2026-07-15T10:15:00Z", | |
| existingEnd: "2026-07-15T10:45:00Z", | |
| expectedCount: 1, | |
| }, | |
| { | |
| scenario: "5. Existing booking ends exactly when the requested interval starts.", | |
| existingStart: "2026-07-15T09:00:00Z", | |
| existingEnd: "2026-07-15T10:00:00Z", | |
| expectedCount: 0, | |
| }, | |
| { | |
| scenario: "6. Existing booking starts exactly when the requested interval ends.", | |
| existingStart: "2026-07-15T11:00:00Z", | |
| existingEnd: "2026-07-15T12:00:00Z", | |
| expectedCount: 0, | |
| }, | |
| ])("$scenario", async ({ existingStart, existingEnd, expectedCount }) => { | |
| 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 | |
| // This allows us to test the exact requested interval [10:00 - 11:00] | |
| bookingLimits: { PER_YEAR: 5 }, | |
| }); | |
| const requestedStart = new Date("2026-07-15T10:00:00Z"); | |
| const requestedEnd = new Date("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 | |
| ? new Date(booking.startTime).getTime() < new Date(startTimeCondition.lt).getTime() | |
| : true; | |
| const endsAfterStart = endTimeCondition?.gt | |
| ? new Date(booking.endTime).getTime() > new Date(endTimeCondition.gt).getTime() | |
| : true; | |
| return startsBeforeEnd && endsAfterStart; | |
| }); | |
| }); | |
| }; | |
| it.each([ | |
| { | |
| scenario: "1. Existing booking starts before the requested interval and ends inside it.", | |
| existingStart: "2026-07-15T09:30:00Z", | |
| existingEnd: "2026-07-15T10:15:00Z", | |
| expectedCount: 1, | |
| }, | |
| { | |
| scenario: "2. Existing booking starts inside the requested interval and ends after it.", | |
| existingStart: "2026-07-15T10:45:00Z", | |
| existingEnd: "2026-07-15T12:00:00Z", | |
| expectedCount: 1, | |
| }, | |
| { | |
| scenario: "3. Existing booking completely surrounds the requested interval.", | |
| existingStart: "2026-07-15T09:00:00Z", | |
| existingEnd: "2026-07-15T12:00:00Z", | |
| expectedCount: 1, | |
| }, | |
| { | |
| scenario: "4. Existing booking is fully contained within the requested interval.", | |
| existingStart: "2026-07-15T10:15:00Z", | |
| existingEnd: "2026-07-15T10:45:00Z", | |
| expectedCount: 1, | |
| }, | |
| { | |
| scenario: "5. Existing booking ends exactly when the requested interval starts.", | |
| existingStart: "2026-07-15T09:00:00Z", | |
| existingEnd: "2026-07-15T10:00:00Z", | |
| expectedCount: 0, | |
| }, | |
| { | |
| scenario: "6. Existing booking starts exactly when the requested interval ends.", | |
| existingStart: "2026-07-15T11:00:00Z", | |
| existingEnd: "2026-07-15T12:00:00Z", | |
| expectedCount: 0, | |
| }, | |
| ])("$scenario", async ({ existingStart, existingEnd, expectedCount }) => { | |
| 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 | |
| // This allows us to test the exact requested interval [10:00 - 11:00] | |
| bookingLimits: { PER_YEAR: 5 }, | |
| }); | |
🤖 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 -
478, Replace Day.js usage in the busy-times overlap tests with native Date
operations: initialize requestedStart/requestedEnd and mock booking timestamps
using Date, compare booking and query boundaries via Date timestamps in
setupPrismaMock, and preserve the existing UTC interval and expected overlap
results without introducing timezone-specific logic.
Source: Coding guidelines
|
|
||
| const setupPrismaMock = (existingBookings: any[]) => { | ||
| vi.mocked(prisma.booking.findMany).mockImplementation(async (args) => { | ||
| const where = args?.where as any; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove as any in favor of a type-safe interface.
For this test mock, you can safely cast the arguments to an inline type representing the expected Prisma where clause structure to maintain type safety.
As per coding guidelines, never use as any - use proper type-safe solutions instead.
🛡️ Proposed fix
- const where = args?.where as any;
+ const where = args?.where as { startTime?: { lt?: Date }; endTime?: { gt?: Date } } | undefined;📝 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.
| const where = args?.where as any; | |
| const where = args?.where as { startTime?: { lt?: Date }; endTime?: { gt?: Date } } | undefined; |
🤖 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` at line 407,
Replace the as any cast in the test mock’s where extraction with an inline type
describing the expected Prisma where-clause structure, preserving type-safe
access to args?.where without weakening types. Keep the surrounding mock
behavior unchanged.
Source: Coding guidelines
| if (!(start instanceof Date) || Number.isNaN(start.getTime())) { | ||
| throw new Error("startTime must be a valid Date."); | ||
| } | ||
|
|
||
| if (!(end instanceof Date) || Number.isNaN(end.getTime())) { | ||
| throw new Error("endTime must be a valid Date."); | ||
| } | ||
|
|
||
| if (start.getTime() >= end.getTime()) { | ||
| throw new Error("startTime must be earlier than endTime."); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate the file exporting ErrorWithCode.
rg -l "class ErrorWithCode"Repository: calcom/cal.diy
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## Search for ErrorWithCode references\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' '\bErrorWithCode\b|class\s+ErrorWithCode|export\s+\{[^}]*ErrorWithCode|export\s+.*ErrorWithCode' .
printf '\n## Search for common error helper names\n'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' '\bErrorWithCode\b|\bTRPCError\b|\bHttpError\b|\bApiError\b' packages apps .
printf '\n## Inspect target file\n'
sed -n '1,140p' packages/lib/prismaIntervalOverlap.tsRepository: calcom/cal.diy
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## Target file'
sed -n '1,140p' packages/lib/prismaIntervalOverlap.ts
echo
echo '## Error helper export'
sed -n '1,120p' packages/lib/errors.ts
echo
echo '## Error conversion usage'
sed -n '1,160p' packages/lib/server/getServerErrorFromUnknown.tsRepository: calcom/cal.diy
Length of output: 10843
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '## Usages of prismaIntervalOverlap'
rg -n --hidden --glob '!node_modules' --glob '!dist' --glob '!build' 'prismaIntervalOverlap|getOverlappingIntervalWhereClause' packages apps .
echo
echo '## Relevant error handling path in the server layer'
sed -n '1,200p' packages/lib/server/defaultResponder.ts
echo
echo '## Relevant error handling path in the server layer'
sed -n '1,200p' packages/lib/server/getServerErrorFromUnknown.tsRepository: calcom/cal.diy
Length of output: 14995
Use ErrorWithCode.Factory.BadRequest for these validation failures Import ErrorWithCode from @calcom/lib/errors and throw ErrorWithCode.Factory.BadRequest(...) here; plain Error falls through to the generic 500 path in server error handling.
🤖 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 31 - 41, Update the
validation failures in the interval validation block to import and use
ErrorWithCode.Factory.BadRequest from `@calcom/lib/errors` instead of plain Error,
covering invalid start dates, invalid end dates, and non-increasing intervals
while preserving the existing messages.
Source: Coding guidelines
|
|
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