fix: handle collective multiple host on destinationCalendar#2
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR refactors destination calendar handling from single-value to array-based architecture, enabling multiple calendars per event. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EventManager
participant CalendarManager
participant CredentialStore as DB/CredentialStore
participant CalendarService
rect rgb(200, 220, 240)
Note over EventManager,CalendarService: Multi-Calendar Event Creation Flow
end
Client->>EventManager: createAllCalendarEvents(event)
activate EventManager
loop For each destination calendar
EventManager->>EventManager: Extract calendar from array
alt Has credentialId
EventManager->>CredentialStore: Resolve credential by credentialId
CredentialStore-->>EventManager: Credential + app slug
else No credentialId
EventManager->>CredentialStore: Query credentials by integration
CredentialStore-->>EventManager: All matching credentials
end
loop For each resolved credential
EventManager->>CalendarManager: createEvent(calEvent, credential, externalId)
activate CalendarManager
CalendarManager->>CalendarService: createEvent(calEventRaw, credentialId)
activate CalendarService
CalendarService->>CalendarService: Select calendar by credentialId
CalendarService-->>CalendarManager: Event result (externalId, credentialId)
deactivate CalendarService
CalendarManager-->>EventManager: EventResult with externalId
deactivate CalendarManager
end
end
rect rgb(240, 240, 220)
Note over EventManager,CredentialStore: Fallback: Use first available calendar credential
end
EventManager-->>Client: Array of calendar event results
deactivate EventManager
sequenceDiagram
participant BookingHandler
participant EventManager
participant CalendarManager
participant WebhookPayload
rect rgb(220, 240, 220)
Note over BookingHandler,WebhookPayload: Destination Calendar Array Propagation
end
BookingHandler->>BookingHandler: Determine selectedDestinationCalendar
Note over BookingHandler: booking.destinationCalendar || user.destinationCalendar
BookingHandler->>BookingHandler: Wrap as array
Note over BookingHandler: destinationCalendar = [selectedDestinationCalendar] || []
BookingHandler->>EventManager: createCalendarEvent(event with dest. calendar array)
activate EventManager
EventManager->>CalendarManager: Iterate over destination calendars
activate CalendarManager
CalendarManager-->>EventManager: Multiple event results
deactivate CalendarManager
deactivate EventManager
BookingHandler->>WebhookPayload: Emit webhook with dest. calendar array
Note over WebhookPayload: payload.destinationCalendar: DestinationCalendar[]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/web/playwright/webhook.e2e.ts (1)
119-119: Critical inconsistency: Mixed destinationCalendar expectations across webhook tests.The BOOKING_REJECTED test (line 249) expects
destinationCalendar: [], but BOOKING_CREATED (line 119) and BOOKING_REQUESTED (line 373) still expectdestinationCalendar: null. If destinationCalendar is now consistently an array throughout the codebase, all three webhook tests should expect the same format to avoid test failures.🔎 Proposed fix to align all webhook tests
location: "[redacted/dynamic]", - destinationCalendar: null, + destinationCalendar: [], hideCalendarNotes: false,location: "[redacted/dynamic]", - destinationCalendar: null, + destinationCalendar: [], requiresConfirmation: "[redacted/dynamic]",Also applies to: 249-249, 373-373
packages/lib/CalendarService.ts (1)
125-198: Critical:createEventsignature doesn't match theCalendarinterface.The base
Calendarinterface requirescreateEvent(event: CalendarEvent, credentialId: number), but this implementation is missing thecredentialIdparameter. This affects all CalDAV-based calendar services that extend this base class.🔎 Proposed fix
-async createEvent(event: CalendarEvent): Promise<NewCalendarEventType> { +async createEvent(event: CalendarEvent, credentialId: number): Promise<NewCalendarEventType> { try { const calendars = await this.listCalendars(event);packages/app-store/office365calendar/lib/CalendarService.ts (1)
72-92: Critical:createEventsignature doesn't match theCalendarinterface.The
Calendarinterface inpackages/types/Calendar.d.tsdefinescreateEvent(event: CalendarEvent, credentialId: number), but this implementation is missing thecredentialId: numberparameter. This violates the interface contract and will cause type errors.packages/app-store/googlecalendar/lib/CalendarService.ts (1)
165-168: Missing fallback to "primary" in patch call.When patching the event to add the hangout link,
selectedCalendarcould beundefinedif no matching destination calendar was found. The insert call on line 154 correctly falls back to"primary", but this patch call doesn't.🔎 Proposed fix
calendar.events.patch({ // Update the same event but this time we know the hangout link - calendarId: selectedCalendar, + calendarId: selectedCalendar || "primary", auth: myGoogleAuth,packages/features/bookings/lib/handleCancelBooking.ts (1)
618-619: MissingPromise.allfor apiDeletes array.
apiDeletesis an array of promises, butawait apiDeletesdoesn't properly await all of them. This should beawait Promise.all(apiDeletes).🔎 Proposed fix
try { - await apiDeletes; + await Promise.all(apiDeletes); } catch (error) { console.error("Error deleting event", error); }packages/features/bookings/lib/handleNewBooking.ts (1)
398-398: Remove debug console.log statement.This debug log should be removed before merging.
🔎 Proposed fix
- console.log("calendarBusyTimes==>>>", bufferedBusyTimes);
🤖 Fix all issues with AI Agents
In @packages/app-store/googlecalendar/lib/CalendarService.ts:
- Around line 315-317: The current assignment to calendarId uses
event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)
which will never match when externalCalendarId is falsy; change the fallback to
select a valid externalId from destinationCalendar instead (e.g., use
event.destinationCalendar?.[0]?.externalId or find by a sensible property like
isPrimary) so that calendarId = externalCalendarId ??
event.destinationCalendar?.[0]?.externalId; update the code referencing
calendarId to rely on this corrected fallback.
- Around line 254-256: The current selection logic for selectedCalendar in
updateEvent is wrong because it does .find(cal => cal.externalId ===
externalCalendarId) when externalCalendarId is falsy, which never matches;
either simplify to just use externalCalendarId (i.e., set selectedCalendar =
externalCalendarId) if you intend undefined when not provided, or implement the
intended resolution by credential like createEvent: change the expression to
event.destinationCalendar?.find(cal => cal.credentialId ===
credentialId)?.externalId, and update the updateEvent signature and all its
callers to accept and pass credentialId so the lookup can succeed.
In @packages/core/EventManager.ts:
- Around line 117-121: The code destructures evt.destinationCalendar into
mainHostDestinationCalendar but doesn’t guard against it being undefined; update
the condition in EventManager (the block using mainHostDestinationCalendar) to
safely access integration — e.g., replace the direct property access with a safe
check using optional chaining or an explicit existence check
(mainHostDestinationCalendar?.integration !== "google_calendar" or
mainHostDestinationCalendar && mainHostDestinationCalendar.integration !==
"google_calendar") before assigning evt.location = "integrations:daily"; ensure
the logic still only runs when evt.location === MeetLocationType.
In @packages/features/bookings/lib/handleCancelBooking.ts:
- Around line 449-460: The code iterates bookingToDelete.user.credentials for
every updatedBooking, causing deletions to be attempted against all user
calendars instead of the one referenced; change the logic to locate the specific
calendar credential that matches each booking reference and call getCalendar
only for that credential (or reuse the credential resolved in the outer scope)
before invoking calendar.deleteEvent. Concretely, for each updBooking in
updatedBookings find bookingRef (existing code), then find the matching
credential from bookingToDelete.user.credentials (e.g., by id/type that
corresponds to bookingRef), call getCalendar(matchingCredential) and call
deleteEvent on that calendar, pushing the result into apiDeletes; avoid
iterating over all credentials.
In @packages/features/bookings/lib/handleNewBooking.ts:
- Around line 1077-1079: The code can silently skip adding team calendars
because evt.destinationCalendar may be null; ensure COLLECTIVE team calendars
are always added by initializing destinationCalendar to an array when needed
before pushing: in the block guarded by isTeamEventType &&
eventType.schedulingType === "COLLECTIVE", check evt.destinationCalendar and if
null set evt.destinationCalendar = [] (or copy existing values) then
push(...teamDestinationCalendars); this ensures evt.destinationCalendar always
accepts the teamDestinationCalendars rather than being a no-op due to optional
chaining.
In @packages/trpc/server/routers/viewer/organizations/create.handler.ts:
- Around line 151-153: The slug collision check in create.handler.ts runs
unconditionally but slug persistence is gated by IS_TEAM_BILLING_ENABLED,
causing downstream createTeams (which validates for metadata.requestedSlug or
organization.slug and can throw NoOrganizationSlugError) to fail; fix by making
behavior consistent—either wrap the slug uniqueness check (the block that
validates slug collision) with the same IS_TEAM_BILLING_ENABLED guard so slug is
neither validated nor stored when billing is disabled, or always persist the
slug and metadata.requestedSlug regardless of IS_TEAM_BILLING_ENABLED so
downstream createTeams receives a slug; update the logic around slug,
metadata.requestedSlug, and the collision check to use the same gating approach
to eliminate the mismatch.
In @packages/types/Calendar.d.ts:
- Line 221: The createEvent interface signature now requires credentialId:
number but many implementations still have the old signature; update each
implementation method to accept the new parameter and pass it through or use it
where appropriate. Specifically, change the method signature for createEvent in
the classes/objects named Basecamp3Calendar.createEvent,
CloseComCalendar.createEvent, Exchange2013Calendar.createEvent,
Exchange2016Calendar.createEvent, ExchangeCalendar.createEvent,
HubSpotCalendar.createEvent, LarkCalendar.createEvent,
Office365Calendar.createEvent, SalesforceCalendar.createEvent,
SendGridCalendar.createEvent, ZohoBiginCalendar.createEvent,
ZohoCrmCalendar.createEvent, and the higher-level CalendarService.createEvent to
include credentialId: number, adjust any internal calls to forward that
credentialId to downstream helpers or HTTP clients, and update return handling
to continue returning Promise<NewCalendarEventType>.
🧹 Nitpick comments (3)
packages/features/ee/payments/api/webhook.ts (1)
207-207: Consider extracting the duplicated calendar selection logic.The same destination calendar selection and array-wrapping logic appears here and in
getBooking(lines 101, 119). While the consistency is good, extracting this to a shared helper function would improve maintainability and reduce the risk of divergence if the logic needs to change.Example:
function getDestinationCalendarArray( bookingCalendar: DestinationCalendar | null, userCalendar: DestinationCalendar | null ): DestinationCalendar[] { const selected = bookingCalendar || userCalendar; return selected ? [selected] : []; }Also applies to: 229-229
packages/trpc/server/routers/viewer/bookings/confirm.handler.ts (1)
175-179: LGTM: Correct array wrapping with fallback chain.The ternary chain correctly wraps destinationCalendar in an array, with proper fallback from booking → user → empty array.
Optional: Simplify the ternary chain for readability
- destinationCalendar: booking?.destinationCalendar - ? [booking.destinationCalendar] - : user.destinationCalendar - ? [user.destinationCalendar] - : [], + destinationCalendar: (booking?.destinationCalendar || user.destinationCalendar) + ? [booking?.destinationCalendar || user.destinationCalendar] + : [],However, the current implementation is clearer about the precedence order, so this refactor is purely optional.
packages/types/Calendar.d.ts (1)
182-182: Consider documenting theschedulingTypefield addition.The new
schedulingTypefield isn't mentioned in the PR summary. While it may be related to collective scheduling enhancements, it would be helpful to understand its purpose and usage in the broader context.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/web/pages/api/cron/bookingReminder.tsapps/web/playwright/webhook.e2e.tspackages/app-store/googlecalendar/lib/CalendarService.tspackages/app-store/larkcalendar/lib/CalendarService.tspackages/app-store/office365calendar/lib/CalendarService.tspackages/core/CalendarManager.tspackages/core/EventManager.tspackages/core/builders/CalendarEvent/builder.tspackages/core/builders/CalendarEvent/class.tspackages/emails/src/templates/BrokenIntegrationEmail.tsxpackages/features/bookings/lib/handleCancelBooking.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/ee/payments/api/paypal-webhook.tspackages/features/ee/payments/api/webhook.tspackages/lib/CalendarService.tspackages/trpc/server/routers/loggedInViewer/deleteCredential.handler.tspackages/trpc/server/routers/viewer/bookings/confirm.handler.tspackages/trpc/server/routers/viewer/bookings/editLocation.handler.tspackages/trpc/server/routers/viewer/bookings/requestReschedule.handler.tspackages/trpc/server/routers/viewer/organizations/create.handler.tspackages/types/Calendar.d.tspackages/types/EventManager.d.ts
🧰 Additional context used
🧬 Code graph analysis (5)
packages/core/CalendarManager.ts (1)
packages/types/Calendar.d.ts (1)
CalendarEvent(146-190)
packages/trpc/server/routers/viewer/organizations/create.handler.ts (1)
packages/lib/constants.ts (1)
IS_TEAM_BILLING_ENABLED(88-88)
packages/features/bookings/lib/handleNewBooking.ts (2)
packages/prisma/selects/user.ts (1)
userSelect(36-51)packages/lib/server/i18n.ts (1)
getTranslation(5-18)
packages/core/EventManager.ts (4)
packages/app-store/locations.ts (1)
MeetLocationType(54-54)packages/core/CalendarManager.ts (2)
createEvent(218-270)updateEvent(272-323)packages/types/EventManager.d.ts (2)
PartialReference(3-12)EventResult(14-27)packages/types/Calendar.d.ts (1)
NewCalendarEventType(62-70)
packages/features/bookings/lib/handleCancelBooking.ts (2)
packages/prisma/index.ts (1)
prisma(56-56)packages/app-store/_utils/getCalendar.ts (1)
getCalendar(26-48)
🔇 Additional comments (39)
packages/trpc/server/routers/viewer/bookings/editLocation.handler.ts (1)
85-89: LGTM! Array-based architecture correctly implemented.The conversion to array-based destinationCalendar handling is correct and aligns with the PR's objective to support multiple calendars. The fallback chain (booking → user → empty array) is logical and uses consistent optional chaining.
The choice of returning an empty array
[]rather thannullwhen no calendar is found is pragmatic—it eliminates the need for null checks in downstream array operations and is consistent with common patterns in array-based refactors.packages/features/ee/payments/api/webhook.ts (1)
101-101: Array handling is correctly implemented and downstream-compatible.The fallback chain (
booking.destinationCalendar || user.destinationCalendar) and array wrapping align with the EventManager and email handler implementations, which already destructure or iterate over the array format. Empty arrays are properly handled via nullish coalescing.Minor: The same calendar selection logic appears in both
getBooking(lines 101, 119) andhandlePaymentSuccess(lines 207, 229). Consider extracting into a helper function to reduce duplication.packages/types/EventManager.d.ts (1)
26-26: LGTM! Clean type extension.The addition of the optional
externalIdfield toEventResult<T>is straightforward and non-breaking. This supports the broader calendar reference architecture introduced in this PR.packages/emails/src/templates/BrokenIntegrationEmail.tsx (1)
88-91: LGTM! Safe array destructuring with proper fallbacks.The code safely extracts the first destination calendar from the array using destructuring with nullish coalescing (
?? []). The fallback to"calendar"whenmainHostDestinationCalendaris undefined ensures the email template always has a valid value to display.packages/features/ee/payments/api/paypal-webhook.ts (1)
152-156: Consistent array wrapping pattern.The
destinationCalendarfield is correctly wrapped as an array with the same fallback chain used in other handlers. TheCalendarEventinterface definesdestinationCalendar?: DestinationCalendar[] | null;, andEventManager.create()properly handles it as an array, iterating over it increateAllCalendarEvents(). This maintains consistency across the codebase for the array-based calendar architecture.packages/trpc/server/routers/loggedInViewer/deleteCredential.handler.ts (1)
290-294: Array wrapping logic is correct.The
destinationCalendaris correctly wrapped as an array with a proper fallback chain. TheCalendarEventtype definition (inpackages/types/Calendar.d.ts) already specifiesdestinationCalendarasDestinationCalendar[] | null, so this change aligns with the existing type system and supports the PR's objective to handle multiple calendars per event.packages/app-store/larkcalendar/lib/CalendarService.ts (2)
128-129: LGTM: Consistent array destructuring pattern.The array destructuring with fallback (
event.destinationCalendar ?? []) is correctly applied across all methods (createEvent, createAttendees, updateEvent, deleteEvent), and optional chaining properly handles undefined values.Also applies to: 164-165, 192-193, 237-238
128-132: Verify: Only first calendar used despite "multiple host" in PR title.The implementation extracts only the first element from
destinationCalendararray. The PR title mentions "handle collective multiple host on destinationCalendar," but this code doesn't iterate over multiple calendars. Confirm whether:
- This is the intended behavior (main host calendar only)
- Multi-calendar support is implemented elsewhere in the EventManager/CalendarManager
- Additional calendar destinations should be processed here
Run the following script to check how multiple destination calendars are handled across calendar services:
#!/bin/bash # Description: Verify if other calendar services handle multiple destination calendars # Search for destinationCalendar array iteration patterns rg -nP --type=ts -C3 'destinationCalendar.*forEach|destinationCalendar.*map\(|for.*destinationCalendar' # Search for similar mainHostDestinationCalendar patterns in other calendar services rg -nP --type=ts -C2 'mainHostDestinationCalendar.*=.*destinationCalendar'apps/web/pages/api/cron/bookingReminder.ts (1)
107-107: LGTM: Correct array wrapping with proper fallback.The code correctly wraps the destination calendar in an array when constructing the CalendarEvent, with appropriate fallback logic (booking → user → empty array).
Also applies to: 130-130
packages/core/builders/CalendarEvent/class.ts (1)
26-26: All CalendarEvent consumers properly handle the array type. The type change fromDestinationCalendar | nulltoDestinationCalendar[] | nullhas been consistently applied across the codebase. All handlers correctly wrap single database values into arrays when constructing CalendarEvent objects (e.g.,[booking.destinationCalendar]), and all access patterns properly handle it as an array using destructuring, iteration, length checks, and indexing.packages/app-store/office365calendar/lib/CalendarService.ts (1)
73-77: Approve array-based destinationCalendar handling.The extraction of
mainHostDestinationCalendarfrom the array and the fallback logic for constructing the events URL is correct and handles edge cases appropriately.packages/core/builders/CalendarEvent/builder.ts (1)
1-2: LGTM: Proper type-only import forBooking.Converting
Bookingto a type-only import is correct since it's only used in type positions (line 237). This follows TypeScript best practices and may improve compilation performance.packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts (1)
240-242: LGTM: Correct array transformation for destinationCalendar.The conditional wrapping of
destinationCalendarin an array aligns with the refactored array-based architecture and properly handles the null/undefined case with an empty array.packages/types/Calendar.d.ts (1)
171-171: Approve destinationCalendar array type change.The change from
DestinationCalendar | nulltoDestinationCalendar[] | nullcorrectly enables multiple calendars per event as intended by the PR.packages/lib/CalendarService.ts (2)
156-164: Approve array-based calendar filtering logic.The extraction of
mainHostDestinationCalendarand its use for filtering calendars byexternalIdcorrectly implements the multi-calendar architecture. The fallback whenexternalIdis not present ensures all calendars are included when no specific destination is set.
509-516: Approve primary calendar determination logic.The logic correctly determines the primary calendar by comparing against
mainHostDestinationCalendar.externalId. This ensures the first destination calendar in the array is marked as primary.packages/core/CalendarManager.ts (4)
220-221: LGTM on signature change.The addition of the optional
externalIdparameter aligns with the multi-calendar architecture, allowing the caller to pass through the destination calendar's external ID for proper reference tracking.
230-230: Good spelling fix.Corrected "organiser" to "organizer" for consistency with US English spelling used elsewhere in the codebase.
234-254: Improved error handling with 404 short-circuit.The 404 handling is a sensible addition for cases where the
externalIddoesn't match the credential's calendar. However, note thatsuccessis set tofalseon line 238 before the 404 check, so the returned result will havesuccess: falsewithcreatedEvent: undefinedfor 404 errors. This is correct behavior.
267-268: LGTM on extended return payload.Adding
externalIdandcredentialIdto the return object enables proper reference tracking for multi-calendar bookings.packages/app-store/googlecalendar/lib/CalendarService.ts (3)
87-87: LGTM on signature change.The
credentialIdparameter enables proper calendar selection when multiple destination calendars are configured.
100-103: Array destructuring for destination calendar extraction.The extraction pattern is consistent with other calendar services. Note that when
destinationCalendaris empty or undefined,mainHostDestinationCalendarwill beundefined, which is handled correctly downstream with optional chaining.
145-154: LGTM on credential-based calendar selection.The logic correctly finds the matching calendar by
credentialIdand falls back to "primary" when no match is found.packages/features/bookings/lib/handleCancelBooking.ts (3)
251-255: LGTM on array-based destinationCalendar.The wrapping of
destinationCalendarin an array with proper fallback logic aligns with the new multi-calendar architecture.
418-420: LGTM on multi-reference filtering.The filter correctly identifies all calendar references for subsequent processing.
528-532: Consistent array wrapping for paid booking path.Matches the pattern used earlier in the file for the main cancellation flow.
packages/features/bookings/lib/handleNewBooking.ts (6)
1-1: LGTM on type import.Adding the
DestinationCalendartype import is necessary for the new array-based calendar handling.
688-689: LGTM on simplified scheduling type check.The check
!!eventType.schedulingType && ["COLLECTIVE", "ROUND_ROBIN"].includes(eventType.schedulingType)is cleaner and functionally equivalent.
724-763: LGTM on loadUsers function.The function is well-structured with:
- Proper validation of
dynamicUserList- Fallback from hosts to
eventType.users- Appropriate error handling with HTTP status codes
987-1006: LGTM on team destination calendar aggregation.The logic correctly collects destination calendars only for COLLECTIVE scheduling, enabling events to be created on all team members' calendars.
1063-1067: LGTM on array-based destinationCalendar.Proper array wrapping with fallback from event type to organizer's calendar.
1874-1879: Only the first destination calendar is connected to the booking.This connects only
evt.destinationCalendar[0]to the booking record. This appears intentional since the booking record has a singledestinationCalendarIdfield, but verify this is the expected behavior for collective events where multiple calendars are involved.packages/core/EventManager.ts (7)
169-170: LGTM on reference field mapping.Correctly maps
externalIdandcredentialIdfrom the result for calendar-type events.
208-209: LGTM on updateLocation reference mapping.Consistent with the create flow.
479-497: LGTM on calendarReference as array.The refactor to treat
calendarReferenceas an array with proper filtering supports the multi-calendar flow.
504-550: LGTM on multi-reference update loop.The iteration over references with credential resolution (including DB fallback) and proper update calls is well-structured. The fallback to matching credentials by type when no
credentialIdis present maintains backward compatibility.
574-583: Good addition of failure entry for missing calendar references.Previously, missing calendar references for "other_calendar" types would silently fail. Now they return a proper failure entry with
success: false.
597-608: Improved error handling returns structured failures.The error handling now returns a mapped array of failure entries for all calendar references instead of a single generic failure. This provides better diagnostics for multi-calendar scenarios.
338-407: Well-structured multi-calendar event creation.The logic properly handles:
- Credential resolution with DB fallback when not in memory
- Calendar selection by
credentialIdor by integration type- Fallback to first available calendar when no destination is specified
- Non-traditional "other_calendar" integrations
The credential construction from DB (lines 357-366) includes all required fields of the
CredentialWithAppNametype (id, appId, type, userId, teamId, key, invalid, appName), making it fully compatible with calendar operations.
| const selectedCalendar = externalCalendarId | ||
| ? externalCalendarId | ||
| : event.destinationCalendar?.externalId; | ||
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; |
There was a problem hiding this comment.
Logic bug: calendar lookup is ineffective.
When externalCalendarId is truthy, it's returned directly (correct). But when it's falsy, the .find() compares cal.externalId === externalCalendarId where externalCalendarId is falsy (e.g., null or undefined), which won't match any calendar with a valid externalId. This makes the fallback branch ineffective.
If the intent is to find a calendar by some other criteria when externalCalendarId is not provided, the logic needs to be revised. If the intent is to just use externalCalendarId when provided and otherwise leave it undefined, then the .find() clause is dead code.
🔎 Proposed fix (assuming you want undefined when not provided)
const selectedCalendar = externalCalendarId
- ? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ ? externalCalendarId
+ : undefined;Or if you need to resolve from destinationCalendar by credentialId (similar to createEvent), you'd need to pass credentialId to updateEvent as well.
📝 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 selectedCalendar = externalCalendarId | |
| ? externalCalendarId | |
| : event.destinationCalendar?.externalId; | |
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; | |
| const selectedCalendar = externalCalendarId | |
| ? externalCalendarId | |
| : undefined; |
🤖 Prompt for AI Agents
In @packages/app-store/googlecalendar/lib/CalendarService.ts around lines 254 -
256, The current selection logic for selectedCalendar in updateEvent is wrong
because it does .find(cal => cal.externalId === externalCalendarId) when
externalCalendarId is falsy, which never matches; either simplify to just use
externalCalendarId (i.e., set selectedCalendar = externalCalendarId) if you
intend undefined when not provided, or implement the intended resolution by
credential like createEvent: change the expression to
event.destinationCalendar?.find(cal => cal.credentialId ===
credentialId)?.externalId, and update the updateEvent signature and all its
callers to accept and pass credentialId so the lookup can succeed.
| const calendarId = externalCalendarId | ||
| ? externalCalendarId | ||
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; |
There was a problem hiding this comment.
Same logic bug as in updateEvent.
The .find() clause when externalCalendarId is falsy will never match, making this dead code.
🔎 Proposed fix
const calendarId = externalCalendarId
- ? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ ? externalCalendarId
+ : undefined;📝 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 calendarId = externalCalendarId | |
| ? externalCalendarId | |
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; | |
| const calendarId = externalCalendarId | |
| ? externalCalendarId | |
| : undefined; |
🤖 Prompt for AI Agents
In @packages/app-store/googlecalendar/lib/CalendarService.ts around lines 315 -
317, The current assignment to calendarId uses
event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)
which will never match when externalCalendarId is falsy; change the fallback to
select a valid externalId from destinationCalendar instead (e.g., use
event.destinationCalendar?.[0]?.externalId or find by a sensible property like
isPrimary) so that calendarId = externalCalendarId ??
event.destinationCalendar?.[0]?.externalId; update the code referencing
calendarId to rely on this corrected fallback.
| // @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") { | ||
| evt["location"] = "integrations:daily"; | ||
| } |
There was a problem hiding this comment.
Potential null pointer access on mainHostDestinationCalendar.
When evt.destinationCalendar is an empty array [], the destructuring const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; results in mainHostDestinationCalendar being undefined. Accessing .integration on line 119 will throw a TypeError.
🔎 Proposed fix
// @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") {
+ if (evt.location === MeetLocationType && mainHostDestinationCalendar?.integration !== "google_calendar") {
evt["location"] = "integrations:daily";
}📝 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.
| // @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") { | |
| evt["location"] = "integrations:daily"; | |
| } | |
| // @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") { | |
| evt["location"] = "integrations:daily"; | |
| } |
🤖 Prompt for AI Agents
In @packages/core/EventManager.ts around lines 117 - 121, The code destructures
evt.destinationCalendar into mainHostDestinationCalendar but doesn’t guard
against it being undefined; update the condition in EventManager (the block
using mainHostDestinationCalendar) to safely access integration — e.g., replace
the direct property access with a safe check using optional chaining or an
explicit existence check (mainHostDestinationCalendar?.integration !==
"google_calendar" or mainHostDestinationCalendar &&
mainHostDestinationCalendar.integration !== "google_calendar") before assigning
evt.location = "integrations:daily"; ensure the logic still only runs when
evt.location === MeetLocationType.
| const promises = bookingToDelete.user.credentials | ||
| .filter((credential) => credential.type.endsWith("_calendar")) | ||
| .map(async (credential) => { | ||
| const calendar = await getCalendar(credential); | ||
| for (const updBooking of updatedBookings) { | ||
| const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar")); | ||
| if (bookingRef) { | ||
| const { uid, externalCalendarId } = bookingRef; | ||
| const deletedEvent = await calendar?.deleteEvent(uid, evt, externalCalendarId); | ||
| apiDeletes.push(deletedEvent); | ||
| } | ||
| } |
There was a problem hiding this comment.
Recurring event deletion iterates over all calendar credentials.
For recurring events with allRemainingBookings, this code iterates over all user calendar credentials (line 449-450), not just the credential matching the current reference. This could result in:
- Attempting to delete the same event UID from multiple calendars where it doesn't exist
- Redundant API calls
Consider filtering to only the credentials that have matching booking references, or using the credential already resolved in the outer scope.
🔎 Proposed fix
- const promises = bookingToDelete.user.credentials
- .filter((credential) => credential.type.endsWith("_calendar"))
- .map(async (credential) => {
- const calendar = await getCalendar(credential);
- for (const updBooking of updatedBookings) {
- const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar"));
- if (bookingRef) {
- const { uid, externalCalendarId } = bookingRef;
- const deletedEvent = await calendar?.deleteEvent(uid, evt, externalCalendarId);
- apiDeletes.push(deletedEvent);
- }
- }
- });
+ // Use the already-resolved calendar for this reference
+ for (const updBooking of updatedBookings) {
+ const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar") && ref.credentialId === credentialId);
+ if (bookingRef) {
+ const { uid, externalCalendarId } = bookingRef;
+ apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId));
+ }
+ }Note: This is a simplified suggestion. You may need to adjust based on how recurring event references are structured.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/features/bookings/lib/handleCancelBooking.ts around lines 449 -
460, The code iterates bookingToDelete.user.credentials for every
updatedBooking, causing deletions to be attempted against all user calendars
instead of the one referenced; change the logic to locate the specific calendar
credential that matches each booking reference and call getCalendar only for
that credential (or reuse the credential resolved in the outer scope) before
invoking calendar.deleteEvent. Concretely, for each updBooking in
updatedBookings find bookingRef (existing code), then find the matching
credential from bookingToDelete.user.credentials (e.g., by id/type that
corresponds to bookingRef), call getCalendar(matchingCredential) and call
deleteEvent on that calendar, pushing the result into apiDeletes; avoid
iterating over all credentials.
| if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") { | ||
| evt.destinationCalendar?.push(...teamDestinationCalendars); | ||
| } |
There was a problem hiding this comment.
Potential silent failure when destinationCalendar is null.
On line 1063-1067, destinationCalendar can be set to null if neither the event type nor the organizer has a destination calendar. In that case, evt.destinationCalendar?.push(...teamDestinationCalendars) will silently do nothing due to optional chaining.
If team calendars should always be included for COLLECTIVE events, consider initializing to an empty array instead of null.
🔎 Proposed fix
destinationCalendar: eventType.destinationCalendar
? [eventType.destinationCalendar]
: organizerUser.destinationCalendar
? [organizerUser.destinationCalendar]
- : null,
+ : [],Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/features/bookings/lib/handleNewBooking.ts around lines 1077 - 1079,
The code can silently skip adding team calendars because evt.destinationCalendar
may be null; ensure COLLECTIVE team calendars are always added by initializing
destinationCalendar to an array when needed before pushing: in the block guarded
by isTeamEventType && eventType.schedulingType === "COLLECTIVE", check
evt.destinationCalendar and if null set evt.destinationCalendar = [] (or copy
existing values) then push(...teamDestinationCalendars); this ensures
evt.destinationCalendar always accepts the teamDestinationCalendars rather than
being a no-op due to optional chaining.
| ...(IS_TEAM_BILLING_ENABLED ? { slug } : {}), | ||
| metadata: { | ||
| ...(IS_TEAM_BILLING_ENABLED && { requestedSlug: slug }), | ||
| ...(IS_TEAM_BILLING_ENABLED ? { requestedSlug: slug } : {}), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the file context around lines 151-153
head -180 packages/trpc/server/routers/viewer/organizations/create.handler.ts | tail -40Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 230
🏁 Script executed:
# Search for how slug is used in organization-related queries and routing
rg -nP --type=ts 'slug.*team|organization.*slug' packages/trpc/server/routers/viewer/organizations/ -A 2 -B 2Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 2126
🏁 Script executed:
# Find organization creation and where slug is used
rg -nP --type=ts 'findUnique|findFirst' packages/trpc/server/routers/viewer/organizations/create.handler.ts -A 3 -B 3Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 476
🏁 Script executed:
# Check if there are any git history or comments explaining the IS_TEAM_BILLING_ENABLED flag logic
rg -nP 'IS_TEAM_BILLING_ENABLED' packages/trpc/server/routers/viewer/organizations/create.handler.ts -B 2 -A 2Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 573
🏁 Script executed:
# Check the full context of the NoOrganizationSlugError check in createTeams
sed -n '60,75p' packages/trpc/server/routers/viewer/organizations/createTeams.handler.tsRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 574
🏁 Script executed:
# Look for tests related to organization creation with/without billing
fd -e test -e spec -g '*/organizations/*' -type f | xargs rg -l 'IS_TEAM_BILLING_ENABLED|createHandler' 2>/dev/null | head -5Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 545
🏁 Script executed:
# Check if organization creation is tested, particularly around slug handling
rg -nP 'createHandler|create.*organization' packages/trpc/server/routers/viewer/organizations/create.handler.ts -A 1 -B 1 | head -30Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 461
🏁 Script executed:
# Look at the full team creation logic to understand the dependency
cat packages/trpc/server/routers/viewer/organizations/create.handler.ts | head -160 | tail -30Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1018
🏁 Script executed:
# Check the organization creation flow and when createTeams is called
rg -nP 'createTeams|create.*team' packages/trpc/server/routers/viewer/organizations/ -B 3 -A 3 | head -50Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 3670
🏁 Script executed:
# Look for the router definition to see how createHandler and createTeams are used
rg -nP 'procedure|router' packages/trpc/server/routers/viewer/organizations/create.handler.tsRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 69
🏁 Script executed:
# Check if there's conditional logic preventing createTeams when billing is disabled
rg -nP 'IS_TEAM_BILLING_ENABLED' packages/trpc/server/routers/viewer/organizations/ -B 3 -A 3Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 6507
🏁 Script executed:
# Check if createTeams has any guards/early returns for non-billing scenarios
sed -n '18,70p' packages/trpc/server/routers/viewer/organizations/createTeams.handler.tsRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1310
🏁 Script executed:
# Look at what requestedSlug is used for - is it only billing-related?
rg -nP 'requestedSlug' packages/trpc/server/routers/viewer/organizations/ -B 2 -A 2 | head -60Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 4488
🏁 Script executed:
# Check if slug is used for routing/URL generation
rg -nP 'slug.*url|url.*slug|routing' packages/trpc/server/routers/viewer/organizations/ -i -B 1 -A 1Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 69
🏁 Script executed:
# Check if createTeams is only called when billing is enabled
rg -nP 'createTeams|IS_TEAM_BILLING_ENABLED' packages/ -B 5 -A 5 --type ts -g '!node_modules' | grep -A 10 -B 10 'createTeams' | head -80Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 7931
🏁 Script executed:
# Check the create.handler.ts around line 180+ to see what happens after organization creation
sed -n '170,202p' packages/trpc/server/routers/viewer/organizations/create.handler.tsRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 877
🏁 Script executed:
# Look for any error handling or guards related to slug being missing
rg -nP 'slug|NoOrganizationSlug' packages/trpc/server/routers/viewer/organizations/create.handler.ts -B 2 -A 2Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1544
🏁 Script executed:
# Check if there's any routing/URL logic that depends on slug for all organizations
rg -nP 'organization.*url|subdomain|routing' packages/ --type ts -g '!node_modules' -i | grep -v node_modules | head -40Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 4691
🏁 Script executed:
# Check how non-slug organizations are handled elsewhere
rg -nP 'organization.*:id|organizations/\[' packages/ --type ts -g '!node_modules' | head -20Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 69
🏁 Script executed:
# Check if there are migration files or comments explaining this change
git log --oneline -20 --all --grep="slug\|billing" -- packages/trpc/server/routers/viewer/organizations/create.handler.ts 2>/dev/null || echo "Git history unavailable"Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 69
🏁 Script executed:
# Check if slug collision validation should be gated too
sed -n '65,90p' packages/trpc/server/routers/viewer/organizations/create.handler.tsRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 884
🏁 Script executed:
# Verify what the input validation requires
cat packages/trpc/server/routers/viewer/organizations/create.schema.ts | head -50Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 489
🏁 Script executed:
# Check if IS_TEAM_BILLING_ENABLED check should gate the slug validation/collision check too
rg -nP 'IS_TEAM_BILLING_ENABLED' packages/trpc/server/routers/viewer/organizations/create.handler.ts -B 5 -A 5 | head -40Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1089
🏁 Script executed:
# Look for any error handling if slug is required but not set
rg -nP 'slug.*undefined|slug.*null' packages/trpc/server/routers/viewer/organizations/create.handler.tsRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 69
The slug validation and persistence gating are inconsistent and will break the team creation workflow.
The slug collision check (lines 74-85) executes unconditionally, but the slug is only persisted when IS_TEAM_BILLING_ENABLED is true (lines 151-153). This means:
- When billing is disabled, organizations are created without
slugorrequestedSlug - Downstream calls to
createTeamswill fail withNoOrganizationSlugErrorbecause the validation at line 67 ofcreateTeams.handler.tsrequires eithermetadata.requestedSlugororganization.slug
Either the slug validation should also be gated by IS_TEAM_BILLING_ENABLED (if slugs aren't needed for non-billing organizations), or the slug should always be persisted regardless of billing status.
🤖 Prompt for AI Agents
In @packages/trpc/server/routers/viewer/organizations/create.handler.ts around
lines 151 - 153, The slug collision check in create.handler.ts runs
unconditionally but slug persistence is gated by IS_TEAM_BILLING_ENABLED,
causing downstream createTeams (which validates for metadata.requestedSlug or
organization.slug and can throw NoOrganizationSlugError) to fail; fix by making
behavior consistent—either wrap the slug uniqueness check (the block that
validates slug collision) with the same IS_TEAM_BILLING_ENABLED guard so slug is
neither validated nor stored when billing is disabled, or always persist the
slug and metadata.requestedSlug regardless of IS_TEAM_BILLING_ENABLED so
downstream createTeams receives a slug; update the logic around slug,
metadata.requestedSlug, and the collision check to use the same gating approach
to eliminate the mismatch.
|
|
||
| export interface Calendar { | ||
| createEvent(event: CalendarEvent): Promise<NewCalendarEventType>; | ||
| createEvent(event: CalendarEvent, credentialId: number): Promise<NewCalendarEventType>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the Calendar interface definition
cat -n packages/types/Calendar.d.ts | head -250 | tail -40Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 1550
🏁 Script executed:
# Find all files that might implement Calendar
find . -type f \( -name "*.ts" -o -name "*.tsx" \) ! -path "*/node_modules/*" ! -path "*/dist/*" | xargs grep -l "implements.*Calendar" 2>/dev/null | head -20Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 961
🏁 Script executed:
# Search for createEvent method definitions across calendar services
rg -nA 3 'createEvent\s*\(' --type=ts --type=tsx -g '!node_modules' -g '!dist' | head -100Repository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 114
🏁 Script executed:
# Check createEvent signatures in all CalendarService.ts files
for file in $(find ./packages/app-store -name "CalendarService.ts" ! -path "*/node_modules/*"); do
echo "=== $file ==="
rg -A 2 'createEvent\s*\(' "$file"
doneRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 3220
🏁 Script executed:
# Also check the base CalendarService
echo "=== packages/lib/CalendarService.ts ==="
rg -A 2 'createEvent\s*\(' ./packages/lib/CalendarService.tsRepository: AI-Code-Review-Evals/coderabbit-cal_dot_com
Length of output: 476
Update 13 Calendar implementations to include credentialId parameter.
The createEvent interface signature now requires a credentialId: number parameter, but only the Google Calendar implementation has been updated. The following implementations are still missing this parameter and must be updated to match the interface:
- basecamp3
- closecom
- exchange2013calendar
- exchange2016calendar
- exchangecalendar
- hubspot
- larkcalendar
- office365calendar
- salesforce
- sendgrid
- zoho-bigin
- zohocrm
- packages/lib/CalendarService.ts
🤖 Prompt for AI Agents
In @packages/types/Calendar.d.ts at line 221, The createEvent interface
signature now requires credentialId: number but many implementations still have
the old signature; update each implementation method to accept the new parameter
and pass it through or use it where appropriate. Specifically, change the method
signature for createEvent in the classes/objects named
Basecamp3Calendar.createEvent, CloseComCalendar.createEvent,
Exchange2013Calendar.createEvent, Exchange2016Calendar.createEvent,
ExchangeCalendar.createEvent, HubSpotCalendar.createEvent,
LarkCalendar.createEvent, Office365Calendar.createEvent,
SalesforceCalendar.createEvent, SendGridCalendar.createEvent,
ZohoBiginCalendar.createEvent, ZohoCrmCalendar.createEvent, and the higher-level
CalendarService.createEvent to include credentialId: number, adjust any internal
calls to forward that credentialId to downstream helpers or HTTP clients, and
update return handling to continue returning Promise<NewCalendarEventType>.
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)*