Comprehensive workflow reminder management for booking lifecycle events#6
Comprehensive workflow reminder management for booking lifecycle events#6akshayutture-augment 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>
|
@coderabbitai review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes introduce a cancelled flag to workflow reminders and refactor reminder cancellation across booking and workflow management layers. Reminder deletion shifts from batch Prisma operations to direct per-reminder function calls that accept reminderId and referenceId. Phone number verification UI in the workflow component is restructured with dedicated verification input and button. A new migration adds the cancelled column to the WorkflowReminder table. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as WorkflowStepContainer
participant API as verifyPhoneNumber API
User->>UI: Enter phone number
User->>UI: Click "Send Code"
UI->>API: send_code action
User->>UI: Enter verification code
User->>UI: Click "Verify"
UI->>API: verifyPhoneNumber mutation
alt Verification Success
API-->>UI: Success response
UI->>UI: Show "number_verified" badge
else Verification Fails
API-->>UI: Error response
UI->>UI: Display error message
end
sequenceDiagram
participant Booking as Booking Handler
participant ReminderMgr as Reminder Manager
participant Prisma as Database
participant ExtAPI as External API
Booking->>Booking: Reschedule booking
Booking->>Prisma: Fetch original booking + workflowReminders
loop For each workflow reminder
Booking->>ReminderMgr: deleteScheduledEmailReminder(id, referenceId)
alt referenceId exists
ReminderMgr->>ExtAPI: POST /v3/user/scheduled_sends (cancel)
ExtAPI-->>ReminderMgr: Cancel confirmation
ReminderMgr->>Prisma: Update reminder.cancelled = true
else referenceId is null
ReminderMgr->>Prisma: Delete reminder by id
end
end
Booking->>Booking: Continue rescheduling flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/features/ee/workflows/api/scheduleEmailReminders.ts (1)
10-11: Tighten cancelled‑reminder query and error handlingThe cancellation flow is generally correct, but a few small tweaks would make it more robust and future‑proof:
- Explicitly filter by
WorkflowMethods.EMAILso usingcancelledfor other methods in future won’t accidentally call SendGrid for non‑email reminders.- Guard against a missing
referenceIdbefore calling the SendGrid cancel API.- Consider per‑reminder error handling so one failing cancel doesn’t prevent processing of remaining reminders.
Example adjustment:
- 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(), + }, + }, + }); - for (const reminder of remindersToCancel) { - await client.request({ + for (const reminder of remindersToCancel) { + if (!reminder.referenceId) { + continue; + } + await client.request({ url: "/v3/user/scheduled_sends", method: "POST", body: { batch_id: reminder.referenceId, status: "cancel", }, });Also applies to: 43-77
packages/features/ee/workflows/components/WorkflowStepContainer.tsx (1)
390-422: Guard against sending verification codes with empty phone and during in‑flight requestsRight now
sendVerificationCodeMutation.mutateis called even ifsendTois empty, and the “Send code” button is only disabled whennumberVerifiedis true, not when the mutation is loading. This can cause avoidable backend calls and accidental double‑sends.Consider:
- Early‑returning if
!form.getValues(...sendTo)before callingmutate, and/or setting a field error.- Also disabling the button while
sendVerificationCodeMutation.isLoadingto avoid rapid repeated clicks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/features/bookings/lib/handleCancelBooking.ts(1 hunks)packages/features/bookings/lib/handleNewBooking.ts(4 hunks)packages/features/ee/workflows/api/scheduleEmailReminders.ts(2 hunks)packages/features/ee/workflows/components/WorkflowStepContainer.tsx(1 hunks)packages/features/ee/workflows/lib/reminders/emailReminderManager.ts(1 hunks)packages/features/ee/workflows/lib/reminders/smsReminderManager.ts(1 hunks)packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql(1 hunks)packages/prisma/schema.prisma(1 hunks)packages/trpc/server/routers/viewer/bookings.tsx(1 hunks)packages/trpc/server/routers/viewer/workflows.tsx(6 hunks)
🔇 Additional comments (7)
packages/prisma/migrations/20230217230604_add_cancelled_to_workflow_reminder/migration.sql (1)
1-2: Migration/schema alignment looks goodAdding a nullable
cancelled BOOLEANcolumn is a safe, backwards‑compatible change and matches the Prisma field type.packages/prisma/schema.prisma (1)
634-645: WorkflowReminder.cancelled field is consistent with migrationThe new optional
cancelledflag onWorkflowReminderaligns with the SQL migration and the new reminder cancellation flow.packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
197-235: Email reminder deletion logic matches new cancellation modelThe
deleteScheduledEmailReminderbranching correctly handles:
- DB‑only delete when there is no
referenceId.- Immediate remote cancellation when
immediateDeleteis true.- Marking reminders as
cancelledfor later cron‑driven cancellation whenimmediateDeleteis false.Given current usage patterns this looks consistent and safe.
packages/features/bookings/lib/handleNewBooking.ts (2)
1-10: New imports are consistent with usageBringing in
WorkflowMethodsand the email/SMS reminder deletion helpers matches their later usage in the reschedule flow.Also applies to: 39-41
771-773: Including workflowReminders is appropriate for reschedule cleanupAdding
workflowReminders: truetogetOriginalRescheduledBookingprovides the data needed to cancel previous reminders during reschedule.packages/features/ee/workflows/lib/reminders/smsReminderManager.ts (1)
177-190: SMS reminder deletion helper is correctly implemented
deleteScheduledSMSRemindercleanly handles both cases (with and withoutreferenceId) and ensures theworkflowReminderrow is removed after any Twilio cancellation.packages/trpc/server/routers/viewer/bookings.tsx (1)
30-31: Reminder deletion helper imports are consistentImporting
deleteScheduledEmailReminderanddeleteScheduledSMSReminderfrom the EE workflows layer fits their use inrequestReschedule.
| //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.
Async reminder cancellations during booking cancel are not awaited
The new reminder‑cancellation loop:
updatedBookings.forEach((booking) => {
booking.workflowReminders.forEach((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
} else if (reminder.method === WorkflowMethods.SMS) {
deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
});
});calls async helpers without awaiting them, so:
- Failures become unhandled rejections.
Promise.all(prismaPromises.concat(apiDeletes))does not wait for reminder cancellations.
Refactor to collect and await the reminder promises:
- //Workflows - cancel all reminders for cancelled bookings
- updatedBookings.forEach((booking) => {
- booking.workflowReminders.forEach((reminder) => {
- if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
- } else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
- }
- });
- });
-
- const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes];
-
- await Promise.all(prismaPromises.concat(apiDeletes));
+ // Workflows - cancel all reminders for cancelled bookings
+ const reminderCancels = updatedBookings.flatMap((booking) =>
+ booking.workflowReminders.map((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ }
+ if (reminder.method === WorkflowMethods.SMS) {
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ return Promise.resolve();
+ })
+ );
+
+ const prismaPromises: Promise<unknown>[] = [attendeeDeletes, bookingReferenceDeletes];
+
+ await Promise.all([...prismaPromises, ...apiDeletes, ...reminderCancels]);Also applies to: 495-497
🤖 Prompt for AI Agents
In packages/features/bookings/lib/handleCancelBooking.ts around lines 484-493
(and similarly at 495-497) the reminder cancellation loop calls async helpers
(deleteScheduledEmailReminder/deleteScheduledSMSReminder) without collecting or
awaiting their returned promises, causing unhandled rejections and making the
overall cancel flow not wait for deletions; fix by creating an array (e.g.,
reminderDeletes), push each call's promise into it instead of calling directly,
then await Promise.all(reminderDeletes) (or include reminderDeletes into the
existing Promise.all by concatenating) so all reminder cancellations are awaited
and errors propagate.
| if (originalRescheduledBooking?.uid) { | ||
| try { | ||
| // cancel workflow reminders from previous rescheduled booking | ||
| originalRescheduledBooking.workflowReminders.forEach((reminder) => { | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); | ||
| } else if (reminder.method === WorkflowMethods.SMS) { | ||
| deleteScheduledSMSReminder(reminder.id, reminder.referenceId); | ||
| } | ||
| }); | ||
| } catch (error) { | ||
| log.error("Error while canceling scheduled workflow reminders", error); | ||
| } |
There was a problem hiding this comment.
Async reminder deletions on reschedule are not awaited
In the reschedule path, async helpers are called inside forEach without awaiting:
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);
}
});Issues:
- Promises are fire‑and‑forget; failures become unhandled rejections.
- The handler proceeds without knowing whether cancellations actually completed.
Refactor to collect and await the promises:
- 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.all(
+ originalRescheduledBooking.workflowReminders.map((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
+ }
+ if (reminder.method === WorkflowMethods.SMS) {
+ return deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
+ }
+ return Promise.resolve();
+ })
+ );
+ } catch (error) {
+ log.error("Error while canceling scheduled workflow reminders", error);
+ }🤖 Prompt for AI Agents
In packages/features/bookings/lib/handleNewBooking.ts around lines 963 to 975,
async deletion helpers for workflow reminders are invoked inside a forEach and
not awaited, causing fire-and-forget Promises and unhandled failures; change the
loop to map reminders to Promises (calling deleteScheduledEmailReminder or
deleteScheduledSMSReminder accordingly), collect them into an array, and await
Promise.all (or Promise.allSettled if you want to tolerate individual failures)
inside the try block, and preserve the existing error logging in the catch.
| <> | ||
| <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> |
There was a problem hiding this comment.
Use an error toast for wrong codes and add minimal client‑side validation
In verifyPhoneNumberMutation.onSuccess, a wrong code currently shows t("wrong_code") with a "success" toast variant. That’s confusing feedback. Also, the Verify button allows submitting with an empty verificationCode and without a phone number guard.
Recommend:
- Switching the toast variant for the
!isVerifiedbranch to an error/severity style. - Adding simple guards before
verifyPhoneNumberMutation.mutate, e.g. bail out (or set a form error) when!form.getValues(...sendTo)or!verificationCode.trim().
🤖 Prompt for AI Agents
In packages/features/ee/workflows/components/WorkflowStepContainer.tsx around
lines 435 to 457, the verification success handler currently shows
t("wrong_code") with a "success" toast and the Verify button allows submitting
with empty phone or code; change the toast variant in the !isVerified branch to
an error/severity style (e.g., "error" or "danger") so wrong codes show an
error, and add minimal client-side guards before calling
verifyPhoneNumberMutation.mutate: check that
form.getValues(`steps.${step.stepNumber - 1}.sendTo`) is non-empty and that
verificationCode.trim() is non-empty, and if missing either bail out and show a
user-facing error toast or set a form error via the form API; keep the mutation
call only when both checks pass.
| 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.
Async reminder deletions in requestReschedule are not awaited
In requestReschedule, workflow reminders are cancelled like this:
//cancel workflow reminders of previous booking
bookingToReschedule.workflowReminders.forEach((reminder) => {
if (reminder.method === WorkflowMethods.EMAIL) {
deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
} else if (reminder.method === WorkflowMethods.SMS) {
deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
});Because the async helpers are not awaited:
- Errors become unhandled rejections.
- The mutation resolves before cancellations complete, so follow‑up behaviour can be inconsistent.
Refactor to await all cancellations:
- //cancel workflow reminders of previous booking
- bookingToReschedule.workflowReminders.forEach((reminder) => {
- if (reminder.method === WorkflowMethods.EMAIL) {
- deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
- } else if (reminder.method === WorkflowMethods.SMS) {
- deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
- }
- });
+ // cancel workflow reminders of previous booking
+ await Promise.all(
+ bookingToReschedule.workflowReminders.map((reminder) => {
+ if (reminder.method === WorkflowMethods.EMAIL) {
+ return deleteScheduledEmailReminder(reminder.id, reminder.referenceId);
+ }
+ 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 488 to 495, the
code calls deleteScheduledEmailReminder/deleteScheduledSMSReminder inside a
forEach without awaiting them, causing unhandled rejections and the mutation to
resolve before cancellations complete; change the forEach to build an array of
promises (map over workflowReminders creating a promise for each delete call
depending on reminder.method), then await Promise.all(promises) so all
cancellations are awaited; ensure the enclosing function is async (or already
async) and consider catching/logging errors from Promise.all if needed.
| //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.
Await reminder cancellation helpers instead of firing them in forEach without handling errors
Across several places you’re calling deleteScheduledEmailReminder / deleteScheduledSMSReminder inside Array.forEach and not awaiting the result:
- Delete workflow: lines 211–218.
- Disable event types: lines 375–382.
- Delete steps: lines 519–525.
- Edit steps: lines 573–579.
Given these helpers are likely async (they previously replaced direct Prisma deletes / batch operations), this pattern means:
- The mutation can return before cancellations complete.
- Any rejection from these helpers is effectively dropped and can’t be surfaced or retried.
- Database changes inside the helpers might race with later Prisma writes in the same mutation.
Safer patterns:
// Example pattern for one of these blocks
const cancellations: Promise<unknown>[] = [];
for (const reminder of scheduledReminders) {
if (reminder.method === WorkflowMethods.EMAIL) {
cancellations.push(deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true));
} else if (reminder.method === WorkflowMethods.SMS) {
cancellations.push(deleteScheduledSMSReminder(reminder.id, reminder.referenceId));
}
}
await Promise.all(cancellations);or, if you prefer sequential behavior:
for (const reminder of scheduledReminders) {
if (reminder.method === WorkflowMethods.EMAIL) {
await deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true);
} else if (reminder.method === WorkflowMethods.SMS) {
await deleteScheduledSMSReminder(reminder.id, reminder.referenceId);
}
}This ensures cancellations finish (or fail visibly) before the mutation completes and avoids silent errors.
Also applies to: 375-382, 519-525, 573-579
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/workflows.tsx around lines 211-218 (also
apply same fix at 375-382, 519-525, 573-579): currently
deleteScheduledEmailReminder / deleteScheduledSMSReminder are invoked inside
Array.forEach without awaiting, which drops rejections and allows the mutation
to return before cancellations complete; replace the forEach with either (a)
collect the returned promises into an array and await Promise.all on them, or
(b) use a for...of loop and await each call sequentially, and ensure any thrown
errors are allowed to propagate (or explicitly rethrow) so failures are visible
to the caller.
Test 6
Summary by CodeRabbit
New Features
Bug Fixes
Replicated from ai-code-review-evaluation/cal.com-coderabbit#6