Conversation
…on fixes NOV-7404 Stores the selected integration ID in sessionStorage so navigating away and returning during onboarding does not reset the provider picker. Clears the entry automatically once the server confirms the integration link, at which point the agent data itself becomes the source of truth. Made-with: Cursor
…gent email fixes NV-7405 - Allow '*' as inbound address local part, routing all mail on a domain to the agent - Skip replyDomain auto-computation for catch-all; require an explicit Reply-from address instead - Show Reply-from input field in the UI only when catch-all mode is active - Gate the Test step on replyFrom being set in catch-all mode - Clear stale replyFrom state when switching away from catch-all - Use synthetic local part (novu-test-<agentId>) for test emails in catch-all mode so the probe triggers the wildcard route correctly - Extract all credential state/effects/handlers into useEmailSetupCredentials hook - CATCH_ALL_ADDRESS constant and deriveReplyDomain() helper replace bare '*' literals Made-with: Cursor
…nbound-address-support
- Remove unused outboundIntegration/isOutboundDemo from EmailSetupGuide destructuring - Fix stale replyDomain bug: explicitly clear replyDomain credential when switching inbound address to catch-all so server state matches UI intent - Fix re-init guard to key off emailIntegration._id rather than the object reference, enabling correct re-hydration when switching integrations - Add orphan route cleanup in upsertAgentRoute: also remove routes where destination === agent._id to prevent stale routes from a previous address - Add queryClient.invalidateQueries after domain route update for cache coherence - Merge two isCatchAll JSX blocks into one fragment - Add email validation on reply-from input with inline error state Made-with: Cursor
…etupCredentials Replace the direct updateDomain() promise chain in upsertAgentRoute with a useMutation that accepts domainId as a variable rather than a closure. This gives proper TanStack Query mutation semantics while working around the constraint that useUpdateDomain() bakes a single domainId into its closure at hook creation time. onSuccess invalidates the domains cache; onError surfaces the toast — removing the manual .then().catch() chain. Made-with: Cursor
…ata model consistency fixes NV-7406 - Make Domain.routes[] the single source of truth for inbound routing - Dynamic From: address resolution via Redis thread state per conversation - Cascade deletion on agent/integration removal (routes + NovuAgent integration) - Validate agent route destinations in UpdateDomain - MongoDB transactions with explicit ClientSession for atomicity - Multi-address configuration UI with outbound provider select - Derive credentials step completion from integration data (survives reload) - Extract OutboundProviderSelect and InboundAddressConfig into own files Made-with: Cursor
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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:
📝 WalkthroughWalkthroughController and DTOs now accept providerId and explicit targetAddress; add-integration supports providerId-based NovuAgent idempotent create/link. Test-email sends directly to provided targetAddress. Email adapter and ChatSdk shift “from” to per-message payload; credential fields and schema entries for inbound/reply were removed. Repos/usecases added transactional session support and domain-route cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as AgentsController
participant Usecase as AddAgentIntegrationUsecase
participant RepoInt as IntegrationRepository
participant RepoAgent as AgentIntegrationRepo
participant RepoDomain as DomainRepository
Client->>Controller: POST /agents/:id/integrations { integrationIdentifier?, providerId? }
Controller->>Usecase: execute(command)
alt providerId == NovuAgent
Usecase->>RepoInt: find by providerId (with session)
alt not found
Usecase->>RepoInt: create Integration (session)
Usecase->>RepoAgent: create AgentIntegration (session)
Usecase->>RepoInt: seed secretKey (session)
else exists
Usecase->>RepoAgent: link existing Integration (session)
end
Usecase->>RepoDomain: remove/add domain routes (session)
Usecase->>Usecase: commit transaction
else other provider
Usecase->>RepoInt: find by integrationIdentifier
Usecase->>RepoAgent: create AgentIntegration link
end
Usecase-->>Controller: linkedIntegration
Controller-->>Client: 200 { integration }
sequenceDiagram
participant Client
participant Controller as AgentsController
participant Usecase as SendAgentTestEmailUsecase
participant ChatSdk as ChatSdkService
Client->>Controller: POST /agents/:id/test-email { targetAddress }
Controller->>Usecase: execute(command with targetAddress)
Usecase->>ChatSdk: buildSendEmailCallback(params.from required)
Usecase->>ChatSdk: send email to command.targetAddress with from
ChatSdk-->>Usecase: send result
Usecase-->>Controller: { success }
Controller-->>Client: 200 { success: true }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
@novu/dal
@novu/shared
@novu/dashboard
@novu/api-service
labels
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…inbound-email-support-with-data-model Made-with: Cursor # Conflicts: # apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts # apps/dashboard/src/components/agents/email-setup-guide.tsx # apps/dashboard/src/components/agents/use-email-setup-credentials.ts
commit: |
![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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts (2)
66-86:⚠️ Potential issue | 🟡 MinorValidate
senderIntegration.credentials.frombefore send.
credentials?.from as stringsilently hides the case where the configured outbound integration has nofromvalue (e.g. user added the integration but hasn't filled in required fields). Depending on the downstream handler, this can produce an opaque provider error or, worse, a send attempt with an empty/undefinedFrom. GivenfindSenderIntegrationonly filters onactive: true, there's no guaranteefromis present.Consider asserting non-empty
from(and, if applicable,senderName) explicitly with a descriptiveBadRequestExceptionbefore buildingmailOptions.🤖 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 66 - 86, The code uses senderIntegration.credentials?.from when building mailOptions which can be undefined; before calling mailFactory.getHandler or constructing mailOptions (in send-agent-test-email.usecase.ts, around findSenderIntegration/senderIntegration usage), explicitly validate that senderIntegration.credentials?.from (and optionally credentials?.senderName) is a non-empty string and, if not, throw a BadRequestException with a clear message (e.g., "Outbound integration missing 'from' address"); only proceed to call mailFactory.getHandler and build mailOptions after these assertions pass.
60-88:⚠️ Potential issue | 🟠 MajorConsider abuse controls for
targetAddress.
command.targetAddressis accepted verbatim from the authenticated user and an email is sent through either Novu-hosted demo infrastructure (no-reply@novu.co) or the org's configured outbound provider. A malicious/curious authenticated user can use this to send "Test email for agent X" messages to arbitrary recipients.While the API is internal/early-access (per existing learning) and the body is canned, the risk includes:
- Abuse of Novu's demo outbound infrastructure to deliver templated emails to arbitrary third parties (minor phishing vector, deliverability reputation).
- No rate limiting evident on this use case.
Suggestions:
- Enforce a per-user/org rate limit (e.g. via a throttler guard) on this endpoint.
- For the
EmailProviderIdEnum.Novudemo branch, restricttargetAddressto the authenticated user's email (or a verified address), and allow arbitrary destinations only when a customer-owned outbound provider is configured.🤖 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 60 - 88, The code accepts command.targetAddress verbatim and sends test emails via the sender integration (see send-agent-test-email.usecase and the call to findSenderIntegration / handler.send), allowing abuse of Novu-hosted demo outbound infra; add enforcement to validate and restrict destinations: 1) add a rate limit/throttler guard around this use case to limit calls per user/org, 2) when senderIntegration.providerId === EmailProviderIdEnum.Novu only allow command.targetAddress to be the authenticated user's email (or a pre-verified address) and reject or return a 403 for other targets, and 3) permit arbitrary targetAddress only when the org has a non-Novu customer-owned outbound provider; apply these checks before creating MailFactory/getHandler and before calling handler.send.packages/chat-adapter-email/src/types.ts (1)
5-20:⚠️ Potential issue | 🟠 MajorBreaking public API change — two breaking modifications without deprecation cycle.
This file exports public types (
NovuEmailAdapterConfigandSendEmailParams). The changes break existing consumers:
- Removed
NovuEmailAdapterConfig.fromAddress(was required) andfromName(was optional) → replaced bysenderName?(optional)- Added
SendEmailParams.fromas required → consumers not passing this field will breakPer coding guidelines: "Treat all exported symbols in packages as public API; follow semver conventions with breaking changes requiring major bumps" and "Deprecate symbols with
@deprecatedJSDoc before removing them from packages".Note: No monorepo consumers of these types were found, so practical impact is confined to this package. However, this is marked
version: 0.0.1andprivate: true—if you intend this as pre-release with unstable API, document that clearly. Otherwise, add a deprecation cycle forfromAddressandfromNamebefore removal.🤖 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 5 - 20, The change removed the previously exported NovuEmailAdapterConfig.fromAddress/fromName and added senderName?, and also made SendEmailParams.from required—this is a breaking API change; restore backward compatibility by reintroducing fromAddress (required) and fromName (optional) into NovuEmailAdapterConfig and mark them with `@deprecated` JSDoc pointing to senderName (keep senderName as the new preferred field), and make SendEmailParams.from optional (or accept either from or fromAddress) so existing callers without from do not break; update JSDoc on SendEmailParams and NovuEmailAdapterConfig types (and package versioning/documentation) to reflect deprecation and migration path.apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts (1)
85-97:⚠️ Potential issue | 🟡 MinorRe-linking an existing NovuAgent integration by identifier rotates its
secretKey.When
command.integrationIdentifierpoints at a pre-existingNovuAgentintegration, this path unconditionally callsseedEmailSecretKey, which overwritescredentials.secretKeyviaencryptSecret(randomBytes(32)). If the integration was previously linked, unlinked, and is being relinked — or any external system retained the old signing secret — verification will break. Consider only seeding the secret when it is absent (e.g.$setOnInsert-equivalent: read first and skip if already set).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts` around lines 85 - 97, The current flow always calls seedEmailSecretKey(...) for NovuAgent integrations which unconditionally rotates credentials.secretKey via encryptSecret(randomBytes(32)); change this so the secret is only created when absent — either read integration.credentials.secretKey and skip calling seedEmailSecretKey if present, or modify seedEmailSecretKey to be idempotent (set-on-insert) and no-op when credentials.secretKey already exists; locate the check in add-agent-integration.usecase (the block that inspects integration.providerId === EmailProviderIdEnum.NovuAgent) and update the logic to guard the secret seeding accordingly.apps/api/src/app/agents/services/chat-sdk.service.ts (1)
296-311:⚠️ Potential issue | 🟡 Minor
senderNamechange won't invalidate the adapter cache.
senderNameis baked into the constructed email adapter (it setsNovuEmailAdapterImpl.userNameand is used bypostMessageto build theFrom:header), but it's missing fromadapterFingerprint. Per the comment on lines 292-295, any field passed intobuildAdaptersthat's consumed at construction time must be part of the fingerprint, otherwise users will see stale sender names until the LRU TTL expires.🔧 Proposed fix
return JSON.stringify({ platform, + senderName: c.senderName ?? null, signingSecret: c.signingSecret ?? null, clientId: c.clientId ?? null, secretKey: c.secretKey ?? null,🤖 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 296 - 311, adapterFingerprint currently omits senderName so changes to senderName won't invalidate cached adapters; update the adapterFingerprint function to include the senderName value from the ResolvedAgentConfig (the same field used when constructing NovuEmailAdapterImpl.userName via buildAdapters) so the JSON fingerprint reflects senderName changes and forces cache invalidation when it changes.packages/chat-adapter-email/src/adapter.ts (1)
129-143:⚠️ Potential issue | 🟡 MinorOutbound reply will hard-fail if no inbound ever set
agentAddress.
postMessagethrows whengetAgentAddressreturns undefined. That's fine for reply-after-inbound flows, but any code path that posts without a prior inbound message in the thread (e.g. agent-initiated DMs viaopenDM, replaying state after a Redis TTL expiry on a long-lived thread, or state key eviction) will surface this as a 500-class error to the caller. Consider either (a) persisting the agent address alongside the thread-id encoding, or (b) accepting an override viaconfigas a final fallback, so proactive sends and post-TTL replies remain possible.🤖 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 129 - 143, postMessage currently throws when threadResolver.getAgentAddress(threadId) returns undefined; update postMessage to fall back to a configurable default agent/from address or persist agent info with the thread id so proactive sends don't hard-fail. Specifically, in postMessage use threadResolver.getAgentAddress(threadId) || this.config.fallbackAgentAddress (or this.config.senderEmail/senderName combined) to build fromHeader, and add validation/error message if no fallback is configured; alternatively implement storing agentAddress alongside thread id in the threadResolver (update encode/decode and getAgentAddress) so getAgentAddress never returns undefined for threads created by openDM. Ensure references: postMessage, threadResolver.getAgentAddress, threadResolver.decodeThreadId, fromHeader, this.config.fallbackAgentAddress.
🧹 Nitpick comments (5)
apps/dashboard/src/components/agents/outbound-provider-select.tsx (1)
156-159: Info icon lacks an accessible name and tooltip.The
ⓘglyph on Line 158 is rendered as plain decorative text withoutaria-label,title, or a tooltip, so screen reader users (and sighted users) won't get any help text. Consider wrapping it in a tooltip primitive or addingtitle/aria-labeldescribing the "Send emails via" semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/outbound-provider-select.tsx` around lines 156 - 159, The info glyph next to the "Send emails via" label in the OutboundProviderSelect component is decorative but lacks an accessible name or tooltip; update the span that renders "ⓘ" (the info icon) to provide accessible help by either wrapping it in the app's Tooltip primitive or adding a descriptive title and aria-label (e.g., title="More info: Send emails via" and aria-label="More information about sending emails via providers") so screen readers and keyboard users can access the explanatory text. Ensure the change targets the span rendering the info glyph in OutboundProviderSelect and preserves visual styling and keyboard focus behavior.apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts (2)
142-170: Minor:_id: { $in: linkedIntegrationIds } as unknown as stringis a type-safety escape hatch.If
IntegrationRepository.find's filter type doesn't accept Mongo operators on_id, the right fix is in the repository's filter type, not a double-cast at every call site. Consider opening a small follow-up to widen the repository filter type to accept the native mongo operator shape, then removing these casts here and at line 231.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts` around lines 142 - 170, The code uses a double-cast `_id: { $in: linkedIntegrationIds } as unknown as string` to bypass the repository filter type; instead widen the IntegrationRepository (and similar repository filter) type to accept Mongo operator shapes (e.g. allow `_id: { $in: string[] }` or a generic Operator<T> union) so callers can pass `{ _id: { $in: linkedIntegrationIds } }` safely, then remove the casts from this use in agentIntegrationRepository.find / integrationRepository.findOne (and the other occurrence at line 231) and keep the rest of the logic (linkedIntegrationIds, EmailProviderIdEnum.NovuAgent, toAgentIntegrationResponse) unchanged.
119-132: Drop theas anyonintegrationRepository.create.Casting the whole payload to
anyhides real type mismatches (e.g. ifIntegrationEntity/create-DTO adds a required field, we'd only find out at runtime). Prefer narrowing to the repository's expected create type or spelling out the partial type that's actually expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts` around lines 119 - 132, Remove the blanket cast "as any" on the payload passed to integrationRepository.create; instead construct the payload using the repository's expected create type (e.g., CreateIntegrationDTO or Partial<IntegrationEntity>) and import that type, so the object with fields providerId (EmailProviderIdEnum.NovuAgent), channel (ChannelTypeEnum.EMAIL), credentials (encryptSecret(...)), configurations, name, identifier, active, _environmentId and _organizationId matches the typed shape expected by integrationRepository.create; update the call signature to integrationRepository.create(payload, { session }) where payload is explicitly typed rather than cast to any.apps/dashboard/src/components/agents/email-setup-guide.tsx (1)
56-69: Test email only exercises the first configured inbound address.
configuredAddresses[0]is always used as the target, so with multi-domain/multi-address setups the UI can never verify the pipeline for addresses 2..N — which undercuts the "full inbound pipeline" framing on Line 157. Consider letting the user pick which address to probe (dropdown next to the button, or iterating over each address), or at minimum surface the chosen target in the button label/toast so users know what was actually tested.🤖 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 56 - 69, The testEmailMutation currently always targets configuredAddresses[0], so update the UI and mutation to let the user choose which configured inbound address to probe (or provide an "all addresses" iteration option): add a selectable value (e.g., selectedAddressIndex or selectedAddress) in the component state/UI (dropdown or radio) and use that instead of configuredAddresses[0] inside testEmailMutation when building targetAddress for sendAgentTestEmail; additionally include the actual targetAddress in the success and error toasts via showSuccessToast/showErrorToast so users know which address was tested; ensure any iteration over addresses calls sendAgentTestEmail for each address and aggregates results appropriately in the toasts.apps/api/src/app/domains/usecases/update-domain/update-domain.usecase.ts (1)
81-94: Batch agent validation into a single$inquery.The loop issues one round-trip per unique destination. Since
agentDestinationsis already deduped, a singlefind({ _id: { $in: agentDestinations }, ... }, ['_id'])call and a set-difference check is both more efficient and clearer. This also gives you the full list of missing IDs in the error message, which is more actionable.♻️ Proposed refactor
- if (agentDestinations.length === 0) return; - - for (const agentId of agentDestinations) { - const agent = await this.agentRepository.findOne( - { - _id: agentId, - _environmentId: command.environmentId, - _organizationId: command.organizationId, - }, - ['_id'] - ); - - if (!agent) { - throw new NotFoundException(`Agent "${agentId}" referenced in route destination does not exist.`); - } - } + if (agentDestinations.length === 0) return; + + const existing = await this.agentRepository.find( + { + _id: { $in: agentDestinations }, + _environmentId: command.environmentId, + _organizationId: command.organizationId, + }, + ['_id'] + ); + + const existingIds = new Set(existing.map((a) => a._id)); + const missing = agentDestinations.filter((id) => !existingIds.has(id)); + + if (missing.length > 0) { + throw new NotFoundException( + `Agent(s) "${missing.join(', ')}" referenced in route destination do not exist.` + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/domains/usecases/update-domain/update-domain.usecase.ts` around lines 81 - 94, The current loop in update-domain.usecase.ts performs one DB round-trip per agentId by calling this.agentRepository.findOne for each id; replace this with a single batched query using this.agentRepository.find (or findMany) with a filter _id: { $in: agentDestinations } plus the same _environmentId and _organizationId to fetch all existing ids at once, then compute the set difference between agentDestinations and the returned ids and, if any missing, throw a NotFoundException listing the missing agent IDs (retain agentDestinations, agentRepository, and NotFoundException identifiers to locate and update the code).
🤖 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/dtos/add-agent-integration-request.dto.ts`:
- Around line 5-21: The DTO currently allows both integrationIdentifier and
providerId to be set because each field's ValidateIf only skips when the other
is present; add a class-level validation that rejects requests where both
integrationIdentifier and providerId are provided. Implement a custom
class-validator (e.g., BothFieldsExclusive or a validator function) and apply it
to the DTO class (the AddAgentIntegrationRequestDto) so it returns a validation
error when integrationIdentifier && providerId; ensure the validator message
clearly states that only one of integrationIdentifier or providerId may be
supplied.
In
`@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts`:
- Around line 104-136: The method findOrCreateNovuEmailLink has a race where
findExistingNovuEmailLink runs outside the transaction so two concurrent callers
can each create a NovuAgent integration; fix by re-checking for an existing link
inside the transaction before creating: inside the
agentIntegrationRepository.withTransaction callback call
findExistingNovuEmailLink (or a new transactional lookup) and if it returns a
link return it; additionally call enforceSingletonEmail (or the same
singleton-enforcing logic) inside the transaction prior to the
integrationRepository.create so only one writer proceeds, or alternatively
implement and rely on a unique constraint on the agent-integration mapping and
handle duplicate-key errors to return the existing link instead of creating a
second one.
In `@apps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.ts`:
- Around line 80-89: The IntegrationRepository.find override currently types its
options without a session and does not forward a session to super.find, causing
the passed { session } to be dropped; update the find method in
IntegrationRepository to extend its options type to include session (matching
the create/delete signatures) and pass that session through when calling
super.find so the session is preserved at runtime.
In `@apps/dashboard/src/components/agents/inbound-address-config.tsx`:
- Around line 43-49: handleAdd currently accepts whitespace-only or invalid
localPart values; trim localPart, validate it against a minimal pattern (e.g.
^(\*|[A-Za-z0-9._%+-]+)$) before calling onAddAddress, and only proceed if the
trimmed value passes the regex; refer to handleAdd, localPart, domainName,
domains, and onAddAddress when making the change. Also compute a derived boolean
(e.g. isLocalPartValid) based on the trimmed value and use it to disable the Add
button in the UI so users cannot submit invalid input, and clear the input with
the trimmed value after a successful add.
In `@apps/dashboard/src/components/agents/outbound-provider-select.tsx`:
- Line 142: The invalidateQueries call uses currentEnvironment._id directly
which can throw if currentEnvironment is undefined; change the query key to use
optional chaining like currentEnvironment?._id to match useFetchIntegrations and
other mutations so the invalidateQueries call uses the same key shape
(reference: queryClient.invalidateQueries and QueryKeys.fetchIntegrations with
currentEnvironment._id).
In `@apps/dashboard/src/components/agents/provider-dropdown.tsx`:
- Around line 297-300: When handling EmailProviderIdEnum.NovuAgent in the branch
that calls addAgentIntegrationMutation.mutateAsync, emit the same "Integration
linked" success toast as the other branches (use link.integration?.name or
fallback to "Novu Agent email") after the mutateAsync resolves and before
calling onSelect(item.providerId, link.integration) and setOpen(false); update
the block around addAgentIntegrationMutation.mutateAsync to call the existing
toast helper used elsewhere so the NovuAgent flow matches the UX of the other
branches.
In `@apps/dashboard/src/components/agents/use-email-setup-credentials.ts`:
- Around line 39-41: The current useEffect merges serverCredentials over
credentialsRef.current which can clobber in-flight local edits; change the merge
so serverCredentials is the base and local pending edits overlay it (e.g., set
credentialsRef.current = { ...serverCredentials, ...credentialsRef.current })
and additionally consult saveQueueRef (or a pendingKeys set) to preserve any
keys currently queued for save so those fields are not overwritten when
emailIntegration refetches; update the effect that runs on emailIntegration to
perform this overlay and skip/retain keys present in saveQueueRef.
- Around line 118-169: addAddress and removeAddress are directly calling
updateDomain(...).then().catch(), bypassing TanStack Query's useMutation
lifecycle and manual refetch/toast handling; replace this with a single
useMutation (or two mutations) that wraps updateDomain and use its onSuccess to
invalidate/refetch the fetchDomains query (or call domainsQuery.refetch()) and
onError to show the error toast; update the addAddress and removeAddress
callbacks to call the mutation's mutate/mutateAsync (instead of chaining
promises) and reference updateDomain, addAddress, removeAddress, and
domainsQuery when wiring the mutation and invalidation.
In `@libs/dal/src/repositories/domain/domain.repository.ts`:
- Around line 62-73: The removeRoutesByDestination method currently calls
this.MongooseModel.updateMany directly (bypassing BaseRepositoryV2 enforcement
and requiring an as any cast); change it to call this.update() instead, passing
the filter with _environmentId: environmentId and _organizationId:
organizationId and the $pull update ({ $pull: { routes: { destination } } })
using the update options shape that accepts session (options.session), so you
preserve EnforceEnvOrOrgIds typing and use UpdateQuery<DomainDBModel> rather
than touching MongooseModel directly.
In `@packages/chat-adapter-email/src/adapter.ts`:
- Around line 93-97: The code assumes the agent address as
payload.to[0]?.address (used in agentAddress and passed to
threadResolver.trackAgentAddress) which fails for multi-recipient/CC/BCC cases
and causes postMessage to throw if not captured; update NovuEmailAdapterConfig
to accept an agentAddresses: string[] (or require the webhook to include a
routedTo: string) and change the handler to resolve the agent address by either
validating payload.to[0]?.address is in config.agentAddresses (if not, search
payload.to for any address in agentAddresses) or to read payload.routedTo and
validate it against config.agentAddresses, then only call
threadResolver.trackAgentAddress(threadId, resolvedAgentAddress) when
resolvedAgentAddress is present and validated; keep existing trackSubject usage
(threadResolver.trackSubject) unchanged.
---
Outside diff comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 296-311: adapterFingerprint currently omits senderName so changes
to senderName won't invalidate cached adapters; update the adapterFingerprint
function to include the senderName value from the ResolvedAgentConfig (the same
field used when constructing NovuEmailAdapterImpl.userName via buildAdapters) so
the JSON fingerprint reflects senderName changes and forces cache invalidation
when it changes.
In
`@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts`:
- Around line 85-97: The current flow always calls seedEmailSecretKey(...) for
NovuAgent integrations which unconditionally rotates credentials.secretKey via
encryptSecret(randomBytes(32)); change this so the secret is only created when
absent — either read integration.credentials.secretKey and skip calling
seedEmailSecretKey if present, or modify seedEmailSecretKey to be idempotent
(set-on-insert) and no-op when credentials.secretKey already exists; locate the
check in add-agent-integration.usecase (the block that inspects
integration.providerId === EmailProviderIdEnum.NovuAgent) and update the logic
to guard the secret seeding accordingly.
In
`@apps/api/src/app/agents/usecases/send-agent-test-email/send-agent-test-email.usecase.ts`:
- Around line 66-86: The code uses senderIntegration.credentials?.from when
building mailOptions which can be undefined; before calling
mailFactory.getHandler or constructing mailOptions (in
send-agent-test-email.usecase.ts, around findSenderIntegration/senderIntegration
usage), explicitly validate that senderIntegration.credentials?.from (and
optionally credentials?.senderName) is a non-empty string and, if not, throw a
BadRequestException with a clear message (e.g., "Outbound integration missing
'from' address"); only proceed to call mailFactory.getHandler and build
mailOptions after these assertions pass.
- Around line 60-88: The code accepts command.targetAddress verbatim and sends
test emails via the sender integration (see send-agent-test-email.usecase and
the call to findSenderIntegration / handler.send), allowing abuse of Novu-hosted
demo outbound infra; add enforcement to validate and restrict destinations: 1)
add a rate limit/throttler guard around this use case to limit calls per
user/org, 2) when senderIntegration.providerId === EmailProviderIdEnum.Novu only
allow command.targetAddress to be the authenticated user's email (or a
pre-verified address) and reject or return a 403 for other targets, and 3)
permit arbitrary targetAddress only when the org has a non-Novu customer-owned
outbound provider; apply these checks before creating MailFactory/getHandler and
before calling handler.send.
In `@packages/chat-adapter-email/src/adapter.ts`:
- Around line 129-143: postMessage currently throws when
threadResolver.getAgentAddress(threadId) returns undefined; update postMessage
to fall back to a configurable default agent/from address or persist agent info
with the thread id so proactive sends don't hard-fail. Specifically, in
postMessage use threadResolver.getAgentAddress(threadId) ||
this.config.fallbackAgentAddress (or this.config.senderEmail/senderName
combined) to build fromHeader, and add validation/error message if no fallback
is configured; alternatively implement storing agentAddress alongside thread id
in the threadResolver (update encode/decode and getAgentAddress) so
getAgentAddress never returns undefined for threads created by openDM. Ensure
references: postMessage, threadResolver.getAgentAddress,
threadResolver.decodeThreadId, fromHeader, this.config.fallbackAgentAddress.
In `@packages/chat-adapter-email/src/types.ts`:
- Around line 5-20: The change removed the previously exported
NovuEmailAdapterConfig.fromAddress/fromName and added senderName?, and also made
SendEmailParams.from required—this is a breaking API change; restore backward
compatibility by reintroducing fromAddress (required) and fromName (optional)
into NovuEmailAdapterConfig and mark them with `@deprecated` JSDoc pointing to
senderName (keep senderName as the new preferred field), and make
SendEmailParams.from optional (or accept either from or fromAddress) so existing
callers without from do not break; update JSDoc on SendEmailParams and
NovuEmailAdapterConfig types (and package versioning/documentation) to reflect
deprecation and migration path.
---
Nitpick comments:
In
`@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts`:
- Around line 142-170: The code uses a double-cast `_id: { $in:
linkedIntegrationIds } as unknown as string` to bypass the repository filter
type; instead widen the IntegrationRepository (and similar repository filter)
type to accept Mongo operator shapes (e.g. allow `_id: { $in: string[] }` or a
generic Operator<T> union) so callers can pass `{ _id: { $in:
linkedIntegrationIds } }` safely, then remove the casts from this use in
agentIntegrationRepository.find / integrationRepository.findOne (and the other
occurrence at line 231) and keep the rest of the logic (linkedIntegrationIds,
EmailProviderIdEnum.NovuAgent, toAgentIntegrationResponse) unchanged.
- Around line 119-132: Remove the blanket cast "as any" on the payload passed to
integrationRepository.create; instead construct the payload using the
repository's expected create type (e.g., CreateIntegrationDTO or
Partial<IntegrationEntity>) and import that type, so the object with fields
providerId (EmailProviderIdEnum.NovuAgent), channel (ChannelTypeEnum.EMAIL),
credentials (encryptSecret(...)), configurations, name, identifier, active,
_environmentId and _organizationId matches the typed shape expected by
integrationRepository.create; update the call signature to
integrationRepository.create(payload, { session }) where payload is explicitly
typed rather than cast to any.
In `@apps/api/src/app/domains/usecases/update-domain/update-domain.usecase.ts`:
- Around line 81-94: The current loop in update-domain.usecase.ts performs one
DB round-trip per agentId by calling this.agentRepository.findOne for each id;
replace this with a single batched query using this.agentRepository.find (or
findMany) with a filter _id: { $in: agentDestinations } plus the same
_environmentId and _organizationId to fetch all existing ids at once, then
compute the set difference between agentDestinations and the returned ids and,
if any missing, throw a NotFoundException listing the missing agent IDs (retain
agentDestinations, agentRepository, and NotFoundException identifiers to locate
and update the code).
In `@apps/dashboard/src/components/agents/email-setup-guide.tsx`:
- Around line 56-69: The testEmailMutation currently always targets
configuredAddresses[0], so update the UI and mutation to let the user choose
which configured inbound address to probe (or provide an "all addresses"
iteration option): add a selectable value (e.g., selectedAddressIndex or
selectedAddress) in the component state/UI (dropdown or radio) and use that
instead of configuredAddresses[0] inside testEmailMutation when building
targetAddress for sendAgentTestEmail; additionally include the actual
targetAddress in the success and error toasts via
showSuccessToast/showErrorToast so users know which address was tested; ensure
any iteration over addresses calls sendAgentTestEmail for each address and
aggregates results appropriately in the toasts.
In `@apps/dashboard/src/components/agents/outbound-provider-select.tsx`:
- Around line 156-159: The info glyph next to the "Send emails via" label in the
OutboundProviderSelect component is decorative but lacks an accessible name or
tooltip; update the span that renders "ⓘ" (the info icon) to provide
accessible help by either wrapping it in the app's Tooltip primitive or adding a
descriptive title and aria-label (e.g., title="More info: Send emails via" and
aria-label="More information about sending emails via providers") so screen
readers and keyboard users can access the explanatory text. Ensure the change
targets the span rendering the info glyph in OutboundProviderSelect and
preserves visual styling and keyboard focus behavior.
🪄 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: 704a2327-9a3d-4c49-859c-7a0e7961c8da
📒 Files selected for processing (28)
apps/api/src/app/agents/agents.controller.tsapps/api/src/app/agents/dtos/add-agent-integration-request.dto.tsapps/api/src/app/agents/dtos/send-agent-test-email-request.dto.tsapps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.command.tsapps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.tsapps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.tsapps/api/src/app/agents/usecases/remove-agent-integration/remove-agent-integration.usecase.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/api/src/app/domains/usecases/update-domain/update-domain.usecase.tsapps/dashboard/src/api/agents.tsapps/dashboard/src/components/agents/email-setup-guide.tsxapps/dashboard/src/components/agents/inbound-address-config.tsxapps/dashboard/src/components/agents/outbound-provider-select.tsxapps/dashboard/src/components/agents/provider-dropdown.tsxapps/dashboard/src/components/agents/setup-guide-primitives.tsxapps/dashboard/src/components/agents/use-email-setup-credentials.tslibs/application-generic/src/dtos/credentials.dto.tslibs/dal/src/repositories/base-repository-v2.tslibs/dal/src/repositories/domain/domain.repository.tslibs/dal/src/repositories/integration/integration.repository.tslibs/dal/src/repositories/integration/integration.schema.tspackages/chat-adapter-email/src/adapter.tspackages/chat-adapter-email/src/thread-resolver.tspackages/chat-adapter-email/src/types.tspackages/shared/src/entities/integration/credential.interface.tspackages/shared/src/types/providers.ts
💤 Files with no reviewable changes (4)
- packages/shared/src/types/providers.ts
- packages/shared/src/entities/integration/credential.interface.ts
- libs/dal/src/repositories/integration/integration.schema.ts
- libs/application-generic/src/dtos/credentials.dto.ts
Comment on lines
+43
to
+49
| function handleAdd() { | ||
| if (!localPart || !domainName) return; | ||
| const domain = domains.find((d) => d.name === domainName); | ||
| if (!domain) return; | ||
| onAddAddress(localPart, domain); | ||
| setLocalPart(''); | ||
| } |
Contributor
There was a problem hiding this comment.
Local part isn't validated or trimmed.
handleAdd accepts any value for localPart other than the empty string — including whitespace-only values, leading/trailing spaces, or characters that aren't valid in an email local part (e.g. foo bar, foo@bar). The server-side UpdateDomain validation may reject it, but that produces a late error round-trip.
Consider trimming and validating against a minimal pattern (e.g. ^(\*|[A-Za-z0-9._%+-]+)$) before calling onAddAddress, and disabling the Add button when invalid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/src/components/agents/inbound-address-config.tsx` around lines
43 - 49, handleAdd currently accepts whitespace-only or invalid localPart
values; trim localPart, validate it against a minimal pattern (e.g.
^(\*|[A-Za-z0-9._%+-]+)$) before calling onAddAddress, and only proceed if the
trimmed value passes the regex; refer to handleAdd, localPart, domainName,
domains, and onAddAddress when making the change. Also compute a derived boolean
(e.g. isLocalPartValid) based on the trimmed value and use it to disable the Add
button in the UI so users cannot submit invalid input, and clear the input with
the trimmed value after a successful add.
Comment on lines
+93
to
+97
| const agentAddress = payload.to[0]?.address; | ||
| await Promise.all([ | ||
| this.threadResolver.trackSubject(threadId, payload.subject), | ||
| agentAddress ? this.threadResolver.trackAgentAddress(threadId, agentAddress) : Promise.resolve(), | ||
| ]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the inbound webhook payload shape to see what routing metadata is available.
fd -t f 'types.ts' packages/chat-adapter-email/src -x cat {}
echo '---'
fd -t f 'webhook-handler.ts' packages/chat-adapter-email/src -x cat {}Repository: novuhq/novu
Length of output: 3196
🏁 Script executed:
fd -type f "EmailWebhookPayload" --search-path . -x grep -l "type EmailWebhookPayload\|interface EmailWebhookPayload" {} \;Repository: novuhq/novu
Length of output: 226
🏁 Script executed:
rg "type EmailWebhookPayload|interface EmailWebhookPayload" --type ts --type tsx -A 15Repository: novuhq/novu
Length of output: 83
🏁 Script executed:
fd -type f -name "*.ts" -path "*shared*" | head -20Repository: novuhq/novu
Length of output: 226
🏁 Script executed:
rg "type EmailWebhookPayload|interface EmailWebhookPayload" -t ts -A 20Repository: novuhq/novu
Length of output: 778
🏁 Script executed:
fd "shared" --type d | grep -v node_modules | head -5Repository: novuhq/novu
Length of output: 253
🏁 Script executed:
rg "payload.to\[0\]" -t ts -B 5 -A 5Repository: novuhq/novu
Length of output: 1710
🏁 Script executed:
rg "parseMessage|parseAndVerify" -t ts -B 3 -A 10 packages/chat-adapter-email/Repository: novuhq/novu
Length of output: 4571
🏁 Script executed:
rg "inbound.*route\|route.*inbound" -t ts -iRepository: novuhq/novu
Length of output: 37
🏁 Script executed:
rg "new NovuEmailAdapter\|initialize\(" -t ts -B 3 -A 8 packages/chat-adapter-email/Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
cat -n packages/chat-adapter-email/src/adapter.ts | head -50Repository: novuhq/novu
Length of output: 2141
🏁 Script executed:
cat -n packages/chat-adapter-email/src/adapter.ts | sed -n '100,180p'Repository: novuhq/novu
Length of output: 3294
🏁 Script executed:
rg "agentAddress\|trackAgentAddress" -t ts -B 2 -A 2Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
rg "class ThreadResolver" -t ts -A 30Repository: novuhq/novu
Length of output: 2479
🏁 Script executed:
cat -n packages/chat-adapter-email/src/thread-resolver.ts | grep -A 10 "trackAgentAddress"Repository: novuhq/novu
Length of output: 513
🏁 Script executed:
rg "getAgentAddress" -t ts -B 2 -A 5 packages/chat-adapter-email/Repository: novuhq/novu
Length of output: 1332
🏁 Script executed:
rg "webhook\|inbound.*route" -t ts -i packages/chat-adapter-email/ | head -20Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
cat -n packages/chat-adapter-email/src/adapter.ts | sed -n '80,103p'Repository: novuhq/novu
Length of output: 1060
🏁 Script executed:
rg "config\|senderName\|signingSecret" -t ts packages/chat-adapter-email/src/types.tsRepository: novuhq/novu
Length of output: 37
🏁 Script executed:
fd -type f "README\|readme\|SETUP\|setup" packages/chat-adapter-email/ -x cat {}Repository: novuhq/novu
Length of output: 226
🏁 Script executed:
rg "signingSecret\|webhook.*endpoint\|inbound.*address" -t ts -B 2 -A 2 | head -40Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
fd "README" packages/chat-adapter-email/Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
fd "adapter.test\|adapter.spec" packages/chat-adapter-email/ -x cat {}Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
rg "NovuEmailAdapterConfig\|NovuEmailAdapterImpl" -t ts -B 3 -A 10 | head -50Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
rg "NovuEmailAdapter\|chat.*initialize\|adapter.*config" -t ts --type-list | head -5Repository: novuhq/novu
Length of output: 445
🏁 Script executed:
git log --oneline -20 packages/chat-adapter-email/Repository: novuhq/novu
Length of output: 283
🏁 Script executed:
cat packages/chat-adapter-email/package.jsonRepository: novuhq/novu
Length of output: 985
🏁 Script executed:
rg "payload\.to\s*\?" -t ts -B 3 -A 3Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
rg "length.*to\|to.*length\|single.*address\|one.*address" -t ts -i packages/chat-adapter-email/Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
git show 6cfa5a0e:packages/chat-adapter-email/src/adapter.ts 2>/dev/null | head -150 | tail -70Repository: novuhq/novu
Length of output: 2600
🏁 Script executed:
rg "NovuEmailAdapterImpl\|new.*Email\|export.*Email" -t ts packages/ apps/ | grep -i "adapter\|export" | head -20Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
rg "config\..*address\|agentAddress.*config" -t ts packages/chat-adapter-email/Repository: novuhq/novu
Length of output: 37
🏁 Script executed:
cat -n packages/chat-adapter-email/src/adapter.ts | sed -n '1,50p'Repository: novuhq/novu
Length of output: 2141
🏁 Script executed:
rg "export.*NovuEmailAdapter\|export.*class" -t ts packages/chat-adapter-email/src/Repository: novuhq/novu
Length of output: 37
Agent address inferred from payload.to[0] without validation of single-recipient assumption.
The adapter uses payload.to[0]?.address to identify the inbound agent address, but this assumes the first recipient is always correct. This breaks when the sender CCs multiple addresses or the agent is in CC/BCC. More critically, there is no configuration field to restrict which agent address(es) this adapter instance should handle, and postMessage will throw if the address wasn't captured in the initial inbound webhook.
To fix: either (1) add an agentAddresses field to NovuEmailAdapterConfig and validate that payload.to[0] is in that list, or (2) require the webhook payload to include explicit routing metadata (e.g., routedTo: string indicating which address the webhook was routed to).
🤖 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 93 - 97, The code
assumes the agent address as payload.to[0]?.address (used in agentAddress and
passed to threadResolver.trackAgentAddress) which fails for
multi-recipient/CC/BCC cases and causes postMessage to throw if not captured;
update NovuEmailAdapterConfig to accept an agentAddresses: string[] (or require
the webhook to include a routedTo: string) and change the handler to resolve the
agent address by either validating payload.to[0]?.address is in
config.agentAddresses (if not, search payload.to for any address in
agentAddresses) or to read payload.routedTo and validate it against
config.agentAddresses, then only call threadResolver.trackAgentAddress(threadId,
resolvedAgentAddress) when resolvedAgentAddress is present and validated; keep
existing trackSubject usage (threadResolver.trackSubject) unchanged.
- Remove unused DomainStatusEnum import - Add session support to IntegrationRepository.find override - Fix race condition: re-check existing link inside transaction - Use optional chaining for currentEnvironment?._id in query key Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts (1)
104-141:⚠️ Potential issue | 🟠 MajorRace condition still open — recheck reads outside the transaction, and there's no unique index.
The in-transaction recheck at line 116 is the right idea, but
findExistingNovuEmailLinkdoesn't accept or pass aClientSession, so both theagentIntegrationRepository.findandintegrationRepository.findOneinside it query outside the transaction snapshot. Even if the session were threaded through, MongoDB transactions use snapshot isolation without a unique constraint: two concurrent callers can each observe "no link" and both commit an insert, leaving the agent with duplicate NovuAgent integrations.To actually close the race, either:
- add a unique partial index such as
{ _agentId: 1, providerId: 1 } where providerId === "novu-agent"(via the linked Integration) or a dedicated sentinel doc onagentIntegrations, and catch the duplicate-key error to return the existing link; or- thread
sessionthroughfindExistingNovuEmailLinkand pair it with the unique index above — session alone is necessary but not sufficient.🛠️ Minimum step: propagate session into the recheck
- private async findExistingNovuEmailLink( - agentId: string, - command: AddAgentIntegrationCommand - ): Promise<AgentIntegrationResponseDto | null> { + private async findExistingNovuEmailLink( + agentId: string, + command: AddAgentIntegrationCommand, + session: ClientSession | null = null + ): Promise<AgentIntegrationResponseDto | null> { const links = await this.agentIntegrationRepository.find( { _agentId: agentId, _environmentId: command.environmentId, _organizationId: command.organizationId, }, - '*' + '*', + { session } ); // ... const emailIntegration = await this.integrationRepository.findOne( { /* ... */ }, - '_id identifier name providerId channel active' + '_id identifier name providerId channel active', + { session } );And at the call site:
return this.agentIntegrationRepository.withTransaction(async (session) => { - const recheck = await this.findExistingNovuEmailLink(agentId, command); + const recheck = await this.findExistingNovuEmailLink(agentId, command, session);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts` around lines 104 - 141, findOrCreateNovuEmailLink has a race because findExistingNovuEmailLink is called outside and inside the transaction but does not accept a ClientSession, and there is no uniqueness guard; fix by (1) updating findExistingNovuEmailLink to accept an optional session param and thread that session through its internal repository calls, then pass the transaction session from agentIntegrationRepository.withTransaction into the recheck call inside findOrCreateNovuEmailLink, and (2) add a unique partial DB index on agent integrations (e.g., on {_agentId: 1, providerId: 1} filtered to providerId === EmailProviderIdEnum.NovuAgent) and update integration/agentIntegration create paths to catch duplicate-key errors (E11000) and return the existing link when a duplicate-key occurs.
🧹 Nitpick comments (1)
apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts (1)
124-137: Drop theas anycast and let the repository type catch shape errors.Casting to
anydefeats the enforcement thatIntegrationQuery/IntegrationDBModelprovides and will silently accept shape drift (e.g., a schema rename). If the cast is there becausecredentials/configurationsare structurally wider than the entity type, prefer defining a narrow local type or usingPartial<IntegrationDBModel> & EnforceEnvOrOrgIdsso TS validates the important fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts` around lines 124 - 137, The create call is currently cast to any which bypasses type checking; remove the "as any" on the integration payload passed to integrationRepository.create and instead ensure the object conforms to the repository/entity type (e.g., IntegrationDBModel or IntegrationQuery). If credentials/configurations are wider than the model, define a narrow local type (or use Partial<IntegrationDBModel> & EnforceEnvOrOrgIds) that includes providerId, channel, credentials, configurations, name, identifier, active, _environmentId, and _organizationId, and use that type for the payload so TypeScript validates shape errors before calling integrationRepository.create.
🤖 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/add-agent-integration/add-agent-integration.usecase.ts`:
- Around line 61-66: The non-NovuAgent branch is not guarding against a missing
integration identifier, causing linkExistingIntegration to run with an undefined
filter; update the decision logic in the method that currently branches on
EmailProviderIdEnum.NovuAgent so that when providerId !==
EmailProviderIdEnum.NovuAgent you validate command.integrationIdentifier is
present (and throw a clear validation error if not) before calling
linkExistingIntegration(agent._id, command); keep the NovuAgent path using
findOrCreateNovuEmailLink as-is and ensure the error message refers to the
missing integrationIdentifier rather than "undefined".
In `@apps/dashboard/src/components/agents/outbound-provider-select.tsx`:
- Around line 156-159: The info glyph in OutboundProviderSelect is a bare <span>
and lacks accessibility semantics; either mark that span as decorative by adding
aria-hidden="true" (if the adjacent "Send emails via" label is sufficient) or
replace/wrap the glyph with the existing Tooltip primitive and provide a
descriptive aria-label/tooltip text so screen readers and keyboard users get the
help text; locate the span rendering the "ⓘ" in the OutboundProviderSelect
component and apply one of these fixes.
---
Duplicate comments:
In
`@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts`:
- Around line 104-141: findOrCreateNovuEmailLink has a race because
findExistingNovuEmailLink is called outside and inside the transaction but does
not accept a ClientSession, and there is no uniqueness guard; fix by (1)
updating findExistingNovuEmailLink to accept an optional session param and
thread that session through its internal repository calls, then pass the
transaction session from agentIntegrationRepository.withTransaction into the
recheck call inside findOrCreateNovuEmailLink, and (2) add a unique partial DB
index on agent integrations (e.g., on {_agentId: 1, providerId: 1} filtered to
providerId === EmailProviderIdEnum.NovuAgent) and update
integration/agentIntegration create paths to catch duplicate-key errors (E11000)
and return the existing link when a duplicate-key occurs.
---
Nitpick comments:
In
`@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts`:
- Around line 124-137: The create call is currently cast to any which bypasses
type checking; remove the "as any" on the integration payload passed to
integrationRepository.create and instead ensure the object conforms to the
repository/entity type (e.g., IntegrationDBModel or IntegrationQuery). If
credentials/configurations are wider than the model, define a narrow local type
(or use Partial<IntegrationDBModel> & EnforceEnvOrOrgIds) that includes
providerId, channel, credentials, configurations, name, identifier, active,
_environmentId, and _organizationId, and use that type for the payload so
TypeScript validates shape errors before calling integrationRepository.create.
🪄 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: 208c025c-0c51-4b3f-8708-52983a91d20f
📒 Files selected for processing (4)
apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.tsapps/dashboard/src/components/agents/outbound-provider-select.tsxapps/dashboard/src/components/agents/use-email-setup-credentials.tslibs/dal/src/repositories/integration/integration.repository.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/dashboard/src/components/agents/use-email-setup-credentials.ts
- Guard against both integrationIdentifier and providerId in same request - Validate and sanitize email local-part input (strip whitespace, reject invalid chars) - Add success toast when NovuAgent integration is linked - Fix TS2345 build error: cast AgentIntegrationEmbedded to IIntegration in onSelect Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/dashboard/src/components/agents/inbound-address-config.tsx (1)
43-53:⚠️ Potential issue | 🟡 MinorAdd button disabled state doesn't reflect
LOCAL_PART_REvalidity.
handleAddsilently returns when the local part failsLOCAL_PART_RE, but the Add button'sdisabled={!localPart || !domainName}(Line 164) only checks emptiness. BecauseonChangestrips only whitespace, a user can still type/paste invalid characters (e.g.foo@bar,foo..bar), see the button enabled, click it, and get no feedback. Mirror the handler's validation in the disabled state so invalid input is visibly rejected.🛠️ Proposed fix
+ const trimmedLocalPart = localPart.trim(); + const isLocalPartValid = trimmedLocalPart.length > 0 && LOCAL_PART_RE.test(trimmedLocalPart); + function handleAdd() { - const trimmed = localPart.trim(); - if (!trimmed || !domainName) return; - if (!LOCAL_PART_RE.test(trimmed)) return; + if (!isLocalPartValid || !domainName) return; const domain = domains.find((d) => d.name === domainName); if (!domain) return; - onAddAddress(trimmed, domain); + onAddAddress(trimmedLocalPart, domain); setLocalPart(''); }- disabled={!localPart || !domainName} + disabled={!isLocalPartValid || !domainName}Also applies to: 161-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/inbound-address-config.tsx` around lines 43 - 53, The Add button's disabled state must mirror the same validation used in handleAdd to prevent enabled-but-no-op clicks: update the button's disabled expression to require domainName, non-empty localPart.trim(), and LOCAL_PART_RE.test(localPart.trim()) (i.e. use LOCAL_PART_RE and localPart.trim() the same way handleAdd does); keep handleAdd unchanged except for ensuring it still calls onAddAddress(trimmed, domain) and setLocalPart('') on success. This ensures the UI visibly rejects invalid chars the handler currently rejects.apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts (1)
65-70:⚠️ Potential issue | 🟠 MajorNon-
NovuAgentproviderIdvalues still fall through tolinkExistingIntegrationwithundefinedidentifier.The XOR guard at lines 44–50 makes
integrationIdentifierandproviderIdmutually exclusive, but the branch at line 65 only catchesproviderId === NovuAgent. Any otherproviderIdvalue (e.g."sendgrid") passes the XOR check withintegrationIdentifierundefined, reacheslinkExistingIntegration, and then queriesintegrationRepository.findOne({ identifier: undefined, … })at line 83 — Mongoose strips the undefined field and you end up matching an unintended integration or surfacing the misleadingIntegration with identifier "undefined" was not found.error.Either restrict
providerIdtoNovuAgentexplicitly, or requireintegrationIdentifierwhenproviderId !== NovuAgent:🛠️ Proposed guard
if (command.providerId === EmailProviderIdEnum.NovuAgent) { return this.findOrCreateNovuEmailLink(agent._id, command); } + if (!command.integrationIdentifier) { + throw new BadRequestException( + 'integrationIdentifier is required when providerId is not NovuAgent.' + ); + } + return this.linkExistingIntegration(agent._id, command);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts` around lines 65 - 70, The code allows non-Novu providerId values to reach linkExistingIntegration with integrationIdentifier undefined, causing a bad query; update the guard in the AddAgentIntegration usecase so that if command.providerId !== EmailProviderIdEnum.NovuAgent you enforce command.integrationIdentifier is present (throw a clear validation error) before calling linkExistingIntegration, or alternatively restrict allowed providerId values to only NovuAgent; reference providerId and integrationIdentifier on the incoming command, and the methods findOrCreateNovuEmailLink and linkExistingIntegration (and the integrationRepository.findOne call) when making the validation change.
🧹 Nitpick comments (1)
apps/dashboard/src/components/agents/inbound-address-config.tsx (1)
43-43: Two small tidy-ups.
- Line 43:
LOCAL_PART_REis recreated on every render. Hoist it to module scope.- Lines 60/68:
fullalready equals*@${addr.domain}whenaddr.address === '*', so the ternary is redundant — just render{full}.♻️ Proposed fix
@@ module scope +const LOCAL_PART_RE = /^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+$/; + export function InboundAddressConfig({- const LOCAL_PART_RE = /^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+$/; - function handleAdd() {- <span className="text-text-sub text-label-xs flex-1 font-medium leading-4"> - {addr.address === '*' ? `*@${addr.domain}` : full} - </span> + <span className="text-text-sub text-label-xs flex-1 font-medium leading-4">{full}</span>Also applies to: 60-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/inbound-address-config.tsx` at line 43, Hoist the LOCAL_PART_RE regex out of the component render so it’s a module-scoped constant (move the line defining LOCAL_PART_RE to top-level) to avoid recreating it on every render; then simplify the JSX that builds the local/full address by removing the redundant ternary (the branch that checks addr.address === '*' is unnecessary because full already equals `*@${addr.domain}`) and just render {full} where that ternary is currently used (update references around the variables named full and addr.address accordingly).
🤖 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/add-agent-integration/add-agent-integration.usecase.ts`:
- Around line 94-98: The call to seedEmailSecretKey inside
linkExistingIntegration unconditionally rotates the NovuAgent secret (breaking
existing webhooks and leaving partial state on failures); modify
linkExistingIntegration so it does not blindly overwrite credentials.secretKey —
either (preferred) gate seeding to only run when the integration has no secret
(check credentials.secretKey existence or use a DB update/filter like
'credentials.secretKey': { $exists: false }), or move the seedEmailSecretKey
call to after createLink's conflict check so a failed link does not rotate the
secret, or skip seeding entirely for the re-link path since
findOrCreateNovuEmailLink already seeds at creation; update
linkExistingIntegration and/or seedEmailSecretKey accordingly to ensure the
update is conditional and atomic.
---
Duplicate comments:
In
`@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts`:
- Around line 65-70: The code allows non-Novu providerId values to reach
linkExistingIntegration with integrationIdentifier undefined, causing a bad
query; update the guard in the AddAgentIntegration usecase so that if
command.providerId !== EmailProviderIdEnum.NovuAgent you enforce
command.integrationIdentifier is present (throw a clear validation error) before
calling linkExistingIntegration, or alternatively restrict allowed providerId
values to only NovuAgent; reference providerId and integrationIdentifier on the
incoming command, and the methods findOrCreateNovuEmailLink and
linkExistingIntegration (and the integrationRepository.findOne call) when making
the validation change.
In `@apps/dashboard/src/components/agents/inbound-address-config.tsx`:
- Around line 43-53: The Add button's disabled state must mirror the same
validation used in handleAdd to prevent enabled-but-no-op clicks: update the
button's disabled expression to require domainName, non-empty localPart.trim(),
and LOCAL_PART_RE.test(localPart.trim()) (i.e. use LOCAL_PART_RE and
localPart.trim() the same way handleAdd does); keep handleAdd unchanged except
for ensuring it still calls onAddAddress(trimmed, domain) and setLocalPart('')
on success. This ensures the UI visibly rejects invalid chars the handler
currently rejects.
---
Nitpick comments:
In `@apps/dashboard/src/components/agents/inbound-address-config.tsx`:
- Line 43: Hoist the LOCAL_PART_RE regex out of the component render so it’s a
module-scoped constant (move the line defining LOCAL_PART_RE to top-level) to
avoid recreating it on every render; then simplify the JSX that builds the
local/full address by removing the redundant ternary (the branch that checks
addr.address === '*' is unnecessary because full already equals
`*@${addr.domain}`) and just render {full} where that ternary is currently used
(update references around the variables named full and addr.address
accordingly).
🪄 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: 6396a570-0356-448c-bf23-949eea9bf790
📒 Files selected for processing (3)
apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.tsapps/dashboard/src/components/agents/inbound-address-config.tsxapps/dashboard/src/components/agents/provider-dropdown.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/dashboard/src/components/agents/provider-dropdown.tsx
- Remove transaction from DeleteAgent to fix CI replica-set incompatibility (operations are idempotent, partial failure is safe) - Guard non-NovuAgent providerId path requiring integrationIdentifier - Gate seedEmailSecretKey with $exists:false to prevent rotating live secrets - Add aria-hidden to decorative info icon for accessibility Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.ts (3)
61-61: Minor:.filter(Boolean)is likely redundant.
_integrationIdis a required field onAgentIntegrationlinks, so every entry returned fromfind(..., ['_integrationId'])should have it set. The filter is harmless defensive coding but can go — otherwise consider an explicit non-null narrowing to keep the typing honest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.ts` at line 61, The `.filter(Boolean)` on the integrationIds extraction is redundant because `_integrationId` is required on AgentIntegration links; update the extraction in delete-agent.usecase.ts by removing the `.filter(Boolean)` and/or replace it with an explicit non-null type assertion or narrowing to keep TypeScript typing correct (e.g., map((l) => l._integrationId) with a non-null assertion) so that the variable `integrationIds` reflects the required `_integrationId` values from `links` returned by the `find(..., ['_integrationId'])` call.
74-85: UsedeleteMany()to batch delete integrations instead of looping.The
IntegrationRepositoryexposesdeleteMany()which properly enforces scoping fields. Since you're deleting multiple integrations by ID with identical_environmentIdand_organizationIdvalues, a single batched operation is more efficient.♻️ Suggested refactor
- for (const integration of novuEmailIntegrations) { - await this.integrationRepository.delete({ - _id: integration._id, - _environmentId: command.environmentId, - _organizationId: command.organizationId, - }); - this.logger.info( - { agentId, integrationId: integration._id }, - 'Deleted orphaned NovuAgent integration', - LOG_CONTEXT - ); - } + const orphanedIds = novuEmailIntegrations.map((i) => i._id); + await this.integrationRepository.deleteMany({ + _id: { $in: orphanedIds }, + _environmentId: command.environmentId, + _organizationId: command.organizationId, + }); + this.logger.info( + { agentId, integrationIds: orphanedIds }, + 'Deleted orphaned NovuAgent integrations' + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.ts` around lines 74 - 85, The loop that calls this.integrationRepository.delete for each item in novuEmailIntegrations should be replaced with a single batched call to this.integrationRepository.deleteMany; build a filter using the array of IDs (from novuEmailIntegrations.map(i => i._id)) together with _environmentId: command.environmentId and _organizationId: command.organizationId so scoping is enforced, call deleteMany once, and then emit a single info log referencing agentId and the number of deleted integrations (or the list of IDs) instead of per-item logs; update references in delete-agent.usecase.ts where novuEmailIntegrations and integrationRepository.delete are used.
8-8:LOG_CONTEXTis not attached as a log context field—callsetContext()in the constructor instead.Per nestjs-pino's API,
PinoLogger.info(obj, msg, ...args)treats trailing arguments as message interpolation values, not context parameters. PassingLOG_CONTEXTas the third argument does not attach it as the context field on the emitted log record. Either callthis.logger.setContext(LOG_CONTEXT)in the constructor and omit the third argument from all.info()and.warn()calls, or merge the context into the first argument object (e.g.,{ agentId, integrationId, context: LOG_CONTEXT }).Note: This pattern is used throughout the codebase; consider a sweeping fix if not already addressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.ts` at line 8, The LOG_CONTEXT constant is not being attached to log records; update the DeleteAgent use case to set the logger context by calling this.logger.setContext(LOG_CONTEXT) inside the class constructor and then remove LOG_CONTEXT as a trailing argument from all this.logger.info(...) and this.logger.warn(...) calls in this file (or alternatively merge it into the first object argument for those calls); reference the LOG_CONTEXT constant, the class constructor, and the this.logger.info/this.logger.warn invocations to locate and fix the usages.apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts (1)
132-145: Drop theas anycast on the integration create payload.The
as anyat Line 143 suppresses type checking for the whole create call, which is what surfaced the earlier "unconditional secret seeding" and "missing required field" classes of bugs in the first place. If a field name or required shape changes inIntegrationEntity, this call site won't be flagged by the compiler. Prefer constructing a typed partial (e.g.Partial<IntegrationEntity>or the repository's declared create DTO type) so the compiler catches schema drift.♻️ Proposed refactor
- const integration = await this.integrationRepository.create( - { - providerId: EmailProviderIdEnum.NovuAgent, - channel: ChannelTypeEnum.EMAIL, - credentials: { secretKey: encryptSecret(randomBytes(32).toString('hex')) }, - configurations: {}, - name: displayName, - identifier, - active: true, - _environmentId: command.environmentId, - _organizationId: command.organizationId, - } as any, - { session } - ); + const payload: Partial<IntegrationEntity> = { + providerId: EmailProviderIdEnum.NovuAgent, + channel: ChannelTypeEnum.EMAIL, + credentials: { secretKey: encryptSecret(randomBytes(32).toString('hex')) }, + configurations: {}, + name: displayName, + identifier, + active: true, + _environmentId: command.environmentId, + _organizationId: command.organizationId, + }; + const integration = await this.integrationRepository.create(payload, { session });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts` around lines 132 - 145, Remove the unsafe "as any" cast on the payload passed to integrationRepository.create and instead construct a properly typed object (e.g. use Partial<IntegrationEntity> or the repository's create DTO type) so the compiler validates required fields; locate the create call in add-agent-integration.usecase.ts (integrationRepository.create) and change the payload typing to the correct typed interface while keeping the same properties (providerId: EmailProviderIdEnum.NovuAgent, channel: ChannelTypeEnum.EMAIL, credentials: { secretKey: encryptSecret(randomBytes(32).toString('hex')) }, configurations, name, identifier, active, _environmentId, _organizationId) so any missing or mismatched fields on IntegrationEntity will be caught at compile time.
🤖 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/dashboard/src/components/agents/outbound-provider-select.tsx`:
- Around line 137-141: The current name generation uses count + 1 which can
collide if earlier-numbered items were deleted; update the logic around
createMutation.mutateAsync (the block using integrations, item.providerId and
item.displayName) to compute a next available numeric suffix by scanning
existing integration names for that provider (filter integrations by providerId
then parse trailing numeric suffixes from name like `${displayName} N`) and pick
the smallest unused positive integer, and if parsing fails or all suffixes are
taken, append a unique fallback (timestamp or short random string) to avoid
collisions.
---
Nitpick comments:
In
`@apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.ts`:
- Around line 132-145: Remove the unsafe "as any" cast on the payload passed to
integrationRepository.create and instead construct a properly typed object (e.g.
use Partial<IntegrationEntity> or the repository's create DTO type) so the
compiler validates required fields; locate the create call in
add-agent-integration.usecase.ts (integrationRepository.create) and change the
payload typing to the correct typed interface while keeping the same properties
(providerId: EmailProviderIdEnum.NovuAgent, channel: ChannelTypeEnum.EMAIL,
credentials: { secretKey: encryptSecret(randomBytes(32).toString('hex')) },
configurations, name, identifier, active, _environmentId, _organizationId) so
any missing or mismatched fields on IntegrationEntity will be caught at compile
time.
In `@apps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.ts`:
- Line 61: The `.filter(Boolean)` on the integrationIds extraction is redundant
because `_integrationId` is required on AgentIntegration links; update the
extraction in delete-agent.usecase.ts by removing the `.filter(Boolean)` and/or
replace it with an explicit non-null type assertion or narrowing to keep
TypeScript typing correct (e.g., map((l) => l._integrationId) with a non-null
assertion) so that the variable `integrationIds` reflects the required
`_integrationId` values from `links` returned by the `find(...,
['_integrationId'])` call.
- Around line 74-85: The loop that calls this.integrationRepository.delete for
each item in novuEmailIntegrations should be replaced with a single batched call
to this.integrationRepository.deleteMany; build a filter using the array of IDs
(from novuEmailIntegrations.map(i => i._id)) together with _environmentId:
command.environmentId and _organizationId: command.organizationId so scoping is
enforced, call deleteMany once, and then emit a single info log referencing
agentId and the number of deleted integrations (or the list of IDs) instead of
per-item logs; update references in delete-agent.usecase.ts where
novuEmailIntegrations and integrationRepository.delete are used.
- Line 8: The LOG_CONTEXT constant is not being attached to log records; update
the DeleteAgent use case to set the logger context by calling
this.logger.setContext(LOG_CONTEXT) inside the class constructor and then remove
LOG_CONTEXT as a trailing argument from all this.logger.info(...) and
this.logger.warn(...) calls in this file (or alternatively merge it into the
first object argument for those calls); reference the LOG_CONTEXT constant, the
class constructor, and the this.logger.info/this.logger.warn invocations to
locate and fix the usages.
🪄 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: 09df4be4-5c2f-4173-97cc-1167ae3c9834
📒 Files selected for processing (3)
apps/api/src/app/agents/usecases/add-agent-integration/add-agent-integration.usecase.tsapps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.tsapps/dashboard/src/components/agents/outbound-provider-select.tsx
…ver 6.x Root cause: In MongoDB Node.js driver 6.x, startTransaction() is purely client-side — the server error only surfaces when the first DB operation is sent with the transaction ID. By that time, the callback has already been entered, setting the `executed` flag to true. The `!executed` guard then prevents the standalone fallback from triggering. On standalone MongoDB, the server rejects the command entirely without executing it, so no data is persisted and re-running fn(null) is safe. Changes: - Remove `executed` guard from withTransaction fallback - Use includes() instead of exact string match for future-proofing - Restore DeleteAgent transaction (was incorrectly removed as workaround) Made-with: Cursor
Scan existing names and find the next available suffix instead of using a simple count, which collides when earlier-numbered items were deleted. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/dal/src/repositories/base-repository-v2.ts (1)
529-538:⚠️ Potential issue | 🟡 MinorFallback may re-execute
fnfrom the top on standalone Mongo.With the
executedguard removed, iffn(txnSession)partially runs (reads succeed, first write triggers the error) beforesession.withTransactionrejects,fn(null)now runs the whole function again without session. This is fine for the intended dev-only standalone fallback where the in-transaction writes were aborted, but any non-idempotent operation insidefn(e.g., logs, metrics, external calls, counters) will observe a duplicate invocation in that path. Consider a short doc comment here noting the fallback contract (fnmust be safe to re-invoke) so callers don't accidentally add non-idempotent side effects later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/dal/src/repositories/base-repository-v2.ts` around lines 529 - 538, The fallback path can re-run the passed-in callback `fn` without a session (via the `fn(null)` call in the catch block when `errorMessage.includes('Transaction numbers are only allowed on')`), which may cause duplicate non-idempotent side effects; add a concise doc comment above this function/method (the one that calls `session.withTransaction` and accepts `fn`) explaining the fallback contract: callers must ensure `fn` is safe to be retried or re-invoked (idempotent, no externally-visible side effects like metrics/logging/external calls/counters), and mention that `fn` may be invoked twice (once with a txn session and once without) on standalone Mongo fallback. Ensure the comment references the `fn` parameter and the `session.withTransaction` fallback behavior.
♻️ Duplicate comments (1)
apps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.ts (1)
80-105:⚠️ Potential issue | 🟡 MinorVerify "orphaned" semantics — NovuAgent integration deletion should check for remaining agent links.
The loop deletes every NovuAgent integration linked to this agent without verifying the integration is not still referenced by another agent's
AgentIntegrationrecord. Although creation of NovuAgent integrations enforces a 1:1 agent-to-integration relationship (each agent creates a unique Integration document), the deletion path does not defensively verify this invariant before deleting. For data consistency and resilience to future changes, check remaining links before deletion:♻️ Sketch: only delete when no other agent references the integration
- for (const integration of novuEmailIntegrations) { - await this.integrationRepository.delete( - { - _id: integration._id, - _environmentId: command.environmentId, - _organizationId: command.organizationId, - }, - { session } - ); - this.logger.info( - { agentId, integrationId: integration._id }, - 'Deleted orphaned NovuAgent integration', - LOG_CONTEXT - ); - } + for (const integration of novuEmailIntegrations) { + const otherLinks = await this.agentIntegrationRepository.find( + { + _integrationId: integration._id, + _agentId: { $ne: agentId }, + _environmentId: command.environmentId, + _organizationId: command.organizationId, + }, + ['_id'], + { session } + ); + if (otherLinks.length > 0) continue; + + await this.integrationRepository.delete( + { + _id: integration._id, + _environmentId: command.environmentId, + _organizationId: command.organizationId, + }, + { session } + ); + this.logger.info( + { agentId, integrationId: integration._id }, + 'Deleted orphaned NovuAgent integration', + LOG_CONTEXT + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.ts` around lines 80 - 105, The loop currently deletes every NovuAgent integration from novuEmailIntegrations without confirming it's not referenced by other agents; update the deletion to first query the AgentIntegration relations for each integration._id (e.g., via this.agentIntegrationRepository.find or count) filtering out the current agentId, and only call this.integrationRepository.delete for an integration when no other AgentIntegration records reference it; retain the existing logging (this.logger.info with agentId and integrationId) but only after the conditional delete to ensure we don't remove integrations still in use.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@libs/dal/src/repositories/base-repository-v2.ts`:
- Around line 529-538: The fallback path can re-run the passed-in callback `fn`
without a session (via the `fn(null)` call in the catch block when
`errorMessage.includes('Transaction numbers are only allowed on')`), which may
cause duplicate non-idempotent side effects; add a concise doc comment above
this function/method (the one that calls `session.withTransaction` and accepts
`fn`) explaining the fallback contract: callers must ensure `fn` is safe to be
retried or re-invoked (idempotent, no externally-visible side effects like
metrics/logging/external calls/counters), and mention that `fn` may be invoked
twice (once with a txn session and once without) on standalone Mongo fallback.
Ensure the comment references the `fn` parameter and the
`session.withTransaction` fallback behavior.
---
Duplicate comments:
In `@apps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.ts`:
- Around line 80-105: The loop currently deletes every NovuAgent integration
from novuEmailIntegrations without confirming it's not referenced by other
agents; update the deletion to first query the AgentIntegration relations for
each integration._id (e.g., via this.agentIntegrationRepository.find or count)
filtering out the current agentId, and only call
this.integrationRepository.delete for an integration when no other
AgentIntegration records reference it; retain the existing logging
(this.logger.info with agentId and integrationId) but only after the conditional
delete to ensure we don't remove integrations still in use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b08dae59-c4a3-4adf-8665-a38b6a6d501d
📒 Files selected for processing (2)
apps/api/src/app/agents/usecases/delete-agent/delete-agent.usecase.tslibs/dal/src/repositories/base-repository-v2.ts
…upport - Extract cascade cleanup into CleanupNovuEmail usecase (shared by DeleteAgent & RemoveAgentIntegration) - Extract NovuAgent auto-creation into FindOrCreateNovuEmail usecase (simplifies AddAgentIntegration) - Fix N+1 query in UpdateDomain.validateAgentDestinations — batch with $in - Fix DomainRepository.removeRoutesByDestination to use this.update() for env/org enforcement - Add duplicate address guard in InboundAddressConfig - Use useMutation for domain route updates in use-email-setup-credentials - Fix credentialsRef merge order to preserve in-flight edits - Improve hasOutboundCredentials heuristic to check all required keys Made-with: Cursor
ChmaraX
deleted the
nv-7406-multi-domain-inbound-email-support-with-data-model
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Domain.routes[]the single source of truth for inbound email routing — removes staticreplyDomain/inboundAddress/inboundDomainfrom integration credentialsFrom:address resolution via Redis thread state per conversation threadClientSessionpassing for all multi-collection operationsemail-setup-guide.tsx— extractOutboundProviderSelectandInboundAddressConfiginto own filesArchitecture
sequenceDiagram participant S as Subscriber participant W as Worker (SMTP) participant D as Domain Collection participant A as API Webhook participant EA as Email Adapter participant R as Redis Thread State participant AG as Agent Handler S->>W: Email to support@acme.com W->>D: Lookup domain "acme.com" D-->>W: Route: address=support → agent=xyz W->>A: POST /webhook (HMAC signed) A->>EA: handleWebhook(payload) EA->>R: trackAgentAddress(threadId, "support@acme.com") EA->>AG: onMessage(message) AG-->>EA: reply text EA->>R: getAgentAddress(threadId) R-->>EA: "support@acme.com" EA->>S: Send reply From: support@acme.comData model consistency fixes
DeleteAgent: cascade removes domain routes + orphaned NovuAgent integrations (transactional)RemoveAgentIntegration: cascade cleans domain routes + NovuAgent integration when email link removed (transactional)AddAgentIntegration.findOrCreateNovuEmailLink: atomic Integration + AgentIntegration creation (transactional)UpdateDomain: validates agent route destinations reference existing agentsBaseRepositoryV2.findOneAndDelete,IntegrationRepository.delete/create,DomainRepository.removeRoutesByDestinationTest plan
Closes NV-7406
Made with Cursor
What changed
Inbound email routing was overhauled to make Domain.routes[] the single source of truth and remove static inbound/reply fields from integration credentials. Reply-from resolution is now dynamic per conversation by storing an agent address in thread state (Redis). Multi-document operations (agents, integrations, domains) are executed transactionally via explicit MongoDB ClientSession propagation to ensure atomic create/delete and cascade cleanup. The dashboard onboarding was refactored to support multiple inbound addresses and a dedicated outbound provider selector, with UI step readiness derived from real integration state.
Affected areas
Key technical decisions
Testing
A manual/integration test plan is included (integration/link creation, multi-domain routing, send/reply flows, transactional cascade deletions, UpdateDomain validation, and UI onboarding persistence); no new automated tests were added in this change.