SMS workflow reminder retry count tracking#4
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>
|
@cubic-dev-ai review this pull request |
@ShashankFC I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/ee/workflows/api/scheduleSMSReminders.ts">
<violation number="1" location="packages/features/ee/workflows/api/scheduleSMSReminders.ts:32">
P1: Bug: The `retryCount > 1` deletion condition is missing the `method: WorkflowMethods.SMS` filter. As written, this will delete **all** workflow reminders (EMAIL, WHATSAPP, etc.) that have `retryCount > 1`, not just SMS ones. The first OR branch correctly scopes to SMS, but the second branch applies globally.</violation>
<violation number="2" location="packages/features/ee/workflows/api/scheduleSMSReminders.ts:179">
P2: The `prisma.workflowReminder.update()` in the `catch` block is not wrapped in its own try/catch. If this DB call fails, the error will propagate uncaught, crashing the handler and skipping all remaining unprocessed reminders. Consider wrapping it in a try/catch or using `.catch()` to ensure the loop continues.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| lte: dayjs().toISOString(), | ||
| }, | ||
| OR: [ | ||
| { |
There was a problem hiding this comment.
P1: Bug: The retryCount > 1 deletion condition is missing the method: WorkflowMethods.SMS filter. As written, this will delete all workflow reminders (EMAIL, WHATSAPP, etc.) that have retryCount > 1, not just SMS ones. The first OR branch correctly scopes to SMS, but the second branch applies globally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/ee/workflows/api/scheduleSMSReminders.ts, line 32:
<comment>Bug: The `retryCount > 1` deletion condition is missing the `method: WorkflowMethods.SMS` filter. As written, this will delete **all** workflow reminders (EMAIL, WHATSAPP, etc.) that have `retryCount > 1`, not just SMS ones. The first OR branch correctly scopes to SMS, but the second branch applies globally.</comment>
<file context>
@@ -28,10 +28,19 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
- lte: dayjs().toISOString(),
- },
+ OR: [
+ {
+ method: WorkflowMethods.SMS,
+ scheduledDate: {
</file context>
| }, | ||
| }); | ||
| } else { | ||
| await prisma.workflowReminder.update({ |
There was a problem hiding this comment.
P2: The prisma.workflowReminder.update() in the catch block is not wrapped in its own try/catch. If this DB call fails, the error will propagate uncaught, crashing the handler and skipping all remaining unprocessed reminders. Consider wrapping it in a try/catch or using .catch() to ensure the loop continues.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/ee/workflows/api/scheduleSMSReminders.ts, line 179:
<comment>The `prisma.workflowReminder.update()` in the `catch` block is not wrapped in its own try/catch. If this DB call fails, the error will propagate uncaught, crashing the handler and skipping all remaining unprocessed reminders. Consider wrapping it in a try/catch or using `.catch()` to ensure the loop continues.</comment>
<file context>
@@ -163,9 +175,26 @@ async function handler(req: NextApiRequest, res: NextApiResponse) {
},
});
+ } else {
+ await prisma.workflowReminder.update({
+ where: {
+ id: reminder.id,
</file context>
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)*