feat(providers): forward email headers across all providers for reply threading fixes NV-7402#10836
Merged
feat(providers): forward email headers across all providers for reply threading fixes NV-7402#10836
Conversation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
📝 WalkthroughWalkthroughAdds conditional propagation of caller-provided email Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (caller)
participant Provider as Email Provider
participant API as External Email API
Client->>Provider: sendMessage(payload with headers)
Provider->>Provider: validate/merge headers (dedupe reply-to, case-insensitive lookup)
Provider->>API: forward message with provider-specific header mapping
API-->>Provider: response/ack
Provider-->>Client: return/send result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 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. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/providers/src/lib/email/mailgun/mailgun.provider.ts (1)
109-117:⚠️ Potential issue | 🟠 MajorPreserve explicit
replyToover custom headers.The new loop runs after Line 110, so
emailOptions.headers['Reply-To']can overwrite the typedreplyTovalue. Keep the structured field authoritative.🐛 Proposed fix
- if (emailOptions.replyTo) { - data['h:Reply-To'] = emailOptions.replyTo; - } - if (emailOptions.headers) { for (const [key, value] of Object.entries(emailOptions.headers)) { + if (emailOptions.replyTo && key.toLowerCase() === 'reply-to') { + continue; + } + data[`h:${key}`] = value; } } + + if (emailOptions.replyTo) { + data['h:Reply-To'] = emailOptions.replyTo; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/lib/email/mailgun/mailgun.provider.ts` around lines 109 - 117, The replyTo value set from emailOptions.replyTo is being overwritten by entries in emailOptions.headers; update the mailgun provider to preserve the explicit replyTo by either moving the headers loop to run before applying emailOptions.replyTo or by skipping header keys that represent Reply-To (case-insensitive, e.g. "Reply-To" or "reply-to") when copying emailOptions.headers into data; ensure the authoritative assignment uses the same key (`h:Reply-To`) and that the code references the existing variables `emailOptions`, `emailOptions.replyTo`, `emailOptions.headers`, and `data` in `mailgun.provider.ts`.
🧹 Nitpick comments (1)
packages/providers/src/lib/email/mailgun/mailgun.provider.spec.ts (1)
36-63: LGTM.Multipart body inspection for
h:-prefixed fields is a reasonable way to verify the Mailgun header convention end-to-end.Optional: to make the assertion more robust against future body reshaping, you could also assert the concrete header values (e.g.,
body.includes('<original-message-id@example.com>')) rather than only the field names.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/lib/email/mailgun/mailgun.provider.spec.ts` around lines 36 - 63, The test 'should forward custom headers as h: prefixed fields' in MailgunEmailProvider spec currently asserts only that the multipart body contains name="h:In-Reply-To" and name="h:References"; update the assertion callback passed to nock.post to also assert the actual header values are present (e.g., that the body includes '<original-message-id@example.com>' or the exact values from the mockNovuMessage.headers) so the test verifies both the h: field names and their concrete values are forwarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/providers/src/lib/email/braze/braze.provider.ts`:
- Line 72: The request currently always sets headers: options.headers || {},
which forces an empty headers object into Braze requests; change this to only
include the headers property when options.headers is provided (and non-empty).
Locate the code that builds the request object (where options and headers are
referenced in braze.provider.ts) and replace the unconditional assignment with a
conditional spread such as ...(options.headers &&
Object.keys(options.headers).length ? { headers: options.headers } : {}) so
headers is omitted when there are no custom headers.
In `@packages/providers/src/lib/email/mailersend/mailersend.provider.ts`:
- Around line 79-83: The code reads the In-Reply-To header using exact casing
(options.headers?.['In-Reply-To']) so variants like "in-reply-to" are missed;
update the header lookup in mailersend.provider.ts (where inReplyTo is assigned
and passed to emailParams.setInReplyTo) to perform a case-insensitive search of
options.headers keys (for example normalize keys to lower-case or check both the
lower-cased 'in-reply-to' and existing key names) and use the found value when
calling emailParams.setInReplyTo.
In `@packages/providers/src/lib/email/mandrill/mandrill.provider.spec.ts`:
- Around line 52-53: The mock implementations for
provider['transporter'].messages.send (the vi.spyOn(...).mockImplementation
async callbacks) are missing the required blank line before their return
statements; update both mock implementations (the one using
provider['transporter'].messages.send and the other similar mock later) to
insert a blank line immediately before each return to satisfy the TypeScript
style rule.
- Around line 90-94: The test's matcher is too loose: instead of using
expect.not.objectContaining({ headers: expect.anything() }) update the assertion
to explicitly verify the headers property is absent on the message object by
pulling the actual call arg from the spy (e.g., const arg = spy.mock.calls[0][0]
or spy.calls[0][0]) and asserting that
Object.prototype.hasOwnProperty.call(arg.message, 'headers') is false (or
expect('headers' in arg.message).toBe(false)), referencing the existing spy and
message.headers to locate the code to change.
In `@packages/providers/src/lib/email/nodemailer/nodemailer.provider.spec.ts`:
- Around line 99-118: The test "should forward custom headers to sendMail" is
never executed because the enclosing test suite uses
describe.skip('NodemailerProvider', ...) — update the test file so the suite is
not skipped (change describe.skip to describe) or move the specific test into
its own non-skipped describe/it block; ensure you modify the
describe.skip('NodemailerProvider') usage (or relocate the test) so that the
NodemailerProvider header propagation assertion actually runs in Vitest.
In `@packages/providers/src/lib/email/ses/ses.provider.spec.ts`:
- Around line 112-113: The mock implementation for SESv2Client.prototype.send
(the vi.spyOn(...).mockImplementation async callback) is missing the required
blank line before its return; edit the mock function passed to
mockImplementation (the async () => { ... }) to insert a blank line immediately
before the return statement so it conforms to the repo TypeScript style rule
requiring a blank line before every return.
---
Outside diff comments:
In `@packages/providers/src/lib/email/mailgun/mailgun.provider.ts`:
- Around line 109-117: The replyTo value set from emailOptions.replyTo is being
overwritten by entries in emailOptions.headers; update the mailgun provider to
preserve the explicit replyTo by either moving the headers loop to run before
applying emailOptions.replyTo or by skipping header keys that represent Reply-To
(case-insensitive, e.g. "Reply-To" or "reply-to") when copying
emailOptions.headers into data; ensure the authoritative assignment uses the
same key (`h:Reply-To`) and that the code references the existing variables
`emailOptions`, `emailOptions.replyTo`, `emailOptions.headers`, and `data` in
`mailgun.provider.ts`.
---
Nitpick comments:
In `@packages/providers/src/lib/email/mailgun/mailgun.provider.spec.ts`:
- Around line 36-63: The test 'should forward custom headers as h: prefixed
fields' in MailgunEmailProvider spec currently asserts only that the multipart
body contains name="h:In-Reply-To" and name="h:References"; update the assertion
callback passed to nock.post to also assert the actual header values are present
(e.g., that the body includes '<original-message-id@example.com>' or the exact
values from the mockNovuMessage.headers) so the test verifies both the h: field
names and their concrete values are forwarded.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f868c7b5-ac3a-4d47-baaa-2cfe9d4de265
📒 Files selected for processing (19)
packages/providers/src/lib/email/braze/braze.provider.tspackages/providers/src/lib/email/emailjs/emailjs.provider.tspackages/providers/src/lib/email/mailersend/mailersend.provider.tspackages/providers/src/lib/email/mailgun/mailgun.provider.spec.tspackages/providers/src/lib/email/mailgun/mailgun.provider.tspackages/providers/src/lib/email/mailjet/mailjet.provider.tspackages/providers/src/lib/email/mandrill/mandril.interface.tspackages/providers/src/lib/email/mandrill/mandrill.provider.spec.tspackages/providers/src/lib/email/mandrill/mandrill.provider.tspackages/providers/src/lib/email/netcore/netcore.provider.tspackages/providers/src/lib/email/nodemailer/nodemailer.provider.spec.tspackages/providers/src/lib/email/nodemailer/nodemailer.provider.tspackages/providers/src/lib/email/outlook365/outlook365.provider.tspackages/providers/src/lib/email/postmark/postmark.provider.spec.tspackages/providers/src/lib/email/postmark/postmark.provider.tspackages/providers/src/lib/email/ses/ses.provider.spec.tspackages/providers/src/lib/email/ses/ses.provider.tspackages/providers/src/lib/email/sparkpost/sparkpost.provider.spec.tspackages/providers/src/lib/email/sparkpost/sparkpost.provider.ts
| plaintext_body: options.text || null, | ||
| extras: options.payloadDetails || {}, | ||
| headers: {}, | ||
| headers: options.headers || {}, |
Contributor
There was a problem hiding this comment.
Omit headers when no custom headers are present.
Line 72 sends headers: {} whenever headers are unset or initialized empty. That differs from the non-empty guard used elsewhere and can change Braze request shape unnecessarily.
🐛 Proposed fix
- headers: options.headers || {},
+ ...(options.headers && Object.keys(options.headers).length > 0 && { headers: options.headers }),📝 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.
Suggested change
| headers: options.headers || {}, | |
| ...(options.headers && Object.keys(options.headers).length > 0 && { headers: options.headers }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/lib/email/braze/braze.provider.ts` at line 72, The
request currently always sets headers: options.headers || {}, which forces an
empty headers object into Braze requests; change this to only include the
headers property when options.headers is provided (and non-empty). Locate the
code that builds the request object (where options and headers are referenced in
braze.provider.ts) and replace the unconditional assignment with a conditional
spread such as ...(options.headers && Object.keys(options.headers).length ? {
headers: options.headers } : {}) so headers is omitted when there are no custom
headers.
… threading fixes NV-7402
Add header forwarding to all email providers that were missing it, enabling
standard reply-threading headers (In-Reply-To, References) to flow through
to the upstream provider API.
- Mailgun: map headers as h:* prefixed form-data keys
- SES: pass headers through to nodemailer SendMailOptions
- Postmark: convert to Headers [{Name, Value}] format
- Nodemailer (Custom SMTP): pass headers to SendMailOptions
- Outlook365: pass headers to SendMailOptions
- SparkPost: add headers inside content.headers
- Mandrill: add headers to message object; extend interface type
- Mailjet: include Headers in transform call (type-safe)
- EmailJS: spread custom headers first so core fields always win
- Braze: populate existing headers field from options
- NetCore: add to personalizations[0].headers
- MailerSend: map In-Reply-To to dedicated setInReplyTo() field
Guards ensure empty default headers ({}) never change existing behaviour.
Add tests for Mailgun, Postmark, SES, SparkPost, and Mandrill.
Made-with: Cursor
…or in checkIntegration Made-with: Cursor
- braze: use ternary to only pass headers when non-empty - mailersend: case-insensitive lookup for In-Reply-To header - mailgun: apply custom headers before explicit replyTo so replyTo always wins; skip Reply-To key from headers when replyTo is set - mandrill spec: blank lines before return in mock impls; tighten no-headers assertion to use not.toHaveProperty - nodemailer spec: lift header forwarding test outside describe.skip into its own non-skipped suite - ses spec: blank line before return in mock implementation Made-with: Cursor
ChmaraX
force-pushed
the
cursor/d056dc5a
branch
from
e436772 to
d86ef25
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/providers/src/lib/email/mailjet/mailjet.provider.ts`:
- Line 121: Add unit tests for the Mailjet provider mirroring Postmark/SES
coverage: in the Mailjet provider test suite (e.g., mailjet.provider.spec.ts)
add one test that calls the provider's send method with non-empty
options.headers (e.g., { "In-Reply-To": "msgid", "References": "ref" }) and
assert the outgoing payload contains Messages[0].Headers with those key/value
pairs, and another test that calls send with options.headers undefined or {} and
asserts the outgoing payload does NOT include a Headers field; use the same
mocking/inspection helpers used by other provider tests to capture the Mailjet
request body, and run pnpm build in packages/ after adding the tests.
In `@packages/providers/src/lib/email/postmark/postmark.provider.spec.ts`:
- Around line 89-102: The test should assert the Headers property is absent
rather than merely non-matching; after calling PostmarkEmailProvider.sendMessage
(and spying on (provider as any).client.sendEmail), capture the actual call
payload from spy.mock.calls[0][0] and assert it does not have the Headers
property (e.g., expect(payload).not.toHaveProperty('Headers') or
expect(payload.Headers).toBeUndefined()), replacing the current
expect.not.objectContaining check in the test for stricter verification.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d14bc426-4557-4ed6-81bb-2a0458b39fb2
📒 Files selected for processing (19)
packages/providers/src/lib/email/braze/braze.provider.tspackages/providers/src/lib/email/emailjs/emailjs.provider.tspackages/providers/src/lib/email/mailersend/mailersend.provider.tspackages/providers/src/lib/email/mailgun/mailgun.provider.spec.tspackages/providers/src/lib/email/mailgun/mailgun.provider.tspackages/providers/src/lib/email/mailjet/mailjet.provider.tspackages/providers/src/lib/email/mandrill/mandril.interface.tspackages/providers/src/lib/email/mandrill/mandrill.provider.spec.tspackages/providers/src/lib/email/mandrill/mandrill.provider.tspackages/providers/src/lib/email/netcore/netcore.provider.tspackages/providers/src/lib/email/nodemailer/nodemailer.provider.spec.tspackages/providers/src/lib/email/nodemailer/nodemailer.provider.tspackages/providers/src/lib/email/outlook365/outlook365.provider.tspackages/providers/src/lib/email/postmark/postmark.provider.spec.tspackages/providers/src/lib/email/postmark/postmark.provider.tspackages/providers/src/lib/email/ses/ses.provider.spec.tspackages/providers/src/lib/email/ses/ses.provider.tspackages/providers/src/lib/email/sparkpost/sparkpost.provider.spec.tspackages/providers/src/lib/email/sparkpost/sparkpost.provider.ts
✅ Files skipped from review due to trivial changes (5)
- packages/providers/src/lib/email/netcore/netcore.provider.ts
- packages/providers/src/lib/email/postmark/postmark.provider.ts
- packages/providers/src/lib/email/mandrill/mandril.interface.ts
- packages/providers/src/lib/email/mailgun/mailgun.provider.spec.ts
- packages/providers/src/lib/email/mandrill/mandrill.provider.spec.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/providers/src/lib/email/outlook365/outlook365.provider.ts
- packages/providers/src/lib/email/nodemailer/nodemailer.provider.ts
- packages/providers/src/lib/email/braze/braze.provider.ts
- packages/providers/src/lib/email/mailersend/mailersend.provider.ts
- packages/providers/src/lib/email/emailjs/emailjs.provider.ts
- packages/providers/src/lib/email/sparkpost/sparkpost.provider.spec.ts
- packages/providers/src/lib/email/ses/ses.provider.spec.ts
- packages/providers/src/lib/email/mailgun/mailgun.provider.ts
- packages/providers/src/lib/email/ses/ses.provider.ts
- packages/providers/src/lib/email/sparkpost/sparkpost.provider.ts
… of module mock The module-level vi.mock for nodemailer was not reliably intercepting sendMail, causing the new non-skipped test to hit a real DNS lookup. Rewrite to use vi.spyOn on provider['transports'].sendMail directly after construction, consistent with how other provider tests (Postmark, Mandrill, SES) are written. Also add a second assertion verifying headers are absent when none are provided. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/providers/src/lib/email/nodemailer/nodemailer.provider.spec.ts`:
- Around line 241-249: Add a test that covers the case where headers is an empty
object so we don't leak headers: {} to Nodemailer: in the existing spec for
NodemailerProvider (nodemailer.provider.spec.ts) call provider.sendMessage using
a message that has headers: {} (e.g., set mockNovuMessage.headers = {}) and
assert the captured payload from provider['transports'].sendMail does not have
the 'headers' property; reference the provider.sendMessage call and the spy on
provider['transports'].sendMail to locate where to add this assertion.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 91af43c2-887c-4d92-98a4-30da53131363
📒 Files selected for processing (1)
packages/providers/src/lib/email/nodemailer/nodemailer.provider.spec.ts
Comment on lines
+241
to
+249
| test('should not include headers field when no custom headers provided', async () => { | ||
| const provider = new NodemailerProvider(mockConfig); | ||
| const spy = vi.spyOn(provider['transports'], 'sendMail').mockResolvedValue({ messageId: 'test-id' } as any); | ||
|
|
||
| await provider.sendMessage(mockNovuMessage); | ||
|
|
||
| const payload = spy.mock.calls[0][0] as Record<string, unknown>; | ||
|
|
||
| expect(payload).not.toHaveProperty('headers'); |
Contributor
There was a problem hiding this comment.
Cover the empty headers: {} path as well.
Line 245 verifies omitted headers, but the worker path described in this PR initializes headers as {}. A truthy-object guard would still leak headers: {} to Nodemailer and this test would not catch it.
🧪 Proposed additional coverage
test('should not include headers field when no custom headers provided', async () => {
const provider = new NodemailerProvider(mockConfig);
const spy = vi.spyOn(provider['transports'], 'sendMail').mockResolvedValue({ messageId: 'test-id' } as any);
await provider.sendMessage(mockNovuMessage);
const payload = spy.mock.calls[0][0] as Record<string, unknown>;
expect(payload).not.toHaveProperty('headers');
});
+
+ test('should not include headers field when custom headers are empty', async () => {
+ const provider = new NodemailerProvider(mockConfig);
+ const spy = vi.spyOn(provider['transports'], 'sendMail').mockResolvedValue({ messageId: 'test-id' } as any);
+
+ await provider.sendMessage({
+ ...mockNovuMessage,
+ headers: {},
+ });
+
+ const payload = spy.mock.calls[0][0] as Record<string, unknown>;
+
+ expect(payload).not.toHaveProperty('headers');
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/providers/src/lib/email/nodemailer/nodemailer.provider.spec.ts`
around lines 241 - 249, Add a test that covers the case where headers is an
empty object so we don't leak headers: {} to Nodemailer: in the existing spec
for NodemailerProvider (nodemailer.provider.spec.ts) call provider.sendMessage
using a message that has headers: {} (e.g., set mockNovuMessage.headers = {})
and assert the captured payload from provider['transports'].sendMail does not
have the 'headers' property; reference the provider.sendMessage call and the spy
on provider['transports'].sendMail to locate where to add this assertion.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
headersforwarding to the 9 email providers that were missing it (Mailgun, SES, Postmark, Nodemailer/SMTP, Outlook365, SparkPost, Mandrill, Mailjet, EmailJS, Braze, NetCore), completing the set started in feat: add custom header support for resend, brevo and sendgrid #5343from,to, andsubjectheaders?: Record<string, string>to the internal Mandrill interface so the type correctly describes the payloadHow it works
Each provider maps
IEmailOptions.headersto its own API-specific format:h:HeaderNamekeys in form-dataSendMailOptions.headersHeaders: [{ Name, Value }]arraycontent.headersobjectmessage.headersobjectHeaderskey insidetransformcallmessages.email.headersfieldpersonalizations[0].headersemailParams.setInReplyTo()forIn-Reply-To(SDK has no generic headers)No regression risk — the worker always initialises
headersto{}, and every provider guards withObject.keys(headers).length > 0(or a no-op spread) before touching the upstream API.Test plan
Headers/headersfield is added when none are suppliedvitest run)Made with Cursor
What changed
Email headers from IEmailOptions are now forwarded into upstream provider payloads so reply-threading headers (e.g., In-Reply-To, References) are preserved across transports. Providers that previously ignored custom headers were updated to map them into each provider’s API format, and EmailJS was fixed so core fields (from, to, subject) cannot be overridden by user-provided headers.
Affected areas
providers: Multiple email providers were updated to forward headers into their API formats: Mailgun (form-data keys as h:HeaderName), Postmark (Headers array of { Name, Value }), SparkPost (content.headers), Mandrill (message.headers), Mailjet (Headers in transformed message), Braze (messages.email.headers), NetCore (personalizations[0].headers), SES/Nodemailer/Outlook365 (SendMailOptions.headers), EmailJS (spread custom headers first so core fields win), and MailerSend (uses emailParams.setInReplyTo() for In-Reply-To). The email worker initializes headers = {} and providers only attach headers when the headers object is present and non-empty to avoid behavior changes when callers don’t supply headers.
Key technical decisions
Testing
New and updated unit tests cover Mailgun, Postmark, SES, SparkPost, Mandrill, and Nodemailer; existing provider tests were run with the new tests and no linter errors were reported. No additional e2e or manual testing was required beyond these provider-specific unit tests.