Cal.com PR #21 — Live Benchmark

Golden Comments (2/2 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	Consider adding `try-catch` around the `await` to handle import failures gracefully	LOW	✓ Found	"The `await appStore[...]` for the dynamic import has no error handling; a failing module import rejects `getCalendar` and can abort whole `Promise.all` batches in callers. Wrap in try/catch and return null on failure" (packages/app-store/_utils/getCalendar.ts L15)
#2	The code uses `forEach` with async callbacks, which causes asynchronous operations (e.g., calendar/video event deletions, payment refunds) to run concurrently without being awaited. This 'fire-and-forget' behavior leads to unhandled promise rejections, race conditions, and incomplete cleanup, as surrounding `try-catch` blocks cannot properly handle errors from these unawaited promises. Replace `forEach` with `for...of` loops or `Promise.all()` with `map()` to ensure proper sequential execution and error handling.	HIGH	✓ Found	"The `forEach(async ...)` loop is fire-and-forget; calendar/video deletions may not complete (or may never run in serverless) and rejections go unhandled. Collect and await the promises with Promise.all" (packages/trpc/server/routers/viewer/bookings.tsx L553)

Supplementary Findings (16 findings)

Finding	File	Severity	Legitimate?
getCalendarCredentials remains synchronous: it eagerly fires all dynamic imports and returns `calendar` as a Promise, causing unhandled rejections and defeating lazy loading	CalendarManager.ts:28	HIGH	✓ Real regression: lazy loading defeated, unhandled rejections possible
Unguarded `await appStore[...]` can reject on module-load failure and bypass the Daily-video fallback in createMeeting	videoClient.ts:26	HIGH	✓ Real: the Daily fallback is silently skipped on import failure
The dynamic import await has no try/catch; a failed import throws out of deletePayment instead of returning false	deletePayment.ts:16	HIGH	✓ Real error-contract break introduced by the conversion
Optional chaining only guards `apps`, so a missing `apps[appId]` throws a TypeError when accessing `.price`/`.currency`	handlePayment.ts:35	HIGH	✓ Real potential TypeError on a missing payment app
In the recurring-event cancel path, async push into `apiDeletes` races with the subsequent `Promise.all(prismaPromises.concat(apiDeletes))`, so some deletions can be missed	handleCancelBooking.ts:460	HIGH	✓ Real race: some calendar deletions silently dropped
The paid-cancel path does `await apiDeletes` (a bare array), which resolves immediately without waiting for the contained promises; should be `await Promise.all(apiDeletes)`	handleCancelBooking.ts:617	HIGH	✓ Real bug: deletions are not actually awaited
The `forEach(async ...)` makes calendar/video cleanup fire-and-forget; the wrapping try/catch no longer catches errors inside async callbacks (they become unhandled rejections)	vital/reschedule.ts:125	HIGH	✓ Real: error logging lost, rejections unhandled
Same fire-and-forget pattern in the wipemycalother reschedule: `forEach(async ...)` calendar/video cleanup with a try/catch that can no longer catch errors inside async callbacks	wipemycalother/reschedule.ts:125	HIGH	✓ Real: error logging lost, rejections unhandled
When the payment app is unavailable, the paid-cancel handler returns null, abandoning cancellation and skipping cleanup/webhook while reporting success	handleCancelBooking.ts:589	MEDIUM	✓ Real silent-abandon path on a missing payment app
The rejectBooking mutation returns null when the payment app is missing, leaving the booking PENDING and never sending declined emails, breaking the response contract	bookings.tsx:970	MEDIUM	✓ Real: booking stuck PENDING, no declined emails
The `lastAttendee` condition `length < 0` is never true, so seat-cancellation never cleans up calendar/video for the final attendee; should be `=== 0`	handleCancelBooking.ts:223	MEDIUM	✓ Real dead condition; pre-existing, now in scope
The key derivation `calendarType.split("_").join("")` plus `as keyof typeof appStore` silently maps unknown credential types to undefined, with no startup verification that every type resolves	getCalendar.ts:15	LOW	✓ Borderline: silent misrouting, a dev-time guard would help
Type-unsafety of `new CalendarService(credential)` relies on structural compatibility across a union of constructors	getCalendar.ts:22	LOW	✓ Borderline: typing improvement, no runtime impact
Logs the CalendarService class at info level on every lookup; should be debug or removed	getCalendar.ts:20	LOW	✓ Real log-noise nit
The performance mark is placed after awaiting all calendar module loads, so the import cost is excluded from the measured metric	CalendarManager.ts:142	LOW	✓ Borderline: measurement-correctness nit
Sequential module resolution in the for loop serializes cold-load latency across multiple credentials; consider resolving imports with `Promise.all`	videoClient.ts:24	LOW	✓ Borderline: performance suggestion, non-blocking

Honest Analysis

FriendlyReviewer found both golden comments of this PR — recall 100% — and on both the underlying finding is a first-class one rather than a near-miss. Golden #1 maps exactly to the unguarded await appStore[...] at getCalendar.ts L15, with the review adding the caller-side consequence the reference implies (a failing import aborts whole Promise.all batches). Golden #2 maps to the forEach(async ...) at bookings.tsx L553, where the review goes beyond the reference comment by spelling out the serverless angle — deletions that may never run at all.

The supplementary set (16) is dominated by genuine conversion regressions: the synchronous getCalendarCredentials (CalendarManager.ts L28) defeats the entire lazy-loading goal of the PR, the unguarded awaits in videoClient and deletePayment break their error contracts, and the two handleCancelBooking items (L460 race, L617 bare-array await) are real unawaited-async bugs. These are the same failure family as the goldens, which explains the clean recall. The low-severity tail is weaker: getCalendar.ts L20 (info-level logging) and L22 (constructor typing) are nits that would not change a merge decision, and handleCancelBooking.ts L223 (lastAttendee) is pre-existing code pulled into scope by the verdict rather than by this PR.

The verdict labels 10 findings HIGH, which overstates the material risk: most of these regressions only fire when a module import fails — a rare event in production — or in narrow cancel/refund paths, while the highest-impact consequence (deletions silently dropped on cancel) is genuinely severe. One gap is worth noting: the golden's example list includes payment refunds, and the review examines the payment helpers (deletePayment, handlePayment) but never the refund flow itself. Overall a strong recall performance, with severity inflation and some scope drift as the honest counterweights.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech	🌤️ Feat
10 high, 3 medium, 5 low	1 mitigated

This MR converts the app-store registry from static namespace imports to async dynamic imports and makes the calendar/video/payment factories async. The mechanical conversion is largely correct, but it introduces several regressions: dynamic imports are awaited without error handling in key paths, causing unhandled rejections; multiple forEach(async ...) loops are fire-and-forget and now race with request completion, which can silently drop calendar/video deletions; getCalendarCredentials was left synchronous, defeating lazy-loading and returning Promise-typed calendars; and a few pre-existing logic bugs (lastAttendee condition, paid-cancel early return) are now in scope. These issues should be addressed before merge.

These might need a close look

🔴 packages/app-store/_utils/getCalendar.ts (L15)
The await appStore[...] for the dynamic import has no error handling; a failing module import rejects getCalendar and can abort whole Promise.all batches in callers. Wrap in try/catch and return null on failure.
🔴 packages/core/CalendarManager.ts (L28)
getCalendarCredentials remains synchronous, so it eagerly fires all dynamic imports and returns calendar as a Promise, causing unhandled rejections and defeating lazy loading. Make it async and await the calendar resolution.
🔴 packages/core/videoClient.ts (L26)
Unguarded await appStore[...] can reject on module-load failure and bypass the Daily-video fallback in createMeeting; wrap it or the call sites in try/catch so the fallback runs.
🔴 packages/lib/payment/deletePayment.ts (L16)
The dynamic import await has no try/catch; a failed import throws out of deletePayment instead of returning false. Add try/catch and match the error contract.
🔴 packages/lib/payment/handlePayment.ts (L35)
Optional chaining only guards apps, so a missing apps[appId] throws a TypeError when accessing .price/.currency. Use apps?.[appId]?.price or resolve through the legacy helpers.
🔴 packages/trpc/server/routers/viewer/bookings.tsx (L553)
The forEach(async ...) loop is fire-and-forget; calendar/video deletions may not complete (or may never run in serverless) and rejections go unhandled. Collect and await the promises with Promise.all.
🔴 packages/features/bookings/lib/handleCancelBooking.ts (L460)
In the recurring-event cancel path, async push into apiDeletes races with the subsequent Promise.all(prismaPromises.concat(apiDeletes)) so some deletions can be missed. Convert to collecting promises and awaiting.
🔴 packages/features/bookings/lib/handleCancelBooking.ts (L617)
The paid-cancel path does await apiDeletes (a bare array) which resolves immediately without waiting for the contained promises; should be await Promise.all(apiDeletes).
🔴 packages/app-store/vital/lib/reschedule.ts (L125)
The forEach(async ...) makes calendar/video cleanup fire-and-forget and the wrapping try/catch no longer catches errors inside async callbacks (they become unhandled rejections); rewrite with for...of/Promise.all to await deletions and restore error logging.
🔴 packages/app-store/wipemycalother/lib/reschedule.ts (L125)
The forEach(async ...) makes calendar/video cleanup fire-and-forget and the wrapping try/catch no longer catches errors inside async callbacks (they become unhandled rejections); rewrite with for...of/Promise.all to await deletions and restore error logging.

Worth checking

🟡 packages/features/bookings/lib/handleCancelBooking.ts (L589)
When the payment app is unavailable, the paid-cancel handler returns null, abandoning cancellation and skipping cleanup/webhook while reporting success; should throw so the client gets a failure.
🟡 packages/trpc/server/routers/viewer/bookings.tsx (L970)
The rejectBooking mutation returns null when the payment app is missing, leaving the booking PENDING and never sending declined emails, breaking the response contract; should throw a TRPCError.
🟡 packages/features/bookings/lib/handleCancelBooking.ts (L223)
The lastAttendee condition length < 0 is never true, so seat-cancellation never cleans up calendar/video for the final attendee; should be === 0.

Small things (take or leave)

🔵 packages/app-store/_utils/getCalendar.ts (L15)
The key derivation calendarType.split("_").join("") combined with as keyof typeof appStore silently maps unknown credential types to undefined, and there is no startup verification that every credential type resolves. Add a dev-time assertion or key in appStore check.
🔵 packages/app-store/_utils/getCalendar.ts (L22)
Type-unsafety of new CalendarService(credential) relies on structural compatibility across a union of constructors; consider typing the registry to enforce a shared construct signature.
🔵 packages/app-store/_utils/getCalendar.ts (L20)
Logs the CalendarService class at info level on every lookup; should be debug or removed.
🔵 packages/core/CalendarManager.ts (L142)
The performance mark is placed after awaiting all calendar module loads, so the import cost is excluded from the measured metric. Move the mark before the Promise.all.
🔵 packages/core/videoClient.ts (L24)
Sequential module resolution in the for loop serializes cold-load latency across multiple credentials; consider resolving imports with Promise.all.

✅ Feature-level checklist

Partially covered

⚠️ Convert static app-store imports to async dynamic imports (lazy loading).
The registry now uses dynamic import() and the factories await it, but getCalendarCredentials remains synchronous and eagerly loads all modules, and several fire-and-forget async call sites mean the async behavior is not reliably lazy/awaited. Error handling around dynamic imports is also incomplete.

Async import of the appStore packages