Comprehensive workflow reminder management for booking lifecycle events#4
Draft
ShashankFC wants to merge 1 commit into
Draft
Comprehensive workflow reminder management for booking lifecycle events#4ShashankFC wants to merge 1 commit into
ShashankFC wants to merge 1 commit into
Conversation
…re still sent (#7232) * small UI fix * fix cancelling scheduled emails * improve comments * delete reminders for rescheduled bookings * add migration file * cancel rescheduled bookings immediately * remove immediate delete for request reschedule --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com>
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
📝 WalkthroughWalkthroughThis PR refactors reminder management to handle cancellation through direct manager functions passing both reminder IDs and reference IDs, replacing batch Prisma operations. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In @packages/features/bookings/lib/handleCancelBooking.ts:
- Around line 484-493: The reminder deletion loop is calling
deleteScheduledEmailReminder and deleteScheduledSMSReminder without awaiting
them; update the handling in the updatedBookings.forEach block so each call is
awaited (or collect promises and await Promise.all) to ensure deletions complete
and errors surface before proceeding—modify the loop that iterates
updatedBookings and booking.workflowReminders to either use async/await (e.g.,
make the outer iterator async and await each
deleteScheduledEmailReminder/deleteScheduledSMSReminder) or push the returned
promises and await Promise.all on that array, and preserve references to the
existing functions deleteScheduledEmailReminder and deleteScheduledSMSReminder
and the booking.workflowReminders structure.
In @packages/features/bookings/lib/handleNewBooking.ts:
- Around line 964-975: The current loop over
originalRescheduledBooking.workflowReminders invokes async functions
deleteScheduledEmailReminder and deleteScheduledSMSReminder inside a forEach
without awaiting, so cancellations run fire-and-forget and errors escape the
try-catch; change this to collect the promises and await them (e.g., const
promises = originalRescheduledBooking.workflowReminders.map(r => r.method ===
WorkflowMethods.EMAIL ? deleteScheduledEmailReminder(r.id, r.referenceId, true)
: deleteScheduledSMSReminder(r.id, r.referenceId)); await
Promise.all(promises)); ensure the enclosing function (e.g., handleNewBooking)
is async so the await is valid and errors propagate to the existing catch for
proper handling.
In @packages/features/ee/workflows/api/scheduleEmailReminders.ts:
- Around line 44-51: The cancellation query fetches all cancelled reminders but
needs to be restricted to email reminders with a non-null referenceId to avoid
SendGrid calls with null batch_id; update the prisma.workflowReminder.findMany
call (variable remindersToCancel) to include where.method === "EMAIL" (or the
enum/constant used) and where.referenceId is not null while keeping cancelled
and scheduledDate filters so only cancelled email reminders that have a
referenceId are returned.
- Around line 53-77: The loop over remindersToCancel stops on the first API
error because each iteration awaits client.request; change this so each reminder
is processed independently and concurrently using Promise.allSettled: for each
reminder in remindersToCancel create an async task that calls client.request({
url: "/v3/user/scheduled_sends", method: "POST", body: { batch_id:
reminder.referenceId, status: "cancel" } }), logs any per-reminder API error
(including reminder.id/referenceId) but continues, and if the cancel succeeds
enqueues or runs prisma.workflowReminder.delete({ where: { id: reminder.id } })
and logs any delete error; then await Promise.allSettled on all tasks and
handle/log failures rather than letting one failure abort the whole operation
(refer to remindersToCancel, client.request, prisma.workflowReminder.delete, and
workflowRemindersToDelete).
In @packages/features/ee/workflows/components/WorkflowStepContainer.tsx:
- Around line 435-460: Replace hardcoded UI strings by using the i18n
translation keys for the verification input and button (use
t('verification_code') for the TextField placeholder and t('verify') for the
Button label), remove the unnecessary leading space in the TextField className
(" border-r-transparent" → "border-r-transparent"), and make the Verify Button
disabled both when verifyPhoneNumberMutation.isLoading OR verificationCode is
empty (i.e., disable when !verificationCode ||
verifyPhoneNumberMutation.isLoading) so users cannot submit blank codes; keep
the existing onClick that mutates verifyPhoneNumberMutation with
form.getValues(...) and verificationCode.
In @packages/trpc/server/routers/viewer/bookings.tsx:
- Around line 487-494: The reminder deletion calls inside
bookingToReschedule.workflowReminders.forEach are not awaited, so
deleteScheduledEmailReminder and deleteScheduledSMSReminder may not complete;
replace the forEach with an async-aware loop or collect promises (e.g., map to
calls to deleteScheduledEmailReminder/deleteScheduledSMSReminder) and await them
(await Promise.all(promises) or use for...of with await) so all scheduled
reminder deletions complete before proceeding.
In @packages/trpc/server/routers/viewer/workflows.tsx:
- Around line 211-218: The scheduledReminders.forEach block fires async
deletions without awaiting them; change it to await the deletions by iterating
with for...of (e.g., for (const reminder of scheduledReminders)) or map to
Promises and await Promise.all, and call
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) and
deleteScheduledSMSReminder(reminder.id, reminder.referenceId) accordingly so
reminders are actually canceled before workflow deletion completes; reference
the scheduledReminders.forEach, deleteScheduledEmailReminder,
deleteScheduledSMSReminder, and WorkflowMethods symbols when applying the fix.
- Around line 517-526: The deletion loop over remindersFromStep is not awaiting
async deletions (using forEach), so ensure you await the calls to
deleteScheduledEmailReminder and deleteScheduledSMSReminder; replace the forEach
with either a Promise.all(remindersFromStep.map(...)) that returns the delete
promise for each reminder or a for...of loop that awaits each call, referencing
the existing remindersFromStep array and WorkflowMethods enum and calling
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) or
deleteScheduledSMSReminder(reminder.id, reminder.referenceId) as appropriate so
all scheduled-reminder deletions complete before proceeding.
- Around line 572-580: The callback passed to remindersToUpdate.forEach is async
but not awaited, so scheduled-deletion can run unhandled or out of order;
replace the forEach with an awaited iteration (either await
Promise.all(remindersToUpdate.map(...)) or a for...of loop) and await
deleteScheduledEmailReminder/deleteScheduledSMSReminder calls accordingly,
ensuring the enclosing function is async so the deletions complete before
proceeding; reference remindersToUpdate, WorkflowMethods,
deleteScheduledEmailReminder, and deleteScheduledSMSReminder when making the
change.
- Around line 375-382: The cancellation loop over remindersToDelete uses forEach
and calls deleteScheduledEmailReminder/deleteScheduledSMSReminder without
awaiting them, which can cause race conditions; change it to collect and await
the async calls (e.g., await Promise.all(remindersToDelete.flat().map(reminder
=> reminder.method === WorkflowMethods.EMAIL ?
deleteScheduledEmailReminder(reminder.id, reminder.referenceId) :
deleteScheduledSMSReminder(reminder.id, reminder.referenceId)))) or use an async
for...of that awaits each delete, ensuring reminders are fully cancelled before
proceeding with the workflow update.
🧹 Nitpick comments (2)
packages/features/ee/workflows/components/WorkflowStepContainer.tsx (1)
392-429: Phone input section looks good overall.The phone number input and "send code" button are correctly implemented with proper state management and error handling.
Minor optimization: Remove unnecessary array copy
On line 403, the
.concat([])creates an unnecessary shallow copy before calling.find(). Since the array isn't being modified, you can call.find()directly:onChange={() => { const isAlreadyVerified = !!verifiedNumbers - ?.concat([]) .find((number) => number === form.getValues(`steps.${step.stepNumber - 1}.sendTo`)); setNumberVerified(isAlreadyVerified); }}packages/prisma/schema.prisma (1)
644-644: Consider adding a default value for the cancelled field.The
cancelledfield is nullable without a default. For consistency with thescheduledfield (line 641) which is a non-nullable Boolean, consider whethercancelledshould default tofalseto simplify queries and make the intent clearer.♻️ Proposed change
- cancelled Boolean? + cancelled Boolean @default(false)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/features/bookings/lib/handleCancelBooking.tspackages/features/bookings/lib/handleNewBooking.tspackages/features/ee/workflows/api/scheduleEmailReminders.tspackages/features/ee/workflows/components/WorkflowStepContainer.tsxpackages/features/ee/workflows/lib/reminders/emailReminderManager.tspackages/features/ee/workflows/lib/reminders/smsReminderManager.tspackages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sqlpackages/prisma/schema.prismapackages/trpc/server/routers/viewer/bookings.tsxpackages/trpc/server/routers/viewer/workflows.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
packages/features/ee/workflows/lib/reminders/smsReminderManager.ts (2)
packages/features/ee/workflows/lib/reminders/smsProviders/twilioProvider.ts (1)
twilio(8-12)packages/prisma/index.ts (1)
prisma(14-14)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
packages/prisma/index.ts (1)
prisma(14-14)
packages/features/ee/workflows/api/scheduleEmailReminders.ts (1)
packages/prisma/index.ts (1)
prisma(14-14)
packages/trpc/server/routers/viewer/bookings.tsx (3)
packages/app-store/zapier/lib/nodeScheduler.ts (1)
cancelScheduledJobs(54-94)packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
deleteScheduledEmailReminder(197-236)packages/features/ee/workflows/lib/reminders/smsReminderManager.ts (1)
deleteScheduledSMSReminder(177-190)
packages/features/bookings/lib/handleCancelBooking.ts (2)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
deleteScheduledEmailReminder(197-236)packages/features/ee/workflows/lib/reminders/smsReminderManager.ts (1)
deleteScheduledSMSReminder(177-190)
packages/features/ee/workflows/components/WorkflowStepContainer.tsx (6)
packages/ui/index.tsx (4)
Label(20-20)Button(6-6)Badge(3-3)TextField(24-24)packages/ui/form/fields.tsx (2)
Label(28-34)TextField(100-102)packages/features/ee/workflows/pages/workflow.tsx (1)
FormValues(32-39)packages/ui/components/button/Button.tsx (1)
Button(165-267)packages/lib/classNames.ts (1)
classNames(3-5)packages/ui/components/badge/Badge.tsx (1)
Badge(32-62)
packages/features/bookings/lib/handleNewBooking.ts (2)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
deleteScheduledEmailReminder(197-236)packages/features/ee/workflows/lib/reminders/smsReminderManager.ts (1)
deleteScheduledSMSReminder(177-190)
🔇 Additional comments (7)
packages/features/ee/workflows/components/WorkflowStepContainer.tsx (2)
390-390: LGTM - Intentional scope narrowing for phone number UI.The condition has been intentionally narrowed to render the phone number input block only when a phone number is specifically needed, rather than when either a phone number or sender ID is needed. This aligns with the PR's goal of improving the verification flow.
430-434: LGTM - Verification badge.The verified number badge provides clear visual feedback to users.
packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql (1)
1-2: LGTM!The migration correctly adds the nullable
cancelledcolumn to theWorkflowRemindertable, which is safe for existing data.packages/features/ee/workflows/lib/reminders/smsReminderManager.ts (1)
177-189: LGTM!The updated signature correctly handles both scheduled reminders (with referenceId) and unscheduled reminders (without referenceId). The conditional check before calling
twilio.cancelSMSprevents errors whenreferenceIdis null, and the reminder record is always cleaned up from the database.packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
197-235: Function signature and behavior look reasonable, with one clarification.The three-branch logic provides flexibility:
- No referenceId → direct DB delete (not yet scheduled in SendGrid)
immediateDelete→ cancel immediately in SendGrid (for rescheduling)- Default → mark as cancelled for later cron processing
The design is sound pending the fix for the
immediateDeletepath noted above.packages/features/bookings/lib/handleNewBooking.ts (1)
772-772: LGTM!Adding
workflowReminders: trueto the booking include is necessary for the new cancellation logic.packages/trpc/server/routers/viewer/workflows.tsx (1)
1-10: LGTM!The import reorganization for
PrismaPromiseandPrismais correct and aligns with the new usage patterns.
Comment on lines
+484
to
493
| //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); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing await on reminder cancellation calls.
The calls to deleteScheduledEmailReminder and deleteScheduledSMSReminder (lines 488, 490) are not awaited, which means errors could be silently ignored and the deletion may not complete before proceeding. This could result in reminders not being cancelled if the process terminates early.
🔧 Proposed fix
//Workflows - cancel all reminders for cancelled bookings
- updatedBookings.forEach((booking) => {
+ await Promise.all(updatedBookings.map(async (booking) => {
+ const cancellations = booking.workflowReminders.map((reminder) => {
- booking.workflowReminders.forEach((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
} else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
+ return Promise.resolve();
});
- });
+ return Promise.all(cancellations);
+ }));📝 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.
Suggested change
| //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); | |
| }); | |
| }); | |
| //Workflows - cancel all reminders for cancelled bookings | |
| await Promise.all(updatedBookings.map(async (booking) => { | |
| const cancellations = booking.workflowReminders.map((reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| return deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| return deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| return Promise.resolve(); | |
| }); | |
| return Promise.all(cancellations); | |
| })); |
🤖 Prompt for AI Agents
In @packages/features/bookings/lib/handleCancelBooking.ts around lines 484 -
493, The reminder deletion loop is calling deleteScheduledEmailReminder and
deleteScheduledSMSReminder without awaiting them; update the handling in the
updatedBookings.forEach block so each call is awaited (or collect promises and
await Promise.all) to ensure deletions complete and errors surface before
proceeding—modify the loop that iterates updatedBookings and
booking.workflowReminders to either use async/await (e.g., make the outer
iterator async and await each
deleteScheduledEmailReminder/deleteScheduledSMSReminder) or push the returned
promises and await Promise.all on that array, and preserve references to the
existing functions deleteScheduledEmailReminder and deleteScheduledSMSReminder
and the booking.workflowReminders structure.
Comment on lines
+964
to
+975
| 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); | ||
| } |
Contributor
There was a problem hiding this comment.
Async delete operations inside forEach are not awaited.
The forEach loop calls async functions deleteScheduledEmailReminder and deleteScheduledSMSReminder without await. These will execute as fire-and-forget operations, and any errors won't be caught by the surrounding try-catch. Additionally, the reschedule flow may proceed before reminders are fully cancelled.
🔧 Suggested fix using Promise.all
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);
- }
- });
+ await Promise.all(
+ originalRescheduledBooking.workflowReminders.map(async (reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ await deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ await deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ })
+ );
} 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 loop over originalRescheduledBooking.workflowReminders invokes async
functions deleteScheduledEmailReminder and deleteScheduledSMSReminder inside a
forEach without awaiting, so cancellations run fire-and-forget and errors escape
the try-catch; change this to collect the promises and await them (e.g., const
promises = originalRescheduledBooking.workflowReminders.map(r => r.method ===
WorkflowMethods.EMAIL ? deleteScheduledEmailReminder(r.id, r.referenceId, true)
: deleteScheduledSMSReminder(r.id, r.referenceId)); await
Promise.all(promises)); ensure the enclosing function (e.g., handleNewBooking)
is async so the await is valid and errors propagate to the existing catch for
proper handling.
Comment on lines
+44
to
+51
| const remindersToCancel = await prisma.workflowReminder.findMany({ | ||
| where: { | ||
| cancelled: true, | ||
| scheduledDate: { | ||
| lte: dayjs().add(1, "hour").toISOString(), | ||
| }, | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing filter for EMAIL method and referenceId in cancellation query.
This query fetches all cancelled reminders regardless of method, but this handler is specifically for email reminders. Additionally, reminders without a referenceId would cause the SendGrid API call to fail with null batch_id.
🔧 Suggested fix
const remindersToCancel = await prisma.workflowReminder.findMany({
where: {
cancelled: true,
+ method: WorkflowMethods.EMAIL,
scheduledDate: {
lte: dayjs().add(1, "hour").toISOString(),
},
+ NOT: {
+ referenceId: null,
+ },
},
});📝 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.
Suggested change
| 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(), | |
| }, | |
| NOT: { | |
| referenceId: null, | |
| }, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
In @packages/features/ee/workflows/api/scheduleEmailReminders.ts around lines 44
- 51, The cancellation query fetches all cancelled reminders but needs to be
restricted to email reminders with a non-null referenceId to avoid SendGrid
calls with null batch_id; update the prisma.workflowReminder.findMany call
(variable remindersToCancel) to include where.method === "EMAIL" (or the
enum/constant used) and where.referenceId is not null while keeping cancelled
and scheduledDate filters so only cancelled email reminders that have a
referenceId are returned.
Comment on lines
+53
to
+77
| try { | ||
| const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; | ||
|
|
||
| for (const reminder of remindersToCancel) { | ||
| await client.request({ | ||
| url: "/v3/user/scheduled_sends", | ||
| method: "POST", | ||
| body: { | ||
| batch_id: reminder.referenceId, | ||
| status: "cancel", | ||
| }, | ||
| }); | ||
|
|
||
| const workflowReminderToDelete = prisma.workflowReminder.delete({ | ||
| where: { | ||
| id: reminder.id, | ||
| }, | ||
| }); | ||
|
|
||
| workflowRemindersToDelete.push(workflowReminderToDelete); | ||
| } | ||
| await Promise.all(workflowRemindersToDelete); | ||
| } catch (error) { | ||
| console.log(`Error cancelling scheduled Emails: ${error}`); | ||
| } |
Contributor
There was a problem hiding this comment.
Single API failure halts processing of remaining reminders.
If the SendGrid cancel request fails for one reminder, the loop breaks and remaining cancelled reminders are not processed. Consider processing each reminder independently and logging individual failures.
🔧 Suggested fix using Promise.allSettled for resilience
- 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 cancelResults = await Promise.allSettled(
+ remindersToCancel.map(async (reminder) => {
+ 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}`);
+ }
+ })
+ );📝 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.
Suggested change
| try { | |
| const workflowRemindersToDelete: Prisma.Prisma__WorkflowReminderClient<WorkflowReminder, never>[] = []; | |
| for (const reminder of remindersToCancel) { | |
| await client.request({ | |
| url: "/v3/user/scheduled_sends", | |
| method: "POST", | |
| body: { | |
| batch_id: reminder.referenceId, | |
| status: "cancel", | |
| }, | |
| }); | |
| const workflowReminderToDelete = prisma.workflowReminder.delete({ | |
| where: { | |
| id: reminder.id, | |
| }, | |
| }); | |
| workflowRemindersToDelete.push(workflowReminderToDelete); | |
| } | |
| await Promise.all(workflowRemindersToDelete); | |
| } catch (error) { | |
| console.log(`Error cancelling scheduled Emails: ${error}`); | |
| } | |
| const cancelResults = await Promise.allSettled( | |
| remindersToCancel.map(async (reminder) => { | |
| 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}`); | |
| } | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In @packages/features/ee/workflows/api/scheduleEmailReminders.ts around lines 53
- 77, The loop over remindersToCancel stops on the first API error because each
iteration awaits client.request; change this so each reminder is processed
independently and concurrently using Promise.allSettled: for each reminder in
remindersToCancel create an async task that calls client.request({ url:
"/v3/user/scheduled_sends", method: "POST", body: { batch_id:
reminder.referenceId, status: "cancel" } }), logs any per-reminder API error
(including reminder.id/referenceId) but continues, and if the cancel succeeds
enqueues or runs prisma.workflowReminder.delete({ where: { id: reminder.id } })
and logs any delete error; then await Promise.allSettled on all tasks and
handle/log failures rather than letting one failure abort the whole operation
(refer to remindersToCancel, client.request, prisma.workflowReminder.delete, and
workflowRemindersToDelete).
Comment on lines
435
to
460
| <> | ||
| <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); | ||
| <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> | ||
|
|
||
| {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={() => { | ||
| verifyPhoneNumberMutation.mutate({ | ||
| phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "", | ||
| code: verificationCode, | ||
| }); | ||
| }}> | ||
| Verify | ||
| </Button> | ||
| </div> | ||
| </> | ||
| )} | ||
| </> | ||
| )} |
Contributor
There was a problem hiding this comment.
Fix internationalization and improve UX for verification input.
The verification code input flow works correctly, but has the following issues:
- Lines 439 & 456: Hardcoded English strings "Verification code" and "Verify" break internationalization. Users in other locales will see these in English.
- Line 449: The Verify button should also be disabled when
verificationCodeis empty to prevent users from submitting blank codes. - Line 438: Minor - unnecessary leading space in className.
Proposed fixes
<TextField
- className=" border-r-transparent"
+ className="border-r-transparent"
- placeholder="Verification code"
+ 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}
+ disabled={verifyPhoneNumberMutation.isLoading || !verificationCode.trim()}
onClick={() => {
verifyPhoneNumberMutation.mutate({
phoneNumber: form.getValues(`steps.${step.stepNumber - 1}.sendTo`) || "",
code: verificationCode,
});
}}>
- Verify
+ {t("verify")}
</Button>Note: Ensure that the translation keys verification_code and verify are added to your localization files.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @packages/features/ee/workflows/components/WorkflowStepContainer.tsx around
lines 435 - 460, Replace hardcoded UI strings by using the i18n translation keys
for the verification input and button (use t('verification_code') for the
TextField placeholder and t('verify') for the Button label), remove the
unnecessary leading space in the TextField className (" border-r-transparent" →
"border-r-transparent"), and make the Verify Button disabled both when
verifyPhoneNumberMutation.isLoading OR verificationCode is empty (i.e., disable
when !verificationCode || verifyPhoneNumberMutation.isLoading) so users cannot
submit blank codes; keep the existing onClick that mutates
verifyPhoneNumberMutation with form.getValues(...) and verificationCode.
Comment on lines
+487
to
494
| //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); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing await on reminder cancellation calls.
Similar to the issue in handleCancelBooking.ts, the calls to deleteScheduledEmailReminder and deleteScheduledSMSReminder (lines 490, 492) are not awaited. This could result in reminders not being properly cancelled if an error occurs or the process completes before the deletions finish.
🔧 Proposed fix
//cancel workflow reminders of previous booking
- bookingToReschedule.workflowReminders.forEach((reminder) => {
+ await Promise.all(bookingToReschedule.workflowReminders.map((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
} else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
- });
+ 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.
Suggested change
| //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); | |
| }); | |
| //cancel workflow reminders of previous booking | |
| await Promise.all(bookingToReschedule.workflowReminders.map((reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| return deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| return deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| return Promise.resolve(); | |
| })); |
🤖 Prompt for AI Agents
In @packages/trpc/server/routers/viewer/bookings.tsx around lines 487 - 494, The
reminder deletion calls inside bookingToReschedule.workflowReminders.forEach are
not awaited, so deleteScheduledEmailReminder and deleteScheduledSMSReminder may
not complete; replace the forEach with an async-aware loop or collect promises
(e.g., map to calls to deleteScheduledEmailReminder/deleteScheduledSMSReminder)
and await them (await Promise.all(promises) or use for...of with await) so all
scheduled reminder deletions complete before proceeding.
Comment on lines
+211
to
218
| //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); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
Async delete operations inside forEach are not awaited.
Same issue as in handleNewBooking.ts: the async deleteScheduledEmailReminder and deleteScheduledSMSReminder calls are not awaited within forEach, making them fire-and-forget. The workflow deletion may complete before reminders are cancelled.
🔧 Suggested fix
- //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(async (reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ await deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ await deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ })
+ );📝 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.
Suggested change
| //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.all( | |
| scheduledReminders.map(async (reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| await deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| await deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In @packages/trpc/server/routers/viewer/workflows.tsx around lines 211 - 218,
The scheduledReminders.forEach block fires async deletions without awaiting
them; change it to await the deletions by iterating with for...of (e.g., for
(const reminder of scheduledReminders)) or map to Promises and await
Promise.all, and call deleteScheduledEmailReminder(reminder.id,
reminder.referenceId, true) and deleteScheduledSMSReminder(reminder.id,
reminder.referenceId) accordingly so reminders are actually canceled before
workflow deletion completes; reference the scheduledReminders.forEach,
deleteScheduledEmailReminder, deleteScheduledSMSReminder, and WorkflowMethods
symbols when applying the fix.
Comment on lines
+375
to
382
| //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); | ||
| }); |
Contributor
There was a problem hiding this comment.
Same async/await issue in forEach for disabled event types.
The async delete operations are not awaited, which could lead to race conditions where the workflow update proceeds before reminders are properly marked as cancelled.
🔧 Suggested 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(async (reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ await deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ await deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ })
+ );📝 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.
Suggested change
| //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(async (reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| await deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| await deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In @packages/trpc/server/routers/viewer/workflows.tsx around lines 375 - 382,
The cancellation loop over remindersToDelete uses forEach and calls
deleteScheduledEmailReminder/deleteScheduledSMSReminder without awaiting them,
which can cause race conditions; change it to collect and await the async calls
(e.g., await Promise.all(remindersToDelete.flat().map(reminder =>
reminder.method === WorkflowMethods.EMAIL ?
deleteScheduledEmailReminder(reminder.id, reminder.referenceId) :
deleteScheduledSMSReminder(reminder.id, reminder.referenceId)))) or use an async
for...of that awaits each delete, ensuring reminders are fully cancelled before
proceeding with the workflow update.
Comment on lines
+517
to
526
| // 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); | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Same async/await issue for deleted steps.
🔧 Suggested 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(async (reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ await deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ await deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ })
+ );
+ }📝 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.
Suggested change
| // 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); | |
| } | |
| }); | |
| } | |
| // cancel all workflow reminders from deleted steps | |
| if (remindersFromStep.length > 0) { | |
| await Promise.all( | |
| remindersFromStep.map(async (reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| await deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| await deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| }) | |
| ); | |
| } |
🤖 Prompt for AI Agents
In @packages/trpc/server/routers/viewer/workflows.tsx around lines 517 - 526,
The deletion loop over remindersFromStep is not awaiting async deletions (using
forEach), so ensure you await the calls to deleteScheduledEmailReminder and
deleteScheduledSMSReminder; replace the forEach with either a
Promise.all(remindersFromStep.map(...)) that returns the delete promise for each
reminder or a for...of loop that awaits each call, referencing the existing
remindersFromStep array and WorkflowMethods enum and calling
deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true) or
deleteScheduledSMSReminder(reminder.id, reminder.referenceId) as appropriate so
all scheduled-reminder deletions complete before proceeding.
Comment on lines
+572
to
580
|
|
||
| //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, | ||
| }, | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Same async/await issue for edited steps.
🔧 Suggested 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(async (reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ await deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ } else if (reminder.method === WorkflowMethods.SMS) {
+ await deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ })
+ );📝 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.
Suggested 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, | |
| }, | |
| }); | |
| }); | |
| //cancel all workflow reminders from steps that were edited | |
| await Promise.all( | |
| remindersToUpdate.map(async (reminder) => { | |
| if (reminder.method === WorkflowMethods.EMAIL) { | |
| await deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| } else if (reminder.method === WorkflowMethods.SMS) { | |
| await deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | |
| } | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In @packages/trpc/server/routers/viewer/workflows.tsx around lines 572 - 580,
The callback passed to remindersToUpdate.forEach is async but not awaited, so
scheduled-deletion can run unhandled or out of order; replace the forEach with
an awaited iteration (either await Promise.all(remindersToUpdate.map(...)) or a
for...of loop) and await deleteScheduledEmailReminder/deleteScheduledSMSReminder
calls accordingly, ensuring the enclosing function is async so the deletions
complete before proceeding; reference remindersToUpdate, WorkflowMethods,
deleteScheduledEmailReminder, and deleteScheduledSMSReminder when making the
change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test 6nn
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/cal.com-coderabbit#6](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/6)*