Cal.com / PR #5

Comprehensive workflow reminder management

calcom/cal.com#7232
Golden recall: 100% Extra findings: 10

Golden Comments (2/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 Async functions deleteScheduledEmailReminder and deleteScheduledSMSReminder called without await in forEach → fire-and-forget, reminders not reliably cancelled HIGH ✓ Found FriendlyReviewer found this bug in 4 files (6 occurrences): handleCancelBooking.ts (L483), bookings.tsx (L488), workflows.tsx (L211, 375, 521, 576). Also identified duplicate processing of reminders during reschedule in handleNewBooking.ts (L961). Each instance classified as HIGH with a recommendation to replace forEach(async) with for...of or Promise.all.
#2 immediateDelete=true: SendGrid email cancelled but DB record not deleted/marked cancelled → orphaned records HIGH ✓ Found FriendlyReviewer precisely identified this bug in handleNewBooking.ts (L963) and emailReminderManager.ts (L197). When immediateDelete=true, the SendGrid batch is cancelled but the DB record is neither deleted nor marked cancelled=true, leaving orphaned records in the database.

Supplementary Findings (13 findings)

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

Finding File Severity Legitimate?
Double processing of reminders during reschedule → duplicate side effects via both tRPC and handleNewBooking handleNewBooking.ts:961 HIGH ✓ Duplicate side effects
remindersToCancel query does not filter null referenceId → SendGrid API call receives malformed data and fails scheduleEmailReminders.ts:41 HIGH ✓ API failure risk
Cancellation loop wrapped in single try/catch → one failure blocks all remaining cancellations scheduleEmailReminders.ts:50 HIGH ✓ Cascade failure
Custom onChange on PhoneInput overrides react-hook-form → phone value never propagates to form state WorkflowStepContainer.tsx:409 HIGH ✓ Form state broken
Inconsistent lifecycle: EMAIL soft-cancelled (cancelled flag), SMS hard-deleted from DB handleCancelBooking.ts:488 MEDIUM ✓ Inconsistent behavior
deleteScheduledSMSReminder always hard-deletes, missing immediateDelete param → inconsistent with EMAIL cancellation path smsReminderManager.ts:177 MEDIUM ✓ Missing param
verificationCode never reset when action type or phone changes → stale code validates against wrong configuration WorkflowStepContainer.tsx:447 MEDIUM ✓ Stale state
Unused import: Prisma imported but no longer used in this file workflows.tsx:1 LOW ✓ Dead import
console.log for error logging instead of proper logger utility emailReminderManager.ts:228 LOW ✓ Inconsistent logging
cancelled column nullable without DEFAULT false → consider adding default migration.sql:2 LOW ✓ Missing default

Honest Analysis

FriendlyReviewer found both golden bugs (100% recall) on this PR, which is the strongest possible result against the reference set. The forEach(async) fire-and-forget pattern was found across 4 files with 6 individual occurrences, exceeding the golden reference's coverage. The immediateDelete orphaned record bug was precisely diagnosed.

The supplementary findings are useful but mixed. The double processing of reminders during reschedule is a real defect that would cause duplicate side effects in production. The verificationCode state leak is a subtle React state bug that a human might miss. But the 'missing disabled prop for loading state' finding — while technically valid — is a frontend best practice rather than a bug.

On the 600% extended coverage number: it's mathematically correct per the formula but the denominator is only 2 golden comments. A single PR with different findings against a small reference set produces absurd percentages. The real story is 13 supplementary findings, 7 of which are genuine HIGH-severity bugs.

FriendlyReviewer's full review

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

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
9 high, 3 medium, 3 low

This merge request adds comprehensive workflow reminder management, introducing scheduled email and SMS reminders for booking workflows. The core direction is useful, but the implementation introduces several critical bugs and code quality issues that must be addressed before merging. The most significant issues include: (1) multiple async functions called without await inside forEach across 4 files, causing reminder cancellations to be silently dropped, (2) orphaned database records when immediateDelete=true cancels the SendGrid email but leaves the DB row untouched, (3) duplicate processing of workflow reminders during reschedule through both the tRPC router and handleNewBooking, (4) a fragile cancellation loop that aborts all remaining cancellations on the first error, (5) a SendGrid API call that will fail with malformed request data when referenceId is null, and (6) a React state bug where verificationCode persists across form changes. The review recommends blocking the merge until these issues are resolved.

These might need a close look

  • 🔴 packages/features/bookings/lib/handleCancelBooking.ts (L483)
    CRITICAL: forEach(async ...) calls deleteScheduledEmailReminder and deleteScheduledSMSReminder without await. Reminder cancellations become fire-and-forget and are silently dropped. Replace with for...of or Promise.all.
  • 🔴 packages/trpc/server/routers/viewer/bookings.tsx (L488)
    CRITICAL: forEach(async ...) fires reminder cancellation functions without awaiting them. Same fire-and-forget pattern as handleCancelBooking.ts.
  • 🔴 packages/trpc/server/routers/viewer/workflows.tsx (L211, 375, 521, 576)
    CRITICAL: Multiple instances of forEach(async ...) across the workflows router — reminders scheduled or cancelled without await. Each instance silently swallows errors and drops operations.
  • 🔴 packages/features/bookings/lib/handleNewBooking.ts (L961)
    BUG: Workflow reminders are processed during reschedule in both the tRPC router and handleNewBooking, causing duplicate side effects. Consolidate reminder processing to a single code path.
  • 🔴 packages/features/bookings/lib/handleNewBooking.ts (L963)
    CRITICAL: When immediateDelete=true, the SendGrid email batch is cancelled but the DB record is not deleted or marked cancelled=true. This creates orphaned database records that will never be cleaned up.
  • 🔴 packages/features/ee/workflows/lib/emailReminderManager.ts (L197)
    CRITICAL: Same immediateDelete orphaned record issue — SendGrid cancellation succeeds but the corresponding database row is left untouched, creating inconsistency between the external system and internal state.
  • 🔴 packages/features/ee/workflows/lib/scheduleEmailReminders.ts (L41)
    BUG: The remindersToCancel query does not filter out null referenceId values. When referenceId is null, the subsequent SendGrid API call receives malformed data and fails.
  • 🔴 packages/features/ee/workflows/lib/scheduleEmailReminders.ts (L50)
    CRITICAL: The entire cancellation loop is wrapped in a single try/catch block. If any single cancellation fails (e.g., SendGrid rate limit), the remaining cancellations are skipped entirely. Add per-item error handling.
  • 🔴 packages/features/ee/workflows/components/WorkflowStepContainer.tsx (L409)
    BUG: Custom onChange handler on PhoneInput overrides react-hook-form's register, so the phone value never propagates to the form state. Also verificationCode is never reset when action type or phone changes, causing stale code validation.

Worth checking

  • 🟡 packages/features/bookings/lib/handleCancelBooking.ts (L488)
    Inconsistent lifecycle: EMAIL reminders are soft-cancelled (cancelled flag set) while SMS reminders are hard-deleted from the database. Unify to a consistent approach.
  • 🟡 packages/features/ee/workflows/lib/smsReminderManager.ts (L177)
    deleteScheduledSMSReminder always hard-deletes records, missing the immediateDelete parameter that the email cancellation path supports. This creates inconsistent behavior between email and SMS cancellation.
  • 🟡 packages/features/ee/workflows/components/WorkflowStepContainer.tsx (L447)
    verificationCode state is never reset when the action type or phone number changes. A previously verified code remains valid for the wrong configuration, causing stale code validation issues.

Small things (take or leave)

  • 🔵 packages/trpc/server/routers/viewer/workflows.tsx (L1)
    Unused import: Prisma is imported but no longer used after the workflow changes. Remove the unused import.
  • 🔵 packages/features/ee/workflows/lib/emailReminderManager.ts (L228)
    console.log used for error logging instead of the project's structured logger. Replace for consistency and observability.
  • 🔵 packages/features/ee/workflows/lib/migrations/migration.sql (L2)
    cancelled column is nullable without a default value. Consider adding DEFAULT false to ensure predictable behavior when records are created without explicitly setting this field.