feat(api-service,framework): agent onReaction event fixes NV-7370#10733
Merged
feat(api-service,framework): agent onReaction event fixes NV-7370#10733
Conversation
Contributor
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds emoji reaction support across the agent stack: new ON_REACTION event, inbound reaction handler, bridge reaction payloads, chat SDK listener, framework dispatch updates, context/type extensions, and corresponding unit and e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
participant ChatSDK as Chat SDK
participant API as Agent API (InboundHandler)
participant DB as Conversations DB
participant Bridge as BridgeExecutor
participant Agent as Agent Runtime
ChatSDK->>API: onReaction(event: { emoji, added, messageId, message?, thread?, user? })
API->>DB: findConversationByThread(thread.id)
DB-->>API: conversation or null
alt conversation found
API->>API: resolve subscriber (if event.user?.userId)
API->>DB: loadConversationHistory(conversation.id)
API->>Bridge: execute({ event: ON_REACTION, reaction: {...}, platformContext })
Bridge->>Agent: deliver AgentBridgeRequest (reaction payload)
Agent-->>Bridge: handler executes (may call ctx.reply())
Bridge->>API: send outbound reply request (if any)
else conversation not found
API-->>ChatSDK: no bridge execution (silent/no-op)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/agents/services/agent-inbound-handler.service.ts`:
- Around line 159-175: The code uses event.message?.author?.userId to identify
the reacting user; change it to use event.user?.userId instead (i.e., set
platformUserId = event.user?.userId) and keep the existing logic that calls
this.subscriberResolver.resolve(...) to build subscriberId (or returns null on
catch); ensure subscriberId resolution and the logger.warn still reference
agentId and handle the case where event.user is undefined by leaving
subscriberId null.
In `@packages/framework/src/handler.ts`:
- Around line 300-307: The current validation uses "event in handlerMap" which
checks inherited properties and can allow prototype keys through; change the
check to use Object.prototype.hasOwnProperty.call(handlerMap, event) (or
handlerMap.hasOwnProperty via Object.prototype) to ensure only own properties
are accepted, and if absent throw the same InvalidActionError; update the
validation around handlerMap / event / AgentEventEnum usage so only
registeredAgent.handlers entries are dispatchable and prototype properties like
"constructor" or "toString" cannot bypass the check.
In `@packages/framework/src/resources/agent/agent.resource.ts`:
- Around line 18-20: The change made the onReaction handler mandatory and throws
if missing, which is a breaking SDK change; instead keep handlers.onReaction
optional by replacing the throw with a noop fallback: if handlers?.onReaction is
missing, set handlers.onReaction = () => {} (or an async no-op) so existing
agents implementing only onMessage continue to work; update the initialization
around handlers and agentId where the check occurs (look for the onReaction
check referencing agentId and handlers) to assign the default no-op handler
rather than throwing, and reserve making it required for a major version bump.
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 106-110: AgentReaction currently drops the message identifier, so
add a messageId?: string (or string | null) field to the AgentReaction interface
and update any related types to carry it through the bridge; specifically update
BridgeReactionPayload to include messageId and modify the mapReaction() function
to copy inbound.messageId (or ctx.reaction.messageId) into the AgentReaction
output so the reaction retains the reacted message identifier even when message
is null. Ensure all call sites that construct or consume
AgentReaction/BridgeReactionPayload are updated to pass the messageId through.
- Around line 132-135: The review flags that making onReaction mandatory in the
AgentHandlers interface is a breaking change; change the AgentHandlers interface
to make onReaction optional again (onReaction?: (ctx: AgentContext) =>
Promise<void>) and audit call sites that invoke handler.onReaction to guard the
call (e.g., if (handler.onReaction) await handler.onReaction(ctx)) or invoke a
default no-op async function when undefined; ensure any code expecting
onReaction handles the absent case to maintain backward compatibility with
existing agent implementations.
🪄 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: 46359d12-5aaf-42d6-9e77-fcc6ed01ce6b
📒 Files selected for processing (11)
apps/api/src/app/agents/dtos/agent-event.enum.tsapps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/bridge-executor.service.tsapps/api/src/app/agents/services/chat-sdk.service.tspackages/framework/src/handler.tspackages/framework/src/index.tspackages/framework/src/resources/agent/agent.context.tspackages/framework/src/resources/agent/agent.resource.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.tspackages/framework/src/resources/agent/index.ts
Comment on lines
+18
to
+20
| if (!handlers?.onReaction) { | ||
| throw new Error(`agent('${agentId}') requires an onReaction handler`); | ||
| } |
Contributor
There was a problem hiding this comment.
Making onReaction mandatory is a breaking SDK change.
Existing agents that only implement onMessage will now fail type-checks and can also throw at runtime here. Keep onReaction optional with a noop fallback until the next major, or version this as a major release.
As per coding guidelines, packages/**/*: “Treat all exported symbols in packages as public API; follow semver conventions with breaking changes requiring major bumps, new exports as minor versions, and fixes as patches”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/framework/src/resources/agent/agent.resource.ts` around lines 18 -
20, The change made the onReaction handler mandatory and throws if missing,
which is a breaking SDK change; instead keep handlers.onReaction optional by
replacing the throw with a noop fallback: if handlers?.onReaction is missing,
set handlers.onReaction = () => {} (or an async no-op) so existing agents
implementing only onMessage continue to work; update the initialization around
handlers and agentId where the check occurs (look for the onReaction check
referencing agentId and handlers) to assign the default no-op handler rather
than throwing, and reserve making it required for a major version bump.
Comment on lines
+106
to
+110
| export interface AgentReaction { | ||
| emoji: { name: string }; | ||
| added: boolean; | ||
| message: AgentMessage | null; | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve messageId on AgentReaction.
InboundReactionEvent.message is optional upstream, but the API layer already has messageId. Dropping it here means ctx.reaction cannot identify the reacted message whenever reaction.message is null.
💡 Suggested API shape
export interface AgentReaction {
+ messageId: string;
emoji: { name: string };
added: boolean;
message: AgentMessage | null;
}Wire the same field through BridgeReactionPayload and mapReaction() so the identifier survives the bridge hop.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export interface AgentReaction { | |
| emoji: { name: string }; | |
| added: boolean; | |
| message: AgentMessage | null; | |
| } | |
| export interface AgentReaction { | |
| messageId: string; | |
| emoji: { name: string }; | |
| added: boolean; | |
| message: AgentMessage | null; | |
| } |
🤖 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 106 -
110, AgentReaction currently drops the message identifier, so add a messageId?:
string (or string | null) field to the AgentReaction interface and update any
related types to carry it through the bridge; specifically update
BridgeReactionPayload to include messageId and modify the mapReaction() function
to copy inbound.messageId (or ctx.reaction.messageId) into the AgentReaction
output so the reaction retains the reacted message identifier even when message
is null. Ensure all call sites that construct or consume
AgentReaction/BridgeReactionPayload are updated to pass the messageId through.
Comment on lines
132
to
135
| export interface AgentHandlers { | ||
| onMessage: (ctx: AgentContext) => Promise<void>; | ||
| onReaction: (ctx: AgentContext) => Promise<void>; | ||
| onAction?: (ctx: AgentContext) => Promise<void>; |
Contributor
There was a problem hiding this comment.
Making onReaction mandatory is a breaking SDK change.
This turns every existing agent definition into a compile/runtime break even if the agent does not care about reactions. Unless this release is a major, keep onReaction optional and no-op when it is absent.
💡 Minor-compatible alternative
export interface AgentHandlers {
onMessage: (ctx: AgentContext) => Promise<void>;
- onReaction: (ctx: AgentContext) => Promise<void>;
+ onReaction?: (ctx: AgentContext) => Promise<void>;
onAction?: (ctx: AgentContext) => Promise<void>;
onResolve?: (ctx: AgentContext) => Promise<void>;
}As per coding guidelines, packages/**/*: Treat all exported symbols in packages as public API; follow semver conventions with breaking changes requiring major bumps, new exports as minor versions, and fixes as patches.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export interface AgentHandlers { | |
| onMessage: (ctx: AgentContext) => Promise<void>; | |
| onReaction: (ctx: AgentContext) => Promise<void>; | |
| onAction?: (ctx: AgentContext) => Promise<void>; | |
| export interface AgentHandlers { | |
| onMessage: (ctx: AgentContext) => Promise<void>; | |
| onReaction?: (ctx: AgentContext) => Promise<void>; | |
| onAction?: (ctx: AgentContext) => Promise<void>; | |
| onResolve?: (ctx: AgentContext) => Promise<void>; | |
| } |
🤖 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 132 -
135, The review flags that making onReaction mandatory in the AgentHandlers
interface is a breaking change; change the AgentHandlers interface to make
onReaction optional again (onReaction?: (ctx: AgentContext) => Promise<void>)
and audit call sites that invoke handler.onReaction to guard the call (e.g., if
(handler.onReaction) await handler.onReaction(ctx)) or invoke a default no-op
async function when undefined; ensure any code expecting onReaction handles the
absent case to maintain backward compatibility with existing agent
implementations.
ChmaraX
force-pushed
the
nv/agent-on-reaction
branch
from
d6622b1 to
328fa73
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/src/app/agents/e2e/mock-agent-handler.ts (1)
131-141: Add an early guard for missingctx.reactioninonReaction.If
ctx.reactionis unexpectedly null, this currently emits a misleading reply (unknown/unreaction) instead of surfacing the wiring issue.Suggested defensive tweak
onReaction: async (ctx) => { + if (!ctx.reaction) { + console.warn(`[${ctx.event}] missing reaction payload`); + + return; + } + console.log('\n─────────────────────────────────────────'); - console.log(`[${ctx.event}] reaction: ${ctx.reaction?.emoji.name} (${ctx.reaction?.added ? 'added' : 'removed'})`); - console.log(`Reacted message: ${ctx.reaction?.message?.text ?? '(unavailable)'}`); + console.log(`[${ctx.event}] reaction: ${ctx.reaction.emoji.name} (${ctx.reaction.added ? 'added' : 'removed'})`); + console.log(`Reacted message: ${ctx.reaction.message?.text ?? '(unavailable)'}`); console.log('─────────────────────────────────────────'); - const emoji = ctx.reaction?.emoji.name ?? 'unknown'; - const added = ctx.reaction?.added ?? false; + const emoji = ctx.reaction.emoji.name; + const added = ctx.reaction.added; await ctx.reply(`Got ${added ? '' : 'un'}reaction: :${emoji}:`); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/e2e/mock-agent-handler.ts` around lines 131 - 141, onReaction currently proceeds when ctx.reaction is null and sends a misleading reply; add an early guard at the top of the onReaction handler (function onReaction) to detect missing ctx.reaction and surface the wiring problem instead of continuing: log an error via console.error or process logger and either return early (no reply) or call ctx.reply with a clear failure message indicating missing reaction context; ensure you reference ctx.reaction in that guard so the rest of the handler can safely assume it is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/api/src/app/agents/e2e/mock-agent-handler.ts`:
- Around line 131-141: onReaction currently proceeds when ctx.reaction is null
and sends a misleading reply; add an early guard at the top of the onReaction
handler (function onReaction) to detect missing ctx.reaction and surface the
wiring problem instead of continuing: log an error via console.error or process
logger and either return early (no reply) or call ctx.reply with a clear failure
message indicating missing reaction context; ensure you reference ctx.reaction
in that guard so the rest of the handler can safely assume it is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 32c16050-e0c3-4b71-83d3-f6ff6da53a5b
📒 Files selected for processing (4)
apps/api/src/app/agents/e2e/agent-webhook.e2e.tsapps/api/src/app/agents/e2e/mock-agent-handler.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/framework/src/resources/agent/agent.types.ts
- packages/framework/src/resources/agent/agent.test.ts
…nt on emoji reaction add/remove fixes NV-7370 When a user adds or removes an emoji reaction on any message in an agent conversation thread, fire an ON_REACTION bridge event so the agent handler can respond. API layer: - Add ON_REACTION to AgentEventEnum - Register chat.onReaction() in ChatSdkService - New handleReaction() on AgentInboundHandler — looks up conversation by thread, resolves subscriber, builds bridge payload (no activity persistence, no ack reaction, no typing indicator) - Extend BridgeExecutorService with reaction field and mapReaction() that reuses mapMessage() for real author data Framework SDK: - AgentHandlers.onReaction (required, same as onMessage) - AgentContext.reaction: AgentReaction | null - AgentBridgeRequest.reaction: AgentReaction | null - agent() factory validates onReaction presence - Uniform map-based handler dispatch in runAgentHandler() Made-with: Cursor
onReaction is now optional in AgentHandlers, consistent with onAction and onResolve — most agents won't need to respond to emoji reactions. - Remove onReaction validation from agent() factory - Remove no-op onReaction stubs from all non-reaction tests - Add "silently skip onReaction when no handler" framework test - Add "batch signals with reply in onReaction" framework test - Add E2E tests: subscriber resolution, null subscriber, platformContext shape, sourceMessage propagation, no activity persistence for reactions - Add onReaction handler to mock-agent-handler for manual testing Made-with: Cursor
ChmaraX
force-pushed
the
nv/agent-on-reaction
branch
from
328fa73 to
3b95369
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/framework/src/resources/agent/agent.types.ts (1)
106-110:⚠️ Potential issue | 🟠 MajorPreserve reacted message identity even when
messageis null.
AgentReactioncurrently loses the target message reference whenevermessageis absent, which makesctx.reactionambiguous for handlers.💡 Suggested type adjustment
export interface AgentReaction { + messageId: string; emoji: { name: string }; added: boolean; message: AgentMessage | null; }Then carry
messageIdthrough the bridge payload mapping so it survives API → framework hops.🤖 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 106 - 110, AgentReaction currently drops the identity of the reacted message when message is null, making ctx.reaction ambiguous; update the AgentReaction interface to include a messageId: string | null (in addition to message: AgentMessage | null) and ensure all places that construct or map reactions (the bridge payload mapping code that transforms API reaction payloads into framework AgentReaction instances) populate messageId from the incoming payload so the message identity survives API→framework hops and handlers can rely on ctx.reaction.messageId even when ctx.reaction.message is null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/framework/src/resources/agent/agent.types.ts`:
- Around line 106-110: AgentReaction currently drops the identity of the reacted
message when message is null, making ctx.reaction ambiguous; update the
AgentReaction interface to include a messageId: string | null (in addition to
message: AgentMessage | null) and ensure all places that construct or map
reactions (the bridge payload mapping code that transforms API reaction payloads
into framework AgentReaction instances) populate messageId from the incoming
payload so the message identity survives API→framework hops and handlers can
rely on ctx.reaction.messageId even when ctx.reaction.message is null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f0da1ea-4cb7-4e8f-b5d8-cc0f36624bcf
📒 Files selected for processing (4)
apps/api/src/app/agents/e2e/agent-webhook.e2e.tsapps/api/src/app/agents/e2e/mock-agent-handler.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/app/agents/e2e/agent-webhook.e2e.ts
- packages/framework/src/resources/agent/agent.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/framework/src/handler.ts (1)
300-314:⚠️ Potential issue | 🟠 MajorHarden event dispatch key validation (prototype-chain bypass).
Line 307 uses
in, which accepts inherited keys. Becauseeventis query-derived, values likeconstructorcan bypass the guard and resolve to non-agent handlers.Suggested fix
- const handlerMap: Record<string, ((ctx: AgentContextImpl) => Promise<void>) | undefined> = { + const handlerMap: Partial<Record<AgentEventEnum, (ctx: AgentContextImpl) => Promise<void>>> = { [AgentEventEnum.ON_MESSAGE]: registeredAgent.handlers.onMessage, [AgentEventEnum.ON_REACTION]: registeredAgent.handlers.onReaction, [AgentEventEnum.ON_ACTION]: registeredAgent.handlers.onAction, [AgentEventEnum.ON_RESOLVE]: registeredAgent.handlers.onResolve, }; - if (!(event in handlerMap)) { + if (!Object.prototype.hasOwnProperty.call(handlerMap, event)) { throw new InvalidActionError(event, AgentEventEnum); } - const handler = handlerMap[event]; + const handler = handlerMap[event as AgentEventEnum]; if (handler) { await handler(ctx); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/framework/src/handler.ts` around lines 300 - 314, The current guard "if (!(event in handlerMap))" is vulnerable to prototype-chain keys; replace it with a direct own-key check using Object.prototype.hasOwnProperty.call(handlerMap, event) (or alternatively check handlerMap[event] === undefined) before throwing InvalidActionError so only own keys of handlerMap (the AgentEventEnum entries) are accepted; update the code around handlerMap, the event validation, and the subsequent retrieval of handler to use this safe check.apps/api/src/app/agents/services/agent-inbound-handler.service.ts (1)
17-23:⚠️ Potential issue | 🟠 MajorResolve reactor identity from the reaction actor field, not the reacted message author.
Line 187 currently derives
platformUserIdfromevent.message?.author?.userId, which can attribute the reaction to the message author instead of the user who reacted.Proposed fix
export interface InboundReactionEvent { + user?: { userId?: string }; emoji: { name: string }; added: boolean; messageId: string; message?: Message; thread?: Thread; } @@ - const platformUserId = event.message?.author?.userId; + const platformUserId = event.user?.userId;For the npm package "chat", in the `chat.onReaction()` callback payload (`ReactionEvent`), which field identifies the user who added/removed the reaction: `event.user` or `event.message.author`?Also applies to: 187-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/agent-inbound-handler.service.ts` around lines 17 - 23, The handler currently attributes reactions to the message author; update the InboundReactionEvent shape to include the reactor field (e.g., add a user or actor property with the platform user id) and change the code that computes platformUserId to read from event.user (or event.actor) instead of event.message?.author; keep a safe fallback to event.message?.author only if event.user is undefined. Specifically, modify the InboundReactionEvent interface to include a user/actor identifier and update the platformUserId assignment (the variable referenced as platformUserId in the reaction handling code) to prefer event.user?.id or event.user?.userId (matching the external ReactionEvent payload) then fallback to event.message?.author?.userId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/agents/services/agent-inbound-handler.service.ts`:
- Around line 183-184: In the if (!conversation) guard inside
AgentInboundHandlerService (the method handling inbound messages), add a single
blank line immediately before the return statement to satisfy the repository
style rule requiring a blank line before every return; locate the block that
checks "if (!conversation) { return; }" and change it so there's an empty line
between the opening brace and the return.
---
Duplicate comments:
In `@apps/api/src/app/agents/services/agent-inbound-handler.service.ts`:
- Around line 17-23: The handler currently attributes reactions to the message
author; update the InboundReactionEvent shape to include the reactor field
(e.g., add a user or actor property with the platform user id) and change the
code that computes platformUserId to read from event.user (or event.actor)
instead of event.message?.author; keep a safe fallback to event.message?.author
only if event.user is undefined. Specifically, modify the InboundReactionEvent
interface to include a user/actor identifier and update the platformUserId
assignment (the variable referenced as platformUserId in the reaction handling
code) to prefer event.user?.id or event.user?.userId (matching the external
ReactionEvent payload) then fallback to event.message?.author?.userId.
In `@packages/framework/src/handler.ts`:
- Around line 300-314: The current guard "if (!(event in handlerMap))" is
vulnerable to prototype-chain keys; replace it with a direct own-key check using
Object.prototype.hasOwnProperty.call(handlerMap, event) (or alternatively check
handlerMap[event] === undefined) before throwing InvalidActionError so only own
keys of handlerMap (the AgentEventEnum entries) are accepted; update the code
around handlerMap, the event validation, and the subsequent retrieval of handler
to use this safe check.
🪄 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: ddfd14fb-071e-403e-9b62-381b19698c28
📒 Files selected for processing (12)
apps/api/src/app/agents/dtos/agent-event.enum.tsapps/api/src/app/agents/e2e/agent-webhook.e2e.tsapps/api/src/app/agents/e2e/mock-agent-handler.tsapps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/bridge-executor.service.tsapps/api/src/app/agents/services/chat-sdk.service.tspackages/framework/src/handler.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
✅ Files skipped from review due to trivial changes (5)
- apps/api/src/app/agents/dtos/agent-event.enum.ts
- packages/framework/src/resources/agent/index.ts
- packages/framework/src/index.ts
- apps/api/src/app/agents/e2e/agent-webhook.e2e.ts
- packages/framework/src/resources/agent/agent.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/framework/src/resources/agent/agent.context.ts
- apps/api/src/app/agents/services/chat-sdk.service.ts
- apps/api/src/app/agents/services/bridge-executor.service.ts
- packages/framework/src/resources/agent/agent.types.ts
Comment on lines
+183
to
+184
| if (!conversation) { | ||
| return; |
Contributor
There was a problem hiding this comment.
Add a blank line before return in this block.
This block violates the TS/JS style rule used in this repo.
Proposed fix
if (!conversation) {
+
return;
}As per coding guidelines: **/*.{ts,tsx,js,jsx} — “Include a blank line before every return statement”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!conversation) { | |
| return; | |
| if (!conversation) { | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/app/agents/services/agent-inbound-handler.service.ts` around
lines 183 - 184, In the if (!conversation) guard inside
AgentInboundHandlerService (the method handling inbound messages), add a single
blank line immediately before the return statement to satisfy the repository
style rule requiring a blank line before every return; locate the block that
checks "if (!conversation) { return; }" and change it so there's an empty line
between the opening brace and the return.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/src/app/agents/services/bridge-executor.service.ts (1)
238-275: GuardON_REACTIONpayload construction.
buildPayload()still allowsevent === AgentEventEnum.ON_REACTIONwithreaction == null, which would serialize an impossible bridge request and hand the framework anonReactionevent withctx.reaction === null. Add an assertion here, or tightenBridgeExecutorParamsinto an event-discriminated union.Suggested guard
private buildPayload(params: BridgeExecutorParams): AgentBridgeRequest { const { event, config, conversation, subscriber, history, message, platformContext, action, reaction } = params; const agentIdentifier = config.agentIdentifier; + + if (event === AgentEventEnum.ON_REACTION && !reaction) { + throw new Error('Missing reaction payload for onReaction bridge request'); + } const apiRootUrl = process.env.API_ROOT_URL || 'http://localhost:3000'; const replyUrl = `${apiRootUrl}/v1/agents/${agentIdentifier}/reply`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/services/bridge-executor.service.ts` around lines 238 - 275, The buildPayload function can produce an ON_REACTION event with reaction === null; update buildPayload (or BridgeExecutorParams) to prevent constructing impossible payloads by asserting reaction is present when event === AgentEventEnum.ON_REACTION: inside buildPayload, add a guard that throws or returns an error if event === AgentEventEnum.ON_REACTION && !reaction (or tighten BridgeExecutorParams into a discriminated union so ON_REACTION requires reaction) and ensure subsequent code uses reaction safely (e.g., rely on reaction after the guard and mapReaction only when non-null).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/app/agents/services/chat-sdk.service.ts`:
- Around line 299-308: The chat.onReaction handler currently forwards
event.thread and event.user without validation causing runtime errors for
partial/malformed reactions; update the chat.onReaction callback to validate
presence and shape of event.thread and event.user (and any other required fields
like event.messageId, event.emoji) and skip/return early for malformed events
instead of calling this.inboundHandler.handleReaction(agentId, config, ...);
ensure the guard logic mirrors the action-path checks (validate thread and user
exist and are well-formed) before invoking inboundHandler.handleReaction so only
complete reaction events are forwarded.
---
Nitpick comments:
In `@apps/api/src/app/agents/services/bridge-executor.service.ts`:
- Around line 238-275: The buildPayload function can produce an ON_REACTION
event with reaction === null; update buildPayload (or BridgeExecutorParams) to
prevent constructing impossible payloads by asserting reaction is present when
event === AgentEventEnum.ON_REACTION: inside buildPayload, add a guard that
throws or returns an error if event === AgentEventEnum.ON_REACTION && !reaction
(or tighten BridgeExecutorParams into a discriminated union so ON_REACTION
requires reaction) and ensure subsequent code uses reaction safely (e.g., rely
on reaction after the guard and mapReaction only when non-null).
🪄 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: 2f386f35-093c-4f9f-bb53-acde9eee6f6a
📒 Files selected for processing (6)
apps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/bridge-executor.service.tsapps/api/src/app/agents/services/chat-sdk.service.tspackages/framework/src/handler.tspackages/framework/src/resources/agent/agent.test.tspackages/framework/src/resources/agent/agent.types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/framework/src/handler.ts
- packages/framework/src/resources/agent/agent.test.ts
- apps/api/src/app/agents/services/agent-inbound-handler.service.ts
Comment on lines
+299
to
+308
| chat.onReaction(async (event: any) => { | ||
| try { | ||
| await this.inboundHandler.handleReaction(agentId, config, { | ||
| emoji: event.emoji, | ||
| added: event.added, | ||
| messageId: event.messageId, | ||
| message: event.message, | ||
| thread: event.thread, | ||
| user: event.user, | ||
| }); |
Contributor
There was a problem hiding this comment.
Skip malformed reaction events before forwarding.
Unlike the action path on Line 284, this branch forwards thread and user without checking them first. If an adapter emits a partial reaction event, this becomes a logged runtime error instead of a clean skip.
Suggested guard
chat.onReaction(async (event: any) => {
try {
+ if (!event.thread || !event.user) {
+ this.logger.warn(`[agent:${agentId}] Reaction received without complete context, skipping`);
+
+ return;
+ }
+
await this.inboundHandler.handleReaction(agentId, config, {
emoji: event.emoji,
added: event.added,
messageId: event.messageId,
message: event.message,As per coding guidelines, apps/api/**: Review with focus on security, authentication, and authorization. Check for proper error handling and input validation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| chat.onReaction(async (event: any) => { | |
| try { | |
| await this.inboundHandler.handleReaction(agentId, config, { | |
| emoji: event.emoji, | |
| added: event.added, | |
| messageId: event.messageId, | |
| message: event.message, | |
| thread: event.thread, | |
| user: event.user, | |
| }); | |
| chat.onReaction(async (event: any) => { | |
| try { | |
| if (!event.thread || !event.user) { | |
| this.logger.warn(`[agent:${agentId}] Reaction received without complete context, skipping`); | |
| return; | |
| } | |
| await this.inboundHandler.handleReaction(agentId, config, { | |
| emoji: event.emoji, | |
| added: event.added, | |
| messageId: event.messageId, | |
| message: event.message, | |
| thread: event.thread, | |
| user: event.user, | |
| }); |
🤖 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 299 - 308,
The chat.onReaction handler currently forwards event.thread and event.user
without validation causing runtime errors for partial/malformed reactions;
update the chat.onReaction callback to validate presence and shape of
event.thread and event.user (and any other required fields like event.messageId,
event.emoji) and skip/return early for malformed events instead of calling
this.inboundHandler.handleReaction(agentId, config, ...); ensure the guard logic
mirrors the action-path checks (validate thread and user exist and are
well-formed) before invoking inboundHandler.handleReaction so only complete
reaction events are forwarded.
…ity, handler safety, messageId - Use event.user (reactor) instead of event.message.author (message author) for subscriber resolution - Use hasOwnProperty check instead of `in` operator for handler dispatch to prevent prototype traversal - Add messageId to AgentReaction and BridgeReactionPayload so consumers can identify the reacted message even when message body is null Made-with: Cursor
ChmaraX
force-pushed
the
nv/agent-on-reaction
branch
from
269f567 to
1f725c4
Compare
This was referenced Apr 16, 2026
This was referenced Apr 27, 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
ON_REACTIONbridge event when a user adds or removes an emoji reaction on any message in an agent conversation threadchat.onReaction(), newhandleReaction()on inbound handler (ephemeral — no activity persistence, no ack reaction, no typing indicator), extend bridge executor withmapReaction()reusingmapMessage()for real author dataonReactionhandler onAgentHandlers,AgentContext.reaction: AgentReaction | null, uniform map-based dispatch inrunAgentHandler(), validation inagent()factoryHow it works
sequenceDiagram participant Slack as Slack (reaction_added/removed) participant ChatSDK as Chat SDK participant Inbound as AgentInboundHandler participant Bridge as BridgeExecutorService participant Framework as Framework SDK participant Handler as onReaction handler Slack->>ChatSDK: reaction event ChatSDK->>Inbound: handleReaction(event) Inbound->>Inbound: lookup conversation by thread Inbound->>Inbound: resolve subscriber Inbound->>Bridge: execute(ON_REACTION, reaction) Bridge->>Bridge: mapReaction() + mapMessage() Bridge->>Framework: POST agent-event?event=onReaction Framework->>Handler: handlers.onReaction(ctx) Handler->>Framework: ctx.reply() / ctx.resolve() / etc.Test plan
Fixes NV-7370
Made with Cursor
What changed
The PR adds ON_REACTION event handling to the agent system, enabling agents to respond when users add or remove emoji reactions on messages in conversation threads. When a reaction occurs on a message, the Chat SDK forwards the event through the inbound handler, which resolves the conversation and subscriber, builds a bridge payload with reaction metadata, and triggers the framework SDK to invoke the agent's
onReactionhandler. Reaction handling is ephemeral—no activity is persisted, no acknowledgment reaction is sent, and no typing indicator is shown.Affected areas
AgentEventEnum.ON_REACTION, createdAgentInboundHandler.handleReaction()to resolve conversations and subscribers from reaction events, registeredchat.onReaction()handler to dispatch reactions, and extendedBridgeExecutorServiceto map reaction payloads with optional message context.AgentContext.reactionandAgentBridgeRequest.reactionfields, introducedAgentReactiontype to represent reaction metadata, added optionalonReactionhandler toAgentHandlers, and refactoredrunAgentHandler()to use a map-based dispatch pattern supporting all agent event types.Key technical decisions
event.user(the reactor) rather thanevent.message.authorto correctly identify the subscriber performing the action.messageIdis explicitly included in reaction payloads so consumers can identify the reacted message even when the message body isnull.hasOwnPropertychecks to avoid prototype traversal risks when looking up handlers by event type.onReactionoptional without requiring explicit null checks in agent implementations.Testing
21 framework agent tests total (18 existing updated + 3 new) verify the feature; all framework agent tests pass. E2E tests validate that reactions correctly dispatch bridge events with proper payload mapping, skip processing for non-agent threads, and do not persist conversation activity. Mock agent handler updated to demonstrate
onReactionimplementation with emoji logging and replies. Manual verification checklist covers Slack dispatch behavior, non-agent-thread filtering, null reaction context on non-reaction events, and real author data mapping when available.