Comprehensive workflow reminder management for booking lifecycle events

[akshayutture-augment]

Test 6

---

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

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

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

---

Code Review: Comprehensive workflow reminder management for booking lifecycle events

Summary

This PR introduces a cancelled field to the WorkflowReminder model to properly track and manage workflow reminders when bookings are cancelled or rescheduled. The changes span database migrations, booking handlers, workflow APIs, and reminder management logic.

✅ Strengths

1. Clear intent: The addition of the cancelled boolean flag provides a cleaner approach to marking reminders that should not be sent
2. Database migration: The migration file is properly structured with nullable boolean
3. Comprehensive coverage: Changes touch all relevant layers from database to UI

⚠️ Critical Issues

1. Incomplete Migration - Missing Default Value (Critical)

File: packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql:2

The migration adds a nullable cancelled column without a default value. This is problematic because:

• Existing rows will have NULL values
• The code uses reminder.cancelled === true comparisons which won't catch NULL values
• Inconsistent behavior between old and new records

Recommendation:

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

2. Logic Bug: deleteScheduledSMSReminder Still Deletes Records (High)

File: packages/features/ee/workflows/lib/reminders/smsReminderManager.ts:177-190

The SMS reminder deletion still immediately deletes records instead of marking them as cancelled (unlike the email version which was updated). This creates inconsistent behavior:

export const deleteScheduledSMSReminder = async (reminderId: number, referenceId: string | null) => {
  try {
    if (referenceId) {
      await twilio.cancelSMS(referenceId);
    }
    await prisma.workflowReminder.delete({  // ❌ Still deletes
      where: {
        id: reminderId,
      },
    });
  } catch (error) {
    console.log(`Error canceling reminder with error ${error}`);
  }
};

Recommendation:

export const deleteScheduledSMSReminder = async (reminderId: number, referenceId: string | null) => {
  try {
    if (referenceId) {
      await twilio.cancelSMS(referenceId);
    }
    // Mark as cancelled instead of deleting
    await prisma.workflowReminder.update({
      where: {
        id: reminderId,
      },
      data: {
        cancelled: true,
      },
    });
  } catch (error) {
    console.log(`Error canceling reminder with error ${error}`);
  }
};

3. Inconsistent Handling in deleteScheduledEmailReminder (Medium)

File: packages/features/ee/workflows/lib/reminders/emailReminderManager.ts:197-236

The function has three different behaviors depending on parameters:

• If !referenceId: deletes the record
• If immediateDelete: cancels via API but doesn't update the database record
• Otherwise: marks as cancelled

This is confusing and error-prone. The immediateDelete path cancels the SendGrid batch but leaves the database record in an inconsistent state.

Recommendation: Standardize the approach:

export const deleteScheduledEmailReminder = async (
  reminderId: number,
  referenceId: string | null,
  immediateDelete?: boolean
) => {
  try {
    // Cancel at provider if we have a referenceId
    if (referenceId) {
      await client.request({
        url: "/v3/user/scheduled_sends",
        method: "POST",
        body: {
          batch_id: referenceId,
          status: "cancel",
        },
      });
    }
    
    // Always mark as cancelled (or delete if no referenceId)
    if (!referenceId) {
      await prisma.workflowReminder.delete({
        where: { id: reminderId },
      });
    } else {
      await prisma.workflowReminder.update({
        where: { id: reminderId },
        data: { cancelled: true },
      });
    }
  } catch (error) {
    console.log(`Error canceling reminder with error ${error}`);
  }
};

4. Missing NULL Checks in scheduleEmailReminders.ts (High)

File: packages/features/ee/workflows/api/scheduleEmailReminders.ts:44-77

The code queries for reminders with cancelled: true but doesn't account for existing records with cancelled: null:

const remindersToCancel = await prisma.workflowReminder.findMany({
  where: {
    cancelled: true,  // ❌ Won't find records where cancelled is NULL
    scheduledDate: {
      lte: dayjs().add(1, "hour").toISOString(),
    },
  },
});

Recommendation:

const remindersToCancel = await prisma.workflowReminder.findMany({
  where: {
    cancelled: { not: false }, // Catches both true and NULL
    scheduledDate: {
      lte: dayjs().add(1, "hour").toISOString(),
    },
  },
});

Or better yet, fix the migration to have a default value.

5. Race Condition Risk in Reschedule Flow (Medium)

File: packages/features/bookings/lib/handleNewBooking.ts:965-975

When rescheduling, workflow reminders are cancelled in a forEach loop without waiting:

originalRescheduledBooking.workflowReminders.forEach((reminder) => {
  if (reminder.method === WorkflowMethods.EMAIL) {
    deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);  // ❌ Not awaited
  } else if (reminder.method === WorkflowMethods.SMS) {
    deleteScheduledSMSReminder(reminder.id, reminder.referenceId);  // ❌ Not awaited
  }
});

Recommendation:

await Promise.all(
  originalRescheduledBooking.workflowReminders.map(async (reminder) => {
    if (reminder.method === WorkflowMethods.EMAIL) {
      await deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
    } else if (reminder.method === WorkflowMethods.SMS) {
      await deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
    }
  })
);

🔍 Minor Issues

6. Poor Error Handling with console.log (Low)

Multiple files use console.log for error handling instead of proper logging:

• emailReminderManager.ts:141, 180, 234
• smsReminderManager.ts:128, 159, 188
• scheduleEmailReminders.ts:76, 222

Recommendation: Use the existing logger:

import logger from "@calcom/lib/logger";
const log = logger.getChildLogger({ prefix: ["workflow-reminders"] });

// Then use:
log.error("Error canceling reminder", error);

7. Type Safety: Prisma.Prisma__WorkflowReminderClient Type Issue (Low)

File: packages/features/ee/workflows/api/scheduleEmailReminders.ts:54

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

This type is incorrect - it's storing client promises, not results. Should be:

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

8. Missing Transaction in handleCancelBooking.ts (Medium)

File: packages/features/bookings/lib/handleCancelBooking.ts:484-493

The workflow reminder cancellation happens outside of any transaction, which could lead to inconsistent state if the process fails partway through.

Recommendation: Consider wrapping the entire cancellation logic in a transaction or at least ensure all database operations complete before returning success.

📝 Suggested Improvements

1. Add Indexes: Consider adding an index on WorkflowReminder.cancelled for the queries in scheduleEmailReminders.ts

2. Add Tests: This PR changes critical booking lifecycle logic but doesn't include tests. Consider adding:

   • Unit tests for the reminder manager functions
   • Integration tests for the reschedule flow
   • Tests verifying cancelled reminders aren't sent

3. Documentation: Add comments explaining the cancelled field behavior, especially the difference between cancelled: true and deleted records

4. Consistency: The UI component WorkflowStepContainer.tsx has many changes but they appear to be formatting only. Consider separating formatting changes from functional changes in future PRs.

🎯 Priority Actions

1. Fix the migration to include NOT NULL DEFAULT false
2. Update deleteScheduledSMSReminder to mark as cancelled instead of deleting
3. Fix the race condition in the reschedule flow by awaiting all cancellations
4. Handle NULL values in the scheduleEmailReminders query

---

Overall Assessment

The PR addresses an important issue with workflow reminders for cancelled/rescheduled bookings. However, there are several critical bugs that need to be fixed before merging:

• The database migration needs a default value
• SMS reminder deletion behavior is inconsistent with email
• NULL value handling needs attention
• Race conditions in the reschedule flow

Once these issues are addressed, this will be a solid improvement to the workflow reminder system.

---

[github-actions]

This PR is being marked as stale due to inactivity.