fix: use correct videoCallUrl per recurring booking occurrence#28932
fix: use correct videoCallUrl per recurring booking occurrence#28932ashif323 wants to merge 3 commits intocalcom:mainfrom
Conversation
|
Welcome to Cal.diy, @ashif323! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
📝 WalkthroughWalkthroughThe pull request refactors video call URL handling for recurring bookings across test expectations and webhook dispatch logic. Test assertions were updated to expect fixed mock URLs instead of deriving them from booking UIDs. In the confirmation handler, per-occurrence video call URLs are now computed from schedule references and stored in each booking's metadata, with BOOKING_CREATED webhooks dispatched separately per occurrence using occurrence-specific room URLs. The RegularBookingService was modified to source metadata video call URLs from pre-computed values rather than re-parsing from calendar event data, ensuring consistent URL propagation through the booking workflow. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/features/bookings/lib/handleConfirmation.ts (2)
451-457:⚠️ Potential issue | 🟠 MajorUse each updated booking’s video URL for workflows.
Line 456 still passes the shared
meetingUrl, so reminders/workflows for later recurring occurrences can get the wrong room even after per-booking metadata is updated. PreferupdatedBookings[index].metadata.videoCallUrl, withmeetingUrlonly as fallback.Suggested direction
+ const bookingMetadata = updatedBookings[index].metadata; + const bookingMeetingUrl = + bookingMetadata && + typeof bookingMetadata === "object" && + "videoCallUrl" in bookingMetadata && + typeof bookingMetadata.videoCallUrl === "string" + ? bookingMetadata.videoCallUrl + : meetingUrl; const evtOfBooking = { ...evt, rescheduleReason: updatedBookings[index].cancellationReason || null, - metadata: { videoCallUrl: meetingUrl }, + metadata: bookingMeetingUrl ? { videoCallUrl: bookingMeetingUrl } : {},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/bookings/lib/handleConfirmation.ts` around lines 451 - 457, The evtOfBooking object is using the shared meetingUrl for metadata, causing later occurrences to get the wrong room; update the metadata assignment in the loop that builds evtOfBooking to use the per-booking URL updatedBookings[index].metadata.videoCallUrl and fall back to meetingUrl when that field is missing (reference updatedBookings, evtOfBooking, meetingUrl, and metadata.videoCallUrl).
278-299:⚠️ Potential issue | 🟠 MajorMove occurrence URL selection into the recurring update loop.
Line 280 and Line 283 compute
videoReference/occurrenceMeetingUrlonce, then Line 298 writes that same URL to every recurring booking. That keeps the shared-URL failure mode for adapters that produce per-occurrence references. Derive the URL for the currentrecurringBookinginside the map, using the occurrence’s reference or the reference index if that is theEventManagercontract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/features/bookings/lib/handleConfirmation.ts` around lines 278 - 299, The code currently computes videoReference and occurrenceMeetingUrl once before mapping unconfirmedRecurringBookings, causing the same URL to be written to every recurring booking; instead, move the logic that derives the occurrenceMeetingUrl into the map callback so each recurringBooking gets its own URL: inside the unconfirmedRecurringBookings.map callback (where recurringBooking is available), pick the per-occurrence reference from scheduleResult.referencesToCreate for that recurringBooking (or use the reference index per the EventManager contract) to derive videoReference and set occurrenceMeetingUrl, then use that occurrenceMeetingUrl in the prisma.booking.update data (metadata.videoCallUrl) and when creating references so each occurrence receives its correct URL.apps/web/pages/api/book/recurring-event.test.ts (1)
113-119:⚠️ Potential issue | 🟡 MinorMake the regression test use distinct meeting URLs per occurrence.
All three updated assertions still expect
meeting-1, and the mock also returnsmeeting-1for every booking. That would not catch a regression where recurring occurrences all receive one shared adapter URL. Please make the mock return per-call URLs, e.g.meeting-1,meeting-2, etc., and assert each booking/webhook receives the matching occurrence URL.Also applies to: 200-207, 463-469, 549-556, 681-687, 767-774
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/pages/api/book/recurring-event.test.ts` around lines 113 - 119, The test currently uses mockSuccessfulVideoMeetingCreation with a fixed url `meeting-1` for every call causing all occurrences to appear to share one adapter URL; change the mock used in the recurring-event.test to generate distinct per-call URLs (e.g., interpolate a call counter into the url: `meeting-1`, `meeting-2`, ...) in mockSuccessfulVideoMeetingCreation and update the corresponding assertions that check the created booking/webhook URLs so each occurrence asserts against its expected per-call URL (reference the mockSuccessfulVideoMeetingCreation invocation and the booking/webhook assertion blocks that verify the meeting url for each occurrence).
🤖 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/lib/handleConfirmation.ts`:
- Around line 592-609: The payload construction is reusing
evtWithoutAssignmentReason which leaves the original event's occurrence fields
(uid, startTime, endTime) on every webhook; update the payload so those
per-occurrence fields come from the current updatedBooking instead: when
building payload in the updatedBookings.flatMap loop (symbols: updatedBooking,
evtWithoutAssignmentReason, payload), explicitly set uid, startTime, and endTime
(or override them after spreading evtWithoutAssignmentReason) using
updatedBooking.uid, updatedBooking.startTime, and updatedBooking.endTime so each
payload reflects the correct occurrence for that bookingId.
---
Outside diff comments:
In `@apps/web/pages/api/book/recurring-event.test.ts`:
- Around line 113-119: The test currently uses
mockSuccessfulVideoMeetingCreation with a fixed url `meeting-1` for every call
causing all occurrences to appear to share one adapter URL; change the mock used
in the recurring-event.test to generate distinct per-call URLs (e.g.,
interpolate a call counter into the url: `meeting-1`, `meeting-2`, ...) in
mockSuccessfulVideoMeetingCreation and update the corresponding assertions that
check the created booking/webhook URLs so each occurrence asserts against its
expected per-call URL (reference the mockSuccessfulVideoMeetingCreation
invocation and the booking/webhook assertion blocks that verify the meeting url
for each occurrence).
In `@packages/features/bookings/lib/handleConfirmation.ts`:
- Around line 451-457: The evtOfBooking object is using the shared meetingUrl
for metadata, causing later occurrences to get the wrong room; update the
metadata assignment in the loop that builds evtOfBooking to use the per-booking
URL updatedBookings[index].metadata.videoCallUrl and fall back to meetingUrl
when that field is missing (reference updatedBookings, evtOfBooking, meetingUrl,
and metadata.videoCallUrl).
- Around line 278-299: The code currently computes videoReference and
occurrenceMeetingUrl once before mapping unconfirmedRecurringBookings, causing
the same URL to be written to every recurring booking; instead, move the logic
that derives the occurrenceMeetingUrl into the map callback so each
recurringBooking gets its own URL: inside the unconfirmedRecurringBookings.map
callback (where recurringBooking is available), pick the per-occurrence
reference from scheduleResult.referencesToCreate for that recurringBooking (or
use the reference index per the EventManager contract) to derive videoReference
and set occurrenceMeetingUrl, then use that occurrenceMeetingUrl in the
prisma.booking.update data (metadata.videoCallUrl) and when creating references
so each occurrence receives its correct URL.
🪄 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: fb21402b-2b71-4013-9528-f65d628be70d
📒 Files selected for processing (3)
apps/web/pages/api/book/recurring-event.test.tspackages/features/bookings/lib/handleConfirmation.tspackages/features/bookings/lib/service/RegularBookingService.ts
| const promises = updatedBookings.flatMap((updatedBooking) => { | ||
| const bookingMeetingUrl = | ||
| updatedBooking.metadata && | ||
| typeof updatedBooking.metadata === "object" && | ||
| "videoCallUrl" in updatedBooking.metadata | ||
| ? (updatedBooking.metadata as { videoCallUrl: string }).videoCallUrl | ||
| : meetingUrl; | ||
|
|
||
| const payload: EventPayloadType = { | ||
| ...evtWithoutAssignmentReason, | ||
| ...eventTypeInfo, | ||
| bookingId: updatedBooking.id, | ||
| eventTypeId: eventType?.id, | ||
| status: "ACCEPTED", | ||
| smsReminderNumber: booking.smsReminderNumber || undefined, | ||
| metadata: bookingMeetingUrl ? { videoCallUrl: bookingMeetingUrl } : {}, | ||
| ...(platformClientParams ? platformClientParams : {}), | ||
| }; |
There was a problem hiding this comment.
Override occurrence fields in each webhook payload.
The loop sends one payload per updatedBooking, but spreading evtWithoutAssignmentReason leaves the original event’s uid, startTime, and endTime on every payload. Consumers can receive different bookingIds with the same occurrence details. Set the per-occurrence fields from updatedBooking.
Suggested fix
const payload: EventPayloadType = {
...evtWithoutAssignmentReason,
+ uid: updatedBooking.uid,
+ startTime: updatedBooking.startTime.toISOString(),
+ endTime: updatedBooking.endTime.toISOString(),
+ title: updatedBooking.title,
...eventTypeInfo,
bookingId: updatedBooking.id,
eventTypeId: eventType?.id,
status: "ACCEPTED",
- smsReminderNumber: booking.smsReminderNumber || undefined,
+ smsReminderNumber: updatedBooking.smsReminderNumber || undefined,
metadata: bookingMeetingUrl ? { videoCallUrl: bookingMeetingUrl } : {},
...(platformClientParams ? platformClientParams : {}),
};📝 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 promises = updatedBookings.flatMap((updatedBooking) => { | |
| const bookingMeetingUrl = | |
| updatedBooking.metadata && | |
| typeof updatedBooking.metadata === "object" && | |
| "videoCallUrl" in updatedBooking.metadata | |
| ? (updatedBooking.metadata as { videoCallUrl: string }).videoCallUrl | |
| : meetingUrl; | |
| const payload: EventPayloadType = { | |
| ...evtWithoutAssignmentReason, | |
| ...eventTypeInfo, | |
| bookingId: updatedBooking.id, | |
| eventTypeId: eventType?.id, | |
| status: "ACCEPTED", | |
| smsReminderNumber: booking.smsReminderNumber || undefined, | |
| metadata: bookingMeetingUrl ? { videoCallUrl: bookingMeetingUrl } : {}, | |
| ...(platformClientParams ? platformClientParams : {}), | |
| }; | |
| const promises = updatedBookings.flatMap((updatedBooking) => { | |
| const bookingMeetingUrl = | |
| updatedBooking.metadata && | |
| typeof updatedBooking.metadata === "object" && | |
| "videoCallUrl" in updatedBooking.metadata | |
| ? (updatedBooking.metadata as { videoCallUrl: string }).videoCallUrl | |
| : meetingUrl; | |
| const payload: EventPayloadType = { | |
| ...evtWithoutAssignmentReason, | |
| uid: updatedBooking.uid, | |
| startTime: updatedBooking.startTime.toISOString(), | |
| endTime: updatedBooking.endTime.toISOString(), | |
| title: updatedBooking.title, | |
| ...eventTypeInfo, | |
| bookingId: updatedBooking.id, | |
| eventTypeId: eventType?.id, | |
| status: "ACCEPTED", | |
| smsReminderNumber: updatedBooking.smsReminderNumber || undefined, | |
| metadata: bookingMeetingUrl ? { videoCallUrl: bookingMeetingUrl } : {}, | |
| ...(platformClientParams ? platformClientParams : {}), | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/features/bookings/lib/handleConfirmation.ts` around lines 592 - 609,
The payload construction is reusing evtWithoutAssignmentReason which leaves the
original event's occurrence fields (uid, startTime, endTime) on every webhook;
update the payload so those per-occurrence fields come from the current
updatedBooking instead: when building payload in the updatedBookings.flatMap
loop (symbols: updatedBooking, evtWithoutAssignmentReason, payload), explicitly
set uid, startTime, and endTime (or override them after spreading
evtWithoutAssignmentReason) using updatedBooking.uid, updatedBooking.startTime,
and updatedBooking.endTime so each payload reflects the correct occurrence for
that bookingId.
Previously, two bugs caused all recurring bookings to share the same videoCallUrl: 1. RegularBookingService.ts: metadata was built using getVideoCallUrlFromCalEvent(evt) which for daily_video always returns the public Cal Video URL (derived from the first booking uid) instead of the actual meeting URL from the video adapter. Fix: use the already-correct videoCallUrl directly. 2. handleConfirmation.ts: the recurring confirmation flow computed a single meetingUrl before the loop and stamped it on every occurrence. Fix: derive occurrenceMeetingUrl from scheduleResult.referencesToCreate so each occurrence gets the correct URL from its video reference. Removes 3 FIXME comments from recurring-event.test.ts and updates assertions to verify correct per-occurrence video URLs. Fixes calcom#28871
ashif323
force-pushed
the
fix/recurring-booking-unique-video-url
branch
from
07ce76f to
2c7a04f
Compare
|
This PR has been marked as stale due to inactivity. If you're still working on it or need any help, please let us know or update the PR to keep it active. |
|
Hi, In handleConfirmation, the BOOKING_CREATED webhook code now returns a map of payload promises but still calls Promise.all(promises) even though promises is no longer defined, causing a runtime/compile failure in this path. Severity: action required | Category: correctness How to fix: Restore promises array definition Agent prompt to fix - you can give this to your LLM of choice:
Found by Qodo code review |
|
Hi, In recurring confirmation, Severity: remediation recommended | Category: correctness How to fix: Select video reference explicitly Agent prompt to fix - you can give this to your LLM of choice:
Found by Qodo code review |
|
|
3 participants
What does this PR do?
Fixes two bugs that caused all recurring bookings to share the same
videoCallUrl.Root causes
Bug 1 —
RegularBookingService.ts(new booking flow):getVideoCallUrlFromCalEvent(evt)was called when building booking metadata. Fordaily_video, this function always returns the public Cal Video URL derived from the booking uid (/video/<uid>) instead of the actual meeting URL from the video adapter. All occurrences ended up with the first booking's uid in their URL.Fix: use the already-correct
videoCallUrlvariable directly, which is populated fromgetVideoCallDetails()→updatedVideoEvent?.url.Bug 2 —
handleConfirmation.ts(confirm flow):A single
meetingUrlwas computed once before the recurring booking loop and stamped onto every occurrence's metadata.Fix: derive
occurrenceMeetingUrlfromscheduleResult.referencesToCreateso each occurrence gets the correct URL from its video reference.Changes
packages/features/bookings/lib/service/RegularBookingService.ts— usevideoCallUrldirectly in metadata instead of re-callinggetVideoCallUrlFromCalEventpackages/features/bookings/lib/handleConfirmation.ts— derive meeting URL from video reference per occurrence in recurring confirmation loopapps/web/pages/api/book/recurring-event.test.ts— remove 3FIXMEcomments, update assertions to verify correct video URLsTesting
All recurring booking tests pass: