Conversation
…V-7451
- Rewrite starter template: remove onResolve, add inline comments explaining
history/subscriber/metadata, show ctx.trigger in resolution path, use
subscriber.firstName in greeting
- Add JSDoc to all undocumented public interfaces in agent.types.ts:
AgentHandlers, AgentContextBase, AgentMessageContext, AgentActionContext,
AgentReactionContext, AgentResolveContext, AgentMessage, AgentConversation,
AgentSubscriber, AgentHistoryEntry, AgentAction, AgentReaction,
AgentPlatformContext, AgentAttachment, AgentMessageAuthor
- Change all AgentHandlers signatures from (ctx) to destructured payloads:
onMessage({ message, ctx }), onAction({ actionId, value, ctx }),
onReaction({ reaction, ctx }), onResolve({ ctx })
Promotes event-specific data to the top level; ctx still available for
everything else. Breaking change — package is in alpha.
- Update handler.ts dispatch from uniform handlerMap to per-event switch,
constructing the correct payload for each handler
- Update all 50 agent tests to use the new handler signatures
Co-authored-by: Cursor <cursoragent@cursor.com>
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces payload-shaped agent handler signatures and event-specific AgentContext types; replaces handler-map dispatch with a switch over AgentEventEnum in runAgentHandler; expands agent types (messages, conversation, actions, reactions); updates runtime imports, tests, templates, and E2E mock handlers to the new shape. ChangesAgent Event Handler Restructuring
Sequence DiagramsequenceDiagram
participant Trigger as Event Trigger
participant Handler as runAgentHandler
participant Switch as Event Switch
participant Context as AgentContextImpl
participant Hook as Agent Hook
participant Reply as ctx.reply
Trigger->>Handler: deliver AgentBridgeRequest (with event)
Handler->>Switch: inspect AgentEventEnum
alt ON_MESSAGE
Switch->>Context: build AgentMessageContext
Context->>Hook: invoke onMessage({ message, ctx })
Hook->>Reply: returns MessageContent?
else ON_ACTION
Switch->>Context: build AgentActionContext
Context->>Hook: invoke onAction({ actionId, value, ctx })
Hook->>Reply: returns MessageContent?
else ON_REACTION
Switch->>Context: build AgentReactionContext
Context->>Hook: invoke onReaction({ reaction, ctx })
Hook->>Reply: returns MessageContent?
else ON_RESOLVE
Switch->>Context: build AgentResolveContext
Context->>Hook: invoke onResolve({ ctx })
Hook->>Reply: returns MessageContent?
else Unknown
Switch->>Handler: throw InvalidActionError
end
Handler->>Reply: if result != null then ctx.reply(result)
Reply->>Handler: reply persisted / acknowledged
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/framework/src/handler.ts (1)
322-355: 💤 Low valueDefensive: non-null assertions on bridge fields can NPE on malformed events.
ctx.message!,ctx.action!,ctx.reaction!assume the bridge always sends the corresponding payload for each event. If a future bridge bug or replay sends anonActionevent withaction: null,ctx.action!.actionIdthrows an opaqueTypeErrorinstead of a structuredAgentDeliveryError. Consider validating before dispatch and returning a typed error so the bridge can observe it via the existing error handling on Lines 236-242. Not strictly required since the bridge controls these payloads, but worth a quick guard given this is the public dispatch entry point.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/framework/src/handler.ts` around lines 322 - 355, Replace the unsafe non-null assertions in the event dispatch switch (in handler.ts) by validating bridge payloads before accessing them: check ctx.message, ctx.action, and ctx.reaction exist when handling AgentEventEnum.ON_MESSAGE, ON_ACTION and ON_REACTION respectively, and if any are missing throw/return a structured AgentDeliveryError (including the event name and missing field) instead of letting a raw TypeError surface; update the branches that call registeredAgent.handlers.onMessage, onAction, onReaction (and onResolve guard) to use these guards so the existing error handling path (the AgentDeliveryError case used by the dispatch error handler) receives a typed error rather than an opaque NPE.packages/framework/src/resources/agent/agent.types.ts (1)
314-318: ⚡ Quick winMake
onActionpayload consistent withonMessage/onReactionby passing the fullAgentAction.
onMessagereceives{ message: AgentMessage, ctx }andonReactionreceives{ reaction: AgentReaction, ctx }, butonActioncherry-picksactionIdandvaluefromAgentActionand dropssourceMessageId. Developers who needsourceMessageId(e.g. to attach a reaction to the source card — see the test on lines 895-941 inagent.test.ts) must reach back intoctx.action.sourceMessageId, which defeats the purpose of the destructured payload and creates an inconsistent SDK surface.Since the package is alpha and there's no deprecation contract yet, I'd lock in the consistent shape now.
♻️ Proposed signature change
- onAction?: (payload: { - actionId: string; - value?: string; - ctx: AgentActionContext; - }) => Awaitable<MessageContent | void>; + onAction?: (payload: { action: AgentAction; ctx: AgentActionContext }) => Awaitable<MessageContent | void>;This also requires the dispatch in
packages/framework/src/handler.ts(Lines 329-337) to pass{ action: ctx.action!, ctx: ... }, and the template/tests to destructure{ action: { actionId, value }, ctx }(or{ action, ctx }).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/framework/src/resources/agent/agent.types.ts` around lines 314 - 318, Change the onAction handler signature to accept the full AgentAction object (make onAction?: (payload: { action: AgentAction; ctx: AgentActionContext }) => Awaitable<MessageContent | void>), so it matches onMessage/onReaction shapes; update the dispatch that invokes onAction (where it currently passes { actionId, value, ctx } or similar) to pass { action: ctx.action!, ctx } and update any templates/tests to destructure either { action, ctx } or { action: { actionId, value, sourceMessageId }, ctx } as needed. Ensure imports/type references for AgentAction and AgentActionContext are present where onAction is declared and where the handler dispatch occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/framework/src/handler.ts`:
- Around line 320-357: The variable result is read after the switch but may
remain unassigned if an optional handler like
registeredAgent.handlers.onAction/onReaction/onResolve is absent; initialize
result when declared (e.g., let result: MessageContent | void = undefined) so
TypeScript TS2454 is resolved and the intent of "no reply by default" is
explicit; update the declaration of result near the switch that branches on
AgentEventEnum and leave the rest of the handler logic unchanged.
---
Nitpick comments:
In `@packages/framework/src/handler.ts`:
- Around line 322-355: Replace the unsafe non-null assertions in the event
dispatch switch (in handler.ts) by validating bridge payloads before accessing
them: check ctx.message, ctx.action, and ctx.reaction exist when handling
AgentEventEnum.ON_MESSAGE, ON_ACTION and ON_REACTION respectively, and if any
are missing throw/return a structured AgentDeliveryError (including the event
name and missing field) instead of letting a raw TypeError surface; update the
branches that call registeredAgent.handlers.onMessage, onAction, onReaction (and
onResolve guard) to use these guards so the existing error handling path (the
AgentDeliveryError case used by the dispatch error handler) receives a typed
error rather than an opaque NPE.
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 314-318: Change the onAction handler signature to accept the full
AgentAction object (make onAction?: (payload: { action: AgentAction; ctx:
AgentActionContext }) => Awaitable<MessageContent | void>), so it matches
onMessage/onReaction shapes; update the dispatch that invokes onAction (where it
currently passes { actionId, value, ctx } or similar) to pass { action:
ctx.action!, ctx } and update any templates/tests to destructure either {
action, ctx } or { action: { actionId, value, sourceMessageId }, ctx } as
needed. Ensure imports/type references for AgentAction and AgentActionContext
are present where onAction is declared and where the handler dispatch occurs.
🪄 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: 8e48935f-04fc-4c96-91f9-2a9430f10e6e
📒 Files selected for processing (4)
packages/framework/src/handler.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.tspackages/novu/src/commands/init/templates/app-agent/ts/app/novu/agents/support-agent.tsx
ChmaraX
changed the title
…d biome Co-authored-by: Cursor <cursoragent@cursor.com>
commit: |
…er signatures Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/framework/src/handler.ts (1)
327-359: 💤 Low valueDouble cast
ctx as unknown as AgentXxxContextsignals a structural mismatch worth abstracting.Each branch repeats
ctx as unknown as AgentMessageContext | AgentActionContext | AgentReactionContext | AgentResolveContext. Theunknownstep is required becauseAgentContextImpldoesn't satisfy any of the narrowed event-specific context types directly, which both hides real type-safety regressions if those interfaces drift and adds noise.Consider a small narrowing helper (or making
AgentContextImplimplement each context interface conditionally) so the cast lives in one place and keepsunknownout of the dispatch site:♻️ Example helper
const asMessageCtx = (c: AgentContextImpl): AgentMessageContext => c as unknown as AgentMessageContext; const asActionCtx = (c: AgentContextImpl): AgentActionContext => c as unknown as AgentActionContext; // ...etcThen call sites become
ctx: asMessageCtx(ctx), etc., keeping the unsafe cast isolated and easy to audit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/framework/src/handler.ts` around lines 327 - 359, The repeated double-casts (ctx as unknown as AgentMessageContext / AgentActionContext / AgentReactionContext / AgentResolveContext) indicate a structural mismatch between AgentContextImpl and the event-specific context interfaces; create a small set of narrowing helpers (e.g., asMessageCtx, asActionCtx, asReactionCtx, asResolveCtx) that accept AgentContextImpl and return the respective AgentXxxContext by performing the cast in one place, then replace the inline casts in the dispatch (the branches using registeredAgent.handlers.onMessage/onAction/onReaction/onResolve) with calls like ctx: asMessageCtx(ctx) etc., isolating the unsafe cast and making future audits easier.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/framework/src/handler.ts`:
- Around line 326-360: The handlers dispatch currently uses non-null assertions
(ctx.message!, ctx.action!, ctx.reaction!) and may call
registeredAgent.handlers.onMessage/onAction/onReaction/onResolve with undefined
on malformed payloads; update the dispatch in the switch branches to validate
the required payload fields (e.g., ensure ctx.message exists for onMessage,
ctx.action and ctx.action.actionId for onAction, ctx.reaction for onReaction,
etc.) before calling replyIfPresent/handler, and if validation fails throw a
clear framework error (e.g., InvalidAgentEventPayload or InvalidActionError)
with a descriptive message so user handlers are never invoked with undefined.
---
Nitpick comments:
In `@packages/framework/src/handler.ts`:
- Around line 327-359: The repeated double-casts (ctx as unknown as
AgentMessageContext / AgentActionContext / AgentReactionContext /
AgentResolveContext) indicate a structural mismatch between AgentContextImpl and
the event-specific context interfaces; create a small set of narrowing helpers
(e.g., asMessageCtx, asActionCtx, asReactionCtx, asResolveCtx) that accept
AgentContextImpl and return the respective AgentXxxContext by performing the
cast in one place, then replace the inline casts in the dispatch (the branches
using registeredAgent.handlers.onMessage/onAction/onReaction/onResolve) with
calls like ctx: asMessageCtx(ctx) etc., isolating the unsafe cast and making
future audits easier.
🪄 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: dfaf47f3-21b4-4348-8aa6-6db79867fab7
📒 Files selected for processing (1)
packages/framework/src/handler.ts
| await replyIfPresent( | ||
| await registeredAgent.handlers.onMessage({ | ||
| message: ctx.message!, | ||
| ctx: ctx as unknown as AgentMessageContext, | ||
| }) | ||
| ); | ||
| break; | ||
| case AgentEventEnum.ON_ACTION: | ||
| if (registeredAgent.handlers.onAction) { | ||
| await replyIfPresent( | ||
| await registeredAgent.handlers.onAction({ | ||
| actionId: ctx.action!.actionId, | ||
| value: ctx.action!.value, | ||
| ctx: ctx as unknown as AgentActionContext, | ||
| }) | ||
| ); | ||
| } | ||
| break; | ||
| case AgentEventEnum.ON_REACTION: | ||
| if (registeredAgent.handlers.onReaction) { | ||
| await replyIfPresent( | ||
| await registeredAgent.handlers.onReaction({ | ||
| reaction: ctx.reaction!, | ||
| ctx: ctx as unknown as AgentReactionContext, | ||
| }) | ||
| ); | ||
| } | ||
| break; | ||
| case AgentEventEnum.ON_RESOLVE: | ||
| if (registeredAgent.handlers.onResolve) { | ||
| await replyIfPresent( | ||
| await registeredAgent.handlers.onResolve({ | ||
| ctx: ctx as unknown as AgentResolveContext, | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Non-null assertions on ctx.message / ctx.action / ctx.reaction may forward undefined to handlers on malformed payloads.
For each event branch, the corresponding field is forced with ! (Lines 328, 337-338, 348) and then handed to user code typed as required. If a request reaches AGENT_EVENT with event=onMessage but the body lacks message (or action.actionId / reaction for the other branches), the handler will be invoked with undefined despite the type promising otherwise, leading to confusing Cannot read properties of undefined errors inside user code rather than a clear framework-level rejection.
Consider validating presence before dispatch and throwing a descriptive error (e.g. via InvalidActionError or a dedicated agent error) so the contract between event name and payload shape is enforced at the boundary.
🛡️ Sketch of a guard at dispatch time
case AgentEventEnum.ON_MESSAGE:
+ if (!ctx.message) {
+ throw new InvalidActionError(event, AgentEventEnum);
+ }
await replyIfPresent(
await registeredAgent.handlers.onMessage({
- message: ctx.message!,
+ message: ctx.message,
ctx: ctx as unknown as AgentMessageContext,
})
);
break;Apply the analogous guard to ON_ACTION (ctx.action), ON_REACTION (ctx.reaction), and ON_RESOLVE if it has any required fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/framework/src/handler.ts` around lines 326 - 360, The handlers
dispatch currently uses non-null assertions (ctx.message!, ctx.action!,
ctx.reaction!) and may call
registeredAgent.handlers.onMessage/onAction/onReaction/onResolve with undefined
on malformed payloads; update the dispatch in the switch branches to validate
the required payload fields (e.g., ensure ctx.message exists for onMessage,
ctx.action and ctx.action.actionId for onAction, ctx.reaction for onReaction,
etc.) before calling replyIfPresent/handler, and if validation fails throw a
clear framework error (e.g., InvalidAgentEventPayload or InvalidActionError)
with a descriptive message so user handlers are never invoked with undefined.
1 participant
Summary
support-agent.tsx): removedonResolve, added inline comments explaininghistory/subscriber/metadata, personalised greeting withctx.subscriber.firstName, addedctx.trigger()example in the resolution pathagent.types.ts): added documentation to all previously undocumented public interfaces —AgentHandlers,AgentContextBaseproperties, all four context subtypes, and all data shape interfacesAgentHandlerscallbacks now receive destructured payloads instead of a singlectxargumentHandler signature change
The single biggest change. Each handler now promotes its event-specific data to the top level:
Why:
actionIdandvalueare accessed in virtually everyonActionbody — they shouldn't require two levels of nesting plus a non-null assertion. Applying the same pattern to all handlers keeps the API consistent: promoted fields = event-specific data that caused the handler to fire.ctxremains available for everything shared (subscriber, history, conversation, metadata, reply, trigger).Breaking: package is in alpha (
2.10.1-alpha.5), no deprecation wrapper added.What's deferred (follow-up tickets)
agent<TActionId>()generic soactionIdis a union instead ofstring— see NV-7523 discussionctx.metadata.setandctx.conversation.metadatactx.metadata.delete/ctx.metadata.clear(NV-7501, PR feat(shared): add ctx.metadata.delete(), clear(), get(), and current fixes NV-7501 #10971): will rebase on top once mergedTest plan
pnpm test --runinpackages/framework)ctx.subscriber,ctx.history,ctx.conversation.metadatain IDE — JSDoc appearsonMessage,onActioninAgentHandlers— per-handler docs appearMade with Cursor
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests