Conversation
Contributor
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR converts YoBrowser from a multi-window model to a session-scoped single-instance sidepanel, reduces tools to Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent/Tool Loop
participant Manager as AgentToolManager
participant Handler as YoBrowserToolHandler
participant Presenter as YoBrowserPresenter
participant State as SessionBrowserState
participant Renderer as BrowserPanel
Agent->>Manager: callTool(toolName, args, conversationId)
Manager->>Manager: is toolName in YO_BROWSER_TOOL_NAMES?
Manager->>Handler: route to YoBrowserToolHandler
Handler->>Presenter: derive sessionId from conversationId
alt load_url
Handler->>Presenter: loadUrl(sessionId, url)
Presenter->>State: ensureSessionBrowserState(sessionId)
Presenter->>State: create/attach view, await host readiness
Presenter->>State: navigate to URL
Presenter-->>Handler: YoBrowserStatus
else get_browser_status
Handler->>Presenter: getBrowserStatus(sessionId)
Presenter-->>Handler: YoBrowserStatus
else cdp_send
Handler->>Presenter: getBrowserPage(sessionId)
Presenter->>State: return page
Handler->>Presenter: sendCdpCommand(sessionId, method, params)
Presenter-->>Handler: CDP response
end
Handler-->>Agent: return stringified result
Presenter->>Renderer: emit WINDOW_CREATED / WINDOW_UPDATED with sessionId
Renderer->>Renderer: update UI for provided sessionId
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly Related PRs
Poem
🚥 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. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4afc8747c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| throw new Error('url is required') | ||
| } | ||
|
|
||
| const hostWindowId = this.resolveHostWindowId() |
There was a problem hiding this comment.
loadUrl picks the host via resolveHostWindowId() (focused/first window) instead of the window bound to sessionId. In multi-window usage, if session A is executing while window B is focused, the open event is emitted with B’s windowId; ChatSidePanel.handleBrowserOpenRequested requires both sessionId and windowId to match, so neither window attaches the browser and waitForSessionHostReady times out. This makes load_url (and subsequent cdp_send) fail for background sessions even though the session is valid.
Useful? React with 👍 / 👎.
![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: 3
🧹 Nitpick comments (5)
src/main/presenter/windowPresenter/index.ts (1)
521-527:createBrowserWindownow behaves like chat-window creation; consider deprecating this API name.This now always creates a chat shell, so the method name can mislead callers and future maintenance. A deprecation annotation (or redirect to
createAppWindow) would make intent explicit.♻️ Minimal clarity refactor
+ /** + * `@deprecated` Standalone browser shell was removed. Use createAppWindow(). + */ public async createBrowserWindow(options?: { x?: number; y?: number }): Promise<number | null> { - return await this.createManagedWindow({ - windowType: 'chat', - x: options?.x, - y: options?.y - }) + return await this.createAppWindow({ + initialRoute: 'chat', + x: options?.x, + y: options?.y + }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/windowPresenter/index.ts` around lines 521 - 527, The public method createBrowserWindow currently always delegates to createManagedWindow with windowType:'chat', which makes the name misleading; mark createBrowserWindow as deprecated and either redirect callers to createAppWindow or change createBrowserWindow to call createAppWindow (or proxy to it) and add a JSDoc `@deprecated` note plus console/process deprecation warning; update callers to use createAppWindow and keep createBrowserWindow as a thin shim that forwards to createAppWindow (or to createManagedWindow with explicit comment) so intent is explicit.test/main/presenter/newAgentPresenter/newAgentPresenter.test.ts (1)
1004-1045: Add one regression test for legacy disabled-tool IDs.These assertions cover canonical
cdp_send, but not the newyo_browser_cdp_send -> cdp_sendnormalization path. A focused case would lock migration safety.Proposed test addition
+ it('normalizes legacy YoBrowser disabled tool ids', async () => { + sqlitePresenter.newSessionsTable.get.mockReturnValue({ + id: 's1', + agent_id: 'deepchat', + title: 'Test', + project_dir: null, + is_pinned: 0, + created_at: 1000, + updated_at: 1000 + }) + + const disabledTools = await presenter.updateSessionDisabledAgentTools('s1', [ + 'yo_browser_cdp_send', + 'exec' + ]) + + expect(disabledTools).toEqual(['cdp_send', 'exec']) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/newAgentPresenter/newAgentPresenter.test.ts` around lines 1004 - 1045, Add a regression test that verifies legacy tool IDs are normalized to the canonical ID (yo_browser_cdp_send -> cdp_send) when updating or reading disabled agent tools: in the existing test block for disabled agent tools, mock sqlitePresenter.newSessionsTable.get to return the session and then call presenter.updateSessionDisabledAgentTools with an array containing the legacy ID "yo_browser_cdp_send" (and possibly duplicates), assert the returned disabled list contains "cdp_send" (not the legacy ID) and that sqlitePresenter.newSessionsTable.updateDisabledAgentTools was called with the normalized array; also add a complementary call to presenter.getSessionDisabledAgentTools (or mock getDisabledAgentTools to include legacy IDs) to assert that getSessionDisabledAgentTools performs the same normalization path.src/main/presenter/browser/YoBrowserPresenter.ts (1)
540-564: Consider cleaning up host window listeners when no sessions are attached.
detachOtherSessionBrowsersanddetachFromWindowproperly detach sessions from windows, butdetachHostWindowListenersis only called in theclosedlistener callback. If all sessions are detached from a window (but the window remains open), the listeners persist.This is likely acceptable since the listeners are lightweight and will be cleaned up when the window closes, but for completeness:
♻️ Optional cleanup enhancement
private detachFromWindow(state: SessionBrowserState, hostWindowId: number): void { const window = BrowserWindow.fromId(hostWindowId) if (window && !window.isDestroyed()) { try { window.contentView.removeChildView(state.view) } catch { // Ignore already detached view. } } state.attachedWindowId = null + + // Clean up listeners if no sessions remain attached to this window + if (!this.findAttachedStateByWindowId(hostWindowId)) { + this.detachHostWindowListeners(hostWindowId) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/browser/YoBrowserPresenter.ts` around lines 540 - 564, The detach methods remove session views but never remove the host window's event listeners unless the window is closed; update detachFromWindow (and/or detachOtherSessionBrowsers) to detect when no more SessionBrowserState entries have attachedWindowId === hostWindowId after setting state.attachedWindowId = null and then call detachHostWindowListeners(hostWindowId) to remove listeners for that window; use the existing functions detachFromWindow, detachOtherSessionBrowsers, and detachHostWindowListeners identifiers to locate where to add the check so listeners are cleaned up when the last session detaches while the window remains open.src/renderer/src/components/sidepanel/BrowserPanel.vue (1)
514-522: Consider using a more explicit watch source for session status changes.The current approach creates a dependency string by joining session IDs and statuses. While functional, this pattern recalculates on every sessionStore change even if unrelated properties change.
A more explicit approach would be to watch the specific data needed:
♻️ Alternative approach
watch( - () => sessionStore.sessions.map((session) => `${session.id}:${session.status}`).join('|'), + () => sessionStore.sessions.map((session) => ({ id: session.id, status: session.status })), () => { void flushPendingSessionDestroys() if (currentSessionId.value) { void loadState(currentSessionId.value) } - } + }, + { deep: true } )This is a minor stylistic preference - the current implementation works correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/sidepanel/BrowserPanel.vue` around lines 514 - 522, Replace the synthetic dependency string in the watch with an explicit watcher over the exact values you care about: instead of () => sessionStore.sessions.map(...).join('|'), watch a function that returns the array of tuples or objects containing only id and status (e.g., sessionStore.sessions.map(s => [s.id, s.status]) or map to {id,status}) so the watcher only reacts to id/status changes; keep the body calling flushPendingSessionDestroys(), and when currentSessionId.value is set call loadState(currentSessionId.value) as before (refer to watch, sessionStore.sessions, flushPendingSessionDestroys, currentSessionId, and loadState).src/main/presenter/browser/YoBrowserToolHandler.ts (1)
43-64: Potential redundant check and inconsistent error handling incdp_send.The
getBrowserPagecheck (lines 43-46) verifies session initialization, butsendCdpCommand(line 50) will also throw if the session doesn't exist. This creates two potential issues:
- The error message "not initialized" at line 45 may be misleading if the session was destroyed between calls.
- The
pageobject retrieved at line 43 is only used for logging metadata (lines 58-60) but not for the actual CDP command execution.Consider consolidating the validation into
sendCdpCommandand catching the specific error for logging:♻️ Suggested refactor
case 'cdp_send': { const method = typeof args.method === 'string' ? args.method : '' if (!method) { throw new Error('CDP method is required') } - const page = await this.presenter.getBrowserPage(sessionId) - if (!page) { - throw new Error(`Session browser for ${sessionId} is not initialized`) - } - try { const params = this.normalizeCdpParams(args.params) const response = await this.presenter.sendCdpCommand(sessionId, method, params) return JSON.stringify(response ?? {}) } catch (error) { if (error instanceof Error && error.name === 'YoBrowserNotReadyError') { + const page = await this.presenter.getBrowserPage(sessionId) logger.warn('[YoBrowser] tool blocked:not-ready', { toolName: 'cdp_send', sessionId, method, - pageId: page.id, - url: page.url, - status: page.status + pageId: page?.id, + url: page?.url, + status: page?.status }) } throw error } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/browser/YoBrowserToolHandler.ts` around lines 43 - 64, Remove the initial getBrowserPage() null-check and let sendCdpCommand(sessionId, method, params) perform session validation; call this.presenter.sendCdpCommand(...) directly after normalizing params. In the catch block, specifically handle Error with name 'YoBrowserNotReadyError' by then calling this.presenter.getBrowserPage(sessionId) to retrieve page metadata for logging (guarding for null and only including page.id/url/status when available) instead of relying on the earlier page variable; keep throwing the original error after logging. Ensure references: normalizeCdpParams, sendCdpCommand, getBrowserPage, YoBrowserNotReadyError, and the page fields (id, url, status) are used as described.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture/tool-system.md`:
- Line 636: Update the docs line that currently references
BrowserTab.ensureSession(): replace that stale implementation pointer with a
reference to the session-scoped CDP guard used in the presenter/tool handler
path (i.e., point readers to the presenter/tool handler CDP guard instead of
BrowserTab.ensureSession()). Locate the old mention of
BrowserTab.ensureSession() in the sentence and change it to explicitly reference
the presenter/tool-handler session guard (the guard implemented in the
presenter/tool handler path) so the doc points to the current code location.
In `@src/main/lib/agentRuntime/sessionPaths.ts`:
- Around line 48-60: The sanitizeToolCallIdForOffload function currently
collates multiple disallowed characters into '_' causing distinct toolCallId
values (e.g., "tool:1" vs "tool/1") to collide; update
sanitizeToolCallIdForOffload to append a short deterministic fingerprint of the
original toolCallId (for example a truncated hex of a hash like SHA-1/MD5/CRC32)
to the sanitized name so that different inputs that sanitize to the same
characters still produce unique filenames, while continuing to replace invalid
chars and trim trailing invalid characters; ensure the appended fingerprint only
contains filesystem-safe characters and fall back to 'tool_call' if the final
result would be empty.
In `@test/main/presenter/floatingButtonPresenter/index.test.ts`:
- Around line 71-100: The inline class declaration for BrowserWindow inside the
vi.mock('electron', ...) object causes a Biome parse error; extract the class
declaration to module scope (e.g., declare class BrowserWindow {} above the
vi.mock call) and then reference that class name in the mock object instead of
declaring it inline, ensuring the mock still exports BrowserWindow, ipcMain,
screen, Menu, and app as before (update any type annotations or imports if
needed to use the new BrowserWindow symbol).
---
Nitpick comments:
In `@src/main/presenter/browser/YoBrowserPresenter.ts`:
- Around line 540-564: The detach methods remove session views but never remove
the host window's event listeners unless the window is closed; update
detachFromWindow (and/or detachOtherSessionBrowsers) to detect when no more
SessionBrowserState entries have attachedWindowId === hostWindowId after setting
state.attachedWindowId = null and then call
detachHostWindowListeners(hostWindowId) to remove listeners for that window; use
the existing functions detachFromWindow, detachOtherSessionBrowsers, and
detachHostWindowListeners identifiers to locate where to add the check so
listeners are cleaned up when the last session detaches while the window remains
open.
In `@src/main/presenter/browser/YoBrowserToolHandler.ts`:
- Around line 43-64: Remove the initial getBrowserPage() null-check and let
sendCdpCommand(sessionId, method, params) perform session validation; call
this.presenter.sendCdpCommand(...) directly after normalizing params. In the
catch block, specifically handle Error with name 'YoBrowserNotReadyError' by
then calling this.presenter.getBrowserPage(sessionId) to retrieve page metadata
for logging (guarding for null and only including page.id/url/status when
available) instead of relying on the earlier page variable; keep throwing the
original error after logging. Ensure references: normalizeCdpParams,
sendCdpCommand, getBrowserPage, YoBrowserNotReadyError, and the page fields (id,
url, status) are used as described.
In `@src/main/presenter/windowPresenter/index.ts`:
- Around line 521-527: The public method createBrowserWindow currently always
delegates to createManagedWindow with windowType:'chat', which makes the name
misleading; mark createBrowserWindow as deprecated and either redirect callers
to createAppWindow or change createBrowserWindow to call createAppWindow (or
proxy to it) and add a JSDoc `@deprecated` note plus console/process deprecation
warning; update callers to use createAppWindow and keep createBrowserWindow as a
thin shim that forwards to createAppWindow (or to createManagedWindow with
explicit comment) so intent is explicit.
In `@src/renderer/src/components/sidepanel/BrowserPanel.vue`:
- Around line 514-522: Replace the synthetic dependency string in the watch with
an explicit watcher over the exact values you care about: instead of () =>
sessionStore.sessions.map(...).join('|'), watch a function that returns the
array of tuples or objects containing only id and status (e.g.,
sessionStore.sessions.map(s => [s.id, s.status]) or map to {id,status}) so the
watcher only reacts to id/status changes; keep the body calling
flushPendingSessionDestroys(), and when currentSessionId.value is set call
loadState(currentSessionId.value) as before (refer to watch,
sessionStore.sessions, flushPendingSessionDestroys, currentSessionId, and
loadState).
In `@test/main/presenter/newAgentPresenter/newAgentPresenter.test.ts`:
- Around line 1004-1045: Add a regression test that verifies legacy tool IDs are
normalized to the canonical ID (yo_browser_cdp_send -> cdp_send) when updating
or reading disabled agent tools: in the existing test block for disabled agent
tools, mock sqlitePresenter.newSessionsTable.get to return the session and then
call presenter.updateSessionDisabledAgentTools with an array containing the
legacy ID "yo_browser_cdp_send" (and possibly duplicates), assert the returned
disabled list contains "cdp_send" (not the legacy ID) and that
sqlitePresenter.newSessionsTable.updateDisabledAgentTools was called with the
normalized array; also add a complementary call to
presenter.getSessionDisabledAgentTools (or mock getDisabledAgentTools to include
legacy IDs) to assert that getSessionDisabledAgentTools performs the same
normalization path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1dfcc6cc-f984-4740-9937-20999e22279e
📒 Files selected for processing (47)
docs/architecture/tool-system.mddocs/specs/yobrowser-optimization/plan.mddocs/specs/yobrowser-optimization/spec.mddocs/specs/yobrowser-optimization/tasks.mdelectron.vite.config.tssrc/main/lib/agentRuntime/sessionPaths.tssrc/main/presenter/agentPresenter/acp/agentToolManager.tssrc/main/presenter/agentPresenter/loop/toolCallProcessor.tssrc/main/presenter/browser/BrowserContextBuilder.tssrc/main/presenter/browser/YoBrowserPresenter.tssrc/main/presenter/browser/YoBrowserToolDefinitions.tssrc/main/presenter/browser/YoBrowserToolHandler.tssrc/main/presenter/deepchatAgentPresenter/toolOutputGuard.tssrc/main/presenter/floatingButtonPresenter/index.tssrc/main/presenter/newAgentPresenter/index.tssrc/main/presenter/tabPresenter.tssrc/main/presenter/toolPresenter/index.tssrc/main/presenter/windowPresenter/index.tssrc/renderer/browser/App.vuesrc/renderer/browser/components/AppBar.vuesrc/renderer/browser/components/BrowserToolbar.vuesrc/renderer/browser/components/icons/CloseIcon.vuesrc/renderer/browser/components/icons/MaximizeIcon.vuesrc/renderer/browser/components/icons/MinimizeIcon.vuesrc/renderer/browser/components/icons/RestoreIcon.vuesrc/renderer/browser/index.htmlsrc/renderer/browser/lib/events.tssrc/renderer/browser/main.tssrc/renderer/browser/stores/window.tssrc/renderer/src/components/sidepanel/BrowserPanel.vuesrc/renderer/src/components/sidepanel/BrowserPlaceholder.vuesrc/renderer/src/components/sidepanel/ChatSidePanel.vuesrc/shared/types/browser.tssrc/shared/types/presenters/legacy.presenters.d.tstest/main/lib/agentRuntime/sessionPaths.test.tstest/main/presenter/YoBrowserPresenter.test.tstest/main/presenter/agentPresenter/loop/toolCallProcessor.test.tstest/main/presenter/browser/YoBrowserToolHandler.test.tstest/main/presenter/deepchatAgentPresenter/deepchatAgentPresenter.test.tstest/main/presenter/deepchatAgentPresenter/dispatch.test.tstest/main/presenter/deepchatAgentPresenter/process.test.tstest/main/presenter/floatingButtonPresenter/index.test.tstest/main/presenter/newAgentPresenter/newAgentPresenter.test.tstest/main/presenter/toolPresenter/toolPresenter.test.tstest/renderer/components/BrowserPanel.test.tstest/renderer/components/McpIndicator.test.tstest/renderer/components/NewThreadPage.test.ts
💤 Files with no reviewable changes (13)
- electron.vite.config.ts
- src/main/presenter/tabPresenter.ts
- src/renderer/browser/lib/events.ts
- src/renderer/browser/components/icons/RestoreIcon.vue
- src/renderer/browser/index.html
- src/renderer/browser/components/BrowserToolbar.vue
- src/renderer/browser/App.vue
- src/renderer/browser/stores/window.ts
- src/renderer/browser/components/icons/CloseIcon.vue
- src/renderer/browser/components/icons/MinimizeIcon.vue
- src/renderer/browser/main.ts
- src/renderer/browser/components/icons/MaximizeIcon.vue
- src/renderer/browser/components/AppBar.vue
| **安全约束**： | ||
| **约束**： | ||
| - `cdp_send` 不会自动创建 browser；必须先调用 `load_url` | ||
| - `local://` URL 禁止 CDP attach（在 `BrowserTab.ensureSession()` 中检查） |
Contributor
There was a problem hiding this comment.
Update the stale implementation reference for the local:// CDP guard.
The mention of BrowserTab.ensureSession() looks outdated for the new session-scoped architecture; point this line to the current guard location used in the presenter/tool handler path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/tool-system.md` at line 636, Update the docs line that
currently references BrowserTab.ensureSession(): replace that stale
implementation pointer with a reference to the session-scoped CDP guard used in
the presenter/tool handler path (i.e., point readers to the presenter/tool
handler CDP guard instead of BrowserTab.ensureSession()). Locate the old mention
of BrowserTab.ensureSession() in the sentence and change it to explicitly
reference the presenter/tool-handler session guard (the guard implemented in the
presenter/tool handler path) so the doc points to the current code location.
This was referenced Mar 23, 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 by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation