Skip to content

Comprehensive workflow reminder management for booking lifecycle events#6

Open
zaibkhan wants to merge 1 commit into
workflow-queue-basefrom
workflow-queue-enhanced
Open

Comprehensive workflow reminder management for booking lifecycle events#6
zaibkhan wants to merge 1 commit into
workflow-queue-basefrom
workflow-queue-enhanced

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

…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>
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Harden reminder cancellations, await async ops, filter SendGrid refs
What’s good: Refactor aligns cancellation flows across email/SMS, introduces cancellable flag and a cron-based cleanup; migration adds a clear state for cancelled reminders.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity Issue Why it matters
High Correctness — Null referenceId can abort SendGrid cancellation loop …/api/scheduleEmailReminders.ts
This query does not filter out reminders with a null referenceId, yet the loop calls SendGrid cancellation using reminder.referenceId. A null batch_id will fail the request and, because the entire loop is inside one try/catch, a single failure can abort cancellation of all remaining reminders.
High Correctness — Unawaited async cancellations risk race conditions …/viewer/workflows.tsx
Async cancellations are launched but never awaited. This can create race conditions where new reminders are scheduled before cancellations complete, and unhandled rejections may be swallowed. Use Promise.all over map to ensure cancel ops finish (still parallel) before proceeding.

Showing top 2 issues. Critical: 0, High: 2. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: Multiple async cancellations are fired-and-forgotten (forEach without awaiting), risking races and orphaned external jobs; tighten DB queries to skip null referenceIds before calling SendGrid.
  • Testing: Add integration tests for: 1) Disabling event types updates: cancellations are awaited and no duplicate reminders get scheduled; 2) Reschedule flow: previous booking reminders are cancelled (SendGrid/Twilio mocked) and DB rows are removed/marked cancelled as intended; 3) Cron API: cancelled reminders with null referenceId are ignored and do not abort the loop.
  • Documentation: Document the semantics of deleteScheduledEmailReminder(reminderId, referenceId, immediateDelete): when to set immediateDelete, whether DB deletion is expected via cascade, and how 'cancelled' flag interacts with the cron scheduler.
  • Compatibility: No external API contract changes detected beyond internal cron endpoint auth via CRON_API_KEY; DB migration adds a nullable boolean column and appears backward compatible.
  • Performance: Batch cancellations using Promise.all and avoid wrapping the entire cancellation loop in a single try/catch to prevent early abort; this will improve throughput and resilience.
  • Security: Cron handler correctly checks CRON_API_KEY; ensure logs do not print secrets on SendGrid/Twilio errors (current logs are generic).
  • Open questions: For immediateDelete in deleteScheduledEmailReminder, do we rely exclusively on FK cascades to remove DB rows in all callers? If any caller uses immediateDelete without deleting the parent, should we also delete the reminder row to avoid lingering records?

Confidence: 3/5 — Needs work before merge (2 high · status: Requires changes)

React with 👍 or 👎 if you found this review useful.

},
});

//cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High: This query does not filter out reminders with a null referenceId, yet the loop calls SendGrid cancellation using reminder.referenceId. A null batch_id will fail the request and, because the entire loop is inside one try/catch, a single failure can abort cancellation of all remaining reminders.

Suggested change
//cancel reminders for cancelled/rescheduled bookings that are scheduled within the next hour
```suggestion
const remindersToCancel = await prisma.workflowReminder.findMany({
where: {
cancelled: true,
referenceId: { not: null },
scheduledDate: { lte: dayjs().add(1, "hour").toISOString() },
},
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants