SMS workflow reminder retry count tracking#7
Conversation
* add retry count to workflow reminder * add logic to for retry count --------- Co-authored-by: CarinaWolli <wollencarina@gmail.com> Co-authored-by: Udit Takkar <53316345+Udit-takkar@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis change introduces retry tracking for SMS reminders by adding a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/features/ee/workflows/api/scheduleSMSReminders.ts:
- Around line 29-45: The deletion predicate is too broad:
prisma.workflowReminder.deleteMany currently uses OR with one clause checking
method: WorkflowMethods.SMS and scheduledDate lte, and another clause checking
retryCount gt 1 which matches reminders of any method; narrow the second clause
to only apply to SMS reminders by making both OR branches include method:
WorkflowMethods.SMS (i.e., change the OR array to [{ method:
WorkflowMethods.SMS, scheduledDate: { lte: ... } }, { method:
WorkflowMethods.SMS, retryCount: { gt: 1 } }]) so only SMS reminders are deleted
for either being past scheduledDate or exceeding retryCount.
🧹 Nitpick comments (2)
packages/features/ee/workflows/api/scheduleSMSReminders.ts (2)
178-187: Consider adding explicit retry limit checks and error tracking.While incrementing
retryCounton scheduling failure is correct, several improvements would make the retry logic more robust:
Missing retry limit check before scheduling: The code attempts to schedule without checking if
retryCounthas already reached the limit. Consider checkingreminder.retryCount >= MAX_RETRIESbefore attempting to schedule.No failure reason captured: When
scheduledSMSis falsy, the reason for failure is lost. Consider logging or persisting error details for debugging.Undocumented retry threshold: The deletion logic uses
retryCount > 1(deletes at 2+), meaning reminders get 2 total attempts. This should be documented as a constant (e.g.,MAX_RETRY_COUNT = 1).📋 Suggested improvements
Add a constant at the top of the file:
const MAX_RETRY_COUNT = 1; // Allow 2 total attempts (0 and 1)Then check before scheduling:
for (const reminder of unscheduledReminders) { // Skip if already exceeded retry limit if (reminder.retryCount > MAX_RETRY_COUNT) { continue; } // ... existing code ... }And update the deletion condition to use the same constant:
retryCount: { - gt: 1, + gt: MAX_RETRY_COUNT, },
189-199: Improve error handling and consider structured error logging.The catch block correctly increments
retryCount, but the error handling could be more robust:
Console logging limitations: Line 198 logs to console, which may not be retained in production environments and lacks structure for monitoring/alerting.
Same retry limit concerns: As with the scheduling failure case (lines 178-187), there's no pre-check for retry limits before attempting to schedule.
Silent failure potential: If the
prisma.workflowReminder.updateitself throws an error, the outer catch doesn't handle it, potentially causing the entire cron job to fail.📋 Suggested improvements
Consider structured error logging and defensive error handling:
} catch (error) { + console.error(`Error scheduling SMS for reminder ${reminder.id}:`, { + reminderId: reminder.id, + bookingUid: reminder.booking?.uid, + error: error instanceof Error ? error.message : String(error), + retryCount: reminder.retryCount, + }); + + try { await prisma.workflowReminder.update({ where: { id: reminder.id, }, data: { retryCount: reminder.retryCount + 1, }, }); + } catch (updateError) { + console.error(`Failed to update retry count for reminder ${reminder.id}:`, updateError); + } - console.log(`Error scheduling SMS with error ${error}`); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/features/ee/workflows/api/scheduleSMSReminders.tspackages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sqlpackages/prisma/schema.prisma
🧰 Additional context used
🧬 Code graph analysis (1)
packages/features/ee/workflows/api/scheduleSMSReminders.ts (1)
packages/features/ee/workflows/lib/getWorkflowReminders.ts (2)
select(112-172)PartialWorkflowReminder(35-40)
🔇 Additional comments (3)
packages/prisma/schema.prisma (1)
1000-1000: LGTM! Clean addition of retry tracking field.The
retryCountfield is properly typed asIntwith a sensible default of 0, which correctly initializes the retry counter for new and existing workflow reminders.packages/prisma/migrations/20240508134359_add_retry_count_to_workflow_reminder/migration.sql (1)
1-2: LGTM! Migration aligns with schema changes.The migration correctly adds the
retryCountcolumn withNOT NULL DEFAULT 0, which safely handles existing rows in the WorkflowReminder table.packages/features/ee/workflows/api/scheduleSMSReminders.ts (1)
48-60: LGTM! Properly extends query to include retry tracking.The
selectobject correctly includesretryCount, and the type assertion(PartialWorkflowReminder & { retryCount: number })[]accurately reflects the extended result structure.
| await prisma.workflowReminder.deleteMany({ | ||
| where: { | ||
| method: WorkflowMethods.SMS, | ||
| scheduledDate: { | ||
| lte: dayjs().toISOString(), | ||
| }, | ||
| OR: [ | ||
| { | ||
| method: WorkflowMethods.SMS, | ||
| scheduledDate: { | ||
| lte: dayjs().toISOString(), | ||
| }, | ||
| }, | ||
| { | ||
| retryCount: { | ||
| gt: 1, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Critical: Overly broad deletion condition risks unintended data loss.
The OR logic at Line 31 will delete any WorkflowReminder with retryCount > 1, regardless of method (SMS, EMAIL, WHATSAPP) or scheduled date. This can delete non-SMS reminders or reminders that are still pending processing.
The intent appears to be: delete SMS reminders that are either past their scheduled date OR have exceeded retry limits. The second condition should be scoped to SMS reminders.
🔧 Proposed fix
await prisma.workflowReminder.deleteMany({
where: {
- OR: [
- {
- method: WorkflowMethods.SMS,
- scheduledDate: {
- lte: dayjs().toISOString(),
- },
- },
- {
- retryCount: {
- gt: 1,
- },
- },
- ],
+ method: WorkflowMethods.SMS,
+ OR: [
+ {
+ scheduledDate: {
+ lte: dayjs().toISOString(),
+ },
+ },
+ {
+ retryCount: {
+ gt: 1,
+ },
+ },
+ ],
},
});🤖 Prompt for AI Agents
In @packages/features/ee/workflows/api/scheduleSMSReminders.ts around lines 29 -
45, The deletion predicate is too broad: prisma.workflowReminder.deleteMany
currently uses OR with one clause checking method: WorkflowMethods.SMS and
scheduledDate lte, and another clause checking retryCount gt 1 which matches
reminders of any method; narrow the second clause to only apply to SMS reminders
by making both OR branches include method: WorkflowMethods.SMS (i.e., change the
OR array to [{ method: WorkflowMethods.SMS, scheduledDate: { lte: ... } }, {
method: WorkflowMethods.SMS, retryCount: { gt: 1 } }]) so only SMS reminders are
deleted for either being past scheduledDate or exceeding retryCount.
2 participants
Test 9nn
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#9](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/9)*