feat: take into account guest availability when host reschedules#28636
feat: take into account guest availability when host reschedules#28636bcornish1797 wants to merge 29 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/trpc/server/routers/viewer/slots/util.ts">
<violation number="1" location="packages/trpc/server/routers/viewer/slots/util.ts:847">
P2: Guest busy-time filtering is applied to all reschedules with a UID, but the feature is host-reschedule-specific; missing initiator gating can incorrectly restrict attendee reschedules.</violation>
<violation number="2" location="packages/trpc/server/routers/viewer/slots/util.ts:867">
P2: Non‑Cal.com guest emails are still used in the OR attendee-email filter, so their bookings can be pulled into guest busy times despite the Cal.com user filter. Narrow userEmails to emails of resolved Cal.com users (or omit it) to avoid including non‑Cal.com guests.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Both issues addressed in 416ecc9: P2 (host-initiator gating): Added P2 (email filter scope): Tests added for both fixes. |
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/trpc/server/routers/viewer/slots/util.ts">
<violation number="1" location="packages/trpc/server/routers/viewer/slots/util.ts:862">
P2: Reschedule initiator detection is incorrect: comparing `original.userId` to `hostUserIds` does not tell whether host or attendee initiated the reschedule.</violation>
</file>
<file name="packages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.ts">
<violation number="1" location="packages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.ts:261">
P2: Newly added Cal.com-only email filtering test conflicts with an existing test that still expects external attendee emails, leaving contradictory test contracts.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Addressed in b8628c5: P2 (initiator detection): Removed the broken P2 (test conflict): Fixed the "pass correct userIds and emails" test to expect only Cal.com user emails, consistent with the email filtering logic. Removed the broken initiator-gating test. |
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/trpc/server/routers/viewer/slots/util.ts">
<violation number="1" location="packages/trpc/server/routers/viewer/slots/util.ts:857">
P2: Guest busy-time blocking now applies to all non-collective reschedules because host/attendee gating was removed, causing attendee-initiated reschedules to incorrectly hide slots.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
|
Addressed in c4fc209 and 25061e9: P2 (host/attendee gating): Added
Comparison is case-insensitive. Orphaned bookings (no user) are handled safely. Type safety fix: Test updates: 5 new test cases covering host-initiated, attendee-initiated, case-insensitive match, and null/undefined backwards compatibility. Existing test data updated to match the new BookingRepository select shape. |
bcornish1797
force-pushed
the
feat/16378-guest-availability-reschedule
branch
2 times, most recently
from
734dd56 to
032bb3a
Compare
|
@cla-bot check |
|
recheck |
|
I have read the CLA Document and I hereby sign the CLA |
|
Hi team 👋 All review feedback from cubic-dev-ai has been addressed (see inline replies). The CLA is signed and passing. Could a maintainer please add the Summary of changes (10 files, 16 tests):
Thank you! |
|
Hi team, friendly follow-up — this PR is ready for CI. Could a maintainer please add the |
|
Hi team 👋 Just a friendly follow-up — all review comments have been addressed and the PR is mergeable. Would appreciate a re-review when you get a chance. Happy to make any further adjustments if needed! |
|
Friendly follow-up — all review comments have been addressed and the PR is ready for re-review. Could a maintainer please add the |
|
@dhairyashiil Could you please review or help get this PR reviewed? All previous review comments have been addressed, and the PR is mergeable. The only blocker is the |
|
Hi maintainers, friendly ping — could someone please approve the CI run for this fork PR? The Current status:
Thanks for your time! |
|
Hi — following up on this. If I understand the CI setup correctly, fork PRs here need a maintainer to add the |
|
Hi @sahitya-chandra — I noticed from the cal.com PR history that you've been handling PR is OPEN since 2026-03-28, mergeable, all cubic-dev-ai P2 review comments have been addressed across commits 416ecc9 → b8628c5 → c4fc209 → 443f085. The only remaining gate is the Thanks for your help with the cal.com bounty contributor flow! |
bcornish1797
force-pushed
the
feat/16378-guest-availability-reschedule
branch
from
443f085 to
8a27404
Compare
|
Someone is attempting to deploy a commit to the cal-diy Team on Vercel. A member of the Team first needs to authorize it. |
|
Update — pushed
Status snapshot
CODEOWNERS for the changed pathsThe two production files in this PR ( Thanks! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHooks were updated to accept an optional 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/features/availability/lib/getUserAvailability.ts (1)
621-626: Use native ISO serialization for guest busy slots.
guestBusyTimesis already typed asDate, so the Day.js round-trip here is unnecessary work in a hot path.♻️ Proposed refactor
- const guestBusyTimesFormatted: EventBusyDetails[] = (initialData?.guestBusyTimes ?? []).map((t) => ({ - start: dayjs.utc(t.start).toISOString(), - end: dayjs.utc(t.end).toISOString(), + const guestBusyTimesFormatted: EventBusyDetails[] = (initialData?.guestBusyTimes ?? []).map((t) => ({ + start: t.start.toISOString(), + end: t.end.toISOString(), title: "Guest busy", source: withSource ? "guest-availability" : "", }));As per coding guidelines "Use
date-fnsor nativeDateinstead of Day.js when timezone awareness isn't needed".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/availability/lib/getUserAvailability.ts` around lines 621 - 626, guestBusyTimesFormatted currently round-trips guestBusyTimes through dayjs.utc which is unnecessary and costly; change the mapping to use native Date ISO serialization instead: for each item in initialData?.guestBusyTimes use new Date(t.start).toISOString() and new Date(t.end).toISOString() to populate start/end, preserve title ("Guest busy") and source logic (withSource ? "guest-availability" : ""), and keep the EventBusyDetails shape so you avoid Day.js in this hot path (update the mapping inside guestBusyTimesFormatted).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/features/users/repositories/UserRepository.ts`:
- Around line 1524-1546: The current aggregation collapses every hit to {id,
email} and loses which secondary email(s) matched; change the logic in
UserRepository (the byPrimary/bySecondary queries and the dedupe loop around
seen) to return the matched secondary address(es) along with the user id (e.g.,
include matchedSecondaryEmails: string[] or an emails array), by selecting the
matching secondaryEmails in the bySecondary query (and preserving the primary
email for byPrimary hits), and when merging [...byPrimary, ...bySecondary]
combine/dedupe per user id so the returned entries include all matched emails
used later by findByUserIdsAndDateRange/viewer slots lookup.
---
Nitpick comments:
In `@packages/features/availability/lib/getUserAvailability.ts`:
- Around line 621-626: guestBusyTimesFormatted currently round-trips
guestBusyTimes through dayjs.utc which is unnecessary and costly; change the
mapping to use native Date ISO serialization instead: for each item in
initialData?.guestBusyTimes use new Date(t.start).toISOString() and new
Date(t.end).toISOString() to populate start/end, preserve title ("Guest busy")
and source logic (withSource ? "guest-availability" : ""), and keep the
EventBusyDetails shape so you avoid Day.js in this hot path (update the mapping
inside guestBusyTimesFormatted).
🪄 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: 821e2db1-208e-428f-bcab-a2d0420dae14
📒 Files selected for processing (10)
apps/web/modules/schedules/hooks/useEvent.tsapps/web/modules/schedules/hooks/useSchedule.tspackages/features/availability/lib/getUserAvailability.tspackages/features/bookings/repositories/BookingRepository.test.tspackages/features/bookings/repositories/BookingRepository.tspackages/features/users/repositories/UserRepository.test.tspackages/features/users/repositories/UserRepository.tspackages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.tspackages/trpc/server/routers/viewer/slots/types.tspackages/trpc/server/routers/viewer/slots/util.ts
Split of calcom#28636 (Part A of 3). Pure additive infra — no call sites change and no existing behaviour is altered. This layer is the data-access foundation that Parts B (frontend `rescheduledBy` plumbing) and C (slots/util.ts business logic) build on. BookingRepository: - findByUidIncludeAttendeeEmails(uid): fetches an original booking's attendee emails and the host user's email, used to detect who the reschedule initiator is and resolve attendees to Cal.com users. - findByUserIdsAndDateRange({ userIds, userEmails, dateFrom, dateTo, excludeUid? }): finds ACCEPTED/PENDING bookings overlapping a date range by userId or attendee email (case-insensitive), with an excludeUid parameter applied at the database level so the caller cannot accidentally include the very booking being rescheduled. UserRepository: - findByEmails({ emails }): resolves a list of emails to Cal.com users, checking both primary email and verified secondary emails, case-insensitively, with input deduplication before the query and output deduplication by user id. Uses Promise.all to fan out the two lookups concurrently. Tests cover: empty-input short-circuits, primary vs secondary lookup, dedup across both lookups, case-insensitive normalization, excludeUid, OR clause composition, and the select shape used downstream.
|
🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/features/users/repositories/UserRepository.ts (1)
1510-1538:⚠️ Potential issue | 🟠 MajorSecondary-email match is still collapsed to the primary email.
The
seen.set(u.id, u)dedup uses whichever row arrives first (primary vs. secondary query), so a user matched only via a verifiedsecondaryEmailstill returns{ id, email: user.email }(the primary). Callers inslots/util.tsthen feed these emails intoBookingRepository.findByUserIdsAndDateRange's attendee-email filter — meaning bookings where the guest appears under the secondary address won't match, and the host will be shown slots that actually conflict with the guest.Recommend selecting the matched
secondaryEmailsin the bySecondary branch and carrying them alongside (e.g.,matchedEmails: string[]oremails: string[]), then merging perid.Note: there's also an existing
findVerifiedUsersByEmailsRawa few lines above (L293–329) that already handles the exact dual-primary/secondary lookup via a raw SQLUNIONand preservesmatchedEmail. Consider either reusing/extending that helper or mirroring its shape so downstream consumers get the matched email.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/users/repositories/UserRepository.ts` around lines 1510 - 1538, findByEmails currently deduplicates users by id using the first-row email, so matches found only via a verified secondary email return the primary email and break downstream attendee-email filters; update findByEmails to include the matched email(s) from the bySecondary query (e.g., add select { secondaryEmails: { where: { email: { in: normalized }, emailVerified: { not: null } }, select: { email: true } } } or mirror the shape of findVerifiedUsersByEmailsRaw to return matchedEmail/matchedEmails), then when merging results for the same id combine/replace the stored email with the matched secondary email(s) (or store matchedEmails: string[]) so callers get the actual matched address; alternatively reuse/extend findVerifiedUsersByEmailsRaw to return matchedEmail and call it from findByEmails and return objects that include id and matchedEmail(s) instead of always returning the primary email.
🧹 Nitpick comments (1)
packages/features/users/repositories/UserRepository.test.ts (1)
117-215: Tests validate the current (collapsed-email) behavior.Once the secondary-email preservation issue flagged in
UserRepository.tsis fixed, the "secondary (verified) email" test at L152–173 should assert that the matched secondary address is returned alongside the user id (not just the primary). Otherwise the tests will lock in the bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/users/repositories/UserRepository.test.ts` around lines 117 - 215, Update the "should look up users by secondary (verified) email" test to assert that findByEmails returns the matched secondary address along with the user id (not only the primary email). Specifically, adjust the mocked Prisma responses and the final expect for repo.findByEmails in that test so the returned user object includes the matched secondary email (e.g., a property like matchedSecondaryEmail or secondaryEmail equal to "secondary@example.com") and assert that value along with id; keep the existing expect.objectContaining on the second findMany call that checks the secondaryEmails.where clause unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/features/bookings/repositories/BookingRepository.ts`:
- Around line 2168-2189: The query can return duplicate booking rows when both
OR branches match the same booking; modify the method that calls
this.prismaClient.booking.findMany (the block using userIds, userEmails,
excludeUid and status filters) to deduplicate results by uid before returning
(e.g., run a unique-by-uid pass over the array returned from
prismaClient.booking.findMany and return only one entry per uid), or
alternatively add a clear comment in BookingRepository.ts documenting that
callers (like getUserAvailability) must dedupe by uid; ensure the change
references the prismaClient.booking.findMany call and preserves the existing
selected fields (uid, startTime, endTime, title, userId, status).
---
Duplicate comments:
In `@packages/features/users/repositories/UserRepository.ts`:
- Around line 1510-1538: findByEmails currently deduplicates users by id using
the first-row email, so matches found only via a verified secondary email return
the primary email and break downstream attendee-email filters; update
findByEmails to include the matched email(s) from the bySecondary query (e.g.,
add select { secondaryEmails: { where: { email: { in: normalized },
emailVerified: { not: null } }, select: { email: true } } } or mirror the shape
of findVerifiedUsersByEmailsRaw to return matchedEmail/matchedEmails), then when
merging results for the same id combine/replace the stored email with the
matched secondary email(s) (or store matchedEmails: string[]) so callers get the
actual matched address; alternatively reuse/extend findVerifiedUsersByEmailsRaw
to return matchedEmail and call it from findByEmails and return objects that
include id and matchedEmail(s) instead of always returning the primary email.
---
Nitpick comments:
In `@packages/features/users/repositories/UserRepository.test.ts`:
- Around line 117-215: Update the "should look up users by secondary (verified)
email" test to assert that findByEmails returns the matched secondary address
along with the user id (not only the primary email). Specifically, adjust the
mocked Prisma responses and the final expect for repo.findByEmails in that test
so the returned user object includes the matched secondary email (e.g., a
property like matchedSecondaryEmail or secondaryEmail equal to
"secondary@example.com") and assert that value along with id; keep the existing
expect.objectContaining on the second findMany call that checks the
secondaryEmails.where clause unchanged.
🪄 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: feb43dd4-b3f2-49aa-ac8b-3562ab914133
📒 Files selected for processing (10)
apps/web/modules/schedules/hooks/useEvent.tsapps/web/modules/schedules/hooks/useSchedule.tspackages/features/availability/lib/getUserAvailability.tspackages/features/bookings/repositories/BookingRepository.test.tspackages/features/bookings/repositories/BookingRepository.tspackages/features/users/repositories/UserRepository.test.tspackages/features/users/repositories/UserRepository.tspackages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.tspackages/trpc/server/routers/viewer/slots/types.tspackages/trpc/server/routers/viewer/slots/util.ts
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)
apps/web/modules/schedules/hooks/useSchedule.ts (1)
106-151:⚠️ Potential issue | 🟠 MajorDo not route reschedules with
rescheduledBythrough API V2 until the DTO supports it.
rescheduledByis added to this payload, but the API V2 available-slots input in the provided context does not define that field. Team-event reschedules using API V2 will therefore lose the initiator context, causing attendee-initiated reschedules to incorrectly apply guest blocking.Conservative local fallback
- const isCallingApiV2Slots = useApiV2 && Boolean(isTeamEvent) && options.enabled; + const isCallingApiV2Slots = + useApiV2 && Boolean(isTeamEvent) && options.enabled && !(rescheduleUid && rescheduledBy);Alternatively, add
rescheduledBy?: string | nullto the API V2 input DTO and pass it through the slots service.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/modules/schedules/hooks/useSchedule.ts` around lines 106 - 151, The API V2 call is receiving rescheduledBy in the payload even though the V2 DTO doesn't support it, losing initiator context; update the call site so reschedules with rescheduledBy don't go through V2: modify the isCallingApiV2Slots guard (used to decide V2 usage) or strip rescheduledBy before calling useApiV2AvailableSlots so that when input.rescheduledBy is present you fall back to V1 (i.e., change isCallingApiV2Slots = useApiV2 && Boolean(isTeamEvent) && options.enabled && !input.rescheduledBy or remove rescheduledBy from the spread passed to useApiV2AvailableSlots), or alternatively add rescheduledBy?: string|null to the API V2 DTO and propagate it through the slots service if you prefer enabling V2.
♻️ Duplicate comments (1)
packages/features/users/repositories/UserRepository.ts (1)
1510-1538:⚠️ Potential issue | 🟠 MajorPreserve the matched secondary email addresses.
Secondary-email hits currently return only the user’s primary
findByUserIdsAndDateRange.Suggested shape
- this.prismaClient.user.findMany({ + this.prismaClient.user.findMany({ where: { secondaryEmails: { some: { email: { in: normalized, mode: "insensitive" }, emailVerified: { not: null }, }, }, }, - select: { id: true, email: true }, + select: { + id: true, + email: true, + secondaryEmails: { + where: { + email: { in: normalized, mode: "insensitive" }, + emailVerified: { not: null }, + }, + select: { email: true }, + }, + }, }), ]); - const seen = new Map<number, { id: number; email: string }>(); - for (const u of [...byPrimary, ...bySecondary]) { - if (!seen.has(u.id)) seen.set(u.id, u); + const seen = new Map<number, { id: number; email: string; matchedEmails: string[] }>(); + for (const u of byPrimary) { + seen.set(u.id, { id: u.id, email: u.email, matchedEmails: [u.email] }); + } + for (const u of bySecondary) { + const existing = seen.get(u.id) ?? { id: u.id, email: u.email, matchedEmails: [] }; + seen.set(u.id, { + ...existing, + matchedEmails: Array.from( + new Set([...existing.matchedEmails, ...u.secondaryEmails.map(({ email }) => email)]) + ), + }); } return Array.from(seen.values());
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.ts (1)
222-266: Remove comments that only restate the assertions.These comments duplicate the
expect(...).toHaveBeenCalled()assertions; the test names already capture the behavior.Suggested cleanup
- // Should proceed to check guest availability (host email matches case-insensitively) expect(mockDependencies.userRepo.findByEmails).toHaveBeenCalled(); ... - // Without rescheduledBy, should still check guest availability (safe default) expect(mockDependencies.userRepo.findByEmails).toHaveBeenCalled(); ... - // Without rescheduledBy, should still check guest availability (safe default) expect(mockDependencies.userRepo.findByEmails).toHaveBeenCalled();As per coding guidelines, “Never add comments that simply restate what the code does”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.ts` around lines 222 - 266, Remove the inline comments that merely restate the subsequent assertions in the two tests titled "should check guest busy times when rescheduledBy is not provided (backwards compat)" and "should check guest busy times when rescheduledBy is null (backwards compat)"; specifically delete the comments immediately preceding the expect(mockDependencies.userRepo.findByEmails).toHaveBeenCalled() lines so the tests remain the same (they still call callGetGuestBusyTimes and assert via mockDependencies.userRepo.findByEmails and bookingRepo mocks) but without duplicate explanatory comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/features/bookings/repositories/BookingRepository.test.ts`:
- Around line 120-149: The test for findByUserIdsAndDateRange is missing an
assertion that the query filters by userIds; update the
BookingRepository.test.ts case (the one calling
repository.findByUserIdsAndDateRange and mocking
mockPrismaClient.booking.findMany) to assert that the prisma call's where
includes the userId predicate (e.g. userId: { in: [10] } or equivalent)
alongside the existing status and date overlap assertions so the test fails if
userIds are not applied.
In
`@packages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.ts`:
- Around line 359-489: Add a new test (using callGetGuestBusyTimes) that mocks
bookingRepo.findByUidIncludeAttendeeEmails to return attendees with
duplicate/case-varied emails (e.g., "Alias@Cal.com" and "alias@cal.com"), mocks
userRepo.findByEmails to return a single Cal.com user (resolved via secondary
email), and mocks bookingRepo.findByUserIdsAndDateRange to return []. Then
assert that userRepo.findByEmails was called with the original attendee emails,
and that bookingRepo.findByUserIdsAndDateRange was called with
deduplicated/userId list and with userEmails deduped and case-normalized (only
one email for the resolved Cal.com user) and include excludeUid/dateFrom/dateTo
as in other tests; reference
mockDependencies.bookingRepo.findByUidIncludeAttendeeEmails,
mockDependencies.userRepo.findByEmails,
mockDependencies.bookingRepo.findByUserIdsAndDateRange, and
callGetGuestBusyTimes to locate where to add the test.
---
Outside diff comments:
In `@apps/web/modules/schedules/hooks/useSchedule.ts`:
- Around line 106-151: The API V2 call is receiving rescheduledBy in the payload
even though the V2 DTO doesn't support it, losing initiator context; update the
call site so reschedules with rescheduledBy don't go through V2: modify the
isCallingApiV2Slots guard (used to decide V2 usage) or strip rescheduledBy
before calling useApiV2AvailableSlots so that when input.rescheduledBy is
present you fall back to V1 (i.e., change isCallingApiV2Slots = useApiV2 &&
Boolean(isTeamEvent) && options.enabled && !input.rescheduledBy or remove
rescheduledBy from the spread passed to useApiV2AvailableSlots), or
alternatively add rescheduledBy?: string|null to the API V2 DTO and propagate it
through the slots service if you prefer enabling V2.
---
Nitpick comments:
In
`@packages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.ts`:
- Around line 222-266: Remove the inline comments that merely restate the
subsequent assertions in the two tests titled "should check guest busy times
when rescheduledBy is not provided (backwards compat)" and "should check guest
busy times when rescheduledBy is null (backwards compat)"; specifically delete
the comments immediately preceding the
expect(mockDependencies.userRepo.findByEmails).toHaveBeenCalled() lines so the
tests remain the same (they still call callGetGuestBusyTimes and assert via
mockDependencies.userRepo.findByEmails and bookingRepo mocks) but without
duplicate explanatory comments.
🪄 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: aea1dcb1-ede6-40ce-8f38-79f857d39545
📒 Files selected for processing (10)
apps/web/modules/schedules/hooks/useEvent.tsapps/web/modules/schedules/hooks/useSchedule.tspackages/features/availability/lib/getUserAvailability.tspackages/features/bookings/repositories/BookingRepository.test.tspackages/features/bookings/repositories/BookingRepository.tspackages/features/users/repositories/UserRepository.test.tspackages/features/users/repositories/UserRepository.tspackages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.tspackages/trpc/server/routers/viewer/slots/types.tspackages/trpc/server/routers/viewer/slots/util.ts
| it("should query bookings by userId when userIds are provided", async () => { | ||
| const mockBookings = [ | ||
| { | ||
| uid: "booking-1", | ||
| startTime: new Date("2026-04-10T09:00:00Z"), | ||
| endTime: new Date("2026-04-10T10:00:00Z"), | ||
| title: "Meeting", | ||
| userId: 10, | ||
| status: BookingStatus.ACCEPTED, | ||
| }, | ||
| ]; | ||
| mockPrismaClient.booking.findMany.mockResolvedValue(mockBookings); | ||
|
|
||
| const result = await repository.findByUserIdsAndDateRange({ | ||
| userIds: [10], | ||
| userEmails: [], | ||
| dateFrom, | ||
| dateTo, | ||
| }); | ||
|
|
||
| expect(result).toEqual(mockBookings); | ||
| expect(mockPrismaClient.booking.findMany).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| where: expect.objectContaining({ | ||
| status: { in: [BookingStatus.ACCEPTED, BookingStatus.PENDING] }, | ||
| AND: [{ startTime: { lt: dateTo } }, { endTime: { gt: dateFrom } }], | ||
| }), | ||
| }) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Assert the userId predicate in the userId-only path.
This test currently verifies status/date overlap but would still pass if the repository forgot to filter by userIds, causing guest busy-time lookups to include unrelated bookings.
Suggested assertion
expect(mockPrismaClient.booking.findMany).toHaveBeenCalledWith(
expect.objectContaining({
where: expect.objectContaining({
status: { in: [BookingStatus.ACCEPTED, BookingStatus.PENDING] },
AND: [{ startTime: { lt: dateTo } }, { endTime: { gt: dateFrom } }],
+ OR: expect.arrayContaining([{ userId: { in: [10] } }]),
}),
})
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/features/bookings/repositories/BookingRepository.test.ts` around
lines 120 - 149, The test for findByUserIdsAndDateRange is missing an assertion
that the query filters by userIds; update the BookingRepository.test.ts case
(the one calling repository.findByUserIdsAndDateRange and mocking
mockPrismaClient.booking.findMany) to assert that the prisma call's where
includes the userId predicate (e.g. userId: { in: [10] } or equivalent)
alongside the existing status and date overlap assertions so the test fails if
userIds are not applied.
…secondary) Secondary-email hits were collapsing to the primary address, so guest busy-time lookup missed bookings where a Cal.com user attended under a verified secondary email. findByEmails now returns matchedEmails: string[] per user (union of primary and/or secondary matches), and the guest-busy flow feeds those actual matched emails into findByUserIdsAndDateRange.
The API V2 available-slots DTO does not carry rescheduledBy, so routing a reschedule through V2 loses the host/attendee initiator context that the V1 guest busy-time gate relies on. The useApiV2Slots guard now opts back to V1 whenever both rescheduleUid and rescheduledBy are present.
Three inline comments in getGuestBusyTimesForReschedule.test.ts just paraphrased the expect lines below them. Removed per the project coding guideline that discourages comments which only restate what the code does.
|
Addressed the three actionable items from the two CodeRabbit passes.
Skipped for now, open to adding if wanted: the optional test cases suggested under "prompt for AI agents" (duplicate / case-varied secondary emails, and an explicit CI is still blocked by the @cubic-dev-ai @coderabbitai — please re-review. |
|
Kicking off a fresh full review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/features/users/repositories/UserRepository.test.ts (1)
203-231: Tighten these assertions to cover the full contract.The last test only checks length, and the normalization test only checks the primary lookup. Assert the secondary lookup input and each returned user’s
matchedEmailsso regressions in the new mapping behavior fail loudly.Suggested assertion hardening
test("should normalize emails to lowercase and deduplicate input", async () => { mockPrismaClient.user.findMany .mockResolvedValueOnce([{ id: 1, email: "user@example.com" }]) .mockResolvedValueOnce([]); await repo.findByEmails({ emails: ["User@Example.COM", "user@example.com"] }); expect(mockPrismaClient.user.findMany).toHaveBeenNthCalledWith( 1, expect.objectContaining({ where: { email: { in: ["user@example.com"], mode: "insensitive" } }, }) ); + expect(mockPrismaClient.user.findMany).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + where: { + secondaryEmails: { + some: { + email: { in: ["user@example.com"], mode: "insensitive" }, + emailVerified: { not: null }, + }, + }, + }, + }) + ); }); test("should return multiple distinct users", async () => { mockPrismaClient.user.findMany .mockResolvedValueOnce([ @@ }); - expect(result).toHaveLength(2); + expect(result).toEqual( + expect.arrayContaining([ + { id: 1, email: "user1@example.com", matchedEmails: ["user1@example.com"] }, + { id: 2, email: "user2@example.com", matchedEmails: ["user2@example.com"] }, + ]) + ); + expect(result).toHaveLength(2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/users/repositories/UserRepository.test.ts` around lines 203 - 231, Tighten the tests for repo.findByEmails: in the "normalize emails" test, assert both calls to mockPrismaClient.user.findMany (the primary and the secondary lookup) receive the expected where.in arrays (normalized/unique emails) and include checking the second call's arguments; in the "should return multiple distinct users" test, assert the returned array contains the exact user objects and that each user has the correct matchedEmails mapping (e.g., matchedEmails for id 1 includes "user1@example.com", for id 2 includes "user2@example.com") so the mapping/secondary-lookup behavior is verified in addition to length.packages/trpc/server/routers/viewer/slots/util.ts (1)
717-720: Keep degradation logs structured and bounded.
safeStringify({ rescheduleUid, error })produces a string payload and may either drop usefulErrorfields or include too much from repository failures. Prefer logging explicit, bounded metadata.Suggested logging adjustment
+ const errorDetails = + error instanceof Error + ? { + name: error.name, + message: error.message, + code: "code" in error ? String(error.code) : undefined, + } + : { value: safeStringify(error) }; + log.warn( "[getGuestBusyTimesForReschedule] degraded to empty result", - safeStringify({ rescheduleUid, error }) + { rescheduleUid, error: errorDetails } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/server/routers/viewer/slots/util.ts` around lines 717 - 720, The degradation log currently calls log.warn with safeStringify({ rescheduleUid, error }) which can either drop Error fields or include unbounded payloads; change the log to emit a small, structured object instead (inside getGuestBusyTimesForReschedule and the log.warn call), e.g. include rescheduleUid and a bounded error object with explicit fields like error.name, error.message and optionally error.code or a boolean flag (omit error.stack and large nested properties) so the logged payload is predictable and size-bounded rather than relying on safeStringify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/modules/schedules/hooks/useSchedule.ts`:
- Around line 106-107: The code forwards an email-valued rescheduledBy from
useSchedule.ts into the slots cache which currently JSON-stringifies inputs and
logs cacheKey in packages/trpc/server/routers/viewer/slots/util.ts (see cache
key handling around the existing log sites), so sanitize before persisting or
logging: update the cache key generation in the slots util to either
remove/redact the rescheduledBy field from the input payload or replace it with
a deterministic hash (e.g., SHA-256) of the email, and ensure the same sanitized
value is used wherever cacheKey is stored or logged (the log calls near the
existing cacheKey creation must log the hashed/redacted key instead of raw
JSON); keep the redaction/hashing logic centralized in the cache key generation
function so all callers (including where rescheduledBy is passed) inherit the
fix.
---
Nitpick comments:
In `@packages/features/users/repositories/UserRepository.test.ts`:
- Around line 203-231: Tighten the tests for repo.findByEmails: in the
"normalize emails" test, assert both calls to mockPrismaClient.user.findMany
(the primary and the secondary lookup) receive the expected where.in arrays
(normalized/unique emails) and include checking the second call's arguments; in
the "should return multiple distinct users" test, assert the returned array
contains the exact user objects and that each user has the correct matchedEmails
mapping (e.g., matchedEmails for id 1 includes "user1@example.com", for id 2
includes "user2@example.com") so the mapping/secondary-lookup behavior is
verified in addition to length.
In `@packages/trpc/server/routers/viewer/slots/util.ts`:
- Around line 717-720: The degradation log currently calls log.warn with
safeStringify({ rescheduleUid, error }) which can either drop Error fields or
include unbounded payloads; change the log to emit a small, structured object
instead (inside getGuestBusyTimesForReschedule and the log.warn call), e.g.
include rescheduleUid and a bounded error object with explicit fields like
error.name, error.message and optionally error.code or a boolean flag (omit
error.stack and large nested properties) so the logged payload is predictable
and size-bounded rather than relying on safeStringify.
🪄 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: 8f47e301-dae7-4148-92fa-a6ef7b556989
📒 Files selected for processing (5)
apps/web/modules/schedules/hooks/useSchedule.tspackages/features/users/repositories/UserRepository.test.tspackages/features/users/repositories/UserRepository.tspackages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.tspackages/trpc/server/routers/viewer/slots/util.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.ts
- packages/features/users/repositories/UserRepository.ts
| rescheduleUid, | ||
| rescheduledBy, |
There was a problem hiding this comment.
Sanitize cache/log handling before adding email-valued rescheduledBy.
Line 107 forwards rescheduledBy, which is later compared to original.user.email, so this is an email/user identifier. The slots cache currently JSON-stringifies the full input and logs the resulting cacheKey in packages/trpc/server/routers/viewer/slots/util.ts Lines 110, 131, and 139. Please make sure the server hashes or redacts that cache key/log payload so this new identifier is not persisted or emitted in raw form.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/modules/schedules/hooks/useSchedule.ts` around lines 106 - 107, The
code forwards an email-valued rescheduledBy from useSchedule.ts into the slots
cache which currently JSON-stringifies inputs and logs cacheKey in
packages/trpc/server/routers/viewer/slots/util.ts (see cache key handling around
the existing log sites), so sanitize before persisting or logging: update the
cache key generation in the slots util to either remove/redact the rescheduledBy
field from the input payload or replace it with a deterministic hash (e.g.,
SHA-256) of the email, and ensure the same sanitized value is used wherever
cacheKey is stored or logged (the log calls near the existing cacheKey creation
must log the hashed/redacted key instead of raw JSON); keep the
redaction/hashing logic centralized in the cache key generation function so all
callers (including where rescheduledBy is passed) inherit the fix.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/slots/util.ts (1)
694-710: Consider clarifying email normalization flow, but code is functionally correct.The three concerns raised need updating:
Line 694 — attendee email normalization:
findByEmailsalready normalizes emails internally (converts to lowercase and deduplicates at the source). While not explicit at the call site, the contract is upheld. This is an optional code clarity improvement, not a functional issue.Line 702 — case dedup of
matchedEmails: While additional case variants could be sent tofindByUserIdsAndDateRange, the booking query usesmode: "insensitive"in its Prisma where clause (line 2175 of BookingRepository), so case variants are handled correctly. This is a minor efficiency consideration rather than a bug.Line 702 — nullish guard for
matchedEmails: This concern is invalid. ThematchedEmailsfield is always initialized as astring[]inUserRepository.findByEmails(lines 1545 and 1553) and is guaranteed to exist. The type signature confirms it is neverundefined.The code functions correctly as-is. If desired, normalizing emails at line 694 would make the contract explicit, but it's not required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/server/routers/viewer/slots/util.ts` around lines 694 - 710, Normalize and dedupe attendee emails before calling findByEmails and ensure matchedEmails are normalized/deduped before passing to findByUserIdsAndDateRange: transform original.attendees.map(a => a.email) into a lowercased, filtered and unique list (e.g., map to .toLowerCase(), filter(Boolean), Array.from(new Set(...))) before calling this.dependencies.userRepo.findByEmails, and then build calUserEmails from calUsers.flatMap(u => u.matchedEmails ?? []).map(e => e.toLowerCase()) and dedupe with Array.from(new Set(...)) before calling this.dependencies.bookingRepo.findByUserIdsAndDateRange; keep the same userIds array (calUsers.map(u => u.id)) and preserve excludeUid/date range parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/trpc/server/routers/viewer/slots/util.ts`:
- Around line 694-710: Normalize and dedupe attendee emails before calling
findByEmails and ensure matchedEmails are normalized/deduped before passing to
findByUserIdsAndDateRange: transform original.attendees.map(a => a.email) into a
lowercased, filtered and unique list (e.g., map to .toLowerCase(),
filter(Boolean), Array.from(new Set(...))) before calling
this.dependencies.userRepo.findByEmails, and then build calUserEmails from
calUsers.flatMap(u => u.matchedEmails ?? []).map(e => e.toLowerCase()) and
dedupe with Array.from(new Set(...)) before calling
this.dependencies.bookingRepo.findByUserIdsAndDateRange; keep the same userIds
array (calUsers.map(u => u.id)) and preserve excludeUid/date range parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1132de45-0d42-421c-b183-0b3fa5cf1e97
📒 Files selected for processing (10)
apps/web/modules/schedules/hooks/useEvent.tsapps/web/modules/schedules/hooks/useSchedule.tspackages/features/availability/lib/getUserAvailability.tspackages/features/bookings/repositories/BookingRepository.test.tspackages/features/bookings/repositories/BookingRepository.tspackages/features/users/repositories/UserRepository.test.tspackages/features/users/repositories/UserRepository.tspackages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.tspackages/trpc/server/routers/viewer/slots/types.tspackages/trpc/server/routers/viewer/slots/util.ts
Add missing assertion on the bySecondary findMany call in the normalization test, and assert the exact { id, email, matchedEmails } shape in the multi-user test so regressions in the matchedEmails mapping fail loudly.
Replace safeStringify({ rescheduleUid, error }) with a structured, size-bounded object that surfaces Error.name/message/code and falls back to safeStringify only for non-Error values. Avoids unbounded nested repository error payloads and dropped Error fields.
|
Addressed the two nitpicks from the latest pass.
On the actionable flagged for The review on @coderabbitai — please re-review. |
|
Kicking off a fresh full review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/slots/util.ts (1)
718-729: Nit:String((error as { code?: unknown }).code)can serialize to the literal"undefined"/"null".
"code" in erroronly checks presence, not value. If the property exists but isundefined/null(common for wrapped errors), the log emitscode: "undefined"/code: "null"which is noisier than simply omitting the field.♻️ Proposed tweak
const errorDetails = error instanceof Error ? { name: error.name, message: error.message, - code: "code" in error ? String((error as { code?: unknown }).code) : undefined, + code: + "code" in error && (error as { code?: unknown }).code != null + ? String((error as { code?: unknown }).code) + : undefined, } : { value: safeStringify(error) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/server/routers/viewer/slots/util.ts` around lines 718 - 729, The log currently checks only for the presence of a "code" property and serializes it which can produce literal "undefined"/"null"; update the errorDetails construction in getGuestBusyTimesForReschedule so you read the code value into a local (e.g., codeVal = (error as any).code) and only include code: String(codeVal) when codeVal != null (i.e., not undefined and not null); keep the rest of the shape (name, message, fallback to safeStringify) and continue passing errorDetails into log.warn.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/trpc/server/routers/viewer/slots/util.ts`:
- Around line 718-729: The log currently checks only for the presence of a
"code" property and serializes it which can produce literal "undefined"/"null";
update the errorDetails construction in getGuestBusyTimesForReschedule so you
read the code value into a local (e.g., codeVal = (error as any).code) and only
include code: String(codeVal) when codeVal != null (i.e., not undefined and not
null); keep the rest of the shape (name, message, fallback to safeStringify) and
continue passing errorDetails into log.warn.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f8a6b83-691c-44f2-8489-238eb1e6d66a
📒 Files selected for processing (2)
packages/features/users/repositories/UserRepository.test.tspackages/trpc/server/routers/viewer/slots/util.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/users/repositories/UserRepository.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/features/bookings/repositories/BookingRepository.ts (1)
2168-2189:⚠️ Potential issue | 🟡 MinorOR branches can return duplicate rows for the same booking.
When a Cal.com guest is both the organizer (
userIdmatch) and also listed amongattendeesfor the same booking (or two guests whose one is organizer and another is an attendee on overlapping data), the PrismaORproduces duplicates — the sameuidrow can appear twice. Downstreamsubtract()ingetUserAvailabilityis idempotent so correctness is fine, but payloads are inflated and busy-time diagnostics can be misleading. Consider deduping byuidbefore returning, or documenting the contract for callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/bookings/repositories/BookingRepository.ts` around lines 2168 - 2189, The findMany OR can yield duplicate booking rows (same uid) when a user is both organizer and an attendee; update the method containing this.prismaClient.booking.findMany to dedupe results by uid before returning (e.g., collect returned entries into a map keyed by uid and return the unique values) so callers like getUserAvailability see a single record per booking; ensure the return shape (uid, startTime, endTime, title, userId, status) is preserved.
🧹 Nitpick comments (3)
packages/features/availability/lib/getUserAvailability.ts (1)
621-626: Minor: settingsource: ""whenwithSourceis false is redundant.
detailedBusyTimesis already stripped ofsourcevia thewithSource ? ... : map(({ source, ...rest }) => rest)branch on Lines 641–643, so assigning an empty string here is just noise. The existingbusyTimes.map(...)above doesn't do this because those entries come with realsourcevalues. Consider only settingsourcewhenwithSourceis true (and leaving itundefinedotherwise) to stay consistent withUserAvailabilityBusyDetails'sOptional<EventBusyDetails, "source">typing.♻️ Proposed tweak
- const guestBusyTimesFormatted: EventBusyDetails[] = (initialData?.guestBusyTimes ?? []).map((t) => ({ - start: t.start.toISOString(), - end: t.end.toISOString(), - title: "Guest busy", - source: withSource ? "guest-availability" : "", - })); + const guestBusyTimesFormatted = (initialData?.guestBusyTimes ?? []).map((t) => ({ + start: t.start.toISOString(), + end: t.end.toISOString(), + title: "Guest busy", + ...(withSource ? { source: "guest-availability" } : {}), + })) as EventBusyDetails[];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/availability/lib/getUserAvailability.ts` around lines 621 - 626, The guestBusyTimesFormatted mapping should not assign an empty string to source when withSource is false; update the mapping in getUserAvailability (the guestBusyTimesFormatted const that maps initialData?.guestBusyTimes) to only include source when withSource is true (omit/leave undefined otherwise) so it matches how detailedBusyTimes is handled and conforms to the UserAvailabilityBusyDetails Optional<EventBusyDetails, "source"> typing; locate guestBusyTimesFormatted, initialData, and withSource and conditionally add the source property instead of setting it to "".apps/web/modules/schedules/hooks/useSchedule.ts (1)
141-145: V2 fallback condition is sound and appropriately scoped. The V2 Available Slots API indeed lacks arescheduledByparameter, so the fallback to V1 when bothrescheduleUidandrescheduledByare present prevents loss of host/attendee initiator gating. Extending the V2 DTO to supportrescheduledByis a valid enhancement but should be a dedicated follow-up with full test coverage for host-initiated reschedule flows, as this impacts guest busy-time lookups and confirmation logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/modules/schedules/hooks/useSchedule.ts` around lines 141 - 145, The V2 fallback logic is correct: keep the computed flag isCallingApiV2Slots (derived from useApiV2, isTeamEvent, options.enabled, rescheduleUid, rescheduledBy) as-is, but add a concise TODO noting that supporting rescheduledBy in the V2 available-slots DTO is a follow-up work item with test coverage for host-initiated reschedules and guest busy-time lookup; reference the symbol isCallingApiV2Slots and the variables rescheduleUid/rescheduledBy in the TODO so the future change can be located easily in useSchedule.ts.packages/trpc/server/routers/viewer/slots/types.ts (1)
29-29: Consider tightening type validation forrescheduledByparameter.The
rescheduledByparameter is part of the documented reschedule flow and controls confirmation behavior (auto-confirm if owner email, manual confirmation otherwise). Instead of genericz.string(), usez.string().email().nullish()for better type safety and to reject malformed inputs early.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/server/routers/viewer/slots/types.ts` at line 29, The rescheduledBy field currently uses a permissive z.string().nullish(); tighten validation by replacing it with z.string().email().nullish() so only valid emails (or null/undefined) are accepted. Locate the rescheduledBy schema entry in types.ts (the line defining rescheduledBy) and update the Zod validator to z.string().email().nullish(), ensuring the rest of the schema stays unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/features/users/repositories/UserRepository.ts`:
- Around line 1510-1557: The primary branch in findByEmails accepts users
regardless of locked/emailVerified state; update the prismaClient.user.findMany
call used for byPrimary to mirror the verified semantics by adding filters
emailVerified: { not: null } and locked: false (same shape as the secondary
query), so both byPrimary and bySecondary only return non-locked, email-verified
users; keep the rest of the merging logic (seen map, matchedEmails) intact.
In
`@packages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.ts`:
- Around line 7-8: The test currently uses typeof
AvailableSlotsService.prototype._getGuestBusyTimesForReschedule which references
the private method and triggers TS2341; replace that with a structural function
type that matches the private method's signature instead of referencing it by
name (e.g. declare type GetGuestBusyTimesForReschedule = (/* same params as
_getGuestBusyTimesForReschedule */) => Promise</* same return type */>), or
reuse an exported public type if one exists; keep references to
AvailableSlotsService and _getGuestBusyTimesForReschedule in the comment for
traceability but do not use typeof on the private member.
---
Duplicate comments:
In `@packages/features/bookings/repositories/BookingRepository.ts`:
- Around line 2168-2189: The findMany OR can yield duplicate booking rows (same
uid) when a user is both organizer and an attendee; update the method containing
this.prismaClient.booking.findMany to dedupe results by uid before returning
(e.g., collect returned entries into a map keyed by uid and return the unique
values) so callers like getUserAvailability see a single record per booking;
ensure the return shape (uid, startTime, endTime, title, userId, status) is
preserved.
---
Nitpick comments:
In `@apps/web/modules/schedules/hooks/useSchedule.ts`:
- Around line 141-145: The V2 fallback logic is correct: keep the computed flag
isCallingApiV2Slots (derived from useApiV2, isTeamEvent, options.enabled,
rescheduleUid, rescheduledBy) as-is, but add a concise TODO noting that
supporting rescheduledBy in the V2 available-slots DTO is a follow-up work item
with test coverage for host-initiated reschedules and guest busy-time lookup;
reference the symbol isCallingApiV2Slots and the variables
rescheduleUid/rescheduledBy in the TODO so the future change can be located
easily in useSchedule.ts.
In `@packages/features/availability/lib/getUserAvailability.ts`:
- Around line 621-626: The guestBusyTimesFormatted mapping should not assign an
empty string to source when withSource is false; update the mapping in
getUserAvailability (the guestBusyTimesFormatted const that maps
initialData?.guestBusyTimes) to only include source when withSource is true
(omit/leave undefined otherwise) so it matches how detailedBusyTimes is handled
and conforms to the UserAvailabilityBusyDetails Optional<EventBusyDetails,
"source"> typing; locate guestBusyTimesFormatted, initialData, and withSource
and conditionally add the source property instead of setting it to "".
In `@packages/trpc/server/routers/viewer/slots/types.ts`:
- Line 29: The rescheduledBy field currently uses a permissive
z.string().nullish(); tighten validation by replacing it with
z.string().email().nullish() so only valid emails (or null/undefined) are
accepted. Locate the rescheduledBy schema entry in types.ts (the line defining
rescheduledBy) and update the Zod validator to z.string().email().nullish(),
ensuring the rest of the schema stays unchanged.
🪄 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: 2caf2b0d-e097-45f3-92ad-1b530683be57
📒 Files selected for processing (10)
apps/web/modules/schedules/hooks/useEvent.tsapps/web/modules/schedules/hooks/useSchedule.tspackages/features/availability/lib/getUserAvailability.tspackages/features/bookings/repositories/BookingRepository.test.tspackages/features/bookings/repositories/BookingRepository.tspackages/features/users/repositories/UserRepository.test.tspackages/features/users/repositories/UserRepository.tspackages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.tspackages/trpc/server/routers/viewer/slots/types.tspackages/trpc/server/routers/viewer/slots/util.ts
| async findByEmails({ emails }: { emails: string[] }) { | ||
| if (!emails.length) return []; | ||
|
|
||
| const normalized = Array.from(new Set(emails.map((e) => e.toLowerCase()))); | ||
|
|
||
| const [byPrimary, bySecondary] = await Promise.all([ | ||
| this.prismaClient.user.findMany({ | ||
| where: { email: { in: normalized, mode: "insensitive" } }, | ||
| select: { id: true, email: true }, | ||
| }), | ||
| this.prismaClient.user.findMany({ | ||
| where: { | ||
| secondaryEmails: { | ||
| some: { | ||
| email: { in: normalized, mode: "insensitive" }, | ||
| emailVerified: { not: null }, | ||
| }, | ||
| }, | ||
| }, | ||
| select: { | ||
| id: true, | ||
| email: true, | ||
| secondaryEmails: { | ||
| where: { | ||
| email: { in: normalized, mode: "insensitive" }, | ||
| emailVerified: { not: null }, | ||
| }, | ||
| select: { email: true }, | ||
| }, | ||
| }, | ||
| }), | ||
| ]); | ||
|
|
||
| const seen = new Map<number, { id: number; email: string; matchedEmails: string[] }>(); | ||
| for (const u of byPrimary) { | ||
| seen.set(u.id, { id: u.id, email: u.email, matchedEmails: [u.email] }); | ||
| } | ||
| for (const u of bySecondary) { | ||
| const existing = seen.get(u.id); | ||
| const secondaryMatches = u.secondaryEmails.map((s) => s.email); | ||
| if (existing) { | ||
| existing.matchedEmails = Array.from(new Set([...existing.matchedEmails, ...secondaryMatches])); | ||
| } else { | ||
| seen.set(u.id, { id: u.id, email: u.email, matchedEmails: secondaryMatches }); | ||
| } | ||
| } | ||
| return Array.from(seen.values()); | ||
| } |
There was a problem hiding this comment.
findByEmails does not filter out locked or unverified primary-email users.
The secondary-email branch requires emailVerified: { not: null }, and the sibling findVerifiedUsersByEmailsRaw additionally enforces locked = FALSE for both branches. Here, the primary-email query (Lines 1516–1519) accepts any user regardless of locked or emailVerified status. In the rescheduling flow this means a guest whose account is locked or whose primary email is unverified can still have their bookings pulled as busy time, which is inconsistent with how verified-secondary matches are gated and can surface bookings from disabled accounts.
Consider aligning with the verified-lookup semantics:
🛡️ Proposed fix
this.prismaClient.user.findMany({
- where: { email: { in: normalized, mode: "insensitive" } },
+ where: {
+ email: { in: normalized, mode: "insensitive" },
+ emailVerified: { not: null },
+ locked: false,
+ },
select: { id: true, email: true },
}),
this.prismaClient.user.findMany({
where: {
+ locked: false,
secondaryEmails: {
some: {
email: { in: normalized, mode: "insensitive" },
emailVerified: { not: null },
},
},
},📝 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.
| async findByEmails({ emails }: { emails: string[] }) { | |
| if (!emails.length) return []; | |
| const normalized = Array.from(new Set(emails.map((e) => e.toLowerCase()))); | |
| const [byPrimary, bySecondary] = await Promise.all([ | |
| this.prismaClient.user.findMany({ | |
| where: { email: { in: normalized, mode: "insensitive" } }, | |
| select: { id: true, email: true }, | |
| }), | |
| this.prismaClient.user.findMany({ | |
| where: { | |
| secondaryEmails: { | |
| some: { | |
| email: { in: normalized, mode: "insensitive" }, | |
| emailVerified: { not: null }, | |
| }, | |
| }, | |
| }, | |
| select: { | |
| id: true, | |
| email: true, | |
| secondaryEmails: { | |
| where: { | |
| email: { in: normalized, mode: "insensitive" }, | |
| emailVerified: { not: null }, | |
| }, | |
| select: { email: true }, | |
| }, | |
| }, | |
| }), | |
| ]); | |
| const seen = new Map<number, { id: number; email: string; matchedEmails: string[] }>(); | |
| for (const u of byPrimary) { | |
| seen.set(u.id, { id: u.id, email: u.email, matchedEmails: [u.email] }); | |
| } | |
| for (const u of bySecondary) { | |
| const existing = seen.get(u.id); | |
| const secondaryMatches = u.secondaryEmails.map((s) => s.email); | |
| if (existing) { | |
| existing.matchedEmails = Array.from(new Set([...existing.matchedEmails, ...secondaryMatches])); | |
| } else { | |
| seen.set(u.id, { id: u.id, email: u.email, matchedEmails: secondaryMatches }); | |
| } | |
| } | |
| return Array.from(seen.values()); | |
| } | |
| async findByEmails({ emails }: { emails: string[] }) { | |
| if (!emails.length) return []; | |
| const normalized = Array.from(new Set(emails.map((e) => e.toLowerCase()))); | |
| const [byPrimary, bySecondary] = await Promise.all([ | |
| this.prismaClient.user.findMany({ | |
| where: { | |
| email: { in: normalized, mode: "insensitive" }, | |
| emailVerified: { not: null }, | |
| locked: false, | |
| }, | |
| select: { id: true, email: true }, | |
| }), | |
| this.prismaClient.user.findMany({ | |
| where: { | |
| locked: false, | |
| secondaryEmails: { | |
| some: { | |
| email: { in: normalized, mode: "insensitive" }, | |
| emailVerified: { not: null }, | |
| }, | |
| }, | |
| }, | |
| select: { | |
| id: true, | |
| email: true, | |
| secondaryEmails: { | |
| where: { | |
| email: { in: normalized, mode: "insensitive" }, | |
| emailVerified: { not: null }, | |
| }, | |
| select: { email: true }, | |
| }, | |
| }, | |
| }), | |
| ]); | |
| const seen = new Map<number, { id: number; email: string; matchedEmails: string[] }>(); | |
| for (const u of byPrimary) { | |
| seen.set(u.id, { id: u.id, email: u.email, matchedEmails: [u.email] }); | |
| } | |
| for (const u of bySecondary) { | |
| const existing = seen.get(u.id); | |
| const secondaryMatches = u.secondaryEmails.map((s) => s.email); | |
| if (existing) { | |
| existing.matchedEmails = Array.from(new Set([...existing.matchedEmails, ...secondaryMatches])); | |
| } else { | |
| seen.set(u.id, { id: u.id, email: u.email, matchedEmails: secondaryMatches }); | |
| } | |
| } | |
| return Array.from(seen.values()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/features/users/repositories/UserRepository.ts` around lines 1510 -
1557, The primary branch in findByEmails accepts users regardless of
locked/emailVerified state; update the prismaClient.user.findMany call used for
byPrimary to mirror the verified semantics by adding filters emailVerified: {
not: null } and locked: false (same shape as the secondary query), so both
byPrimary and bySecondary only return non-locked, email-verified users; keep the
rest of the merging logic (seen map, matchedEmails) intact.
| type GetGuestBusyTimesForReschedule = | ||
| typeof AvailableSlotsService.prototype._getGuestBusyTimesForReschedule; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the method is declared private and see if tsc flags the access.
rg -nP 'private\s+(async\s+)?_getGuestBusyTimesForReschedule\b' -C1
fd -t f 'tsconfig.json' | head -5 | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'
# Attempt typecheck of just this file if tsc is available
fd -t f 'getGuestBusyTimesForReschedule.test.ts'Repository: calcom/cal.diy
Length of output: 1764
🌐 Web query:
Does TypeScript emit TS2341 for typeof ClassName.prototype.privateMember when the member is declared private?
💡 Result:
Yes, TypeScript emits error TS2341 when accessing typeof ClassName.prototype.privateMember for a private member, because ClassName.prototype is typed as the instance type ClassName, and private members are not accessible on the instance type from outside the class declaration context.
Citations:
- 1: https://stackoverflow.com/questions/55254712/typescript-class-prototype-includes-instance-member
- 2: https://www.geeksforgeeks.org/typescript/what-is-default-visibility-for-properties-methods-in-typescript-classes/
- 3: https://generalistprogrammer.com/errors/ts2341-property-is-private
- 4: Class's prototype property is typed incorrectly and includes instance properties microsoft/TypeScript#55904
typeof access to a private method will trigger a TypeScript error.
_getGuestBusyTimesForReschedule is declared private in AvailableSlotsService. TypeScript emits TS2341 ("Property is private and only accessible within class") when accessing a private member via typeof ClassName.prototype.member from outside the class context, regardless of whether the runtime cast works. This will fail during type-checking in CI.
Replace the alias with a structural type that avoids referencing the private member by name:
♻️ Suggested fix
- type GetGuestBusyTimesForReschedule =
- typeof AvailableSlotsService.prototype._getGuestBusyTimesForReschedule;
+ type GetGuestBusyTimesForReschedule = (params: {
+ rescheduleUid: string | null | undefined;
+ rescheduledBy?: string | null | undefined;
+ schedulingType: SchedulingType | null;
+ dateFrom: Date;
+ dateTo: Date;
+ }) => Promise<{ start: Date; end: Date }[]>;📝 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.
| type GetGuestBusyTimesForReschedule = | |
| typeof AvailableSlotsService.prototype._getGuestBusyTimesForReschedule; | |
| type GetGuestBusyTimesForReschedule = (params: { | |
| rescheduleUid: string | null | undefined; | |
| rescheduledBy?: string | null | undefined; | |
| schedulingType: SchedulingType | null; | |
| dateFrom: Date; | |
| dateTo: Date; | |
| }) => Promise<{ start: Date; end: Date }[]>; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.ts`
around lines 7 - 8, The test currently uses typeof
AvailableSlotsService.prototype._getGuestBusyTimesForReschedule which references
the private method and triggers TS2341; replace that with a structural function
type that matches the private method's signature instead of referencing it by
name (e.g. declare type GetGuestBusyTimesForReschedule = (/* same params as
_getGuestBusyTimesForReschedule */) => Promise</* same return type */>), or
reuse an exported public type if one exists; keep references to
AvailableSlotsService and _getGuestBusyTimesForReschedule in the comment for
traceability but do not use typeof on the private member.
The OR between userId and attendees.email can surface the same booking twice when a guest is both organizer and attendee on that row. Downstream subtract() in getUserAvailability is idempotent, so correctness holds, but payloads were inflated and busy-time diagnostics misleading. Dedupe by uid in-memory before returning.
- util.ts: guard null/undefined error.code so the log does not emit literal code: "undefined" / "null" - types.ts: validate rescheduledBy as z.string().email().nullish() to reject malformed inputs early - getUserAvailability.ts: omit source instead of setting source: "" when withSource is false, matches the Optional<EventBusyDetails, "source"> typing - useSchedule.ts: TODO breadcrumb for the V2 DTO follow-up so the fallback site is easy to locate later
|
Addressed most of the round-3 review.
Declined for this PR:
@coderabbitai — please re-review. |
|
On the two declined items:
Kicking off a fresh full review now. 🧠 Learnings used✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/availability/lib/getUserAvailability.ts (1)
621-638:⚠️ Potential issue | 🟡 MinorType inconsistency:
sourceis required onEventBusyDetails.Per
packages/types/Calendar.d.ts,EventBusyDetails.source: stringis required (not optional). The newguestBusyTimesFormattedis typed asEventBusyDetails[]but only setssourcewhenwithSourceis true, so whenwithSource=falsethe objects don't satisfy the declared type. Functionally it works becausesourceis later stripped at Line 643, but the inconsistency differs from the sibling entries (busyTimes,busyTimesFromLimits,busyTimesFromTeamLimits) which always carrysource.Recommend always setting
source: "guest-availability"here and letting the existing strip step at Line 643 handle thewithSource=falsepath — matching how the other arrays are constructed.♻️ Proposed fix
- const guestBusyTimesFormatted: EventBusyDetails[] = (initialData?.guestBusyTimes ?? []).map((t) => ({ - start: t.start.toISOString(), - end: t.end.toISOString(), - title: "Guest busy", - ...(withSource ? { source: "guest-availability" } : {}), - })); + const guestBusyTimesFormatted: EventBusyDetails[] = (initialData?.guestBusyTimes ?? []).map((t) => ({ + start: t.start.toISOString(), + end: t.end.toISOString(), + title: "Guest busy", + source: "guest-availability", + }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/availability/lib/getUserAvailability.ts` around lines 621 - 638, guestBusyTimesFormatted is declared as EventBusyDetails[] but only sets source when withSource is true, causing a type mismatch; always set source: "guest-availability" on each guestBusyTimesFormatted entry (matching busyTimes/busyTimesFromLimits/busyTimesFromTeamLimits) and rely on the existing later strip step to remove source when withSource is false so types remain consistent and the runtime behavior is unchanged.
🧹 Nitpick comments (3)
packages/features/bookings/repositories/BookingRepository.ts (1)
2141-2197: New repo methods LGTM; one small nit onexcludeUidfalsy handling.
findByUidIncludeAttendeeEmailsandfindByUserIdsAndDateRangeare well-scoped: DB-levelexcludeUid, early[]on empty inputs, and post-query dedupe byuidaddress the prior duplicate-row concern fromORfan-out.Minor:
excludeUid ? { uid: { not: excludeUid } } : {}treats an empty string the same as undefined. If a caller ever passes""by accident it would silently include the self-booking — considerexcludeUid != nullor explicit length check. Non-blocking given current call sites.Including
PENDINGalongsideACCEPTEDis intentional per the PR discussion (reschedule flow treats pending-with-confirmation as blocking), so no change needed there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/bookings/repositories/BookingRepository.ts` around lines 2141 - 2197, The conditional that builds the Prisma where clause in findByUserIdsAndDateRange treats an empty string excludeUid as a valid value; change the spread that currently uses "excludeUid ? { uid: { not: excludeUid } } : {}" to explicitly check for non-null/non-empty (for example excludeUid != null && excludeUid !== "" or excludeUid?.length > 0) so that an empty string won't be applied to the uid not filter; update the condition in BookingRepository.findByUserIdsAndDateRange accordingly.packages/features/bookings/repositories/BookingRepository.test.ts (1)
104-242: Good test coverage for the new methods.Early-return behavior,
ORcomposition foruserId/email branches,excludeUid, and select shape are all exercised. The combined-OR test at Line 176–198 also guards the predicate shape.Nit:
findByUserIdsAndDateRangededup-by-uid (repo Lines 2190–2196) isn't directly asserted — a test that stubsfindManyto return two rows with the sameuidand asserts a length-1 result would lock in that behavior. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/bookings/repositories/BookingRepository.test.ts` around lines 104 - 242, Add a test for BookingRepository.findByUserIdsAndDateRange that verifies deduplication by uid: stub mockPrismaClient.booking.findMany to resolve with two objects that have the same uid but otherwise identical/different fields, call repository.findByUserIdsAndDateRange (e.g., with userIds or userEmails and dateFrom/dateTo), and assert the returned array length is 1 and the single item matches the expected booking; target the BookingRepository.findByUserIdsAndDateRange code path that performs the dedupe (the repository method and mockPrismaClient.booking.findMany) so the test locks in the dedup-by-uid behavior.packages/trpc/server/routers/viewer/slots/util.ts (1)
813-829: Guest lookup runs on everycalculateHostsAndAvailabilitiescall, including fallback flows.
calculateHostsAndAvailabilitiesis invoked up to three times per request whenhasFallbackRRHoststriggers re-computation (see lines 1097, 1132, 1168). Each invocation firesgetGuestBusyTimesForReschedule, which issues 3 DB queries on the reschedule path. The date ranges differ across calls, so trivial caching isn't safe, but for the common case where only hosts/date-window varies and attendee resolution is identical, consider memoizing at least thefindByUidIncludeAttendeeEmails+findByEmailslookups (they don't depend ondateFrom/dateTo) to avoid redundant reads.Non-blocking; gracefully degrades.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/server/routers/viewer/slots/util.ts` around lines 813 - 829, calculateHostsAndAvailabilities repeatedly calls getGuestBusyTimesForReschedule causing duplicate DB reads; memoize the guest resolution steps (the parts that call findByUidIncludeAttendeeEmails and findByEmails) per-request so repeated getGuestBusyTimesForReschedule invocations reuse resolved attendee emails/ids even if date ranges differ. Add a lightweight request-scoped cache (e.g., a Map keyed by rescheduleUid+rescheduledBy) accessible to calculateHostsAndAvailabilities and used inside getGuestBusyTimesForReschedule (or pass the memo map as an extra param), store the results of findByUidIncludeAttendeeEmails and findByEmails there, and only run the date-dependent DB queries when needed. This keeps semantics but avoids redundant attendee lookups across the three recalculations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/modules/schedules/hooks/useEvent.ts`:
- Around line 107-117: The rescheduledBy query param is passed raw from
searchParams?.get("rescheduledBy") into useSchedule and can be an empty string
or malformed email which will fail server zod validation; normalize this value
before calling useSchedule by converting empty string to null and perform a
lightweight client-side email shape check (e.g., simple regex or indexOf('@')
sanity check) to only pass a non-empty string that looks like an email,
otherwise pass null; update the variable referenced as rescheduledBy and ensure
useSchedule(...) is invoked with that normalized value so bad query params don't
break the schedule query.
In
`@packages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.ts`:
- Around line 204-223: The test only asserts
mockDependencies.userRepo.findByEmails was called, which doesn't prove the
host-branch executed; update the "should handle case-insensitive host email
comparison" test (which calls callGetGuestBusyTimes with rescheduleUid and
rescheduledBy) to also assert
mockDependencies.bookingRepo.findByUserIdsAndDateRange was called (or
alternately assert the test did not early-return by checking a side-effect
unique to the host path) so the case-insensitive host matching flow
(rescheduledBy -> userRepo.findByEmails ->
bookingRepo.findByUserIdsAndDateRange) is fully verified.
In `@packages/trpc/server/routers/viewer/slots/util.ts`:
- Around line 686-692: The gating logic currently treats any rescheduledBy value
that is not equal to hostEmail as an attendee-initiated reschedule and returns
[] (see rescheduledBy, original.user?.email, hostEmail, isHostReschedule), which
erroneously excludes host-adjacent initiators; change the check to explicitly
detect whether rescheduledBy matches an attendee email from the slot's attendee
list (only return [] when rescheduledBy is equal to an attendee email), and
ensure you defensively handle original.user being null/undefined when computing
hostEmail; alternatively, if the current behavior is intentional, add a clear
comment above this block explaining that non-host/non-attendee initiators should
fall through.
---
Outside diff comments:
In `@packages/features/availability/lib/getUserAvailability.ts`:
- Around line 621-638: guestBusyTimesFormatted is declared as EventBusyDetails[]
but only sets source when withSource is true, causing a type mismatch; always
set source: "guest-availability" on each guestBusyTimesFormatted entry (matching
busyTimes/busyTimesFromLimits/busyTimesFromTeamLimits) and rely on the existing
later strip step to remove source when withSource is false so types remain
consistent and the runtime behavior is unchanged.
---
Nitpick comments:
In `@packages/features/bookings/repositories/BookingRepository.test.ts`:
- Around line 104-242: Add a test for
BookingRepository.findByUserIdsAndDateRange that verifies deduplication by uid:
stub mockPrismaClient.booking.findMany to resolve with two objects that have the
same uid but otherwise identical/different fields, call
repository.findByUserIdsAndDateRange (e.g., with userIds or userEmails and
dateFrom/dateTo), and assert the returned array length is 1 and the single item
matches the expected booking; target the
BookingRepository.findByUserIdsAndDateRange code path that performs the dedupe
(the repository method and mockPrismaClient.booking.findMany) so the test locks
in the dedup-by-uid behavior.
In `@packages/features/bookings/repositories/BookingRepository.ts`:
- Around line 2141-2197: The conditional that builds the Prisma where clause in
findByUserIdsAndDateRange treats an empty string excludeUid as a valid value;
change the spread that currently uses "excludeUid ? { uid: { not: excludeUid } }
: {}" to explicitly check for non-null/non-empty (for example excludeUid != null
&& excludeUid !== "" or excludeUid?.length > 0) so that an empty string won't be
applied to the uid not filter; update the condition in
BookingRepository.findByUserIdsAndDateRange accordingly.
In `@packages/trpc/server/routers/viewer/slots/util.ts`:
- Around line 813-829: calculateHostsAndAvailabilities repeatedly calls
getGuestBusyTimesForReschedule causing duplicate DB reads; memoize the guest
resolution steps (the parts that call findByUidIncludeAttendeeEmails and
findByEmails) per-request so repeated getGuestBusyTimesForReschedule invocations
reuse resolved attendee emails/ids even if date ranges differ. Add a lightweight
request-scoped cache (e.g., a Map keyed by rescheduleUid+rescheduledBy)
accessible to calculateHostsAndAvailabilities and used inside
getGuestBusyTimesForReschedule (or pass the memo map as an extra param), store
the results of findByUidIncludeAttendeeEmails and findByEmails there, and only
run the date-dependent DB queries when needed. This keeps semantics but avoids
redundant attendee lookups across the three recalculations.
🪄 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: d0634495-fb7e-4302-9d11-b1285e72880b
📒 Files selected for processing (10)
apps/web/modules/schedules/hooks/useEvent.tsapps/web/modules/schedules/hooks/useSchedule.tspackages/features/availability/lib/getUserAvailability.tspackages/features/bookings/repositories/BookingRepository.test.tspackages/features/bookings/repositories/BookingRepository.tspackages/features/users/repositories/UserRepository.test.tspackages/features/users/repositories/UserRepository.tspackages/trpc/server/routers/viewer/slots/getGuestBusyTimesForReschedule.test.tspackages/trpc/server/routers/viewer/slots/types.tspackages/trpc/server/routers/viewer/slots/util.ts
| if (rescheduledBy) { | ||
| const hostEmail = original.user?.email; | ||
| const isHostReschedule = hostEmail && rescheduledBy.toLowerCase() === hostEmail.toLowerCase(); | ||
| if (!isHostReschedule) { | ||
| return []; | ||
| } | ||
| } |
There was a problem hiding this comment.
Gating logic treats any non-host rescheduledBy as "attendee-initiated".
The check rescheduledBy.toLowerCase() === hostEmail.toLowerCase() returns [] when rescheduledBy is anything other than the host email — including admin/team-owner reschedules, delegated-credential flows, or edge cases where original.user is null (making hostEmail undefined, which collapses to isHostReschedule = false and thus silently skips guest blocking).
If the intent is strictly "not attendee", consider matching against the attendee list explicitly (return [] only when rescheduledBy matches an attendee) so host-adjacent initiators still benefit from guest blocking. Otherwise, a brief comment documenting that "non-host, non-attendee" initiators intentionally fall through to no-op would help future readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/trpc/server/routers/viewer/slots/util.ts` around lines 686 - 692,
The gating logic currently treats any rescheduledBy value that is not equal to
hostEmail as an attendee-initiated reschedule and returns [] (see rescheduledBy,
original.user?.email, hostEmail, isHostReschedule), which erroneously excludes
host-adjacent initiators; change the check to explicitly detect whether
rescheduledBy matches an attendee email from the slot's attendee list (only
return [] when rescheduledBy is equal to an attendee email), and ensure you
defensively handle original.user being null/undefined when computing hostEmail;
alternatively, if the current behavior is intentional, add a clear comment above
this block explaining that non-host/non-attendee initiators should fall through.
…uestBusyTimesFormatted typing Two regressions surfaced by local type-check: - util.ts imported a now-removed RoutingFormResponseRepository type; the field that referenced it was refactored out of IAvailableSlotsService in upstream but the import lingered in the rebased branch (TS2307). - guestBusyTimesFormatted used a conditional spread for source which made the object shape incompatible with EventBusyDetails's required source. Cast the mapped array to EventBusyDetails[] so the conditional spread is accepted (TS2322).
Revert the conditional-spread approach and always set source to guest-availability. The existing strip step in detailedBusyTimes already drops source when withSource is false, and this keeps the array shape consistent with the sibling busyTimes/busyTimesFromLimits/busyTimesFromTeamLimits arrays.
…-by-uid - findByUserIdsAndDateRange: use excludeUid != null && excludeUid !== "" so an accidental empty string no longer silently includes the self-booking - Two new tests: assert empty excludeUid is ignored, and assert the OR/uid dedup collapses identical rows to one result
Server zod now rejects non-email values for rescheduledBy; drop empty / at-less URL strings to null client-side so stale or malformed URLs do not break the schedule query.
…nch assertion - util.ts: explicit comment on the gating semantics (default-safe-for-attendee when the initiator cannot be positively identified as host) - test: assert findByUserIdsAndDateRange was invoked in the case-insensitive host test, so an accidental early-return would fail loudly
|
Round 4 follow-through.
On the gating semantics: Current behaviour is "default-safe-for-attendee": when On the memoization nit ( Acknowledged as a real perf win for the 3x Local @coderabbitai — please re-review. |
|
Only repository collaborators, contributors, or members can run CodeRabbit commands. |
3 participants
/claim #16378
What does this PR do?
When a host reschedules a booking, the system now checks whether attendees are Cal.com users and fetches their busy times. Only mutually available slots are shown in the reschedule calendar.
Before: Host could freely pick any time when rescheduling, even if the guest was busy.
After: Guest's existing bookings are fetched and blocked automatically.
How it works
BookingRepository.findByUidIncludeAttendeeEmails()— gets attendee emails from the original bookingUserRepository.findByEmails()— resolves emails to Cal.com users (primary + verified secondary emails, case-insensitive, deduplicated viaPromise.all)BookingRepository.findByUserIdsAndDateRange()— fetches guest bookings in the date range, withexcludeUidfiltering at the database levelScope
Per @CarinaWolli's clarification: this applies only when the event type owner reschedules. If attendees reschedule, all slots are shown.
[]on failure, never blocks rescheduling)Demo
Tests
30 tests passing (3 new test files, 631 lines added):
UserRepository.findByEmails— primary/secondary email lookup, dedup, normalization (6 tests)BookingRepository.findByUserIdsAndDateRange— userId/email query, excludeUid, empty input (6 tests)BookingRepository.findByUidIncludeAttendeeEmails— uid lookup, null handling (2 tests)_getGuestBusyTimesForReschedule— early exits, busy time collection, multi-guest, error handling (13 tests)Self-review notes
select(notinclude) per cal.com conventionswithReportingwrapper for observabilityPromise.allfor zero added latencyexcludeUidfiltering at database level (not JS post-filter)