Comprehensive workflow reminder management for booking lifecycle events #6
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,6 +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 { bookingMetadataSchema } from "@calcom/prisma/zod-utils"; | ||||||
|
|
||||||
| import customTemplate, { VariablesType } from "../lib/reminders/templates/customTemplate"; | ||||||
|
|
@@ -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(), | ||||||
| }, | ||||||
| }, | ||||||
|
There was a problem hiding this comment. 🟠 MAJOR — Race Condition / Delivery Window (confidence: 100%) The cron job only cancels reminders with Evidence:
Agent: architecture There was a problem hiding this comment. 🟡
--- 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 |
||||||
| }); | ||||||
|
|
||||||
| 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", | ||||||
| }, | ||||||
|
There was a problem hiding this comment. 🟠 MAJOR — Null safety / Potential SendGrid API error (confidence: 100%) The cron job queries for reminders with Evidence:
Agent: logic There was a problem hiding this comment. 🟡
--- 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 |
||||||
| }); | ||||||
|
|
||||||
| 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 Evidence:
Agent: security |
||||||
| 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 Evidence:
Agent: security There was a problem hiding this comment. 🟡
--- 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 There was a problem hiding this comment. 🟠 MAJOR — Unnecessary work (confidence: 100%) The cancellation loop calls the SendGrid batch cancel API even for reminders where Evidence:
Agent: performance There was a problem hiding this comment. 🟡
--- 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 |
||||||
| }, | ||||||
| }); | ||||||
|
|
||||||
| 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:
Agent: security There was a problem hiding this comment. ✅
Suggested change
🤖 Grapple PR auto-fix • minor • confidence: 91% |
||||||
| } catch (error) { | ||||||
|
There was a problem hiding this comment. 🟠 MAJOR — N+1 queries (confidence: 100%) 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. Evidence:
Agent: performance There was a problem hiding this comment. 🟡
--- 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 |
||||||
| console.log(`Error cancelling scheduled Emails: ${error}`); | ||||||
|
There was a problem hiding this comment. 🟠 MAJOR — Error handling / Partial failure (confidence: 100%) 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. Evidence:
Agent: logic There was a problem hiding this comment. 🟡 MINOR — Code organization (confidence: 100%) 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. Evidence:
Agent: style There was a problem hiding this comment. 🟡
--- 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 There was a problem hiding this comment. 🟡
--- 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 |
||||||
| } | ||||||
|
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:
Agent: style 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:
Agent: style There was a problem hiding this comment. 🟡
--- 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. 🟠 MAJOR — Error Handling — Sequential Loop Aborts on First Failure (confidence: 100%) The cleanup loop uses Evidence:
Agent: security There was a problem hiding this comment. 🟡
--- 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 |
||||||
| //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 Evidence:
Agent: security There was a problem hiding this comment. 🟡
--- 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 |
||||||
| where: { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -387,81 +387,75 @@ export default function WorkflowStepContainer(props: WorkflowStepProps) { | |
| }} | ||
| /> | ||
| </div> | ||
| {isPhoneNumberNeeded && ( | ||
| <div className="mt-2 rounded-md bg-gray-50 p-4 pt-0"> | ||
| <Label className="pt-4">{t("custom_phone_number")}</Label> | ||
| <div className="block sm:flex"> | ||
| <PhoneInput<FormValues> | ||
| control={form.control} | ||
| name={`steps.${step.stepNumber - 1}.sendTo`} | ||
| placeholder={t("phone_number")} | ||
| id={`steps.${step.stepNumber - 1}.sendTo`} | ||
| className="min-w-fit sm:rounded-tl-md sm:rounded-bl-md sm:border-r-transparent" | ||
| required | ||
| onChange={() => { | ||
| const isAlreadyVerified = !!verifiedNumbers | ||
| ?.concat([]) | ||
| .find((number) => number === form.getValues(`steps.${step.stepNumber - 1}.sendTo`)); | ||
| setNumberVerified(isAlreadyVerified); | ||
| }} | ||
| /> | ||
| <Button | ||
| color="secondary" | ||
| disabled={numberVerified || false} | ||
| className={classNames( | ||
| "-ml-[3px] h-[40px] min-w-fit sm:block sm:rounded-tl-none sm:rounded-bl-none ", | ||
| numberVerified ? "hidden" : "mt-3 sm:mt-0" | ||
| )} | ||
| onClick={() => | ||
| sendVerificationCodeMutation.mutate({ | ||
| phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "", | ||
| }) | ||
| }> | ||
| {t("send_code")} | ||
| </Button> | ||
| </div> | ||
|
|
||
| {form.formState.errors.steps && | ||
| form.formState?.errors?.steps[step.stepNumber - 1]?.sendTo && ( | ||
| <p className="mt-1 text-xs text-red-500"> | ||
| {form.formState?.errors?.steps[step.stepNumber - 1]?.sendTo?.message || ""} | ||
| </p> | ||
| )} | ||
| {numberVerified ? ( | ||
| <div className="mt-1"> | ||
| <Badge variant="green">{t("number_verified")}</Badge> | ||
| </div> | ||
| ) : ( | ||
| <> | ||
| <div className="mt-3 flex"> | ||
| <TextField | ||
| className=" border-r-transparent" | ||
| placeholder="Verification code" | ||
| value={verificationCode} | ||
| onChange={(e) => { | ||
| setVerificationCode(e.target.value); | ||
| }} | ||
| required | ||
| /> | ||
| <Button | ||
| color="secondary" | ||
| 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:
Agent: style |
||
| verifyPhoneNumberMutation.mutate({ | ||
| phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "", | ||
| code: verificationCode, | ||
| }); | ||
| }}> | ||
| Verify | ||
| </Button> | ||
| </div> | ||
| </> | ||
| )} | ||
| </div> | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -194,20 +194,41 @@ export const scheduleEmailReminder = async ( | |||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| export const deleteScheduledEmailReminder = async ( | ||||||||||||||||||||||||||||||||
| reminderId: number, | ||||||||||||||||||||||||||||||||
| referenceId: string | null, | ||||||||||||||||||||||||||||||||
| immediateDelete?: boolean | ||||||||||||||||||||||||||||||||
| ) => { | ||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||
| if (!referenceId) { | ||||||||||||||||||||||||||||||||
| await prisma.workflowReminder.delete({ | ||||||||||||||||||||||||||||||||
| where: { | ||||||||||||||||||||||||||||||||
| id: reminderId, | ||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if (immediateDelete) { | ||||||||||||||||||||||||||||||||
| await client.request({ | ||||||||||||||||||||||||||||||||
| url: "/v3/user/scheduled_sends", | ||||||||||||||||||||||||||||||||
| method: "POST", | ||||||||||||||||||||||||||||||||
| body: { | ||||||||||||||||||||||||||||||||
| batch_id: referenceId, | ||||||||||||||||||||||||||||||||
| status: "cancel", | ||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. 🟠 MAJOR — Logic Error — immediateDelete Semantics Inversion (confidence: 100%) The Evidence:
Agent: security There was a problem hiding this comment. ✅
Suggested change
🤖 Grapple PR auto-fix • major • confidence: 100% |
||||||||||||||||||||||||||||||||
| await prisma.workflowReminder.update({ | ||||||||||||||||||||||||||||||||
| where: { | ||||||||||||||||||||||||||||||||
| id: reminderId, | ||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. 🔴 CRITICAL — Data integrity / Orphaned records (confidence: 100%) When Evidence:
Agent: logic There was a problem hiding this comment. 🟠 MAJOR — Unnecessary work / orphaned records (confidence: 100%) The Evidence:
Agent: performance There was a problem hiding this comment. ✅
Suggested change
🤖 Grapple PR auto-fix • critical • confidence: 100% There was a problem hiding this comment. ✅
Suggested change
🤖 Grapple PR auto-fix • major • confidence: 100% |
||||||||||||||||||||||||||||||||
| 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:
Agent: style |
||||||||||||||||||||||||||||||||
| } 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:
Agent: security There was a problem hiding this comment. ✅
Suggested change
🤖 Grapple PR auto-fix • minor • confidence: 77% |
||||||||||||||||||||||||||||||||
| console.log(`Error canceling reminder with error ${error}`); | ||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. 🟠 MAJOR — Data Exposure (confidence: 100%) When Evidence:
Agent: security There was a problem hiding this comment. 🟡
--- 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 There was a problem hiding this comment. 🔴 CRITICAL — Incomplete Cancellation / Data Integrity — Uncancelled SendGrid Batches (confidence: 100%) When Evidence:
Agent: security There was a problem hiding this comment. 🟡
--- 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 |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -174,9 +174,16 @@ 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) { | ||
|
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:
Agent: style There was a problem hiding this comment. 🟡
--- 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 |
||
| console.log(`Error canceling reminder with error ${error}`); | ||
|
There was a problem hiding this comment. 🟡 MINOR — N+1 queries (confidence: 100%) Each call to Evidence:
Agent: performance There was a problem hiding this comment. 🟡
--- 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 |
||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,2 @@ | ||||||||||
| -- AlterTable | ||||||||||
| ALTER TABLE "WorkflowReminder" ADD COLUMN "cancelled" BOOLEAN; | ||||||||||
|
There was a problem hiding this comment. 🟡 MINOR — Missing indexes (confidence: 100%) The new Evidence:
Agent: performance 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:
Agent: style There was a problem hiding this comment. ✅
Suggested change
🤖 Grapple PR auto-fix • minor • confidence: 100% |
||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -641,6 +641,7 @@ model WorkflowReminder { | |||||
| scheduled Boolean | ||||||
| workflowStepId Int | ||||||
| workflowStep WorkflowStep @relation(fields: [workflowStepId], references: [id], onDelete: Cascade) | ||||||
| cancelled Boolean? | ||||||
|
There was a problem hiding this comment. 🟡 MINOR — Migration Safety (confidence: 93%) The Evidence:
Agent: architecture There was a problem hiding this comment. ✅
Suggested change
🤖 Grapple PR auto-fix • minor • confidence: 93% |
||||||
| } | ||||||
|
|
||||||
| enum WorkflowTemplates { | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 CRITICAL — Fire-and-Forget Async / Silent Failures (confidence: 100%)
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.Evidence:
remindersToDeleteand awaited them viaPromise.all; the new code has no equivalentcancelled=truefails, the reminder will still fire on schedule with no indication of failureAgent: architecture