Repository navigation
fix(mailer): drop hand-built appName/appContact, brand the subjects - #4177
Conversation
render() already injects appName/appContact from the resolved brand (#4164-#4171), so the hand-built params at the 14 sendMail call sites are redundant. Drop them, and switch the 8 subjects that embedded the app title to mailer.getBrand().name so a mailer.brand.name override reaches the subject too. Subjects built from org.name and the 4 fixed English subjects are unchanged. With no brand config, brand.name falls back to config.app.title, so output is byte-identical. Refs #4131
mockMailer.getBrand.mockReturnValue(...) persisted for every subsequent call within the test instead of just the one assertion; mockReturnValueOnce keeps the override scoped. Kimi review nit from Phase 0.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (16)
💤 Files with no reviewable changes (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughEmail call sites remove manually supplied app-name and contact parameters. Billing, invitation, and organization email subjects now use the mailer brand name in place of the configured app title. ChangesEmail branding and parameters
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~12 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The branding changes have no established merge-blocking regression; the PR is ready for normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected branding edits preserve email recipients, invitation-token links and legacy template parameters. No specific security regression is established, but uncertainty about the comparison baseline prevents an unqualified minimal-risk assessment. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4177 +/- ##
==========================================
- Coverage 94.55% 94.55% -0.01%
==========================================
Files 174 174
Lines 6137 6134 -3
Branches 1983 1979 -4
==========================================
- Hits 5803 5800 -3
Misses 271 271
Partials 63 63
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Summary
appName/appContactparams at the 14sendMailcall sites acrossauth,billing,invitations,organizations, andusers— the mailer'srender()already injects both fromgetBrand(). Switched the 8 subjects that embedded the app title togetBrand().name(5 inbilling.email.js, 1 each inbilling.referral.service.js,invitations.service.js,organizations.service.js). Subjects built fromorg.name, and the 4 fixed English subjects, are unchanged.getBrand()(from ✨ Mailer: central brand values via config.mailer.brand #4130) is now the single source of truth for brand values with aconfig.app.title/config.app.contactfallback baked in, so passingappName/appContactby hand at every call site was redundant and a drift risk the moment a downstream setsconfig.mailer.brand.Scope
auth,billing,invitations,organizations,usersnone(all call sites consume the samelib/helpers/mailergetBrand()/render()seam already in place)low(deletion-only at 13 of 14 call sites; the 14th and the 5 billing subjects move from readingconfig.app?.titletomailer.getBrand().name, which falls back to the sameconfig.app.titlewhen no brand config is set)Validation
npm run lint— cleannpm test— 2770/2770 unit tests pass (coverage gate passes); integration/E2E run in CI (local MongoDB infra was down for this pass)Guardrails check
.env*,secrets/**, keys, tokens)billing.init.email-alerts,billing.referral.service,invitations.service,organizations.emailVerification(.policy),organizations.membership.addMember.email,organizations.service.signup,organizations.service.welcomeEmail)Notes for reviewers
render().getBrand()landing first (🐛 Billing emails: fall back to getBaseUrl() when config.app.url is unset #4128/✨ Mailer: central brand values via config.mailer.brand #4130, already onmaster); no further sequencing needed.Summary by CodeRabbit