Async import of the appStore packages#1
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request converts synchronous calendar and video adapter retrieval operations to asynchronous throughout the codebase. Key changes include: making Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/app-store/wipemycalother/lib/reschedule.ts (1)
125-134: Critical:forEachwith async callback creates unhandled promises.
Array.prototype.forEachdoes not await async callbacks. The promises returned by each iteration are discarded, meaning:
- Calendar/video deletions run as fire-and-forget operations
- The outer
try-catchwon't catch errors from these async callbacks- The function may return before deletions complete
🔎 Proposed fix using Promise.all with map
try { - bookingRefsFiltered.forEach(async (bookingRef) => { + await Promise.all(bookingRefsFiltered.map(async (bookingRef) => { if (bookingRef.uid) { if (bookingRef.type.endsWith("_calendar")) { const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); } else if (bookingRef.type.endsWith("_video")) { return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); } } - }); + })); } catch (error) {packages/app-store/vital/lib/reschedule.ts (1)
125-134: Critical:forEachwith async callback creates unhandled promises.Same issue as in
packages/app-store/wipemycalother/lib/reschedule.ts-forEachdoes not await async callbacks, resulting in fire-and-forget deletions and thetry-catchnot catching errors from these callbacks.🔎 Proposed fix using Promise.all with map
try { - bookingRefsFiltered.forEach(async (bookingRef) => { + await Promise.all(bookingRefsFiltered.map(async (bookingRef) => { if (bookingRef.uid) { if (bookingRef.type.endsWith("_calendar")) { const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); return calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); } else if (bookingRef.type.endsWith("_video")) { return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); } } - }); + })); } catch (error) {Additionally, this file contains nearly identical logic to
packages/app-store/wipemycalother/lib/reschedule.ts. Consider extracting shared reschedule logic into a common utility to reduce duplication.packages/trpc/server/routers/viewer/bookings.tsx (1)
553-567: Critical:forEachwith async callback creates unhandled promises.Same issue as in the reschedule files -
forEachdoes not await async callbacks. The calendar/video deletions will run as fire-and-forget operations, and errors won't propagate.🔎 Proposed fix using Promise.all with map
- bookingRefsFiltered.forEach(async (bookingRef) => { + await Promise.all(bookingRefsFiltered.map(async (bookingRef) => { if (bookingRef.uid) { if (bookingRef.type.endsWith("_calendar")) { const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); return calendar?.deleteEvent( bookingRef.uid, builder.calendarEvent, bookingRef.externalCalendarId ); } else if (bookingRef.type.endsWith("_video")) { return deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); } } - }); + }));packages/features/bookings/lib/handleCancelBooking.ts (1)
458-470: Critical:forEachwith async callback does not await operations.The
.forEach(async ...)pattern does not wait for async callbacks to complete. This creates a race condition whereapiDeletesmay not contain all delete promises whenPromise.all(apiDeletes)is called later (line 652), potentially leaving calendar events orphaned.🔎 Proposed fix using for...of loop
- bookingToDelete.user.credentials - .filter((credential) => credential.type.endsWith("_calendar")) - .forEach(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); - } - } - }); + const calendarCredentials = bookingToDelete.user.credentials.filter((credential) => + credential.type.endsWith("_calendar") + ); + for (const credential of calendarCredentials) { + 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; + if (calendar) { + apiDeletes.push(calendar.deleteEvent(uid, evt, externalCalendarId)); + } + } + } + }Note: The fix also changes from pushing the awaited result to pushing the Promise, which is the correct pattern for later use with
Promise.all.
🧹 Nitpick comments (2)
packages/core/videoClient.ts (1)
21-36: LGTM - async adapter retrieval is correctly implemented.The conversion to async is properly done with awaited appStore lookups.
The sequential
forloop could potentially be parallelized usingPromise.allfor better performance when loading multiple adapters:🔎 Optional: Parallel adapter loading
const getVideoAdapters = async (withCredentials: CredentialPayload[]): Promise<VideoApiAdapter[]> => { - const videoAdapters: VideoApiAdapter[] = []; - - for (const cred of withCredentials) { + const adapterPromises = withCredentials.map(async (cred) => { const appName = cred.type.split("_").join(""); // Transform `zoom_video` to `zoomvideo`; const app = await appStore[appName as keyof typeof appStore]; - if (app && "lib" in app && "VideoApiAdapter" in app.lib) { const makeVideoApiAdapter = app.lib.VideoApiAdapter as VideoApiAdapterFactory; - const videoAdapter = makeVideoApiAdapter(cred); - videoAdapters.push(videoAdapter); + return makeVideoApiAdapter(cred); } - } - - return videoAdapters; + return undefined; + }); + const adapters = await Promise.all(adapterPromises); + return adapters.filter((adapter): adapter is VideoApiAdapter => adapter !== undefined); };packages/core/CalendarManager.ts (1)
27-32: Consider adding explicit type annotation for clarity.Since
getCalendaris now async, thecalendarproperty in the returned objects is nowPromise<Calendar | null>rather thanCalendar | null. While this works (asgetConnectedCalendarsawaits it at line 48), the implicit Promise type may confuse future maintainers.🔎 Suggested type annotation
const credentials = app.credentials.flatMap((credential) => { - const calendar = getCalendar(credential); + const calendar: Promise<Calendar | null> = getCalendar(credential); return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : []; });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/app-store/_utils/getCalendar.tspackages/app-store/index.tspackages/app-store/vital/lib/reschedule.tspackages/app-store/wipemycalother/lib/reschedule.tspackages/core/CalendarManager.tspackages/core/EventManager.tspackages/core/videoClient.tspackages/features/bookings/lib/handleCancelBooking.tspackages/features/bookings/lib/handleNewBooking.tspackages/lib/payment/deletePayment.tspackages/lib/payment/handlePayment.tspackages/trpc/server/routers/viewer/bookings.tsx
🧰 Additional context used
🧬 Code graph analysis (8)
packages/app-store/vital/lib/reschedule.ts (1)
packages/app-store/_utils/getCalendar.ts (1)
getCalendar(9-23)
packages/features/bookings/lib/handleNewBooking.ts (1)
packages/app-store/_utils/getCalendar.ts (1)
getCalendar(9-23)
packages/app-store/wipemycalother/lib/reschedule.ts (1)
packages/app-store/_utils/getCalendar.ts (1)
getCalendar(9-23)
packages/trpc/server/routers/viewer/bookings.tsx (1)
packages/app-store/_utils/getCalendar.ts (1)
getCalendar(9-23)
packages/core/videoClient.ts (2)
packages/types/Credential.d.ts (1)
CredentialPayload(8-17)packages/types/VideoApiAdapter.d.ts (2)
VideoApiAdapter(14-28)VideoApiAdapterFactory(30-30)
packages/core/EventManager.ts (1)
packages/app-store/_utils/getCalendar.ts (1)
getCalendar(9-23)
packages/core/CalendarManager.ts (2)
packages/app-store/_utils/getCalendar.ts (1)
getCalendar(9-23)packages/app-store/googlecalendar/lib/CalendarService.ts (1)
deleteEvent(284-318)
packages/features/bookings/lib/handleCancelBooking.ts (1)
packages/app-store/_utils/getCalendar.ts (1)
getCalendar(9-23)
🔇 Additional comments (17)
packages/lib/payment/handlePayment.ts (1)
26-30: LGTM - async appStore lookup is correctly awaited.The async pattern is properly implemented. The subsequent check on line 27 handles the case where the payment app is not found or doesn't have the expected structure.
packages/lib/payment/deletePayment.ts (1)
16-20: LGTM - async appStore lookup is correctly awaited.The await pattern is properly implemented and consistent with
handlePayment.ts.packages/core/videoClient.ts (1)
142-152: LGTM - deleteMeeting correctly awaits adapter retrieval.The async pattern is properly implemented.
packages/trpc/server/routers/viewer/bookings.tsx (1)
967-978: LGTM - async appStore lookup is correctly awaited.The payment app retrieval correctly uses
awaitfor the dynamic import.packages/app-store/index.ts (1)
1-31: Dynamic imports pattern is correct.The conversion to dynamic
import()calls enables lazy-loading of app modules. All identified consumers ofappStorehave been properly updated to await the Promises:
packages/lib/payment/handlePayment.tspackages/lib/payment/deletePayment.tspackages/core/videoClient.tspackages/trpc/server/routers/viewer/bookings.tsxpackages/features/bookings/lib/handleCancelBooking.tspackages/app-store/_utils/getCalendar.tsEach usage follows the consistent pattern:
const app = await appStore[key as keyof typeof appStore];, maintaining type safety while properly handling the Promise resolution.packages/features/bookings/lib/handleNewBooking.ts (1)
887-894: LGTM!The async
getCalendarcall is properly awaited within the asynclastAttendeeDeleteBookingfunction, and the null check before callingdeleteEventis correctly in place.packages/core/EventManager.ts (1)
488-490: LGTM!The async
getCalendarcall is properly awaited, and the optional chaining ondeleteEventcorrectly handles the case wherecalendarmight be null.packages/app-store/_utils/getCalendar.ts (1)
9-22: LGTM!The conversion to async is properly implemented. The function correctly awaits the dynamic import from
appStoreand the return type accurately reflects the Promise-based contract.packages/features/bookings/lib/handleCancelBooking.ts (4)
243-249: LGTM!The async
getCalendarcall is properly awaited within thefor...ofloop, and the null check before operations is correctly in place.
264-271: LGTM!Proper async handling with null check before calling
updateEvent.
477-484: LGTM!The refactor from
forEachtofor...ofwith properawaitis correctly implemented, ensuring calendar deletions complete before proceeding.
589-596: LGTM!The async dynamic import for the payment app is properly awaited, and the subsequent type checking guards are correctly in place.
packages/core/CalendarManager.ts (5)
47-56: LGTM!The restructured code correctly awaits the
calendarPromise fromitem.calendar, and the null check is properly handled.
141-146: LGTM!Efficient parallel fetching of calendars using
Promise.allwith proper await.
233-234: LGTM!The async
getCalendarcall is properly awaited increateEvent.
284-285: LGTM!The async
getCalendarcall is properly awaited inupdateEvent.
330-341: LGTM!The
deleteEventfunction is properly converted to async with correct await ongetCalendarand appropriate return type.
2 participants
Test 2nn
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/cal.com-coderabbit#2](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/2)*