chore: Remove SendGrid related code from emailReminderManager#23591
chore: Remove SendGrid related code from emailReminderManager#23591
Conversation
WalkthroughRemoved SendGrid-specific branching and feature-flag gating from the workflows email reminder flow and consolidated it to a single pathway using Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T08:45:41.834Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/ee/workflows/lib/reminders/aiPhoneCallManager.ts:167-193
Timestamp: 2025-08-19T08:45:41.834Z
Learning: In calcom/cal.com AI phone call scheduling (aiPhoneCallManager.ts), workflow reminder records are intentionally kept in the database even when task scheduling fails, as they provide valuable debugging information for troubleshooting scheduling issues, per maintainer Udit-takkar in PR #22995.
Applied to files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
🧬 Code graph analysis (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
packages/features/ee/workflows/lib/reminders/providers/emailProvider.ts (1)
sendOrScheduleWorkflowEmails(9-31)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Detect changes
🔇 Additional comments (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
283-288: Ignore redundantshouldSchedulesuggestion
The proposedshouldSchedulevariable doesn’t exist andreminderUidis alreadyundefinedwhen no reminder is created, so introducing a non-existent flag is unnecessary. Disregard this change.Likely an incorrect or invalid review comment.
| import { getTranslation } from "@calcom/lib/server/i18n"; | ||
| import prisma from "@calcom/prisma"; | ||
| import type { TimeUnit } from "@calcom/prisma/enums"; | ||
| import type { TimeUnit } from "@calcom/prisma"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm TimeUnit source and make unit normalization deterministic.
- Verify that
@calcom/prismais the intended source forTimeUnit(vs.@calcom/prisma/enumsor generated client types). Mismatched enum sources can break the.toLocaleLowerCase()mapping later. - Prefer
.toLowerCase()over.toLocaleLowerCase()to avoid locale-dependent surprises.
Apply:
- const timeUnit: timeUnitLowerCase | undefined = timeSpan.timeUnit?.toLocaleLowerCase() as timeUnitLowerCase;
+ const timeUnit: timeUnitLowerCase | undefined = timeSpan.timeUnit?.toLowerCase() as timeUnitLowerCase;Run to confirm import usage patterns:
🏁 Script executed:
#!/bin/bash
# Check all imports of TimeUnit to ensure consistency across the repo
rg -nP 'import\s+type\s*{\s*TimeUnit\s*}\s+from\s*[\"\\\'][^\"\\\']+[\"\\\']' -C2Length of output: 116
🏁 Script executed:
#!/bin/bash
# Find all imports of TimeUnit
rg -n "import type { TimeUnit } from" -C2 --glob "*.ts"
# Find any imports from @calcom/prisma/enums"
rg -n "from '@calcom/prisma/enums'" -C2 --glob "*.ts"Length of output: 805
🏁 Script executed:
#!/bin/bash
# Find all imports of TimeUnit across the repo
rg -F "import type { TimeUnit" -n --glob "*.ts"
# Check for any imports of TimeUnit or enums from '@calcom/prisma/enums'
rg -F "@calcom/prisma/enums" -n --glob "*.ts"
# Locate all occurrences of toLocaleLowerCase to verify usage sites
rg -n ".toLocaleLowerCase" -C2 --glob "*.ts"Length of output: 63356
Normalize TimeUnit import source and use .toLowerCase() deterministically across all reminder managers
- In packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (line 12), change
import type { TimeUnit } from "@calcom/prisma";
to
import type { TimeUnit } from "@calcom/prisma/enums"; - In the four EE reminder managers, replace all
.toLocaleLowerCase()calls ontimeSpan.timeUnitwith.toLowerCase()and cast totimeUnitLowerCase:
• emailReminderManager.ts (line 84)
• aiPhoneCallManager.ts (line 150)
• whatsappReminderManager.ts (line 53)
• smsReminderManager.ts (line 122) - Verify each
timeUnitis typed astimeUnitLowerCase | undefinedafter normalization.
🤖 Prompt for AI Agents
In packages/features/ee/workflows/lib/reminders/emailReminderManager.ts around
line 12 and at normalization site near line 84, change the type import from
`import type { TimeUnit } from "@calcom/prisma";` to `import type { TimeUnit }
from "@calcom/prisma/enums";`, and where you normalize `timeSpan.timeUnit`
replace any `.toLocaleLowerCase()` with `.toLowerCase()`, assign the result to a
new variable typed as `timeUnitLowerCase | undefined` (e.g., `const timeUnit:
timeUnitLowerCase | undefined = timeSpan?.timeUnit?.toLowerCase()`), and apply
the same `.toLowerCase()` replacement and typing pattern in the other EE
reminder managers at aiPhoneCallManager.ts line 150, whatsappReminderManager.ts
line 53, and smsReminderManager.ts line 122 so every `timeUnit` is
deterministically normalized and properly typed.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (3)
216-217: Nit: comment still references SendGrid.Update wording to match SMTP/tasker flow.
Apply:
- // Allows debugging generated email content without waiting for sendgrid to send emails + // Allows debugging generated email content without waiting for tasker to run or SMTP to send
84-85: Use.toLowerCase()for deterministic normalization.Avoid locale-dependent
.toLocaleLowerCase().Apply:
- const timeUnit: timeUnitLowerCase | undefined = timeSpan.timeUnit?.toLocaleLowerCase() as timeUnitLowerCase; + const timeUnit: timeUnitLowerCase | undefined = timeSpan.timeUnit?.toLowerCase() as timeUnitLowerCase;
269-281: Setscheduledbased on whether the date is in the future; avoid inconsistent rows.Tasker drops jobs with
sendAt <= now, yet we persistscheduled: true. Record should be kept but flagged correctly. This aligns with prior maintainer guidance about retaining reminders for diagnostics while reflecting reality.Apply:
- let reminderUid = undefined; - if (scheduledDate) { + let reminderUid: string | undefined; + const shouldSchedule = Boolean(scheduledDate && scheduledDate.isAfter(dayjs())); + if (scheduledDate) { const reminder = await prisma.workflowReminder.create({ data: { bookingUid: uid, workflowStepId, method: WorkflowMethods.EMAIL, scheduledDate: scheduledDate.toDate(), - scheduled: true, + scheduled: shouldSchedule, }, }); reminderUid = reminder.uuid; }
🧹 Nitpick comments (3)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (3)
283-288: Only passsendAtwhen the job should actually be scheduled.Passing a past
sendAtcauses the provider to no-op. If product wants immediate send for past-due items, omitsendAt; otherwise, keep suppressing. Confirm desired behavior for BEFORE vs AFTER triggers.Apply:
await sendOrScheduleWorkflowEmails({ ...mailData, to: sendTo, - sendAt: scheduledDate?.toDate(), + sendAt: shouldSchedule ? scheduledDate!.toDate() : undefined, referenceUid: reminderUid ?? undefined, });
292-296: Prisma: select only needed fields.We only use
uuidandreferenceId; addselectper repo guideline.Apply:
- const workflowReminder = await prisma.workflowReminder.findUnique({ - where: { - id: reminderId, - }, - }); + const workflowReminder = await prisma.workflowReminder.findUnique({ + where: { id: reminderId }, + select: { uuid: true, referenceId: true }, + });
298-301: Prefer logger overconsole.error.Use the file logger for consistency.
Apply:
- if (!workflowReminder) { - console.error("Workflow reminder not found"); - return; - } + if (!workflowReminder) { + log.warn("Workflow reminder not found"); + return; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/features/ee/round-robin/roundRobinManualReassignment.ts(0 hunks)packages/features/ee/workflows/lib/reminders/emailReminderManager.ts(1 hunks)packages/features/ee/workflows/lib/reminders/reminderScheduler.ts(0 hunks)packages/features/ee/workflows/lib/reminders/scheduleMandatoryReminder.ts(0 hunks)packages/trpc/server/routers/viewer/workflows/util.ts(0 hunks)
💤 Files with no reviewable changes (4)
- packages/trpc/server/routers/viewer/workflows/util.ts
- packages/features/ee/workflows/lib/reminders/scheduleMandatoryReminder.ts
- packages/features/ee/workflows/lib/reminders/reminderScheduler.ts
- packages/features/ee/round-robin/roundRobinManualReassignment.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T08:45:41.834Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/ee/workflows/lib/reminders/aiPhoneCallManager.ts:167-193
Timestamp: 2025-08-19T08:45:41.834Z
Learning: In calcom/cal.com AI phone call scheduling (aiPhoneCallManager.ts), workflow reminder records are intentionally kept in the database even when task scheduling fails, as they provide valuable debugging information for troubleshooting scheduling issues, per maintainer Udit-takkar in PR #22995.
Applied to files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
🧬 Code graph analysis (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
packages/features/ee/workflows/lib/reminders/providers/emailProvider.ts (1)
sendOrScheduleWorkflowEmails(9-31)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
12-12: LGTM on enum import source.
TimeUnitimport normalized to@calcom/prisma/enumsas recommended.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (3)
86-86: Use.toLowerCase()for deterministic casing.Avoid locale-dependent
.toLocaleLowerCase()here.- const timeUnit: timeUnitLowerCase | undefined = timeSpan.timeUnit?.toLocaleLowerCase() as timeUnitLowerCase; + const timeUnit: timeUnitLowerCase | undefined = timeSpan.timeUnit?.toLowerCase() as timeUnitLowerCase;
227-229: Nit: Update debug comment to remove “sendgrid”.- // Allows debugging generated email content without waiting for sendgrid to send emails + // Allows debugging generated email content without waiting for tasker/SMTP to send emails
280-292: Setscheduledbased on future-ness; typereminderUidand add debug.Marking
scheduled: trueeven whensendAtis in the past leaves inconsistent rows (tasker won’t schedule). Keep the row (per team practice) but setscheduledaccurately and type the uid.- let reminderUid = undefined; - if (scheduledDate) { + let reminderUid: string | undefined; + const shouldSchedule = Boolean(scheduledDate && scheduledDate.isAfter(dayjs())); + if (scheduledDate) { const reminder = await prisma.workflowReminder.create({ data: { bookingUid: uid, workflowStepId, method: WorkflowMethods.EMAIL, scheduledDate: scheduledDate.toDate(), - scheduled: true, + scheduled: shouldSchedule, }, }); reminderUid = reminder.uuid; + log.debug( + `Created workflowReminder ${reminder.uuid} for booking ${uid} at ${scheduledDate.toDate().toISOString()} (shouldSchedule=${shouldSchedule})` + ); }
🧹 Nitpick comments (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
294-299: Confirm desired behavior for pastsendAt; optionally gate scheduling.With a past
sendAt,sendOrScheduleWorkflowEmailsno-ops. If that’s intended, keep as is. If you prefer immediate send when the window has passed, gate byshouldSchedulefrom above.- sendAt: scheduledDate?.toDate(), - referenceUid: reminderUid ?? undefined, + sendAt: shouldSchedule ? scheduledDate!.toDate() : undefined, + referenceUid: shouldSchedule ? reminderUid : undefined,
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts(1 hunks)packages/features/ee/workflows/lib/reminders/reminderScheduler.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/features/ee/workflows/lib/reminders/reminderScheduler.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T08:45:41.834Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/ee/workflows/lib/reminders/aiPhoneCallManager.ts:167-193
Timestamp: 2025-08-19T08:45:41.834Z
Learning: In calcom/cal.com AI phone call scheduling (aiPhoneCallManager.ts), workflow reminder records are intentionally kept in the database even when task scheduling fails, as they provide valuable debugging information for troubleshooting scheduling issues, per maintainer Udit-takkar in PR #22995.
Applied to files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
🧬 Code graph analysis (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
packages/features/ee/workflows/lib/reminders/providers/emailProvider.ts (1)
sendOrScheduleWorkflowEmails(9-31)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/features/ee/workflows/lib/test/workflows.test.ts (2)
1099-1105: Strengthen the expectation to avoid false positivesAlso assert call count to ensure only the unified SMTP path is hit once.
- expect(emailProvider.sendOrScheduleWorkflowEmails).toHaveBeenCalled(); + expect(emailProvider.sendOrScheduleWorkflowEmails).toHaveBeenCalledTimes(1);
1099-1105: Optional: Use module mocking instead of spying on the exportThe test currently does
import * as emailProvider from "../reminders/providers/emailProvider"; vi.spyOn(emailProvider, "sendOrScheduleWorkflowEmails");which only works so long as
emailReminderManager.tspulls in that named export. If you change the import style (e.g. move the function into a different module or switch to a default export), the spy will silently stop intercepting. Instead, mock the provider module at the top of the test to decouple from import details:vi.mock("../reminders/providers/emailProvider", () => ({ sendOrScheduleWorkflowEmails: vi.fn(), }));This ensures any import of that function is replaced by your mock, regardless of how the implementation imports it.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/ee/workflows/lib/test/workflows.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/ee/workflows/lib/test/workflows.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/ee/workflows/lib/test/workflows.test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/ee/workflows/lib/test/workflows.test.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
- GitHub Check: Type check / check-types
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/features/ee/workflows/lib/test/workflows.test.ts (1)
1099-1102: Stub the provider and tighten the assertionAvoid accidental network/side effects and assert key args.
Apply this diff to strengthen the expectation:
- await scheduleEmailReminder(baseArgs); - expect(emailProvider.sendOrScheduleWorkflowEmails).toHaveBeenCalled(); + await scheduleEmailReminder(baseArgs); + expect(emailProvider.sendOrScheduleWorkflowEmails).toHaveBeenCalledWith( + expect.objectContaining({ + to: baseArgs.sendTo, + sendAt: undefined, // NEW_EVENT shouldn't schedule + }) + );Additionally, stub the spy to resolve immediately (outside this range, shown for context):
// near line 1062 or in beforeEach vi.spyOn(emailProvider, "sendOrScheduleWorkflowEmails").mockResolvedValue(undefined as any);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/ee/workflows/lib/test/workflows.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/ee/workflows/lib/test/workflows.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/ee/workflows/lib/test/workflows.test.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/ee/workflows/lib/test/workflows.test.ts
🧬 Code graph analysis (1)
packages/features/ee/workflows/lib/test/workflows.test.ts (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
scheduleEmailReminder(61-300)
🔇 Additional comments (1)
packages/features/ee/workflows/lib/test/workflows.test.ts (1)
1099-1102: LGTM: unified SMTP path is exercisedThe test now asserts the SMTP flow via
sendOrScheduleWorkflowEmails; leftover SendGrid/teamIdconcerns are resolved.
E2E results are ready! |
| /** | ||
| * @deprecated only needed for SendGrid, use SMTP with tasker instead | ||
| */ | ||
| if (IMMEDIATE_WORKFLOW_TRIGGER_EVENTS.includes(triggerEvent)) { |
There was a problem hiding this comment.
NIT: import in line https://github.com/calcom/cal.com/pull/23591/files#diff-675b1da8590d6cdd4d5e3d50ada134c6cbdf4ec2893b991247595df3b95c9617L22 can be removed
CarinaWolli
left a comment
CarinaWolli
left a comment
There was a problem hiding this comment.
Should this PR also cover removing the sendgrid related code in scheduleEmailReminders.ts?
…m/calcom/cal.com into chore/use-smtp-workflow-reminders
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (3)
85-85: Use.toLowerCase()(deterministic) instead of.toLocaleLowerCase().Prevents locale-dependent behavior and matches enum normalization across managers.
- const timeUnit: timeUnitLowerCase | undefined = timeSpan.timeUnit?.toLocaleLowerCase() as timeUnitLowerCase; + const timeUnit: timeUnitLowerCase | undefined = timeSpan.timeUnit?.toLowerCase() as timeUnitLowerCase;
226-227: Update debug comment to reflect SMTP/tasker (no SendGrid).Minor wording cleanup.
- // Allows debugging generated email content without waiting for sendgrid to send emails + // Allows debugging generated email content without waiting for the SMTP/tasker pipeline to send emails
279-291: Don’t mark reminders as scheduled when sendAt is past; type reminderUid and gate referenceUid.Set scheduled based on shouldSchedule and only pass referenceUid/sendAt when scheduling. Keeps the DB row for debugging while avoiding inconsistent state.
- let reminderUid = undefined; - if (scheduledDate) { + let reminderUid: string | undefined; + const shouldSchedule = Boolean(scheduledDate && scheduledDate.isAfter(dayjs())); + if (scheduledDate) { const reminder = await prisma.workflowReminder.create({ data: { bookingUid: uid, workflowStepId, method: WorkflowMethods.EMAIL, scheduledDate: scheduledDate.toDate(), - scheduled: true, + scheduled: shouldSchedule, }, }); reminderUid = reminder.uuid; } - await sendOrScheduleWorkflowEmails({ + await sendOrScheduleWorkflowEmails({ ...mailData, to: sendTo, - sendAt: scheduledDate?.toDate(), - referenceUid: reminderUid ?? undefined, + sendAt: shouldSchedule ? scheduledDate?.toDate() : undefined, + referenceUid: shouldSchedule ? reminderUid : undefined, });Also applies to: 293-298
🧹 Nitpick comments (2)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (2)
302-307: Prisma: select only needed fields.Avoid fetching unnecessary columns; you only use uuid and referenceId here.
- const workflowReminder = await prisma.workflowReminder.findUnique({ - where: - { id: reminderId }, - }); + const workflowReminder = await prisma.workflowReminder.findUnique({ + where: { id: reminderId }, + select: { uuid: true, referenceId: true }, + });
309-309: Use logger instead of console.error.Keep logging consistent and structured.
- console.error("Workflow reminder not found"); + log.warn("Workflow reminder not found");
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
🧠 Learnings (1)
📚 Learning: 2025-08-19T08:45:41.834Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/features/ee/workflows/lib/reminders/aiPhoneCallManager.ts:167-193
Timestamp: 2025-08-19T08:45:41.834Z
Learning: In calcom/cal.com AI phone call scheduling (aiPhoneCallManager.ts), workflow reminder records are intentionally kept in the database even when task scheduling fails, as they provide valuable debugging information for troubleshooting scheduling issues, per maintainer Udit-takkar in PR #22995.
Applied to files:
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts
🧬 Code graph analysis (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
packages/features/ee/workflows/lib/reminders/providers/emailProvider.ts (1)
sendOrScheduleWorkflowEmails(9-31)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (1)
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (1)
89-93: Allow 0-minute offsets (don’t skip when timeSpan.time is 0)
In emailReminderManager.ts (lines 89–93), replace the truthy checktimeSpan.time && timeUnitwithtimeSpan.time != null && timeUnitso that a zero-minute offset still schedules a reminder. Would 0-minute reminders be a valid use case in your workflows?
4 participants
What does this PR do?
Removing code as I was about to change the following; which made less sense than complete removal.
to