fix: handle collective multiple host on destinationCalendar#9
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the handling of destination calendars to support multiple calendars for collective team events. Previously, destinationCalendar was a single object; now it's an array to enable syncing events to multiple calendars when multiple hosts are involved.
Changes:
- Converted
destinationCalendarfrom single object to array throughout the codebase - Added per-member calendar configuration support for collective team events
- Enhanced credential resolution to fetch from database when not found in memory
Reviewed changes
Copilot reviewed 20 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/trpc/server/routers/viewer/organizations/create.handler.ts | Refactored conditional spread operators for cleaner ternary expressions |
| packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts | Converted destinationCalendar to array format |
| packages/trpc/server/routers/viewer/bookings/editLocation.handler.ts | Wrapped destinationCalendar in array with fallback logic |
| packages/trpc/server/routers/viewer/bookings/confirm.handler.ts | Updated to use array format for destinationCalendar |
| packages/trpc/server/routers/loggedInViewer/deleteCredential.handler.ts | Modified destinationCalendar handling to use array |
| packages/lib/CalendarService.ts | Extracted first calendar from array for backward compatibility |
| packages/features/ee/payments/api/webhook.ts | Added intermediate variable and array wrapping for destinationCalendar |
| packages/features/ee/payments/api/paypal-webhook.ts | Converted destinationCalendar to array format |
| packages/features/bookings/lib/handleNewBooking.ts | Major refactoring: fixed typo, improved team calendar handling, added collective event support |
| packages/features/bookings/lib/handleCancelBooking.ts | Updated calendar reference handling to support multiple calendars and fetch credentials from DB |
| packages/emails/src/templates/BrokenIntegrationEmail.tsx | Extracted first calendar from array |
| packages/core/builders/CalendarEvent/class.ts | Changed destinationCalendar type from single object to array |
| packages/core/builders/CalendarEvent/builder.ts | Adjusted import order and type specification |
| packages/core/EventManager.ts | Refactored to iterate over destinationCalendar array and fetch missing credentials |
| packages/core/CalendarManager.ts | Added externalId parameter and credentialId to return value |
| packages/app-store/office365calendar/lib/CalendarService.ts | Extracted first calendar from array for API calls |
| packages/app-store/larkcalendar/lib/CalendarService.ts | Updated to use first calendar from array |
| packages/app-store/googlecalendar/lib/CalendarService.ts | Enhanced to find appropriate calendar by credentialId |
| apps/web/playwright/webhook.e2e.ts | Updated test expectation from null to empty array |
| apps/web/pages/api/cron/bookingReminder.ts | Added intermediate variable and wrapped in array |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| location: bookingToReschedule?.location, | ||
| destinationCalendar: bookingToReschedule?.destinationCalendar || bookingToReschedule?.destinationCalendar, | ||
| destinationCalendar: bookingToReschedule?.destinationCalendar | ||
| ? [bookingToReschedule?.destinationCalendar] |
There was a problem hiding this comment.
The optional chaining on line 240 is redundant since the ternary already checks for truthiness. Simplify to bookingToReschedule.destinationCalendar to avoid unnecessary optional chaining.
| ? [bookingToReschedule?.destinationCalendar] | |
| ? [bookingToReschedule.destinationCalendar] |
| if (!Array.isArray(dynamicUserList) || dynamicUserList.length === 0) { | ||
| throw new Error("dynamicUserList is not properly defined or empty."); | ||
| } | ||
|
|
There was a problem hiding this comment.
The error message doesn't distinguish between 'not an array' and 'empty array' cases. Consider splitting into two separate error messages: 'dynamicUserList must be an array' and 'dynamicUserList cannot be empty' for clearer debugging.
| if (!Array.isArray(dynamicUserList) || dynamicUserList.length === 0) { | |
| throw new Error("dynamicUserList is not properly defined or empty."); | |
| } | |
| if (!Array.isArray(dynamicUserList)) { | |
| throw new Error("dynamicUserList must be an array."); | |
| } | |
| if (dynamicUserList.length === 0) { | |
| throw new Error("dynamicUserList cannot be empty."); | |
| } |
| if (!Array.isArray(hosts)) { | ||
| throw new Error("eventType.hosts is not properly defined."); | ||
| } | ||
|
|
There was a problem hiding this comment.
This check is redundant because eventType.hosts is already assigned to hosts on line 744, and TypeScript should enforce that eventType.hosts is an array or undefined. If it's undefined, the fallback on line 755 handles it. Remove this unnecessary check.
| if (!Array.isArray(hosts)) { | |
| throw new Error("eventType.hosts is not properly defined."); | |
| } |
| if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") { | ||
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one | ||
| const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; | ||
| if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") { |
There was a problem hiding this comment.
Potential null reference error. mainHostDestinationCalendar could be undefined if evt.destinationCalendar is empty or null. Add optional chaining: mainHostDestinationCalendar?.integration.
| if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") { | |
| if (evt.location === MeetLocationType && mainHostDestinationCalendar?.integration !== "google_calendar") { |
| credentialId: cred.id, | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
The empty line 573 appears to be unintentional and creates unnecessary whitespace. The conditional check on line 574 should directly follow line 572.
| const selectedCalendar = externalCalendarId | ||
| ? externalCalendarId | ||
| : event.destinationCalendar?.externalId; | ||
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; |
There was a problem hiding this comment.
The logic is incorrect. This tries to find a calendar where cal.externalId === externalCalendarId and then returns its externalId, which will always be externalCalendarId if found. The intended logic is unclear, but this likely should use a different comparison or search criteria.
| const calendarId = externalCalendarId ? externalCalendarId : event.destinationCalendar?.externalId; | ||
| const calendarId = externalCalendarId | ||
| ? externalCalendarId | ||
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; |
There was a problem hiding this comment.
Same issue as in the updateEvent method. The find operation searches for a calendar where cal.externalId === externalCalendarId, making the search redundant since it returns the same externalCalendarId. This logic needs to be corrected.
Test 4nn
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/cal.com-coderabbit#4](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/4)*