Automated Test: workflow-queue-enhanced #365
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>
📝 WalkthroughWalkthroughThis pull request refactors workflow reminder cancellation across the booking system. Function signatures for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
197-235:⚠️ Potential issue | 🟠 MajorConsistent error handling: SendGrid failure in
immediateDeletepath also prevents return.Similar to the SMS manager, if the
client.requestcall on Line 214 throws, the entire function falls into the catch block and the error is silently logged. If the intent is to always clean up the DB, the external call should be isolated. This is the same pattern as flagged insmsReminderManager.ts.packages/features/ee/workflows/components/WorkflowStepContainer.tsx (1)
155-168:⚠️ Potential issue | 🟠 MajorWrong toast variant when verification fails — "wrong_code" shown as "success".
On Line 157, when
isVerifiedisfalse, the toast displays the "wrong_code" message with a"success"variant. This should be"error"to properly communicate failure to the user.Proposed fix
onSuccess: async (isVerified) => { - showToast(isVerified ? t("verified_successfully") : t("wrong_code"), "success"); + showToast(isVerified ? t("verified_successfully") : t("wrong_code"), isVerified ? "success" : "error"); setNumberVerified(isVerified); utils.viewer.workflows.getVerifiedNumbers.invalidate(); },packages/trpc/server/routers/viewer/workflows.tsx (2)
211-225:⚠️ Potential issue | 🔴 CriticalRace condition: workflow deletion may cascade-remove reminder rows before async cancellation completes.
Lines 212–218 fire off
deleteScheduledEmailReminder/deleteScheduledSMSReminderwithoutawait, then line 220 immediately deletes the workflow. If the schema cascades deletes toWorkflowReminder, the reminder rows could be removed before the async cancellation functions finish reading/updating them.Proposed fix: await cancellations before deleting the workflow
- //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); - } - }); + //cancel workflow reminders of deleted workflow + 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(); + }) + );
516-531:⚠️ Potential issue | 🔴 CriticalRace condition: step deletion may cascade-remove reminder rows before async cancellation finishes.
Same pattern as the workflow delete path — non-awaited cancellations at lines 519–525 followed by immediate
workflowStep.deleteat line 527, which may cascade.Proposed fix
- // cancel all workflow reminders from deleted steps - if (remindersFromStep.length > 0) { - remindersFromStep.forEach((reminder) => { - if (reminder.method === WorkflowMethods.EMAIL) { - deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); - } else if (reminder.method === WorkflowMethods.SMS) { - deleteScheduledSMSReminder(reminder.id, reminder.referenceId); - } - }); - } + // cancel all workflow reminders from deleted steps + if (remindersFromStep.length > 0) { + await Promise.all( + remindersFromStep.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(); + }) + ); + }
🤖 Fix all issues with AI agents
In `@packages/features/bookings/lib/handleCancelBooking.ts`:
- Around line 484-493: The loop in handleCancelBooking uses
deleteScheduledEmailReminder and deleteScheduledSMSReminder (both async) inside
forEach without awaiting, causing silent failures; change it to collect the
promises (from deleteScheduledEmailReminder/deleteScheduledSMSReminder for each
reminder in updatedBookings) into an array and await them with Promise.all (or
Promise.allSettled) inside a try/catch block so errors are handled and logged
similarly to the pattern in handleNewBooking.
In `@packages/features/bookings/lib/handleNewBooking.ts`:
- Around line 964-975: The current try/catch won’t catch asynchronous rejections
because originalRescheduledBooking.workflowReminders.forEach invokes async
deleteScheduledEmailReminder/deleteScheduledSMSReminder without awaiting; change
the logic to map reminders to promise-returning calls (use the reminder.method
check with WorkflowMethods.EMAIL to choose deleteScheduledEmailReminder or
deleteScheduledSMSReminder), collect those promises into an array and await
Promise.allSettled(...) inside the try block, then log any rejected results so
errors are handled and captured by the existing try/catch.
In `@packages/features/ee/workflows/api/scheduleEmailReminders.ts`:
- Around line 56-64: The loop in scheduleEmailReminders.ts is sending
reminder.referenceId (which can be null) to SendGrid via client.request; add a
null-check inside the for (const reminder of remindersToCancel) loop to skip
reminders with a falsy or null referenceId (e.g., if (!reminder.referenceId)
continue) before calling client.request so you never send batch_id: null;
optionally log or mark skipped reminders for traceability.
- Around line 53-77: The current try/catch around the entire loop causes one
SendGrid failure to abort processing of remaining reminders; inside
scheduleEmailReminders.ts, move the try/catch into the for (const reminder of
remindersToCancel) loop so each reminder is handled independently: for each
reminder call client.request(...) and prisma.workflowReminder.delete(...) inside
a per-iteration try/catch (log the error but continue), still collect the delete
promises in workflowRemindersToDelete and await
Promise.all(workflowRemindersToDelete) after the loop to flush DB deletions.
In `@packages/features/ee/workflows/components/WorkflowStepContainer.tsx`:
- Around line 436-457: The UI strings "Verification code" and "Verify" in
WorkflowStepContainer are hardcoded; update the TextField placeholder and the
Button label to use the i18n translator (e.g., t('...')) like other UI text in
this component. Ensure you import/use the same translation hook/function used
elsewhere in this file (the t instance used by other strings), then replace the
placeholder on the TextField and the Button children with translated keys (e.g.,
t('verification.code') and t('verify') or existing keys), keeping existing
handlers (verificationCode state, verifyPhoneNumberMutation, form.getValues)
unchanged.
In `@packages/features/ee/workflows/lib/reminders/emailReminderManager.ts`:
- Around line 213-223: When immediateDelete is true in emailReminderManager.ts
(the block that calls client.request to cancel the SendGrid scheduled send using
referenceId), also update the corresponding WorkflowReminder DB row so it isn't
left appearing active; fetch the reminder by its id/reference (eg.
WorkflowReminder with batch_id/referenceId or the function's local reminder id)
and either delete it or set a cancelled flag (e.g., cancelled: true / deletedAt
timestamp) and save the change before returning, so the CRON in
scheduleEmailReminders.ts won't see it as active and re-attempt cancellation.
In `@packages/features/ee/workflows/lib/reminders/smsReminderManager.ts`:
- Around line 177-189: The deleteScheduledSMSReminder function currently wraps
twilio.cancelSMS and prisma.workflowReminder.delete in one try block causing DB
records to remain if Twilio call fails; change it so twilio.cancelSMS is called
in its own try/catch (or catch and log its error) and ensure
prisma.workflowReminder.delete is executed regardless (or on Twilio failure mark
the record as failed) so the DB cleanup always runs; update references to
twilio.cancelSMS and prisma.workflowReminder.delete accordingly and keep clear
logging for both operations.
In `@packages/trpc/server/routers/viewer/workflows.tsx`:
- Around line 375-382: The cancellation calls for remindersToDelete are
currently fire-and-forget — change the forEach to collect promises (map over
remindersToDelete.flat() and for each reminder call deleteScheduledEmailReminder
or deleteScheduledSMSReminder and return the promise) then await
Promise.all(...) so that deleteScheduledEmailReminder/deleteScheduledSMSReminder
complete before proceeding to the subsequent deleteMany/create operations;
reference remindersToDelete, WorkflowMethods, deleteScheduledEmailReminder and
deleteScheduledSMSReminder when making this change.
- Around line 572-580: The forEach with an async callback on remindersToUpdate
doesn't await deletions, so cancellations may run after scheduling new
reminders; replace remindersToUpdate.forEach(async (reminder) => { ... }) with
an awaited approach — either use for (const reminder of remindersToUpdate) {
await ... } or await Promise.all(remindersToUpdate.map(reminder =>
reminder.method === WorkflowMethods.EMAIL ?
deleteScheduledEmailReminder(reminder.id, reminder.referenceId) :
deleteScheduledSMSReminder(reminder.id, reminder.referenceId))); ensure you
await the deleteScheduledEmailReminder/deleteScheduledSMSReminder calls before
proceeding to the re-scheduling logic.
🧹 Nitpick comments (3)
packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql (1)
1-2: Looks good — consider adding a default value.The nullable
BOOLEANcolumn works with the current query pattern (cancelled: true), sinceNULLwon't match. However, addingDEFAULT falsewould make the semantics more explicit and avoid ambiguity between "not cancelled" and "unknown."This is a minor suggestion and not blocking.
packages/prisma/schema.prisma (1)
634-645: Consider adding a composite index for the CRON query pattern.The CRON in
scheduleEmailReminders.tsqueriesWHERE cancelled = true AND scheduledDate <= .... As theWorkflowRemindertable grows, an index on(cancelled, scheduledDate)would help this query. Not urgent if the table is small, but worth considering proactively.packages/trpc/server/routers/viewer/bookings.tsx (1)
488-494: Async reminder cancellations are not awaited — fire-and-forget.
forEachdoes not await the returned promises fromdeleteScheduledEmailReminder/deleteScheduledSMSReminder. This means the function proceeds to send reschedule emails and webhooks before cancellations complete. While both helpers have internal try/catch, any DB errors (e.g., Prisma connection issues) will result in unhandled promise rejections.Consider using
Promise.allwith.map()to await all cancellations, or at minimum wrapping with.catch().Proposed fix
- 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); + } + }) + );
| //Workflows - cancel all reminders for cancelled bookings | ||
| updatedBookings.forEach((booking) => { | ||
| booking.workflowReminders.forEach((reminder) => { | ||
| if (reminder.scheduled && reminder.referenceId) { | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.referenceId); | ||
| } | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| const reminderToDelete = prisma.workflowReminder.deleteMany({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| }); | ||
| remindersToDelete.push(reminderToDelete); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Unhandled async calls — fire-and-forget pattern risks silent failures.
deleteScheduledEmailReminder and deleteScheduledSMSReminder are async, but they are called inside forEach without await or error handling. Any failure (network/external service) will produce unhandled promise rejections and go unnoticed. Compare with handleNewBooking.ts (lines 964–975), where the same pattern is wrapped in try/catch.
Proposed fix: collect promises and await them
- //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 reminderCancellations: Promise<unknown>[] = [];
+ updatedBookings.forEach((booking) => {
+ booking.workflowReminders.forEach((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ reminderCancellations.push(
+ deleteScheduledEmailReminder(reminder.id, reminder.referenceId).catch((e) =>
+ console.error("Failed to cancel email reminder", reminder.id, e)
+ )
+ );
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ reminderCancellations.push(
+ deleteScheduledSMSReminder(reminder.id, reminder.referenceId).catch((e) =>
+ console.error("Failed to cancel SMS reminder", reminder.id, e)
+ )
+ );
+ }
+ });
+ });
+ await Promise.all(reminderCancellations);🤖 Prompt for AI Agents
In `@packages/features/bookings/lib/handleCancelBooking.ts` around lines 484 -
493, The loop in handleCancelBooking uses deleteScheduledEmailReminder and
deleteScheduledSMSReminder (both async) inside forEach without awaiting, causing
silent failures; change it to collect the promises (from
deleteScheduledEmailReminder/deleteScheduledSMSReminder for each reminder in
updatedBookings) into an array and await them with Promise.all (or
Promise.allSettled) inside a try/catch block so errors are handled and logged
similarly to the pattern in handleNewBooking.
| 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); | ||
| } |
There was a problem hiding this comment.
try/catch won't catch async errors from forEach — rejections will be unhandled.
The forEach callback fires off async deleteScheduled*Reminder calls without await. Since forEach does not await its callback, any rejection from the delete calls will bypass this catch block entirely, resulting in unhandled promise rejections. The try/catch here only guards against synchronous errors during iteration setup.
Proposed fix: use `Promise.allSettled` to properly handle async errors
try {
// cancel workflow reminders from previous rescheduled booking
- originalRescheduledBooking.workflowReminders.forEach((reminder) => {
+ await Promise.allSettled(originalRescheduledBooking.workflowReminders.map((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
} else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
- });
+ return Promise.resolve();
+ }));
} catch (error) {
log.error("Error while canceling scheduled workflow reminders", error);
}🤖 Prompt for AI Agents
In `@packages/features/bookings/lib/handleNewBooking.ts` around lines 964 - 975,
The current try/catch won’t catch asynchronous rejections because
originalRescheduledBooking.workflowReminders.forEach invokes async
deleteScheduledEmailReminder/deleteScheduledSMSReminder without awaiting; change
the logic to map reminders to promise-returning calls (use the reminder.method
check with WorkflowMethods.EMAIL to choose deleteScheduledEmailReminder or
deleteScheduledSMSReminder), collect those promises into an array and await
Promise.allSettled(...) inside the try block, then log any rejected results so
errors are handled and captured by the existing try/catch.
| 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}`); | ||
| } |
There was a problem hiding this comment.
A single SendGrid failure aborts cancellation of all remaining reminders.
The entire for loop is wrapped in one try/catch. If client.request throws for any reminder (e.g., invalid batch_id, network error), all subsequent reminders in remindersToCancel are skipped and their DB records are not deleted.
Move the try/catch inside the loop so each reminder is processed independently.
Proposed fix
- try {
- const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = [];
-
- for (const reminder of remindersToCancel) {
+ const workflowRemindersToDelete: Promise<WorkflowReminder>[] = [];
+
+ for (const reminder of remindersToCancel) {
+ try {
+ if (!reminder.referenceId) {
+ 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);
+ workflowRemindersToDelete.push(
+ prisma.workflowReminder.delete({ where: { id: reminder.id } })
+ );
+ } catch (error) {
+ console.log(`Error cancelling reminder ${reminder.id}: ${error}`);
}
- await Promise.all(workflowRemindersToDelete);
- } catch (error) {
- console.log(`Error cancelling scheduled Emails: ${error}`);
}
+ await Promise.all(workflowRemindersToDelete);🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/api/scheduleEmailReminders.ts` around lines 53
- 77, The current try/catch around the entire loop causes one SendGrid failure
to abort processing of remaining reminders; inside scheduleEmailReminders.ts,
move the try/catch into the for (const reminder of remindersToCancel) loop so
each reminder is handled independently: for each reminder call
client.request(...) and prisma.workflowReminder.delete(...) inside a
per-iteration try/catch (log the error but continue), still collect the delete
promises in workflowRemindersToDelete and await
Promise.all(workflowRemindersToDelete) after the loop to flush DB deletions.
| 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.
reminder.referenceId can be null — sending null as batch_id to SendGrid will fail.
referenceId is nullable in the schema (String?). Reminders written to the DB for the CRON path (Lines 163–171 in emailReminderManager.ts) have no referenceId. If such a reminder gets cancelled: true, this code sends batch_id: null to SendGrid, which will throw.
Add a null-check before calling the SendGrid API.
🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/api/scheduleEmailReminders.ts` around lines 56
- 64, The loop in scheduleEmailReminders.ts is sending reminder.referenceId
(which can be null) to SendGrid via client.request; add a null-check inside the
for (const reminder of remindersToCancel) loop to skip reminders with a falsy or
null referenceId (e.g., if (!reminder.referenceId) continue) before calling
client.request so you never send batch_id: null; optionally log or mark skipped
reminders for traceability.
| <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" | ||
| 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({ | ||
| className="-ml-[3px] rounded-tl-none rounded-bl-none " | ||
| disabled={verifyPhoneNumberMutation.isLoading} | ||
| onClick={() => { | ||
| verifyPhoneNumberMutation.mutate({ | ||
| phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "", | ||
| }) | ||
| }> | ||
| {t("send_code")} | ||
| code: verificationCode, | ||
| }); | ||
| }}> | ||
| Verify | ||
| </Button> |
There was a problem hiding this comment.
Hardcoded English strings — missing i18n for "Verification code" and "Verify".
Line 439 ("Verification code") and Line 456 ("Verify") are not wrapped in t(), unlike all other UI text in this component. This breaks localization.
Proposed fix
<TextField
className=" border-r-transparent"
- placeholder="Verification code"
+ placeholder={t("verification_code")}
value={verificationCode} }}>
- Verify
+ {t("verify")}
</Button>🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/components/WorkflowStepContainer.tsx` around
lines 436 - 457, The UI strings "Verification code" and "Verify" in
WorkflowStepContainer are hardcoded; update the TextField placeholder and the
Button label to use the i18n translator (e.g., t('...')) like other UI text in
this component. Ensure you import/use the same translation hook/function used
elsewhere in this file (the t instance used by other strings), then replace the
placeholder on the TextField and the Button children with translated keys (e.g.,
t('verification.code') and t('verify') or existing keys), keeping existing
handlers (verificationCode state, verifyPhoneNumberMutation, form.getValues)
unchanged.
| 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.
immediateDelete path cancels at SendGrid but leaves the DB record behind.
When immediateDelete is true, the SendGrid scheduled send is cancelled but the WorkflowReminder row is not deleted or marked. It will linger until the CRON's deleteMany sweep (Line 34–41 in scheduleEmailReminders.ts) cleans it up after scheduledDate passes. In the interim, the record appears active (not cancelled, not deleted), which could confuse queries or cause the CRON's cancellation flow to re-attempt the already-cancelled batch.
Consider also deleting the DB record or marking cancelled: true to make the state explicit.
Proposed fix
if (immediateDelete) {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: referenceId,
status: "cancel",
},
});
+ await prisma.workflowReminder.delete({
+ where: {
+ id: reminderId,
+ },
+ });
return;
}🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/lib/reminders/emailReminderManager.ts` around
lines 213 - 223, When immediateDelete is true in emailReminderManager.ts (the
block that calls client.request to cancel the SendGrid scheduled send using
referenceId), also update the corresponding WorkflowReminder DB row so it isn't
left appearing active; fetch the reminder by its id/reference (eg.
WorkflowReminder with batch_id/referenceId or the function's local reminder id)
and either delete it or set a cancelled flag (e.g., cancelled: true / deletedAt
timestamp) and save the change before returning, so the CRON in
scheduleEmailReminders.ts won't see it as active and re-attempt cancellation.
| export const deleteScheduledSMSReminder = async (reminderId: number, referenceId: string | null) => { | ||
| try { | ||
| await twilio.cancelSMS(referenceId); | ||
| if (referenceId) { | ||
| await twilio.cancelSMS(referenceId); | ||
| } | ||
| await prisma.workflowReminder.delete({ | ||
| where: { | ||
| id: reminderId, | ||
| }, | ||
| }); | ||
| } catch (error) { | ||
| console.log(`Error canceling reminder with error ${error}`); | ||
| } |
There was a problem hiding this comment.
If Twilio cancellation fails, the DB record is orphaned.
Both the twilio.cancelSMS call and prisma.workflowReminder.delete are in the same try block. If cancelSMS throws, the delete is skipped, leaving a stale reminder record in the database with no mechanism to retry or clean it up.
Consider separating the external call from the DB cleanup, or handling the Twilio failure independently so the DB record is still deleted (or at least marked).
Proposed fix
export const deleteScheduledSMSReminder = async (reminderId: number, referenceId: string | null) => {
try {
if (referenceId) {
- await twilio.cancelSMS(referenceId);
+ try {
+ await twilio.cancelSMS(referenceId);
+ } catch (error) {
+ console.log(`Error canceling SMS with Twilio: ${error}`);
+ }
}
await prisma.workflowReminder.delete({
where: {
id: reminderId,
},
});
} catch (error) {
console.log(`Error canceling reminder with error ${error}`);
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const deleteScheduledSMSReminder = async (reminderId: number, referenceId: string | null) => { | |
| try { | |
| await twilio.cancelSMS(referenceId); | |
| if (referenceId) { | |
| await twilio.cancelSMS(referenceId); | |
| } | |
| await prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminderId, | |
| }, | |
| }); | |
| } catch (error) { | |
| console.log(`Error canceling reminder with error ${error}`); | |
| } | |
| export const deleteScheduledSMSReminder = async (reminderId: number, referenceId: string | null) => { | |
| try { | |
| if (referenceId) { | |
| try { | |
| await twilio.cancelSMS(referenceId); | |
| } catch (error) { | |
| console.log(`Error canceling SMS with Twilio: ${error}`); | |
| } | |
| } | |
| await prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminderId, | |
| }, | |
| }); | |
| } catch (error) { | |
| console.log(`Error canceling reminder with error ${error}`); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/lib/reminders/smsReminderManager.ts` around
lines 177 - 189, The deleteScheduledSMSReminder function currently wraps
twilio.cancelSMS and prisma.workflowReminder.delete in one try block causing DB
records to remain if Twilio call fails; change it so twilio.cancelSMS is called
in its own try/catch (or catch and log its error) and ensure
prisma.workflowReminder.delete is executed regardless (or on Twilio failure mark
the record as failed) so the DB cleanup always runs; update references to
twilio.cancelSMS and prisma.workflowReminder.delete accordingly and keep clear
logging for both operations.
| //cancel workflow reminders for all bookings from event types that got disabled | ||
| remindersToDelete.flat().forEach((reminder) => { | ||
| //already scheduled reminders | ||
| if (reminder.referenceId) { | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.referenceId); | ||
| } | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| const deleteReminder = ctx.prisma.workflowReminder.deleteMany({ | ||
| where: { | ||
| id: reminder.id, | ||
| booking: { | ||
| userId: ctx.user.id, | ||
| }, | ||
| }, | ||
| }); | ||
| deleteReminderPromise.push(deleteReminder); | ||
| }); |
There was a problem hiding this comment.
Same fire-and-forget issue: reminder cancellations for deactivated event types are not awaited.
These cancellations run concurrently with the subsequent deleteMany/create operations below without any ordering guarantee.
Proposed fix
- //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);
+ }
+ return Promise.resolve();
+ })
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //cancel workflow reminders for all bookings from event types that got disabled | |
| remindersToDelete.flat().forEach((reminder) => { | |
| //already scheduled reminders | |
| if (reminder.referenceId) { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.referenceId); | |
| } | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| const deleteReminder = ctx.prisma.workflowReminder.deleteMany({ | |
| where: { | |
| id: reminder.id, | |
| booking: { | |
| userId: ctx.user.id, | |
| }, | |
| }, | |
| }); | |
| deleteReminderPromise.push(deleteReminder); | |
| }); | |
| //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); | |
| } | |
| return Promise.resolve(); | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/workflows.tsx` around lines 375 - 382,
The cancellation calls for remindersToDelete are currently fire-and-forget —
change the forEach to collect promises (map over remindersToDelete.flat() and
for each reminder call deleteScheduledEmailReminder or
deleteScheduledSMSReminder and return the promise) then await Promise.all(...)
so that deleteScheduledEmailReminder/deleteScheduledSMSReminder complete before
proceeding to the subsequent deleteMany/create operations; reference
remindersToDelete, WorkflowMethods, deleteScheduledEmailReminder and
deleteScheduledSMSReminder when making this change.
|
|
||
| //cancel all workflow reminders from steps that were edited | ||
| remindersToUpdate.forEach(async (reminder) => { | ||
| if (reminder.referenceId) { | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.referenceId); | ||
| } | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| await ctx.prisma.workflowReminder.deleteMany({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Non-awaited async forEach — cancellations for edited steps will not complete before new reminders are scheduled.
remindersToUpdate.forEach(async (reminder) => { ... }) fires and forgets, so the subsequent re-scheduling logic (lines 581+) may execute while old reminders are still being cancelled. This could lead to duplicate or stale reminders.
Proposed fix
- //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);
- }
- });
+ //cancel all workflow reminders from steps that were edited
+ 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);
+ }
+ return Promise.resolve();
+ })
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //cancel all workflow reminders from steps that were edited | |
| remindersToUpdate.forEach(async (reminder) => { | |
| if (reminder.referenceId) { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.referenceId); | |
| } | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| await ctx.prisma.workflowReminder.deleteMany({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| }); | |
| }); | |
| //cancel all workflow reminders from steps that were edited | |
| 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); | |
| } | |
| return Promise.resolve(); | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/workflows.tsx` around lines 572 - 580,
The forEach with an async callback on remindersToUpdate doesn't await deletions,
so cancellations may run after scheduling new reminders; replace
remindersToUpdate.forEach(async (reminder) => { ... }) with an awaited approach
— either use for (const reminder of remindersToUpdate) { await ... } or await
Promise.all(remindersToUpdate.map(reminder => reminder.method ===
WorkflowMethods.EMAIL ? deleteScheduledEmailReminder(reminder.id,
reminder.referenceId) : deleteScheduledSMSReminder(reminder.id,
reminder.referenceId))); ensure you await the
deleteScheduledEmailReminder/deleteScheduledSMSReminder calls before proceeding
to the re-scheduling logic.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor