Conversation
❌ Deploy Preview for dashboard-v2-novu-staging failed. Why did it fail? →
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds first-class Novu Email agent support: new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Chat Client
participant Adapter as NovuEmailAdapter
participant Thread as ThreadResolver
participant SendFn as sendEmail callback
participant IntegrationRepo as IntegrationRepository
participant MailFactory as MailFactory
Client->>Adapter: postMessage(threadId, message)
Adapter->>Adapter: renderMessage (html/text)
Adapter->>Thread: getReplyHeaders(threadId)
Adapter->>SendFn: sendEmail({ to, subject, html, inReplyTo, references, outboundIntegrationId })
SendFn->>IntegrationRepo: fetch outboundIntegrationId (env/org)
IntegrationRepo-->>SendFn: integration (credentials)
SendFn->>SendFn: decrypt credentials
SendFn->>MailFactory: build mail handler
SendFn->>MailFactory: send(options)
MailFactory-->>SendFn: { messageId }
SendFn-->>Adapter: { messageId }
Adapter->>Thread: trackMessage(threadId, messageId)
Adapter-->>Client: RawMessage
sequenceDiagram
participant Webhook as Inbound Email Webhook
participant Handler as WebhookHandler
participant Worker as DomainRouteStrategy
participant AgentIntRepo as AgentIntegrationRepository
participant GetKey as IntegrationRepository (decrypt)
participant HTTP as HttpClientService
participant API as Target API (API_ROOT_URL)
Webhook->>Handler: POST /webhook (novu-signature header)
Handler->>Handler: verify HMAC + timestamp
Handler-->>Worker: parsed EmailWebhookPayload
Worker->>AgentIntRepo: findLinksForAgents(agentId, env, org)
AgentIntRepo-->>Worker: agent integration link (integrationId)
Worker->>GetKey: fetch/decrypt integration secret (integrationId)
GetKey-->>Worker: secretKey
Worker->>Worker: compute novu-signature header
Worker->>HTTP: POST API/webhook (novu-signature + payload)
HTTP-->>API: request
API-->>HTTP: response
HTTP-->>Worker: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 |
github-actions
Bot
added
@novu/dal
@novu/shared
@novu/worker
@novu/dashboard
@novu/api-service
labels
github-actions
Bot
changed the title
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 20
♻️ Duplicate comments (2)
packages/chat-adapter-email/src/utils.ts (1)
10-14:⚠️ Potential issue | 🟡 MinorReplace regex parsing on untrusted email/HTML input.
These regexes are already flagged by CodeQL for uncontrolled input. Prefer bounded string scanning for address extraction and a linear text extractor for HTML fallback to avoid ReDoS noise and repeated security findings.
♻️ Proposed bounded parsing approach
-const EMAIL_ANGLE_BRACKET_RE = /<([^>]+)>/; const DISPLAY_NAME_RE = /^([^<]+)<[^>]+>$/; @@ export function parseEmailAddress(input: string): string { const trimmed = input.trim(); - const match = trimmed.match(EMAIL_ANGLE_BRACKET_RE); + const open = trimmed.indexOf('<'); + const close = trimmed.indexOf('>', open + 1); - return (match?.[1] ?? trimmed).toLowerCase(); + return (open >= 0 && close > open ? trimmed.slice(open + 1, close) : trimmed).toLowerCase(); } @@ export function stripHtml(html: string): string { - return html.replace(/<[^>]+>/g, '').trim(); + let output = ''; + let insideTag = false; + + for (const char of html) { + if (char === '<') { + insideTag = true; + continue; + } + + if (char === '>') { + insideTag = false; + continue; + } + + if (!insideTag) { + output += char; + } + } + + return output.trim(); }Also applies to: 29-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat-adapter-email/src/utils.ts` around lines 10 - 14, The parseEmailAddress function uses EMAIL_ANGLE_BRACKET_RE to parse untrusted input; replace the regex with a bounded, linear extractor: first trim and scan for the first '<' and the last '>' within a reasonable max length (e.g., 256 chars) and if both exist extract the substring between them as the address; if no angle brackets are present, treat the trimmed string as the address. For HTML fallback, implement a simple linear scanner that strips tags by skipping characters between '<' and '>' (no regex) and then apply the same bounded extraction logic. Ensure the logic lives in parseEmailAddress and remove reliance on EMAIL_ANGLE_BRACKET_RE to prevent ReDoS and CodeQL findings.packages/chat-adapter-email/src/message-renderer.ts (1)
48-50:⚠️ Potential issue | 🟡 MinorReuse the shared HTML-to-text helper instead of duplicating regex stripping.
This repeats the tag-stripping pattern already flagged by CodeQL. Import the utility once it is made linear/safe, and keep the rendering path consistent.
♻️ Proposed consolidation
import { EmailFormatConverter } from './format-converter.js'; +import { stripHtml } from './utils.js'; @@ function stripForText(html: string): string { - return html.replace(/<[^>]+>/g, '').trim(); + return stripHtml(html); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat-adapter-email/src/message-renderer.ts` around lines 48 - 50, The local stripForText function duplicates the project's shared HTML-to-text logic; remove stripForText and replace its usages with the centralized helper (e.g., htmlToText) to keep rendering consistent and safe. Import the shared helper at the top of the module, call it where stripForText was used (ensuring you still call .trim() if the helper does not already), and delete the stripForText declaration so all HTML-to-text conversion uses the single canonical utility.
🧹 Nitpick comments (3)
packages/chat-adapter-email/src/message-renderer.ts (1)
6-19: Export the public input/output types forrenderMessage.
renderMessageis exported, but its signature types are private. Exporting them makes this package API easier to consume and avoids unnamed declaration types leaking through generated.d.tsfiles.♻️ Proposed API polish
-interface RenderInput { +export interface RenderInput { text?: string; formatted?: Root; card?: CardNode; } -interface RenderOutput { +export interface RenderOutput { html: string; text: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat-adapter-email/src/message-renderer.ts` around lines 6 - 19, The RenderInput and RenderOutput interfaces used by the exported function renderMessage are currently internal; make them part of the public API by exporting them (export interface RenderInput { ... } and export interface RenderOutput { ... }) so the function signature in renderMessage: Promise<RenderOutput> and callers can rely on named types in generated .d.ts; update any imports/usage in this file or consumers to use the exported names and keep renderMessage itself exported as-is.apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.spec.ts (1)
90-160: Assert the new AGENT forwarding path.These tests now set up AGENT routes and HTTP/HMAC collaborators, but they only verify
SendWebhookMessage. Please also assert that the AGENT-only and fan-out cases callhttpClientService.requestwith the expected/v1/agents/agent-001/webhook/novu-email-agent-testtarget, payload, and HMAC headers; otherwise the core email-agent forwarding behavior can regress while this spec still passes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.spec.ts` around lines 90 - 160, The AGENT-only and fan-out tests currently only assert SendWebhookMessage; also assert that httpClientService.request is invoked with the agent-forwarding URL, payload, and HMAC headers: in the test that creates an AGENT route (makeVerifiedDomain with DomainRouteTypeEnum.AGENT and destination 'agent-001') add an assertion that httpClientService.request was called once with path '/v1/agents/agent-001/webhook/novu-email-agent-test' (or full request config containing that path), that the request body/payload matches the expected forwarded email payload, and that the headers include the generated HMAC header; repeat the same assertions in the fan-out test (where both WEBHOOK and AGENT routes exist) and ensure you reference httpClientService.request and strategy.execute when locating the code to modify.apps/dashboard/src/components/agents/email-setup-guide.tsx (1)
596-600: Move the static gradient into Tailwind classes.This inline
styleis static and can be expressed as an arbitrary Tailwind background utility.Suggested fix
<div className="absolute bottom-0 left-[22px] top-0 w-px" - style={{ - background: 'linear-gradient(to bottom, transparent 0%, `#E1E4EA` 10%, `#E1E4EA` 90%, transparent 100%)', - }} + className="absolute bottom-0 left-[22px] top-0 w-px bg-[linear-gradient(to_bottom,transparent_0%,`#E1E4EA_10`%,`#E1E4EA_90`%,transparent_100%)]" />As per coding guidelines,
apps/dashboard/src/**/*.{ts,tsx}: “Use Tailwind utility classes for all styling; avoid inlinestyleprops except for dynamic values that cannot be expressed as utilities”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/email-setup-guide.tsx` around lines 596 - 600, Replace the inline style on the div element with a Tailwind arbitrary background utility: remove the style prop on the div that currently has className "absolute bottom-0 left-[22px] top-0 w-px" and add an arbitrary class like bg-[linear-gradient(to_bottom,transparent_0%,`#E1E4EA_10`%,`#E1E4EA_90`%,transparent_100%)] (or bg-[linear-gradient(to%20bottom,transparent%200%,`#E1E4EA`%2010%,`#E1E4EA`%2090%,transparent%20100%)] depending on your Tailwind JIT escaping) so the static gradient is expressed entirely with Tailwind utilities; keep the existing positioning/size classes and remove the style prop.
🤖 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 344-362: The In-Reply-To and References header values passed into
mailOptions in buildSendEmailCallback must be RFC 5322 message-id wrapped in
angle brackets; update the header construction to defensively wrap
params.inReplyTo and params.references in <> (strip any existing surrounding <>
first) before assigning (handle References as a list or space/comma-separated
string by wrapping each message-id individually), so the headers sent via
handler.send are always RFC-compliant even if getReplyHeaders() returns bare
IDs.
In
`@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts`:
- Around line 121-134: seedEmailSecretKey currently copies the environment API
key into integration.credentials.secretKey which breaks on env key rotation and
expands blast radius; change seedEmailSecretKey to generate a dedicated
per-integration webhook secret (e.g., crypto.randomBytes(32).toString('hex')),
encrypt that value and persist it (e.g., into
integration.credentials.webhookSecret or replace secretKey with a
per-integration name) via integrationRepository.update, and update the worker
path (DomainRouteStrategy.handleAgentRoute) to read and use the stored
per-integration secret from the integration record for signing instead of
calling getDecryptedSecretKey at request time.
- Around line 90-119: enforceSingletonEmail performs an application-level
singleton check but is TOCTOU-racy; add a database-enforced unique compound
index (e.g., on agentIntegration collection: {_agentId: 1, providerId: 1,
_environmentId: 1, _organizationId: 1} or at minimum {_agentId: 1, providerId:
1}) to guarantee uniqueness, and keep the existing enforceSingletonEmail for
fast-fail UX; also update the create flow (where
agentIntegrationRepository.create/save is called—the addAgentIntegration use
case) to catch duplicate-key errors from the DB and translate them into a
ConflictException so concurrent requests get a safe, consistent error instead of
creating duplicates.
In
`@apps/dashboard/src/components/agents/agent-integration-guides/email-agent-integration-guide.tsx`:
- Around line 26-27: The current code computes integrationId as
integrationLink?.integration._id which only guards integrationLink and can throw
if integration is undefined; update the integrationId expression to use full
optional chaining (integrationLink?.integration?._id) so it safely handles
missing integration on AgentIntegrationLink, leaving isConnected as-is
(Boolean(integrationLink?.connectedAt)).
In `@apps/dashboard/src/components/agents/email-setup-guide.tsx`:
- Around line 330-345: The local-part input and domain select lack accessible
labels; update the input element tied to localPart (handlers: onLocalPartChange,
onLocalPartBlur) and the select bound to domainName (handler:
handleDomainSelect) to include accessible labeling—either add aria-label
attributes (e.g., aria-label="Inbound address local part" and
aria-label="Inbound domain") or add visible <label> elements associated via
htmlFor/id so screen readers can identify both controls; ensure the ids used on
the input and select match the labels and that any placeholder text remains
unchanged.
- Around line 556-567: The "Send test email" SetupButton (inside the SetupStep
for testStepIndex using deriveStepStatus and firstIncompleteStep) becomes
enabled but has no onClick handler, producing a clickable no-op; add a proper
onClick prop that invokes the test-email action (or a function like
handleSendTestEmail) and wire it into the component state/props so the button
triggers the test flow, or alternatively keep the button disabled until a test
handler exists by changing the disabled logic to also check for the presence of
the handler (e.g., disabled={firstIncompleteStep < testStepIndex ||
!handleSendTestEmail}) to ensure it cannot be clicked with no action.
- Around line 426-449: The saveCredentials function currently swallows
updateIntegration errors (via saveQueueRef.current.catch(() => undefined)),
which lets the UI advance while NovuAgent credentials weren't persisted; modify
the save queue handling so failures set an explicit save state/error (e.g.,
setCredentialSaveError or setCredentialsSaveState) and do not silently drop the
error: in saveCredentials (referencing saveQueueRef, credentialsRef,
updateIntegration, emailIntegration) attach .catch(err => {
setCredentialSaveError(err); /* keep queue healthy by returning undefined */
return undefined; }) and in the success branch clear that error and set a
“saved” state; expose this state to disable the test step (and show an error
message) until persistence succeeds. Ensure the queue still resolves to avoid
poisoning but surface the failure via the new state so the UI can react.
- Around line 387-401: The component initializes outboundId, localPart and
domainName from serverCredentials too early, so when emailIntegration loads
later those local states stay blank; add a useEffect that watches
emailIntegration or serverCredentials and updates the setOutboundId,
setLocalPart, and setDomainName when new saved credentials arrive (or when the
saved outboundIntegrationId changes) but only if the user hasn't already edited
the fields—implement a hasUserEdited ref (toggled true from the input onChange
handlers) and gate the state sync on !hasUserEdited.current to preserve dirty
edits; keep existing credentialsRef logic and update it inside the same or a
related effect.
In
`@apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.ts`:
- Line 229: The current date assignment in domain-route.strategy.ts uses
command.date instanceof Date which treats valid ISO string timestamps as invalid
and falls back to now; replace that logic in the object construction that sets
date (the line referencing command.date) with a parsing + validity check: create
const parsed = new Date(command.date) and if parsed is valid (e.g.,
!isNaN(parsed.getTime())) use parsed.toISOString(), otherwise fallback to new
Date().toISOString(); update the date field to use this parsed/validated value
so string timestamps are preserved.
- Around line 159-173: The code currently falls back to 'http://localhost:3000'
when API_ROOT_URL is unset and builds the webhook URL without encoding
identifiers or enforcing org-scoping; change it to require a configured API root
in production (do not silently default to localhost — throw or abort when
process.env.NODE_ENV === 'production' and API_ROOT_URL is missing), remove the
unsafe default fallback, and use encodeURIComponent(agentId) and
encodeURIComponent(integrationIdentifier) when building url; additionally,
update the integration lookup logic (the integration lookup in this domain route
strategy) to include an organization/environment scope filter (e.g., include
domain._organizationId or the correct org id alongside _environmentId) so
integrations cannot be resolved cross-org. Ensure these changes touch the code
that calls getDecryptedSecretKey.execute, buildNovuSignatureHeader, and
httpClientService.request in this DomainRoute strategy.
- Around line 182-206: resolveIntegrationIdentifier: the Mongo query passed to
integrationRepository.findOne currently filters only by _id and _environmentId
and must include _organizationId to enforce tenancy; update the query object in
resolveIntegrationIdentifier (and keep the existing error handling using
links[0]) to include _organizationId: organizationId, and either add a short
comment or an assertion documenting that links[0] is expected to be the correct
integration for this email-only flow (or validate links length/contents) to make
the singleton assumption explicit.
In `@packages/chat-adapter-email/package.json`:
- Around line 12-18: The new workspace package has a "build" script in its
package.json but the PR did not run the package build; run the build and include
any failures in the PR. From the repo root run either "pnpm -w -F
chat-adapter-email build" or change into packages/chat-adapter-email and run
"pnpm build", capture and fix any TypeScript/biome errors reported (or restore
missing files/config), re-run until success, and attach the build output (or
commit any necessary build-fix changes) to the PR; reference the "build" and
"prebuild" scripts in package.json when locating the task to execute.
In `@packages/chat-adapter-email/src/adapter.ts`:
- Around line 217-225: openDM currently generates and tracks a synthetic
messageId (via generateMessageId/hashMessageId and threadResolver.trackMessage)
before any outbound email is actually sent, causing subsequent postMessage to
include In-Reply-To/References headers that point to non-existent messages;
change the flow so you do NOT create or track the synthetic root message prior
to the first outbound send—either delay calling threadResolver.trackMessage
until after the first real message is successfully sent (and use the real
message-id from the sent email), or add a flag on the thread (or to postMessage)
to detect "no prior messages" and suppress adding In-Reply-To/References; apply
the same fix to the analogous logic referenced around lines 132-145.
- Around line 133-136: The subject construction can duplicate "Re:" prefixes;
update the logic around threadResolver.getSubject and the subject variable so
that if storedSubject already begins with a case-insensitive "Re:" (with
optional whitespace) you do not add another "Re:". Normalize storedSubject by
trimming whitespace and checking /^re:\s*/i; if it matches, use storedSubject
as-is, otherwise set subject = `Re: ${storedSubject}`; when storedSubject is
falsy keep 'New message'. Apply this change where replyHeaders and storedSubject
are used to build subject.
In `@packages/chat-adapter-email/src/card-renderer.tsx`:
- Around line 64-95: Implement a URL-sanitization guard and apply it to image
and link rendering: create a small helper (e.g., isSafeUrl(url): boolean) inside
card-renderer.tsx that only allows safe schemes (https, http, mailto, and
optionally data for images) and rejects dangerous schemes like javascript:,
vbscript:, or malformed values; then use it wherever node.url or node.imageUrl
is used in the 'image', 'link-button', 'button' (when node.url present) and
'link' cases—if isSafeUrl returns false, omit the href/src (or render the
element as plain text/no-image) instead of directly passing the unsafe value to
Img, Button or Link so unsafe links/images are not emitted.
In `@packages/chat-adapter-email/src/message-parser.ts`:
- Around line 30-55: The metadata.dateSent assignment currently uses new
Date(raw.createdAt) which can produce an Invalid Date; update the message
construction in the function that returns new this.MessageClass(...) to validate
raw.createdAt (e.g., parse and check isNaN(date.getTime())) and if it's missing
or unparseable fall back to new Date() before assigning metadata.dateSent so
downstream serialization always has a valid Date object.
In `@packages/chat-adapter-email/src/thread-resolver.ts`:
- Around line 42-56: The current encodeThreadId/decodeThreadId pair is
vulnerable to colons in recipientAddress; update encodeThreadId to produce a
delimiter-safe representation (e.g., base64 or URL-safe encoding) of
recipientAddress and ensure decodeThreadId decodes that back, or alternatively
change decodeThreadId to split from the right using lastIndexOf so it treats
everything before the final colon(s) as the address and the final segment as
rootMessageIdHash; specifically modify encodeThreadId (function encodeThreadId)
to encode recipientAddress and modify decodeThreadId (function decodeThreadId)
to reverse that encoding (or use lastIndexOf-based splitting and validate parts)
so addresses containing ':' no longer break parsing.
In `@packages/chat-adapter-email/src/utils.ts`:
- Around line 23-27: The generateMessageId function currently trusts
fromAddress.split('@')[1]; validate and sanitize that derived domain before
using it in the Message-ID: trim whitespace and surrounding brackets, reject any
value containing CRLF or characters outside allowed domain-label charset
(letters, digits, hyphen, dots), and if the sanitized value does not match a
safe domain pattern (e.g. DNS labels separated by dots), use the fallback
'novu.co' for the domain; keep using randomUUID() for the local part and ensure
the final return still formats as `<${randomUUID()}@${domain}>`.
In `@packages/chat-adapter-email/src/webhook-handler.ts`:
- Around line 46-72: In verifySignature, ensure you reject future-skewed
timestamps by treating negative ages as invalid: after parsing timestamp and
computing age in the verifySignature method, update the freshness check that
currently tests Number.isNaN(age) || age > MAX_TIMESTAMP_AGE_MS to also reject
age < 0 (i.e., Number.isNaN(age) || age < 0 || age > MAX_TIMESTAMP_AGE_MS), so
timestamps ahead of Date.now() fail the verification; reference the timestamp,
age, and MAX_TIMESTAMP_AGE_MS symbols in your change to locate the check.
In `@packages/shared/src/entities/integration/credential.interface.ts`:
- Around line 57-60: The CredentialsKeyEnum is missing entries for the new email
routing credential fields defined on ICredentials; update the CredentialsKeyEnum
to include replyDomain, outboundIntegrationId, inboundAddress, and inboundDomain
so type checks align with usages in chat-sdk.service.ts and
email-setup-guide.tsx; modify the enum declaration (CredentialsKeyEnum) in the
shared credential interface module to add these four keys exactly as named to
restore type consistency with ICredentials.
---
Duplicate comments:
In `@packages/chat-adapter-email/src/message-renderer.ts`:
- Around line 48-50: The local stripForText function duplicates the project's
shared HTML-to-text logic; remove stripForText and replace its usages with the
centralized helper (e.g., htmlToText) to keep rendering consistent and safe.
Import the shared helper at the top of the module, call it where stripForText
was used (ensuring you still call .trim() if the helper does not already), and
delete the stripForText declaration so all HTML-to-text conversion uses the
single canonical utility.
In `@packages/chat-adapter-email/src/utils.ts`:
- Around line 10-14: The parseEmailAddress function uses EMAIL_ANGLE_BRACKET_RE
to parse untrusted input; replace the regex with a bounded, linear extractor:
first trim and scan for the first '<' and the last '>' within a reasonable max
length (e.g., 256 chars) and if both exist extract the substring between them as
the address; if no angle brackets are present, treat the trimmed string as the
address. For HTML fallback, implement a simple linear scanner that strips tags
by skipping characters between '<' and '>' (no regex) and then apply the same
bounded extraction logic. Ensure the logic lives in parseEmailAddress and remove
reliance on EMAIL_ANGLE_BRACKET_RE to prevent ReDoS and CodeQL findings.
---
Nitpick comments:
In `@apps/dashboard/src/components/agents/email-setup-guide.tsx`:
- Around line 596-600: Replace the inline style on the div element with a
Tailwind arbitrary background utility: remove the style prop on the div that
currently has className "absolute bottom-0 left-[22px] top-0 w-px" and add an
arbitrary class like
bg-[linear-gradient(to_bottom,transparent_0%,`#E1E4EA_10`%,`#E1E4EA_90`%,transparent_100%)]
(or
bg-[linear-gradient(to%20bottom,transparent%200%,`#E1E4EA`%2010%,`#E1E4EA`%2090%,transparent%20100%)]
depending on your Tailwind JIT escaping) so the static gradient is expressed
entirely with Tailwind utilities; keep the existing positioning/size classes and
remove the style prop.
In
`@apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.spec.ts`:
- Around line 90-160: The AGENT-only and fan-out tests currently only assert
SendWebhookMessage; also assert that httpClientService.request is invoked with
the agent-forwarding URL, payload, and HMAC headers: in the test that creates an
AGENT route (makeVerifiedDomain with DomainRouteTypeEnum.AGENT and destination
'agent-001') add an assertion that httpClientService.request was called once
with path '/v1/agents/agent-001/webhook/novu-email-agent-test' (or full request
config containing that path), that the request body/payload matches the expected
forwarded email payload, and that the headers include the generated HMAC header;
repeat the same assertions in the fan-out test (where both WEBHOOK and AGENT
routes exist) and ensure you reference httpClientService.request and
strategy.execute when locating the code to modify.
In `@packages/chat-adapter-email/src/message-renderer.ts`:
- Around line 6-19: The RenderInput and RenderOutput interfaces used by the
exported function renderMessage are currently internal; make them part of the
public API by exporting them (export interface RenderInput { ... } and export
interface RenderOutput { ... }) so the function signature in renderMessage:
Promise<RenderOutput> and callers can rely on named types in generated .d.ts;
update any imports/usage in this file or consumers to use the exported names and
keep renderMessage itself exported as-is.
🪄 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: bbed7cc8-3f79-44ea-a9bc-17b613bc4bc0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
apps/api/package.jsonapps/api/src/app/agents/dtos/agent-platform.enum.tsapps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.tsapps/api/src/app/agents/utils/provider-to-platform.tsapps/dashboard/src/components/agents/agent-integration-guides/email-agent-integration-guide.tsxapps/dashboard/src/components/agents/agent-integration-guides/resolve-agent-integration-guide.tsxapps/dashboard/src/components/agents/agent-setup-guide.tsxapps/dashboard/src/components/agents/email-setup-guide.tsxapps/dashboard/src/components/agents/provider-dropdown.tsxapps/dashboard/src/components/integrations/components/integrations-list.tsxapps/dashboard/src/utils/provider-square-icon.tsapps/worker/src/app/shared/shared.module.tsapps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.spec.tsapps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.tslibs/application-generic/src/dtos/credentials.dto.tslibs/dal/src/repositories/integration/integration.schema.tspackages/chat-adapter-email/package.jsonpackages/chat-adapter-email/src/adapter.tspackages/chat-adapter-email/src/card-renderer.tsxpackages/chat-adapter-email/src/format-converter.tspackages/chat-adapter-email/src/index.tspackages/chat-adapter-email/src/message-parser.tspackages/chat-adapter-email/src/message-renderer.tspackages/chat-adapter-email/src/thread-resolver.tspackages/chat-adapter-email/src/types.tspackages/chat-adapter-email/src/utils.tspackages/chat-adapter-email/src/webhook-handler.tspackages/chat-adapter-email/tsconfig.jsonpackages/shared/src/consts/providers/channels/email.tspackages/shared/src/consts/providers/conversational-providers.tspackages/shared/src/consts/providers/providers.tspackages/shared/src/entities/integration/credential.interface.tspackages/shared/src/types/agent.tspackages/shared/src/types/index.tspackages/shared/src/types/providers.ts
commit: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
packages/chat-adapter-email/src/card-renderer.tsx (1)
77-108:⚠️ Potential issue | 🟡 MinorOmit unsafe card media/actions instead of rendering broken fallbacks.
Line 80 still emits
<Img src="">when both URLs are unsafe/missing, and Lines 91/108 turn unsafe links into clickable#actions. Prefer no image and plain text/non-clickable action for rejected URLs.Proposed refinement
case 'image': - return ( - <Img - src={safeUrl(node.url) ?? safeUrl(node.imageUrl) ?? ''} - alt={node.label || ''} - style={{ maxWidth: '100%', margin: '8px 0' }} - /> - ); + { + const src = safeUrl(node.url) ?? safeUrl(node.imageUrl); + + return src ? ( + <Img + src={src} + alt={node.label || ''} + style={{ maxWidth: '100%', margin: '8px 0' }} + /> + ) : null; + } case 'actions': return <Section style={{ margin: '12px 0' }}>{renderChildren(node.children)}</Section>; case 'link-button': - return ( - <Button href={safeUrl(node.url) ?? '#'} style={{ padding: '8px 16px', margin: '4px', backgroundColor: '#0066cc', color: '#ffffff', borderRadius: '4px', fontSize: '14px', textDecoration: 'none' }}> - {node.label || ''} - </Button> - ); + { + const href = safeUrl(node.url); + + return href ? ( + <Button href={href} style={{ padding: '8px 16px', margin: '4px', backgroundColor: '#0066cc', color: '#ffffff', borderRadius: '4px', fontSize: '14px', textDecoration: 'none' }}> + {node.label || ''} + </Button> + ) : ( + <Text>{node.label || ''}</Text> + ); + } @@ case 'link': - return <Link href={safeUrl(node.url) ?? '#'} style={{ color: '#0066cc' }}>{node.label || ''}</Link>; + { + const href = safeUrl(node.url); + + return href ? <Link href={href} style={{ color: '#0066cc' }}>{node.label || ''}</Link> : <Text>{node.label || ''}</Text>; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/chat-adapter-email/src/card-renderer.tsx` around lines 77 - 108, The card renderer currently renders broken fallbacks for unsafe/missing URLs; update the cases using safeUrl (the 'image' case that returns <Img>, the 'link-button' and 'link' cases that use href="#" and the 'button' case) so that if safeUrl(node.url) (or safeUrl(node.imageUrl) for images) is falsy you do NOT render a clickable element or an empty <Img>; instead render nothing for images and render a non-clickable Text/plain-styled element for actions/links (preserving visual styling) so only valid URLs produce <Img>, <Button href=...>, or <Link href=...>. Use the existing safeUrl function and node properties to decide rendering in Img, Button, and Link branches.apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.ts (1)
185-208:⚠️ Potential issue | 🟠 MajorResolve the Novu Email integration explicitly instead of using
links[0].
findLinksForAgents()returns every integration linked to the agent, solinks[0]can be Slack/Chat/etc. when the agent has multiple integrations. That forwards the inbound email to the wrongintegrationIdentifier.Suggested fix
import { DomainRouteTypeEnum, DomainStatusEnum, + EmailProviderIdEnum, EmailWebhookPayload, WebhookEventEnum, WebhookObjectTypeEnum, } from '@novu/shared'; @@ - const integration = await this.integrationRepository.findOne( - { _id: links[0]._integrationId, _environmentId: environmentId, _organizationId: organizationId }, - 'identifier' - ); + const integrationIds = links.map((link) => link._integrationId); + const integration = await this.integrationRepository.findOne( + { + _id: { $in: integrationIds }, + _environmentId: environmentId, + _organizationId: organizationId, + providerId: EmailProviderIdEnum.NovuAgent, + }, + 'identifier' + ); if (!integration) { - this.throwError(`Integration ${links[0]._integrationId} not found for agent ${agentId}`); + this.throwError(`Novu Email integration not found for agent ${agentId}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.ts` around lines 185 - 208, resolveIntegrationIdentifier currently uses links[0] which can pick the wrong integration when an agent has multiple links; update the function (resolveIntegrationIdentifier) to iterate over the results of agentIntegrationRepository.findLinksForAgents and for each link call integrationRepository.findOne({_id: link._integrationId, _environmentId: environmentId, _organizationId: organizationId}, 'identifier') until you find the integration whose identifier (or provider/type) matches the Novu Email integration (use the known identifier string/constant for Novu Email), return that identifier, and if none match throw the existing error; this ensures you select the Novu Email integration rather than blindly using links[0].apps/dashboard/src/components/agents/email-setup-guide.tsx (1)
484-510:⚠️ Potential issue | 🟠 MajorTrack credential save state before enabling the test step.
The error toast keeps failures visible, but the test button can still run while a queued save is pending or after it failed, so the backend may read stale
inboundAddress/inboundDomain. Also preserve existingemailIntegration.configurationsinstead of overwriting them with{}.Suggested fix direction
+ const [credentialSaveState, setCredentialSaveState] = useState<'idle' | 'saving' | 'saved' | 'error'>('idle'); + function saveCredentials(patch: Record<string, unknown>) { if (!emailIntegration) return; + setCredentialSaveState('saving'); credentialsRef.current = { ...credentialsRef.current, ...patch }; const snapshot = { ...credentialsRef.current }; @@ active: emailIntegration.active, primary: emailIntegration.primary ?? false, credentials: snapshot, - configurations: {}, + configurations: emailIntegration.configurations ?? {}, check: false, }, }) ) - .then(() => undefined) + .then(() => { + setCredentialSaveState('saved'); + }) .catch((err: unknown) => { const message = err instanceof Error ? err.message : 'Could not save credentials.'; + setCredentialSaveState('error'); showErrorToast(message, 'Settings not saved'); }); } @@ - disabled={firstIncompleteStep < testStepIndex || testEmailMutation.isPending} + disabled={ + firstIncompleteStep < testStepIndex || + testEmailMutation.isPending || + credentialSaveState === 'saving' || + credentialSaveState === 'error' + }Also applies to: 648-666
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/email-setup-guide.tsx` around lines 484 - 510, The saveCredentials function currently overwrites configurations with {} and doesn't expose the pending save state, allowing the test button to run against stale data; update saveCredentials to (1) include the existing configurations by passing emailIntegration.configurations ?? {} into updateIntegration instead of {} and (2) propagate the save promise/state so callers can wait for completion (e.g., return the chained promise from saveCredentials or assign it to a dedicated lastSavePromiseRef) and keep the existing saveQueueRef chaining and error handling (use saveQueueRef, credentialsRef, updateIntegration and showErrorToast as-is). Ensure callers that run the test step await the returned promise or check the last-save state before enabling the test.
🧹 Nitpick comments (3)
apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.ts (1)
211-233: Extract date normalization out of the inline IIFE.The current one-liner works, but it obscures the payload construction and violates the return-spacing guideline.
Suggested refactor
private buildWebhookPayload(command: InboundEmailParseCommand): EmailWebhookPayload { const from = command.from[0]; const refs = normalizeReferences(command.references); + const parsedDate = new Date(command.date as unknown as string); + const date = Number.isNaN(parsedDate.getTime()) ? new Date().toISOString() : parsedDate.toISOString(); return { @@ - date: (() => { const d = new Date(command.date as unknown as string); return Number.isNaN(d.getTime()) ? new Date().toISOString() : d.toISOString(); })(), + date, }; }As per coding guidelines,
**/*.{ts,tsx,js,jsx}: “Include a blank line before everyreturnstatement”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.ts` around lines 211 - 233, In buildWebhookPayload, remove the inline IIFE used to normalize command.date and instead compute a normalizedDate variable above the return (e.g., const normalizedDate = ... using new Date(...) and fallback to new Date().toISOString() when invalid), ensure there is a blank line before the return statement per the return-spacing guideline, and replace the IIFE in the returned object with date: normalizedDate; reference the buildWebhookPayload function and the date field in the EmailWebhookPayload.packages/chat-adapter-email/src/adapter.ts (1)
135-139: Replace the nested ternary in subject construction.This is easier to scan as a small helper/branch and avoids the repository’s no-nested-ternary rule.
Suggested refactor
const messageId = generateMessageId(this.config.fromAddress); const replyHeaders = await this.threadResolver.getReplyHeaders(threadId); const storedSubject = await this.threadResolver.getSubject(threadId); - const subject = storedSubject - ? /^re:/i.test(storedSubject) - ? storedSubject - : `Re: ${storedSubject}` - : 'New message'; + let subject = 'New message'; + if (storedSubject) { + subject = /^re:/i.test(storedSubject) ? storedSubject : `Re: ${storedSubject}`; + }As per coding guidelines,
**/*.{ts,tsx,js,jsx}: “Do not use nested ternaries”.🤖 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 135 - 139, The nested ternary used to compute subject (using storedSubject and /^re:/i.test(storedSubject)) violates the no-nested-ternary rule; replace it with a small helper or simple branching: compute subject by first checking storedSubject, then if present check the /^re:/i test and set subject accordingly, otherwise set 'New message'—update the logic near the subject assignment (referencing the storedSubject variable and the subject constant) to use an if/else or a named function like buildReplySubject to make the intent explicit.apps/dashboard/src/components/agents/email-setup-guide.tsx (1)
512-530: WrapupdateDomainin a React Query mutation and invalidate domains.This direct server mutation bypasses the dashboard server-state pattern and leaves
domainsstale, which can cause repeated route writes from olddomain.routes.Suggested fix direction
+ const queryClient = useQueryClient(); + const upsertDomainRouteMutation = useMutation({ + mutationFn: ({ domainId, routes }: { domainId: string; routes: DomainResponse['routes'] }) => + updateDomain(domainId, { routes }, requireEnvironment(currentEnvironment, 'No environment selected')), + onSuccess: () => { + void queryClient.invalidateQueries({ queryKey: [QueryKeys.fetchDomains, currentEnvironment?._id] }); + }, + onError: () => { + showErrorToast('Could not create inbound route on the domain.', 'Route creation failed'); + }, + }); + function upsertAgentRoute(address: string, domain: DomainResponse) { if (!currentEnvironment || !agent._id) return; @@ - updateDomain(domain._id, { routes: updatedRoutes }, currentEnvironment).catch(() => { - showErrorToast('Could not create inbound route on the domain.', 'Route creation failed'); - }); + upsertDomainRouteMutation.mutate({ domainId: domain._id, routes: updatedRoutes }); }As per coding guidelines,
apps/dashboard/src/**/*.{ts,tsx}: “Use TanStack Query (useQuery,useMutation) for all server state” and “Invalidate related queries after mutations”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/email-setup-guide.tsx` around lines 512 - 530, The upsertAgentRoute function is performing a direct server call via updateDomain which bypasses the app's TanStack Query pattern and will leave the cached domains stale; replace the direct call with a React Query mutation (useMutation) that calls updateDomain and in the mutation's onSuccess handler invalidate the domains query (invalidateQueries using the same query key used to fetch domains) so the updated domain.routes are refetched; keep the same route construction logic (address, DomainRouteTypeEnum.AGENT, destination: agent._id) and surface errors via the mutation's onError to showErrorToast, and ensure the mutation is used inside the component where upsertAgentRoute is defined so it can access currentEnvironment and agent._id.
🤖 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/agents.controller.ts`:
- Around line 263-286: The sendAgentTestEmail controller method is missing an
OpenAPI success response; add an `@ApiResponse/`@ApiOkResponse decorator
documenting the 200 response shape (e.g., { success: boolean }) for the
sendAgentTestEmail function so the endpoint contract is complete. Either create
or reuse a simple DTO (e.g., SuccessResponseDto with a boolean success property)
and annotate sendAgentTestEmail with `@ApiOkResponse`({ type: SuccessResponseDto
}) or use `@ApiResponse`({ status: 200, description: 'Success', schema: { example:
{ success: true } } }) to satisfy the documentation requirement for
SendAgentTestEmailCommand responses. Ensure the decorator sits alongside the
existing `@ApiOperation` and `@RequirePermissions` decorators.
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 326-350: The lookup for outboundIntegrationId currently only
filters by id/environment/organization and can return non-email or inbound
integrations causing MailFactory.getHandler to throw; update the
integrationRepository.findOne call used to fetch integration (the record read
before decryptCredentials and MailFactory.getHandler) to also require channel:
EMAIL and exclude the inbound provider (e.g. providerId !== 'NovuAgent' or add a
condition to filter out the NovuAgent provider), then proceed with the existing
active check, decryptCredentials, and MailFactory.getHandler; this ensures only
active email outbound integrations reach MailFactory.getHandler.
In
`@apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts`:
- Around line 47-49: The test currently ignores the agent email integration's
outboundIntegrationId; update the sender integration resolution in
send-agent-test-email.usecase (where you call findSenderIntegration, create
MailFactory(), and call mailFactory.getHandler) to first look up the integration
referenced by the agent email integration's outboundIntegrationId and use that
as senderIntegration (fall back to existing logic only if that
outboundIntegrationId is missing); apply the same change for the other
occurrences in this file (the block around lines 75-109) so the
MailFactory.getHandler is always given the agent's configured outbound provider
rather than an arbitrary active provider.
- Around line 27-45: The current integrationRepository.findOne call returns any
NovuAgent Email integration in the environment and can pick another agent's
integration; update the query used in send-agent-test-email.usecase (the
integrationRepository.findOne call that sets emailIntegration) to include the
agent link from the incoming command (e.g., add a filter such as _agentId:
command.agentId or agentIdentifier: command.identifier) so you fetch the
NovuAgent integration tied to the specific agent being tested; keep the existing
inboundAddress/inboundDomain checks and the to =
`${inboundAddress}@${inboundDomain}` logic unchanged.
- Around line 53-65: The email body currently interpolates user-controlled
agent.name directly into the HTML (and the subject), which allows injected
markup; update the send-agent-test-email logic to HTML-escape agent.name before
interpolation (replace &, <, >, " and ' with their HTML entities) and use the
escaped value in the subject and in the html template array (the places
referencing agent.name in the subject template and the
`<strong>${agent.name}</strong>` interpolation). Ensure the escaping function is
applied where the html array and subject are constructed so no raw agent.name is
injected into the email body.
In `@packages/chat-adapter-email/src/thread-resolver.ts`:
- Around line 127-132: The code assumes each element of parsed (from
JSON.parse(trimmed)) is a string and calls s.trim(), which throws for non-string
items like numbers; update the array handling so you only call .trim() on actual
strings (e.g., filter by typeof s === 'string' before mapping) or coerce safely
(e.g., String(s)) if intended, then push the trimmed/validated values into ids;
adjust the block around parsed, ids, and trimmed in thread-resolver.ts to guard
parsed array elements before calling .trim() so malformed-but-valid arrays like
[123] do not throw.
---
Duplicate comments:
In `@apps/dashboard/src/components/agents/email-setup-guide.tsx`:
- Around line 484-510: The saveCredentials function currently overwrites
configurations with {} and doesn't expose the pending save state, allowing the
test button to run against stale data; update saveCredentials to (1) include the
existing configurations by passing emailIntegration.configurations ?? {} into
updateIntegration instead of {} and (2) propagate the save promise/state so
callers can wait for completion (e.g., return the chained promise from
saveCredentials or assign it to a dedicated lastSavePromiseRef) and keep the
existing saveQueueRef chaining and error handling (use saveQueueRef,
credentialsRef, updateIntegration and showErrorToast as-is). Ensure callers that
run the test step await the returned promise or check the last-save state before
enabling the test.
In
`@apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.ts`:
- Around line 185-208: resolveIntegrationIdentifier currently uses links[0]
which can pick the wrong integration when an agent has multiple links; update
the function (resolveIntegrationIdentifier) to iterate over the results of
agentIntegrationRepository.findLinksForAgents and for each link call
integrationRepository.findOne({_id: link._integrationId, _environmentId:
environmentId, _organizationId: organizationId}, 'identifier') until you find
the integration whose identifier (or provider/type) matches the Novu Email
integration (use the known identifier string/constant for Novu Email), return
that identifier, and if none match throw the existing error; this ensures you
select the Novu Email integration rather than blindly using links[0].
In `@packages/chat-adapter-email/src/card-renderer.tsx`:
- Around line 77-108: The card renderer currently renders broken fallbacks for
unsafe/missing URLs; update the cases using safeUrl (the 'image' case that
returns <Img>, the 'link-button' and 'link' cases that use href="#" and the
'button' case) so that if safeUrl(node.url) (or safeUrl(node.imageUrl) for
images) is falsy you do NOT render a clickable element or an empty <Img>;
instead render nothing for images and render a non-clickable Text/plain-styled
element for actions/links (preserving visual styling) so only valid URLs produce
<Img>, <Button href=...>, or <Link href=...>. Use the existing safeUrl function
and node properties to decide rendering in Img, Button, and Link branches.
---
Nitpick comments:
In `@apps/dashboard/src/components/agents/email-setup-guide.tsx`:
- Around line 512-530: The upsertAgentRoute function is performing a direct
server call via updateDomain which bypasses the app's TanStack Query pattern and
will leave the cached domains stale; replace the direct call with a React Query
mutation (useMutation) that calls updateDomain and in the mutation's onSuccess
handler invalidate the domains query (invalidateQueries using the same query key
used to fetch domains) so the updated domain.routes are refetched; keep the same
route construction logic (address, DomainRouteTypeEnum.AGENT, destination:
agent._id) and surface errors via the mutation's onError to showErrorToast, and
ensure the mutation is used inside the component where upsertAgentRoute is
defined so it can access currentEnvironment and agent._id.
In
`@apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.ts`:
- Around line 211-233: In buildWebhookPayload, remove the inline IIFE used to
normalize command.date and instead compute a normalizedDate variable above the
return (e.g., const normalizedDate = ... using new Date(...) and fallback to new
Date().toISOString() when invalid), ensure there is a blank line before the
return statement per the return-spacing guideline, and replace the IIFE in the
returned object with date: normalizedDate; reference the buildWebhookPayload
function and the date field in the EmailWebhookPayload.
In `@packages/chat-adapter-email/src/adapter.ts`:
- Around line 135-139: The nested ternary used to compute subject (using
storedSubject and /^re:/i.test(storedSubject)) violates the no-nested-ternary
rule; replace it with a small helper or simple branching: compute subject by
first checking storedSubject, then if present check the /^re:/i test and set
subject accordingly, otherwise set 'New message'—update the logic near the
subject assignment (referencing the storedSubject variable and the subject
constant) to use an if/else or a named function like buildReplySubject to make
the intent explicit.
🪄 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: eeccd431-e490-457b-9480-cfd0eb548211
📒 Files selected for processing (18)
apps/api/src/app/agents/agents.controller.tsapps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/app/agents/usecases/index.tsapps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.command.tsapps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.tsapps/dashboard/src/api/agents.tsapps/dashboard/src/components/agents/agent-integration-guides/email-agent-integration-guide.tsxapps/dashboard/src/components/agents/email-setup-guide.tsxapps/worker/src/app/workflow/specs/inbound-email-parse.spec.tsapps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.tspackages/chat-adapter-email/src/adapter.tspackages/chat-adapter-email/src/card-renderer.tsxpackages/chat-adapter-email/src/message-parser.tspackages/chat-adapter-email/src/message-renderer.tspackages/chat-adapter-email/src/thread-resolver.tspackages/chat-adapter-email/src/utils.tspackages/chat-adapter-email/src/webhook-handler.tspackages/shared/src/types/providers.ts
✅ Files skipped from review due to trivial changes (3)
- apps/dashboard/src/api/agents.ts
- apps/api/src/app/agents/usecases/index.ts
- packages/chat-adapter-email/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/types/providers.ts
- apps/dashboard/src/components/agents/agent-integration-guides/email-agent-integration-guide.tsx
- packages/chat-adapter-email/src/webhook-handler.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/api/src/app/agents/services/chat-sdk.service.ts (1)
314-376: LGTM on the outbound email callback — prior review concerns addressed.The past feedback is resolved:
- Integration lookup constrained by
channel: EMAIL(line 330) andNovuAgentexplicitly rejected (lines 339–343), with a clear inactive-integration error (line 346–348).- RFC 5322 angle-bracket wrapping applied to
Message-ID,In-Reply-To, andReferencesviawrapMsgId(lines 366–368), withReferencescorrectly re-wrapping each individual id.One small optional refactor: the inline callback parameter type on line 317 is long enough to hurt readability — consider extracting it to a named interface (e.g.,
SendEmailCallbackParams) alongside the existing DTOs, which would also be reusable by the adapter package's type declaration.♻️ Optional: extract the callback param type
+interface SendEmailCallbackParams { + to: string; + subject: string; + html: string; + text?: string; + inReplyTo?: string; + references?: string; + messageId?: string; +} + private buildSendEmailCallback( config: ResolvedAgentConfig, outboundIntegrationId: string | undefined - ): (params: { to: string; subject: string; html: string; text?: string; inReplyTo?: string; references?: string; messageId?: string }) => Promise<{ messageId: string }> { + ): (params: SendEmailCallbackParams) => 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 314 - 376, The inline callback parameter type in buildSendEmailCallback is long and hurts readability; extract it to a named interface (e.g., SendEmailCallbackParams) and replace the inline type in the returned function signature with that interface. Update the signature of buildSendEmailCallback's returned function and any related usages to reference SendEmailCallbackParams so the type is reusable (use the names buildSendEmailCallback, SendEmailCallbackParams, and the returned callback function in your changes).apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts (1)
152-158: Remove the unnecessaryas unknown as stringcast.The
as unknown as stringcast on line 157 bypasses type checking and is not needed. TheIntegrationRepository.countActiveExcludingNovu()method already demonstrates the correct pattern for using$ninwith enum values — simply pass the operator object directly without casting:providerId: { $nin: [EmailProviderIdEnum.NovuAgent, EmailProviderIdEnum.Novu] }The query type
FilterQuery<IntegrationDBModel> & EnforceEnvOrOrgIdsproperly supports MongoDB operators, and theproviderIdfield is typed asString, so the array of enum values is already compatible. Remove the cast to maintain type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts` around lines 152 - 158, Remove the unnecessary type cast on the $nin operator in the query that fetches anyEmailProvider: update the call to this.integrationRepository.findOne used in send-agent-test-email.usecase to pass providerId: { $nin: [EmailProviderIdEnum.NovuAgent, EmailProviderIdEnum.Novu] } directly (no "as unknown as string"), keeping ChannelTypeEnum.EMAIL and the existing environment/organization filters; this aligns with IntegrationRepository.countActiveExcludingNovu() and preserves type safety for providerId.
🤖 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/usecases/send-agent-test-email/send-agent-test-email.usecase.ts`:
- Around line 105-130: In findSenderIntegration
(send-agent-test-email.usecase.ts) change the control flow so that when an
outboundIntegrationId is provided but the integration lookup
(integrationRepository.findOne) yields no usable configured integration (null
because inactive, wrong channel, or missing), the method fails fast by throwing
a clear error (e.g., "Configured outbound integration not found or inactive")
instead of falling through to the Novu demo fallback; keep the existing
special-case handling for configured.providerId === EmailProviderIdEnum.Novu and
the decryptCredentials(...) path for valid configured integrations, and only
allow the Novu demo credentials fallback when outboundIntegrationId is not
provided at all.
In `@packages/chat-adapter-email/src/utils.ts`:
- Around line 32-34: The current stripHtml function fails to decode HTML
entities and mishandles edge-case attributes; replace its naive regex with a
proper HTML parser and entity decoder: update stripHtml to use a library such as
sanitize-html (or htmlparser2 + he for entity decoding) to parse the HTML,
remove disallowed tags/attributes, decode entities, and return trimmed plain
text; ensure message-parser.ts continues to call stripHtml (same function name)
so downstream code receives correctly parsed and decoded user-facing text.
---
Nitpick comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 314-376: The inline callback parameter type in
buildSendEmailCallback is long and hurts readability; extract it to a named
interface (e.g., SendEmailCallbackParams) and replace the inline type in the
returned function signature with that interface. Update the signature of
buildSendEmailCallback's returned function and any related usages to reference
SendEmailCallbackParams so the type is reusable (use the names
buildSendEmailCallback, SendEmailCallbackParams, and the returned callback
function in your changes).
In
`@apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts`:
- Around line 152-158: Remove the unnecessary type cast on the $nin operator in
the query that fetches anyEmailProvider: update the call to
this.integrationRepository.findOne used in send-agent-test-email.usecase to pass
providerId: { $nin: [EmailProviderIdEnum.NovuAgent, EmailProviderIdEnum.Novu] }
directly (no "as unknown as string"), keeping ChannelTypeEnum.EMAIL and the
existing environment/organization filters; this aligns with
IntegrationRepository.countActiveExcludingNovu() and preserves type safety for
providerId.
🪄 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: 11f87d55-c07e-4c7c-a7d0-72ddb4a23392
📒 Files selected for processing (6)
apps/api/src/app/agents/agents.controller.tsapps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.tspackages/chat-adapter-email/src/message-renderer.tspackages/chat-adapter-email/src/thread-resolver.tspackages/chat-adapter-email/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/chat-adapter-email/src/message-renderer.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/usecases/send-agent-test-email/send-agent-test-email.usecase.ts`:
- Around line 80-98: The subject currently uses escapedName which HTML-escapes
characters and will show entities in inboxes; change mailOptions.subject to use
the raw agent.name (e.g., `agent.name`) instead of `escapedName`, while keeping
`escapedName` only for interpolation inside the HTML body (mailOptions.html) so
user-visible subject remains plain text and the HTML body remains safe; update
the subject assignment in the send-agent-test-email use case
(mailOptions.subject) accordingly.
- Around line 39-60: The code incorrectly picks the first link with any
_integrationId via links.find((l) => l._integrationId) which can point to a
non-email integration; instead collect all link _integrationId values from links
(variable links returned by agentIntegrationRepository.findLinksForAgents),
query integrationRepository for integrations matching _id: { $in: ids },
_environmentId: command.environmentId, _organizationId: command.organizationId,
providerId: EmailProviderIdEnum.NovuAgent, channel: ChannelTypeEnum.EMAIL, then
locate the matching integration and its corresponding link (replace
emailLink/emailIntegration logic), and keep the same BadRequestException
behavior if no matching email integration/link is found.
In
`@apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.ts`:
- Around line 178-207: The resolveIntegration method can pick a non-email link
from findLinksForAgents; update resolveIntegration to filter links to the
email/NovuAgent integration before using links[0] (use ChannelTypeEnum.EMAIL or
EmailProviderIdEnum.NovuAgent from `@novu/shared`), e.g. call findLinksForAgents
or post-filter its result for link._channel === ChannelTypeEnum.EMAIL (or
link.provider === EmailProviderIdEnum.NovuAgent) and then pass that
link._integrationId into integrationRepository.findOne; also constrain the
integrationRepository.findOne query to the email provider/channel if possible
and keep the existing missing-secret check and decryptSecret usage.
🪄 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: 33eeac21-c4a9-419f-bccb-2bf39e0c2410
📒 Files selected for processing (8)
apps/api/src/app/agents/agents.controller.tsapps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.tsapps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.tsapps/worker/src/app/workflow/specs/inbound-email-parse.spec.tsapps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.spec.tsapps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.tspackages/chat-adapter-email/src/message-renderer.tspackages/chat-adapter-email/src/utils.ts
✅ Files skipped from review due to trivial changes (1)
- packages/chat-adapter-email/src/utils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/worker/src/app/workflow/specs/inbound-email-parse.spec.ts
- packages/chat-adapter-email/src/message-renderer.ts
- apps/worker/src/app/workflow/usecases/inbound-email-parse/strategies/domain-route.strategy.spec.ts
- apps/api/src/app/agents/agents.controller.ts
… conversations Add EMAIL platform enum and NovuAgent email provider as foundations for the upcoming Chat SDK email adapter integration. Made-with: Cursor
Implement a Chat SDK Adapter for email as a new internal ESM package at packages/chat-adapter-email/. Follows the same adapter pattern as @chat-adapter/slack, @chat-adapter/teams, and @chat-adapter/whatsapp so email enters through the unified webhook → Chat SDK → AgentInboundHandler pipeline with zero special-case branches. Key design choices: - Redis-backed thread state via Chat SDK StateAdapter (survives restarts, supports horizontal scaling) - Provider-agnostic sendEmail callback - HMAC-SHA256 webhook verification (same format as bridge signing) - Card rendering via @react-email/components for cross-client HTML - Markdown rendering via mdast-util-to-hast + hast-util-to-html Made-with: Cursor
Add EMAIL platform case to buildAdapters() that creates the email adapter via dynamic import of @novu/chat-adapter-email. The sendEmail callback loads the outbound provider by outboundIntegrationId, decrypts credentials, and sends through MailFactory with RFC 2822 threading headers (Message-ID, In-Reply-To, References). Add replyDomain and outboundIntegrationId to ICredentials so the NovuEmail integration can persist its email-specific configuration. Made-with: Cursor
…d agent emails Wire handleAgentRoute() in DomainRouteStrategy to forward inbound emails to the API's agent webhook endpoint. The worker resolves the integration identifier via AgentIntegrationRepository, maps the parsed email to EmailWebhookPayload, HMAC-signs it, and POSTs to /v1/agents/:agentId/webhook/:integrationIdentifier. Extract EmailWebhookPayload and NovuEmailAttachment to @novu/shared so the worker and chat-adapter-email share a single compile-time contract. Made-with: Cursor
…tial persistence Add the Email setup guide panel for configuring Novu Email agent integrations in the dashboard, with supporting backend and shared-layer fixes: - Email setup guide with outbound provider selection, inbound address configuration, domain picker, and credentials sidebar for non-demo providers - Auto-seed secretKey on agent integration link for HMAC webhook verification - Add email credential fields (replyDomain, outboundIntegrationId, inboundAddress, inboundDomain) to CredentialsDto, Mongoose schema, and ICredentials interface - Enforce singleton constraint (one Novu Email integration per agent) - Hide NovuAgent from integration store, prevent duplicate creation in dropdown - Race-safe credential persistence using ref + sequential save queue Made-with: Cursor
… picker Aligns the inbound domain selector with the existing design pattern used by the outbound provider dropdown (searchable, RiAddLine action). Made-with: Cursor
…domain route - Add POST /agents/:identifier/test-email endpoint that sends a test email to the configured inbound address via the Novu demo provider - Wire the "Send test email" button in the email setup guide with loading state and success/error toasts - Auto-create an agent route on the domain when the user configures the inbound local part + domain, so they don't need to manually set up routing in the domains page Made-with: Cursor
DomainRouteStrategy now depends on HttpClientService, GetDecryptedSecretKey, IntegrationRepository, and AgentIntegrationRepository (added in the worker webhook POST phase). The test module was missing stub providers for these, causing the beforeEach hook to fail. Made-with: Cursor
Security: - Prevent ReDoS: change [^>]+ to [^<>]* in HTML-stripping regexes - Validate domain with SAFE_DOMAIN_RE before embedding in Message-ID header - Add safeUrl() guard in card-renderer to block javascript: / malformed URLs - Reject future-skewed timestamps (>30s ahead) in webhook signature verification Correctness: - Remove trackMessage() from openDM() so the first outbound email has no spurious In-Reply-To pointing at a never-sent synthetic message ID - Avoid Re: Re: subject prefix when stored subject already starts with Re: - Guard invalid raw.createdAt with NaN-safe Date construction fallback - Fix date handling in domain-route: use new Date(command.date) instead of instanceof Date check which silently dropped valid ISO strings - encodeURIComponent on thread-resolver recipientAddress to handle edge-case colons in addresses - Wrap In-Reply-To / References header values in RFC 5322 angle brackets in buildSendEmailCallback - Throw early if API_ROOT_URL is unset to prevent silent localhost mis-routing - Add _organizationId filter to integration lookup in domain-route strategy - encodeURIComponent on agentId and integrationIdentifier in URL path Dashboard: - Fix state-sync bug: useState initializers run before integrations load; use a one-time useEffect to populate outboundId, localPart, domainName - Surface updateIntegration failures as error toast instead of swallowing - Add aria-label to inbound address input and domain select button - Remove unused needsInboundStep variable - Fix optional chain: integrationLink?.integration?._id Shared: - Add ReplyDomain, OutboundIntegrationId, InboundAddress, InboundDomain entries to CredentialsKeyEnum for consistency with ICredentials Made-with: Cursor
Made-with: Cursor
…ments Security: - Strip residual < > chars from stripHtml/stripForText output to silence CodeQL incomplete-sanitization findings (output is plain text, not HTML) - Escape agent.name before interpolating into test email HTML body Correctness: - Scope NovuAgent integration lookup in send-agent-test-email to the specific agent via AgentIntegrationRepository instead of an env-wide findOne that could pick another agent's integration - Use agent's configured outboundIntegrationId for the test send; fall back to Novu demo only if not set, matching what the actual email flow does - Guard JSON References array against non-string elements (numbers, nulls) before calling .trim() in thread-resolver extractMessageIds - Add channel: EMAIL constraint and NovuAgent guard to the outbound integration lookup in buildSendEmailCallback so a misconfigured non-email integration ID fails with a clear error instead of a cryptic MailFactory crash API: - Add 200 @apiresponse decorator to POST /:identifier/test-email endpoint Made-with: Cursor
Replace the env API key snapshot in `seedEmailSecretKey` with a `crypto.randomBytes(32)` dedicated secret so inbound email verification survives environment key rotation without breaking existing agents. The worker's `DomainRouteStrategy` now reads and decrypts the per-integration `credentials.secretKey` directly (via `decryptSecret`) instead of fetching the environment API key at request time, removing the `GetDecryptedSecretKey` dependency from both usecases. Unit tests updated to use `encryptSecret` with a known test key so the `decryptSecret` round-trip works correctly without touching env secrets. Made-with: Cursor
- Fix TS2353 compile error: remove invalid @apiresponse options-object usage in agents.controller.ts (custom decorator only accepts a DTO type) - fail-fast in findSenderIntegration when outboundIntegrationId is set but the integration lookup returns null, instead of silently falling through to the Novu demo fallback - Escape agent.name in test email subject (was only escaped in HTML body) - Replace regex-based stripHtml/stripForText with a character-scanning state machine — resolves CodeQL incomplete-multi-character-sanitization findings #241 and #242; add HTML entity decoder (named, decimal, hex) to stripHtml so inbound email text is properly decoded Made-with: Cursor
Both the test-email usecase and the worker's DomainRouteStrategy were picking the first linked integration ID without verifying its type. If an agent has multiple integrations (e.g. email + Slack), a non-email link could be selected, causing wrong-secret HMAC failures or spurious "not found" errors. Fix: collect all linked integration IDs, use $in with providerId: NovuAgent + channel: EMAIL so the DB always returns the correct integration regardless of link order. Also revert subject escaping: agent.name is used raw in the email subject (plain-text field — HTML entities look broken in inboxes); escapedName is kept for the HTML body only. Made-with: Cursor
ChmaraX
force-pushed
the
nv-7386-email-integration-for-conversational-flow
branch
from
64c33ed to
f2796ba
Compare
This was referenced Apr 23, 2026
feat(providers): forward email headers across all providers for reply threading fixes NV-7402
#10836
Merged
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
2 participants
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
Adds email as a first-class conversational agent platform in Novu, following the Chat SDK adapter pattern used by Slack, Teams, and WhatsApp.
What's included
packages/chat-adapter-email/) — implements theAdapterinterface for email with HMAC webhook verification, RFC 2822 thread resolution, and HTML rendering (markdown, cards via React Email)EMAILplatform case inbuildAdapters(),sendEmailcallback that routes replies through the user's chosen outbound provider viaMailFactoryDomainRouteStrategy.handleAgentRoute()forwards inbound emails toPOST /v1/agents/:agentId/webhook/:integrationIdentifierwith per-integration HMAC signaturePOST /v1/agents/:identifier/test-emailsends a real email to the agent's configured inbound address using the outbound provider (or Novu demo as fallback)replyDomain,outboundIntegrationId,inboundAddress,inboundDomain) toCredentialsDto, Mongoose schema, andICredentialsinterfacesecretKey— seeded withcrypto.randomBytes(32)on integration link; rotation-proof (decoupled from environment API key)Architecture
sequenceDiagram participant User as Email Sender participant SMTP as SMTP Server (inbound-mail) participant Worker as Worker (DomainRouteStrategy) participant API as API (webhook controller) participant SDK as ChatSdkService participant Adapter as NovuEmailAdapter participant Handler as AgentInboundHandler participant Bridge as Customer Bridge User->>SMTP: sends email to agent@domain.com SMTP->>Worker: BullMQ job Worker->>API: POST /agents/:id/webhook/:identifier (HMAC signed) API->>SDK: handleWebhook() SDK->>Adapter: parse & verify inbound email Adapter-->>SDK: fires onNewMention SDK->>Handler: unified inbound pipeline Handler->>Bridge: POST to bridge URL Bridge-->>Handler: agent reply Handler->>Adapter: thread.post(reply) Adapter->>API: sendEmail callback → MailFactory → outbound providerRemaining work (not in this PR)
In-Reply-To/Referencesfor SES, Mailgun, Postmark, etc.)Test plan
secretKeyseededoutboundIntegrationIdpersisted on integrationinboundAddress,inboundDomain,replyDomainpersistedswaks→ conversation created, fallback reply attemptedMade with Cursor
What changed
Email is added as a first-class conversational agent platform. A new ESM Chat SDK email adapter package implements inbound webhook parsing with timestamped HMAC verification, RFC‑2822 threading (In‑Reply‑To / References) with Redis-backed thread state, HTML/text rendering (Markdown + React Email cards), and outbound sending routed through the agent’s selected mail provider. API, worker, and dashboard flows are wired end‑to‑end: agents can be linked to an email integration, send test emails, and receive inbound email webhooks that are routed into agent conversations.
Affected areas
Key technical decisions
Testing
Unit tests updated for worker/domain-route wiring; a test plan and manual verification steps are included. E2E tests and backend support for the dashboard "send test email" flow are listed as follow-ups.