feat: add SMTP support as alternative to Resend#2259
Conversation
|
@aryansingh1718 is attempting to deploy a commit to the OpenStatus Team on Vercel. A member of the Team first needs to authorize it. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/emails/src/send.ts">
<violation number="1" location="packages/emails/src/send.ts:10">
P2: SMTP port parsing lacks validation for NaN or valid range, and the env schema treats SMTP_PORT as an unconstrained string. Invalid values like \"abc\" produce NaN and malformed values like \"587foo\" silently parse as 587, masking configuration errors.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const resendClient = env.RESEND_API_KEY ? new Resend(env.RESEND_API_KEY) : null; | ||
| const smtpTransporter = env.SMTP_HOST ? nodemailer.createTransport({ | ||
| host: env.SMTP_HOST, | ||
| port: env.SMTP_PORT ? Number.parseInt(env.SMTP_PORT) : 587, |
There was a problem hiding this comment.
P2: SMTP port parsing lacks validation for NaN or valid range, and the env schema treats SMTP_PORT as an unconstrained string. Invalid values like "abc" produce NaN and malformed values like "587foo" silently parse as 587, masking configuration errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/emails/src/send.ts, line 10:
<comment>SMTP port parsing lacks validation for NaN or valid range, and the env schema treats SMTP_PORT as an unconstrained string. Invalid values like \"abc\" produce NaN and malformed values like \"587foo\" silently parse as 587, masking configuration errors.</comment>
<file context>
@@ -1,10 +1,18 @@
+const resendClient = env.RESEND_API_KEY ? new Resend(env.RESEND_API_KEY) : null;
+const smtpTransporter = env.SMTP_HOST ? nodemailer.createTransport({
+ host: env.SMTP_HOST,
+ port: env.SMTP_PORT ? Number.parseInt(env.SMTP_PORT) : 587,
+ auth: env.SMTP_USER && env.SMTP_PASS ? {
+ user: env.SMTP_USER,
</file context>
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/emails/src/send.ts">
<violation number="1" location="packages/emails/src/send.ts:10">
P2: SMTP port parsing lacks validation for NaN or valid range, and the env schema treats SMTP_PORT as an unconstrained string. Invalid values like \"abc\" produce NaN and malformed values like \"587foo\" silently parse as 587, masking configuration errors.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@claude review |
|
Claude finished @mxkaske's task in 3m 16s —— View job Code Review: SMTP Support for OpenStatus Emails
Overall the approach is sound — reading provider config from env at construction time and routing through private 🔴 Critical: Breaking constructor change leaves 3 callers unupdated
TypeScript will reject 🟡 Important: Optional chaining silently no-ops on constructor-guaranteed fieldsIn // client.tsx:68 — smtpTransporter is guaranteed set when type === "smtp"
await this.smtpTransporter?.sendMail(...) // ← silent no-op if undefined
// client.tsx:77 — resendClient is guaranteed set when type === "resend"
await this.resendClient?.emails.send(...) // ← silent no-op if undefinedSame applies to 🟡 Important:
|
|
thanks @aryansingh1718! bare with us for the review |
|
Hey @mxkaske , I've addressed the review comments — updated the 3 callers to new EmailClient(), replaced ?. with ! assertions with comments, and added the this.type === "resend" guard for the rate limit check. Please take a look! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Hey @mxkaske, fixed the CI test failures — the EmailClient constructor was throwing when neither RESEND_API_KEY nor SMTP_HOST was set, which crashed module imports in the test environment. Added a non-production early return in the constructor and updated all send method guards from === "development" to !== "production". Please take a look! |
|
@claude review |
|
Claude finished @mxkaske's task in 3m 18s —— View job Code Review: SMTP Support for OpenStatus Emails
Good progress on addressing the previous round of feedback — env validation, catalog versioning, 🔴 Critical: Two more
|
| File | Lines | Broken call |
|---|---|---|
packages/notifications/email/src/index.ts |
20, 49, 77 | new EmailClient({ apiKey: env.RESEND_API_KEY }) |
packages/subscriptions/src/channels/email.ts |
16 | new EmailClient({ apiKey: process.env.RESEND_API_KEY }) |
Both will fail TypeScript compilation with the current zero-arg constructor signature.
🔴 Critical: packages/subscriptions/src/channels/email.ts doesn't support SMTP
getEmailClient() in packages/subscriptions/src/channels/email.ts:10-18 hardcodes a check for RESEND_API_KEY and throws if it's absent, even when SMTP is configured. Self-hosters using only SMTP will get Error: RESEND_API_KEY environment variable is required for email notifications when subscriptions try to send status report updates.
// packages/subscriptions/src/channels/email.ts:10-15 — blocks SMTP users
function getEmailClient(): EmailClient {
if (!process.env.RESEND_API_KEY) {
throw new Error("RESEND_API_KEY environment variable is required ...");
}
emailClient = new EmailClient({ apiKey: ... }); // also broken signature
}The check should be removed (the constructor already throws with a proper error if neither provider is configured).
🟡 Important: send.ts module-level throw has no NODE_ENV guard
client.tsx constructor returns early in non-production (if (process.env.NODE_ENV !== "production") return), but send.ts throws unconditionally at module import time when neither provider is configured:
// packages/emails/src/send.ts:23-25 — throws at import time, no env guard
if (!resendClient && !smtpTransporter) {
throw new Error("Either RESEND_API_KEY or SMTP_HOST must be provided.");
}CI currently passes because packages/subscriptions/src/test-preload.ts and packages/services/test/preload.ts stub RESEND_API_KEY. That's a workaround but the inconsistency creates a trap: any new test suite that transitively imports send.ts without reading the preload pattern will crash confusingly. Recommend adding if (process.env.NODE_ENV === "production") around that block to match client.tsx.
🟡 Important: SMTP batch sends have no concurrency limit
sendBatch in client.tsx:106-115 sends all SMTP emails simultaneously via Promise.all with no chunking:
// client.tsx:106-115 — unbounded parallelism for SMTP
const sendEmailPromises = opts.map(async (email) => {
return transporter.sendMail({ ... });
});
await Promise.all(sendEmailPromises);Resend is capped at 100 per batch. For a self-hoster with 500+ subscribers getting a status report update, this fires 500 concurrent SMTP connections. Most SMTP servers will either reject the excess or start queuing. Worth adding a chunk(opts, 50) loop similar to the Resend path.
🟠 Minor: Stale mock signature in index.test.ts
packages/notifications/email/src/index.test.ts:10 mocks EmailClient with the old { apiKey: string } constructor signature:
EmailClient: mock((_args: { apiKey: string }) => { ... })This should be mock(() => { ... }) to match the current zero-arg constructor (and avoid misleading future readers).
🟠 Minor: Redundant ?. in send.ts
send.ts:54 and send.ts:79 use optional chaining inside else if (resendClient) blocks:
} else if (resendClient) {
await resendClient?.emails.send(...) // ?. is dead code
...
await resendClient?.batch.send(...) // ?. is dead codeThe ?. can't be reached when resendClient is falsy, so these should be .emails.send(...) and .batch.send(...).
✅ What's done well
- Constructor callers: The 3 previously-identified call sites (
apps/server,apps/status-page,apps/workflows) are all updated tonew EmailClient(). !assertions:sendSingleandsendBatchinclient.tsxnow throw with a clear error message when the expected transporter is missing.- Rate limit guard:
sendFollowUpBatchedandsendSlackFeedbackBatchedcorrectly gate the re-throw onthis.type === "resend". - Env validation:
SMTP_PORTuses/^\d+$/regex + range refine, and all fields use.min(1)to reject empty strings. - Catalog versioning:
nodemailerand@types/nodemailerare correctly inpnpm-workspace.yaml'scatalog:section. - Non-production guard in constructor:
EmailClientreturns early in non-production, preventing test crashes.
|
few thoughts @aryansingh1718!
|
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/emails/src/send.ts">
<violation number="1" location="packages/emails/src/send.ts:10">
P2: SMTP port parsing lacks validation for NaN or valid range, and the env schema treats SMTP_PORT as an unconstrained string. Invalid values like \"abc\" produce NaN and malformed values like \"587foo\" silently parse as 587, masking configuration errors.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/emails/src/send.ts">
<violation number="1" location="packages/emails/src/send.ts:10">
P2: SMTP port parsing lacks validation for NaN or valid range, and the env schema treats SMTP_PORT as an unconstrained string. Invalid values like \"abc\" produce NaN and malformed values like \"587foo\" silently parse as 587, masking configuration errors.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Hey @mxkaske, I've addressed all the review comments:
Let me know if anything else needs to be addressed! |
|
Hey @mxkaske, just a gentle reminder on this PR — it's been a few days and I wanted to make sure it didn't get lost. Let me know if any changes are needed! |
|
@claude review |
|
Claude finished @mxkaske's task in 2m 51s —— View job Code Review: SMTP Support for OpenStatus Emails
Good progress across all rounds — the constructor callers, env validation, catalog versioning, non-prod guards, and 🔴 CI is failing: formatting errors in
|
|
@aryansingh1718 thanks! will try to give it a push so we can merge this week |
|
Hey @mxkaske, just pushed a follow-up commit (990b8e8) that addresses the two review points from earlier:
The formatting issue (import order/spacing/indentation) was already resolved by the autofix-ci bot in cfc830f. Looks like the only thing blocking checks right now is the pending workflow approval (Vercel deploys need maintainer auth) — would you be able to approve those when you get a chance? Happy to make any further changes if needed. Thanks again for the review! |
|
@claude review |
|
Claude finished @mxkaske's task in 2m 11s —— View job Code Review: SMTP Support for OpenStatus Emails
All CI checks are now passing (autofix.ci ✅, DX Check ✅, Tests ✅). All issues raised in previous review rounds have been addressed. Here's the final verification: ✅ All Previously Raised Issues ResolvedConstructor callers — all 5 sites updated to
SMTP batch concurrency — Formatting — autofix.ci applied the import-order and indentation fixes in Env validation — Catalog versioning — Non-prod guard — constructor returns early when Rate-limit guard — correctly gated on
🟠 Minor Residual Notes (non-blocking)
SummaryThree rounds of review feedback have been systematically addressed. The implementation is clean, fail-fast, and edge-cases around env validation, concurrency, type mapping, and test isolation are all handled correctly. This is ready to merge. |
|
Hey @mxkaske, looks like all the review feedback has been addressed and the bot review marked this ready to merge. The only outstanding items are the Vercel deployment checks, which seem to need authorization on your end rather than anything I can fix from my side. Could you take a look and merge whenever you get a chance? Thanks! |
|
Hey @mxkaske, just following up on this — would love to get it merged when you get a chance. Happy to make any additional changes if needed. Thanks for your time! |
|
Hey @aryansingh1718 Can you rebase the branch with main for resolving merge conflicts. |
589080f to
52561c7
Compare
52561c7 to
7cb9ebf
Compare
# Conflicts: # packages/emails/src/client.tsx # packages/emails/src/send.ts
git rebase --continue git push origin feat/smtp-support# Last command done (1 command done):
521b622 to
0cf8a50
Compare
|
@aggmoulik, I rebased the branch with main and fixed all the merge conflicts. Looks like the CI workflows and Vercel previews are waiting on maintainer approval to run — let me know if there's anything else needed from my side in the meantime! |
3 participants
Summary
This PR adds SMTP as an alternative email transport to Resend, allowing self-hosters to use any SMTP provider instead of being locked into Resend.
Closes #2213
Changes
packages/emails/src/env.ts— added optional SMTP environment variables (SMTP_HOST,SMTP_PORT,SMTP_USER,SMTP_PASS,SMTP_FROM)packages/emails/src/client.ts— refactoredEmailClientto support both Resend and SMTP via nodemailerpackages/emails/src/send.ts— updated generic email functions to support Resend/SMTP. Removed legacysendEmailHtmlfunctionpackages/emails/src/index.ts— removedsendEmailHtmlexport.env.docker.example— documented SMTP env vars alongside ResendDOCKER.md,COOLIFY_ENVIRONMENT_GUIDE.md,COOLIFY_SETUP.md— updated email configuration docsHow it works
SMTP_HOSTis set → uses nodemailer with SMTPRESEND_API_KEYis set → uses ResendConfiguration
Option 1: Resend
Option 2: SMTP