Comprehensive workflow reminder management for booking lifecycle events

[ShashankFC]

Test 6nn---nReplicated from ai-code-review-evaluation/cal.com-greptile#6

[github-actions]

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Comprehensive workflow reminder management for booking lifecycle events". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

[ShashankFC]

@claude

[claude]

Claude finished @ShashankFC's task in 2m 59s —— View job

---

Code Review Complete

Todo List

• Read repository guidelines (CLAUDE.md)
• Review PR changes and understand the implementation
• Analyze database migration and schema changes
• Review booking lifecycle management changes in detail
• Check workflow reminder management implementation
• Provide comprehensive code review feedback

---

Overview

This PR implements comprehensive workflow reminder management for booking lifecycle events (cancel/reschedule). The changes introduce a new cancelled field to track reminders that should be cancelled, and refactors the reminder deletion logic to handle different scenarios more gracefully.

Issues Found

1. Critical: PR Title Doesn't Follow Conventional Commits

The PR title "Comprehensive workflow reminder management for booking lifecycle events" needs a conventional commit prefix. Based on the changes, this should be:

fix: comprehensive workflow reminder management for booking lifecycle events

The bot comment flagged this - please update the PR title to include a valid prefix (fix, feat, refactor, etc.).

---

2. Bug: Missing null check in scheduleEmailReminders.ts (packages/features/ee/workflows/api/scheduleEmailReminders.ts:44-77)

The new code cancels reminders marked as cancelled: true, but doesn't check if reminder.referenceId exists before calling the SendGrid API:

for (const reminder of remindersToCancel) {
  await client.request({
    url: "/v3/user/scheduled_sends",
    method: "POST",
    body: {
      batch_id: reminder.referenceId,  // ⚠️ Could be null
      status: "cancel",
    },
  });

Impact: Will throw an error if referenceId is null (which can happen for unscheduled reminders).

Fix: Add a null check similar to the pattern in emailReminderManager.ts:

for (const reminder of remindersToCancel) {
  if (reminder.referenceId) {
    await client.request({
      url: "/v3/user/scheduled_sends",
      method: "POST",
      body: {
        batch_id: reminder.referenceId,
        status: "cancel",
      },
    });
  }
  // ... rest of the code
}

---

3. Design Issue: Inconsistent deletion behavior between EMAIL and SMS (packages/features/ee/workflows/lib/reminders/)

For EMAIL reminders, the code uses a soft-delete approach (marking as cancelled: true) which allows a CRON job to clean them up later:

// emailReminderManager.ts:225-232
await prisma.workflowReminder.update({
  where: { id: reminderId },
  data: { cancelled: true },
});

For SMS reminders, the code uses immediate hard-delete:

// smsReminderManager.ts:181-185
await prisma.workflowReminder.delete({
  where: { id: reminderId },
});

Problem: This inconsistency means:

• Cancelled email reminders remain in the database until the CRON job runs
• Cancelled SMS reminders are immediately removed
• If an SMS reminder is scheduled more than 1 hour away, it cannot be cancelled by the CRON job

Recommendation: Consider making the behavior consistent. Either:

• Option A: Both use soft-delete with cancelled: true flag
• Option B: Both use immediate deletion where possible

---

4. Migration Issue: Missing default value (packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql)

The migration adds a nullable cancelled column without a default value:

ALTER TABLE "WorkflowReminder" ADD COLUMN "cancelled" BOOLEAN;

While nullable columns are fine, it would be clearer to set existing rows to false explicitly:

ALTER TABLE "WorkflowReminder" ADD COLUMN "cancelled" BOOLEAN DEFAULT false;

This makes the intent clearer: existing reminders are not cancelled.

---

5. Code Quality: Incorrect type annotation (packages/features/ee/workflows/api/scheduleEmailReminders.ts:54)

const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];

This type is incorrect. Prisma.Prisma__WorkflowReminderClient is the return type of a single Prisma operation, not what should be in an array. The correct type should be:

const workflowRemindersToDelete: Promise<WorkflowReminder>[] = [];

---

6. Potential Race Condition: CRON timing vs immediate operations (packages/features/ee/workflows/api/scheduleEmailReminders.ts)

The CRON job cancels reminders scheduled "within the next hour":

scheduledDate: {
  lte: dayjs().add(1, "hour").toISOString(),
}

However, when immediateDelete is true in deleteScheduledEmailReminder, it cancels immediately without marking as cancelled. This could lead to:

• CRON tries to cancel a reminder that was already cancelled
• Race condition if both run at the same time

Recommendation: Ensure the CRON job filters out already-processed reminders or handles errors gracefully.

---

7. Code Smell: Removed unused imports but changed signature (packages/features/bookings/lib/handleCancelBooking.ts:485-492)

The refactor removed the Prisma and PrismaPromise imports and changed from batching deletes to calling delete functions directly. While this simplifies the code, the workflow reminder deletion is now not atomic with the booking deletion.

Before: All deletes were in a single Promise.all()
After: Reminder deletes happen in a separate loop

Consider if this could cause issues where a booking is deleted but reminders aren't, or vice versa.

---

8. Missing Error Handling: Silent failures (multiple files)

Several places call delete functions without handling failures:

// handleCancelBooking.ts:485-492
updatedBookings.forEach((booking) => {
  booking.workflowReminders.forEach((reminder) => {
    if (reminder.method === WorkflowMethods.EMAIL) {
      deleteScheduledEmailReminder(reminder.id, reminder.referenceId); // No await, no error handling
    } else if (reminder.method === WorkflowMethods.SMS) {
      deleteScheduledSMSReminder(reminder.id, reminder.referenceId); // No await, no error handling
    }
  });
});

These functions are async but aren't being awaited, and errors are only logged internally. This means:

• The code continues even if reminder deletion fails
• No way to know if the operation succeeded

Recommendation: Either await these calls properly or document that failures are acceptable.

---

9. Inconsistent Parameter Naming: immediateDelete flag (packages/features/ee/workflows/lib/reminders/emailReminderManager.ts:200)

The immediateDelete parameter is used only for rescheduling scenarios but the naming doesn't make this clear. Consider renaming to skipSoftDelete or forceImmediate to better communicate intent.

---

10. Unused Code: Redundant API call (packages/features/ee/workflows/lib/reminders/emailReminderManager.ts:214-227)

The old code had two API calls to cancel a reminder:

// Old code (removed)
await client.request({ url: "/v3/user/scheduled_sends", method: "POST", ... });
await client.request({ url: `/v3/user/scheduled_sends/${referenceId}`, method: "DELETE" });

The new code only uses the first one. Was the DELETE call redundant, or is there a reason it was removed? If it's intentional, great! If not, this might cause reminders not to be fully cancelled in SendGrid.

---

Positive Aspects

✅ Good: The soft-delete approach with the cancelled field is a good pattern for handling scheduled reminders that can't be cancelled immediately

✅ Good: The refactoring simplifies the deletion logic in handleCancelBooking.ts by removing complex batching

✅ Good: Adding the immediateDelete parameter provides flexibility for different cancellation scenarios

✅ Good: The CRON job properly handles the cleanup of cancelled reminders

✅ Good: Null checks added for referenceId in most places (just missing in one location)

---

Recommendations

1. Fix the critical bug in scheduleEmailReminders.ts by adding null check for referenceId
2. Update the PR title to follow Conventional Commits
3. Consider making deletion behavior consistent between EMAIL and SMS reminders
4. Add proper await/error handling for async reminder deletion calls
5. Fix the type annotation for workflowRemindersToDelete
6. Add migration default value for clarity
7. Document the intentional change from two API calls to one for SendGrid cancellation

---

Testing Recommendations

Please ensure the following scenarios are tested:

• Cancel a booking with scheduled email reminders (< 1 hour away)
• Cancel a booking with scheduled email reminders (> 1 hour away)
• Cancel a booking with scheduled SMS reminders
• Reschedule a booking with existing reminders
• Handle reminders with null referenceId
• CRON job properly cancels marked reminders
• Multiple simultaneous cancellations don't cause race conditions

---

[github-actions]

This PR is being marked as stale due to inactivity.