Comprehensive workflow reminder management for booking lifecycle events#7
Open
ShashankFC wants to merge 1 commit into
Open
Comprehensive workflow reminder management for booking lifecycle events#7ShashankFC wants to merge 1 commit into
ShashankFC wants to merge 1 commit into
Conversation
…re still sent (#7232) * small UI fix * fix cancelling scheduled emails * improve comments * delete reminders for rescheduled bookings * add migration file * cancel rescheduled bookings immediately * remove immediate delete for request reschedule --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com>
Author
|
@cubic-dev-ai review this pull request |
Contributor
@ShashankFC I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 10 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/bookings/lib/handleCancelBooking.ts">
<violation number="1" location="packages/features/bookings/lib/handleCancelBooking.ts:487">
P1: The `deleteScheduledEmailReminder` and `deleteScheduledSMSReminder` calls are fire-and-forget here — their returned promises are neither awaited nor collected. These async functions perform DB writes internally (marking reminders as `cancelled: true` or deleting them). The old code explicitly awaited the DB cleanup via `prismaPromises`. Collect these promises and include them in the `Promise.all` call to ensure reminder cancellation completes before the response is sent.</violation>
</file>
<file name="packages/features/ee/workflows/lib/reminders/smsReminderManager.ts">
<violation number="1" location="packages/features/ee/workflows/lib/reminders/smsReminderManager.ts:179">
P1: Bug: If `twilio.cancelSMS()` throws an error, `prisma.workflowReminder.delete()` is never executed, leaving orphaned reminder records in the database. The DB cleanup should not depend on a successful Twilio API call. Wrap the Twilio call in its own try-catch, or perform the DB delete in a `finally` block, so the record is always removed.</violation>
</file>
<file name="packages/features/ee/workflows/api/scheduleEmailReminders.ts">
<violation number="1" location="packages/features/ee/workflows/api/scheduleEmailReminders.ts:46">
P1: Missing `method: WorkflowMethods.EMAIL` filter. This query will also fetch cancelled SMS reminders and attempt to cancel them via SendGrid, which is incorrect — SMS reminders use Twilio and are handled by `scheduleSMSReminders.ts`.</violation>
<violation number="2" location="packages/features/ee/workflows/api/scheduleEmailReminders.ts:53">
P1: The `try/catch` wraps the entire `for` loop, so a single SendGrid API failure aborts processing of all remaining reminders **and** abandons the pending DB deletes for already-cancelled ones. Move the `try/catch` inside the loop so each reminder is handled independently.</violation>
<violation number="3" location="packages/features/ee/workflows/api/scheduleEmailReminders.ts:61">
P2: `reminder.referenceId` may be `null` if the reminder was cancelled before being scheduled with SendGrid. Sending a null `batch_id` to the SendGrid API will fail. Add a filter for `scheduled: true` (or `referenceId: { not: null }`) to the query, or guard before the API call.</violation>
</file>
<file name="packages/prisma/schema.prisma">
<violation number="1" location="packages/prisma/schema.prisma:644">
P2: Consider using `Boolean @default(false)` instead of `Boolean?` for the `cancelled` field. A nullable boolean creates three states (`true`, `false`, `null`) where only two are meaningful (cancelled or not). This adds query complexity since all lookups must handle `null` as equivalent to `false`. Using a non-nullable boolean with a default is consistent with the majority of flag fields in this schema (`paid`, `hidden`, `requiresConfirmation`, etc.) and with the `scheduled` field on this same model.</violation>
</file>
<file name="packages/features/ee/workflows/lib/reminders/emailReminderManager.ts">
<violation number="1" location="packages/features/ee/workflows/lib/reminders/emailReminderManager.ts:210">
P1: The `immediateDelete` path cancels the SendGrid scheduled send but doesn't update or delete the `workflowReminder` DB record. This leaves an orphaned record (`scheduled: true`, `cancelled: false`) that no longer corresponds to an active SendGrid job. The record won't be picked up by the cron's cancellation logic (which looks for `cancelled: true`) and will linger until the generic past-due cleanup sweeps it. Either delete the DB record or mark it as cancelled after the SendGrid cancellation.</violation>
</file>
<file name="packages/features/bookings/lib/handleNewBooking.ts">
<violation number="1" location="packages/features/bookings/lib/handleNewBooking.ts:966">
P1: Async reminder deletion functions are called without `await` inside `forEach`, making them fire-and-forget. The surrounding `try/catch` will never catch their errors, and the reschedule proceeds before old reminders are confirmed cancelled. Use `Promise.all` with `.map` to properly await all deletions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+487
to
498
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| const reminderToDelete = prisma.workflowReminder.deleteMany({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| }); | ||
| remindersToDelete.push(reminderToDelete); | ||
| }); | ||
| }); | ||
|
|
||
| const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes].concat( | ||
| remindersToDelete | ||
| ); | ||
| const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes]; | ||
|
|
||
| await Promise.all(prismaPromises.concat(apiDeletes)); | ||
|
|
Contributor
There was a problem hiding this comment.
P1: The deleteScheduledEmailReminder and deleteScheduledSMSReminder calls are fire-and-forget here — their returned promises are neither awaited nor collected. These async functions perform DB writes internally (marking reminders as cancelled: true or deleting them). The old code explicitly awaited the DB cleanup via prismaPromises. Collect these promises and include them in the Promise.all call to ensure reminder cancellation completes before the response is sent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/bookings/lib/handleCancelBooking.ts, line 487:
<comment>The `deleteScheduledEmailReminder` and `deleteScheduledSMSReminder` calls are fire-and-forget here — their returned promises are neither awaited nor collected. These async functions perform DB writes internally (marking reminders as `cancelled: true` or deleting them). The old code explicitly awaited the DB cleanup via `prismaPromises`. Collect these promises and include them in the `Promise.all` call to ensure reminder cancellation completes before the response is sent.</comment>
<file context>
@@ -483,29 +481,18 @@ async function handler(req: NextApiRequest & { userId?: number }) {
- } else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.referenceId);
- }
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ } else if (reminder.method === WorkflowMethods.SMS) {
</file context>
Suggested change
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| const reminderToDelete = prisma.workflowReminder.deleteMany({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| }); | |
| remindersToDelete.push(reminderToDelete); | |
| }); | |
| }); | |
| const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes].concat( | |
| remindersToDelete | |
| ); | |
| const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes]; | |
| await Promise.all(prismaPromises.concat(apiDeletes)); | |
| const reminderCancellationPromises: Promise<unknown>[] = []; | |
| updatedBookings.forEach((booking) => { | |
| booking.workflowReminders.forEach((reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| reminderCancellationPromises.push(deleteScheduledEmailReminder(reminder.id, reminder.referenceId)); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| reminderCancellationPromises.push(deleteScheduledSMSReminder(reminder.id, reminder.referenceId)); | |
| } | |
| }); | |
| }); | |
| const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes]; | |
| await Promise.all(prismaPromises.concat(apiDeletes).concat(reminderCancellationPromises)); |
Comment on lines
+179
to
+186
| if (referenceId) { | ||
| await twilio.cancelSMS(referenceId); | ||
| } | ||
| await prisma.workflowReminder.delete({ | ||
| where: { | ||
| id: reminderId, | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
P1: Bug: If twilio.cancelSMS() throws an error, prisma.workflowReminder.delete() is never executed, leaving orphaned reminder records in the database. The DB cleanup should not depend on a successful Twilio API call. Wrap the Twilio call in its own try-catch, or perform the DB delete in a finally block, so the record is always removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/ee/workflows/lib/reminders/smsReminderManager.ts, line 179:
<comment>Bug: If `twilio.cancelSMS()` throws an error, `prisma.workflowReminder.delete()` is never executed, leaving orphaned reminder records in the database. The DB cleanup should not depend on a successful Twilio API call. Wrap the Twilio call in its own try-catch, or perform the DB delete in a `finally` block, so the record is always removed.</comment>
<file context>
@@ -174,9 +174,16 @@ export const scheduleSMSReminder = async (
+export const deleteScheduledSMSReminder = async (reminderId: number, referenceId: string | null) => {
try {
- await twilio.cancelSMS(referenceId);
+ if (referenceId) {
+ await twilio.cancelSMS(referenceId);
+ }
</file context>
Suggested change
| if (referenceId) { | |
| await twilio.cancelSMS(referenceId); | |
| } | |
| await prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminderId, | |
| }, | |
| }); | |
| if (referenceId) { | |
| try { | |
| await twilio.cancelSMS(referenceId); | |
| } catch (error) { | |
| console.log(`Error canceling SMS on Twilio with error ${error}`); | |
| } | |
| } | |
| await prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminderId, | |
| }, | |
| }); |
| //cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour | ||
| const remindersToCancel = await prisma.workflowReminder.findMany({ | ||
| where: { | ||
| cancelled: true, |
Contributor
There was a problem hiding this comment.
P1: Missing method: WorkflowMethods.EMAIL filter. This query will also fetch cancelled SMS reminders and attempt to cancel them via SendGrid, which is incorrect — SMS reminders use Twilio and are handled by scheduleSMSReminders.ts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/ee/workflows/api/scheduleEmailReminders.ts, line 46:
<comment>Missing `method: WorkflowMethods.EMAIL` filter. This query will also fetch cancelled SMS reminders and attempt to cancel them via SendGrid, which is incorrect — SMS reminders use Twilio and are handled by `scheduleSMSReminders.ts`.</comment>
<file context>
@@ -39,6 +40,42 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
+ //cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour
+ const remindersToCancel = await prisma.workflowReminder.findMany({
+ where: {
+ cancelled: true,
+ scheduledDate: {
+ lte: dayjs().add(1, "hour").toISOString(),
</file context>
Suggested change
| cancelled: true, | |
| method: WorkflowMethods.EMAIL, | |
| cancelled: true, |
Comment on lines
+53
to
+77
| try { | ||
| const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; | ||
|
|
||
| for (const reminder of remindersToCancel) { | ||
| await client.request({ | ||
| url: "/v3/user/scheduled_sends", | ||
| method: "POST", | ||
| body: { | ||
| batch_id: reminder.referenceId, | ||
| status: "cancel", | ||
| }, | ||
| }); | ||
|
|
||
| const workflowReminderToDelete = prisma.workflowReminder.delete({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| }); | ||
|
|
||
| workflowRemindersToDelete.push(workflowReminderToDelete); | ||
| } | ||
| await Promise.all(workflowRemindersToDelete); | ||
| } catch (error) { | ||
| console.log(`Error cancelling scheduled Emails: ${error}`); | ||
| } |
Contributor
There was a problem hiding this comment.
P1: The try/catch wraps the entire for loop, so a single SendGrid API failure aborts processing of all remaining reminders and abandons the pending DB deletes for already-cancelled ones. Move the try/catch inside the loop so each reminder is handled independently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/ee/workflows/api/scheduleEmailReminders.ts, line 53:
<comment>The `try/catch` wraps the entire `for` loop, so a single SendGrid API failure aborts processing of all remaining reminders **and** abandons the pending DB deletes for already-cancelled ones. Move the `try/catch` inside the loop so each reminder is handled independently.</comment>
<file context>
@@ -39,6 +40,42 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
+ },
+ });
+
+ try {
+ const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];
+
</file context>
Suggested change
| try { | |
| const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; | |
| for (const reminder of remindersToCancel) { | |
| await client.request({ | |
| url: "/v3/user/scheduled_sends", | |
| method: "POST", | |
| body: { | |
| batch_id: reminder.referenceId, | |
| status: "cancel", | |
| }, | |
| }); | |
| const workflowReminderToDelete = prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| }); | |
| workflowRemindersToDelete.push(workflowReminderToDelete); | |
| } | |
| await Promise.all(workflowRemindersToDelete); | |
| } catch (error) { | |
| console.log(`Error cancelling scheduled Emails: ${error}`); | |
| } | |
| for (const reminder of remindersToCancel) { | |
| try { | |
| await client.request({ | |
| url: "/v3/user/scheduled_sends", | |
| method: "POST", | |
| body: { | |
| batch_id: reminder.referenceId, | |
| status: "cancel", | |
| }, | |
| }); | |
| await prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| }); | |
| } catch (error) { | |
| console.error(`Error cancelling scheduled Email for reminder ${reminder.id}: ${error}`); | |
| } | |
| } |
| }, | ||
| }); | ||
|
|
||
| return; |
Contributor
There was a problem hiding this comment.
P1: The immediateDelete path cancels the SendGrid scheduled send but doesn't update or delete the workflowReminder DB record. This leaves an orphaned record (scheduled: true, cancelled: false) that no longer corresponds to an active SendGrid job. The record won't be picked up by the cron's cancellation logic (which looks for cancelled: true) and will linger until the generic past-due cleanup sweeps it. Either delete the DB record or mark it as cancelled after the SendGrid cancellation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/ee/workflows/lib/reminders/emailReminderManager.ts, line 210:
<comment>The `immediateDelete` path cancels the SendGrid scheduled send but doesn't update or delete the `workflowReminder` DB record. This leaves an orphaned record (`scheduled: true`, `cancelled: false`) that no longer corresponds to an active SendGrid job. The record won't be picked up by the cron's cancellation logic (which looks for `cancelled: true`) and will linger until the generic past-due cleanup sweeps it. Either delete the DB record or mark it as cancelled after the SendGrid cancellation.</comment>
<file context>
@@ -194,20 +194,41 @@ export const scheduleEmailReminder = async (
+ },
+ });
+
+ return;
+ }
</file context>
Suggested change
| return; | |
| await prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminderId, | |
| }, | |
| }); | |
| return; |
Comment on lines
+966
to
+972
| originalRescheduledBooking.workflowReminders.forEach((reminder) => { | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
P1: Async reminder deletion functions are called without await inside forEach, making them fire-and-forget. The surrounding try/catch will never catch their errors, and the reschedule proceeds before old reminders are confirmed cancelled. Use Promise.all with .map to properly await all deletions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/bookings/lib/handleNewBooking.ts, line 966:
<comment>Async reminder deletion functions are called without `await` inside `forEach`, making them fire-and-forget. The surrounding `try/catch` will never catch their errors, and the reschedule proceeds before old reminders are confirmed cancelled. Use `Promise.all` with `.map` to properly await all deletions.</comment>
<file context>
@@ -950,6 +961,19 @@ async function handler(req: NextApiRequest & { userId?: number | undefined }) {
if (originalRescheduledBooking?.uid) {
+ try {
+ // cancel workflow reminders from previous rescheduled booking
+ originalRescheduledBooking.workflowReminders.forEach((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
</file context>
Suggested change
| originalRescheduledBooking.workflowReminders.forEach((reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| }); | |
| 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); | |
| } | |
| })); |
| url: "/v3/user/scheduled_sends", | ||
| method: "POST", | ||
| body: { | ||
| batch_id: reminder.referenceId, |
Contributor
There was a problem hiding this comment.
P2: reminder.referenceId may be null if the reminder was cancelled before being scheduled with SendGrid. Sending a null batch_id to the SendGrid API will fail. Add a filter for scheduled: true (or referenceId: { not: null }) to the query, or guard before the API call.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/ee/workflows/api/scheduleEmailReminders.ts, line 61:
<comment>`reminder.referenceId` may be `null` if the reminder was cancelled before being scheduled with SendGrid. Sending a null `batch_id` to the SendGrid API will fail. Add a filter for `scheduled: true` (or `referenceId: { not: null }`) to the query, or guard before the API call.</comment>
<file context>
@@ -39,6 +40,42 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
+ url: "/v3/user/scheduled_sends",
+ method: "POST",
+ body: {
+ batch_id: reminder.referenceId,
+ status: "cancel",
+ },
</file context>
| scheduled Boolean | ||
| workflowStepId Int | ||
| workflowStep WorkflowStep @relation(fields: [workflowStepId], references: [id], onDelete: Cascade) | ||
| cancelled Boolean? |
Contributor
There was a problem hiding this comment.
P2: Consider using Boolean @default(false) instead of Boolean? for the cancelled field. A nullable boolean creates three states (true, false, null) where only two are meaningful (cancelled or not). This adds query complexity since all lookups must handle null as equivalent to false. Using a non-nullable boolean with a default is consistent with the majority of flag fields in this schema (paid, hidden, requiresConfirmation, etc.) and with the scheduled field on this same model.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/prisma/schema.prisma, line 644:
<comment>Consider using `Boolean @default(false)` instead of `Boolean?` for the `cancelled` field. A nullable boolean creates three states (`true`, `false`, `null`) where only two are meaningful (cancelled or not). This adds query complexity since all lookups must handle `null` as equivalent to `false`. Using a non-nullable boolean with a default is consistent with the majority of flag fields in this schema (`paid`, `hidden`, `requiresConfirmation`, etc.) and with the `scheduled` field on this same model.</comment>
<file context>
@@ -641,6 +641,7 @@ model WorkflowReminder {
scheduled Boolean
workflowStepId Int
workflowStep WorkflowStep @relation(fields: [workflowStepId], references: [id], onDelete: Cascade)
+ cancelled Boolean?
}
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test 6nn
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/cal.com-coderabbit#6](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/6)*