Skip to content

Conversation

@akshayutture-augment
Copy link

…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 <[email protected]>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@akshayutture-augment
Copy link
Author

@greptileai

@greptile-apps
Copy link

greptile-apps bot commented Nov 14, 2025

Greptile Overview

Confidence Score: 3/5

  • This PR has a critical bug that will cause runtime errors in the cron job
  • The deferred cancellation approach is well-designed and simplifies the codebase significantly. However, the cron job in scheduleEmailReminders.ts:57-64 attempts to call SendGrid API with potentially null referenceId values without validation, which will cause API errors for reminders that were never scheduled with SendGrid (scheduled > 72 hours out). This is a definite runtime error that needs to be fixed before merge.
  • Pay close attention to packages/features/ee/workflows/api/scheduleEmailReminders.ts - the cron job logic needs a null check

Important Files Changed

File Analysis

Filename Score Overview
packages/features/ee/workflows/api/scheduleEmailReminders.ts 3/5 added cron job logic to cancel reminders marked as cancelled, but missing null check for referenceId
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts 5/5 updated deleteScheduledEmailReminder to support marking reminders as cancelled instead of immediate deletion
packages/features/bookings/lib/handleCancelBooking.ts 5/5 simplified cancellation logic to call updated reminder deletion functions without checking scheduled status
packages/features/bookings/lib/handleNewBooking.ts 5/5 added logic to cancel workflow reminders from previous booking when rescheduling with proper error handling
packages/prisma/schema.prisma 5/5 added optional cancelled field to WorkflowReminder model

Sequence Diagram

sequenceDiagram
    participant User
    participant BookingHandler
    participant ReminderManager
    participant Database
    participant SendGrid
    participant CronJob

    Note over User,CronJob: Booking Cancellation Flow
    User->>BookingHandler: Cancel Booking
    BookingHandler->>Database: Update booking status
    BookingHandler->>ReminderManager: deleteScheduledEmailReminder(id, refId)
    alt referenceId exists
        ReminderManager->>Database: Mark reminder as cancelled=true
    else no referenceId
        ReminderManager->>Database: Delete reminder immediately
    end
    
    Note over User,CronJob: Booking Reschedule Flow
    User->>BookingHandler: Reschedule Booking
    BookingHandler->>Database: Load workflowReminders
    BookingHandler->>ReminderManager: deleteScheduledEmailReminder(id, refId, immediate=true)
    alt immediate delete
        ReminderManager->>SendGrid: Cancel scheduled email
    end
    BookingHandler->>Database: Create new booking
    BookingHandler->>ReminderManager: scheduleWorkflowReminders()
    ReminderManager->>SendGrid: Schedule new reminders
    ReminderManager->>Database: Create new WorkflowReminder records

    Note over User,CronJob: Cron Job Processing
    CronJob->>Database: Find cancelled reminders (next hour)
    loop For each cancelled reminder
        CronJob->>SendGrid: Cancel via batch_id
        CronJob->>Database: Delete reminder record
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

10 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +57 to +64
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: reminder.referenceId,
status: "cancel",
},
});
Copy link

Choose a reason for hiding this comment

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

logic: null referenceId will cause SendGrid API error

Suggested change
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: reminder.referenceId,
status: "cancel",
},
});
if (reminder.referenceId) {
await client.request({
url: "/v3/user/scheduled_sends",
method: "POST",
body: {
batch_id: reminder.referenceId,
status: "cancel",
},
});
}
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/features/ee/workflows/api/scheduleEmailReminders.ts
Line: 57:64

Comment:
**logic:** null `referenceId` will cause SendGrid API error

```suggestion
      if (reminder.referenceId) {
        await client.request({
          url: "/v3/user/scheduled_sends",
          method: "POST",
          body: {
            batch_id: reminder.referenceId,
            status: "cancel",
          },
        });
      }
```

How can I resolve this? If you propose a fix, please make it concise.

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.

3 participants