feat(api,framework): replace ctx.update with replyHandle.edit#10773
Merged
feat(api,framework): replace ctx.update with replyHandle.edit#10773
Conversation
ctx.update sent a new message without signals instead of editing the
previously sent one, producing thread clutter and misleading DX.
Framework:
- Remove ctx.update.
- ctx.reply now returns a ReplyHandle with edit(), closing over the
messageId/platformThreadId so subsequent edits mutate the same message.
- Export ReplyHandle, EditPayload, SentMessageInfo from the public API.
API:
- Replace update in AgentReplyPayloadDto with edit { messageId, content }.
- ChatSdkService.postToConversation now returns SentMessageInfo; add
editInConversation calling adapter.editMessage.
- HandleAgentReply returns SentMessageInfo | null (no phantom status field);
persists an EDIT activity with platformMessageId on the edit path.
- Reject edit combined with signals or resolve.
DAL:
- ConversationActivityRepository.createAgentActivity accepts platformMessageId.
- ConversationActivityTypeEnum: UPDATE ('update') renamed to EDIT ('edit').
Pre-GA feature, flag-gated (IS_CONVERSATIONAL_AGENTS_ENABLED).
Made-with: Cursor
✅ Deploy preview added
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
Contributor
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughRefactors agent reply/update flow: renames Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Ctrl as Webhook Controller
participant UC as HandleAgentReply Usecase
participant ChatSDK as Chat SDK Service
participant Adapter as Platform Adapter
participant DB as Conversation Activity DB
Client->>Ctrl: POST /v1/agents/:id/reply (reply or edit)
Ctrl->>UC: HandleAgentReplyCommand (reply/edit payload)
alt reply
UC->>ChatSDK: postToConversation(content)
ChatSDK->>Adapter: thread.post(message)
Adapter-->>ChatSDK: SentMessageInfo {messageId, threadId}
ChatSDK-->>UC: SentMessageInfo
UC->>DB: createAgentActivity(REPLY, platformMessageId, platformThreadId)
else edit
UC->>ChatSDK: editInConversation(platformThreadId, platformMessageId, content)
ChatSDK->>Adapter: adapter.editMessage(...)
Adapter-->>ChatSDK: SentMessageInfo {messageId, threadId}
ChatSDK-->>UC: SentMessageInfo
UC->>DB: createAgentActivity(EDIT, platformMessageId, platformThreadId)
end
UC-->>Ctrl: SentMessageInfo | null
Ctrl-->>Client: { messageId?, platformThreadId? }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
packages/framework/src/resources/agent/agent.types.ts (1)
134-154:AgentContext.updateremoved without a@deprecatedcycle.Per coding guidelines, symbols in
packages/**should carry an@deprecatedJSDoc before being removed. The priorupdate(content)method onAgentContexthas been removed outright in this PR. The PR description calls this out as acceptable because the feature is pre-GA behindIS_CONVERSATIONAL_AGENTS_ENABLED; please make sure that rationale is captured in the package changelog / migration notes so downstream consumers of@novu/frameworkaren't surprised by a TS build break on upgrade.As per coding guidelines: "Deprecate symbols with
@deprecatedJSDoc before removing them from packages".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framework/src/resources/agent/agent.types.ts` around lines 134 - 154, The AgentContext.update method was removed without a deprecation cycle; either restore a deprecated stub or document the breaking change: add back a no-op update(content) signature on the AgentContext interface annotated with `@deprecated` JSDoc pointing to the feature flag IS_CONVERSATIONAL_AGENTS_ENABLED and noting it will be removed, or, if you intentionally remove it now, add explicit migration notes to the package changelog for `@novu/framework` explaining the removal and the feature-flag rationale so downstream consumers aren’t caught by a TS build break.
🤖 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/e2e/agent-reply.e2e.ts`:
- Line 103: Test title is stale: the it(...) description says "update activity"
but the test asserts an EDIT activity; update the test description in the
it(...) call in agent-reply.e2e.ts (the spec starting with the string "should
edit a previously sent message and persist an update activity") to accurately
reflect the assertion, e.g., change "persist an update activity" to "persist an
EDIT activity" or otherwise match the asserted activity type in the test body.
In
`@apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts`:
- Around line 187-223: deliverEdit currently saves an EDIT activity but doesn’t
update the parent conversation’s lastActivityAt/lastMessagePreview like
deliverMessage does via conversationRepository.touchActivity, so edits never
update inbox preview or bump last activity; modify deliverEdit to call the
conversation touch primitive after creating the activity—either call
conversationRepository.touchActivity(...) (if it can be used without
incrementing messageCount) or add/use a new method (e.g.,
conversationRepository.touchActivityPreview or touchActivityWithoutIncrement)
that updates lastActivityAt and lastMessagePreview without incrementing
messageCount, and invoke it conditionally when the edited message is the latest
(compare sent.messageId or edit.messageId against the conversation’s current
latest platform message id stored on the conversation) using conversation._id,
channel.platform, channel.platformThreadId and textFallback to set the preview.
In
`@libs/dal/src/repositories/conversation-activity/conversation-activity.entity.ts`:
- Around line 5-11: The enum rename from UPDATE → EDIT in
ConversationActivityTypeEnum changed the persisted string value and will leave
existing records with type:'update' orphaned; add a data migration that finds
ConversationActivity records where type === 'update' and updates them to 'edit'
(or run a one-off backfill if using raw DB access), ensure the migration
references ConversationActivityTypeEnum and the string literals 'update' →
'edit' so it runs before/with deployment, and gate or document the migration in
the IS_CONVERSATIONAL_AGENTS_ENABLED rollout so production data is corrected
before clients/aggregations rely on ConversationActivityTypeEnum.EDIT.
In `@packages/framework/src/resources/agent/agent.context.ts`:
- Around line 65-80: The implementation of ReplyHandleImpl.edit currently
returns a new ReplyHandleImpl (using info.messageId/info.platformThreadId) which
contradicts the documented contract; change edit() to mutate the existing
handle: after a successful poster.post call, update this.messageId and
this.platformThreadId from the returned info (if present) and then return this
(the same ReplyHandleImpl) to preserve chaining. Also ensure the
ReplyHandle.edit JSDoc in agent.types.ts and the example in AgentContext.reply's
JSDoc describe the in-place mutation/chaining behavior so types/docs match the
implementation.
- Around line 223-225: Update the stale comment in the JSON parse catch block in
agent.context.ts to reflect current behavior: replace the line "// non-JSON
responses are tolerated (e.g. flush path returns {status} only)" with a comment
that says something like "// flush-only responses return null or an empty body;
tolerate and return null" so the catch in the response-parsing logic correctly
documents that flush returns null rather than a {status} envelope.
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 125-132: The JSDoc on ReplyHandle.edit claims it "Returns the same
handle for chaining" but the implementation ReplyHandleImpl.edit currently
constructs and returns a new handle from the edit response; update the contract
to match the implementation: change the JSDoc on ReplyHandle.edit to state it
returns a new ReplyHandle (new handle-per-edit semantics) and update the usage
example (the ctx.reply example) to await and reassign the returned handle (e.g.
msg = await msg.edit(...)); ensure any inline comments or examples referencing
same-handle chaining are adjusted accordingly so docs and ReplyHandleImpl.edit
remain consistent.
---
Nitpick comments:
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 134-154: The AgentContext.update method was removed without a
deprecation cycle; either restore a deprecated stub or document the breaking
change: add back a no-op update(content) signature on the AgentContext interface
annotated with `@deprecated` JSDoc pointing to the feature flag
IS_CONVERSATIONAL_AGENTS_ENABLED and noting it will be removed, or, if you
intentionally remove it now, add explicit migration notes to the package
changelog for `@novu/framework` explaining the removal and the feature-flag
rationale so downstream consumers aren’t caught by a TS build break.
🪄 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: 860b712c-13da-488a-82c3-7371d5610e09
📒 Files selected for processing (13)
apps/api/src/app/agents/agents-webhook.controller.tsapps/api/src/app/agents/dtos/agent-reply-payload.dto.tsapps/api/src/app/agents/e2e/agent-reply.e2e.tsapps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.command.tsapps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.tslibs/dal/src/repositories/conversation-activity/conversation-activity.entity.tslibs/dal/src/repositories/conversation-activity/conversation-activity.repository.tspackages/framework/src/index.tspackages/framework/src/resources/agent/agent.context.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.tspackages/framework/src/resources/agent/index.ts
Comment on lines
+69
to
73
| if (command.edit) { | ||
| const agentName = await this.resolveValidatedAgentNameForDelivery(command, conversation); | ||
|
|
||
| await this.deliverMessage( | ||
| command, | ||
| conversation, | ||
| channel, | ||
| command.update, | ||
| ConversationActivityTypeEnum.UPDATE, | ||
| agentName | ||
| ); | ||
|
|
||
| return { status: 'update_sent' }; | ||
| return this.deliverEdit(command, conversation, channel, command.edit, agentName); | ||
| } |
Contributor
There was a problem hiding this comment.
Edit path still requires serializedThread via getPrimaryChannel.
getPrimaryChannel (line 135-137) throws BadRequestException when channel.serializedThread is missing, but deliverEdit only consumes channel.platform, channel.platformThreadId, and channel._integrationId — not the serialized thread. A conversation that is missing serializedThread for whatever reason (e.g. partial backfill, reconciliation) would unnecessarily fail edits. Consider relaxing the check to a per-path requirement (only needed for deliverMessage).
Low priority — flag if this scenario is realistic in production data.
Comment on lines
5
to
11
| export enum ConversationActivityTypeEnum { | ||
| MESSAGE = 'message', | ||
| /** Interim status update sent via ctx.update() */ | ||
| UPDATE = 'update', | ||
| /** In-place edit of a previously sent agent message, via replyHandle.edit() */ | ||
| EDIT = 'edit', | ||
| /** System-generated timeline event (e.g. workflow triggered, conversation resolved) */ | ||
| SIGNAL = 'signal', | ||
| } |
Contributor
There was a problem hiding this comment.
Breaking enum rename requires a data migration.
Renaming UPDATE = 'update' → EDIT = 'edit' changes the persisted string value. Any pre-existing ConversationActivity documents written with type: 'update' will no longer match queries filtering by ConversationActivityTypeEnum.EDIT and will silently fall outside any type-based aggregations/UI. Even though this is pre-GA behind IS_CONVERSATIONAL_AGENTS_ENABLED, please confirm there is either (a) no production data with type: 'update', or (b) a migration/backfill updating those documents to 'edit'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@libs/dal/src/repositories/conversation-activity/conversation-activity.entity.ts`
around lines 5 - 11, The enum rename from UPDATE → EDIT in
ConversationActivityTypeEnum changed the persisted string value and will leave
existing records with type:'update' orphaned; add a data migration that finds
ConversationActivity records where type === 'update' and updates them to 'edit'
(or run a one-off backfill if using raw DB access), ensure the migration
references ConversationActivityTypeEnum and the string literals 'update' →
'edit' so it runs before/with deployment, and gate or document the migration in
the IS_CONVERSATIONAL_AGENTS_ENABLED rollout so production data is corrected
before clients/aggregations rely on ConversationActivityTypeEnum.EDIT.
Comment on lines
+125
to
+132
| export interface ReplyHandle { | ||
| /** Platform-native message id (e.g. Slack ts, Teams activityId). */ | ||
| readonly messageId: string; | ||
| /** Platform-native thread id this message lives in. */ | ||
| readonly platformThreadId: string; | ||
| /** Edit this message in place with new content. Returns the same handle for chaining. */ | ||
| edit(content: MessageContent): Promise<ReplyHandle>; | ||
| } |
Contributor
There was a problem hiding this comment.
JSDoc "Returns the same handle for chaining" doesn't match ReplyHandleImpl.edit.
The implementation returns a new ReplyHandleImpl built from the edit response (agent.context.ts lines 79). Either update the doc to describe the new-handle-per-edit semantics (and update the ctx.reply example on line 150-152 accordingly, e.g. msg = await msg.edit('Here is the answer')), or change the impl to return this. See the paired comment on agent.context.ts for tradeoffs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/framework/src/resources/agent/agent.types.ts` around lines 125 -
132, The JSDoc on ReplyHandle.edit claims it "Returns the same handle for
chaining" but the implementation ReplyHandleImpl.edit currently constructs and
returns a new handle from the edit response; update the contract to match the
implementation: change the JSDoc on ReplyHandle.edit to state it returns a new
ReplyHandle (new handle-per-edit semantics) and update the usage example (the
ctx.reply example) to await and reassign the returned handle (e.g. msg = await
msg.edit(...)); ensure any inline comments or examples referencing same-handle
chaining are adjusted accordingly so docs and ReplyHandleImpl.edit remain
consistent.
ChmaraX
changed the title
- ReplyHandleImpl.edit now mutates in place and returns `this`, matching
the ReplyHandle.edit JSDoc contract ("returns the same handle for
chaining"). messageId/platformThreadId are refreshed from the edit
response so subsequent edits target the correct platform message even
if an adapter returns a refreshed id.
- Fix stale e2e test title ("update activity" -> "edit activity").
- Fix stale inline comment about flush responses returning {status}.
Made-with: Cursor
Previously, deliverEdit persisted an EDIT activity but never touched the parent conversation — so a common flow (post "Thinking..." placeholder, then edit with the real answer) left the inbox preview stuck on "Thinking..." forever. - Add ConversationRepository.touchPreview: updates lastActivityAt and lastMessagePreview WITHOUT incrementing messageCount (edits mutate an existing message, they don't add a new one). - Call it from HandleAgentReply.deliverEdit alongside the EDIT activity write. - Strengthen the edit e2e: assert lastMessagePreview reflects the edited content AND messageCount stays unchanged. Made-with: Cursor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/framework/src/resources/agent/agent.context.ts (1)
206-240: Response parsing is reasonable; consider a narrow log on unexpected shapes.The envelope handling (raw or
{ data }), empty-body tolerance, and field-type guards are all sensible and the updated catch comment now matches the flush→null contract. One optional enhancement: whenrawis non-empty and parses successfully but the shape doesn't match (falls through toreturn nullat line 239), consider aconsole.warnso a silently-malformed server response doesn't manifest downstream as the generic "did not return a message handle" error fromreply()/edit(). Not blocking — current behavior is safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framework/src/resources/agent/agent.context.ts` around lines 206 - 240, The _post method currently tolerates non-empty/parsable responses that lack the expected envelope shape and silently returns null; add a narrow warning log when raw is non-empty and parsing succeeds but the resulting envelope does not contain string messageId and platformThreadId. Inside _post (near variables raw, parsed, envelope) after the type guards that check envelope.messageId and envelope.platformThreadId fail, emit a concise console.warn (or use an existing logger if available) including the raw response and a short context string (e.g., "Agent reply parsed but missing message handle") so malformed shapes are visible without changing the return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/framework/src/resources/agent/agent.context.ts`:
- Around line 206-240: The _post method currently tolerates non-empty/parsable
responses that lack the expected envelope shape and silently returns null; add a
narrow warning log when raw is non-empty and parsing succeeds but the resulting
envelope does not contain string messageId and platformThreadId. Inside _post
(near variables raw, parsed, envelope) after the type guards that check
envelope.messageId and envelope.platformThreadId fail, emit a concise
console.warn (or use an existing logger if available) including the raw response
and a short context string (e.g., "Agent reply parsed but missing message
handle") so malformed shapes are visible without changing the return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0cf32c66-9f12-4e3a-927a-d15dfa448d22
📒 Files selected for processing (2)
apps/api/src/app/agents/e2e/agent-reply.e2e.tspackages/framework/src/resources/agent/agent.context.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/app/agents/e2e/agent-reply.e2e.ts
"Inbox" is a distinct Novu product; the field we're touching is `lastMessagePreview` on a Conversation. Reworded comments and PR body to avoid the terminology collision. No behavior change. Made-with: Cursor
…handle Made-with: Cursor # Conflicts: # apps/api/src/app/agents/services/chat-sdk.service.ts
This was referenced Apr 20, 2026
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
ctx.update→replyHandle.edit.ctx.updateposted a new message (without signals) instead of editing the previously sent one — producing thread clutter and misleading the developer who expected "update" to mean "edit in place." Replaced with a handle-based API that mirrors the underlying chat SDK'sSentMessage.edit().ctx.replyreturns aReplyHandlewith{ messageId, platformThreadId, edit(content) }. Callinghandle.edit(...)mutates the existing platform message viaadapter.editMessageinstead of posting a new one.AgentReplyPayload.updatereplaced withedit: { messageId, content }. The API usecase now returnsSentMessageInfo | null— the phantomstatus: 'ok' | 'edited'tag was only read by tests; the framework's response parser only ever extractedmessageId/platformThreadId.ConversationActivityTypeEnum.EDIT(renamed fromUPDATE) withplatformMessageIdrecorded.New DX
Before:
After:
msg.edit(...)mutatesmsgin place and returns the same handle, so chained edits (msg.edit(a); msg.edit(b);) keep working even on platforms whereeditMessagehappens to return a refreshed id.Flow
sequenceDiagram participant User as user code participant Ctx as AgentContextImpl participant API as POST /v1/agents/:id/reply participant Chat as chat SDK adapter User->>Ctx: ctx.reply(content) Ctx->>API: { reply: content } API->>Chat: thread.post(...) Chat-->>API: { id, threadId } API-->>Ctx: { messageId, platformThreadId } Ctx-->>User: ReplyHandle Note over User,Chat: later — same messageId, mutates in place User->>API: msg.edit(content) → { edit: { messageId, content } } API->>Chat: adapter.editMessage(threadId, messageId, ...) Chat-->>API: { id, threadId } API-->>User: refreshed ids (same handle)Breaking changes
ctx.update(content)removed. Migration:const msg = await ctx.reply(...); await msg.edit(...). No@deprecatedcycle — the agent surface is pre-GA behindIS_CONVERSATIONAL_AGENTS_ENABLEDand has no external consumers.AgentReplyPayload.updateremoved; useedit: { messageId, content }.{ messageId, platformThreadId }directly (nostatuswrapper); flush-only path returnsnull.ConversationActivityTypeEnum.UPDATE('update') →EDIT('edit'). Pre-GA feature behind the same flag — no material prod data impact; two code consumers (usecase + e2e), zero UI/analytics consumers. Because the feature is flag-gated and unreleased, no backfill migration is shipped with this PR.editcannot be combined withreply,signals, orresolve— each is now a hard 400.Notes for reviewers
editrequires the platform adapter to implementeditMessage; returns 400 with a clear error if not supported.UPDATEwas named afterctx.updatewhich no longer exists.SentMessageInfolives in@novu/frameworkand is imported directly byapps/api— single source of truth for the wire-message-reference type.Addressed from CodeRabbit review
ReplyHandleImpl.editnow mutatesthisand returns the same handle (matches theReplyHandle.editcontract: "returns the same handle for chaining"). The handle'smessageId/platformThreadIdare refreshed from the edit response so subsequent edits target the right platform message even if an adapter ever returns a refreshed id.ConversationRepository.touchPreviewprimitive;deliverEditnow refresheslastActivityAt+lastMessagePreviewwithout incrementingmessageCount. Previously the common "post placeholder, then edit with the real answer" flow left the conversation'slastMessagePreviewstuck on the placeholder text. E2e asserts the new behavior and thatmessageCountstays unchanged.{status}(they returnnullnow).api-service→api(matches allowed scope list).Intentionally not addressed
UPDATE → EDITenum rename. Feature is pre-GA behindIS_CONVERSATIONAL_AGENTS_ENABLEDwith no external consumers — no backfill needed.@deprecatedcycle forctx.update. Same rationale: pre-GA surface, zero downstream consumers; the cycle would be ceremony.getPrimaryChannelrequiresserializedThreadeven on the edit path which doesn't use it. Hypothetical in practice; any conversation you can edit already has aserializedThread. Will relax if we hit a real case.Test plan
packages/frameworkunit tests: 26/26 pass (includes newmsg.edit()cases covering payload shape and that signals/resolve are not attached to edits).apps/apitypecheck clean onagents/.pnpm --filter @novu/api test:e2e agent-reply.e2e.ts— covers reply, edit, flush-only paths, plus rejection of invalid combinations.Made with Cursor