Automated Test: workflow-queue-enhanced #357
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 PR refactors the reminder cancellation system across bookings and workflows. It introduces a soft-delete pattern by adding a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
197-235:⚠️ Potential issue | 🟠 MajorSoft-delete path has a timing gap — reminders scheduled >1 hour away won't be cancelled on SendGrid until the CRON processes them.
When
immediateDeleteis falsy andreferenceIdis present (lines 225-232), the reminder is only markedcancelled: truein the DB without immediately calling the SendGrid cancel API. A background CRON job (scheduleEmailReminders.ts lines 43-77) periodically cancels these reminders on SendGrid, but only for those scheduled within the next 1 hour. This creates a race condition: if a reminder is cancelled now but scheduled >1 hour away, SendGrid retains the scheduled email until the CRON processes it at a later run. If the booking or workflow re-activates before the CRON runs, the email may still be sent.For immediate, guaranteed cancellation, use
immediateDelete: trueinstead of relying on the asynchronous CRON mechanism.
🤖 Fix all issues with AI agents
In `@packages/features/bookings/lib/handleCancelBooking.ts`:
- Around line 484-493: The reminder deletions inside updatedBookings.forEach are
being fired without awaiting their async work; collect the promises returned by
deleteScheduledEmailReminder and deleteScheduledSMSReminder (from the
updatedBookings -> booking.workflowReminders loop) into an array (e.g.,
reminderDeletionPromises) and then await Promise.all(reminderDeletionPromises)
before finishing the handler so cleanup is awaited; also catch/log rejections
from Promise.all to surface failures (or use Promise.allSettled if you want
best-effort logging without failing the whole flow).
In `@packages/features/bookings/lib/handleNewBooking.ts`:
- Around line 964-975: The try-catch around
originalRescheduledBooking.workflowReminders.forEach(...) is ineffective because
forEach doesn't await async calls; replace the fire-and-forget loop with
collecting promises (map over originalRescheduledBooking.workflowReminders and
push either deleteScheduledEmailReminder(reminder.id, reminder.referenceId,
true) or deleteScheduledSMSReminder(reminder.id, reminder.referenceId)) and then
await Promise.allSettled(...) (or Promise.all(...) if you want fail-fast) so
errors from deleteScheduledEmailReminder and deleteScheduledSMSReminder are
observed and can be logged; keep references to WorkflowMethods to choose the
correct deletion call and log failures from the settled results.
In `@packages/features/ee/workflows/api/scheduleEmailReminders.ts`:
- Around line 44-51: The cancellation query is missing a method filter and may
include non-email reminders; update the prisma.workflowReminder.findMany call
(the remindersToCancel query) to include where.method: WorkflowMethods.EMAIL so
only email reminders are selected, and add an import/reference to
WorkflowMethods in this file if it's not already present; keep the existing
cancelled: true and scheduledDate <= dayjs().add(1, "hour").toISOString()
predicates.
- Around line 53-77: The current try-catch around the entire loop means a single
failing client.request aborts processing of the rest of reminders; change the
logic so each reminder in remindersToCancel is processed inside its own
try-catch: for each reminder call client.request(...) inside a per-iteration try
block, handle/log errors for that reminder in the catch and continue to the next
reminder, and only create/push the prisma.workflowReminder.delete
(workflowRemindersToDelete) for reminders you intended to delete (e.g., after a
successful cancel or according to your desired failure policy) so that failures
for one reminder do not prevent others from being cancelled/deleted.
In `@packages/features/ee/workflows/components/WorkflowStepContainer.tsx`:
- Around line 436-457: Replace the two hardcoded English strings with i18n keys:
use the translation function for the TextField placeholder and the Button label
in the WorkflowStepContainer component—update the TextField placeholder from
"Verification code" to t("verification_code") and the Button text from "Verify"
to t("verify"); ensure the component imports/has access to t and leave the rest
of the logic around verificationCode, verifyPhoneNumberMutation,
form.getValues(...) and step.stepNumber unchanged.
In `@packages/features/ee/workflows/lib/reminders/smsReminderManager.ts`:
- Around line 177-189: deleteScheduledSMSReminder currently wraps both
twilio.cancelSMS and prisma.workflowReminder.delete in the same try/catch so a
failure in cancelSMS prevents the DB delete; update deleteScheduledSMSReminder
to call twilio.cancelSMS inside its own try/catch (log the Twilio error but
swallow it) and then always call prisma.workflowReminder.delete in a separate
try/catch (or let it bubble) so the reminder record is removed regardless of
Twilio failures; reference the functions twilio.cancelSMS and
prisma.workflowReminder.delete when making the changes.
In `@packages/trpc/server/routers/viewer/bookings.tsx`:
- Around line 484-494: The reschedule path currently soft-deletes reminders
without cancelling external scheduled sends and discards the returned Promises;
update the logic around cancelScheduledJobs and the loop over
bookingToReschedule.workflowReminders to (1) call the external-cancel path with
immediateDelete: true when invoking deleteScheduledEmailReminder /
deleteScheduledSMSReminder so SendGrid is synchronously cancelled, and (2) await
the async operations instead of using .forEach() — e.g., collect the
deleteScheduled* calls into an array (or await cancelScheduledJobs if it returns
a Promise) and await Promise.all(...) to ensure all cancellations complete
before returning.
In `@packages/trpc/server/routers/viewer/workflows.tsx`:
- Around line 517-526: The reminder cancellation calls inside the
remindersFromStep loop are currently fire-and-forget; collect the Promises
returned by deleteScheduledEmailReminder and deleteScheduledSMSReminder
(checking reminder.method === WorkflowMethods.EMAIL) into an array, await
Promise.all on that array to ensure all cancel requests complete, then proceed
to delete the workflow step from the DB; update the block that iterates
remindersFromStep to push each delete promise and await them before the step
deletion.
- Around line 211-218: The scheduledReminders.forEach is dropping Promises from
the async functions deleteScheduledEmailReminder and deleteScheduledSMSReminder,
so collect those Promises (e.g., map scheduledReminders to an array of calls to
deleteScheduledEmailReminder/deleteScheduledSMSReminder) and await them with
await Promise.all(promises) before proceeding to workflow.deleteMany and before
returning the mutation response; apply the same change to the other occurrences
of this pattern in the file (the later blocks around the second and subsequent
reminder-handling sites) and the analogous block in bookings.tsx so all
SendGrid/SMS cancel calls are awaited.
- Around line 375-382: The current loop cancels email reminders but leaves
soft-deleted rows until their scheduled time, causing table bloat; update the
cancellation path in the remindersToDelete.flat().forEach block so that when
reminder.method === WorkflowMethods.EMAIL you perform an immediate hard-delete
instead of a soft-cancel: either call an existing hard-delete helper (e.g.,
deleteScheduledEmailReminderImmediate or deleteScheduledEmailReminder with a
force/hard flag) or implement a new deleteScheduledEmailReminderHard function
that removes the DB row and invoke it here; keep deleteScheduledSMSReminder
unchanged for SMS.
🧹 Nitpick comments (4)
packages/prisma/schema.prisma (1)
634-645: Consider adding an index oncancelled(or a composite index withscheduledDate).The cron handler in
scheduleEmailReminders.tsqueriesWHERE cancelled = true AND scheduledDate <= .... Without an index, this becomes a full table scan as theWorkflowRemindertable grows.cancelled Boolean? + + @@index([cancelled, scheduledDate]) }packages/features/ee/workflows/api/scheduleEmailReminders.ts (1)
54-54: Avoid using internal Prisma client types.
Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>is an internal Prisma type. UsePromise<WorkflowReminder>instead for clarity and forward compatibility.packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
203-211: When!referenceId, the reminder is hard-deleted from the DB without checkingimmediateDelete.If
referenceIdis falsy, the function immediately deletes the DB record regardless of whetherimmediateDeletewas requested or the caller expected a soft-delete. This is fine for the current call sites (reminders without areferenceIdhave no external state to cancel), but the priority of the!referenceIdcheck overimmediateDeleteshould be documented to prevent confusion for future callers.💡 Add a brief comment explaining the rationale
try { if (!referenceId) { + // No external scheduled send exists — safe to hard-delete the DB record directly await prisma.workflowReminder.delete({ where: { id: reminderId, }, }); return; }packages/trpc/server/routers/viewer/workflows.tsx (1)
1-10:PrismaPromiseis imported as a value but used only as a type.
PrismaPromiseis only used in a type annotation (line 342). Importing it as a value may prevent tree-shaking from eliminating it. Consider using a type-only import forPrismaPromisewhile keepingPrismaas a value import.💡 Suggested import split
+import type { PrismaPromise } from "@prisma/client"; import { - PrismaPromise, WorkflowTemplates, WorkflowActions, WorkflowTriggerEvents, BookingStatus, WorkflowMethods, TimeUnit, Prisma } from "@prisma/client";
| //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.
Same fire-and-forget pattern — reminder deletions are not awaited.
forEach doesn't await the async deleteScheduledEmailReminder/deleteScheduledSMSReminder calls, so these promises are not included in the Promise.all on line 497. The handler could respond with "Booking successfully cancelled" while reminder cleanup is still in-flight (or has silently failed).
If this is intentional best-effort cleanup, consider at minimum collecting the promises so failures are logged:
♻️ Proposed fix
//Workflows - cancel all reminders for cancelled bookings
+ const reminderDeletions: Promise<void>[] = [];
updatedBookings.forEach((booking) => {
booking.workflowReminders.forEach((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ reminderDeletions.push(deleteScheduledEmailReminder(reminder.id, reminder.referenceId));
} else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ reminderDeletions.push(deleteScheduledSMSReminder(reminder.id, reminder.referenceId));
}
});
});
- const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes];
+ const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes, ...reminderDeletions];🤖 Prompt for AI Agents
In `@packages/features/bookings/lib/handleCancelBooking.ts` around lines 484 -
493, The reminder deletions inside updatedBookings.forEach are being fired
without awaiting their async work; collect the promises returned by
deleteScheduledEmailReminder and deleteScheduledSMSReminder (from the
updatedBookings -> booking.workflowReminders loop) into an array (e.g.,
reminderDeletionPromises) and then await Promise.all(reminderDeletionPromises)
before finishing the handler so cleanup is awaited; also catch/log rejections
from Promise.all to surface failures (or use Promise.allSettled if you want
best-effort logging without failing the whole flow).
| 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.
forEach with async callbacks — deletions are fire-and-forget and the catch block is ineffective.
Array.prototype.forEach does not await the returned promises from async callbacks. The try-catch here will never catch rejection errors from deleteScheduledEmailReminder or deleteScheduledSMSReminder — they become unhandled promise rejections.
If fire-and-forget is intentional, remove the misleading try-catch. If errors should be caught, use Promise.all (or Promise.allSettled):
🐛 Proposed fix (if errors should be caught)
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);
}
- });
+ }));
} catch (error) {
log.error("Error while canceling scheduled workflow reminders", 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.
| 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); | |
| } | |
| try { | |
| // cancel workflow reminders from previous rescheduled booking | |
| await Promise.allSettled(originalRescheduledBooking.workflowReminders.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); | |
| } | |
| })); | |
| } 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 try-catch around originalRescheduledBooking.workflowReminders.forEach(...)
is ineffective because forEach doesn't await async calls; replace the
fire-and-forget loop with collecting promises (map over
originalRescheduledBooking.workflowReminders and push either
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) or
deleteScheduledSMSReminder(reminder.id, reminder.referenceId)) and then await
Promise.allSettled(...) (or Promise.all(...) if you want fail-fast) so errors
from deleteScheduledEmailReminder and deleteScheduledSMSReminder are observed
and can be logged; keep references to WorkflowMethods to choose the correct
deletion call and log failures from the settled results.
| const remindersToCancel = await prisma.workflowReminder.findMany({ | ||
| where: { | ||
| cancelled: true, | ||
| scheduledDate: { | ||
| lte: dayjs().add(1, "hour").toISOString(), | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Missing method: WorkflowMethods.EMAIL filter — this may cancel SMS reminders too.
This cron is specifically for email reminders, but the cancellation query doesn't filter by method. It will pick up all cancelled reminders (including SMS) scheduled within the next hour, and attempt to cancel them via the SendGrid API (which will fail or be a no-op for SMS reminders). It also deletes them from the DB, preventing the SMS cron from processing them.
const remindersToCancel = await prisma.workflowReminder.findMany({
where: {
cancelled: true,
+ method: WorkflowMethods.EMAIL,
scheduledDate: {
lte: dayjs().add(1, "hour").toISOString(),
},
},
});📝 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.
| const remindersToCancel = await prisma.workflowReminder.findMany({ | |
| where: { | |
| cancelled: true, | |
| scheduledDate: { | |
| lte: dayjs().add(1, "hour").toISOString(), | |
| }, | |
| }, | |
| }); | |
| const remindersToCancel = await prisma.workflowReminder.findMany({ | |
| where: { | |
| cancelled: true, | |
| method: WorkflowMethods.EMAIL, | |
| scheduledDate: { | |
| lte: dayjs().add(1, "hour").toISOString(), | |
| }, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/api/scheduleEmailReminders.ts` around lines 44
- 51, The cancellation query is missing a method filter and may include
non-email reminders; update the prisma.workflowReminder.findMany call (the
remindersToCancel query) to include where.method: WorkflowMethods.EMAIL so only
email reminders are selected, and add an import/reference to WorkflowMethods in
this file if it's not already present; keep the existing cancelled: true and
scheduledDate <= dayjs().add(1, "hour").toISOString() predicates.
| 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 for all remaining reminders.
The await client.request(...) on line 57 is inside a for loop wrapped in a single try-catch. If the SendGrid call fails for one reminder, the exception propagates to the outer catch and all subsequent reminders in remindersToCancel are skipped (never cancelled, never deleted).
Move the try-catch inside the loop so each reminder is handled 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 {
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 for reminder ${reminder.id}: ${error}`);
}
- await Promise.all(workflowRemindersToDelete);
- } catch (error) {
- console.log(`Error cancelling scheduled Emails: ${error}`);
}
+ await Promise.all(workflowRemindersToDelete);📝 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.
| 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}`); | |
| } | |
| const workflowRemindersToDelete: Promise<WorkflowReminder>[] = []; | |
| 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({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| }); | |
| workflowRemindersToDelete.push(workflowReminderToDelete); | |
| } catch (error) { | |
| console.log(`Error cancelling scheduled Email for reminder ${reminder.id}: ${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 means a single failing
client.request aborts processing of the rest of reminders; change the logic so
each reminder in remindersToCancel is processed inside its own try-catch: for
each reminder call client.request(...) inside a per-iteration try block,
handle/log errors for that reminder in the catch and continue to the next
reminder, and only create/push the prisma.workflowReminder.delete
(workflowRemindersToDelete) for reminders you intended to delete (e.g., after a
successful cancel or according to your desired failure policy) so that failures
for one reminder do not prevent others from being cancelled/deleted.
| <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".
The placeholder on line 439 ("Verification code") and the button text on line 456 ("Verify") are hardcoded in English instead of using t("verification_code") and t("verify") respectively. Other strings in this component are properly localized.
<TextField
className=" border-r-transparent"
- placeholder="Verification code"
+ placeholder={t("verification_code")}
value={verificationCode}- Verify
+ {t("verify")}📝 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.
| <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> | |
| <div className="mt-3 flex"> | |
| <TextField | |
| className=" border-r-transparent" | |
| placeholder={t("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={() => { | |
| verifyPhoneNumberMutation.mutate({ | |
| phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "", | |
| code: verificationCode, | |
| }); | |
| }}> | |
| {t("verify")} | |
| </Button> |
🤖 Prompt for AI Agents
In `@packages/features/ee/workflows/components/WorkflowStepContainer.tsx` around
lines 436 - 457, Replace the two hardcoded English strings with i18n keys: use
the translation function for the TextField placeholder and the Button label in
the WorkflowStepContainer component—update the TextField placeholder from
"Verification code" to t("verification_code") and the Button text from "Verify"
to t("verify"); ensure the component imports/has access to t and leave the rest
of the logic around verificationCode, verifyPhoneNumberMutation,
form.getValues(...) and step.stepNumber unchanged.
| 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.cancelSMS throws, the DB record is never deleted.
Both the Twilio cancellation and the Prisma delete are in the same try block. If cancelSMS fails (e.g., the message was already delivered or Twilio is unreachable), the reminder record will persist in the database, potentially causing repeated cancellation attempts.
Consider either separating the error handling or proceeding to delete regardless:
♻️ 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 via 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, deleteScheduledSMSReminder currently wraps both
twilio.cancelSMS and prisma.workflowReminder.delete in the same try/catch so a
failure in cancelSMS prevents the DB delete; update deleteScheduledSMSReminder
to call twilio.cancelSMS inside its own try/catch (log the Twilio error but
swallow it) and then always call prisma.workflowReminder.delete in a separate
try/catch (or let it bubble) so the reminder record is removed regardless of
Twilio failures; reference the functions twilio.cancelSMS and
prisma.workflowReminder.delete when making the changes.
| // delete scheduled jobs of previous booking | ||
| cancelScheduledJobs(bookingToReschedule); | ||
|
|
||
| //cancel workflow reminders | ||
| const remindersToDelete: PrismaPromise<Prisma.BatchPayload>[] = []; | ||
|
|
||
| //cancel workflow reminders of previous booking | ||
| bookingToReschedule.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.
Soft-delete in the reschedule path may cause stale reminders to fire for a cancelled booking.
The reschedule flow uses the default (no immediateDelete) path, which only sets cancelled: true in the DB without calling the SendGrid cancel API. If the background job that processes cancelled reminders hasn't run by the time the scheduled send is due, the user will receive a reminder for a booking that no longer exists.
Consider passing immediateDelete: true here (same as the workflow-delete path) to cancel the SendGrid scheduled send synchronously, since the booking is definitively being cancelled/rescheduled.
Additionally, the same unawaited-Promise concern flagged in workflows.tsx applies here — the Promises from deleteScheduledEmailReminder / deleteScheduledSMSReminder are discarded by .forEach().
🔧 Proposed fix — immediate cancel + await
//cancel workflow reminders of previous booking
- bookingToReschedule.workflowReminders.forEach((reminder) => {
+ await Promise.allSettled(bookingToReschedule.workflowReminders.map((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
} else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
- });
+ }));🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/bookings.tsx` around lines 484 - 494, The
reschedule path currently soft-deletes reminders without cancelling external
scheduled sends and discards the returned Promises; update the logic around
cancelScheduledJobs and the loop over bookingToReschedule.workflowReminders to
(1) call the external-cancel path with immediateDelete: true when invoking
deleteScheduledEmailReminder / deleteScheduledSMSReminder so SendGrid is
synchronously cancelled, and (2) await the async operations instead of using
.forEach() — e.g., collect the deleteScheduled* calls into an array (or await
cancelScheduledJobs if it returns a Promise) and await Promise.all(...) to
ensure all cancellations complete before returning.
| //cancel workflow reminders of deleted workflow | ||
| scheduledReminders.forEach((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, true); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Unawaited async deletion calls — Promises are silently discarded by forEach.
deleteScheduledEmailReminder and deleteScheduledSMSReminder are async functions, but their returned Promises are dropped by .forEach(). This means:
- The cascade
workflow.deleteManyon line 220 may execute before the SendGrid cancel API calls complete — if the cancel fails, the scheduled email is still delivered but the DB record is already gone. - The mutation response is sent before all cancellations finish, giving the user a false sense of completion.
This same pattern repeats at lines 376-382, 519-525, 574-579, and in bookings.tsx at lines 488-493.
🔧 Proposed fix — collect Promises and await them
- //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.allSettled(
+ 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);
+ }
+ })
+ );Apply the same pattern to the other four call sites.
📝 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 of deleted workflow | |
| scheduledReminders.forEach((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, true); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| }); | |
| //cancel workflow reminders of deleted workflow | |
| await Promise.allSettled( | |
| 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); | |
| } | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/workflows.tsx` around lines 211 - 218,
The scheduledReminders.forEach is dropping Promises from the async functions
deleteScheduledEmailReminder and deleteScheduledSMSReminder, so collect those
Promises (e.g., map scheduledReminders to an array of calls to
deleteScheduledEmailReminder/deleteScheduledSMSReminder) and await them with
await Promise.all(promises) before proceeding to workflow.deleteMany and before
returning the mutation response; apply the same change to the other occurrences
of this pattern in the file (the later blocks around the second and subsequent
reminder-handling sites) and the analogous block in bookings.tsx so all
SendGrid/SMS cancel calls are awaited.
| //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.
🧩 Analysis chain
🏁 Script executed:
# Check if any cleanup/purge logic exists for cancelled workflow reminders
rg -n 'cancelled.*true' --type=ts --type=tsx -g '!*.d.ts' -g '!node_modules/**' | head -30Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 117
🏁 Script executed:
# Check for deleteMany on workflowReminder
rg -n 'workflowReminder\.deleteMany' --type=ts --type=tsx -g '!*.d.ts'Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 117
🏁 Script executed:
# Also check for background jobs, cron jobs, or migrations related to reminders
fd -e ts -e tsx | xargs grep -l 'background\|cron\|schedule.*clean\|purge\|migration' | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1025
🏁 Script executed:
# Look at the actual file to understand context of the soft-delete
head -n 400 packages/trpc/server/routers/viewer/workflows.tsx | tail -n 50Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1849
🏁 Script executed:
# Check how the delete mutation works - search for the delete implementation
rg -B5 -A10 'deleteMany.*workflow' --type=ts --type=tsx | head -50Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 117
🏁 Script executed:
# Search for deleteScheduledEmailReminder and deleteScheduledSMSReminder implementations
rg -n 'deleteScheduledEmailReminder|deleteScheduledSMSReminder' --type=ts | head -30Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 3371
🏁 Script executed:
# Look for where these functions are defined
rg -B5 -A15 'export.*deleteScheduledEmailReminder|function deleteScheduledEmailReminder' --type=ts | head -60Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1864
🏁 Script executed:
# Check for any updates to workflowReminder that set cancelled
rg -n 'workflowReminder.*update|cancelled.*true' --type=ts | head -40Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 612
🏁 Script executed:
# Check for background jobs or cleanup logic
find . -path ./node_modules -prune -o -type f -name '*cron*' -o -name '*background*' -o -name '*clean*' | grep -v node_modulesRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 469
🏁 Script executed:
# Look for workflow-related migrations or jobs
fd 'migration|job|cron|clean' -e ts -e js | grep -v node_modules | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 72
🏁 Script executed:
# Get full deleteScheduledEmailReminder function
sed -n '197,250p' packages/features/ee/workflows/lib/reminders/emailReminderManager.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 870
🏁 Script executed:
# Get full deleteScheduledSMSReminder function
sed -n '177,230p' packages/features/ee/workflows/lib/reminders/smsReminderManager.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 462
🏁 Script executed:
# Search for any cleanup or purge logic for cancelled workflow reminders
rg -n 'cancelled.*false|deleteMany.*workflowReminder|purge.*reminder|cleanup.*reminder' --type=tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 72
🏁 Script executed:
# Check database schema/migrations for workflowReminder
find . -path ./node_modules -prune -o -name '*.prisma' -print | head -5Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 121
🏁 Script executed:
# Look at prisma schema for workflowReminder
rg -B3 -A15 'model WorkflowReminder' --type=prismaRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 120
🏁 Script executed:
# Check the prisma schema file directly
cat packages/prisma/schema.prisma | grep -A20 "model WorkflowReminder"Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 670
🏁 Script executed:
# Search for any cleanup or scheduled jobs that might handle cancelled reminders
rg -n 'cancelled|workflowReminder' packages/features/ee/workflows --type=ts | grep -i 'delete\|clean\|purge' | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1087
🏁 Script executed:
# Check the workflow reminder cron jobs mentioned in the github workflows
cat .github/workflows/cron-scheduleEmailReminders.ymlRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 925
🏁 Script executed:
# Also check scheduleEmailReminders implementation to understand the full picture
head -n 100 packages/features/ee/workflows/api/scheduleEmailReminders.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 3162
🏁 Script executed:
# Get more context around the cancelled reminder cleanup in scheduleEmailReminders
sed -n '40,80p' packages/features/ee/workflows/api/scheduleEmailReminders.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1228
🏁 Script executed:
# Check scheduleSMSReminders to see if it has similar cleanup logic
cat packages/features/ee/workflows/api/scheduleSMSReminders.ts | head -150Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 5048
Email reminders are soft-deleted without immediate cleanup — scheduled far in the future will accumulate.
When event types are removed from a workflow, email reminders are soft-deleted (cancelled: true) but only cleaned up when approaching their scheduled date (within 1-hour window in scheduleEmailReminders cron). Reminders scheduled far in the future remain in the database until their scheduled date nears, causing table bloat over time. SMS reminders are hard-deleted immediately and don't have this issue.
Consider cleaning up cancelled = true records more aggressively (e.g., delete all cancelled reminders regardless of scheduled date), or switch to hard-delete for this path if soft-delete semantics aren't needed.
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/workflows.tsx` around lines 375 - 382,
The current loop cancels email reminders but leaves soft-deleted rows until
their scheduled time, causing table bloat; update the cancellation path in the
remindersToDelete.flat().forEach block so that when reminder.method ===
WorkflowMethods.EMAIL you perform an immediate hard-delete instead of a
soft-cancel: either call an existing hard-delete helper (e.g.,
deleteScheduledEmailReminderImmediate or deleteScheduledEmailReminder with a
force/hard flag) or implement a new deleteScheduledEmailReminderHard function
that removes the DB row and invoke it here; keep deleteScheduledSMSReminder
unchanged for SMS.
| // cancel all workflow reminders from deleted steps | ||
| if (remindersFromStep.length > 0) { | ||
| remindersFromStep.forEach((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, true); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Reminder cancellation for deleted steps is also fire-and-forget, but step deletion follows immediately.
Lines 520-524 call deleteScheduledEmailReminder(..., true) / deleteScheduledSMSReminder(...) without awaiting, then line 527 deletes the workflow step from the DB. If cascade deletes the reminders before the SendGrid cancel completes, the in-flight cancel will succeed or fail independently — but there's no guarantee the emails are actually stopped before the records vanish.
Same fix as suggested on lines 211-218: collect and await the Promises before deleting the step.
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/workflows.tsx` around lines 517 - 526,
The reminder cancellation calls inside the remindersFromStep loop are currently
fire-and-forget; collect the Promises returned by deleteScheduledEmailReminder
and deleteScheduledSMSReminder (checking reminder.method ===
WorkflowMethods.EMAIL) into an array, await Promise.all on that array to ensure
all cancel requests complete, then proceed to delete the workflow step from the
DB; update the block that iterates remindersFromStep to push each delete promise
and await them before the step deletion.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements