Cal.com PR #1 — Live Benchmark — FriendlyReviewer

Golden Comments (2/2 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	Missing try-catch around dynamic imports in `appStore` — network or cache failures can cause the entire import chain to crash	LOW	● Partial	FriendlyReviewer finds a related but different issue: `Promise.all` without error isolation in `CalendarManager.ts` (L142). An import failure in any one app rejects the entire `getCachedResults` promise. This is the same class of failure (no fault barrier) but at a different call site — not on the individual dynamic imports themselves.
#2	`forEach(async` callback) → fire-and-forget — calendar/video/payment operations launched via `forEach(async)` are never awaited, creating unhandled promise rejections, race conditions, and incomplete cleanup	HIGH	✓ Found	FriendlyReviewer found this bug in 3 files (vs. 2 in the golden set): `vital/reschedule.ts` (L133), `handleCancelBooking.ts` (L458), `bookings.tsx` (L564). Each classified as HIGH with a recommendation to replace with `for...of` or `Promise.all`.

Supplementary Findings (14 findings)

Additional issues found by FriendlyReviewer that are not part of the golden (reference) set.

Finding	File	Severity	Legitimate?
`getCalendar` becomes async but `getCalendarCredentials` calls it without `await` → calendar field is a Promise, not a Calendar	CalendarManager.ts:23	HIGH	✓ Breaking type contract
`deleteEvent` called without `externalCalendarId` → calendar integration may fail to delete the correct event	vital/reschedule.ts:136	HIGH	✓ Bug real
Byte-for-byte identical duplication of `vital/lib/reschedule.ts` in `wipemycalother/lib/reschedule.ts`	vital/reschedule.ts:1	HIGH	✓ Maintenance burden
Permission bypass: any authenticated user (or unauthenticated with `userId=-1`) can cancel any future booking by UID	handleCancelBooking.ts:53	HIGH	✓ Security vulnerability
`findUnique({ where: { id, uid } })` with `id = undefined` → Prisma runtime error	handleCancelBooking.ts:18	HIGH	✓ Runtime crash
Check `.length < 0` always false → calendar event never deleted for the last attendee of a seated booking	handleCancelBooking.ts:232	HIGH	✓ Logical error
`refund()` non-transactional: if refund succeeds but `prisma.booking.update` fails, payment is refunded but status remains incorrect	handleCancelBooking.ts:610	HIGH	✓ Race condition
Potential NPE: `const [organizerUser] = users` crashes if `users` is empty (e.g., seat-based events)	handleNewBooking.ts:776	HIGH	✓ Runtime crash
`return null` when payment app not found → booking status not updated to REJECTED, email not sent	bookings.tsx:964	HIGH	✓ Silent failure
`cancelScheduledJobs` called without `await` → fire-and-forget	bookings.tsx:488	HIGH	✓ Missing await
`Promise.all` without error isolation: if any `getCalendar` call rejects, the entire `getCachedResults` fails	CalendarManager.ts:142	MEDIUM	✓ Related to golden #1
Missing null check after `prisma.credential.findUnique` → `getCalendar(null)` returns null	EventManager.ts:485	MEDIUM	✓ Dangling reference
`getVideoAdapters` silently skips credentials without app → no log or warning	videoClient.ts:28	MEDIUM	✓ Silent failure
Missing scheduled job cancellation and workflow reminders cleanup in reschedule path	vital/reschedule.ts:146	MEDIUM	✓ Incomplete cleanup

Honest Analysis

FriendlyReviewer detected both golden bugs on this PR — one exactly, one partially. The forEach(async) fire-and-forget pattern (Golden #2) was found across 3 files, matching and exceeding the golden reference. The missing try-catch around dynamic imports (Golden #1) was only partially covered: FriendlyReviewer found the Promise.all error isolation problem in CalendarManager.ts, which is the same class of failure, but at a different call site. The specific try-catch around the dynamic import() calls themselves was not flagged.

The real story here is the supplementary findings. FriendlyReviewer found 14 additional issues, 10 of them HIGH severity, and every single one is legitimate. The permission bypass in handleCancelBooking.ts (L53) — any authenticated user can cancel any future booking by UID — is a genuine security vulnerability that was lurking in code that predates this PR. The Prisma runtime error with undefined id in findUnique is a crash-in-production waiting to happen. These are not edge cases; they are operational bugs that would surface in normal usage.

The blast radius pattern is visible again here. The code duplication finding (identical reschedule.ts in two directories) is the kind of cross-file issue that a human reviewer focused on the diff would miss, but FriendlyReviewer catches because it reads the full tree. Similarly, the non-transactional refund (refund() + prisma.booking.update without atomicity) is a race condition that needs to be understood at the system level, not just the line level.

That said, this PR page itself has some limitations. The 800% extended coverage number is mathematically correct per the formula but practically meaningless — it reflects that FriendlyReviewer finds many more issues than a small golden set captures, not that 800% of the relevant bugs were found. The golden set of 2 comments is simply too small to be informative on its own. Additionally, some findings (the misleading comment, the byte-for-byte duplication) are genuine quality issues but not production bugs; separating "will cause an incident" from "should be fixed eventually" requires human judgment that no automated score can provide.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
13 high, 4 medium

This merge request refactors the app store from static to dynamic imports, introducing async behavior to calendar, video, and payment integrations. While the direction is correct for reducing bundle size, the implementation introduces several critical bugs and code quality issues that must be addressed before merging. The most significant issues include: (1) silent promise leaks from async functions called synchronously or via forEach(async ...) across multiple files, (2) a permission bypass in booking cancellation that allows any user to cancel future bookings, (3) Prisma runtime errors from undefined fields in findUnique, (4) missing cleanup and error handling in reschedule flows, (5) regression in event deletion parameters (missing externalCalendarId), and (6) dead code and non-transactional payment refunds. Code duplication and logging inconsistencies reduce maintainability. The review recommends blocking the merge until these issues are resolved.

These might need a close look

🔴 packages/core/CalendarManager.ts (L23)
CRITICAL: getCalendarCredentials remains synchronous but calls the async getCalendar(), causing the calendar field in returned objects to be a Promise. This breaks the type contract and relies on callers accidentally awaiting it.
🔴 packages/app-store/vital/lib/reschedule.ts (L133)
CRITICAL: forEach(async ...) creates fire-and-forget promises; errors become unhandled rejections, and deleteEvent/deleteMeeting are never awaited. Replace with for...of or Promise.all(map(...)).
🔴 packages/app-store/vital/lib/reschedule.ts (L136)
BUG: Missing externalCalendarId parameter in deleteEvent call. This may cause calendar integrations (e.g., Google Calendar) to fail to delete the correct event.
🔴 packages/app-store/vital/lib/reschedule.ts (L1)
CRITICAL CODE DUPLICATION: This file is a byte-for-byte identical copy of wipemycalother/lib/reschedule.ts. Extract shared logic to a common utility.
🔴 packages/features/bookings/lib/handleCancelBooking.ts (L458)
CRITICAL: forEach(async ...) leaves apiDeletes promises unawaited and errors unhandled. Convert to for...of or Promise.all.
🔴 packages/features/bookings/lib/handleCancelBooking.ts (L53)
CRITICAL: Permission bypass - any authenticated user (or unauthenticated with userId=-1) can cancel any future booking by UID. Add organizer/team-owner check.
🔴 packages/features/bookings/lib/handleCancelBooking.ts (L18)
CRITICAL: findUnique({ where: { id, uid } }) with undefined id causes Prisma runtime error. Use conditional where or findFirst.
🔴 packages/features/bookings/lib/handleCancelBooking.ts (L232)
BUG: lastAttendee check .length < 0 is always false; should be .length === 0. This prevents calendar event deletion for the last attendee of a seated booking.
🔴 packages/features/bookings/lib/handleCancelBooking.ts (L610)
Non-transactional refund: if refund() succeeds but prisma.booking.update fails, the payment is refunded but status remains unchanged. Wrap in a transaction.
🔴 packages/features/bookings/lib/handleNewBooking.ts (L776)
Potential NPE: const [organizerUser] = users crashes if users is empty (e.g., seat-based events). Add guard to throw HttpError.
🔴 packages/trpc/server/routers/viewer/bookings.tsx (L564)
CRITICAL: Array.forEach(async ...) in requestReschedule - calendar/video deletions are fire-and-forget. Replace with for...of or Promise.all.
🔴 packages/trpc/server/routers/viewer/bookings.tsx (L964)
CRITICAL: return null when payment app not found bypasses booking status update to REJECTED and declines email. Throw TRPCError instead.
🔴 packages/trpc/server/routers/viewer/bookings.tsx (L488)
BUG: cancelScheduledJobs called without await, creating fire-and-forget promise. Add await.

Worth checking

🟡 packages/core/CalendarManager.ts (L142)
Promise.all without error isolation: if any single getCalendar call rejects (e.g., dynamic import fails), the entire getCachedResults function fails. Use Promise.allSettled or wrap each call in try/catch.
🟡 packages/core/EventManager.ts (L485)
Missing null check after prisma.credential.findUnique: if credential is not found, getCalendar(null) returns null and the old calendar event reference is left dangling. Add a null check and log a warning.
🟡 packages/core/videoClient.ts (L28)
getVideoAdapters silently skips credentials whose app lookup fails. Consider logging a warning, similar to getCalendar.
🟡 packages/app-store/vital/lib/reschedule.ts (L146)
Missing scheduled job cancellation and workflow reminders cleanup, unlike the trpc router version. This could leave stale reminders after rescheduling.