feat(api-service): add Gmail email reactions#10860
Merged
Conversation
Add first-class support for custom MIME alternatives (eg. Gmail reaction content type) across the email stack. Changes include: - Introduce EmailAlternative/IEmailAlternative types and add alternatives to SendEmail/Email options in shared, stateless, and adapter types. - Forward alternatives through providers: Nodemailer, Outlook365, SES and SendGrid (SendGrid builds a content array including alternatives instead of plain html when present). - Implement NovuEmailAdapterImpl.addReaction to send Gmail-compatible reaction messages (builds reaction payload, references, subject, and alternatives) and helper methods for emoji/name handling. - Update ChatSdkService to skip and warn when outbound provider does not support custom MIME alternatives (list of supported providers added) and to pass alternatives to mail handlers. - Add/adjust unit tests to cover forwarding custom MIME alternatives and ChatSdkService behavior. These changes enable sending and handling of email reactions and other custom MIME parts end-to-end.
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
📝 WalkthroughWalkthroughAdds support for email MIME “alternatives” across providers and types, implements Gmail-specific reactions via an email alternative in the email adapter, and makes ChatSdkService selectively forward or strip alternatives based on a provider allowlist with unit tests covering behavior. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Adapter as Email Adapter
participant ChatSDK as ChatSdkService
participant Provider as Email Provider
participant SMTP as SMTP Transport
Client->>Adapter: addReaction(threadId, messageId, emoji)
Adapter->>Adapter: decode recipient, validate Gmail origin
Adapter->>Adapter: map/validate emoji, build reaction alternative (JSON)
Adapter->>ChatSDK: sendEmail(params + alternatives)
ChatSDK->>ChatSDK: resolve outbound integration provider
ChatSDK->>ChatSDK: check allowlist for alternatives
alt allowlisted
ChatSDK->>Provider: send mailOptions (includes alternatives)
else not allowlisted
ChatSDK->>ChatSDK: logger.warn (providerId, outboundIntegrationId)
ChatSDK->>Provider: send mailOptions (without alternatives)
end
Provider->>SMTP: transport.send / provider-specific send
SMTP->>Client: deliver email (with MIME alternatives if included)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
![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: 3
🧹 Nitpick comments (7)
packages/providers/src/lib/email/ses/ses.provider.ts (1)
37-48: Optional: simplify the alternatives guard.Because
alternatives = []defaults the value,alternatives?.lengthcan bealternatives.length(no optional chain needed). Tiny readability nit.♻️ Optional
- ...(alternatives?.length ? { alternatives } : {}), + ...(alternatives.length ? { alternatives } : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/providers/src/lib/email/ses/ses.provider.ts` around lines 37 - 48, The guard using alternatives?.length is unnecessary because alternatives defaults to an array; update the mailOptions construction to use alternatives.length instead of alternatives?.length (referencing the alternatives parameter and the mailOptions object created in the method that calls this.transform) to simplify the check and improve readability.packages/chat-adapter-email/src/adapter.ts (2)
22-26: Limited reaction-name vocabulary.
EMAIL_REACTION_EMOJI_BY_NAMEcurrently only mapseyes. Combined with the throw intoReactionEmoji(line 328), any other named emoji resolved bychat'sgetEmoji(e.g.thumbs_up,heart, etc.) reachesaddReactionas an object with.nameand will reject because it’s not in the table and is not a raw emoji string. If reactions on Gmail are intended to support a broader set, expand this map (or pull the unicode char off the resolvedEmojiValue); if not, please add a code comment documenting the intentional Gmail-only-eyesscope and fail fast at the API layer with a clearer message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat-adapter-email/src/adapter.ts` around lines 22 - 26, The EMAIL_REACTION_EMOJI_BY_NAME map is too limited (only 'eyes'), causing toReactionEmoji (used by addReaction) to throw for other named emojis — either expand the map to include other supported names (e.g., thumbs_up, heart, laugh, etc.) or change toReactionEmoji to accept an EmojiValue object returned by chat.getEmoji and extract its Unicode character (fallback to lookup in EMAIL_REACTION_EMOJI_BY_NAME), and update addReaction to call the revised toReactionEmoji; alternatively, if only Gmail's 'eyes' reaction is intentionally supported, add a clear comment near EMAIL_REACTION_EMOJI_BY_NAME and change toReactionEmoji to return a clear API-level error message when an unsupported name is passed.
188-222:addReactionlooks good; one concurrency note worth flagging.The flow (decode → Gmail-domain gate → resolve emoji → build reaction MIME) is clean and mirrors
postMessage. Two observations:
toReactionEmoji(emoji)throws on unsupported emojis (see comment onEMAIL_REACTION_EMOJI_BY_NAME). Any error here will propagate toChatSdkService.reactToMessageand bubble up to the caller. Confirm that is the intended behavior versus a no-op for unsupported emojis (matching the silent no-op on non-Gmail recipients).
referencesis built fromreplyHeaders?.Referencesplus the message being reacted to, but on a long thread this can produce a very longReferencesheader. Gmail/SMTP have practical limits (~998 octets per header line, RFC 5322); on deep threads consider truncating to keep the most recent N parents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat-adapter-email/src/adapter.ts` around lines 188 - 222, addReaction currently throws if toReactionEmoji rejects (see toReactionEmoji and EMAIL_REACTION_EMOJI_BY_NAME) and builds a potentially very long References header via buildReactionReferences(replyHeaders?.References, messageId); change addReaction to treat unsupported emojis as a no-op (catch errors from toReactionEmoji and return early, matching the silent return for non-Gmail recipients) and truncate the computed References to a safe length before sending (limit the number of parent message IDs kept—implement truncation inside buildReactionReferences or immediately after calling it so only the most recent N references are included to avoid exceeding SMTP/Gmail header length limits); keep the existing error throw for other unexpected failures but do not let an unsupported emoji error bubble up.packages/stateless/src/lib/provider/provider.interface.ts (1)
26-29: Consider extractingIEmailAlternativeto avoid duplication.The PR also defines an
IEmailAlternativeshape in@novu/shared(and similarEmailAlternativein@novu/chat-adapter-email) with the same{ contentType; content: string | Buffer }structure. Inlining a third copy here invites drift the next time the shape evolves (e.g., adding aheadersfield or constrainingcontentType). Prefer exporting a singleIEmailAlternativeand reusing it inIEmailOptions.♻️ Suggested shape
+export interface IEmailAlternative { + contentType: string; + content: string | Buffer; +} + export interface IEmailOptions { to: string[]; subject: string; html: string; from?: string; text?: string; - alternatives?: Array<{ - contentType: string; - content: string | Buffer; - }>; + alternatives?: IEmailAlternative[]; attachments?: IAttachmentOptions[];As per coding guidelines: "Treat all exported symbols in packages as public API; follow semver conventions … new exports as minor versions." This addition is a non-breaking minor change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stateless/src/lib/provider/provider.interface.ts` around lines 26 - 29, Extract the repeated alternative shape into a shared exported interface named IEmailAlternative and use it for the alternatives property in IEmailOptions: create/export IEmailAlternative (contentType: string; content: string | Buffer) in the shared module (or reuse the existing IEmailAlternative/IemailAlternative from `@novu/shared` or `@novu/chat-adapter-email`), then replace the inline alternatives type in provider.interface.ts with alternatives?: IEmailAlternative[] so all packages reference the single shared symbol.apps/api/src/app/agents/services/chat-sdk.service.spec.ts (1)
6-57: Test asserts effect indirectly; consider strengthening.A few small hardenings to make this test more resilient/explicit:
- Assert the warn payload mentions the unsupported
providerIdand theoutboundIntegrationId(so a regression that drops those fields fails here).- Assert
integrationRepository.findOnewas called with the expected_id/channelfilter (this also documents the contract).- Optionally exercise the negative case (no
alternatives→ no warn) so the suite documents both branches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.spec.ts` around lines 6 - 57, Update the test for ChatSdkService.buildSendEmailCallback to more strongly assert behavior: after invoking sendEmail, assert that logger.warn was called with a message/payload containing the unsupported providerId (EmailProviderIdEnum.Resend) and the outbound integration id ('outbound-integration-id'), and assert that integrationRepository.findOne was called with a filter containing _id: 'outbound-integration-id' and channel: ChannelTypeEnum.EMAIL; optionally add a separate negative-case test that calls the built callback without an alternatives array and asserts logger.warn was not called to document both branches. Ensure you reference the ChatSdkService class, the buildSendEmailCallback method, integrationRepository.findOne stub, and logger.warn stub when adding these assertions.apps/api/src/app/agents/services/chat-sdk.service.ts (1)
42-47: Add comment documenting the intentional limitation of MIME-alternatives support to a specific provider set.The allowlist
EMAIL_ALTERNATIVES_SUPPORTED_PROVIDERSrestricts MIME-alternatives (for Gmail reactions) to CustomSMTP, Outlook365, SendGrid, and SES. Other available providers (Braze, Brevo, Mailgun, Mailjet, Mailtrap, Mandrill, Plunk, Postmark, Resend, Sparkpost, and others) are excluded. Add a comment above this set explaining this is an intentional limitation, so future maintainers don't assume all providers should support alternatives as new ones are added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 42 - 47, Add a brief comment above the EMAIL_ALTERNATIVES_SUPPORTED_PROVIDERS constant explaining that the set intentionally limits MIME-alternatives (Gmail reactions) support to only CustomSMTP, Outlook365, SendGrid, and SES, and that other providers (Braze, Brevo, Mailgun, Mailjet, Mailtrap, Mandrill, Plunk, Postmark, Resend, Sparkpost, etc.) are excluded by design so maintainers know this is a deliberate allowlist rather than an omission when adding new providers; reference the constant name EMAIL_ALTERNATIVES_SUPPORTED_PROVIDERS in the comment so it’s clear which configuration it documents.packages/chat-adapter-email/src/types.ts (1)
11-22: ReuseIEmailAlternativefrom@novu/sharedto avoid type duplication.
EmailAlternativehere is structurally identical toIEmailAlternativedefined inpackages/shared/src/types/events.ts. Since this file already re-exports types from@novu/shared(line 3), reusing the shared type prevents drift if the shape evolves later (e.g., additional encoding/charset fields).♻️ Proposed change
-import type { Adapter } from 'chat'; +import type { Adapter } from 'chat'; +import type { IEmailAlternative } from '@novu/shared'; export type { EmailWebhookPayload, NovuEmailAttachment } from '@novu/shared'; export interface NovuEmailAdapterConfig { senderName?: string; signingSecret: string; sendEmail: (params: SendEmailParams) => Promise<{ messageId: string }>; } -export interface EmailAlternative { - contentType: string; - content: string | Buffer; -} +export type EmailAlternative = IEmailAlternative; export interface SendEmailParams { from: string; to: string; subject: string; html: string; text?: string; alternatives?: EmailAlternative[];If keeping
EmailAlternativeas a public name is required for consumers, the alias above preserves the export while keeping a single source of truth.As per coding guidelines: "
packages/sharedshould contain types, DTOs, enums, and utility functions used across both frontend and backend".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat-adapter-email/src/types.ts` around lines 11 - 22, Replace the locally defined EmailAlternative with the shared type to avoid duplication: import IEmailAlternative from `@novu/shared` (it's already re-exported in this module) and either alias the local export with "export type EmailAlternative = IEmailAlternative;" or change SendEmailParams.alternatives to use IEmailAlternative[] directly; update any references to the local EmailAlternative identifier so SendEmailParams now references the shared type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 370-381: When skipping sending because the outbound provider
doesn't support MIME alternatives, don't return a fake empty sent id; add a
defensive guard inside buildSendEmailCallback: if params.alternatives?.length &&
!EMAIL_ALTERNATIVES_SUPPORTED_PROVIDERS.has(integration.providerId) and
params.messageId is missing, log a clear warning referencing
integration.providerId and outboundIntegrationId (same logger used now) and
return without claiming success (e.g., return { messageId: undefined } or
throw), and add a short comment documenting the invariant that
NovuEmailAdapterImpl.addReaction currently supplies reactionMessageId so callers
must provide messageId.
In `@packages/chat-adapter-email/src/adapter.ts`:
- Line 306: The removeReaction method currently no-ops which can mislead callers
and hide Gmail limitations; update the removeReaction(_threadId, _messageId,
_emoji) implementation to either (1) add a clear inline comment on why removal
is unsupported (e.g., Gmail API limitation) and keep the no-op, or (2) emit a
structured warning/log (e.g., console.warn or the adapter's logger) describing
that removeReaction is not supported for this adapter so callers/operators see
the failure; reference the removeReaction method and ensure parity with
addReaction behavior in logs so the asymmetry is explicit.
- Around line 318-329: The current toReactionEmoji implementation uses a
permissive regex and may treat multi-character strings like "hi there" as raw
emoji; update toReactionEmoji to first resolve a name via toEmojiName and the
EMAIL_REACTION_EMOJI_BY_NAME map, and if the input is a string but not a mapped
name validate it strictly as a single emoji grapheme (e.g., using a
Unicode-aware check such as an Emoji property/regex or a grapheme cluster check
via Intl.Segmenter or a small utility that ensures exactly one grapheme and/or
matches \p{Emoji} code points) before returning it; if the string fails that
stricter validation, throw the same Unsupported email reaction emoji error.
---
Nitpick comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.spec.ts`:
- Around line 6-57: Update the test for ChatSdkService.buildSendEmailCallback to
more strongly assert behavior: after invoking sendEmail, assert that logger.warn
was called with a message/payload containing the unsupported providerId
(EmailProviderIdEnum.Resend) and the outbound integration id
('outbound-integration-id'), and assert that integrationRepository.findOne was
called with a filter containing _id: 'outbound-integration-id' and channel:
ChannelTypeEnum.EMAIL; optionally add a separate negative-case test that calls
the built callback without an alternatives array and asserts logger.warn was not
called to document both branches. Ensure you reference the ChatSdkService class,
the buildSendEmailCallback method, integrationRepository.findOne stub, and
logger.warn stub when adding these assertions.
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 42-47: Add a brief comment above the
EMAIL_ALTERNATIVES_SUPPORTED_PROVIDERS constant explaining that the set
intentionally limits MIME-alternatives (Gmail reactions) support to only
CustomSMTP, Outlook365, SendGrid, and SES, and that other providers (Braze,
Brevo, Mailgun, Mailjet, Mailtrap, Mandrill, Plunk, Postmark, Resend, Sparkpost,
etc.) are excluded by design so maintainers know this is a deliberate allowlist
rather than an omission when adding new providers; reference the constant name
EMAIL_ALTERNATIVES_SUPPORTED_PROVIDERS in the comment so it’s clear which
configuration it documents.
In `@packages/chat-adapter-email/src/adapter.ts`:
- Around line 22-26: The EMAIL_REACTION_EMOJI_BY_NAME map is too limited (only
'eyes'), causing toReactionEmoji (used by addReaction) to throw for other named
emojis — either expand the map to include other supported names (e.g.,
thumbs_up, heart, laugh, etc.) or change toReactionEmoji to accept an EmojiValue
object returned by chat.getEmoji and extract its Unicode character (fallback to
lookup in EMAIL_REACTION_EMOJI_BY_NAME), and update addReaction to call the
revised toReactionEmoji; alternatively, if only Gmail's 'eyes' reaction is
intentionally supported, add a clear comment near EMAIL_REACTION_EMOJI_BY_NAME
and change toReactionEmoji to return a clear API-level error message when an
unsupported name is passed.
- Around line 188-222: addReaction currently throws if toReactionEmoji rejects
(see toReactionEmoji and EMAIL_REACTION_EMOJI_BY_NAME) and builds a potentially
very long References header via
buildReactionReferences(replyHeaders?.References, messageId); change addReaction
to treat unsupported emojis as a no-op (catch errors from toReactionEmoji and
return early, matching the silent return for non-Gmail recipients) and truncate
the computed References to a safe length before sending (limit the number of
parent message IDs kept—implement truncation inside buildReactionReferences or
immediately after calling it so only the most recent N references are included
to avoid exceeding SMTP/Gmail header length limits); keep the existing error
throw for other unexpected failures but do not let an unsupported emoji error
bubble up.
In `@packages/chat-adapter-email/src/types.ts`:
- Around line 11-22: Replace the locally defined EmailAlternative with the
shared type to avoid duplication: import IEmailAlternative from `@novu/shared`
(it's already re-exported in this module) and either alias the local export with
"export type EmailAlternative = IEmailAlternative;" or change
SendEmailParams.alternatives to use IEmailAlternative[] directly; update any
references to the local EmailAlternative identifier so SendEmailParams now
references the shared type.
In `@packages/providers/src/lib/email/ses/ses.provider.ts`:
- Around line 37-48: The guard using alternatives?.length is unnecessary because
alternatives defaults to an array; update the mailOptions construction to use
alternatives.length instead of alternatives?.length (referencing the
alternatives parameter and the mailOptions object created in the method that
calls this.transform) to simplify the check and improve readability.
In `@packages/stateless/src/lib/provider/provider.interface.ts`:
- Around line 26-29: Extract the repeated alternative shape into a shared
exported interface named IEmailAlternative and use it for the alternatives
property in IEmailOptions: create/export IEmailAlternative (contentType: string;
content: string | Buffer) in the shared module (or reuse the existing
IEmailAlternative/IemailAlternative from `@novu/shared` or
`@novu/chat-adapter-email`), then replace the inline alternatives type in
provider.interface.ts with alternatives?: IEmailAlternative[] so all packages
reference the single shared symbol.
🪄 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: 7dbdb18a-c16e-4a12-a850-9d8aeec84cb1
📒 Files selected for processing (14)
apps/api/src/app/agents/services/chat-sdk.service.spec.tsapps/api/src/app/agents/services/chat-sdk.service.tspackages/chat-adapter-email/src/adapter.tspackages/chat-adapter-email/src/types.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.spec.tspackages/providers/src/lib/email/outlook365/outlook365.provider.tspackages/providers/src/lib/email/sendgrid/sendgrid.provider.spec.tspackages/providers/src/lib/email/sendgrid/sendgrid.provider.tspackages/providers/src/lib/email/ses/ses.provider.spec.tspackages/providers/src/lib/email/ses/ses.provider.tspackages/shared/src/types/events.tspackages/stateless/src/lib/provider/provider.interface.ts
commit: |
Improve handling of email reaction alternatives and emoji support across adapters and providers. Key changes: - ChatSdkService: add an allowlist for providers that preserve custom MIME alternatives; treat sendEmail as returning an optional messageId and warn/return undefined when unsupported providers receive custom alternatives without a messageId to avoid falsely claiming delivery. Updated tests to cover these cases. - NovuEmailAdapterImpl: extend supported reaction emojis, validate single-emoji graphemes using Intl.Segmenter fallback, make removeReaction a no-op, and limit reaction reference IDs to the most recent 20. - Types: unify EmailAlternative type with shared IEmailAlternative and allow sendEmail to return an optional messageId. - SES provider: fix alternatives spread to check alternatives.length before including the field. - Provider interface: add IEmailAlternative and use it in IEmailOptions. These changes prevent misreporting deliveries for providers that strip custom MIME alternatives (e.g., Gmail reactions) and improve emoji handling and type consistency.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/api/src/app/agents/services/chat-sdk.service.ts (2)
332-339: Use the sharedIEmailAlternativetype instead of an inline shape.The PR explicitly unifies on
IEmailAlternativein@novu/sharedandSendEmailParams. Re-declaring the shape inline here drifts away from that source of truth and will rot ifIEmailAlternativeever gains a field (e.g.cid, encoding hints).♻️ Suggested change
-import { ChannelTypeEnum, EmailProviderIdEnum, type IEmailOptions } from '@novu/shared'; +import { ChannelTypeEnum, EmailProviderIdEnum, type IEmailAlternative, type IEmailOptions } from '@novu/shared'; @@ - alternatives?: Array<{ - contentType: string; - content: string | Buffer; - }>; + alternatives?: IEmailAlternative[]; inReplyTo?: string; references?: string; messageId?: string; - }) => Promise<{ messageId?: string }> { + }) => Promise<{ messageId?: string }> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 332 - 339, The inline alternatives type in the function signature should be replaced with the shared IEmailAlternative type from `@novu/shared` to avoid type drift; import IEmailAlternative from '@novu/shared' and update the parameter shape (the alternatives?: Array<{ contentType: string; content: string | Buffer; }>) to alternatives?: IEmailAlternative[] (and update any related references in the same file like SendEmailParams usage) so the service uses the single source of truth for email alternative data.
373-399: Optional: collapse the twologger.warnbranches.Both branches log the same context with only the message and the returned
messageIddiffering. A singlewarnplus a ternary on the return keeps the behavior identical and reduces churn.♻️ Suggested simplification
- const hasUnsupportedAlternatives = - params.alternatives?.length && !EMAIL_ALTERNATIVES_SUPPORTED_PROVIDERS.has(integration.providerId); - if (hasUnsupportedAlternatives) { - // NovuEmailAdapterImpl.addReaction supplies a reaction Message-ID; any custom MIME alternative caller must do - // the same so skipped unsupported sends don't claim provider delivery. - if (!params.messageId) { - this.logger.warn( - { - providerId: integration.providerId, - outboundIntegrationId, - }, - 'Skipping email with custom MIME alternatives because the outbound provider is unsupported and no messageId was supplied' - ); - - return { messageId: undefined }; - } - - this.logger.warn( - { - providerId: integration.providerId, - outboundIntegrationId, - }, - 'Skipping email reaction because the outbound provider does not support custom MIME alternatives' - ); - - return { messageId: params.messageId }; - } + const hasUnsupportedAlternatives = + params.alternatives?.length && !EMAIL_ALTERNATIVES_SUPPORTED_PROVIDERS.has(integration.providerId); + if (hasUnsupportedAlternatives) { + // NovuEmailAdapterImpl.addReaction supplies a reaction Message-ID; any custom MIME alternative caller must + // do the same so skipped unsupported sends don't claim provider delivery. + const reason = params.messageId + ? 'Skipping email reaction because the outbound provider does not support custom MIME alternatives' + : 'Skipping email with custom MIME alternatives because the outbound provider is unsupported and no messageId was supplied'; + this.logger.warn({ providerId: integration.providerId, outboundIntegrationId }, reason); + + return { messageId: params.messageId }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.ts` around lines 373 - 399, The two logger.warn branches inside the hasUnsupportedAlternatives check are duplicated; consolidate them by calling this.logger.warn once with the same context ({ providerId: integration.providerId, outboundIntegrationId }) and choose the warning message via a conditional (based on params.messageId) or a ternary, then return either { messageId: undefined } or { messageId: params.messageId } using the same condition; update the block guarded by hasUnsupportedAlternatives (and reference functions/vars like hasUnsupportedAlternatives, params.messageId, this.logger.warn, integration.providerId, outboundIntegrationId) to use a single warn + conditional return.apps/api/src/app/agents/services/chat-sdk.service.spec.ts (1)
6-119: Optional: hoist the shared setup intobeforeEachto cut ~30 lines of duplication.Both
itblocks build identicallogger,integrationRepository,service, andsendEmailinstances; only thesendEmail(...)call payload and assertions differ. Extracting the setup keeps each test's intent (the gating contract) front and centre.♻️ Sketch
describe('buildSendEmailCallback', () => { let logger: { warn: sinon.SinonStub; error: sinon.SinonStub; debug: sinon.SinonStub; info: sinon.SinonStub }; let integrationRepository: { findOne: sinon.SinonStub }; let sendEmail: (params: any) => Promise<{ messageId?: string }>; beforeEach(() => { logger = { warn: sinon.stub(), error: sinon.stub(), debug: sinon.stub(), info: sinon.stub() }; integrationRepository = { findOne: sinon.stub().resolves({ _id: 'outbound-integration-id', _environmentId: 'env-id', _organizationId: 'org-id', providerId: EmailProviderIdEnum.Resend, channel: ChannelTypeEnum.EMAIL, credentials: {}, active: true, }), }; const service = new ChatSdkService(logger as any, {} as any, {} as any, {} as any, integrationRepository as any); sendEmail = (service as any).buildSendEmailCallback( { environmentId: 'env-id', organizationId: 'org-id', credentials: {} }, 'outbound-integration-id' ); }); // each `it` then only constructs the params + assertions });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/chat-sdk.service.spec.ts` around lines 6 - 119, Hoist the duplicated test setup for ChatSdkService.buildSendEmailCallback into a beforeEach: create shared locals logger (with warn/error/debug/info stubs), integrationRepository (findOne stub resolving the outbound integration), instantiate new ChatSdkService, and assign sendEmail = (service as any).buildSendEmailCallback(...). Use those shared variables in each it block so each test only builds its params and assertions; retain unique payloads and assertions about logger.warn and result messageId.
🤖 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/chat-adapter-email/src/adapter.ts`:
- Around line 215-217: The fallback subject 'New message' for reactions is
misleading; in addReaction, change behavior when
threadResolver.getSubject(threadId) returns null: do not send a reaction with a
generic subject—match other silent no-op paths by logging a warning and
returning early. Update the addReaction flow to check storedSubject (from
threadResolver.getSubject), if absent call logger.warn with context (threadId,
reactionMessageId from generateMessageId(agentAddress)) and return without
calling toReplySubject or sending the email; keep existing behavior when
storedSubject exists.
---
Nitpick comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.spec.ts`:
- Around line 6-119: Hoist the duplicated test setup for
ChatSdkService.buildSendEmailCallback into a beforeEach: create shared locals
logger (with warn/error/debug/info stubs), integrationRepository (findOne stub
resolving the outbound integration), instantiate new ChatSdkService, and assign
sendEmail = (service as any).buildSendEmailCallback(...). Use those shared
variables in each it block so each test only builds its params and assertions;
retain unique payloads and assertions about logger.warn and result messageId.
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 332-339: The inline alternatives type in the function signature
should be replaced with the shared IEmailAlternative type from `@novu/shared` to
avoid type drift; import IEmailAlternative from '@novu/shared' and update the
parameter shape (the alternatives?: Array<{ contentType: string; content: string
| Buffer; }>) to alternatives?: IEmailAlternative[] (and update any related
references in the same file like SendEmailParams usage) so the service uses the
single source of truth for email alternative data.
- Around line 373-399: The two logger.warn branches inside the
hasUnsupportedAlternatives check are duplicated; consolidate them by calling
this.logger.warn once with the same context ({ providerId:
integration.providerId, outboundIntegrationId }) and choose the warning message
via a conditional (based on params.messageId) or a ternary, then return either {
messageId: undefined } or { messageId: params.messageId } using the same
condition; update the block guarded by hasUnsupportedAlternatives (and reference
functions/vars like hasUnsupportedAlternatives, params.messageId,
this.logger.warn, integration.providerId, outboundIntegrationId) to use a single
warn + conditional return.
🪄 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: eaaca5fd-13d2-4816-89ee-66e90eb1c558
📒 Files selected for processing (6)
apps/api/src/app/agents/services/chat-sdk.service.spec.tsapps/api/src/app/agents/services/chat-sdk.service.tspackages/chat-adapter-email/src/adapter.tspackages/chat-adapter-email/src/types.tspackages/providers/src/lib/email/ses/ses.provider.tspackages/stateless/src/lib/provider/provider.interface.ts
✅ Files skipped from review due to trivial changes (2)
- packages/stateless/src/lib/provider/provider.interface.ts
- packages/providers/src/lib/email/ses/ses.provider.ts
Comment on lines
+215
to
+217
| const reactionMessageId = generateMessageId(agentAddress); | ||
| const storedSubject = await this.threadResolver.getSubject(threadId); | ||
| const subject = storedSubject ? this.toReplySubject(storedSubject) : 'New message'; |
Contributor
There was a problem hiding this comment.
Minor: 'New message' is an odd subject for a reaction.
A reaction should be threaded against a prior inbound message, so storedSubject should always be populated by the time addReaction runs. If it isn't, falling back to 'New message' will produce a reaction email with a generic subject and rely solely on In-Reply-To/References for threading — which Gmail will mostly handle, but the visible subject in the recipient's client will be misleading. Consider either dropping the reaction (consistent with the other silent no-op paths in this method) or logging a warning so the missing-subject case is observable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/chat-adapter-email/src/adapter.ts` around lines 215 - 217, The
fallback subject 'New message' for reactions is misleading; in addReaction,
change behavior when threadResolver.getSubject(threadId) returns null: do not
send a reaction with a generic subject—match other silent no-op paths by logging
a warning and returning early. Update the addReaction flow to check
storedSubject (from threadResolver.getSubject), if absent call logger.warn with
context (threadId, reactionMessageId from generateMessageId(agentAddress)) and
return without calling toReplySubject or sending the email; keep existing
behavior when storedSubject exists.
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.
What changed? Why was the change needed?
What changed
Gmail-style email reactions are supported by adding an "alternatives" mechanism to email payloads: reactions are sent as custom MIME parts (text/vnd.google.email-reaction+json) containing emoji metadata. The email adapter implements addReaction (previously not implemented) and providers were updated to forward MIME alternatives. The system now avoids falsely claiming delivery for providers that strip custom alternatives and improves emoji validation/consistency.
Affected areas
Key technical decisions
Testing
Unit tests added for ChatSdkService and provider implementations to verify alternatives handling, logging for unsupported providers, and that alternatives are forwarded into provider payloads; no e2e tests included.
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer