Comprehensive workflow reminder management for booking lifecycle events#7
Open
ShashankFC wants to merge 1 commit into
Open
Comprehensive workflow reminder management for booking lifecycle events#7ShashankFC 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>
Author
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
codeant-ai
Bot
added
the
size:L
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+402
to
+404
| const isAlreadyVerified = !!verifiedNumbers | ||
| ?.concat([]) | ||
| .find((number) => number === form.getValues(`steps.${step.stepNumber - 1}.sendTo`)); |
There was a problem hiding this comment.
Suggestion: The use of optional chaining with verifiedNumbers?.concat([]).find(...) is unsafe: if verifiedNumbers is undefined (e.g. the query has not returned yet), verifiedNumbers?.concat([]) evaluates to undefined, and the subsequent .find(...) call on undefined will throw a runtime TypeError when the phone input changes. [possible bug]
Severity Level: Critical 🚨
- ❌ SMS workflow step editor crashes while typing phone number.
- ⚠️ Admins may be blocked configuring SMS reminder workflows.
Suggested change
| const isAlreadyVerified = !!verifiedNumbers | |
| ?.concat([]) | |
| .find((number) => number === form.getValues(`steps.${step.stepNumber - 1}.sendTo`)); | |
| const currentNumber = form.getValues(`steps.${step.stepNumber - 1}.sendTo`); | |
| const isAlreadyVerified = | |
| Array.isArray(verifiedNumbers) && currentNumber | |
| ? verifiedNumbers.includes(currentNumber) | |
| : false; |
Steps of Reproduction ✅
1. Open the workflows details UI that renders `WorkflowDetailsPage` at
`packages/features/ee/workflows/components/WorkflowDetailsPage.tsx:26-88`, which in turn
renders `WorkflowStepContainer` at lines 132-143 for each step.
2. Ensure there is a workflow step whose `action` is `WorkflowActions.SMS_NUMBER`, so that
`isPhoneNumberNeeded` is `true` per `WorkflowStepContainer` at
`packages/features/ee/workflows/components/WorkflowStepContainer.tsx:68-70` and the phone
number input block at lines 390-407 is rendered.
3. When the page first loads, the TRPC query
`trpc.viewer.workflows.getVerifiedNumbers.useQuery()` at `WorkflowStepContainer.tsx:61-62`
has not yet resolved, so `_verifiedNumbers` is `undefined` and therefore `verifiedNumbers`
is `undefined`.
4. Before the `getVerifiedNumbers` query finishes loading, type into the `PhoneInput`
rendered at `WorkflowStepContainer.tsx:394-407`; its `onChange` handler at lines 401-406
executes, evaluates `verifiedNumbers?.concat([])` to `undefined`, then calls `.find(...)`
on `undefined`, causing a runtime `TypeError` and breaking the React component for the
workflow step editor.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/features/ee/workflows/components/WorkflowStepContainer.tsx
**Line:** 402:404
**Comment:**
*Possible Bug: The use of optional chaining with `verifiedNumbers?.concat([]).find(...)` is unsafe: if `verifiedNumbers` is undefined (e.g. the query has not returned yet), `verifiedNumbers?.concat([])` evaluates to `undefined`, and the subsequent `.find(...)` call on `undefined` will throw a runtime TypeError when the phone input changes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| deleteScheduledSMSReminder(reminder.referenceId); | ||
| } | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); |
There was a problem hiding this comment.
Suggestion: When rescheduling a booking, email workflow reminders are "cancelled" only in the database but the SendGrid scheduled send is never actually cancelled, because deleteScheduledEmailReminder is called without the immediateDelete flag; this can result in reminder emails still being sent for rescheduled bookings. [logic error]
Severity Level: Major ⚠️
- ❌ Attendees receive reminders for original, now-invalid booking time.
- ⚠️ Hosts may receive duplicate or conflicting workflow reminder emails.
- ⚠️ Undermines trust in automated workflow reminder management workflows.
Suggested change
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); |
Steps of Reproduction ✅
1. Configure an event type with a workflow email reminder triggered BEFORE_EVENT or
AFTER_EVENT (workflow configuration handled via `workflowsRouter.update` in
`packages/trpc/server/routers/viewer/workflows.tsx:232-847`, which ultimately leads to
scheduling reminders through `scheduleEmailReminder`).
2. Create and confirm a booking for that event type so that workflow reminders are
scheduled. This flows through booking creation/confirmation code which calls
`scheduleWorkflowReminders` (e.g., `handleNewBooking` at
`packages/features/bookings/lib/handleNewBooking.ts:1269-1276` and
`bookingsRouter.confirm` at `packages/trpc/server/routers/viewer/bookings.tsx:1005-1023`),
which in turn calls `scheduleEmailReminder` in
`packages/features/ee/workflows/lib/reminders/emailReminderManager.ts:29-195`.
3. Ensure the reminder meets SendGrid scheduling conditions so it is provider-scheduled:
`scheduleEmailReminder` checks the BEFORE/AFTER time window and, when within 1–72 hours,
calls `sgMail.send` with `sendAt` and stores `referenceId` (batchId) and `scheduled: true`
in `workflowReminder` (lines `148-177` in `emailReminderManager.ts`), meaning SendGrid
will send this reminder independently of the app.
4. From the logged-in user interface, trigger a reschedule for this booking, which calls
the TRPC mutation `requestReschedule` at
`packages/trpc/server/routers/viewer/bookings.tsx:385-613`. Inside this mutation, the code
at `484-493` calls `deleteScheduledEmailReminder(reminder.id, reminder.referenceId)`
without the `immediateDelete` flag. In `deleteScheduledEmailReminder`
(`emailReminderManager.ts:197-236`), when `referenceId` is set and `immediateDelete` is
falsy, it only marks the DB record as `cancelled: true` and does NOT call the SendGrid
`/v3/user/scheduled_sends` cancel endpoint (lines `213-221`). As a result, when the
original reminder time is reached, SendGrid still delivers the scheduled email for the old
time, despite the booking having been rescheduled.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/trpc/server/routers/viewer/bookings.tsx
**Line:** 490:490
**Comment:**
*Logic Error: When rescheduling a booking, email workflow reminders are "cancelled" only in the database but the SendGrid scheduled send is never actually cancelled, because `deleteScheduledEmailReminder` is called without the `immediateDelete` flag; this can result in reminder emails still being sent for rescheduled bookings.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| deleteScheduledSMSReminder(reminder.referenceId); | ||
| } | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); |
There was a problem hiding this comment.
Suggestion: When disabling a workflow on specific event types, email reminders for those event types are marked as cancelled or deleted in the database but the underlying SendGrid scheduled sends are not cancelled because deleteScheduledEmailReminder is invoked without the immediateDelete flag, so reminders can still fire for disabled workflows. [logic error]
Severity Level: Major ⚠️
- ❌ Disabled workflows still send email reminders via SendGrid.
- ⚠️ Users receive unexpected reminders after configuration changes.
- ⚠️ Admins cannot reliably stop reminders per event type.
Suggested change
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); |
Steps of Reproduction ✅
1. Create a workflow with trigger BEFORE_EVENT or AFTER_EVENT and an email step
(WorkflowActions.EMAIL_HOST/EMAIL_ATTENDEE) on an event type, then create a confirmed
booking for that event type so workflow reminders are scheduled. This flows through
`scheduleWorkflowReminders()` in
`packages/features/ee/workflows/lib/reminders/reminderScheduler.ts:15-94`, which calls
`scheduleEmailReminder()` in
`packages/features/ee/workflows/lib/reminders/emailReminderManager.ts:29-195`.
2. In `scheduleEmailReminder()`, when the trigger is BEFORE_EVENT/AFTER_EVENT and the date
is within the 60-min–72-hour window, an email is scheduled with SendGrid using
`sgMail.send({... sendAt: scheduledDate.unix(), batchId: batchIdResponse[1].batch_id })`
and a `workflowReminder` row is created with `scheduled: true` and `referenceId` set to
the SendGrid `batch_id` (`emailReminderManager.ts:148-178`).
3. Later, disable this workflow only for that event type via the TRPC mutation
`workflowsRouter.update` in `packages/trpc/server/routers/viewer/workflows.tsx:232-847`,
by removing the event type ID from `activeOn`. The handler computes `removedEventTypes`
(`workflows.tsx:331-340`), queries reminders for those event types into
`remindersToDeletePromise` (`workflows.tsx:342-371`), awaits them into `remindersToDelete`
(`workflows.tsx:373`), and then iterates them in the snippet at `workflows.tsx:375-381`.
4. For email reminders, `deleteScheduledEmailReminder(reminder.id, reminder.referenceId)`
is called without the `immediateDelete` flag. In `emailReminderManager.ts:197-236`, when
`referenceId` is present and `immediateDelete` is falsy, the function does NOT call the
SendGrid `/v3/user/scheduled_sends` cancel API (lines 213-221) and instead only sets
`cancelled: true` in the database (`workflowReminder.update` at 225-232). As a result, the
SendGrid scheduled send created in step 2 is never cancelled and still fires at `sendAt`,
sending reminder emails for a workflow that has been disabled on that event type.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/trpc/server/routers/viewer/workflows.tsx
**Line:** 378:378
**Comment:**
*Logic Error: When disabling a workflow on specific event types, email reminders for those event types are marked as cancelled or deleted in the database but the underlying SendGrid scheduled sends are not cancelled because `deleteScheduledEmailReminder` is invoked without the `immediateDelete` flag, so reminders can still fire for disabled workflows.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| deleteScheduledSMSReminder(reminder.referenceId); | ||
| } | ||
| if (reminder.method === WorkflowMethods.EMAIL) { | ||
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); |
There was a problem hiding this comment.
Suggestion: When editing a workflow step, existing email reminders tied to that step are "cancelled" in the database but any SendGrid scheduled sends are left intact because deleteScheduledEmailReminder is called without the immediateDelete flag, so old reminders can still be delivered even after a step is changed. [logic error]
Severity Level: Major ⚠️
- ❌ Edited workflow steps still send old scheduled emails.
- ⚠️ Attendees may receive duplicate or conflicting reminders.
- ⚠️ Workflow editor changes not reliably reflected in reminders.
Suggested change
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId); | |
| deleteScheduledEmailReminder(reminder.id, reminder.referenceId, true); |
Steps of Reproduction ✅
1. Create a workflow with trigger BEFORE_EVENT or AFTER_EVENT and at least one email step,
assign it to an event type, and create a confirmed booking. This schedules reminders
through `scheduleWorkflowReminders()` in
`packages/features/ee/workflows/lib/reminders/reminderScheduler.ts:15-94`, which calls
`scheduleEmailReminder()` in
`packages/features/ee/workflows/lib/reminders/emailReminderManager.ts:29-195` to schedule
a SendGrid email with `sendAt` and to create a `workflowReminder` row with `scheduled:
true` and a non-null `referenceId` (batch_id).
2. From the UI or via direct TRPC call, edit that workflow step (e.g., change action,
timing, body) using `workflowsRouter.update` in
`packages/trpc/server/routers/viewer/workflows.tsx:232-847`, passing the updated `steps`
array containing the modified step with the same `id`.
3. In `workflowsRouter.update`, the existing steps are iterated (`userWorkflow.steps.map`
at `workflows.tsx:505-683`). For the edited step, the `else if (JSON.stringify(oldStep)
!== JSON.stringify(newStep))` branch is taken (`workflows.tsx:533-565`), which updates the
step in Prisma, then gathers existing reminders for that step into `remindersFromStep` and
filters them into `remindersToUpdate` (`workflows.tsx:567-571`). For those reminders, the
snippet at `workflows.tsx:573-579` runs: for email reminders it calls
`deleteScheduledEmailReminder(reminder.id, reminder.referenceId)` without the
`immediateDelete` flag.
4. In `deleteScheduledEmailReminder()` (`emailReminderManager.ts:197-236`), because
`referenceId` is non-null and `immediateDelete` is undefined/false, the SendGrid cancel
API (`client.request` to `/v3/user/scheduled_sends` at 213-221) is not invoked. Instead,
only the `workflowReminder` row is updated with `cancelled: true` (225-232). At the
original `sendAt` time, SendGrid still sends the old scheduled email based on the
unchanged `batch_id`, while new reminders for the edited step are scheduled later in the
same update handler (`workflows.tsx:586-679`) via `scheduleEmailReminder`. This can result
in outdated or duplicate reminder emails being delivered after the step has been edited.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/trpc/server/routers/viewer/workflows.tsx
**Line:** 576:576
**Comment:**
*Logic Error: When editing a workflow step, existing email reminders tied to that step are "cancelled" in the database but any SendGrid scheduled sends are left intact because `deleteScheduledEmailReminder` is called without the `immediateDelete` flag, so old reminders can still be delivered even after a step is changed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished running the review. |
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.
User description
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)*CodeAnt-AI Description
Stop sending reminders for cancelled/rescheduled bookings and improve phone verification in workflows
What Changed
Impact
✅ Fewer reminder emails and SMS sent after cancellations✅ Fewer reminders sent for deleted or edited workflows✅ Clearer phone number verification for SMS workflows💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.