Async import of the appStore packages#2
Conversation
WalkthroughThe changes refactor the app store architecture and related modules to support asynchronous, on-demand loading of application modules using dynamic imports. All functions that interact with the app store or depend on dynamically loaded modules—such as calendar, video, and payment services—are updated to handle asynchronous retrievals via Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AppStore
participant AppModule
participant Service
Caller->>+AppStore: await appStore["calendar"]
AppStore-->>-Caller: Promise<AppModule>
Caller->>+AppModule: new CalendarService(credential)
AppModule-->>-Caller: CalendarService instance
Caller->>Service: serviceMethod(...)
Service-->>Caller: result
sequenceDiagram
participant Caller
participant AppStore
participant AppModule
participant PaymentService
Caller->>+AppStore: await appStore["payment"]
AppStore-->>-Caller: Promise<AppModule>
Caller->>+AppModule: new PaymentService(credential)
AppModule-->>-Caller: PaymentService instance
Caller->>PaymentService: deletePayment(...)
PaymentService-->>Caller: result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (3)
packages/trpc/server/routers/viewer/bookings.tsx (1)
553-567: Critical: forEach with async callback doesn't wait for completion.Using
forEachwith anasynccallback is problematic becauseforEachdoesn't wait for the promises to resolve. This means the calendar deletion operations will run concurrently but the function won't wait for them to complete, potentially causing race conditions or unhandled promise rejections.Replace the
forEachwithPromise.allandmapto properly handle the async operations:- 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/app-store/vital/lib/reschedule.ts (1)
125-134: Critical: forEach with async callback doesn't wait for completion.This is the same issue as in the bookings router -
forEachdoesn't wait for async callbacks to complete, which can lead to race conditions and unhandled promise rejections.Replace with
Promise.allandmapfor proper async handling:- 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); } } - }); + }) + );packages/app-store/wipemycalother/lib/reschedule.ts (1)
125-134: Critical: forEach with async callback doesn't wait for completion.Same issue as in the other reschedule files -
forEachwith async callbacks doesn't properly wait for promise resolution.Apply the same fix using
Promise.all:- 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); } } - }); + }) + );
🧹 Nitpick comments (2)
packages/app-store/wipemycalother/lib/reschedule.ts (1)
125-134: Consider refactoring duplicate reschedule logic.This reschedule implementation appears very similar to the one in
packages/app-store/vital/lib/reschedule.ts. Consider extracting the common booking reference cleanup logic into a shared utility function to reduce code duplication and improve maintainability.packages/app-store/index.ts (1)
1-31: LGTM: Correct conversion to dynamic imports.The conversion from static to dynamic imports is properly implemented and provides several benefits:
- Lazy loading: Modules are only loaded when actually needed, improving initial bundle size and startup performance
- On-demand resolution: Apps are resolved asynchronously, allowing for better resource management
- Consistent pattern: All modules follow the same
import("./modulename")patternThe change maintains the same export interface while enabling asynchronous module loading throughout the codebase.
📜 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 context used
🧬 Code Graph Analysis (7)
packages/app-store/vital/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/app-store/_utils/getCalendar.ts (2)
packages/types/Credential.d.ts (1)
CredentialPayload(8-17)packages/types/Calendar.d.ts (1)
Calendar(213-231)
packages/core/EventManager.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/features/bookings/lib/handleCancelBooking.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)
🔇 Additional comments (25)
packages/features/bookings/lib/handleNewBooking.ts (1)
888-888: LGTM! Correctly updated to await the asynchronous getCalendar function.The addition of
awaitaligns with the refactoredgetCalendarfunction that now returns aPromise<Calendar | null>due to asynchronous app store loading.packages/core/EventManager.ts (1)
488-488: LGTM! Correctly updated to await the asynchronous getCalendar function.The addition of
awaitproperly handles the asynchronous nature ofgetCalendarfollowing the app store refactor to dynamic imports.packages/lib/payment/deletePayment.ts (1)
16-16: LGTM! Correctly updated to await asynchronous app store access.The addition of
awaitproperly handles the asynchronous app store retrieval following the refactor to dynamic imports.packages/lib/payment/handlePayment.ts (1)
26-26: LGTM! Correctly updated to await asynchronous app store access.The addition of
awaitproperly handles the asynchronous app store retrieval following the refactor to dynamic imports, maintaining consistent behavior while supporting the new async architecture.packages/trpc/server/routers/viewer/bookings.tsx (1)
967-967: LGTM: Correct async appStore access.The payment app retrieval correctly awaits the dynamic import from
appStore, which aligns with the new asynchronous module loading architecture.packages/app-store/_utils/getCalendar.ts (1)
9-23: LGTM: Correct async conversion.The function signature and implementation have been properly updated to handle the asynchronous
appStoreaccess. The return type correctly reflects the promise-based nature, and theawaitusage is appropriate for the dynamic import pattern.packages/features/bookings/lib/handleCancelBooking.ts (5)
243-243: LGTM! Proper async handling for calendar retrieval.The
getCalendarcall is correctly awaited, consistent with the new asynchronous signature.
265-265: LGTM! Consistent async pattern.Calendar retrieval is properly awaited for the event update scenario.
452-452: LGTM! Async calendar retrieval handled correctly.The calendar instance is properly awaited before use in the booking reference handling logic.
477-483: LGTM! Proper conversion to sequential async processing.The code correctly converts from
forEachtofor...ofloop to handle asyncgetCalendarcalls sequentially, avoiding race conditions.
589-589: LGTM! Consistent async app store access.The payment app retrieval from
appStoreis properly awaited, aligning with the new asynchronous module loading pattern.packages/core/CalendarManager.ts (6)
31-31: Minor formatting improvement.Added blank line improves code readability.
47-48: LGTM! Proper async calendar handling.The calendar is correctly awaited from
item.calendarbefore use, ensuring the Promise is resolved.
142-142: LGTM! Consistent async pattern for calendar array.All calendar credentials are properly mapped to awaited
getCalendarcalls usingPromise.all.
233-233: LGTM! Proper async calendar retrieval.The
getCalendarcall is correctly awaited in thecreateEventfunction.
284-284: LGTM! Consistent async handling.Calendar retrieval is properly awaited in the
updateEventfunction.
330-335: LGTM! Proper async deleteEvent implementation.The function signature is correctly updated to async and the
getCalendarcall is properly awaited.packages/core/videoClient.ts (8)
21-36: LGTM! Proper async conversion of getVideoAdapters.The function is correctly converted to async with proper error handling:
- Uses
for...ofloop for sequential async processing- Properly awaits app store access
- Maintains existing logic flow
- Returns Promise<VideoApiAdapter[]> as expected
38-39: LGTM! Consistent async pattern.
getBusyVideoTimesproperly awaitsgetVideoAdaptersbefore processing results.
52-52: LGTM! Proper async handling in createMeeting.The
getVideoAdapterscall is correctly awaited.
111-111: LGTM! Consistent async pattern in updateMeeting.Video adapters are properly awaited before destructuring.
142-144: LGTM! Proper async handling in deleteMeeting.The function correctly awaits
getVideoAdaptersand handles the async flow.
162-171: LGTM! Consistent async pattern for Cal video fallback.Video adapter creation is properly awaited in the fallback scenario.
185-194: LGTM! Proper async handling for recordings.The
getVideoAdapterscall is correctly awaited for Cal video recordings functionality.
206-215: LGTM! Consistent async pattern for download links.Video adapter retrieval is properly awaited for recording download functionality.
| .forEach(async (credential) => { | ||
| const calendar = getCalendar(credential); | ||
| const calendar = await getCalendar(credential); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential race condition in forEach with async operations.
The forEach loop with async getCalendar calls may not execute sequentially and could lead to race conditions or performance issues.
Replace the forEach with for...of loop to ensure proper sequential execution:
- bookingToDelete.user.credentials
- .filter((credential) => credential.type.endsWith("_calendar"))
- .forEach(async (credential) => {
- const calendar = await getCalendar(credential);
+ const calendarCredentials = bookingToDelete.user.credentials
+ .filter((credential) => credential.type.endsWith("_calendar"));
+ for (const credential of calendarCredentials) {
+ const calendar = await getCalendar(credential);🤖 Prompt for AI Agents
In packages/features/bookings/lib/handleCancelBooking.ts at lines 460-461, the
use of forEach with an async callback causes the async operations to run
concurrently, which can lead to race conditions or unexpected behavior. Replace
the forEach loop with a for...of loop to await each async getCalendar call
sequentially, ensuring proper order and avoiding concurrency issues.
|
This PR is being marked as stale due to inactivity. |
2 participants
Test 2
Summary by CodeRabbit
Refactor
Bug Fixes