-
Notifications
You must be signed in to change notification settings - Fork 0
Async import of the appStore packages #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: appstore-sync-refactor-base
Are you sure you want to change the base?
Async import of the appStore packages #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Greptile OverviewConfidence Score: 0/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Client
participant CalendarManager
participant getCalendar
participant appStore
participant CalendarApp
participant CalendarService
Client->>CalendarManager: getCalendarCredentials(credentials)
CalendarManager->>getCalendar: getCalendar(credential)
Note over CalendarManager,getCalendar: ⚠️ CRITICAL: Not awaited!
getCalendar->>appStore: await appStore[calendarType]
appStore-->>getCalendar: Promise<CalendarApp>
getCalendar->>CalendarApp: Check lib.CalendarService
CalendarApp-->>getCalendar: CalendarService class
getCalendar->>CalendarService: new CalendarService(credential)
CalendarService-->>getCalendar: Calendar instance
getCalendar-->>CalendarManager: Promise<Calendar>
Note over CalendarManager: Returns Promise, not Calendar!
CalendarManager-->>Client: calendarCredentials (with Promise objects)
Client->>CalendarManager: getConnectedCalendars(calendarCredentials)
CalendarManager->>CalendarManager: await item.calendar
Note over CalendarManager: Correctly awaits the Promise here
CalendarManager->>CalendarService: calendar.listCalendars()
CalendarService-->>CalendarManager: calendar list
CalendarManager-->>Client: connectedCalendars
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (5)
-
packages/app-store/vital/lib/reschedule.ts, line 125-134 (link)logic:
asynccallback inforEachdoesn't wait for promises. The calendar deletions won't complete before the function continues. -
packages/app-store/wipemycalother/lib/reschedule.ts, line 125-134 (link)logic:
asynccallback inforEachdoesn't wait for promises. The calendar deletions won't complete before the function continues. -
packages/trpc/server/routers/viewer/bookings.tsx, line 553-565 (link)logic:
asynccallback inforEachdoesn't wait for promises. The calendar/video deletions won't complete before the function continues. -
packages/features/bookings/lib/handleCancelBooking.ts, line 458-470 (link)logic:
asynccallback inforEachdoesn't wait for promises. The calendar updates won't complete before the function continues. -
packages/core/CalendarManager.ts, line 23-36 (link)logic:
getCalendaris nowasyncbut not awaited here. This stores promises instead ofCalendarobjects, breaking the return type.export const getCalendarCredentials = async (credentials: Array<CredentialPayload>) => { const calendarCredentials = await Promise.all( getApps(credentials) .filter((app) => app.type.endsWith("_calendar")) .flatMap(async (app) => { const credentials = await Promise.all( app.credentials.map(async (credential) => { const calendar = await getCalendar(credential); return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : []; }) ); return credentials.flat(); }) ); return calendarCredentials.flat(); };
12 files reviewed, 5 comments
Test 2
Replicated from ai-code-review-evaluation/cal.com-greptile#2