refactor(dashboard, api-service): ack flow for conversational#10788
Merged
refactor(dashboard, api-service): ack flow for conversational#10788
Conversation
Replace the previous thinkingIndicatorEnabled + reactions object with a simplified behavior model: acknowledgeOnReceived (boolean, default true) and reactionOnResolved (emoji | null). Update DTOs, schema, repository entity, update-agent usecase, and agent config resolver to read the new fields. Inbound handling now shows a typing indicator when supported or falls back to adding an "eyes" reaction for first messages on platforms without typing (SLACK and WHATSAPP); handle-agent-reply removes that fallback reaction on reply. Dashboard types and UI, e2e tests, and chat SDK comments were adjusted to reflect the new behavior fields.
Replace the negative PLATFORMS_WITHOUT_TYPING_INDICATOR with PLATFORMS_WITH_TYPING_INDICATOR and update the set to include Slack and Teams (leaving WhatsApp out). Update agent inbound logic to check the positive set when deciding to start a typing indicator, and adjust the API DTO and dashboard tooltip text to reflect the corrected platform behavior and wording.
✅ 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:
📝 WalkthroughWalkthroughReplaces nested reaction and typing-indicator behavior with a simplified behavior shape: boolean Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant InboundHandler as AgentInboundHandler
participant Platform
participant DB as Channel/Cache
Client->>InboundHandler: Incoming first message (message, platform)
InboundHandler->>DB: Read cached ResolvedAgentConfig
DB-->>InboundHandler: ResolvedAgentConfig (acknowledgeOnReceived, reactionOnResolved, platform)
alt Platform supports typing (PLATFORMS_WITH_TYPING_INDICATOR.has(platform))
InboundHandler->>Platform: Show typing indicator
else Platform does not support typing
opt acknowledgeOnReceived
InboundHandler->>Platform: Add reaction "eyes"
InboundHandler->>DB: Persist firstPlatformMessageId
end
end
Platform-->>InboundHandler: Ack/Reaction created
InboundHandler-->>Client: Continue processing inbound event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
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 |
Replace static emoji button with an interactive ResolvedEmojiPicker and wire up behavior updates via a react-query mutation. AgentBehaviorSection now accepts an agent prop, reads current behavior values, and uses updateAgent (with requireEnvironment) to persist changes; on success it invalidates the agent detail query and on error shows a toast. UI changes include a Popover picker with a Disabled option, disabled state while mutation is pending, and updated imports. Also pass the agent prop from AgentConnectedOverview.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/app/agents/dtos/agent-behavior.dto.ts (1)
18-27:⚠️ Potential issue | 🟡 MinorMark
reactionOnResolvedas nullable in Swagger.The DTO accepts
nullto disable resolved reactions, but the OpenAPI metadata only marks the field optional. Generated clients may not knownullis valid.Proposed fix
`@ApiPropertyOptional`({ description: 'Cross-platform emoji name for resolved conversations (e.g. "check", "star"). ' + 'Set to null to disable. Default: "check"', default: 'check', + nullable: true, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/dtos/agent-behavior.dto.ts` around lines 18 - 27, The OpenAPI metadata for the DTO property reactionOnResolved is missing nullable=true, so update the `@ApiPropertyOptional` decorator on the reactionOnResolved property to include nullable: true (e.g. `@ApiPropertyOptional`({ ..., default: 'check', nullable: true })) so generated clients know null is an accepted value while keeping the existing `@IsOptional/`@ValidateIf/@IsWellKnownEmoji usage intact.apps/dashboard/src/components/agents/agent-behavior-section.tsx (1)
23-25:⚠️ Potential issue | 🟠 MajorInclude the environment ID in the emoji query key.
The
listAgentEmoji()function requirescurrentEnvironmentto fetch the correct emoji list per environment, butgetAgentEmojiQueryKey()is static. When switching environments, React Query returns the cached data from the previous environment instead of refetching, violating React Query's requirement that query keys include all dependencies that affect the returned data.Proposed fix
- queryKey: getAgentEmojiQueryKey(), + queryKey: getAgentEmojiQueryKey(currentEnvironment?._id),And update the API helper:
export function getAgentEmojiQueryKey(environmentId: string | undefined) { return [AGENT_EMOJI_QUERY_KEY, environmentId] as const; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/agent-behavior-section.tsx` around lines 23 - 25, The query key must include the environment dependency so React Query refetches when environment changes: update the call in agent-behavior-section to pass the environment into the key (e.g., useQuery({ queryKey: getAgentEmojiQueryKey(currentEnvironment!), queryFn: ({ signal }) => listAgentEmoji(currentEnvironment!, signal) })) and change the helper signature getAgentEmojiQueryKey(environmentId) to return [AGENT_EMOJI_QUERY_KEY, environmentId] as const; ensure the helper and all callers accept and forward the environmentId parameter.
🧹 Nitpick comments (1)
apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts (1)
343-359: Avoid hardcoding'eyes'— reuse the shared fallback-emoji constant.The inbound handler adds this reaction via
ACKNOWLEDGE_FALLBACK_EMOJI(seeagent-inbound-handler.service.ts), but here the removal references the literal'eyes'. If the constant is ever changed, acks will silently stop being cleared on reply. Keep add/remove in lock-step by importing the same constant.♻️ Proposed change
-import { AgentConfigResolver, ResolvedAgentConfig } from '../../services/agent-config-resolver.service'; +import { ACKNOWLEDGE_FALLBACK_EMOJI } from '../../services/agent-inbound-handler.service'; +import { AgentConfigResolver, ResolvedAgentConfig } from '../../services/agent-config-resolver.service';await this.chatSdkService.removeReaction( conversation._agentId, config.integrationIdentifier, channel.platform, channel.platformThreadId, firstMessageId, - 'eyes' + ACKNOWLEDGE_FALLBACK_EMOJI );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts` around lines 343 - 359, The removeAckReaction method currently hardcodes the emoji string 'eyes' which can drift from the inbound handler; instead import and use the shared ACKNOWLEDGE_FALLBACK_EMOJI constant (the same one used in the inbound handler) and pass that to chatSdkService.removeReaction; update the imports at the top of the file to include ACKNOWLEDGE_FALLBACK_EMOJI and replace the literal in removeAckReaction so add/remove remain in lock-step.
🤖 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 124-125: The call to thread.startTyping inside the supportsTyping
branch is an external platform call and must be best-effort so it cannot abort
the inbound handler; change the code around supportsTyping/thread.startTyping to
make the typing ack non-blocking and swallow transient errors (e.g. call
thread.startTyping('Thinking...') without awaiting it and attach a .catch(...)
that logs a warning via the service logger, or wrap the await in try/catch and
log the error) so failures do not prevent the rest of
agent-inbound-handler.service (the handler that executes the bridge and
processes the message) from running.
- Around line 126-145: The two side-effect calls in the isFirstMessage branch
(thread.createSentMessageFromMessage(...).addReaction(ACKNOWLEDGE_FALLBACK_EMOJI)
and conversationRepository.setFirstPlatformMessageId(...)) must be awaited
before continuing to bridge execution; change the fire-and-forget pattern to
await both results (e.g., capture the Promise from
thread.createSentMessageFromMessage(...).addReaction(...) and the Promise from
conversationRepository.setFirstPlatformMessageId(...) and use Promise.all or
sequential awaits inside a try/catch), log any errors with this.logger.warn but
ensure the awaits complete so the ack reaction and firstPlatformMessageId are
persisted before the bridge reply runs.
In `@apps/dashboard/src/components/agents/agent-behavior-section.tsx`:
- Around line 169-171: The label and tooltip for the ToggleRow controlling
resolved-reaction are inconsistent (label: "React to the final message when a
conversation is resolved" vs tooltip: "Add an emoji reaction to the first
message in the thread when the conversation is resolved."); update one so both
describe the same target message. Locate the ToggleRow instance in
agent-behavior-section.tsx (the component rendering label and tooltip) and
change either the label text or the tooltip text so they match—e.g., make both
say "final message" or both say "first message" depending on intended
behavior—and ensure the wording clearly states when the reaction is applied.
- Around line 70-83: The trigger currently shows "Off" when unicodeMap lacks an
entry for currentEmoji because displayUnicode falls back to '' — change the
logic so displayUnicode is undefined when the emoji key is missing (e.g., set
displayUnicode = currentEmoji ? unicodeMap.get(currentEmoji) : null) and update
the render branch in the PopoverTrigger/button to: show the unicode when
displayUnicode is a string, show a loading/placeholder (or nothing) when
currentEmoji is present but displayUnicode is undefined, and only render "Off"
when currentEmoji is null; update references to displayUnicode, currentEmoji and
unicodeMap in the agent behavior component accordingly.
In `@libs/dal/src/repositories/agent/agent.schema.ts`:
- Around line 21-24: The schema change replaced old behavior fields and
introduced behavior.acknowledgeOnReceived and behavior.reactionOnResolved but
left legacy fields (behavior.thinkingIndicatorEnabled and
behavior.reactions.onMessageReceived/onResolved) and used Schema.Types.Mixed for
reactionOnResolved; update the schema to use Schema.Types.String (allowing null)
for reactionOnResolved, and add a data migration script to unset the legacy
fields and optionally backfill the new fields from the old values (map
thinkingIndicatorEnabled -> acknowledgeOnReceived and
reactions.{onMessageReceived,onResolved} -> reactionOnResolved as appropriate)
so no orphaned fields persist and Mongoose change-tracking works correctly for
nested updates.
---
Outside diff comments:
In `@apps/api/src/app/agents/dtos/agent-behavior.dto.ts`:
- Around line 18-27: The OpenAPI metadata for the DTO property
reactionOnResolved is missing nullable=true, so update the `@ApiPropertyOptional`
decorator on the reactionOnResolved property to include nullable: true (e.g.
`@ApiPropertyOptional`({ ..., default: 'check', nullable: true })) so generated
clients know null is an accepted value while keeping the existing
`@IsOptional/`@ValidateIf/@IsWellKnownEmoji usage intact.
In `@apps/dashboard/src/components/agents/agent-behavior-section.tsx`:
- Around line 23-25: The query key must include the environment dependency so
React Query refetches when environment changes: update the call in
agent-behavior-section to pass the environment into the key (e.g., useQuery({
queryKey: getAgentEmojiQueryKey(currentEnvironment!), queryFn: ({ signal }) =>
listAgentEmoji(currentEnvironment!, signal) })) and change the helper signature
getAgentEmojiQueryKey(environmentId) to return [AGENT_EMOJI_QUERY_KEY,
environmentId] as const; ensure the helper and all callers accept and forward
the environmentId parameter.
---
Nitpick comments:
In
`@apps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.ts`:
- Around line 343-359: The removeAckReaction method currently hardcodes the
emoji string 'eyes' which can drift from the inbound handler; instead import and
use the shared ACKNOWLEDGE_FALLBACK_EMOJI constant (the same one used in the
inbound handler) and pass that to chatSdkService.removeReaction; update the
imports at the top of the file to include ACKNOWLEDGE_FALLBACK_EMOJI and replace
the literal in removeAckReaction so add/remove remain in lock-step.
🪄 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: 685dec7f-365c-4f3a-9bf1-e2dfdfc2c5c2
📒 Files selected for processing (13)
apps/api/src/app/agents/dtos/agent-behavior.dto.tsapps/api/src/app/agents/dtos/agent-platform.enum.tsapps/api/src/app/agents/e2e/agents.e2e.tsapps/api/src/app/agents/services/agent-config-resolver.service.tsapps/api/src/app/agents/services/agent-inbound-handler.service.tsapps/api/src/app/agents/services/chat-sdk.service.tsapps/api/src/app/agents/usecases/handle-agent-reply/handle-agent-reply.usecase.tsapps/api/src/app/agents/usecases/update-agent/update-agent.usecase.tsapps/dashboard/src/api/agents.tsapps/dashboard/src/components/agents/agent-behavior-section.tsxapps/dashboard/src/components/agents/agent-connected-overview.tsxlibs/dal/src/repositories/agent/agent.entity.tslibs/dal/src/repositories/agent/agent.schema.ts
Comment on lines
+124
to
+125
| if (supportsTyping) { | ||
| await thread.startTyping('Thinking...'); |
Contributor
There was a problem hiding this comment.
Keep typing acknowledgements best-effort.
thread.startTyping() is an external platform call. If Slack/Teams returns a transient error, this aborts the inbound handler before the bridge executes, so the agent can miss the message.
Proposed fix
if (supportsTyping) {
- await thread.startTyping('Thinking...');
+ await thread.startTyping('Thinking...').catch((err) => {
+ this.logger.warn(err, `[agent:${agentId}] Failed to start typing indicator`);
+ });
} else if (isFirstMessage && message.id) {📝 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 (supportsTyping) { | |
| await thread.startTyping('Thinking...'); | |
| if (supportsTyping) { | |
| await thread.startTyping('Thinking...').catch((err) => { | |
| this.logger.warn(err, `[agent:${agentId}] Failed to start typing indicator`); | |
| }); |
🤖 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 124 - 125, The call to thread.startTyping inside the supportsTyping branch
is an external platform call and must be best-effort so it cannot abort the
inbound handler; change the code around supportsTyping/thread.startTyping to
make the typing ack non-blocking and swallow transient errors (e.g. call
thread.startTyping('Thinking...') without awaiting it and attach a .catch(...)
that logs a warning via the service logger, or wrap the await in try/catch and
log the error) so failures do not prevent the rest of
agent-inbound-handler.service (the handler that executes the bridge and
processes the message) from running.
Comment on lines
21
to
24
| behavior: { | ||
| thinkingIndicatorEnabled: Schema.Types.Boolean, | ||
| reactions: { | ||
| onMessageReceived: Schema.Types.Mixed, | ||
| onResolved: Schema.Types.Mixed, | ||
| }, | ||
| acknowledgeOnReceived: Schema.Types.Boolean, | ||
| reactionOnResolved: Schema.Types.Mixed, | ||
| }, |
Contributor
There was a problem hiding this comment.
Schema rename needs a data migration; consider String over Mixed for reactionOnResolved.
Two concerns on this behavior subdocument:
-
Migration for stale fields. This rename silently leaves existing agent documents with orphaned
behavior.thinkingIndicatorEnabledandbehavior.reactions.{onMessageReceived,onResolved}fields. They won't be read anymore, but they'll persist indefinitely and may leak through if any code ever does a$set: { behavior: {...} }wholesale replace, or confuse future debugging. Plan a migration to$unsetthe old fields (and optionally backfillacknowledgeOnReceived/reactionOnResolvedfrom the old values to preserve user intent). -
Schema.Types.MixedforreactionOnResolved. The entity types it asstring | null;Schema.Types.Stringaccepts null and gives you field-level casting/validation.Mixedopts out of that and also disables Mongoose change-tracking on nested mutations, which can bite later.
♻️ Proposed type tightening
behavior: {
acknowledgeOnReceived: Schema.Types.Boolean,
- reactionOnResolved: Schema.Types.Mixed,
+ reactionOnResolved: Schema.Types.String,
},As per coding guidelines: "Flag any schema changes that need migrations."
📝 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
| behavior: { | |
| thinkingIndicatorEnabled: Schema.Types.Boolean, | |
| reactions: { | |
| onMessageReceived: Schema.Types.Mixed, | |
| onResolved: Schema.Types.Mixed, | |
| }, | |
| acknowledgeOnReceived: Schema.Types.Boolean, | |
| reactionOnResolved: Schema.Types.Mixed, | |
| }, | |
| behavior: { | |
| acknowledgeOnReceived: Schema.Types.Boolean, | |
| reactionOnResolved: Schema.Types.String, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/dal/src/repositories/agent/agent.schema.ts` around lines 21 - 24, The
schema change replaced old behavior fields and introduced
behavior.acknowledgeOnReceived and behavior.reactionOnResolved but left legacy
fields (behavior.thinkingIndicatorEnabled and
behavior.reactions.onMessageReceived/onResolved) and used Schema.Types.Mixed for
reactionOnResolved; update the schema to use Schema.Types.String (allowing null)
for reactionOnResolved, and add a data migration script to unset the legacy
fields and optionally backfill the new fields from the old values (map
thinkingIndicatorEnabled -> acknowledgeOnReceived and
reactions.{onMessageReceived,onResolved} -> reactionOnResolved as appropriate)
so no orphaned fields persist and Mongoose change-tracking works correctly for
nested updates.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/dashboard/src/components/agents/agent-behavior-section.tsx (1)
70-83:⚠️ Potential issue | 🟡 MinorTrigger still flashes "Off" before
emojiListloads.The default
reactionOnResolvedis'check', but untilemojiListresolves,unicodeMap.get('check')returnsundefined, the?? ''collapses to an empty string, and the falsy check on line 79 then renders theOffbranch — even though reactions are enabled. Distinguish "unknown/loading" from "explicitly disabled" (null).Proposed fix
- const displayUnicode = currentEmoji ? (unicodeMap.get(currentEmoji) ?? '') : null; + const resolvedUnicode = currentEmoji ? unicodeMap.get(currentEmoji) : null; @@ - {displayUnicode ? ( - <span className="text-label-sm leading-5">{displayUnicode}</span> - ) : ( - <span className="text-text-soft text-label-sm leading-5">Off</span> - )} + {currentEmoji === null ? ( + <span className="text-text-soft text-label-sm leading-5">Off</span> + ) : ( + <span className="text-label-sm leading-5">{resolvedUnicode ?? ''}</span> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/agent-behavior-section.tsx` around lines 70 - 83, The UI shows "Off" while emoji data is loading because displayUnicode collapses missing entries to '' — change the display logic so there are three states: null = explicitly disabled (show "Off"), undefined = loading/unknown (show a loading placeholder or nothing), and string = found emoji (show it). Concretely, compute displayUnicode from currentEmoji and unicodeMap (e.g., if currentEmoji == null => null; else if unicodeMap.has(currentEmoji) => unicodeMap.get(currentEmoji) ?? '' ; else => undefined) and update the JSX branch in AgentBehaviorSection (the button render that currently checks displayUnicode) to test displayUnicode === null for the "Off" branch, displayUnicode === undefined for a loading state, and otherwise render the emoji string.
🧹 Nitpick comments (1)
apps/dashboard/src/components/agents/agent-behavior-section.tsx (1)
136-180: SharedisPendingcross-disables unrelated controls.A single
useMutationis shared by both the acknowledgeSwitch(line 164) and theResolvedEmojiPicker(line 177). Toggling one control disables the other for the duration of the in-flight request, which is surprising for independent settings. Consider tracking pending state per field (e.g., thevariablesfromuseMutation, or two separate mutations) so each control only disables itself.Sketch
- const { mutate, isPending } = useMutation({ ... }); + const mutation = useMutation({ ... }); + const pendingAck = mutation.isPending && mutation.variables?.acknowledgeOnReceived !== undefined; + const pendingReaction = mutation.isPending && mutation.variables?.reactionOnResolved !== undefined; ... - disabled={isPending} + disabled={pendingAck} ... - disabled={isPending} + disabled={pendingReaction}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/agent-behavior-section.tsx` around lines 136 - 180, The current single useMutation (symbols: useMutation, mutate, isPending) is shared by both the acknowledge Switch and the ResolvedEmojiPicker so toggling one disables the other; split this into two independent mutations (e.g., create mutateAcknowledge and isPendingAcknowledge for acknowledgeOnReceived, and mutateReaction and isPendingReaction for reactionOnResolved) or otherwise derive per-field pending state from the mutation variables so each control (Switch and ResolvedEmojiPicker) only disables itself when its own mutation is in-flight; update the mutationFn calls to pass the correct body ({ acknowledgeOnReceived } vs { reactionOnResolved }) and wire each control to its corresponding mutate and isPending flag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/dashboard/src/components/agents/agent-behavior-section.tsx`:
- Around line 70-83: The UI shows "Off" while emoji data is loading because
displayUnicode collapses missing entries to '' — change the display logic so
there are three states: null = explicitly disabled (show "Off"), undefined =
loading/unknown (show a loading placeholder or nothing), and string = found
emoji (show it). Concretely, compute displayUnicode from currentEmoji and
unicodeMap (e.g., if currentEmoji == null => null; else if
unicodeMap.has(currentEmoji) => unicodeMap.get(currentEmoji) ?? '' ; else =>
undefined) and update the JSX branch in AgentBehaviorSection (the button render
that currently checks displayUnicode) to test displayUnicode === null for the
"Off" branch, displayUnicode === undefined for a loading state, and otherwise
render the emoji string.
---
Nitpick comments:
In `@apps/dashboard/src/components/agents/agent-behavior-section.tsx`:
- Around line 136-180: The current single useMutation (symbols: useMutation,
mutate, isPending) is shared by both the acknowledge Switch and the
ResolvedEmojiPicker so toggling one disables the other; split this into two
independent mutations (e.g., create mutateAcknowledge and isPendingAcknowledge
for acknowledgeOnReceived, and mutateReaction and isPendingReaction for
reactionOnResolved) or otherwise derive per-field pending state from the
mutation variables so each control (Switch and ResolvedEmojiPicker) only
disables itself when its own mutation is in-flight; update the mutationFn calls
to pass the correct body ({ acknowledgeOnReceived } vs { reactionOnResolved })
and wire each control to its corresponding mutate and isPending flag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1461cf2a-b81d-4622-80e0-88a93c764f83
📒 Files selected for processing (1)
apps/dashboard/src/components/agents/agent-behavior-section.tsx
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)
apps/dashboard/src/components/agents/whatsapp-setup-guide.tsx (1)
59-96:⚠️ Potential issue | 🟠 MajorReset credential progress when the setup target changes.
isCredentialsSavedis local state, but onlyisConnectedresets whenintegrationIdchanges. After saving credentials for one integration, switching to another can mark “Save credentials in Novu” as completed and jump straight to the webhook step.🐛 Proposed fix
// biome-ignore lint/correctness/useExhaustiveDependencies: reset when the watched integration changes useEffect(() => { setIsConnected(false); + setIsCredentialsSaved(false); + setIsCredentialsSidebarOpen(false); - }, [integrationId]); + }, [integrationId, agent._id]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/whatsapp-setup-guide.tsx` around lines 59 - 96, The effect that resets progress when the watched integration changes currently only calls setIsConnected(false); also reset the local credential state so saved credentials don't carry over to a different integration: update the useEffect that watches integrationId (the block with the biome-ignore comment) to call setIsCredentialsSaved(false) alongside setIsConnected(false) so the component recalculates firstIncompleteStep correctly when integrationId changes; ensure you reference the existing isCredentialsSaved state setter and keep integrationId in the dependency array.
🤖 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 `@apps/dashboard/src/components/agents/whatsapp-setup-guide.tsx`:
- Around line 59-96: The effect that resets progress when the watched
integration changes currently only calls setIsConnected(false); also reset the
local credential state so saved credentials don't carry over to a different
integration: update the useEffect that watches integrationId (the block with the
biome-ignore comment) to call setIsCredentialsSaved(false) alongside
setIsConnected(false) so the component recalculates firstIncompleteStep
correctly when integrationId changes; ensure you reference the existing
isCredentialsSaved state setter and keep integrationId in the dependency array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2cdb8496-457f-4785-8cf6-163d1cca3288
📒 Files selected for processing (1)
apps/dashboard/src/components/agents/whatsapp-setup-guide.tsx
UI: Adjust ResolvedEmojiPicker logic to distinguish an explicit "off" (currentEmoji === null) from unmapped emoji values. displayUnicode now returns undefined when no mapping exists; the picker shows "Off" for null, the mapped unicode when available, and falls back to the raw emoji string otherwise. Also update the toggle label and tooltip to clarify it targets the first message. Schema: Change agent.behavior.reactionOnResolved from Mixed to String to enforce string values for stored reactions. Note: existing records using non-string values may require migration or cleanup.
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)
apps/dashboard/src/components/agents/slack-setup-guide.tsx (1)
148-151:⚠️ Potential issue | 🟡 MinorMake the Slack completion callback idempotent.
Line 269 adds a second source for
handleSlackWorkspaceConnected, whileListeningStatusstill calls the same handler on Line 282. This can invokeonStepsCompletedtwice for one install flow.🐛 Proposed fix
-import { useCallback, useEffect, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; @@ const [isCredentialsSidebarOpen, setIsCredentialsSidebarOpen] = useState(false); const [isCredentialsSaved, setIsCredentialsSaved] = useState(false); const [isSlackWorkspaceConnected, setIsSlackWorkspaceConnected] = useState(false); + const hasReportedSlackWorkspaceConnectedRef = useRef(false); @@ useEffect(() => { setIsSlackWorkspaceConnected(false); + hasReportedSlackWorkspaceConnectedRef.current = false; }, [integrationId]); const handleSlackWorkspaceConnected = useCallback(() => { setIsSlackWorkspaceConnected(true); + + if (hasReportedSlackWorkspaceConnectedRef.current) { + return; + } + + hasReportedSlackWorkspaceConnectedRef.current = true; onStepsCompleted?.(); }, [onStepsCompleted]);Also applies to: 263-269, 278-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/agents/slack-setup-guide.tsx` around lines 148 - 151, The handler handleSlackWorkspaceConnected currently sets state and calls onStepsCompleted and can be invoked twice; make it idempotent by returning early if the workspace is already marked connected (or if a ref/flag like isSlackWorkspaceConnectedRef is true) so onStepsCompleted is only called once; update handleSlackWorkspaceConnected (and any other callers that may invoke it) to check the flag before calling setIsSlackWorkspaceConnected and before invoking onStepsCompleted, or update the invocation to guard with the same flag to ensure a single invocation per install flow.
🤖 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 `@apps/dashboard/src/components/agents/slack-setup-guide.tsx`:
- Around line 148-151: The handler handleSlackWorkspaceConnected currently sets
state and calls onStepsCompleted and can be invoked twice; make it idempotent by
returning early if the workspace is already marked connected (or if a ref/flag
like isSlackWorkspaceConnectedRef is true) so onStepsCompleted is only called
once; update handleSlackWorkspaceConnected (and any other callers that may
invoke it) to check the flag before calling setIsSlackWorkspaceConnected and
before invoking onStepsCompleted, or update the invocation to guard with the
same flag to ensure a single invocation per install flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e101227a-1d2d-4081-a45a-561a89997fec
📒 Files selected for processing (1)
apps/dashboard/src/components/agents/slack-setup-guide.tsx
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.
What changed? Why was the change needed?
What changed
Refactors conversational acknowledgement: replaces the typing/reaction model (thinkingIndicatorEnabled + nested reactions) with two flat behavior fields—acknowledgeOnReceived (boolean) that controls whether the agent shows typing or sends an acknowledgement on first inbound messages, and reactionOnResolved (emoji | null) for a post-resolution reaction. Platform logic was inverted to an explicit whitelist (PLATFORMS_WITH_TYPING_INDICATOR) so Slack/Teams show typing and other platforms fall back to a single 'eyes' emoji acknowledgement. The dashboard exposes controlled UI to toggle acknowledgement and pick/disable the resolved-reaction emoji.
Affected areas
api: DTOs, config resolver, inbound handler, chat SDK comments, usecases (update-agent, handle-agent-reply) and services now use acknowledgeOnReceived and reactionOnResolved; platform constant renamed/inverted; first-message flow prefers typing where supported, otherwise adds an 'eyes' reaction.
dashboard: Adds AgentBehavior types, wires AgentBehaviorSection to accept agent props, introduces ResolvedEmojiPicker and behavior toggles that call updateAgent mutations with query invalidation and error toasts.
dal: Agent entity and Mongoose schema updated—removed thinkingIndicatorEnabled and nested reactions, added acknowledgeOnReceived (Boolean) and reactionOnResolved (String | null).
tests: Agent e2e tests updated to verify PATCH/GET behavior for acknowledgeOnReceived and reactionOnResolved.
Key technical decisions
Testing
E2E tests were updated to cover the new behavior fields (enable/disable ack and set/clear resolved emoji); UI changes rely on these tests and should be manually verified for picker behavior and mutation UX.
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer