Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR removes sound-enabled UI/config/events and replaces legacy ACP hook dispatching with a new ProcessHooks + NewSessionHooksBridge system integrated into DeepChatAgentPresenter, adding agentId propagation and richer process usage/stop metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as DeepChatAgent
participant Bridge as NewSessionHooksBridge
participant Dispatcher as HookDispatcher
participant Notifs as HooksNotificationsService
Agent->>Agent: initSession(config { agentId? })
Agent->>Bridge: dispatch('SessionStart', { sessionId, agentId, promptPreview?, providerId?, modelId?, projectDir? })
Bridge->>Dispatcher: dispatch(event, mappedContext)
Dispatcher->>Notifs: forward(mappedContext)
Agent->>Agent: processMessage(content)
Agent->>Bridge: dispatch('UserPromptSubmit', { sessionId, messageId, promptPreview })
Bridge->>Dispatcher: dispatch('UserPromptSubmit', mappedContext)
Dispatcher->>Notifs: notifySubscribers
Agent->>Agent: executeTools(..., hooks?)
Agent->>Bridge: dispatch('PreToolUse', toolContext)
Bridge->>Dispatcher: dispatch('PreToolUse', mappedContext)
Note over Agent: tool execution (may request permission)
Agent->>Bridge: dispatch('PostToolUse' / 'PostToolUseFailure', result|error + usage)
Bridge->>Dispatcher: dispatch(event, mappedContext)
Dispatcher->>Notifs: forward(mappedContext)
Agent->>Bridge: dispatch('Stop'/'SessionEnd', { stopReason, usage, error? })
Bridge->>Dispatcher: dispatch(finalEvent, mappedContext)
Dispatcher->>Notifs: finalize session
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/presenter/agentPresenter/streaming/llmEventHandler.ts (2)
152-155: Consider removing the empty conditional block.The
isAcpProvidercheck now guards an empty block containing only a comment. If legacy hook dispatch is fully removed, this conditional and theisAcpProvidervariable declaration (line 150) could be removed to reduce dead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentPresenter/streaming/llmEventHandler.ts` around lines 152 - 155, Remove the dead code by deleting the isAcpProvider variable and the empty conditional guarded by it inside the tool_call && !shouldSkipToolCall branch (the block that only contains the legacy hook comment); if you need to preserve the explanatory note, move the comment outside the conditional or into a higher-level comment near the agent pipeline code, but do not leave an empty if (isAcpProvider) { } block.
453-457: Consider removing the empty try-finally block.The try block contains only a comment with no actual logic. The
errorByEventId.delete(eventId)cleanup could be moved directly after line 451 or consolidated with the early return cleanup at line 443. This would simplify the code structure.♻️ Proposed refactor
presenter.sessionManager.clearPendingPermission(state.conversationId) presenter.sessionManager.clearPendingQuestion(state.conversationId) } - try { - // Legacy hook dispatch disabled. New session hooks are emitted by the new agent pipeline. - } finally { - this.errorByEventId.delete(eventId) - } + // Legacy hook dispatch disabled. New session hooks are emitted by the new agent pipeline. + this.errorByEventId.delete(eventId) await this.streamUpdateScheduler.flushAll(eventId, 'final')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/agentPresenter/streaming/llmEventHandler.ts` around lines 453 - 457, Remove the empty try-finally wrapper around the legacy hook comment and move the cleanup call this.errorByEventId.delete(eventId) out of the try-finally; either place it immediately after the surrounding logic that runs when the event completes (i.e., directly after the preceding block that handles successful completion) or consolidate it with the existing early-return cleanup that also deletes the event entry, ensuring the same this.errorByEventId.delete(eventId) is invoked in both normal and early-return paths and the redundant try-finally is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/deepchatAgentPresenter/dispatch.ts`:
- Around line 445-450: The onPreToolUse hook is being called too early (before
QUESTION_TOOL_NAME handling and preCheckToolPermission), causing false
PreToolUse events and duplicate calls on retries; move the
hooks?.onPreToolUse?.({ callId: tc.id, name: tc.name, params: tc.arguments })
invocation to after the branches that short-circuit tool execution (i.e., after
you handle QUESTION_TOOL_NAME and after preCheckToolPermission()
returns/pauses), so it only fires when the tool is actually going to be executed
(and keep respondToolInteraction()'s existing PreToolUse calls from duplicating
behavior).
- Around line 603-617: The branch that handles guardToolOutput() returning
"terminal_error" currently returns early and never calls
hooks?.onPostToolUseFailure; update the terminal guard-exit path in the
guardToolOutput handling to invoke hooks?.onPostToolUseFailure with the same
payload shape used elsewhere (callId: tc.id, name: tc.name, params:
tc.arguments, error: toolMessageContent) before returning so
context-budget/offload terminal failures are emitted; reference the
guardToolOutput result handling and the existing
hooks?.onPostToolUseFailure/onPostToolUse call sites to mirror the failure
emission logic.
In `@src/main/presenter/deepchatAgentPresenter/index.ts`:
- Around line 438-449: The PreToolUse dispatch is being emitted before the
permission pause, which allows the grant path to publish
PostToolUse/PostToolUseFailure before resumeAssistantMessage() rewrites the
outcome and leaves the denied path without any terminal hook; update the flow so
that the terminal tool hooks are deferred until the permission flow settles:
stop emitting the terminal hook here (the dispatchHook('PreToolUse') block) and
instead emit PostToolUse or PostToolUseFailure only after
resumeAssistantMessage() has completed its decision (and ensure the granted:
false branch also emits an appropriate terminal hook), i.e., move or
conditionally trigger the terminal dispatches to the code that runs after
resumeAssistantMessage() so webhook consumers always receive a single final
outcome.
- Line 102: Session webhooks are getting the wrong agentId because
sessionAgentIds is only set in initSession(); when state is rebuilt via
getSessionState() you must rehydrate the agentId into sessionAgentIds (or
persist it when creating the session) before any dispatchHook() call. Update
getSessionState() (and any session-reopen path) to read the stored agentId from
the session DB/state and call sessionAgentIds.set(sessionId, agentId) so
dispatchHook() reads the correct value (and change dispatchHook() to only
fallback to 'deepchat' when no agentId exists). Ensure the same rehydration is
performed for all code paths that call dispatchHook() (including the places
currently relying on the default).
---
Nitpick comments:
In `@src/main/presenter/agentPresenter/streaming/llmEventHandler.ts`:
- Around line 152-155: Remove the dead code by deleting the isAcpProvider
variable and the empty conditional guarded by it inside the tool_call &&
!shouldSkipToolCall branch (the block that only contains the legacy hook
comment); if you need to preserve the explanatory note, move the comment outside
the conditional or into a higher-level comment near the agent pipeline code, but
do not leave an empty if (isAcpProvider) { } block.
- Around line 453-457: Remove the empty try-finally wrapper around the legacy
hook comment and move the cleanup call this.errorByEventId.delete(eventId) out
of the try-finally; either place it immediately after the surrounding logic that
runs when the event completes (i.e., directly after the preceding block that
handles successful completion) or consolidate it with the existing early-return
cleanup that also deletes the event entry, ensuring the same
this.errorByEventId.delete(eventId) is invoked in both normal and early-return
paths and the redundant try-finally is removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1a2c156-36c2-4407-ab45-535270847312
⛔ Files ignored due to path filters (2)
src/renderer/public/sounds/sfx-fc.mp3is excluded by!**/*.mp3src/renderer/public/sounds/sfx-typing.mp3is excluded by!**/*.mp3
📒 Files selected for processing (39)
src/main/events.tssrc/main/presenter/agentPresenter/acp/chatSettingsTools.tssrc/main/presenter/agentPresenter/index.tssrc/main/presenter/agentPresenter/loop/toolCallProcessor.tssrc/main/presenter/agentPresenter/streaming/llmEventHandler.tssrc/main/presenter/agentPresenter/streaming/streamGenerationHandler.tssrc/main/presenter/configPresenter/index.tssrc/main/presenter/deepchatAgentPresenter/dispatch.tssrc/main/presenter/deepchatAgentPresenter/index.tssrc/main/presenter/deepchatAgentPresenter/process.tssrc/main/presenter/deepchatAgentPresenter/types.tssrc/main/presenter/hooksNotifications/index.tssrc/main/presenter/hooksNotifications/newSessionBridge.tssrc/main/presenter/index.tssrc/main/presenter/newAgentPresenter/index.tssrc/renderer/settings/components/CommonSettings.vuesrc/renderer/src/components/chat/ChatStatusBar.vuesrc/renderer/src/events.tssrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/renderer/src/pages/ChatPage.vuesrc/renderer/src/stores/sound.tssrc/shared/types/agent-interface.d.tssrc/shared/types/chatSettings.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/types/i18n.d.tstest/main/presenter/agentPresenter/chatSettingsTools.test.tstest/main/presenter/deepchatAgentPresenter/deepchatAgentPresenter.test.tstest/main/presenter/newAgentPresenter/integration.test.ts
💤 Files with no reviewable changes (22)
- src/main/events.ts
- src/renderer/src/i18n/fa-IR/settings.json
- src/renderer/src/i18n/pt-BR/settings.json
- src/renderer/src/events.ts
- src/types/i18n.d.ts
- src/renderer/src/i18n/ru-RU/settings.json
- src/shared/types/presenters/legacy.presenters.d.ts
- src/renderer/src/stores/sound.ts
- src/renderer/src/i18n/ja-JP/settings.json
- src/renderer/src/i18n/fr-FR/settings.json
- src/main/presenter/agentPresenter/streaming/streamGenerationHandler.ts
- src/renderer/src/i18n/zh-TW/settings.json
- src/renderer/src/i18n/zh-HK/settings.json
- src/renderer/src/i18n/en-US/settings.json
- src/renderer/src/i18n/ko-KR/settings.json
- src/main/presenter/agentPresenter/index.ts
- src/renderer/src/i18n/he-IL/settings.json
- src/main/presenter/agentPresenter/loop/toolCallProcessor.ts
- src/renderer/src/i18n/da-DK/settings.json
- src/renderer/src/i18n/zh-CN/settings.json
- src/main/presenter/configPresenter/index.ts
- src/renderer/settings/components/CommonSettings.vue
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)
src/main/presenter/deepchatAgentPresenter/index.ts (1)
581-588:⚠️ Potential issue | 🟠 MajorResume-terminal failures still miss
Stop/SessionEndhooks.When
resumeAssistantMessage()returnsfalsefrom its terminal error path, this caller only emits the resolved tool failure hook. Unlike theprocessStreampath,applyProcessResultStatus()is never reached, so webhook consumers do not see the session terminate for that error branch. Mirror the terminal-hook dispatch here, or return enough detail to calldispatchTerminalHooks().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/deepchatAgentPresenter/index.ts` around lines 581 - 588, The caller of resumeAssistantMessage (in this block returning { resumed }) currently only calls emitResolvedToolHook and never triggers terminal hooks when resumeAssistantMessage returns false, so terminal errors don't reach webhook consumers; update this call site (around resumeAssistantMessage, emitResolvedToolHook) to mirror the processStream path by invoking applyProcessResultStatus/dispatchTerminalHooks for the terminal-failure branch (or change resumeAssistantMessage to return a richer result indicating terminal state and ensure dispatchTerminalHooks is called when resumed === false) so that Stop/SessionEnd hooks are emitted on terminal resume failures.
♻️ Duplicate comments (2)
src/main/presenter/deepchatAgentPresenter/index.ts (1)
170-173:⚠️ Potential issue | 🟠 MajorPersist
config.agentId; reopen only rehydrates fromnewSessionsTable.
initSession()caches the passedagentIdin memory, butgetSessionAgentId()can rebuild it only fromnewSessionsTable. If a session is created with a directagentIdand no matchingnewSessionsTablerow, hooks are correct until restart and then silently fall back to'deepchat'. Persist the value with the session or another durable store before relying on it for hook dispatch.Also applies to: 785-797
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/deepchatAgentPresenter/index.ts` around lines 170 - 173, The current initSession() only stores config.agentId in the in-memory sessionAgentIds map (via sessionAgentIds.set) so a restart loses it and getSessionAgentId() falls back to newSessionsTable or 'deepchat'; persist the passed agentId alongside the session (e.g., add/extend the sessions/newSessionsTable row or another durable store) when you call initSession()/sessionAgentIds.set so future rehydrates read the persisted agentId instead of defaulting to 'deepchat'; update the code paths that set sessionAgentIds (including the block using config.agentId?.trim()) to write the same value to the chosen durable store and adjust getSessionAgentId() to prefer that persisted field.src/main/presenter/deepchatAgentPresenter/dispatch.ts (1)
508-537:⚠️ Potential issue | 🟠 Major
PreToolUsestill fires before the runtime permission-pause path.This now skips question/pre-check pauses, but a tool that returns
rawData.requiresPermissionstill emitsonPreToolUsehere before the user approves it. That produces a false pre-use event on deny, and a duplicate one becauserespondToolInteraction()emitsPreToolUseagain on the approved retry path. Please defer the pre-use hook until after therequiresPermissionbranch decides the tool will actually proceed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/deepchatAgentPresenter/dispatch.ts` around lines 508 - 537, The onPreToolUse hook is being called before checking toolRawData.requiresPermission, causing false/duplicate pre-use events; change the flow in dispatch.ts so that hooks?.onPreToolUse?.(...) is invoked only after you've determined the tool will actually execute (i.e., after handling the requiresPermission branch and any autoGrantPermission + retry via toolPresenter.callTool), or call it only when toolRawData does not require permission; specifically move or conditionally invoke hooks?.onPreToolUse around the code that follows the permission handling (after toolPresenter.callTool retry logic and before the final execution/response path such as respondToolInteraction), referencing hooks?.onPreToolUse, toolPresenter.callTool, toolRawData.requiresPermission, and autoGrantPermission to locate and adjust the logic.
🤖 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 `@src/main/presenter/deepchatAgentPresenter/index.ts`:
- Around line 581-588: The caller of resumeAssistantMessage (in this block
returning { resumed }) currently only calls emitResolvedToolHook and never
triggers terminal hooks when resumeAssistantMessage returns false, so terminal
errors don't reach webhook consumers; update this call site (around
resumeAssistantMessage, emitResolvedToolHook) to mirror the processStream path
by invoking applyProcessResultStatus/dispatchTerminalHooks for the
terminal-failure branch (or change resumeAssistantMessage to return a richer
result indicating terminal state and ensure dispatchTerminalHooks is called when
resumed === false) so that Stop/SessionEnd hooks are emitted on terminal resume
failures.
---
Duplicate comments:
In `@src/main/presenter/deepchatAgentPresenter/dispatch.ts`:
- Around line 508-537: The onPreToolUse hook is being called before checking
toolRawData.requiresPermission, causing false/duplicate pre-use events; change
the flow in dispatch.ts so that hooks?.onPreToolUse?.(...) is invoked only after
you've determined the tool will actually execute (i.e., after handling the
requiresPermission branch and any autoGrantPermission + retry via
toolPresenter.callTool), or call it only when toolRawData does not require
permission; specifically move or conditionally invoke hooks?.onPreToolUse around
the code that follows the permission handling (after toolPresenter.callTool
retry logic and before the final execution/response path such as
respondToolInteraction), referencing hooks?.onPreToolUse,
toolPresenter.callTool, toolRawData.requiresPermission, and autoGrantPermission
to locate and adjust the logic.
In `@src/main/presenter/deepchatAgentPresenter/index.ts`:
- Around line 170-173: The current initSession() only stores config.agentId in
the in-memory sessionAgentIds map (via sessionAgentIds.set) so a restart loses
it and getSessionAgentId() falls back to newSessionsTable or 'deepchat'; persist
the passed agentId alongside the session (e.g., add/extend the
sessions/newSessionsTable row or another durable store) when you call
initSession()/sessionAgentIds.set so future rehydrates read the persisted
agentId instead of defaulting to 'deepchat'; update the code paths that set
sessionAgentIds (including the block using config.agentId?.trim()) to write the
same value to the chosen durable store and adjust getSessionAgentId() to prefer
that persisted field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2a39f13-ca93-4d71-8745-98236de79310
📒 Files selected for processing (4)
src/main/presenter/deepchatAgentPresenter/dispatch.tssrc/main/presenter/deepchatAgentPresenter/index.tstest/main/presenter/deepchatAgentPresenter/deepchatAgentPresenter.test.tstest/main/presenter/deepchatAgentPresenter/dispatch.test.ts
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 by CodeRabbit