fix: handle collective multiple host on destinationCalendar#4
Conversation
WalkthroughThis change set refactors the handling of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant EventManager
participant CalendarService
participant CredentialDB
User->>API: Create/Update Booking
API->>EventManager: create(event) / update(event)
EventManager->>CredentialDB: Fetch credentials for each destinationCalendar
loop For each destinationCalendar
EventManager->>CalendarService: createEvent(event, credentialId)
CalendarService->>CalendarService: Select calendar by credentialId
CalendarService->>EventManager: Return event result (with externalId)
end
EventManager->>API: Return result with all calendar references
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
packages/core/EventManager.ts (1)
338-397: Refactor duplicate credential fetching logicThe credential fetching logic is duplicated here and in
updateAllCalendarEvents. This increases maintenance burden and potential for bugs.Consider extracting a helper method:
private async getCalendarCredential(credentialId: number): Promise<CredentialWithAppName | null> { let credential = this.calendarCredentials.find((c) => c.id === credentialId); if (!credential) { const credentialFromDB = await prisma.credential.findUnique({ include: { app: { select: { slug: true, }, }, }, where: { id: credentialId, }, }); if (credentialFromDB && credentialFromDB.app?.slug) { credential = { appName: credentialFromDB.app.slug, id: credentialFromDB.id, type: credentialFromDB.type, key: credentialFromDB.key, userId: credentialFromDB.userId, teamId: credentialFromDB.teamId, invalid: credentialFromDB.invalid, appId: credentialFromDB.appId, }; } } return credential || null; }Also consider batching credential fetches to reduce database queries:
const missingCredentialIds = event.destinationCalendar ?.filter(dest => dest.credentialId && !this.calendarCredentials.find(c => c.id === dest.credentialId)) .map(dest => dest.credentialId) ?? []; if (missingCredentialIds.length > 0) { const credentials = await prisma.credential.findMany({ where: { id: { in: missingCredentialIds } }, include: { app: { select: { slug: true } } } }); // Process and add to this.calendarCredentials }
♻️ Duplicate comments (2)
packages/app-store/googlecalendar/lib/CalendarService.ts (1)
315-317: Same logic error indeleteEventas inupdateEvent.The calendar selection logic has the same circular search issue. When
externalCalendarIdis provided, the find operation will never match.Apply similar fix as suggested for
updateEvent:const calendarId = externalCalendarId - ? externalCalendarId - : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; + || event.destinationCalendar?.[0]?.externalId + || defaultCalendarId;packages/core/EventManager.ts (1)
503-550: Duplicated credential fetching logicThis is the same credential fetching logic as in
createAllCalendarEvents. As mentioned earlier, this should be extracted to a shared method.
🧹 Nitpick comments (1)
packages/app-store/googlecalendar/lib/CalendarService.ts (1)
87-87: Well-implemented support for multiple destination calendars increateEvent.The code correctly:
- Extracts the first calendar for backward compatibility
- Selects the appropriate calendar based on
credentialId- Falls back to "primary" when no match is found
One minor suggestion: Line 168 still uses
selectedCalendarwithout the fallback to "primary". Consider applying the same fallback pattern:- calendarId: selectedCalendar, + calendarId: selectedCalendar || "primary",Also applies to: 100-103, 122-124, 147-154
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/web/pages/api/cron/bookingReminder.ts(2 hunks)apps/web/playwright/webhook.e2e.ts(1 hunks)packages/app-store/googlecalendar/lib/CalendarService.ts(8 hunks)packages/app-store/larkcalendar/lib/CalendarService.ts(4 hunks)packages/app-store/office365calendar/lib/CalendarService.ts(1 hunks)packages/core/CalendarManager.ts(3 hunks)packages/core/EventManager.ts(8 hunks)packages/core/builders/CalendarEvent/builder.ts(1 hunks)packages/core/builders/CalendarEvent/class.ts(1 hunks)packages/emails/src/templates/BrokenIntegrationEmail.tsx(1 hunks)packages/features/bookings/lib/handleCancelBooking.ts(3 hunks)packages/features/bookings/lib/handleNewBooking.ts(8 hunks)packages/features/ee/payments/api/paypal-webhook.ts(1 hunks)packages/features/ee/payments/api/webhook.ts(4 hunks)packages/lib/CalendarService.ts(2 hunks)packages/trpc/server/routers/loggedInViewer/deleteCredential.handler.ts(1 hunks)packages/trpc/server/routers/viewer/bookings/confirm.handler.ts(1 hunks)packages/trpc/server/routers/viewer/bookings/editLocation.handler.ts(1 hunks)packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts(1 hunks)packages/trpc/server/routers/viewer/organizations/create.handler.ts(1 hunks)packages/types/Calendar.d.ts(4 hunks)packages/types/EventManager.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/trpc/server/routers/viewer/organizations/create.handler.ts (1)
packages/lib/constants.ts (1)
IS_TEAM_BILLING_ENABLED(88-88)
packages/core/CalendarManager.ts (1)
packages/types/Calendar.d.ts (1)
CalendarEvent(146-190)
packages/core/EventManager.ts (4)
packages/app-store/locations.ts (1)
MeetLocationType(54-54)packages/prisma/index.ts (1)
prisma(56-56)packages/core/CalendarManager.ts (2)
createEvent(218-270)updateEvent(272-323)packages/types/EventManager.d.ts (2)
PartialReference(3-12)EventResult(14-27)
packages/features/bookings/lib/handleNewBooking.ts (3)
packages/prisma/index.ts (1)
prisma(56-56)packages/prisma/selects/user.ts (1)
userSelect(36-51)packages/lib/server/i18n.ts (1)
getTranslation(5-18)
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)
packages/app-store/googlecalendar/lib/CalendarService.ts (1)
packages/types/Calendar.d.ts (2)
CalendarEvent(146-190)NewCalendarEventType(62-70)
🔇 Additional comments (34)
packages/core/builders/CalendarEvent/builder.ts (1)
1-2: LGTM! Good TypeScript import optimization.The conversion to type-only import for
Bookingand the separation ofPrismaimport improves type safety and import clarity.packages/types/EventManager.d.ts (1)
26-26: LGTM! Appropriate addition for multi-calendar support.The optional
externalIdproperty enhances theEventResult<T>interface to support external calendar identification in multi-calendar workflows while maintaining backward compatibility.apps/web/playwright/webhook.e2e.ts (1)
249-249: No changes needed: tests correctly reflect distinctdestinationCalendarbehavior
The webhook payload builder intentionally returnsnullfor newly created/requested bookings (viahandleNewBooking) and an empty array[]for rejected bookings (viahandleCancelBooking). The existing assertions inBOOKING_CREATED,BOOKING_REQUESTED, andBOOKING_REJECTEDaccurately mirror this logic—no update required.Likely an incorrect or invalid review comment.
packages/core/builders/CalendarEvent/class.ts (1)
26-26: LGTM! Critical foundational change for multi-calendar support.The type change from
DestinationCalendar | nulltoDestinationCalendar[] | nullis essential for supporting multiple destination calendars per event. This foundational update enables the system-wide transition to multi-calendar handling while maintaining backward compatibility by keeping the property nullable.packages/trpc/server/routers/loggedInViewer/deleteCredential.handler.ts (1)
290-294: LGTM! Proper migration to array-based destinationCalendar.The logic correctly handles the transition to array-based
destinationCalendarby:
- Wrapping
booking.destinationCalendarin an array if it exists- Falling back to
booking.user?.destinationCalendarwrapped in an array- Providing an empty array as the final fallback
This ensures consistent array handling while maintaining the original logic flow for calendar selection.
packages/trpc/server/routers/viewer/bookings/requestReschedule.handler.ts (1)
240-242: LGTM! Consistent with the destinationCalendar array refactor.The change correctly wraps the
destinationCalendarin an array when it exists, or provides an empty array otherwise. This aligns with the systematic refactor to standardizedestinationCalendaras an array throughout the codebase.packages/trpc/server/routers/viewer/bookings/confirm.handler.ts (1)
175-179: LGTM! Well-structured fallback logic for destinationCalendar array.The implementation correctly prioritizes
booking.destinationCalendaroveruser.destinationCalendarand ensures the result is always an array. The fallback chain maintains backwards compatibility while conforming to the new array structure.apps/web/pages/api/cron/bookingReminder.ts (1)
107-107: LGTM! Clean approach using intermediate variable.The use of
selectedDestinationCalendaras an intermediate variable makes the logic more readable while maintaining the same fallback behavior (booking → user → empty array). This approach aligns well with the destinationCalendar array refactor.Also applies to: 130-130
packages/features/ee/payments/api/paypal-webhook.ts (1)
152-156: LGTM! Consistent with the destinationCalendar array standardization.The fallback logic correctly prioritizes
booking.destinationCalendaroveruser.destinationCalendarand ensures the result is always an array, maintaining consistency with the broader refactor across the codebase.packages/features/ee/payments/api/webhook.ts (2)
101-101: LGTM! Clean implementation in getBooking function.The use of
selectedDestinationCalendarintermediate variable and array wrapping follows the established pattern for the destinationCalendar refactor, making the code readable and consistent.Also applies to: 119-119
207-207: LGTM! Consistent implementation in handlePaymentSuccess function.The same clean pattern is applied here using the intermediate variable approach. The implementation maintains consistency with the
getBookingfunction and the broader destinationCalendar array refactor.Also applies to: 229-229
packages/trpc/server/routers/viewer/bookings/editLocation.handler.ts (1)
85-89: LGTM! Proper array conversion for destinationCalendar.The implementation correctly handles the conversion from single object to array format with proper null safety and fallback logic.
packages/app-store/larkcalendar/lib/CalendarService.ts (4)
128-129: LGTM! Consistent array destructuring pattern.The destructuring of the first destination calendar from the array is implemented correctly with proper fallback handling.
164-165: LGTM! Consistent implementation.Same correct pattern applied in the createAttendees method.
192-193: LGTM! Consistent implementation.Same correct pattern applied in the updateEvent method.
237-238: LGTM! Consistent implementation.Same correct pattern applied in the deleteEvent method.
packages/emails/src/templates/BrokenIntegrationEmail.tsx (1)
88-91: LGTM! Proper array handling for calendar integration name.The destructuring pattern correctly extracts the first destination calendar and handles the undefined case with appropriate fallback logic.
packages/lib/CalendarService.ts (2)
156-164: LGTM! Proper calendar filtering logic.The destructuring pattern correctly extracts the first destination calendar for filtering, with appropriate fallback when no specific calendar is specified.
509-516: LGTM! Correct primary calendar determination.The implementation properly uses the first destination calendar's externalId to determine which calendar should be marked as primary.
packages/app-store/office365calendar/lib/CalendarService.ts (1)
73-77: LGTM! Proper calendar URL construction.The destructuring pattern correctly extracts the first destination calendar and constructs the appropriate Office365 API endpoint with proper fallback to the default calendar.
packages/types/Calendar.d.ts (3)
11-11: LGTM! Necessary import for the newschedulingTypeproperty.
171-171: Breaking change:destinationCalendaris now an array.This change from a single
DestinationCalendarobject to an array enables support for multiple destination calendars per event. The addition of theschedulingTypeproperty provides scheduling context for events.Ensure all consumers of the
CalendarEventinterface have been updated to handle the array type.Also applies to: 182-182
221-221: Interface change:createEventnow requirescredentialIdparameter.This addition enables calendar services to select the appropriate destination calendar based on the credential ID when multiple calendars are available.
packages/core/CalendarManager.ts (4)
220-221: Good addition ofexternalIdparameter for tracking purposes.The optional parameter maintains backward compatibility while enabling external ID tracking.
235-236: Correct implementation of the newcreateEventsignature.Properly passes
credential.idas required by the updated interface.
237-254: Improved error handling with specific 404 case handling.Good approach to handle mismatched calendar/credential pairs gracefully by returning
undefinedfor 404 errors. ThecalErrorextraction provides better error visibility.
267-268: Good addition of tracking fields to the return object.Including both
externalIdandcredentialIdin the result enables better event tracking and debugging.packages/features/bookings/lib/handleCancelBooking.ts (1)
251-255: Correct implementation ofdestinationCalendaras array.The nested ternary properly handles backward compatibility by checking both booking-level and user-level destination calendars, wrapping single objects in arrays.
packages/features/bookings/lib/handleNewBooking.ts (4)
1-1: LGTM!The import of
DestinationCalendartype is necessary for proper typing of theteamDestinationCalendarsarray introduced in this change.
724-763: Well-structured error handling improvements!The enhanced validation and error handling in
loadUsersprovides better debugging context and proper HTTP status code propagation. The specific handling ofHttpErrorandPrisma.PrismaClientKnownRequestErrorensures appropriate error responses.
1874-1879: Correct handling of array to single calendar connectionThe implementation properly connects the first destination calendar from the array, maintaining backward compatibility with the database schema while supporting the new multiple calendar format.
987-1006: Null Safety for destinationCalendar ConfirmedThe Prisma schema shows
destinationCalendaris nullable, but the existing guard at lines 990–994:if (isTeamEventType && eventType.schedulingType === "COLLECTIVE" && user.destinationCalendar) { teamDestinationCalendars.push(user.destinationCalendar); }ensures only non-null
destinationCalendarvalues are collected. No further null checks are needed.packages/core/EventManager.ts (2)
169-170: Good addition of calendar metadata to referencesAdding
externalCalendarIdandcredentialIdto references enables proper tracking of which specific calendar and credential were used for each event, supporting the multi-calendar architecture.Also applies to: 208-209
117-121: Verify Google Calendar prioritization in MeetLocationType eventsIt looks like
destinationCalendaris still being handled by only inspecting its first element, and there’s no existing test or documentation enforcing a Google Calendar preference when multiple calendars are present. If your intent is to fall back to “integrations:daily” only when none of the calendars are Google, please confirm and update accordingly.• Location: packages/core/EventManager.ts (around lines 117–121)
• Current code:// @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"; }• Suggested change if we need to consider all calendars:
- const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; - if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") { + const hasGoogleCalendar = evt.destinationCalendar?.some( + (cal) => cal.integration === "google_calendar" + ); + if (evt.location === MeetLocationType && !hasGoogleCalendar) { evt["location"] = "integrations:daily"; }Please verify whether multi-calendar events require this adjustment, and add or update tests/docs to reflect the intended behavior.
| const [mainHostDestinationCalendar] = | ||
| event?.destinationCalendar && event?.destinationCalendar.length > 0 ? event.destinationCalendar : []; |
There was a problem hiding this comment.
Logic error in calendar selection for updateEvent.
The calendar extraction for organizer email is correct, but there's a logic issue at line 256:
The current logic will never find a match because you're searching for a calendar where cal.externalId === externalCalendarId when externalCalendarId is already provided. This appears to be attempting to validate the externalCalendarId, but the logic is circular.
Consider removing this redundant check:
const selectedCalendar = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ : undefined;Or if you need to select a default calendar when externalCalendarId is not provided:
const selectedCalendar = externalCalendarId
- ? externalCalendarId
- : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
+ || mainHostDestinationCalendar?.externalId
+ || "primary";Also applies to: 228-230, 254-256
🤖 Prompt for AI Agents
In packages/app-store/googlecalendar/lib/CalendarService.ts around lines
228-230, 254-256, and 198-199, the logic for selecting the calendar using
externalCalendarId is flawed because it redundantly checks if a calendar's
externalId matches the already provided externalCalendarId, which is circular
and ineffective. To fix this, remove the redundant matching condition and
instead directly use the externalCalendarId if provided; if not provided,
implement logic to select a default calendar from the available calendars.
Adjust the code to handle both cases clearly without unnecessary or circular
checks.
| // Check if the disabledNotes flag is set to true | ||
| if (calEvent.hideCalendarNotes) { | ||
| calEvent.additionalNotes = "Notes have been hidden by the organiser"; // TODO: i18n this string? | ||
| calEvent.additionalNotes = "Notes have been hidden by the organizer"; // TODO: i18n this string? |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address the TODO: Internationalize this string.
The spelling change to American English is fine, but as the TODO indicates, this string should be properly internationalized.
Would you like me to help implement the internationalization for this string or create an issue to track it?
🤖 Prompt for AI Agents
In packages/core/CalendarManager.ts at line 230, the string assigned to
calEvent.additionalNotes is currently hardcoded and marked with a TODO for
internationalization. Replace this hardcoded string with a call to the
appropriate internationalization function or method used in the project, passing
the string key or default message to enable translation. If the i18n setup is
not yet in place, create a new issue to track adding internationalization
support for this string.
| newBookingId?: number | ||
| ): Promise<Array<EventResult<NewCalendarEventType>>> { | ||
| let calendarReference: PartialReference | undefined = undefined, | ||
| let calendarReference: PartialReference[] | undefined = undefined, |
There was a problem hiding this comment.
Fix variable declaration syntax
Multiple variables declared with single let statement can cause confusion and potential bugs.
- let calendarReference: PartialReference[] | undefined = undefined,
- credential;
+ let calendarReference: PartialReference[] | undefined = undefined;
+ let credential;📝 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.
| let calendarReference: PartialReference[] | undefined = undefined, | |
| let calendarReference: PartialReference[] | undefined = undefined; | |
| let credential; |
🤖 Prompt for AI Agents
In packages/core/EventManager.ts at line 479, the variable calendarReference is
declared using a single let statement alongside other variables, which can cause
confusion. Refactor the code to declare calendarReference in its own separate
let statement to improve clarity and reduce potential bugs.
| if (!calendarReference) { | ||
| return { | ||
| appName: cred.appName, | ||
| type: cred.type, | ||
| success: false, | ||
| uid: "", | ||
| originalEvent: event, | ||
| credentialId: cred.id, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling for missing calendar reference
When a calendar reference is missing for "other_calendar" types, the method returns a partial error result. However, this might not be the desired behavior - it could silently fail without proper error propagation.
Consider logging the error or throwing an exception:
if (!calendarReference) {
+ console.error(`Missing calendar reference for ${cred.type}`);
return {
appName: cred.appName,
type: cred.type,
success: false,
uid: "",
originalEvent: event,
credentialId: cred.id,
};
}📝 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.
| if (!calendarReference) { | |
| return { | |
| appName: cred.appName, | |
| type: cred.type, | |
| success: false, | |
| uid: "", | |
| originalEvent: event, | |
| credentialId: cred.id, | |
| }; | |
| } | |
| if (!calendarReference) { | |
| console.error(`Missing calendar reference for ${cred.type}`); | |
| return { | |
| appName: cred.appName, | |
| type: cred.type, | |
| success: false, | |
| uid: "", | |
| originalEvent: event, | |
| credentialId: cred.id, | |
| }; | |
| } |
🤖 Prompt for AI Agents
In packages/core/EventManager.ts around lines 574 to 583, the code returns a
partial error result when a calendar reference is missing, which may cause
silent failures. Instead, modify the code to either log a clear error message
indicating the missing calendar reference or throw an exception to ensure the
error is properly propagated and handled by the caller.
| const bookingCalendarReference = bookingToDelete.references.filter((reference) => | ||
| reference.type.includes("_calendar") | ||
| ); | ||
|
|
||
| if (bookingCalendarReference) { | ||
| const { credentialId, uid, externalCalendarId } = bookingCalendarReference; | ||
| // If the booking calendar reference contains a credentialId | ||
| if (credentialId) { | ||
| // Find the correct calendar credential under user credentials | ||
| const calendarCredential = bookingToDelete.user.credentials.find( | ||
| (credential) => credential.id === credentialId | ||
| ); | ||
| if (calendarCredential) { | ||
| const calendar = await getCalendar(calendarCredential); | ||
| if ( | ||
| bookingToDelete.eventType?.recurringEvent && | ||
| bookingToDelete.recurringEventId && | ||
| allRemainingBookings | ||
| ) { | ||
| 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); | ||
| if (bookingCalendarReference.length > 0) { | ||
| for (const reference of bookingCalendarReference) { | ||
| const { credentialId, uid, externalCalendarId } = reference; | ||
| // If the booking calendar reference contains a credentialId | ||
| if (credentialId) { | ||
| // Find the correct calendar credential under user credentials | ||
| let calendarCredential = bookingToDelete.user.credentials.find( | ||
| (credential) => credential.id === credentialId | ||
| ); | ||
| if (!calendarCredential) { | ||
| // get credential from DB | ||
| const foundCalendarCredential = await prisma.credential.findUnique({ | ||
| where: { | ||
| id: credentialId, | ||
| }, | ||
| }); | ||
| if (foundCalendarCredential) { | ||
| calendarCredential = foundCalendarCredential; | ||
| } | ||
| } | ||
| if (calendarCredential) { | ||
| const calendar = await getCalendar(calendarCredential); | ||
| if ( | ||
| bookingToDelete.eventType?.recurringEvent && | ||
| bookingToDelete.recurringEventId && | ||
| allRemainingBookings | ||
| ) { | ||
| 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); | ||
| } | ||
| } | ||
| }); | ||
| try { | ||
| await Promise.all(promises); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| logger.error(error.message); | ||
| } | ||
| }); | ||
| try { | ||
| await Promise.all(promises); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| logger.error(error.message); | ||
| } | ||
| } else { | ||
| apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); | ||
| } | ||
| } else { | ||
| } | ||
| } else { | ||
| // For bookings made before the refactor we go through the old behavior of running through each calendar credential | ||
| const calendarCredentials = bookingToDelete.user.credentials.filter((credential) => | ||
| credential.type.endsWith("_calendar") | ||
| ); | ||
| for (const credential of calendarCredentials) { | ||
| const calendar = await getCalendar(credential); | ||
| apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); | ||
| } | ||
| } | ||
| } else { | ||
| // For bookings made before the refactor we go through the old behaviour of running through each calendar credential | ||
| const calendarCredentials = bookingToDelete.user.credentials.filter((credential) => | ||
| credential.type.endsWith("_calendar") | ||
| ); | ||
| for (const credential of calendarCredentials) { | ||
| const calendar = await getCalendar(credential); | ||
| apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Comprehensive refactor for multi-calendar support with good error handling.
The implementation correctly:
- Handles multiple calendar references
- Fetches missing credentials from the database
- Maintains backward compatibility for legacy bookings
- Includes proper error handling
However, there's a potential issue at line 449-453 where you're filtering credentials again inside the recurring event block, which seems redundant since you already have the calendar instance.
Consider simplifying the recurring event deletion logic:
- const promises = bookingToDelete.user.credentials
- .filter((credential) => credential.type.endsWith("_calendar"))
- .map(async (credential) => {
- const calendar = await getCalendar(credential);
+ const promises = updatedBookings.map(async (updBooking) => {
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);
}
}
});This would use the already obtained calendar instance instead of creating new ones.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/features/bookings/lib/handleCancelBooking.ts between lines 418 and
484, the recurring event deletion logic redundantly filters user credentials and
creates new calendar instances inside the Promise.all block, despite already
having a calendar instance from the outer scope. To fix this, refactor the code
to use the existing calendar instance for deleting events in updatedBookings
instead of filtering credentials and creating new calendar instances again. This
will simplify the logic and avoid unnecessary operations.
| destinationCalendar: eventType.destinationCalendar | ||
| ? [eventType.destinationCalendar] | ||
| : organizerUser.destinationCalendar | ||
| ? [organizerUser.destinationCalendar] | ||
| : null, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify nested ternary and ensure consistent array type
The nested ternary operator reduces readability. Also, based on the EventManager changes, destinationCalendar should always be an array (possibly empty) rather than null.
- destinationCalendar: eventType.destinationCalendar
- ? [eventType.destinationCalendar]
- : organizerUser.destinationCalendar
- ? [organizerUser.destinationCalendar]
- : null,
+ destinationCalendar: eventType.destinationCalendar
+ ? [eventType.destinationCalendar]
+ : organizerUser.destinationCalendar
+ ? [organizerUser.destinationCalendar]
+ : [],📝 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.
| destinationCalendar: eventType.destinationCalendar | |
| ? [eventType.destinationCalendar] | |
| : organizerUser.destinationCalendar | |
| ? [organizerUser.destinationCalendar] | |
| : null, | |
| destinationCalendar: eventType.destinationCalendar | |
| ? [eventType.destinationCalendar] | |
| : organizerUser.destinationCalendar | |
| ? [organizerUser.destinationCalendar] | |
| : [], |
🤖 Prompt for AI Agents
In packages/features/bookings/lib/handleNewBooking.ts around lines 1063 to 1067,
simplify the nested ternary operator used to assign destinationCalendar by
replacing it with a clearer conditional structure. Ensure destinationCalendar is
always assigned an array type, using an empty array if neither
eventType.destinationCalendar nor organizerUser.destinationCalendar is present,
to maintain consistent typing and improve readability.
| if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") { | ||
| evt.destinationCalendar?.push(...teamDestinationCalendars); | ||
| } |
There was a problem hiding this comment.
Potential issue with null destinationCalendar
If evt.destinationCalendar is null (as it can be from lines 1063-1067), the optional chaining will prevent team calendars from being added.
if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") {
- evt.destinationCalendar?.push(...teamDestinationCalendars);
+ if (!evt.destinationCalendar) {
+ evt.destinationCalendar = [];
+ }
+ evt.destinationCalendar.push(...teamDestinationCalendars);
}📝 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.
| if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") { | |
| evt.destinationCalendar?.push(...teamDestinationCalendars); | |
| } | |
| if (isTeamEventType && eventType.schedulingType === "COLLECTIVE") { | |
| if (!evt.destinationCalendar) { | |
| evt.destinationCalendar = []; | |
| } | |
| evt.destinationCalendar.push(...teamDestinationCalendars); | |
| } |
🤖 Prompt for AI Agents
In packages/features/bookings/lib/handleNewBooking.ts around lines 1077 to 1079,
the code uses optional chaining on evt.destinationCalendar which can be null,
preventing teamDestinationCalendars from being added. To fix this, check if
evt.destinationCalendar is null and if so, initialize it as an empty array
before pushing teamDestinationCalendars. This ensures the calendars are always
added regardless of the initial state.
| ...(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.
💡 Verification agent
🧩 Analysis chain
Verify the inverted conditional logic for slug handling.
The conditional logic for slug and requestedSlug has been inverted. Now when IS_TEAM_BILLING_ENABLED is true, both the slug property and requestedSlug metadata are included, whereas when false, neither is included.
This represents a significant behavioral change:
- Before:
slugincluded when billing disabled,requestedSlugin metadata when billing enabled - After: Both
slugandrequestedSlugincluded when billing enabled, neither when billing disabled
Please confirm this change aligns with the intended organization creation workflow, particularly regarding how organizations without billing handle slug assignment.
🏁 Script executed:
#!/bin/bash
# Description: Search for usages of organization slug and requestedSlug to understand the impact
# Expected: Find how downstream code handles these properties
echo "=== Searching for organization slug usage ==="
rg -A 3 -B 3 "organization.*slug" --type ts
echo -e "\n=== Searching for requestedSlug usage ==="
rg -A 3 -B 3 "requestedSlug" --type ts
echo -e "\n=== Searching for IS_TEAM_BILLING_ENABLED usage in organization context ==="
rg -A 5 -B 5 "IS_TEAM_BILLING_ENABLED.*organization|organization.*IS_TEAM_BILLING_ENABLED" --type tsLength of output: 40608
Revert slug inclusion for billing-enabled orgs
When IS_TEAM_BILLING_ENABLED is true, we should only set requestedSlug in metadata (deferring the real slug until publish/payment). The current change adds both slug and requestedSlug on creation, which bypasses the payment flow and will break routing for unpaid orgs.
• File: packages/trpc/server/routers/viewer/organizations/create.handler.ts (around line 151)
• Invert the top-level slug spread so it only applies when billing is disabled:
- ...(IS_TEAM_BILLING_ENABLED ? { slug } : {}),
+ ...(IS_TEAM_BILLING_ENABLED ? {} : { slug }),The metadata block stays the same:
- ...(IS_TEAM_BILLING_ENABLED ? { requestedSlug: slug } : {}),
+ ...(IS_TEAM_BILLING_ENABLED ? { requestedSlug: slug } : {}),🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/organizations/create.handler.ts around
lines 151 to 153, the current code incorrectly includes the top-level slug when
IS_TEAM_BILLING_ENABLED is true, bypassing the payment flow. To fix this, invert
the condition so that the top-level slug is only included when
IS_TEAM_BILLING_ENABLED is false, while keeping the requestedSlug inside
metadata unchanged.
|
This PR is being marked as stale due to inactivity. |
2 participants
Test 4
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Style