Async import of the appStore packages

[akshayutture-augment]

Test 2

---

Replicated from ai-code-review-evaluation/cal.com-greptile#2

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

Code Review: Async Import of appStore Packages

I've completed my review of this PR which converts synchronous imports to async dynamic imports for app store packages. Here are my findings:

---

🔴 Critical Issues

1. Race Condition in forEach with async callbacks (Multiple files)

Location: packages/app-store/vital/lib/reschedule.ts:125, packages/app-store/wipemycalother/lib/reschedule.ts:125, packages/trpc/server/routers/viewer/bookings.tsx:553

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);
    }
  }
});

Problem: Using async callbacks with forEach creates "fire-and-forget" promises that are never awaited. The return values are ignored, and errors won't be caught. This means:

• Calendar deletions may not complete before the function returns
• Errors will be silently swallowed
• No guarantee of execution order or completion

Recommendation: Replace forEach with for...of loop or Promise.all() with map():

// Option 1: Sequential execution
for (const bookingRef of bookingRefsFiltered) {
  if (bookingRef.uid) {
    if (bookingRef.type.endsWith("_calendar")) {
      const calendar = await getCalendar(credentialsMap.get(bookingRef.type));
      await calendar?.deleteEvent(bookingRef.uid, builder.calendarEvent);
    }
  }
}

// Option 2: Parallel execution
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);
    }
  }
}));

2. Inconsistent Promise Handling in CalendarManager.getCalendarCredentials

Location: packages/core/CalendarManager.ts:28

const credentials = app.credentials.flatMap((credential) => {
  const calendar = getCalendar(credential); // Returns Promise, but not awaited!
  return app.variant === "calendar" ? [{ integration: app, credential, calendar }] : [];
});

Problem: getCalendar() is now async but the result is not awaited. This returns a Promise object instead of the actual calendar instance. While the code does await it later in getConnectedCalendars:48, this creates a confusing API where sometimes it's a Promise and sometimes it's resolved.

Recommendation: Keep the current approach consistent (store promises and await them when needed), but add a comment explaining this pattern for clarity.

---

⚠️ High Priority Issues

3. Missing Await in Sequential Loop

Location: packages/features/bookings/lib/handleCancelBooking.ts:477-482

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>);
}

Problem: While getCalendar is awaited, the loop doesn't await the completion of deleteEvent. If error handling or timing is critical, this could cause issues. However, since the promises are collected in apiDeletes array and presumably awaited later, this is less severe than issue #1.

Recommendation: Verify that apiDeletes is properly awaited with error handling later in the function.

---

💡 Moderate Issues

4. Type Safety with Dynamic Import

Location: packages/app-store/index.ts

All imports are now dynamic using import(), which returns Promise<Module>. When accessing these modules:

const calendarApp = await appStore[calendarType.split("_").join("") as keyof typeof appStore];

Problem: TypeScript may not properly infer the module structure from dynamic imports. The type system might not catch breaking changes in module exports.

Recommendation: Consider adding explicit type annotations or a type definition for the appStore to ensure type safety:

type AppStoreModule = {
  lib?: {
    CalendarService?: any;
    VideoApiAdapter?: any;
    PaymentService?: any;
  };
};

const appStore: Record<string, Promise<AppStoreModule>> = {
  // ...
};

5. Performance Consideration: Import Timing

Location: packages/app-store/index.ts

All 30 app imports are now lazy-loaded. Each import statement creates a promise that will resolve when needed.

Impact:

• ✅ Reduced initial bundle size
• ✅ Faster application startup
• ⚠️ Potential latency on first use of each calendar/video integration
• ⚠️ Multiple concurrent requests could trigger many parallel dynamic imports

Recommendation: Consider pre-warming commonly used integrations or implementing a module cache to optimize repeated access patterns.

---

✅ Positive Changes

1. Bundle Size Optimization: Converting to dynamic imports significantly reduces the initial JavaScript bundle size by code-splitting app packages.

2. Consistent Async Pattern: The refactor consistently makes getCalendar() async throughout the codebase, which is a cleaner API.

3. Error Handling Preservation: Most existing try-catch blocks and error handling logic remain intact.

4. Good Test Coverage Points: The changes in CalendarManager.ts, videoClient.ts, and EventManager.ts are well-structured for unit testing with proper async/await patterns.

---

📋 Testing Recommendations

Since I don't have permissions to run the test suite, I recommend:

1. Integration Tests: Test calendar creation, update, and deletion flows end-to-end
2. Error Scenarios: Verify error handling when app modules fail to load
3. Concurrent Operations: Test multiple simultaneous bookings/cancellations
4. Performance Tests: Measure latency impact on first calendar/video integration use
5. Backward Compatibility: Ensure existing bookings can still be cancelled/rescheduled

---

🎯 Summary

Must Fix Before Merge:

• Issue feat: add calendar cache status and actions (#22532) #1: Race conditions with forEach async callbacks (3 occurrences)

Should Address:

• Issue Add guest management functionality to existing bookings #2: Add clarifying comments about Promise handling in getCalendarCredentials
• Issue SMS workflow reminder retry count tracking #3: Verify apiDeletes Promise array is properly awaited
• Issue OAuth credential sync and app integration enhancements #4: Add type definitions for dynamic imports

Nice to Have:

• Issue Advanced date override handling and timezone compatibility improvements #5: Consider pre-warming strategy for common integrations

The core concept of the refactor is solid and will improve bundle size, but the async callback handling needs immediate attention to prevent production bugs.

---

|

[github-actions]

This PR is being marked as stale due to inactivity.