fix: handle collective multiple host on destinationCalendar

[akshayutture-augment]

Test 4

---

Replicated from ai-code-review-evaluation/cal.com-greptile#4

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

Code Review Complete

Overview

This PR implements support for multiple destination calendars in collective scheduling scenarios, changing destinationCalendar from a single object to an array throughout the codebase. This allows each host in a collective event to have their own calendar destination.

Key Changes

Type Definitions (packages/types/Calendar.d.ts)

• ✅ Changed destinationCalendar?: DestinationCalendar | null to destinationCalendar?: DestinationCalendar[] | null
• ✅ Added schedulingType?: SchedulingType | null to CalendarEvent interface
• ✅ Updated Calendar.createEvent() signature to include credentialId: number parameter
• ✅ Added externalId?: string | null to EventResult<T> in EventManager types

Core Logic (packages/core/EventManager.ts, packages/core/CalendarManager.ts)

• ✅ Updated to iterate over multiple destination calendars when creating/updating events
• ✅ Added credential fetching from database when not found in memory
• ✅ Changed from single calendar reference to array of references in updateAllCalendarEvents()
• ✅ Fixed typo: "organiser" → "organizer" (line 230 in CalendarManager.ts)

Issues Found

🔴 Critical Issues

1. Type Safety Issue in Office365 Calendar Service (packages/app-store/office365calendar/lib/CalendarService.ts:87)

   • The createEvent method signature doesn't include the new credentialId parameter that the interface now requires

   async createEvent(event: CalendarEvent): Promise<NewCalendarEventType> {

   Should be:

   async createEvent(event: CalendarEvent, credentialId: number): Promise<NewCalendarEventType> {

2. Same Issue in Lark Calendar Service (packages/app-store/larkcalendar/lib/CalendarService.ts:125)

   • Missing credentialId parameter in method signature

3. Same Issue in BaseCalendarService (packages/lib/CalendarService.ts:125)

   • Missing credentialId parameter in method signature

🟡 Medium Priority Issues

4. Inconsistent Array Destructuring Pattern

   • Throughout the codebase, the pattern const [mainHostDestinationCalendar] = event.destinationCalendar ?? []; is used
   • This always takes the first element, which may not be correct for all collective scheduling scenarios
   • Example locations:
     • packages/core/EventManager.ts:118
     • packages/app-store/googlecalendar/lib/CalendarService.ts:100-103, 198-199
     • packages/app-store/larkcalendar/lib/CalendarService.ts:128, 164, 192, 237
     • packages/lib/CalendarService.ts:156, 509
   • Question: Should collective events create calendar entries in ALL host calendars, not just the main host's?

5. Potential Null Reference in Google Calendar Service (packages/app-store/googlecalendar/lib/CalendarService.ts:119)

   const [mainHostDestinationCalendar] = calEventRaw?.destinationCalendar && calEventRaw?.destinationCalendar.length > 0
     ? calEventRaw.destinationCalendar
     : [];

   Then used without null check:

   email: mainHostDestinationCalendar?.externalId
     ? mainHostDestinationCalendar.externalId
     : calEventRaw.organizer.email,

   If mainHostDestinationCalendar is undefined (empty array destructuring), this could cause issues.

6. Incomplete Credential Handling (packages/app-store/googlecalendar/lib/CalendarService.ts:147-149)

   const selectedCalendar = calEventRaw.destinationCalendar?.find(
     (cal) => cal.credentialId === credentialId
   )?.externalId;

   Good: Now correctly searches through the array for matching credentialId.
   However, there's no fallback or error handling if no matching calendar is found.

7. Logic Error in deleteEvent (packages/app-store/googlecalendar/lib/CalendarService.ts:317)

   const calendarId = externalCalendarId
     ? externalCalendarId
     : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;

   This logic is circular - if externalCalendarId exists, use it; otherwise find a calendar with externalId === externalCalendarId. The second part will never execute since externalCalendarId is already checked.

8. Typo Fix Missed in handleNewBooking (packages/features/bookings/lib/handleNewBooking.ts:370)

   • Fixed typo: orginalBookingDuration → originalBookingDuration ✅
   • Good catch!

9. Array Push to Potentially Null (packages/features/bookings/lib/handleNewBooking.ts:1078)

   if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") {
     evt.destinationCalendar?.push(...teamDestinationCalendars);
   }

   evt.destinationCalendar could be null, which would cause a runtime error. Should use optional chaining or ensure it's initialized.

10. Missing credentialId parameter in calendar service calls

    • In CalendarService.ts (base class), the createEvent method signature needs updating to match the interface

🟢 Positive Changes

• ✅ Comprehensive update across all affected files
• ✅ Added proper credential fetching from database as fallback
• ✅ Fixed duplicate conditional in EventManager.ts (line 573-574)
• ✅ Better error handling in handleNewBooking.ts with try-catch blocks
• ✅ Improved team member handling for collective events
• ✅ Proper filtering and iteration over multiple calendar references

⚠️ Suggestions

1. Add Validation: Consider adding validation to ensure destinationCalendar array is not empty when required
2. Documentation: Add JSDoc comments explaining the collective scheduling behavior and when multiple calendars are used
3. Testing: Ensure comprehensive tests cover:
   • Single host events (array with 1 element)
   • Collective events with multiple hosts
   • Edge cases where destinationCalendar might be null or empty
4. Consistent Handling: Define a clear strategy for which calendar(s) should receive events in collective scheduling

📝 Summary

The PR successfully implements the core functionality of handling multiple destination calendars. However, there are 3 critical type safety issues where calendar service implementations don't match the updated interface signature. These must be fixed before merging.

The medium-priority issues around array handling and null checks should also be addressed to prevent potential runtime errors in production.

---

[github-actions]

This PR is being marked as stale due to inactivity.