feat: implement declarative calendar event handling in reschedule #22660
Conversation
- Fix EventManager.reschedule to create new calendar events when none exist - Add comprehensive tests for missing and empty calendar event UID scenarios - Ensure valid calendar event always exists after rescheduling - Update booking references with newly created calendar event information Fixes the issue where rescheduling fails if original Google Calendar event is missing Co-Authored-By: Devanshu Sharma <devanshusharma658@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis set of changes updates the booking rescheduling logic and its associated tests. In the Estimated code review effort3 (~45 minutes) Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
✅ No security or compliance issues detected. Reviewed everything up to 8f2915c. Security Overview
Detected Code Changes
Reply to this PR with |
- Add mockCalendarToHaveNoBusySlots calls with proper create parameters to tests that use Google Calendar - Fixes 'Unimplemented appStoreMock' errors in CI - Ensures tests that trigger createAllCalendarEvents have proper mocking Co-Authored-By: Devanshu Sharma <devanshusharma658@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/21/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/21/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/lib/EventManager.ts (3)
600-604: Update condition evaluation order for better readability.The condition includes the new
createdNewCalendarEventsflag appropriately, but consider reordering for logical flow.const shouldUpdateBookingReferences = - !!changedOrganizer || - isLocationChanged || - !!isBookingRequestedReschedule || - isDailyVideoRoomExpired || - createdNewCalendarEvents; + !!changedOrganizer || + isLocationChanged || + !!isBookingRequestedReschedule || + isDailyVideoRoomExpired || + createdNewCalendarEvents;
652-679: Verify the new calendar event creation logic and address the static analysis hint.The implementation correctly handles the case where no valid calendar event exists by creating new events and updating booking references. However, there's a static analysis warning about optional chaining on line 652.
Apply this fix for the optional chaining issue:
-if (bookingCalendarReference && bookingCalendarReference.uid) { +if (bookingCalendarReference?.uid) {Additionally, verify that the
externalIdproperty mapping is correct on line 669. The interface discrepancy betweenexternalCalendarIdandexternalIdmentioned in the PR objectives should be confirmed:// Line 669: Ensure this maps correctly externalCalendarId: result.externalId || null,
702-713: Ensure consistent variable naming and logic.The variable is renamed from
shouldUpdateBookingReferencestofinalShouldUpdateBookingReferenceswhich is redundant. The logic is correct but can be simplified.Consider simplifying this:
-const finalShouldUpdateBookingReferences = - !!changedOrganizer || - isLocationChanged || - !!isBookingRequestedReschedule || - isDailyVideoRoomExpired || - createdNewCalendarEvents; return { results, - referencesToCreate: finalShouldUpdateBookingReferences + referencesToCreate: shouldUpdateBookingReferences ? updatedBookingReferences : [...booking.references], };Since
shouldUpdateBookingReferencesis already calculated above and includescreatedNewCalendarEvents, the duplication is unnecessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/features/bookings/lib/handleNewBooking/test/fresh-booking.test.ts(5 hunks)packages/features/bookings/lib/handleNewBooking/test/reschedule.test.ts(2 hunks)packages/lib/EventManager.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.256Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
packages/features/bookings/lib/handleNewBooking/test/fresh-booking.test.ts (1)
Learnt from: vijayraghav-io
PR: #21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.370Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
packages/lib/EventManager.ts (2)
Learnt from: vijayraghav-io
PR: #21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.370Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: vijayraghav-io
PR: #21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.256Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The upsertSelectedCalendarsForEventTypeIds method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like @@unique([outlookSubscriptionId, eventTypeId]) should not be applied as they would prevent this intended functionality.
packages/features/bookings/lib/handleNewBooking/test/reschedule.test.ts (2)
Learnt from: vijayraghav-io
PR: #21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.370Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: vijayraghav-io
PR: #21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.256Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The upsertSelectedCalendarsForEventTypeIds method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like @@unique([outlookSubscriptionId, eventTypeId]) should not be applied as they would prevent this intended functionality.
🧬 Code Graph Analysis (1)
packages/features/bookings/lib/handleNewBooking/test/fresh-booking.test.ts (1)
apps/web/test/utils/bookingScenario/bookingScenario.ts (1)
mockCalendarToHaveNoBusySlots(1879-1892)
🪛 Biome (1.9.4)
packages/lib/EventManager.ts
[error] 652-652: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (9)
packages/features/bookings/lib/handleNewBooking/test/fresh-booking.test.ts (5)
2434-2439: LGTM! Enhanced calendar mock for better test coverage.This change improves the mock to provide more realistic calendar event creation responses with both
uidandidproperties, supporting the new rescheduling logic that handles missing calendar event UIDs.
2573-2578: LGTM! Consistent mock enhancement for calendar event creation.The addition of mock identifiers aligns with the enhanced rescheduling functionality and provides better test coverage for scenarios involving calendar event handling.
2672-2677: LGTM! Mock enhancement supports new rescheduling scenarios.This change maintains consistency with other test updates by providing mock calendar event identifiers, which is essential for testing the new declarative calendar event handling logic.
2908-2913: LGTM! Enhanced calendar mock for paid events testing.The addition of mock calendar event identifiers ensures comprehensive testing of the new rescheduling functionality across different booking scenarios, including paid events.
3069-3074: LGTM! Complete mock enhancement for comprehensive test coverage.This final update completes the consistent enhancement of calendar mocks across all test scenarios, ensuring the new rescheduling logic that handles missing calendar event UIDs is properly tested.
packages/lib/EventManager.ts (1)
590-590: LGTM: Clear flag initialization.The
createdNewCalendarEventsflag is properly initialized to track when new calendar events are created during the reschedule process.packages/features/bookings/lib/handleNewBooking/test/reschedule.test.ts (3)
35-35: LGTM: Import statement correctly addedThe import for
expectSuccessfulCalendarEventCreationInCalendaris properly added to support the new test cases that verify calendar event creation.
2831-3024: Excellent test coverage for missing calendar event UID scenarioThis test thoroughly validates the declarative calendar event handling approach when rescheduling a booking that lacks a calendar event reference. Key strengths:
- Properly simulates a booking with only video meeting reference (no calendar reference)
- Correctly mocks calendar event creation with realistic return values
- Verifies that new calendar events are created during rescheduling
- Confirms booking references are updated with new calendar event information
- Validates all expected side effects (emails, webhooks, database state)
The test implementation aligns perfectly with the PR objectives for handling missing calendar events.
3026-3141: Well-designed test for empty calendar event UID edge caseThis test effectively covers the scenario where a booking has a calendar reference with empty UID values. Notable aspects:
- Accurately simulates the edge case with empty uid, meetingId, meetingPassword, and meetingUrl
- Appropriately uses "inPerson" location type for this test scenario
- Correctly sets
videoCallUrl: nullfor in-person meetings- Properly verifies that new calendar events are created when existing ones have invalid/empty UIDs
- Validates that booking references are updated with new calendar event information
This test complements the first one by covering a different but related edge case in the declarative calendar event handling.
packages/lib/EventManager.ts
Outdated
| log.debug("No valid calendar event found for booking, creating new calendar events"); | ||
| const createdEvents = await this.createAllCalendarEvents(evt); | ||
| results.push(...createdEvents); | ||
|
|
||
| // Update booking references with newly created calendar events | ||
| const calendarReferences = createdEvents | ||
| .filter((result) => result.type.includes("_calendar") && result.success) | ||
| .map((result) => ({ | ||
| type: result.type, | ||
| uid: result.uid, | ||
| meetingId: result.uid, | ||
| meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "", | ||
| meetingUrl: result.originalEvent.videoCallData?.url || "", | ||
| externalCalendarId: result.externalId || null, | ||
| credentialId: result.credentialId || null, | ||
| })); | ||
|
|
||
| // Preserve existing non-calendar references and add new calendar references | ||
| const existingNonCalendarReferences = booking.references.filter( | ||
| (ref) => !ref.type.includes("_calendar") | ||
| ); | ||
| updatedBookingReferences.push(...existingNonCalendarReferences, ...calendarReferences); | ||
| createdNewCalendarEvents = calendarReferences.length > 0; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error handling for calendar event creation failures.
While the logic correctly handles missing calendar events by creating new ones, consider what happens if createAllCalendarEvents() fails or returns empty results.
Please verify the error handling behavior when calendar event creation fails:
🏁 Script executed:
#!/bin/bash
# Search for error handling patterns in createAllCalendarEvents method
ast-grep --pattern $'createAllCalendarEvents($_) {
$$$
}'
# Look for how calendar creation failures are handled
rg -A 10 -B 5 "createAllCalendarEvents.*catch|try.*createAllCalendarEvents"Length of output: 138
🏁 Script executed:
#!/bin/bash
# Locate definition(s) of createAllCalendarEvents in the repo
rg -n "createAllCalendarEvents" --color=never
# Show the full implementation of createAllCalendarEvents in EventManager.ts
rg -n "createAllCalendarEvents" -A 20 -B 5 packages/lib/EventManager.ts
# Check for any try/catch blocks wrapping createAllCalendarEvents calls in EventManager.ts
rg -n "createAllCalendarEvents" -C 5 packages/lib/EventManager.ts | grep -E "try|catch"Length of output: 4487
#!/bin/bash
# Display the full body of createAllCalendarEvents for inspection
sed -n '813,950p' packages/lib/EventManager.tsAdd robust error handling around calendar event creation
The createAllCalendarEvents method currently invokes createEvent (and related calls) without any try/catch, so any exception will bubble up and potentially abort the booking flow. To prevent a single provider failure from breaking the entire process, please:
• In packages/lib/EventManager.ts within the private async createAllCalendarEvents(event: CalendarEvent) method (around line 813), wrap each external call—especially createEvent(credential, event)—in a try/catch.
• Inside the catch, log the error (using log.error or log.warn), include context (credential ID, event ID), and either continue to the next credential or record a failure EventResult so downstream logic can handle partial successes.
• Ensure the method still returns a complete array of EventResult<NewCalendarEventType> reflecting both successes and failures, rather than throwing.
This will make the booking flow more resilient if a calendar provider is temporarily unavailable or misconfigured.
🤖 Prompt for AI Agents
In packages/lib/EventManager.ts around line 813, the createAllCalendarEvents
method calls createEvent without error handling, risking the entire booking flow
if a provider fails. Wrap each call to createEvent inside a try/catch block, log
any caught errors with context including credential ID and event ID using
log.error or log.warn, and ensure the method continues processing remaining
credentials. Modify the method to return an array of EventResult objects that
include both successful and failed attempts, so downstream logic can handle
partial successes without throwing exceptions.
packages/lib/EventManager.ts
Outdated
| const calendarReferences = createdEvents | ||
| .filter((result) => result.type.includes("_calendar") && result.success) | ||
| .map((result) => ({ | ||
| type: result.type, | ||
| uid: result.uid, | ||
| meetingId: result.uid, | ||
| meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "", | ||
| meetingUrl: result.originalEvent.videoCallData?.url || "", | ||
| externalCalendarId: result.externalId || null, | ||
| credentialId: result.credentialId || null, | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review the booking reference construction logic.
The mapping from created events to booking references looks correct, but the meetingPassword and meetingUrl assignments seem questionable for calendar events.
The meetingPassword field is being populated with timezone information and meetingUrl with video call data, which doesn't make sense for calendar references:
const calendarReferences = createdEvents
.filter((result) => result.type.includes("_calendar") && result.success)
.map((result) => ({
type: result.type,
uid: result.uid,
meetingId: result.uid,
- meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "",
- meetingUrl: result.originalEvent.videoCallData?.url || "",
+ meetingPassword: undefined,
+ meetingUrl: undefined,
externalCalendarId: result.externalId || null,
credentialId: result.credentialId || null,
}));These fields should be null or undefined for calendar events, or properly mapped if they have different meanings in this context.
📝 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 calendarReferences = createdEvents | |
| .filter((result) => result.type.includes("_calendar") && result.success) | |
| .map((result) => ({ | |
| type: result.type, | |
| uid: result.uid, | |
| meetingId: result.uid, | |
| meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "", | |
| meetingUrl: result.originalEvent.videoCallData?.url || "", | |
| externalCalendarId: result.externalId || null, | |
| credentialId: result.credentialId || null, | |
| })); | |
| const calendarReferences = createdEvents | |
| .filter((result) => result.type.includes("_calendar") && result.success) | |
| .map((result) => ({ | |
| type: result.type, | |
| uid: result.uid, | |
| meetingId: result.uid, | |
| meetingPassword: undefined, | |
| meetingUrl: undefined, | |
| externalCalendarId: result.externalId || null, | |
| credentialId: result.credentialId || null, | |
| })); |
🤖 Prompt for AI Agents
In packages/lib/EventManager.ts around lines 661 to 671, the meetingPassword and
meetingUrl fields are incorrectly assigned timezone and video call URL data for
calendar events. Update the mapping logic so that for calendarReferences,
meetingPassword and meetingUrl are set to null or undefined, reflecting that
these fields do not apply to calendar events.
…dule-declarative-1753086185
packages/lib/EventManager.ts
Outdated
| log.debug("No valid calendar event found for booking, creating new calendar events"); | ||
| const createdEvents = await this.createAllCalendarEvents(evt); | ||
| results.push(...createdEvents); | ||
|
|
||
| // Update booking references with newly created calendar events | ||
| const calendarReferences = createdEvents | ||
| .filter((result) => result.type.includes("_calendar") && result.success) | ||
| .map((result) => ({ | ||
| type: result.type, | ||
| uid: result.uid, | ||
| meetingId: result.uid, | ||
| meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "", | ||
| meetingUrl: result.originalEvent.videoCallData?.url || "", | ||
| externalCalendarId: result.externalId || null, | ||
| credentialId: result.credentialId || null, | ||
| })); |
There was a problem hiding this comment.
The reschedule fn is already doing a lot of things violating Single Responsibility principle. Let's do this handling in createMissingCalendarEvents or handleMissingCalendarEvents
packages/lib/EventManager.ts
Outdated
| @@ -596,7 +597,11 @@ export default class EventManager { | |||
| } | |||
|
|
|||
| const shouldUpdateBookingReferences = | |||
There was a problem hiding this comment.
This variable seems to be unused now.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/lib/server/service/bookingReference.ts (1)
9-10: Review the field mapping logic for calendar events.The
meetingPasswordandmeetingUrlfields are populated with timezone and video call data respectively. For calendar references, these fields should likely benullorundefinedsince they don't apply to calendar events:return { type: result.type, uid: result.uid, meetingId: result.uid, - meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "", - meetingUrl: result.originalEvent.videoCallData?.url || "", + meetingPassword: result.type.includes("_calendar") ? null : result.originalEvent.attendees?.[0]?.timeZone || "", + meetingUrl: result.type.includes("_calendar") ? null : result.originalEvent.videoCallData?.url || "", externalCalendarId: result.externalId || null, credentialId: result.credentialId || null, };packages/lib/EventManager.ts (1)
662-664: Review booking reference construction.The use of
BookingReferenceService.buildFromResultis appropriate here and follows the established pattern. However, this duplicates the logic that was flagged in the previous review comment about incorrect field mappings for calendar events.
🧹 Nitpick comments (2)
packages/lib/server/service/bookingReference.ts (1)
3-15: Consider using a simple function instead of a static-only class.The static analysis tool correctly identifies that this class contains only static members. Consider refactoring to a simple exported function for better simplicity and tree-shaking:
-export class BookingReferenceService { - static buildFromResult(result: EventResult<unknown>) { +export function buildBookingReferenceFromResult(result: EventResult<unknown>) { return { type: result.type, uid: result.uid, meetingId: result.uid, meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "", meetingUrl: result.originalEvent.videoCallData?.url || "", externalCalendarId: result.externalId || null, credentialId: result.credentialId || null, }; - } -}packages/lib/EventManager.ts (1)
653-653: Use optional chaining for safer property access.The static analysis tool correctly identifies that optional chaining would be safer here:
-if (bookingCalendarReference && bookingCalendarReference.uid) { +if (bookingCalendarReference?.uid) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/lib/EventManager.ts(5 hunks)packages/lib/server/service/bookingReference.ts(1 hunks)
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
packages/lib/EventManager.tspackages/lib/server/service/bookingReference.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
packages/lib/EventManager.ts (3)
Learnt from: vijayraghav-io
PR: #21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: vijayraghav-io
PR: #21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The upsertSelectedCalendarsForEventTypeIds method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like @@unique([outlookSubscriptionId, eventTypeId]) should not be applied as they would prevent this intended functionality.
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
🧬 Code Graph Analysis (1)
packages/lib/server/service/bookingReference.ts (1)
packages/types/EventManager.d.ts (1)
EventResult(16-30)
🪛 Biome (1.9.4)
packages/lib/EventManager.ts
[error] 653-653: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/lib/server/service/bookingReference.ts
[error] 3-15: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
packages/lib/EventManager.tspackages/lib/server/service/bookingReference.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
packages/lib/EventManager.ts (3)
Learnt from: vijayraghav-io
PR: #21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: vijayraghav-io
PR: #21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The upsertSelectedCalendarsForEventTypeIds method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like @@unique([outlookSubscriptionId, eventTypeId]) should not be applied as they would prevent this intended functionality.
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
🧬 Code Graph Analysis (1)
packages/lib/server/service/bookingReference.ts (1)
packages/types/EventManager.d.ts (1)
EventResult(16-30)
🪛 Biome (1.9.4)
packages/lib/EventManager.ts
[error] 653-653: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/lib/server/service/bookingReference.ts
[error] 3-15: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Security Check
🔇 Additional comments (4)
packages/lib/EventManager.ts (4)
25-25: Import looks good.The import of
BookingReferenceServiceis correctly added to support the new booking reference construction logic.
591-591: Good addition of tracking flag.The
createdNewCalendarEventsflag properly tracks when new calendar events are created, enabling conditional booking reference updates.
667-671: Solid logic for preserving existing references.The approach of filtering out calendar references and then combining existing non-calendar references with new calendar references is well-implemented and maintains data integrity.
695-706: Clean conditional logic for reference updates.The separation of the condition into
finalShouldUpdateBookingReferencesimproves readability and properly incorporates the newcreatedNewCalendarEventsflag.
packages/lib/EventManager.ts
Outdated
| } else { | ||
| log.debug("No valid calendar event found for booking, creating new calendar events"); | ||
| const createdEvents = await this.createAllCalendarEvents(evt); | ||
| results.push(...createdEvents); | ||
|
|
||
| // Update booking references with newly created calendar events | ||
| const calendarReferences = createdEvents | ||
| .filter((result) => result.type.includes("_calendar") && result.success) | ||
| .map((result) => BookingReferenceService.buildFromResult(result)); | ||
|
|
||
| // Preserve existing non-calendar references and add new calendar references | ||
| const existingNonCalendarReferences = booking.references.filter( | ||
| (ref) => !ref.type.includes("_calendar") | ||
| ); | ||
| updatedBookingReferences.push(...existingNonCalendarReferences, ...calendarReferences); | ||
| createdNewCalendarEvents = calendarReferences.length > 0; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error handling for calendar event creation failures.
The logic correctly handles missing calendar events, but consider what happens if createAllCalendarEvents() fails or returns empty results. The current implementation could leave bookings in an inconsistent state.
Please verify the error handling behavior when calendar event creation fails:
Additionally, consider wrapping the calendar creation call in a try-catch block to handle potential failures gracefully.
🏁 Script executed:
#!/bin/bash
# Check how createAllCalendarEvents handles failures and what it returns
ast-grep --pattern $'createAllCalendarEvents($$$) {
$$$
}'
# Look for error handling patterns around calendar creation
rg -A 10 -B 5 "createAllCalendarEvents.*try|createAllCalendarEvents.*catch"Length of output: 139
To inspect how failures are handled (or not) inside createAllCalendarEvents, let’s dump its definition and all call sites with context:
🏁 Script executed:
#!/bin/bash
# Show definition and context for createAllCalendarEvents
rg -n -C 20 "createAllCalendarEvents\\s*\\(" .Length of output: 9502
Here’s the full body of createAllCalendarEvents for review:
🏁 Script executed:
#!/bin/bash
sed -n '806,950p' packages/lib/EventManager.tsLength of output: 6669
To verify whether failures from createAllCalendarEvents() bubble up unhandled or are already caught, let’s inspect the surrounding code (lines 580–720) for any try/catch:
🏁 Script executed:
#!/bin/bash
sed -n '580,720p' packages/lib/EventManager.tsLength of output: 5646
Handle createAllCalendarEvents failures to avoid inconsistent bookings
Currently, any exception thrown by createAllCalendarEvents will bubble up unhandled—and if it returns an empty array, we silently proceed without creating calendar events, which can leave the booking in an unexpected state. Please update the code in packages/lib/EventManager.ts (around lines 656–672) as follows:
• Wrap the call in a try/catch to surface failures:
try {
const createdEvents = await this.createAllCalendarEvents(evt);
results.push(...createdEvents);
// …rest of success path…
} catch (error) {
log.error(
"Failed to create calendar events for booking",
safeStringify({ bookingId: newBookingId, err: error })
);
// decide whether to retry, abort the overall operation, or mark booking as errored
throw new Error("Calendar event creation failed—booking not completed");
}• After the call, check for an empty createdEvents array and handle it explicitly (e.g. throw or fallback), instead of proceeding silently.
• Ensure any thrown error is caught at the top level of the booking flow so you can roll back or return a clear failure response.
Fix these in the else block starting at line 657 in EventManager.ts.
🤖 Prompt for AI Agents
In packages/lib/EventManager.ts around lines 656 to 672, wrap the call to
createAllCalendarEvents(evt) in a try/catch block to catch and log any errors
using log.error with bookingId and error details, then throw a new error to
abort the booking process. After the call, check if the returned createdEvents
array is empty and if so, throw an error to prevent proceeding silently. This
ensures failures in calendar event creation are handled explicitly and can be
rolled back or reported properly.
|
@Devanshusharma2005 Please add a loom/recording showing how earlier it was failing and now a reschedule would successfully create an event even if it didn't exist already. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/lib/EventManager.ts (2)
656-672: Add robust error handling for calendar event creation.This addresses a previous review concern about error handling. The current implementation doesn't handle failures from
createAllCalendarEvents(), which could leave bookings in an inconsistent state if calendar creation fails.+try { const createdEvents = await this.createAllCalendarEvents(evt); results.push(...createdEvents); // Update booking references with newly created calendar events const calendarReferences = createdEvents .filter((result) => result.type.includes("_calendar") && result.success) .map((result) => BookingReferenceService.buildFromResult(result)); // Preserve existing non-calendar references and add new calendar references const existingNonCalendarReferences = booking.references.filter( (ref) => !ref.type.includes("_calendar") ); updatedBookingReferences.push(...existingNonCalendarReferences, ...calendarReferences); createdNewCalendarEvents = calendarReferences.length > 0; +} catch (error) { + log.error( + "Failed to create calendar events during rescheduling", + safeStringify({ bookingId: booking.id, rescheduleUid, error }) + ); + throw new Error("Calendar event creation failed during rescheduling"); +}
662-664: Review the field mapping logic from BookingReferenceService.As noted in the previous file review, the
BookingReferenceService.buildFromResultmethod has incorrect field mappings for calendar events (meetingPassword using timezone data, meetingUrl for calendar events). This should be addressed to ensure booking references are created correctly.The specific issues are:
meetingPasswordis populated with timezone information instead of being null for calendar eventsmeetingUrlis populated with video call data which doesn't apply to calendar eventsPlease refer to the BookingReferenceService review for the specific fix.
🧹 Nitpick comments (2)
packages/lib/server/service/BookingReferenceService.ts (1)
3-15: Consider using a simple function instead of a static-only class.The static analysis tool correctly identifies that this class contains only static members. Following functional programming principles, this could be simplified to a plain function.
-export class BookingReferenceService { - static buildFromResult(result: EventResult<unknown>) { - return { - type: result.type, - uid: result.uid, - meetingId: result.uid, - meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "", - meetingUrl: result.originalEvent.videoCallData?.url || "", - externalCalendarId: result.externalId || null, - credentialId: result.credentialId || null, - }; - } -} +export function buildBookingReferenceFromResult(result: EventResult<unknown>) { + return { + type: result.type, + uid: result.uid, + meetingId: result.uid, + meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "", + meetingUrl: result.originalEvent.videoCallData?.url || "", + externalCalendarId: result.externalId || null, + credentialId: result.credentialId || null, + }; +}packages/lib/EventManager.ts (1)
653-653: Apply optional chaining as suggested by static analysis.The static analysis tool correctly identifies that this can be simplified using optional chaining for better readability and safety.
-if (bookingCalendarReference && bookingCalendarReference.uid) { +if (bookingCalendarReference?.uid) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/lib/EventManager.ts(5 hunks)packages/lib/server/service/BookingReferenceService.ts(1 hunks)
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
packages/lib/EventManager.tspackages/lib/server/service/BookingReferenceService.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
packages/lib/EventManager.ts (3)
Learnt from: vijayraghav-io
PR: #21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: vijayraghav-io
PR: #21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The upsertSelectedCalendarsForEventTypeIds method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like @@unique([outlookSubscriptionId, eventTypeId]) should not be applied as they would prevent this intended functionality.
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
🧬 Code Graph Analysis (2)
packages/lib/EventManager.ts (1)
packages/lib/server/service/BookingReferenceService.ts (1)
BookingReferenceService(3-15)
packages/lib/server/service/BookingReferenceService.ts (1)
packages/types/EventManager.d.ts (1)
EventResult(16-30)
🪛 Biome (1.9.4)
packages/lib/EventManager.ts
[error] 653-653: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/lib/server/service/BookingReferenceService.ts
[error] 3-15: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
packages/lib/EventManager.tspackages/lib/server/service/BookingReferenceService.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
packages/lib/EventManager.ts (3)
Learnt from: vijayraghav-io
PR: #21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: vijayraghav-io
PR: #21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The upsertSelectedCalendarsForEventTypeIds method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like @@unique([outlookSubscriptionId, eventTypeId]) should not be applied as they would prevent this intended functionality.
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
🧬 Code Graph Analysis (2)
packages/lib/EventManager.ts (1)
packages/lib/server/service/BookingReferenceService.ts (1)
BookingReferenceService(3-15)
packages/lib/server/service/BookingReferenceService.ts (1)
packages/types/EventManager.d.ts (1)
EventResult(16-30)
🪛 Biome (1.9.4)
packages/lib/EventManager.ts
[error] 653-653: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/lib/server/service/BookingReferenceService.ts
[error] 3-15: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Security Check
🔇 Additional comments (5)
packages/lib/server/service/BookingReferenceService.ts (1)
11-11: No action needed:externalIdcorrectly maps toexternalCalendarId
TheEventResultinterface exposes the calendar identifier asexternalId, and theBookingReferencemodel (and correspondingPartialReference) expects that value on itsexternalCalendarIdfield. The code inBookingReferenceService.buildFromResulttherefore correctly assignsresult.externalId || nulltoexternalCalendarId, matching both the TypeScript definitions and the Prisma schema.packages/lib/EventManager.ts (4)
25-25: LGTM - Clean import of the new service.The import of
BookingReferenceServiceis correctly added and will be used in the rescheduling logic below.
591-591: Good tracking mechanism for new calendar events.The
createdNewCalendarEventsflag provides clear tracking of whether new calendar events were created during rescheduling, which is essential for determining when to update booking references.
600-605: Excellent implementation of declarative logic flow.The updated conditions using
createdNewCalendarEventsprovide clear, declarative logic that makes the booking reference update behavior explicit and predictable. The consistent use of this flag in both the initial condition and final return logic ensures proper handling of the rescheduling scenarios described in the PR objectives.Also applies to: 695-700
657-658: Good debugging support with descriptive log message.The debug log clearly explains the condition being handled, which will be valuable for troubleshooting rescheduling issues in production.
| meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "", | ||
| meetingUrl: result.originalEvent.videoCallData?.url || "", |
There was a problem hiding this comment.
Fix incorrect field mappings for calendar events.
The meetingPassword and meetingUrl fields are being populated with inappropriate data for calendar events:
meetingPasswordis set to timezone information, which is semantically incorrectmeetingUrlis set to video call data, which doesn't apply to calendar events
These fields should be null or undefined for calendar events, or properly mapped if they have different meanings in this context.
- meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "",
- meetingUrl: result.originalEvent.videoCallData?.url || "",
+ meetingPassword: null,
+ meetingUrl: 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.
| meetingPassword: result.originalEvent.attendees?.[0]?.timeZone || "", | |
| meetingUrl: result.originalEvent.videoCallData?.url || "", | |
| meetingPassword: null, | |
| meetingUrl: null, |
🤖 Prompt for AI Agents
In packages/lib/server/service/BookingReferenceService.ts around lines 9 to 10,
the fields meetingPassword and meetingUrl are incorrectly assigned data
unrelated to calendar events (timezone and video call URL). Update the code to
set meetingPassword and meetingUrl to null or undefined for calendar events, or
map them correctly if they have specific meanings in this context, ensuring the
fields reflect appropriate values.
…r.reschedule - Extract lines 656-671 into new handleMissingCalendarEvents method - Improve code organization following Single Responsibility Principle - Preserve original behavior and typing - Add proper TypeScript typing for filter function parameter - Clean up variable usage and improve maintainability Co-Authored-By: Devanshu Sharma <devanshusharma658@gmail.com>
…ing with BookingReferenceService - Keep refactored handleMissingCalendarEvents method structure - Adopt BookingReferenceService.buildFromResult usage from remote changes - Maintain Single Responsibility Principle improvements - Preserve all existing functionality and typing Co-Authored-By: Devanshu Sharma <devanshusharma658@gmail.com>
|
This PR is being marked as stale due to inactivity. |
feat: implement declarative calendar event handling in reschedule
Summary
fixes : #22192
CAL: 6020
This PR fixes the issue where rescheduling a booking fails if the original Google Calendar event is missing or has an empty UID. The solution implements a declarative approach: after rescheduling, a valid calendar event will always exist, regardless of whether one existed before.
Key Changes
Technical Implementation
The fix follows this declarative approach:
createAllCalendarEvents()Review & Testing Checklist for Human
externalIdvsexternalCalendarId)Recommended Test Plan:
Diagram
%%{ init : { "theme" : "default" }}%% graph TD handleNewBooking["handleNewBooking.ts<br/>(reschedule flow)"]:::context EventManager["EventManager.ts<br/>reschedule()"]:::major-edit rescheduleTest["reschedule.test.ts<br/>(new test cases)"]:::major-edit handleNewBooking --> EventManager EventManager --> createAllCalendarEvents["createAllCalendarEvents()"]:::context EventManager --> updateAllCalendarEvents["updateAllCalendarEvents()"]:::context rescheduleTest -.-> EventManager EventManager --> BookingReferences["Booking References<br/>(calendar + non-calendar)"]:::minor-edit subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#FFFFFFNotes
createAllCalendarEvents()method rather than duplicating logicexternalCalendarIdvsexternalIdwere encountered during development - the EventResult interface usesexternalIdSession: https://app.devin.ai/sessions/4127248fd8474da3852f8b4c4ace1d18
Requested by: @Devanshusharma2005