Async import of the appStore packages#10
Conversation
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis pull request converts the application's synchronous calendar and adapter retrieval patterns to asynchronous ones. The Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Handler
participant appStore
participant Calendar
Note over Caller,Calendar: Old Flow (Synchronous)
Caller->>Handler: handleCancelBooking()
Handler->>appStore: getCalendar(credential)
appStore-->>Handler: Calendar object
Handler->>Calendar: deleteEvent()
Note over Caller,Calendar: New Flow (Asynchronous)
Caller->>Handler: handleCancelBooking()
Handler->>appStore: await import("./calendar")
appStore-->>Handler: Promise<Calendar>
Handler->>Handler: await getCalendar(credential)
Handler->>Calendar: await deleteEvent()
Calendar-->>Handler: resolved result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (5)
packages/app-store/vital/lib/reschedule.ts (1)
125-133: <!-- [scratchpad_start]
[task review_file_vital_reschedule verify_issue_severity 2/2]
Verification complete.[observations]
- getCalendar:
export const getCalendar = async (credential: CredentialPayload | null): Promise<Calendar | null>(packages/app-store/_utils/getCalendar.ts:9)
- Definitely async function
- deleteMeeting:
const deleteMeeting = async (credential: CredentialPayload, uid: string): Promise<unknown>(packages/core/videoClient.ts:142)
- Definitely async function
- Correct pattern shown in EventManager.ts:488-489:
const calendar = await getCalendar(...)await calendar?.deleteEvent(...)- Both are awaited
[analysis]
ISSUE CONFIRMED - The review comment is correct and severe:
- forEach(async callback) is NOT awaited → fire-and-forget behavior
- Try/catch does NOT catch errors from async callbacks (execute after try/catch completes)
- getCalendar is awaited inside callback, but if it rejects, error bypasses outer try/catch
- deleteMeeting and calendar?.deleteEvent are returned (not awaited) inside callback
- Function returns true before any deletions complete
- No error handling for async operations
- Codebase shows correct pattern elsewhere (EventManager.ts)
[severity]
This is a MAJOR issue:
- Critical path: booking rescheduling
- Real bug: async work not awaited, errors not caught
- Reliability risk: deletions may not complete
- Original review marked - verification confirms
[actions]
Generate rewritten review comment with tag
[done]
</scratchpad_end] -->Async
forEachdoes not wait for deletions and errors bypass try/catchThe code uses
forEachwith anasynccallback without awaiting it. This makes all callbacks fire-and-forget—the function returns before deletions complete, and any errors thrown inside the callbacks (fromgetCalendarordeleteMeeting) will not be caught by the surroundingtry/catchblock since they execute asynchronously after that block completes.Switch to an
awaited loop to ensure all deletions complete and errors are properly caught:- try { - bookingRefsFiltered.forEach(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) { + try { + await Promise.all( + bookingRefsFiltered.map(async (bookingRef) => { + if (!bookingRef.uid) return; + if (bookingRef.type.endsWith("_calendar")) { + const calendar = await getCalendar(credentialsMap.get(bookingRef.type)); + await calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); + } else if (bookingRef.type.endsWith("_video")) { + await deleteMeeting(credentialsMap.get(bookingRef.type), bookingRef.uid); + } + }) + ); + } catch (error) { if (error instanceof Error) { logger.error(error.message); } }This ensures deletions complete before proceeding and errors are properly handled.
packages/app-store/wipemycalother/lib/reschedule.ts (1)
117-139: AvoidasyncforEachfor calendar/video deletions; await the operations
bookingRefsFiltered.forEach(async (bookingRef) => { ... })runs deletions in fire‑and‑forget fashion: the outer function neither waits for them nor can thetry/catchsee async errors. IfgetCalendarordeleteEvent/deleteMeetingrejects, it will surface as an unhandled rejection, andReschedulewill returntruebefore cleanup finishes.Refactor to collect and await the tasks, for example:
- try { - bookingRefsFiltered.forEach(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) { + const deletionPromises = bookingRefsFiltered.map(async (bookingRef) => { + if (!bookingRef.uid) return; + + const credential = credentialsMap.get(bookingRef.type); + + if (bookingRef.type.endsWith("_calendar")) { + const calendar = await getCalendar(credential); + await calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent); + } else if (bookingRef.type.endsWith("_video")) { + await deleteMeeting(credential, bookingRef.uid); + } + }); + + try { + await Promise.allSettled(deletionPromises); + } catch (error) { if (error instanceof Error) { logger.error(error.message); } }This keeps the error logging behavior while ensuring all deletions are triggered and awaited.
packages/trpc/server/routers/viewer/bookings.tsx (2)
544-568: Refactor asyncforEachwhen cancelling calendar/video referencesHere too,
bookingRefsFiltered.forEach(async (bookingRef) => { ... })launches async work without awaiting it. Any rejection fromgetCalendarordeleteEvent/deleteMeetingbecomes an unhandled rejection, and the mutation returns before cancellations finish.Prefer an awaited aggregation instead:
- const bookingRefsFiltered: BookingReference[] = bookingToReschedule.references.filter( - (ref) => !!credentialsMap.get(ref.type) - ); - bookingRefsFiltered.forEach(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); - } - } - }); + const bookingRefsFiltered: BookingReference[] = bookingToReschedule.references.filter( + (ref) => !!credentialsMap.get(ref.type) + ); + + const deletionPromises = bookingRefsFiltered.map(async (bookingRef) => { + if (!bookingRef.uid) return; + + const credential = credentialsMap.get(bookingRef.type); + + if (bookingRef.type.endsWith("_calendar")) { + const calendar = await getCalendar(credential); + await calendar?.deleteEvent( + bookingRef.uid, + builder.calendarEvent, + bookingRef.externalCalendarId + ); + } else if (bookingRef.type.endsWith("_video")) { + await deleteMeeting(credential, bookingRef.uid); + } + }); + + await Promise.allSettled(deletionPromises);This makes the cancellation behavior deterministic and prevents unhandled rejections.
782-804: Fix authorization predicate and booking confirmation message constructionTwo correctness issues in
confirmshould be addressed:
- Authorization check always succeeds for collective events
if ( eventType?.schedulingType === SchedulingType.COLLECTIVE && eventType.users.find((user) => user.id === user.id) ) {The predicate
user.id === user.idis always true, so any collective event with at least one user passes authorization, regardless of the logged‑in user. It should compare against the outeruserfromctx:- if ( - eventType?.schedulingType === SchedulingType.COLLECTIVE && - eventType.users.find((user) => user.id === user.id) - ) { + if ( + eventType?.schedulingType === SchedulingType.COLLECTIVE && + eventType.users.some((member) => member.id === user.id) + ) {
- Confirmation/rejection message ignores
confirmedvalueconst message = "Booking " + confirmed ? "confirmed" : "rejected";Due to operator precedence,
"Booking " + confirmedis evaluated first and always truthy, somessageis always"confirmed". It should embed the ternary:- const message = "Booking " + confirmed ? "confirmed" : "rejected"; + const message = `Booking ${confirmed ? "confirmed" : "rejected"}`;These directly impact authorization and API responses and should be fixed before release.
Also applies to: 997-1000
packages/features/bookings/lib/handleCancelBooking.ts (1)
437-483: Fix async deletion for recurring bookings andapiDeleteshandlingThere are a couple of issues around the recurring cancellations and API deletions:
- Async
forEach+ resolved values pushed toapiDeletesbookingToDelete.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); } } });
forEach(async ...)is never awaited, soapiDeletesmay still be empty whenPromise.all(prismaPromises.concat(apiDeletes))is called later.deletedEventis the resolved result ofdeleteEvent, butapiDeletesis treated elsewhere as an array of Promises (Promise.all(...),await apiDeletes).This is a regression from the prior pattern where
calendar?.deleteEvent(...)Promises were pushed synchronously and awaited together.Refactor to push Promises and await them deterministically, e.g.:
- if ( - bookingToDelete.eventType?.recurringEvent && - bookingToDelete.recurringEventId && - allRemainingBookings - ) { - 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); - } - } - }); - } else { - apiDeletes.push(calendar?.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); - } + if ( + bookingToDelete.eventType?.recurringEvent && + bookingToDelete.recurringEventId && + allRemainingBookings + ) { + const calendarCredentials = bookingToDelete.user.credentials.filter((credential) => + credential.type.endsWith("_calendar") + ); + + for (const credential of calendarCredentials) { + const calendarForCred = await getCalendar(credential); + if (!calendarForCred) continue; + + for (const updBooking of updatedBookings) { + const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar")); + if (!bookingRef) continue; + + const { uid, externalCalendarId } = bookingRef; + apiDeletes.push( + calendarForCred.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown> + ); + } + } + } else if (calendar) { + apiDeletes.push(calendar.deleteEvent(uid, evt, externalCalendarId) as Promise<unknown>); + }Also declare
apiDeleteswith an explicit type to avoid accidentally mixing values and Promises:- const apiDeletes = []; + const apiDeletes: Promise<unknown>[] = [];
await apiDeletesin the paid‑booking branch does not wait for deletionsIn the paid‑booking path:
// We skip the deletion of the event, because that would also delete the payment reference, which we should keep await apiDeletes; req.statusCode = 200; return { message: "Booking successfully cancelled." };
apiDeletesis an array, soawait apiDeletesresolves immediately and does not wait on the contained Promises. If you intend to await them here (and then return early), this should be:- await apiDeletes; + await Promise.all(apiDeletes);Together, these changes will restore the original behavior (all deletions dispatched and awaited) under the new async
getCalendarmodel and prevent unhandled rejections or silently skipped deletions for recurring bookings.Also applies to: 513-620
🧹 Nitpick comments (3)
packages/core/EventManager.ts (1)
478-490: Guard against missing calendar credentials before awaitinggetCalendarThe switch to
await getCalendar(calendarCredential)is the right move, butprisma.credential.findUniquecan returnnull, socalendarCredentialmay be undefined here. It would be safer to bail out when no credential is found instead of callinggetCalendaron a potentially null value.For example:
- if (oldCalendarEvent?.credentialId) { - const calendarCredential = await prisma.credential.findUnique({ + if (oldCalendarEvent?.credentialId) { + const calendarCredential = await prisma.credential.findUnique({ where: { id: oldCalendarEvent.credentialId, }, }); - const calendar = await getCalendar(calendarCredential); - await calendar?.deleteEvent(oldCalendarEvent.uid, event, oldCalendarEvent.externalCalendarId); + if (calendarCredential) { + const calendar = await getCalendar(calendarCredential); + await calendar?.deleteEvent( + oldCalendarEvent.uid, + event, + oldCalendarEvent.externalCalendarId + ); + } }packages/lib/payment/handlePayment.ts (1)
26-40: Async resolution ofpaymentAppmatches the dynamic app-store patternAwaiting
appStore[dirName]here is consistent with a Promise-based appStore and ensures you only touchpaymentApp.lib.PaymentServiceonce the module is loaded. You might consider loggingpaymentAppCredentials.app?.dirNamein the warning instead of${paymentApp}for clearer diagnostics, but the core async change looks good.packages/core/videoClient.ts (1)
21-36: Consider parallel loading for better performance with multiple credentials.The sequential
awaitin the loop (line 26) means credentials are processed one at a time. With multiple credentials, this could be noticeably slower than necessary.Apply this diff to load modules in parallel:
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 null; + }); - return videoAdapters; + const results = await Promise.all(adapterPromises); + return results.filter((adapter): adapter is VideoApiAdapter => adapter !== null); };This processes all credentials concurrently rather than sequentially.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/app-store/_utils/getCalendar.ts(1 hunks)packages/app-store/index.ts(1 hunks)packages/app-store/vital/lib/reschedule.ts(1 hunks)packages/app-store/wipemycalother/lib/reschedule.ts(1 hunks)packages/core/CalendarManager.ts(6 hunks)packages/core/EventManager.ts(1 hunks)packages/core/videoClient.ts(7 hunks)packages/features/bookings/lib/handleCancelBooking.ts(6 hunks)packages/features/bookings/lib/handleNewBooking.ts(1 hunks)packages/lib/payment/deletePayment.ts(1 hunks)packages/lib/payment/handlePayment.ts(1 hunks)packages/trpc/server/routers/viewer/bookings.tsx(2 hunks)
🔇 Additional comments (15)
packages/features/bookings/lib/handleNewBooking.ts (1)
883-893: AwaitinggetCalendarhere is correct with the new async calendar APIUsing
await getCalendar(credential)insidelastAttendeeDeleteBookingmatches the async return type and ensures you only enqueuedeleteEventcalls once the calendar instance is resolved. No further changes needed in this block.packages/lib/payment/deletePayment.ts (1)
16-24: Async lookup ofpaymentAppfromappStorelooks correctAwaiting
appStore[dirName]is consistent with a dynamic-import–backed app store and keeps the subsequentPaymentServicechecks intact. No issues spotted in this change.packages/trpc/server/routers/viewer/bookings.tsx (1)
962-979: AsyncappStorepayment app lookup is correctThe change to
const paymentApp = await appStore[...]aligns with the async appStore design and the subsequent runtime checks onpaymentApp.lib.PaymentService. The surrounding error handling (if (!paymentApp...){ console.warn; return null; }) still behaves as before, so the refactor here looks good.packages/app-store/_utils/getCalendar.ts (1)
9-22: AsyncgetCalendarimplementation looks goodMaking
getCalendarasyncand awaitingappStore[calendarType...]to resolve the app module is consistent with the new appStore contract. The existing guards for missing credentials and unsupported calendar types are preserved, and returning aCalendarServiceinstance wrapped in a Promise is appropriate. From the surrounding changes, callers are already updated toawait getCalendar, so this refactor looks sound.packages/features/bookings/lib/handleCancelBooking.ts (2)
229-276: Seat attendee calendar/video updates now correctly awaitgetCalendarIn the seated‑event branch, switching to
const calendar = await getCalendar(credential);and then pushingcalendar?.deleteEvent/updateEventPromises intointegrationsToDelete, followed byawait Promise.all(integrationsToDelete), keeps the behavior correct with the new asyncgetCalendar. This segment looks good.
588-597: AsyncappStorepayment app resolution is aligned with new designUpdating
const paymentApp = await appStore[...]is consistent with the async appStore contract, and the existing guard that checks forpaymentApp.lib.PaymentServiceremains intact. The refund logic wrapped intry/catchstill behaves the same. No issues here.packages/core/CalendarManager.ts (1)
23-36: AsyncgetCalendarusage inCalendarManageris consistent and correct
- In
getCalendarCredentialsyou now store the Promise fromgetCalendar(credential)and laterawait item.calendaringetConnectedCalendars, which keeps the API clean while deferring calendar resolution.getCachedResultscorrectly resolves calendars withPromise.all(calendarCredentials.map(getCalendar))before callinggetAvailability.createEvent,updateEvent, anddeleteEventnow awaitgetCalendar(credential)and guard on a falsy calendar, preserving previous behavior while aligning with the asyncgetCalendarcontract. Returning a resolved empty object fromdeleteEventwhen no calendar is available is a reasonable no‑op fallback.Based on the code shown, these async refactors look sound.
Also applies to: 135-175, 228-341
packages/app-store/index.ts (1)
1-31: Clarify: Are lazy-loaded dynamic imports intended here?All dynamic imports execute immediately when this module loads, rather than being deferred until first access. If the goal is true lazy loading (to reduce initial bundle and defer network/parse cost), wrap each import in a function:
const appStore = { - applecalendar: import("./applecalendar"), + applecalendar: () => import("./applecalendar"),Then consumers would call
await appStore.applecalendar()instead ofawait appStore.applecalendar.If immediate execution is intentional (e.g., to begin prefetching modules early), the current pattern is correct.
Which behavior do you want?
- Immediate: Current code is correct; imports start when appStore loads
- Lazy: Apply the function-wrapper pattern above
packages/core/videoClient.ts (7)
38-41: LGTM!The async conversion is correct. The function properly awaits adapter retrieval before proceeding with availability checks.
52-53: LGTM!Correctly awaits the video adapters before destructuring and using them.
111-111: LGTM!Correctly awaits the video adapters before destructuring.
142-151: LGTM!The async conversion is correct. The function properly handles both the credential and no-credential cases.
162-172: LGTM!Correctly awaits the video adapter before using it to create a meeting.
185-195: LGTM!Correctly awaits the video adapter before retrieving recordings.
206-216: LGTM!Correctly awaits the video adapter before retrieving the download link.
2 participants
Test 2
Summary by CodeRabbit
Replicated from ai-code-review-evaluation/cal.com-coderabbit#2