feat(email): add implementation of email sending module - #14
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
📝 WalkthroughWalkthroughThis PR refactors the email system by centralizing SMTP configuration into a dedicated provider, consolidating error handling across email queue consumers with structured logging, adding retry/backoff resilience to the queue service, and providing comprehensive test coverage including a Resend SMTP smoke test script. ChangesEmail Configuration and Resilience Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/mailer.config.ts`:
- Line 12: The current fallback for the mailer `from` (process.env.MAIL_FROM ??
`"FlowBrand" <${smtpUser}>`) can produce an invalid address when `smtpUser` is
not an email (e.g., "resend"); update the logic that builds the `from` field in
the mailer config so that it: checks process.env.MAIL_FROM first, else uses
`"FlowBrand" <${smtpUser}>` only if `smtpUser` contains an '@', and otherwise
falls back to a safe sender (e.g., `"FlowBrand"
<no-reply@${process.env.MAIL_DOMAIN ?? 'localhost'}>`); locate and change the
`from` assignment that references `process.env.MAIL_FROM` and `smtpUser` in
mailer.config.ts.
- Around line 8-11: The mailer config currently allows building an SMTP config
with missing credentials (host, user, pass) which defers failure; update the
config creation logic around host, port, user, pass (the variables assigned from
process.env.RESEND_SMTP_HOST/SMTP_HOST, smtpUser, and
process.env.RESEND_SMTP_API_KEY/SMTP_PASSWORD) to validate required values
immediately and throw a clear Error if host or password/user are missing so the
app fails fast during startup rather than later when jobs run.
In `@scripts/test-resend.ts`:
- Line 7: The port assignment using Number(...) can produce NaN for non-numeric
env values; update the code that sets the port (the const port variable in
scripts/test-resend.ts) to parse and validate the environment value(s)
(RESEND_SMTP_PORT, SMTP_PORT): attempt parseInt on the chosen string, check
Number.isFinite/Number.isInteger and that the value is within 1–65535, and if
invalid fall back to the default 587 (or log/throw a clear error); ensure the
validated numeric port is used when creating the nodemailer transport.
- Around line 26-30: The nodemailer transport created in
nodemailer.createTransport (const transporter) lacks an explicit secure flag
which breaks SMTPS on port 465; update the transport options to set secure: true
when port === 465 (or numeric 465) and secure: false for port 587 (or other
ports), e.g., compute a boolean from the port env and include secure in the
object passed to nodemailer.createTransport along with host/port/auth so the
transporter uses SMTPS for 465 and STARTTLS for 587.
In `@src/modules/email/email.consumer.ts`:
- Around line 13-18: The error log currently includes full recipient emails in
the logger.error call (see use of logger.error and job.data?.mail?.to); change
this to log a non-PII representation by replacing job.data?.mail?.to with a
masked or hashed value (e.g., maskEmail(recipient) or sha256(recipient)) before
calling logger.error, preserving other fields (context, error, jobId) and
ensuring the masking/hashing helper is used wherever recipient is logged in
email.consumer.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e3e44fd6-11fe-4876-9883-a8585d9250dd
📒 Files selected for processing (9)
.env.exampleconfig/mailer.config.tsscripts/test-resend.tssrc/app.module.tssrc/modules/email/email.consumer.spec.tssrc/modules/email/email.consumer.tssrc/modules/email/email.module.tssrc/modules/email/queue.service.spec.tssrc/modules/email/queue.service.ts
| dotenv.config(); | ||
|
|
||
| const host = process.env.RESEND_SMTP_HOST ?? process.env.SMTP_HOST; | ||
| const port = Number(process.env.RESEND_SMTP_PORT ?? process.env.SMTP_PORT ?? 587); |
There was a problem hiding this comment.
Validate port number conversion to prevent NaN runtime errors.
If RESEND_SMTP_PORT or SMTP_PORT contains a non-numeric string, Number() will return NaN, which will cause nodemailer to fail when creating the transport. Add validation to ensure the port is a valid number.
🛡️ Proposed fix to validate port number
-const port = Number(process.env.RESEND_SMTP_PORT ?? process.env.SMTP_PORT ?? 587);
+const portEnv = process.env.RESEND_SMTP_PORT ?? process.env.SMTP_PORT ?? '587';
+const port = Number(portEnv);
+if (isNaN(port) || port <= 0 || port > 65535) {
+ console.error(`Invalid port number: ${portEnv}`);
+ process.exit(1);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const port = Number(process.env.RESEND_SMTP_PORT ?? process.env.SMTP_PORT ?? 587); | |
| const portEnv = process.env.RESEND_SMTP_PORT ?? process.env.SMTP_PORT ?? '587'; | |
| const port = Number(portEnv); | |
| if (isNaN(port) || port <= 0 || port > 65535) { | |
| console.error(`Invalid port number: ${portEnv}`); | |
| process.exit(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-resend.ts` at line 7, The port assignment using Number(...) can
produce NaN for non-numeric env values; update the code that sets the port (the
const port variable in scripts/test-resend.ts) to parse and validate the
environment value(s) (RESEND_SMTP_PORT, SMTP_PORT): attempt parseInt on the
chosen string, check Number.isFinite/Number.isInteger and that the value is
within 1–65535, and if invalid fall back to the default 587 (or log/throw a
clear error); ensure the validated numeric port is used when creating the
nodemailer transport.
| const transporter = nodemailer.createTransport({ | ||
| host, | ||
| port, | ||
| auth: { user, pass }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check nodemailer documentation for secure option requirements
# Search for nodemailer SMTP transport configuration in the codebase
rg -n "createTransport" --type=ts -A 5 -B 2Repository: hngprojects/flowbrand-be
Length of output: 444
🏁 Script executed:
#!/bin/bash
# Get more context around the transport creation in test-resend.ts
echo "=== Full test-resend.ts file context ==="
cat -n scripts/test-resend.ts | head -50
echo -e "\n=== Check for nodemailer version in package.json ==="
grep -i "nodemailer" package.json
echo -e "\n=== Search for other transport configurations in the codebase ==="
rg -n "createTransport" --type=ts,js -A 3
echo -e "\n=== Check for any secure option usage elsewhere ==="
rg -n "secure:" --type=ts,js -B 2 -A 1Repository: hngprojects/flowbrand-be
Length of output: 2127
🏁 Script executed:
#!/bin/bash
# Check for package.json in root
echo "=== Checking for package.json ==="
find . -name "package.json" -type f | head -5
# Try to find package.json more directly
echo -e "\n=== Package.json content (looking for nodemailer) ==="
if [ -f "package.json" ]; then
cat package.json | grep -A 2 -B 2 "nodemailer" || echo "nodemailer not found in package.json"
fi
# Look for node_modules or other clues about nodemailer version
echo -e "\n=== Search for any documentation about nodemailer ==="
rg -i "nodemailer" --type-add "docs:md,txt" -t docs -A 2 -B 2 || echo "No docs found"
# Look at environment variable usage for port/host
echo -e "\n=== Check how port variable is used ==="
rg -n "port" scripts/test-resend.tsRepository: hngprojects/flowbrand-be
Length of output: 1215
🌐 Web query:
nodemailer createTransport secure option default behavior port 587 465
💡 Result:
In Nodemailer, the secure option for SMTP createTransport defaults to false [1]. - If secure: true, Nodemailer connects with TLS/SSL immediately. Use this for port 465 (SMTPS) [1]. - If secure: false (default), it connects unencrypted initially and upgrades to TLS via STARTTLS if the server supports it (automatic unless ignoreTLS: true) [1]. Use this for port 587 (submission) or 25 [2][1]. The port defaults to 587 if unspecified when secure: false, or 465 if secure: true [1]. Examples from official docs: - Port 587: { port: 587, secure: false } (STARTTLS) [2][3]. - Port 465: { port: 465, secure: true } (implicit TLS) [1][4].
Citations:
- 1: https://nodemailer.com/smtp
- 2: https://www.nodemailer.com/
- 3: https://nodemailer.com/
- 4: https://nodemailer.com/smtp/pooled
Add explicit secure option to transport configuration.
The transport configuration relies on nodemailer's default secure: false, which happens to work for port 587 (STARTTLS) but will fail for port 465 (SMTPS). Since the port is configurable via environment variables, this creates a fragility: if SMTP_PORT is set to 465, the connection will fail without the explicit secure option. Set secure: true for port 465 and secure: false for port 587 to match the protocol requirements.
🔒 Proposed fix
const transporter = nodemailer.createTransport({
host,
port,
+ secure: port === 465,
auth: { user, pass },
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const transporter = nodemailer.createTransport({ | |
| host, | |
| port, | |
| auth: { user, pass }, | |
| }); | |
| const transporter = nodemailer.createTransport({ | |
| host, | |
| port, | |
| secure: port === 465, | |
| auth: { user, pass }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-resend.ts` around lines 26 - 30, The nodemailer transport
created in nodemailer.createTransport (const transporter) lacks an explicit
secure flag which breaks SMTPS on port 465; update the transport options to set
secure: true when port === 465 (or numeric 465) and secure: false for port 587
(or other ports), e.g., compute a boolean from the port env and include secure
in the object passed to nodemailer.createTransport along with host/port/auth so
the transporter uses SMTPS for 465 and STARTTLS for 587.
4 participants
Pull Request
Description
This PR implements a centralized and resilient Email Sending Module, transitioning the application from a brittle, single-provider setup to a modular system that supports automatic provider fallback and self-healing background processing.
Key Improvements:
MailerModuleconfiguration fromAppModuleinto a dedicatedEmailModulefor better encapsulation.RESEND_SMTP_*) while automatically falling back to generic SMTP credentials to ensure 100% delivery uptime.Related Issue
Fixes BE-003 Implement Email Sending Module
Type of Change
How Has This Been Tested?
Test Evidence
Checklist
Additional Notes
The
MAIL_FROMlogic was standardized to support theName <email>format.Summary by CodeRabbit
New Features
Bug Fixes
Tests