Comprehensive workflow reminder management for booking lifecycle events#6
Comprehensive workflow reminder management for booking lifecycle events#6Frawatson 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>
🤖 Grapple PR ReviewSummaryThis PR adds 1 file (migration.sql), modifies 9 files. Title: "Comprehensive workflow reminder management for booking lifecycle events" Changes: 10 files (+241 / -203 lines)
Resolves: #6 (Comprehensive workflow reminder management for booking lifecycle events) CI: ⏳ 0 passed, 0 failed Impact Diagramgraph TD
pr_0[/"🔀 PR Changes: 10 files"/]
f1["🟡 handleCancelBooking.ts +6/-19"]
pr_0 --> f1
f2["🟡 handleNewBooking.ts +26/-2"]
pr_0 --> f2
f3["🟡 scheduleEmailReminders.ts +37/-0"]
pr_0 --> f3
f4["🟡 WorkflowStepContainer.tsx +60/-66"]
pr_0 --> f4
f5["🟡 emailReminderManager.ts +33/-12"]
pr_0 --> f5
f6["🟡 smsReminderManager.ts +9/-2"]
pr_0 --> f6
f7["🟢 migration.sql +2/-0"]
pr_0 --> f7
f8["🟡 schema.prisma +1/-0"]
pr_0 --> f8
f9["🟡 bookings.tsx +6/-19"]
pr_0 --> f9
f10["🟡 workflows.tsx +61/-83"]
pr_0 --> f10
f1 -.- f2
f5 -.- f6
f9 -.- f10
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
Review details
Powered by Grapple PR — AI that understands your code. |
🤖 Grapple PR ReviewSummaryThis PR overhauls workflow reminder management across the booking lifecycle by introducing a soft-cancel pattern (via a new Changes: 10 files (+241 / -203 lines)
Resolves: #6 (Comprehensive workflow reminder management for booking lifecycle events) CI: ⏳ 0 passed, 0 failed Impact Diagramgraph TD
pr_0[/"🔀 PR Changes: 10 files"/]
f1["🟡 handleCancelBooking.ts +6/-19"]
pr_0 --> f1
f2["🟡 handleNewBooking.ts +26/-2"]
pr_0 --> f2
f3["🟡 scheduleEmailReminders.ts +37/-0"]
pr_0 --> f3
f4["🟡 WorkflowStepContainer.tsx +60/-66"]
pr_0 --> f4
f5["🟡 emailReminderManager.ts +33/-12"]
pr_0 --> f5
f6["🟡 smsReminderManager.ts +9/-2"]
pr_0 --> f6
f7["🟢 migration.sql +2/-0"]
pr_0 --> f7
f8["🟡 schema.prisma +1/-0"]
pr_0 --> f8
f9["🟡 bookings.tsx +6/-19"]
pr_0 --> f9
f10["🟡 workflows.tsx +61/-83"]
pr_0 --> f10
f1 -.- f2
f5 -.- f6
f9 -.- f10
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
Review details
Powered by Grapple PR — AI that understands your code. |
![grapple-pr[bot]](https://avatars.githubusercontent.com/in/3205222?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR overhauls workflow reminder lifecycle management for bookings by introducing a soft-cancel mechanism (via a new cancelled boolean field on WorkflowReminder) instead of immediately deleting reminders, and fixes reminder cleanup logic for cancellations, rescheduling, and workflow updates to handle cases where reminders may not yet have a referenceId.
Changes: 10 files (+241 / -203 lines)
- 🟢 1 file added:
packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql - 🟡 9 files modified:
packages/features/bookings/lib/handleCancelBooking.ts,packages/features/bookings/lib/handleNewBooking.ts,packages/features/ee/workflows/api/scheduleEmailReminders.ts,packages/features/ee/workflows/components/WorkflowStepContainer.tsx,packages/features/ee/workflows/lib/reminders/emailReminderManager.ts+4 more
Resolves: #6 (Comprehensive workflow reminder management for booking lifecycle events)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 10 files"/]
f1["🟡 handleCancelBooking.ts +6/-19"]
pr_0 --> f1
f2["🟡 handleNewBooking.ts +26/-2"]
pr_0 --> f2
f3["🟡 scheduleEmailReminders.ts +37/-0"]
pr_0 --> f3
f4["🟡 WorkflowStepContainer.tsx +60/-66"]
pr_0 --> f4
f5["🟡 emailReminderManager.ts +33/-12"]
pr_0 --> f5
f6["🟡 smsReminderManager.ts +9/-2"]
pr_0 --> f6
f7["🟢 migration.sql +2/-0"]
pr_0 --> f7
f8["🟡 schema.prisma +1/-0"]
pr_0 --> f8
f9["🟡 bookings.tsx +6/-19"]
pr_0 --> f9
f10["🟡 workflows.tsx +61/-83"]
pr_0 --> f10
f1 -.- f2
f5 -.- f6
f9 -.- f10
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
3 major issues found — review recommended.
| Severity | Count |
|---|---|
| 🟠 Major | 3 |
| 🟡 Minor | 5 |
| 🔵 Info | 4 |
Review details
- Reviewed at: 2026-04-07T21:10:37.736Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 101.6s
- Low-confidence findings suppressed: 1
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| } | ||
|
|
||
| //find all unscheduled Email reminders | ||
| const unscheduledReminders = await prisma.workflowReminder.findMany({ |
There was a problem hiding this comment.
🟠 MAJOR — Data Exposure (confidence: 100%)
The scheduleEmailReminders endpoint queries for all cancelled reminders without any authentication or authorization check. This is a server-side API endpoint that, if reachable without auth, could expose reminder data or allow an unauthenticated caller to trigger mass cancellation of SendGrid email batches. Additionally, the endpoint uses reminder.referenceId from the DB directly in a SendGrid API call without validating that it is a valid batch ID format.
Evidence:
- The handler processes
remindersToCancelwhich is fetched from DB with onlycancelled: trueandscheduledDatefilters — no user/tenant scoping reminder.referenceIdis passed directly asbatch_idto the SendGrid API without sanitization or format validation- If referenceId is null for a cancelled reminder, the SendGrid call will be made with
batch_id: null, which could result in unexpected API behavior
Agent: security
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.log(`Error canceling reminder with error ${error}`); |
There was a problem hiding this comment.
🟠 MAJOR — Data Exposure (confidence: 100%)
When immediateDelete=true, deleteScheduledEmailReminder calls the SendGrid cancel API and returns early without deleting or marking the DB record. This orphans the WorkflowReminder record in the database indefinitely. The orphaned record retains the referenceId (a SendGrid batch ID) which could be exploited in future API calls if the record is ever processed again.
Evidence:
- The
if (immediateDelete)block calls the SendGrid API and thenreturn;— no DB deletion orcancelled=trueupdate occurs - PR edge cases acknowledge: 'deleteScheduledEmailReminder with immediateDelete=true does not delete the DB record and does not mark it cancelled — it only calls the SendGrid cancel API and returns early; the DB record will be orphaned'
- Orphaned records with valid referenceIds could be picked up by the scheduleEmailReminders cron if
cancelledis later set to true through another code path
Agent: security
| await Promise.all(workflowRemindersToDelete); | ||
| } catch (error) { | ||
| console.log(`Error cancelling scheduled Emails: ${error}`); | ||
| } |
There was a problem hiding this comment.
🟡 MINOR — Code Organization (confidence: 100%)
Type annotation for workflowRemindersToDelete array is overly complex and incorrect. The array should contain Promise types, not Prisma client method return types.
Evidence:
- Line 52 declares:
const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient[] = []; - This type represents Prisma client instances, not Promise types. The array should be typed as
Promise[]to match the operations being pushed (prisma.workflowReminder.delete() returns a Promise). - This mismatch could cause type safety issues and makes the code harder to understand.
Agent: style
|
|
||
| workflowRemindersToDelete.push(workflowReminderToDelete); | ||
| } | ||
| await Promise.all(workflowRemindersToDelete); |
There was a problem hiding this comment.
🟡 MINOR — Data Exposure (confidence: 91%)
The catch block in the cancellation loop logs the raw error from SendGrid to console, which may include API response bodies containing credentials or internal system details.
Evidence:
console.log(Error cancelling scheduled Emails: ${error})inside a catch that wraps SendGrid API calls- SendGrid error responses can include request details that reveal API key usage patterns
Agent: security
| }); | ||
| } catch (error) { | ||
| log.error("Error while canceling scheduled workflow reminders", error); | ||
| } |
There was a problem hiding this comment.
🟡 MINOR — Documentation (confidence: 83%)
Missing comment explaining why immediateDelete=true is passed when cancelling reminders during rescheduling. This differs from other cancellation paths and warrants clarification.
Evidence:
- Line 967: deleteScheduledEmailReminder passes true for immediateDelete during rescheduling
- In workflows.tsx line 208 (workflow deletion), immediateDelete=true is also used with a similar pattern
- In bookings.tsx reschedule path, immediateDelete is not passed (defaults to false)
- The inconsistency between handleNewBooking (immediateDelete=true) and bookings.tsx (no immediateDelete parameter) is not explained, making maintenance harder.
Agent: style
| cancelled: true, | ||
| }, | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
🟡 MINOR — Data Exposure (confidence: 77%)
Error messages from the SendGrid API (including potential response bodies containing API keys or internal details) are logged to console with the raw error object. This could expose sensitive information in server logs.
Evidence:
console.log(Error canceling reminder with error ${error})logs the full error object which may contain HTTP response details from SendGrid including request headers or authentication tokens
Agent: security
| }); | ||
|
|
||
| const workflowReminderToDelete = prisma.workflowReminder.delete({ | ||
| where: { |
There was a problem hiding this comment.
🔵 INFO — Input Validation (confidence: 100%)
The SendGrid batch cancel API is called with reminder.referenceId without verifying that the value is non-null or matches the expected SendGrid batch ID format (UUID). A null or malformed referenceId could cause unexpected SendGrid API behavior or error responses that abort the entire cancellation loop.
Evidence:
- The
remindersToCancelquery only filters oncancelled: trueandscheduledDate— it does not filter out records wherereferenceId IS NULL - A cancelled reminder with a null referenceId (e.g., one that was never scheduled) would reach the SendGrid API call with
batch_id: null - This would cause a SendGrid API error that aborts the for loop's remaining iterations due to the try/catch wrapping the entire loop
Agent: security
| await Promise.all(workflowRemindersToDelete); | ||
| } catch (error) { | ||
| console.log(`Error cancelling scheduled Emails: ${error}`); | ||
| } |
There was a problem hiding this comment.
🔵 INFO — Error Handling (confidence: 86%)
Error logging uses console.log instead of structured logging. Inconsistent with the codebase pattern that uses logger/console with context.
Evidence:
- Line 76 uses:
console.log(Error cancelling scheduled Emails: ${error}); - Other files in the codebase (emailReminderManager.ts line 234, smsReminderManager.ts line 184) use consistent error logging pattern.
- Should follow the same logging pattern for consistency and debuggability.
Agent: style
|
|
||
| workflowRemindersToDelete.push(workflowReminderToDelete); | ||
| } | ||
| await Promise.all(workflowRemindersToDelete); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (2 lines, 1 file))
The catch block in the cancellation loop logs the raw error from SendGrid to console, which may include API response bodies containing credentials or internal system details.
| await Promise.all(workflowRemindersToDelete); | |
| console.error("Error cancelling scheduled Emails"); |
🤖 Grapple PR auto-fix • minor • confidence: 91%
| }); | ||
| } catch (error) { | ||
| log.error("Error while canceling scheduled workflow reminders", error); | ||
| } |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (8 lines, 1 file))
Missing comment explaining why immediateDelete=true is passed when cancelling reminders during rescheduling. This differs from other cancellation paths and warrants clarification.
| } | |
| // immediateDelete=true is required here (unlike the soft-cancel/bookings.tsx path) | |
| // because rescheduling immediately schedules new reminders for the updated booking. | |
| // Without immediate deletion, the old scheduled emails would still fire for the | |
| // original (now-cancelled) booking time, resulting in duplicate or stale notifications. | |
| // The soft-cancel flag (cancelled=true) is insufficient here since the job must be | |
| // removed from the scheduler queue before new reminders are enqueued. | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); |
🤖 Grapple PR auto-fix • minor • confidence: 83%
| cancelled: true, | ||
| }, | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (2 lines, 1 file))
Error messages from the SendGrid API (including potential response bodies containing API keys or internal details) are logged to console with the raw error object. This could expose sensitive information in server logs.
| } catch (error) { | |
| console.error(`Error canceling reminder: ${error instanceof Error ? error.message : "Unknown error"}`); |
🤖 Grapple PR auto-fix • minor • confidence: 77%
| } | ||
|
|
||
| //find all unscheduled Email reminders | ||
| const unscheduledReminders = await prisma.workflowReminder.findMany({ |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (24 lines, 1 file) — review recommended)
The scheduleEmailReminders endpoint queries for all cancelled reminders without any authentication or authorization check. This is a server-side API endpoint that, if reachable without auth, could expose reminder data or allow an unauthenticated caller to trigger mass cancellation of SendGrid email batches. Additionally, the endpoint uses
reminder.referenceIdfrom the DB directly in a SendGrid API call without validating that it is a valid batch ID format.
--- a/packages/features/ee/workflows/api/scheduleEmailReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleEmailReminders.ts
@@ -40,12 +40,18 @@ 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: {
+ method: WorkflowMethods.EMAIL,
cancelled: true,
scheduledDate: {
lte: dayjs().add(1, "hour").toISOString(),
},
+ // Only attempt SendGrid cancellation for reminders that have a batch ID assigned
+ referenceId: {
+ not: null,
+ },
},
});
+ // SendGrid batch IDs are UUIDs — validate format before making API calls
+ const SENDGRID_BATCH_ID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
+
try {
const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];
for (const reminder of remindersToCancel) {
+ // Guard: referenceId must be a valid SendGrid batch ID UUID before calling the API.
+ // Reminders soft-cancelled before a batch ID was assigned are filtered at the query level,
+ // but we double-check here to be safe and to satisfy TypeScript narrowing.
+ if (!reminder.referenceId || !SENDGRID_BATCH_ID_REGEX.test(reminder.referenceId)) {
+ console.log(
+ `Skipping SendGrid cancellation for reminder ${reminder.id}: referenceId "${reminder.referenceId}" is not a valid batch ID`
+ );
+ // Still clean up the DB record even if we can't cancel the SendGrid batch
+ workflowRemindersToDelete.push(
+ prisma.workflowReminder.delete({
+ where: { id: reminder.id },
+ })
+ );
+ continue;
+ }
+
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}`);
}🤖 Grapple PR auto-fix • major • Review this diff before applying
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.log(`Error canceling reminder with error ${error}`); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (14 lines, 1 file) — review recommended)
When
immediateDelete=true, deleteScheduledEmailReminder calls the SendGrid cancel API and returns early without deleting or marking the DB record. This orphans the WorkflowReminder record in the database indefinitely. The orphaned record retains the referenceId (a SendGrid batch ID) which could be exploited in future API calls if the record is ever processed again.
--- a/packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
+++ b/packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
@@ -213,7 +213,14 @@ export const deleteScheduledEmailReminder = async (
if (immediateDelete) {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: referenceId,
status: "cancel",
},
});
- return;
+ // Mark the DB record as cancelled so it is not orphaned with a live
+ // referenceId. Without this, the record could be picked up again by
+ // the scheduleEmailReminders cron if `cancelled` is ever flipped
+ // through another code path.
+ await prisma.workflowReminder.update({
+ where: {
+ id: reminderId,
+ },
+ data: {
+ cancelled: true,
+ },
+ });
+ return;
}🤖 Grapple PR auto-fix • major • Review this diff before applying
| await Promise.all(workflowRemindersToDelete); | ||
| } catch (error) { | ||
| console.log(`Error cancelling scheduled Emails: ${error}`); | ||
| } |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Small fix (4 lines, 1 file))
Type annotation for workflowRemindersToDelete array is overly complex and incorrect. The array should contain Promise types, not Prisma client method return types.
--- a/packages/features/ee/workflows/api/scheduleEmailReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleEmailReminders.ts
@@ -7,7 +7,7 @@ import type { NextApiRequest, NextApiResponse } from "next";
import dayjs from "@calcom/dayjs";
import { defaultHandler } from "@calcom/lib/server";
import prisma from "@calcom/prisma";
-import { Prisma, WorkflowReminder } from "@calcom/prisma/client";
+import { WorkflowReminder } from "@calcom/prisma/client";
import { bookingMetadataSchema } from "@calcom/prisma/zod-utils";
import customTemplate, { VariablesType } from "../lib/reminders/templates/customTemplate";
@@ -51,7 +51,7 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
try {
- const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];
+ const workflowRemindersToDelete: Promise<WorkflowReminder>[] = [];
for (const reminder of remindersToCancel) {
await client.request({🤖 Grapple PR auto-fix • minor • Review this diff before applying
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR refactors workflow reminder management across the booking lifecycle (new bookings, cancellations, reschedules) to use a soft-cancel pattern for email reminders (marking them as cancelled=true for deferred deletion) while immediately deleting SMS reminders and their DB records. It also adds a new cancelled column to the WorkflowReminder table and introduces a scheduled cleanup mechanism in scheduleEmailReminders.ts that processes cancelled email reminders due within the next hour.
Changes: 10 files (+241 / -203 lines)
- 🟢 1 file added:
packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql - 🟡 9 files modified:
packages/features/bookings/lib/handleCancelBooking.ts,packages/features/bookings/lib/handleNewBooking.ts,packages/features/ee/workflows/api/scheduleEmailReminders.ts,packages/features/ee/workflows/components/WorkflowStepContainer.tsx,packages/features/ee/workflows/lib/reminders/emailReminderManager.ts+4 more
Resolves: #6 (Comprehensive workflow reminder management for booking lifecycle events)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 10 files"/]
f1["🟡 handleCancelBooking.ts +6/-19"]
pr_0 --> f1
f2["🟡 handleNewBooking.ts +26/-2"]
pr_0 --> f2
f3["🟡 scheduleEmailReminders.ts +37/-0"]
pr_0 --> f3
f4["🟡 WorkflowStepContainer.tsx +60/-66"]
pr_0 --> f4
f5["🟡 emailReminderManager.ts +33/-12"]
pr_0 --> f5
f6["🟡 smsReminderManager.ts +9/-2"]
pr_0 --> f6
f7["🟢 migration.sql +2/-0"]
pr_0 --> f7
f8["🟡 schema.prisma +1/-0"]
pr_0 --> f8
f9["🟡 bookings.tsx +6/-19"]
pr_0 --> f9
f10["🟡 workflows.tsx +61/-83"]
pr_0 --> f10
f1 -.- f2
f5 -.- f6
f9 -.- f10
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
2 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 2 |
| 🟠 Major | 3 |
| 🟡 Minor | 1 |
Review details
- Reviewed at: 2026-04-08T04:14:26.791Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 97.0s
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.log(`Error canceling reminder with error ${error}`); |
There was a problem hiding this comment.
🔴 CRITICAL — Incomplete Cancellation / Data Integrity — Uncancelled SendGrid Batches (confidence: 100%)
When deleteScheduledEmailReminder is called WITHOUT immediateDelete=true (e.g., from handleCancelBooking.ts and bookings.tsx), it only sets cancelled=true in the DB and never calls the SendGrid batch cancel API. The cleanup job in scheduleEmailReminders.ts only processes reminders where scheduledDate <= now + 1 hour. Any reminder cancelled more than 1 hour before its scheduled send time will remain marked cancelled=true in the DB but will NOT have its SendGrid batch cancelled — meaning the email WILL be delivered to the attendee of a cancelled or rescheduled booking.
Evidence:
- emailReminderManager.ts new logic: when
immediateDeleteis falsy andreferenceIdexists, onlyprisma.workflowReminder.update({ cancelled: true })is called — no SendGrid API call - scheduleEmailReminders.ts cleanup query:
scheduledDate: { lte: dayjs().add(1, 'hour').toISOString() }— only catches reminders due within 1 hour - handleCancelBooking.ts calls
deleteScheduledEmailReminder(reminder.id, reminder.referenceId)withoutimmediateDelete, so only soft-cancel occurs - PR edge cases explicitly state: 'If a booking is cancelled more than 1 hour before the reminder is due, the reminder will be marked cancelled=true but the SendGrid batch will never be explicitly cancelled'
Agent: security
| } catch (error) { | ||
| console.log(`Error cancelling scheduled Emails: ${error}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Error Handling — Sequential Loop Aborts on First Failure (confidence: 100%)
The cleanup loop uses await inside a for...of loop with a single outer try/catch. If the SendGrid API call fails for any reminder (network error, invalid batch_id, rate limit), the exception propagates to the outer catch, which logs and exits. All subsequent reminders in remindersToCancel are never processed, and workflowRemindersToDelete (which only contains promises for reminders processed before the failure) is never awaited — those DB deletions are also lost.
Evidence:
- Pattern:
for (const reminder of remindersToCancel) { await client.request(...) ... workflowRemindersToDelete.push(...) }inside a singletry { } catch - If
client.requestthrows on reminder N, reminders N+1..M are skipped entirely await Promise.all(workflowRemindersToDelete)is inside the same try block, so if the throw occurs before it's reached, those deletes never execute- This means a transient SendGrid error causes permanently orphaned
cancelled=truerecords that are retried on every scheduled job run until they fall outside the 1-hour window
Agent: security
| }); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Logic Error — immediateDelete Semantics Inversion (confidence: 100%)
The immediateDelete parameter name is misleading and its behavior is inverted from what the name implies. When immediateDelete=true, the function cancels the SendGrid batch immediately but does NOT delete the DB record (returns early). When immediateDelete=false/undefined, it does NOT cancel SendGrid but DOES mark the record as cancelled in the DB. The caller in handleNewBooking.ts passes immediateDelete=true for reschedule flows, expecting the record to be cleaned up by the scheduled job — but reminders more than 1 hour out will never be cleaned up by that job.
Evidence:
if (immediateDelete) { await client.request(...cancel...); return; }— returns without deleting DB record- Called from handleNewBooking.ts:
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true)during reschedule - scheduleEmailReminders.ts cleanup: only processes
scheduledDate <= now + 1 hour— reminders far in the future are never cleaned up - PR acceptance criteria states 'deleteScheduledEmailReminder with immediateDelete=true cancels via SendGrid but does NOT delete the DB record' confirming the design, but the orphaned record risk is unmitigated
Agent: security
|
|
||
| const workflowReminderToDelete = prisma.workflowReminder.delete({ | ||
| where: { | ||
| id: reminder.id, |
There was a problem hiding this comment.
🟠 MAJOR — Null Dereference / API Misuse — SendGrid Called with Null batch_id (confidence: 94%)
The cleanup query fetches all WorkflowReminder records where cancelled=true and scheduledDate <= now + 1 hour. This includes records where referenceId is null (reminders cancelled before SendGrid was ever called, e.g., when deleteScheduledEmailReminder is called and referenceId is null, the record is deleted immediately — but in other code paths a record with null referenceId could reach cancelled=true). The SendGrid API is then called with batch_id: null, which will result in an API error that causes the entire try block to abort, skipping deletion of all subsequent reminders in the batch.
Evidence:
- Query:
prisma.workflowReminder.findMany({ where: { cancelled: true, scheduledDate: { lte: ... } } })— no filter onreferenceId IS NOT NULL - SendGrid call:
body: { batch_id: reminder.referenceId, status: 'cancel' }— referenceId could be null - A single null referenceId in the batch causes the SendGrid call to fail, which throws and exits the entire for loop via the outer catch, leaving all remaining reminders unprocessed and undeleted
- The sequential
awaitinsidefor...ofmeans one failure aborts all subsequent iterations
Agent: security
| id: reminder.id, | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 MINOR — forEach with async callbacks — Unhandled Promise Rejections (confidence: 93%)
The remindersToUpdate.forEach(async (reminder) => { ... }) pattern does not await the async operations. deleteScheduledEmailReminder and deleteScheduledSMSReminder are async functions — using them inside forEach with async means their returned promises are discarded. Errors will be swallowed and the caller will not wait for these operations to complete before proceeding.
Evidence:
remindersToUpdate.forEach(async (reminder) => {— async callback in forEach- Both delete functions are async and return promises
- No
await Promise.all(remindersToUpdate.map(...))pattern used - This was present in the original code too but is worth flagging as a reliability issue
Agent: security
| }); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (4 lines, 1 file))
The
immediateDeleteparameter name is misleading and its behavior is inverted from what the name implies. WhenimmediateDelete=true, the function cancels the SendGrid batch immediately but does NOT delete the DB record (returns early). WhenimmediateDelete=false/undefined, it does NOT cancel SendGrid but DOES mark the record as cancelled in the DB. The caller inhandleNewBooking.tspassesimmediateDelete=truefor reschedule flows, expecting the record to be cleaned up by the scheduled job — but reminders more than 1 hour out will never be cleaned up by that job.
| // Fall through to mark DB record as cancelled so the scheduled cleanup | |
| // job in scheduleEmailReminders.ts can eventually purge it, regardless | |
| // of how far in the future the reminder is scheduled. |
🤖 Grapple PR auto-fix • major • confidence: 100%
| id: reminder.id, | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (2 lines, 1 file))
The
remindersToUpdate.forEach(async (reminder) => { ... })pattern does not await the async operations.deleteScheduledEmailReminderanddeleteScheduledSMSReminderare async functions — using them insideforEachwithasyncmeans their returned promises are discarded. Errors will be swallowed and the caller will not wait for these operations to complete before proceeding.
| }); | |
| remindersToUpdate.forEach((reminder) => { |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.log(`Error canceling reminder with error ${error}`); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (13 lines, 1 file) — review recommended)
When
deleteScheduledEmailReminderis called WITHOUTimmediateDelete=true(e.g., from handleCancelBooking.ts and bookings.tsx), it only setscancelled=truein the DB and never calls the SendGrid batch cancel API. The cleanup job in scheduleEmailReminders.ts only processes reminders wherescheduledDate <= now + 1 hour. Any reminder cancelled more than 1 hour before its scheduled send time will remain markedcancelled=truein the DB but will NOT have its SendGrid batch cancelled — meaning the email WILL be delivered to the attendee of a cancelled or rescheduled booking.
--- a/packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
+++ b/packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
@@ -213,10 +213,16 @@ export const deleteScheduledEmailReminder = async (
if (immediateDelete) {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: referenceId,
status: "cancel",
},
});
return;
}
+ // Cancel the SendGrid batch immediately to prevent email delivery,
+ // even when the reminder is due more than 1 hour from now.
+ // The cleanup job in scheduleEmailReminders.ts only processes reminders
+ // within 1 hour, so we must cancel the SendGrid batch here explicitly.
+ await client.request({
+ url: "/v3/user/scheduled_sends",
+ method: "POST",
+ body: {
+ batch_id: referenceId,
+ status: "cancel",
+ },
+ });
+
await prisma.workflowReminder.update({
where: {
id: reminderId,
},
data: {
cancelled: true,
},
});
} catch (error) {
console.log(`Error canceling reminder with error ${error}`);
}
};🤖 Grapple PR auto-fix • critical • Review this diff before applying
| } catch (error) { | ||
| console.log(`Error cancelling scheduled Emails: ${error}`); | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (16 lines, 1 file) — review recommended)
The cleanup loop uses
awaitinside afor...ofloop with a single outer try/catch. If the SendGrid API call fails for any reminder (network error, invalid batch_id, rate limit), the exception propagates to the outer catch, which logs and exits. All subsequent reminders inremindersToCancelare never processed, andworkflowRemindersToDelete(which only contains promises for reminders processed before the failure) is never awaited — those DB deletions are also lost.
--- a/packages/features/ee/workflows/api/scheduleEmailReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleEmailReminders.ts
@@ -52,24 +52,30 @@
- try {
- const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];
-
- for (const reminder of remindersToCancel) {
+ for (const reminder of remindersToCancel) {
+ try {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: reminder.referenceId,
status: "cancel",
},
});
-
- const workflowReminderToDelete = prisma.workflowReminder.delete({
+ await prisma.workflowReminder.delete({
where: {
id: reminder.id,
},
});
-
- workflowRemindersToDelete.push(workflowReminderToDelete);
+ } catch (error) {
+ console.log(`Error cancelling scheduled Email reminder with id ${reminder.id}: ${error}`);
}
- await Promise.all(workflowRemindersToDelete);
- } catch (error) {
- console.log(`Error cancelling scheduled Emails: ${error}`);
}🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| const workflowReminderToDelete = prisma.workflowReminder.delete({ | ||
| where: { | ||
| id: reminder.id, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (65 lines, 1 file) — review recommended)
The cleanup query fetches all
WorkflowReminderrecords wherecancelled=trueandscheduledDate <= now + 1 hour. This includes records wherereferenceIdis null (reminders cancelled before SendGrid was ever called, e.g., whendeleteScheduledEmailReminderis called andreferenceIdis null, the record is deleted immediately — but in other code paths a record with null referenceId could reach cancelled=true). The SendGrid API is then called withbatch_id: null, which will result in an API error that causes the entire try block to abort, skipping deletion of all subsequent reminders in the batch.
--- a/packages/features/ee/workflows/api/scheduleEmailReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleEmailReminders.ts
@@ -40,22 +40,43 @@ 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(),
},
+ // Only fetch reminders that were actually scheduled with SendGrid;
+ // records cancelled before a batch_id was assigned have null referenceId
+ // and must not be passed to the SendGrid cancel API.
+ referenceId: {
+ not: null,
+ },
},
});
+ // Reminders that were marked cancelled before SendGrid was ever called
+ // (no batch_id assigned) can be deleted directly without a SendGrid API call.
+ const remindersToDeleteWithoutCancel = await prisma.workflowReminder.findMany({
+ where: {
+ cancelled: true,
+ scheduledDate: {
+ lte: dayjs().add(1, "hour").toISOString(),
+ },
+ referenceId: null,
+ },
+ });
+
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);
+ try {
+ 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);
+ } catch (error) {
+ console.log(`Error cancelling scheduled Email with referenceId ${reminder.referenceId}: ${error}`);
+ }
}
+
+ // Delete reminders that never had a SendGrid batch_id without calling the API
+ for (const reminder of remindersToDeleteWithoutCancel) {
+ 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}`);
}🤖 Grapple PR auto-fix • major • Review this diff before applying
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR refactors workflow reminder cancellation for booking lifecycle events (cancellation, rescheduling, workflow deletion) by introducing a soft-delete 'cancelled' flag on WorkflowReminder records instead of immediately deleting them, and updates deletion signatures to accept reminderId alongside referenceId to handle cases where reminders haven't been scheduled yet.
Changes: 10 files (+241 / -203 lines)
- 🟢 1 file added:
packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql - 🟡 9 files modified:
packages/features/bookings/lib/handleCancelBooking.ts,packages/features/bookings/lib/handleNewBooking.ts,packages/features/ee/workflows/api/scheduleEmailReminders.ts,packages/features/ee/workflows/components/WorkflowStepContainer.tsx,packages/features/ee/workflows/lib/reminders/emailReminderManager.ts+4 more
Resolves: #6 (Comprehensive workflow reminder management for booking lifecycle events)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 10 files"/]
f1["🟡 handleCancelBooking.ts +6/-19"]
pr_0 --> f1
f2["🟡 handleNewBooking.ts +26/-2"]
pr_0 --> f2
f3["🟡 scheduleEmailReminders.ts +37/-0"]
pr_0 --> f3
f4["🟡 WorkflowStepContainer.tsx +60/-66"]
pr_0 --> f4
f5["🟡 emailReminderManager.ts +33/-12"]
pr_0 --> f5
f6["🟡 smsReminderManager.ts +9/-2"]
pr_0 --> f6
f7["🟢 migration.sql +2/-0"]
pr_0 --> f7
f8["🟡 schema.prisma +1/-0"]
pr_0 --> f8
f9["🟡 bookings.tsx +6/-19"]
pr_0 --> f9
f10["🟡 workflows.tsx +61/-83"]
pr_0 --> f10
f1 -.- f2
f5 -.- f6
f9 -.- f10
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
4 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 4 |
| 🟠 Major | 8 |
| 🟡 Minor | 13 |
| 🔵 Info | 2 |
Review details
- Reviewed at: 2026-04-12T02:08:34.986Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 113.8s
- Low-confidence findings suppressed: 9
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| await prisma.workflowReminder.update({ | ||
| where: { | ||
| id: reminderId, | ||
| }, |
There was a problem hiding this comment.
🔴 CRITICAL — Data integrity / Orphaned records (confidence: 100%)
When immediateDelete=true, the function cancels the SendGrid scheduled send but does NOT delete or update the corresponding WorkflowReminder database record. This leaves orphaned rows in the database. This path is used during rescheduling (handleNewBooking.ts), workflow deletion (workflows.tsx), and step deletion (workflows.tsx). For workflow/step deletion, the Prisma cascade on workflowStep may clean up some records, but for rescheduling the records will persist indefinitely with no mechanism to clean them up. The cron job only processes records with cancelled=true, but this path sets neither cancelled=true nor deletes the record.
Evidence:
- immediateDelete path at lines 218-228 calls SendGrid cancel API then returns without any DB operation
- handleNewBooking.ts line 968 calls deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) for rescheduled bookings
- workflows.tsx line 211 calls deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) for workflow deletion
- workflows.tsx line 521 calls deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) for step deletion
- The intent specification confirms: 'The immediateDelete=true path in deleteScheduledEmailReminder cancels the SendGrid batch but does NOT delete or update the DB record, leaving a stale record in the database — this appears to be a bug for the rescheduling case'
- The
immediateDeletebranch (lines 213-224) calls the SendGrid API and then returns without any Prisma delete or update - handleNewBooking.ts line 968 calls
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true)for rescheduled bookings - workflows.tsx line 213 calls
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true)when deleting a workflow - workflows.tsx line 522 calls
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true)when deleting a step - The cron job only processes records where
cancelled: true, but theimmediateDeletepath neither deletes nor setscancelled=true, so the cron will never clean these up either
Agent: logic
| }); | ||
| } catch (error) { | ||
| log.error("Error while canceling scheduled workflow reminders", error); | ||
| } |
There was a problem hiding this comment.
🔴 CRITICAL — Fire-and-forget async / Silent failures (confidence: 100%)
The try/catch block wraps a forEach that calls async functions (deleteScheduledEmailReminder, deleteScheduledSMSReminder) without awaiting them. Since forEach does not await async callbacks, all promises are fire-and-forget. The try/catch will never catch errors from these async operations — they will reject silently. Additionally, the rescheduling flow may proceed to create new reminders before old ones are cancelled, creating a race condition.
Evidence:
- forEach at line 965 calls async deleteScheduledEmailReminder and deleteScheduledSMSReminder without await
- The try/catch at lines 963/972 only catches synchronous errors, not promise rejections from the unawaited calls
- Intent specification edge case: 'handleNewBooking wraps the reminder cancellation in try/catch but the forEach inside is synchronous over async calls without await, so errors from individual promises won't be caught by the outer try/catch'
forEachwith async callbacks does not await — the callback returns a promise that is ignored- The surrounding try/catch on line 963/975 only catches synchronous errors, not rejected promises from the forEach body
- If deleteScheduledEmailReminder fails for a rescheduled booking, the old reminder may still fire while the new one is also scheduled
Agent: logic
| }, | ||
| }); | ||
| remindersToDelete.push(reminderToDelete); | ||
| }); |
There was a problem hiding this comment.
🔴 CRITICAL — Fire-and-Forget Async / Silent Failures (confidence: 100%)
The forEach loop calls async deleteScheduledEmailReminder and deleteScheduledSMSReminder without await. These are fire-and-forget: if any cancellation fails, the error is silently swallowed, and the calling function may complete (and return a success response to the user) before the deletions finish. Since the old Promise.all(remindersToDelete) was removed and not replaced with any awaited promise collection, there is no error handling or guarantee of completion.
Evidence:
- Lines 486-491: forEach calls async functions without await
- The old code collected PrismaPromise[] and awaited them at line 497 via Promise.all
- Intent specification: 'handleCancelBooking no longer awaits or collects promises from deleteScheduledEmailReminder/deleteScheduledSMSReminder calls inside forEach, meaning failures are silently swallowed'
- forEach does not await async function return values — the promises are discarded
- The old code collected promises into
remindersToDeleteand awaited them viaPromise.all; the new code has no equivalent - For the soft-delete (email) path, if the Prisma update to set
cancelled=truefails, the reminder will still fire on schedule with no indication of failure
Agent: architecture
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } |
There was a problem hiding this comment.
🔴 CRITICAL — Fire-and-Forget Async / Silent Failures (confidence: 100%)
Same fire-and-forget pattern: forEach calls async deletion functions without collecting or awaiting promises. The old code had await Promise.all(remindersToDelete) which has been removed. For the email soft-delete path, if the Prisma update fails, the reminder remains active and will fire.
Evidence:
- Lines 485-492: forEach calls async functions without await
- Old code had
await Promise.all(remindersToDelete)which was removed - Intent specification lists this as a risk area
- The removed
await Promise.all(remindersToDelete)on the old line was the mechanism that ensured completions were awaited - The new forEach on line 487 discards all returned promises
Agent: architecture
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR — Fire-and-forget async / Silent failures (confidence: 100%)
When deleting a workflow, reminder cancellation calls are fire-and-forget inside forEach. Since deleteScheduledEmailReminder with immediateDelete=true also doesn't update DB records (separate bug), and these calls aren't awaited, the workflow deletion could complete while SendGrid cancellation is still in-flight. If the cancellation fails, reminders will still be sent.
Evidence:
- Lines 211-217: forEach calls async functions without await
- This is the workflow deletion path where immediateDelete=true is used
- Line 211-218: forEach with async calls, no promise collection or await
- Line 220:
await ctx.prisma.workflow.deleteMany(...)will cascade-delete WorkflowStep and WorkflowReminder records, but SMS reminders may already be deleted by the fire-and-forget call, creating a race condition
Agent: logic
| data: { | ||
| cancelled: true, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🟡 MINOR — Documentation (confidence: 90%)
The immediateDelete parameter and its behavior are not documented. The function has three distinct code paths (no referenceId, immediateDelete=true, default) with different side effects, but there's no JSDoc explaining when and why to use each path.
Evidence:
- Function signature has optional immediateDelete parameter but no documentation
- Three distinct behaviors: (1) delete from DB if no referenceId, (2) cancel in SendGrid API without DB update if immediateDelete=true, (3) mark cancelled=true in DB
- The immediateDelete=true path cancels in SendGrid but does NOT update the database, which is semantically different from the other paths
Agent: style
|
|
||
| await Promise.all(deleteReminderPromise); | ||
|
|
||
| //update active on & reminders for new eventTypes |
There was a problem hiding this comment.
🟡 MINOR — Code organization (confidence: 87%)
Async function calls in forEach loop without await when disabling event types. The deleteScheduledEmailReminder and deleteScheduledSMSReminder calls are not awaited.
Evidence:
- deleteScheduledEmailReminder and deleteScheduledSMSReminder are async but called without await
- No Promise collection mechanism after the forEach
- Previously used PrismaPromise collection pattern but now using fire-and-forget calls
Agent: style
| id: reminderId, | ||
| }, | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
🟡 MINOR — Documentation (confidence: 83%)
Missing documentation on the function behavior change. The updated deleteScheduledSMSReminder always deletes the DB record, regardless of whether Twilio cancellation succeeds or fails, but this behavior isn't documented.
Evidence:
- Function always executes await prisma.workflowReminder.delete(...) even if twilio.cancelSMS throws
- The acceptance criteria state 'deleteScheduledSMSReminder always deletes the DB record regardless of whether Twilio cancellation is attempted' but this is not explained in comments
- No JSDoc explaining the signature change or new referenceId nullability handling
Agent: style
| sendVerificationCodeMutation.mutate({ | ||
| className="-ml-[3px] rounded-tl-none rounded-bl-none " | ||
| disabled={verifyPhoneNumberMutation.isLoading} | ||
| onClick={() => { |
There was a problem hiding this comment.
🔵 INFO — Code organization (confidence: 84%)
Code refactoring splits phone number and sender ID inputs into separate conditional blocks, but the closing brace structure and empty block at line 449 appears disconnected. The refactoring is valid but the block structure could be clearer.
Evidence:
- Changed from single
{(isPhoneNumberNeeded || isSenderIdNeeded) && (...)}to separate{isPhoneNumberNeeded && (...)}and presumably another block for isSenderIdNeeded - The diff shows the phone number input block but sender ID block is truncated - unclear if the refactoring is complete
- Line 449 shows closing `` without clear pairing
Agent: style
| @@ -0,0 +1,2 @@ | |||
| -- AlterTable | |||
| ALTER TABLE "WorkflowReminder" ADD COLUMN "cancelled" BOOLEAN; | |||
There was a problem hiding this comment.
🔵 INFO — Naming conventions (confidence: 81%)
Boolean field 'cancelled' is not explicitly set to a default value. Since the field is nullable (Boolean?) and existing rows won't be migrated to true/false, old records will have NULL. This could be intentional but the migration comment doesn't clarify intent.
Evidence:
- ALTER TABLE adds 'cancelled' BOOLEAN without DEFAULT clause
- Schema shows cancelled as Boolean? (nullable)
- Existing WorkflowReminder records will have cancelled=NULL, which is semantically different from cancelled=false
Agent: style
| await prisma.workflowReminder.update({ | ||
| where: { | ||
| id: reminderId, | ||
| }, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (5 lines, 1 file))
When
immediateDelete=true, the function cancels the SendGrid scheduled send but does NOT delete or update the corresponding WorkflowReminder database record. This leaves orphaned rows in the database. This path is used during rescheduling (handleNewBooking.ts), workflow deletion (workflows.tsx), and step deletion (workflows.tsx). For workflow/step deletion, the Prisma cascade onworkflowStepmay clean up some records, but for rescheduling the records will persist indefinitely with no mechanism to clean them up. The cron job only processes records withcancelled=true, but this path sets neithercancelled=truenor deletes the record.
| }, | |
| await prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminderId, | |
| }, | |
| }); |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| await prisma.workflowReminder.update({ | ||
| where: { | ||
| id: reminderId, | ||
| }, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (8 lines, 1 file))
The
immediateDelete=truepath cancels the SendGrid batch but returns without deleting or updating the WorkflowReminder DB record. This leaves a stale/orphaned row in the database indefinitely. The cron job won't pick it up (cancelled=true is never set) and it won't be deleted.
| }, | |
| // Delete the DB record so no orphaned/stale WorkflowReminder rows accumulate. | |
| // The cron job only processes rows where cancelled=false and scheduled<=now, | |
| // so a cancelled-but-not-deleted row would never be cleaned up otherwise. | |
| await prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminderId, | |
| }, | |
| }); |
🤖 Grapple PR auto-fix • major • confidence: 100%
| if (originalRescheduledBooking?.uid) { | ||
| try { | ||
| // cancel workflow reminders from previous rescheduled booking | ||
| originalRescheduledBooking.workflowReminders.forEach((reminder) => { |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
The code accesses
originalRescheduledBooking.workflowReminderswithout a null/undefined check. While the Prisma query at line 771 includesworkflowReminders: true, there are multiple code paths whereoriginalRescheduledBookingis set, and not all may include this relation. IfworkflowRemindersis undefined,.forEachwill throw a TypeError.
| originalRescheduledBooking.workflowReminders.forEach((reminder) => { | |
| originalRescheduledBooking.workflowReminders?.forEach((reminder) => { |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| @@ -0,0 +1,2 @@ | |||
| -- AlterTable | |||
| ALTER TABLE "WorkflowReminder" ADD COLUMN "cancelled" BOOLEAN; | |||
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — performance agent (Small fix (3 lines, 1 file))
The new
cancelledcolumn is added without an index, but the cron job queriesWHERE cancelled = true AND scheduledDate <= ?. Without an index oncancelled, this will perform a full table scan on WorkflowReminder every time the cron runs.
| ALTER TABLE "WorkflowReminder" ADD COLUMN "cancelled" BOOLEAN; | |
| -- CreateIndex | |
| CREATE INDEX "WorkflowReminder_cancelled_scheduledDate_idx" ON "WorkflowReminder"("cancelled", "scheduledDate"); |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| scheduled Boolean | ||
| workflowStepId Int | ||
| workflowStep WorkflowStep @relation(fields: [workflowStepId], references: [id], onDelete: Cascade) | ||
| cancelled Boolean? |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (2 lines, 1 file))
The
cancelledfield isBoolean?(nullable) with no default value. This means all existing WorkflowReminder records will havecancelled = null. While the cron querycancelled: truecorrectly excludes null values, other code paths that checkcancelledmay need to handle the three-state logic (true/false/null). Consider using a non-nullable Boolean with a default offalsefor clearer semantics.
| cancelled Boolean? | |
| cancelled Boolean @default(false) |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| }); | ||
| } catch (error) { | ||
| log.error("Error while canceling scheduled workflow reminders", error); | ||
| } |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (30 lines, 1 file) — review recommended)
The try/catch block wraps a forEach that calls async functions (deleteScheduledEmailReminder, deleteScheduledSMSReminder) without awaiting them. Since forEach does not await async callbacks, all promises are fire-and-forget. The try/catch will never catch errors from these async operations — they will reject silently. Additionally, the rescheduling flow may proceed to create new reminders before old ones are cancelled, creating a race condition.
--- a/packages/features/bookings/lib/handleNewBooking.ts
+++ b/packages/features/bookings/lib/handleNewBooking.ts
@@ -963,15 +963,22 @@ 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);
- } else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
- }
- });
- } catch (error) {
- log.error("Error while canceling scheduled workflow reminders", error);
- }
+ // cancel workflow reminders from previous rescheduled booking
+ // Await all cancellations before proceeding to reschedule to prevent a race condition
+ // where old reminders fire after new ones have already been scheduled.
+ // Each promise is individually caught so one failure doesn't abort the others.
+ await Promise.all(
+ originalRescheduledBooking.workflowReminders.map((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true).catch((error) => {
+ log.error("Error while canceling scheduled email workflow reminder", { reminderId: reminder.id, error });
+ });
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId).catch((error) => {
+ log.error("Error while canceling scheduled SMS workflow reminder", { reminderId: reminder.id, error });
+ });
+ }
+ return Promise.resolve();
+ })
+ );
// Use EventManager to conditionally use all needed integrations.
const updateManager = await eventManager.reschedule(🤖 Grapple PR auto-fix • critical • Review this diff before applying
| }, | ||
| }); | ||
| remindersToDelete.push(reminderToDelete); | ||
| }); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (15 lines, 1 file) — review recommended)
The
forEachloop calls asyncdeleteScheduledEmailReminderanddeleteScheduledSMSReminderwithoutawait. These are fire-and-forget: if any cancellation fails, the error is silently swallowed, and the calling function may complete (and return a success response to the user) before the deletions finish. Since the oldPromise.all(remindersToDelete)was removed and not replaced with any awaited promise collection, there is no error handling or guarantee of completion.
--- a/packages/features/bookings/lib/handleCancelBooking.ts
+++ b/packages/features/bookings/lib/handleCancelBooking.ts
@@ -481,14 +481,19 @@ async function handler(req: NextApiRequest & { userId?: number }) {
cancelScheduledJobs(booking);
});
- //Workflows - cancel all reminders for cancelled bookings
- updatedBookings.forEach((booking) => {
- booking.workflowReminders.forEach((reminder) => {
+ //Workflows - cancel all reminders for cancelled bookings, collecting promises to await
+ const reminderDeletionPromises: Promise<unknown>[] = [];
+ updatedBookings.forEach((booking) => {
+ booking.workflowReminders.forEach((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ reminderDeletionPromises.push(deleteScheduledEmailReminder(reminder.id, reminder.referenceId));
} else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ reminderDeletionPromises.push(deleteScheduledSMSReminder(reminder.id, reminder.referenceId));
}
});
});
- const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes];
+ const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes];
- await Promise.all(prismaPromises.concat(apiDeletes));
+ await Promise.all(prismaPromises.concat(apiDeletes).concat(reminderDeletionPromises));
await sendCancelledEmails(evt);🤖 Grapple PR auto-fix • critical • Review this diff before applying
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (13 lines, 1 file) — review recommended)
Same fire-and-forget pattern:
forEachcalls async deletion functions without collecting or awaiting promises. The old code hadawait Promise.all(remindersToDelete)which has been removed. For the email soft-delete path, if the Prisma update fails, the reminder remains active and will fire.
--- a/packages/trpc/server/routers/viewer/bookings.tsx
+++ b/packages/trpc/server/routers/viewer/bookings.tsx
@@ -484,12 +484,15 @@ export const bookingsRouter = router({
// delete scheduled jobs of previous booking
cancelScheduledJobs(bookingToReschedule);
- //cancel workflow reminders of previous booking
- bookingToReschedule.workflowReminders.forEach((reminder) => {
+ // cancel workflow reminders of previous booking
+ // NOTE: These async functions perform Prisma soft-deletes internally; we must
+ // await all of them to surface errors and ensure reminders are marked cancelled
+ // before the rescheduled booking is created. Silently dropped promises would
+ // leave reminders active and cause duplicate notifications.
+ const reminderCancellations = bookingToReschedule.workflowReminders.map((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
} else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
});
+ await Promise.all(reminderCancellations);
const [mainAttendee] = bookingToReschedule.attendees;🤖 Grapple PR auto-fix • critical • Review this diff before applying
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (17 lines, 1 file) — review recommended)
When deleting a workflow, reminder cancellation calls are fire-and-forget inside forEach. Since deleteScheduledEmailReminder with immediateDelete=true also doesn't update DB records (separate bug), and these calls aren't awaited, the workflow deletion could complete while SendGrid cancellation is still in-flight. If the cancellation fails, reminders will still be sent.
--- a/packages/trpc/server/routers/viewer/workflows.tsx
+++ b/packages/trpc/server/routers/viewer/workflows.tsx
@@ -209,12 +209,15 @@
});
//cancel workflow reminders of deleted workflow
- scheduledReminders.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(
+ scheduledReminders.map((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ return Promise.resolve();
+ })
+ );
await ctx.prisma.workflow.deleteMany({
where: {🤖 Grapple PR auto-fix • major • Review this diff before applying
| } | ||
| await Promise.all(workflowRemindersToDelete); | ||
| } catch (error) { | ||
| console.log(`Error cancelling scheduled Emails: ${error}`); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (52 lines, 1 file) — review recommended)
In the cron job, if the SendGrid cancel API call fails for any reminder during the for-loop, the catch block is triggered and ALL already-collected workflowRemindersToDelete are skipped. This means reminders that were successfully cancelled via SendGrid will not be deleted from the DB and will be re-processed on the next cron run, causing duplicate cancel requests to SendGrid. The catch is too broad — it should handle errors per-reminder.
--- a/packages/features/ee/workflows/api/scheduleEmailReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleEmailReminders.ts
@@ -53,22 +53,26 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
- 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",
+ },
+ });
+
+ // Only delete from DB after SendGrid successfully confirms cancellation.
+ // If we delete before confirmation, a SendGrid failure would leave a
+ // cancelled-but-still-scheduled email with no DB record to retry.
+ await prisma.workflowReminder.delete({
+ where: {
+ id: reminder.id,
+ },
+ });
+ } catch (error) {
+ // Per-reminder error isolation: a failure for one reminder must not
+ // prevent already-cancelled reminders from being cleaned up, nor block
+ // subsequent reminders from being processed. The reminder stays in DB
+ // and will be retried on the next cron execution.
+ console.log(`Error cancelling scheduled Email reminder ${reminder.id}: ${error}`);
+ }
+ }🤖 Grapple PR auto-fix • major • Review this diff before applying
| body: { | ||
| batch_id: reminder.referenceId, | ||
| status: "cancel", | ||
| }, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (14 lines, 1 file) — review recommended)
The cron job queries for reminders with
cancelled: truebut does not filter byreferenceId IS NOT NULL. If a record somehow hascancelled=truebutreferenceId=null, the SendGrid API call will be made withbatch_id: null, which will likely cause an API error and break the entire batch processing due to the broad try/catch.
--- a/packages/features/ee/workflows/api/scheduleEmailReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleEmailReminders.ts
@@ -43,9 +43,16 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
const remindersToCancel = await prisma.workflowReminder.findMany({
where: {
cancelled: true,
+ referenceId: { not: null },
scheduledDate: {
lte: dayjs().add(1, "hour").toISOString(),
},
},
});
+ // Clean up cancelled reminders that were never scheduled with SendGrid (referenceId=null).
+ // These have no SendGrid batch to cancel — just remove them from the DB directly.
+ await prisma.workflowReminder.deleteMany({
+ where: {
+ cancelled: true,
+ referenceId: null,
+ scheduledDate: {
+ lte: dayjs().add(1, "hour").toISOString(),
+ },
+ },
+ });
+
try {
const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];
for (const reminder of remindersToCancel) {
+ // reminder.referenceId is guaranteed non-null here due to the query filter above
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: reminder.referenceId,
status: "cancel",
},
});🤖 Grapple PR auto-fix • major • Review this diff before applying
| } | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (16 lines, 1 file) — review recommended)
The
remindersToUpdate.forEach(async (reminder) => { ... })pattern creates unawaited promises. Theasynckeyword on the forEach callback returns a promise that forEach ignores. This means the deleteScheduledEmailReminder/deleteScheduledSMSReminder calls for edited workflow steps are fire-and-forget.
--- a/packages/trpc/server/routers/viewer/workflows.tsx
+++ b/packages/trpc/server/routers/viewer/workflows.tsx
@@ -569,11 +569,11 @@ export const workflowsRouter = router({
//cancel all workflow reminders from steps that were edited
- remindersToUpdate.forEach(async (reminder) => {
- if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
- } else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
- }
- });
+ await Promise.all(
+ remindersToUpdate.map((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ })
+ );🤖 Grapple PR auto-fix • major • Review this diff before applying
| scheduledDate: { | ||
| lte: dayjs().add(1, "hour").toISOString(), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (14 lines, 1 file) — review recommended)
The cron job only cancels reminders with
scheduledDate <= now + 1 hour. A reminder markedcancelled=truebut scheduled more than 1 hour in the future will NOT be cancelled in this run. If the cron runs infrequently (or the SendGrid scheduled send triggers before the next cron run that covers that window), the email will be delivered despite the cancellation. This creates a delivery window where cancelled reminders can still fire.
--- a/packages/features/ee/workflows/api/scheduleEmailReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleEmailReminders.ts
@@ -43,10 +43,13 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
- //cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour
+ //cancel ALL reminders for cancelled/rescheduled bookings that are scheduled in the future
+ // NOTE: We do NOT apply an upper-bound on scheduledDate here. Any reminder flagged
+ // cancelled=true must be submitted to SendGrid immediately regardless of how far out
+ // it is scheduled, because SendGrid will fire the scheduled send at its scheduled time
+ // independently of this cron — waiting for the reminder to fall within the 1-hour window
+ // creates a delivery race condition if the cron runs infrequently.
const remindersToCancel = await prisma.workflowReminder.findMany({
where: {
cancelled: true,
- scheduledDate: {
- lte: dayjs().add(1, "hour").toISOString(),
- },
+ // Only target future reminders; past ones are already purged by the deleteMany above
+ scheduledDate: {
+ gte: dayjs().toISOString(),
+ },
},
});🤖 Grapple PR auto-fix • major • Review this diff before applying
| workflowRemindersToDelete.push(workflowReminderToDelete); | ||
| } | ||
| await Promise.all(workflowRemindersToDelete); | ||
| } catch (error) { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (43 lines, 1 file) — review recommended)
Sequential awaited SendGrid API calls inside a for loop, followed by batched DB deletes. Each SendGrid cancellation is awaited one at a time, making this O(n) serial HTTP requests. For large numbers of cancelled reminders this will be very slow and may hit rate limits.
--- a/packages/features/ee/workflows/api/scheduleEmailReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleEmailReminders.ts
@@ -53,23 +53,26 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
try {
const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];
- for (const reminder of remindersToCancel) {
- await client.request({
+ // Fire all SendGrid cancellation requests concurrently instead of serially
+ // to avoid O(n) sequential HTTP calls which are slow and can hit rate limits.
+ // Note: SendGrid has no true batch-cancel endpoint per batch_id, so we must
+ // make N calls — but we can make them in parallel.
+ await Promise.all(
+ remindersToCancel.map((reminder) =>
+ client.request({
+ url: "/v3/user/scheduled_sends",
+ method: "POST",
+ body: {
+ batch_id: reminder.referenceId,
+ status: "cancel",
+ },
+ })
+ )
+ );
+
+ for (const reminder of remindersToCancel) {
+ workflowRemindersToDelete.push(
+ prisma.workflowReminder.delete({
+ where: {
+ id: reminder.id,
+ },
+ })
+ );
- 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}`);🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| const workflowReminderToDelete = prisma.workflowReminder.delete({ | ||
| where: { | ||
| id: reminder.id, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (23 lines, 1 file) — review recommended)
The cancellation loop calls the SendGrid batch cancel API even for reminders where
referenceIdis null. A null referenceId means the reminder was never scheduled with SendGrid, so the API call will fail or be a no-op, wasting a network round-trip per unscheduled reminder.
--- a/packages/features/ee/workflows/api/scheduleEmailReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleEmailReminders.ts
@@ -53,16 +53,21 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
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",
- },
- });
-
+ // Only call SendGrid if the reminder was actually scheduled (has a referenceId).
+ // A null referenceId means the reminder was cancelled before being sent to SendGrid,
+ // so there is nothing to cancel on their end — but we still need to delete the DB record.
+ if (reminder.referenceId) {
+ 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}`);
}🤖 Grapple PR auto-fix • major • Review this diff before applying
| ); | ||
| const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes]; | ||
|
|
||
| await Promise.all(prismaPromises.concat(apiDeletes)); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (25 lines, 1 file) — review recommended)
Async function calls in forEach loop without await - fire-and-forget behavior where errors are silently swallowed. The deleteScheduledEmailReminder and deleteScheduledSMSReminder calls are not awaited, so failures won't be surfaced and the booking cancellation may complete before reminder deletions finish.
--- a/packages/features/bookings/lib/handleCancelBooking.ts
+++ b/packages/features/bookings/lib/handleCancelBooking.ts
@@ -481,13 +481,18 @@ async function handler(req: NextApiRequest & { userId?: number }) {
cancelScheduledJobs(booking);
});
- //Workflows - cancel all reminders for cancelled bookings
- updatedBookings.forEach((booking) => {
- booking.workflowReminders.forEach((reminder) => {
- if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
- } else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
- }
- });
- });
+ //Workflows - cancel all reminders for cancelled bookings
+ const reminderDeletions: Promise<unknown>[] = [];
+ updatedBookings.forEach((booking) => {
+ booking.workflowReminders.forEach((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ reminderDeletions.push(deleteScheduledEmailReminder(reminder.id, reminder.referenceId));
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ reminderDeletions.push(deleteScheduledSMSReminder(reminder.id, reminder.referenceId));
+ }
+ });
+ });
- const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes];
+ const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes];
- await Promise.all(prismaPromises.concat(apiDeletes));
+ await Promise.all(prismaPromises.concat(apiDeletes).concat(reminderDeletions));🤖 Grapple PR auto-fix • minor • Review this diff before applying
| }); | ||
| remindersToDelete.push(reminderToDelete); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (17 lines, 1 file) — review recommended)
Async function calls in forEach loop without await - fire-and-forget behavior where errors are silently swallowed. The deleteScheduledEmailReminder and deleteScheduledSMSReminder calls are not awaited.
--- a/packages/trpc/server/routers/viewer/bookings.tsx
+++ b/packages/trpc/server/routers/viewer/bookings.tsx
@@ -484,11 +484,14 @@ export const bookingsRouter = router({
// delete scheduled jobs of previous booking
cancelScheduledJobs(bookingToReschedule);
//cancel workflow reminders of previous booking
- bookingToReschedule.workflowReminders.forEach((reminder) => {
- if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
- } else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
- }
- });
+ await Promise.all(
+ bookingToReschedule.workflowReminders.map((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ return Promise.resolve();
+ })
+ );
const [mainAttendee] = bookingToReschedule.attendees;🤖 Grapple PR auto-fix • minor • Review this diff before applying
| } | ||
| await Promise.all(workflowRemindersToDelete); | ||
| } catch (error) { | ||
| console.log(`Error cancelling scheduled Emails: ${error}`); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (45 lines, 1 file) — review recommended)
Sequential for loop with await inside it, then batch delete - error handling doesn't clean up already-processed reminders. If SendGrid cancellation fails partway through the loop, successfully-processed reminders won't be deleted from the database because the catch block prevents the workflowRemindersToDelete.push operations.
--- a/packages/features/ee/workflows/api/scheduleEmailReminders.ts
+++ b/packages/features/ee/workflows/api/scheduleEmailReminders.ts
@@ -53,24 +53,27 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
- 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.log(`Error cancelling scheduled Email reminder ${reminder.id}: ${error}`);
+ }
+ }🤖 Grapple PR auto-fix • minor • Review this diff before applying
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.log(`Error canceling reminder with error ${error}`); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — performance agent (Larger fix (37 lines, 1 file) — review recommended)
Each call to
deleteScheduledSMSReminderissues an individualprisma.workflowReminder.deleteper reminder. When called from aforEachloop (e.g. in handleCancelBooking, bookings.tsx, workflows.tsx), this produces one DELETE query per reminder rather than a single batched delete.
--- a/packages/features/ee/workflows/lib/reminders/smsReminderManager.ts
+++ b/packages/features/ee/workflows/lib/reminders/smsReminderManager.ts
@@ -177,6 +177,28 @@ export const scheduleSMSReminder = async (
export const deleteScheduledSMSReminder = async (reminderId: number, referenceId: string | null) => {
try {
if (referenceId) {
await twilio.cancelSMS(referenceId);
}
await prisma.workflowReminder.delete({
where: {
id: reminderId,
},
});
} catch (error) {
console.log(`Error canceling reminder with error ${error}`);
}
};
+
+/**
+ * Batch version of deleteScheduledSMSReminder.
+ * Cancels all provided SMS reminders with a single DB DELETE (deleteMany)
+ * instead of one DELETE per reminder, avoiding the N+1 query problem when
+ * called from forEach loops in handleCancelBooking, bookings.tsx, workflows.tsx.
+ *
+ * Edge case: if any individual Twilio cancellation fails the error is caught
+ * per-reminder so remaining reminders still get cleaned up.
+ */
+export const deleteScheduledSMSReminders = async (
+ reminders: { reminderId: number; referenceId: string | null }[]
+) => {
+ if (!reminders.length) return;
+
+ // Cancel Twilio messages concurrently — failures are isolated per reminder
+ await Promise.allSettled(
+ reminders
+ .filter((r) => r.referenceId !== null)
+ .map((r) =>
+ twilio.cancelSMS(r.referenceId as string).catch((error) => {
+ console.log(`Error canceling Twilio SMS for reminder ${r.reminderId}: ${error}`);
+ })
+ )
+ );
+
+ // Single batched DELETE instead of N individual deletes
+ try {
+ await prisma.workflowReminder.deleteMany({
+ where: {
+ id: { in: reminders.map((r) => r.reminderId) },
+ },
+ });
+ } catch (error) {
+ console.log(`Error bulk-deleting SMS reminders with error ${error}`);
+ }
+};🤖 Grapple PR auto-fix • minor • Review this diff before applying
| }, | ||
| }); | ||
| }); | ||
| const eventTypesToUpdateReminders = activeOn.filter((eventTypeId) => { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (16 lines, 1 file) — review recommended)
Async function calls in forEach loop with async keyword but without await. The forEach callback is declared async but promises from deleteScheduledEmailReminder and deleteScheduledSMSReminder are not awaited, and the forEach itself is not awaited.
--- a/packages/trpc/server/routers/viewer/workflows.tsx
+++ b/packages/trpc/server/routers/viewer/workflows.tsx
@@ -572,10 +572,12 @@
//cancel all workflow reminders from steps that were edited
- remindersToUpdate.forEach(async (reminder) => {
- if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
- } else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
- }
- });
+ await Promise.all(
+ remindersToUpdate.map((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ })
+ );🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
|
||
| await Promise.all(deleteReminderPromise); | ||
|
|
||
| //update active on & reminders for new eventTypes |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (18 lines, 1 file) — review recommended)
Async function calls in forEach loop without await when disabling event types. The deleteScheduledEmailReminder and deleteScheduledSMSReminder calls are not awaited.
--- a/packages/trpc/server/routers/viewer/workflows.tsx
+++ b/packages/trpc/server/routers/viewer/workflows.tsx
@@ -373,10 +373,13 @@ export const workflowsRouter = router({
const remindersToDelete = await Promise.all(remindersToDeletePromise);
- //cancel workflow reminders for all bookings from event types that got disabled
- remindersToDelete.flat().forEach((reminder) => {
- if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
- } else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
- }
- });
+ //cancel workflow reminders for all bookings from event types that got disabled
+ await Promise.all(
+ remindersToDelete.flat().map((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ })
+ );
//update active on & reminders for new eventTypes🤖 Grapple PR auto-fix • minor • Review this diff before applying
| id: reminderId, | ||
| }, | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (14 lines, 1 file) — review recommended)
Missing documentation on the function behavior change. The updated deleteScheduledSMSReminder always deletes the DB record, regardless of whether Twilio cancellation succeeds or fails, but this behavior isn't documented.
--- a/packages/features/ee/workflows/lib/reminders/smsReminderManager.ts
+++ b/packages/features/ee/workflows/lib/reminders/smsReminderManager.ts
@@ -174,7 +174,20 @@ export const scheduleSMSReminder = async (
}
};
+/**
+ * Deletes a scheduled SMS reminder, cancelling it with Twilio if applicable.
+ *
+ * @param reminderId - The database ID of the WorkflowReminder record to delete.
+ * @param referenceId - The Twilio message/schedule SID used to cancel the SMS.
+ * Pass `null` if the reminder was never sent to Twilio (e.g. it was created
+ * but not yet scheduled), in which case Twilio cancellation is skipped.
+ *
+ * @remarks
+ * The DB record is **always** deleted regardless of whether Twilio cancellation
+ * succeeds, fails, or is skipped. If Twilio cancellation throws, the error is
+ * logged and the delete proceeds, ensuring no orphaned records remain in the
+ * database even when the Twilio API is unavailable.
+ */
export const deleteScheduledSMSReminder = async (reminderId: number, referenceId: string | null) => {
try {
if (referenceId) {
await twilio.cancelSMS(referenceId);
}
await prisma.workflowReminder.delete({
where: {
id: reminderId,
},
});
} catch (error) {
console.log(`Error canceling reminder with error ${error}`);
}
};🤖 Grapple PR auto-fix • minor • Review this diff before applying
2 participants
Benchmark test PR #6 - Replicating Greptile benchmark for Grapple PR evaluation.