Merged
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
📝 WalkthroughWalkthroughEmbedded browser lifecycle and host-readiness were added: a new OPEN_REQUESTED event starts embedded-window initialization; host bounds stabilization is awaited before navigation; BrowserTab enforces interactive readiness for CDP, DOM extraction, scripts, and screenshots; presenter and renderer components coordinate visibility and bounds sync. Changes
Sequence DiagramsequenceDiagram
participant User as "User/App"
participant ChatPanel as "ChatSidePanel"
participant IPC as "IPC Channel"
participant Presenter as "YoBrowserPresenter"
participant BrowserTab as "BrowserTab"
participant WebContents as "WebContents"
participant BrowserPanel as "BrowserPanel"
User->>ChatPanel: request open browser
ChatPanel->>IPC: emit OPEN_REQUESTED
IPC->>Presenter: receive open request
Presenter->>Presenter: mark host NOT ready & schedule readiness
Presenter->>BrowserPanel: ensure attachment (await stable bounds)
BrowserPanel->>BrowserPanel: capture container bounds / waitForStableRect
BrowserPanel-->>Presenter: syncVisibleBounds
Presenter->>Presenter: mark host READY
Presenter->>BrowserTab: navigateUntilDomReady(url)
BrowserTab->>WebContents: loadURL(url)
WebContents-->>BrowserTab: did-start-loading
WebContents-->>BrowserTab: dom-ready
BrowserTab->>BrowserTab: set interactiveReady=true
BrowserTab-->>Presenter: ready for interactions
WebContents-->>BrowserTab: did-finish-load
BrowserTab->>BrowserTab: set fullReady=true
User->>BrowserPanel: request script/screenshot
BrowserPanel->>BrowserTab: runPageAction (ensureInteractiveReady)
BrowserTab->>WebContents: perform action
WebContents-->>BrowserTab: result
BrowserTab-->>BrowserPanel: respond
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
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)
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: 2
🧹 Nitpick comments (4)
src/main/presenter/browser/YoBrowserToolHandler.ts (1)
125-139: Avoid double-loggingYoBrowserNotReadyError.This block warns and then rethrows into
callTool()'s outer catch, which still logs the same retryable condition as an error. The new not-ready path will therefore emit noisy duplicate logs for an expected lifecycle race.🤖 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 125 - 139, The catch is double-logging YoBrowserNotReadyError downstream; modify the catch in YoBrowserToolHandler around the browserPage.sendCdpCommand call so that when error instanceof Error && error.name === 'YoBrowserNotReadyError' you mark the error to suppress further logging (e.g. set a flag like error.suppressLogging = true or error.handledBy = 'YoBrowserToolHandler') before rethrowing instead of changing control flow—this preserves retry behavior while allowing callTool()'s outer catch to skip noisy duplicate logging by checking that flag.src/renderer/src/components/sidepanel/BrowserPanel.vue (1)
340-346: Void-returning presenter methods cannot be distinguished from errors.The
navigateWindowmethod returnsPromise<void>per the interface (context snippet 3). When successful,callPresenter<void>will returnundefined, but the checkresult === nulltreats this as success. However, if there's an IPC error,callPresenterreturnsnull.Since
voidmethods returnundefinedon success andnullon error, this works correctly. But using<void>as the generic type may be confusing since you're comparing againstnull. Consider adding a comment for clarity or using a more explicit pattern.♻️ Suggested clarification
- const result = await callPresenter<void>( + // callPresenter returns null on error, undefined on success for void methods + const result = await callPresenter<void>( 'navigateWindow', yoBrowserPresenter.navigateWindow(browserWindowId.value, nextUrl) )🤖 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 340 - 346, The null-vs-undefined ambiguity arises because callPresenter<void>(...) returns undefined on success and null on IPC error; update the navigateWindow call to explicitly check for null to detect errors (e.g., treat result === null as failure) and add a short inline comment explaining that callPresenter<void> returns undefined on success and null on error, referencing the call to callPresenter with yoBrowserPresenter.navigateWindow(browserWindowId.value, nextUrl) so future readers understand why the null comparison is used.test/main/presenter/browser/BrowserTab.test.ts (1)
5-64: Consider extracting shared MockWebContents to reduce duplication.This
MockWebContentsclass is nearly identical to the one inYoBrowserPresenter.test.ts. While functional, maintaining two copies increases the risk of divergence.♻️ Suggestion: Extract to shared test utility
Consider creating a shared test utility file:
// test/main/__mocks__/MockWebContents.ts export class MockWebContents extends EventEmitter { // ... shared implementation }Then import in both test files:
import { MockWebContents } from '../__mocks__/MockWebContents'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/browser/BrowserTab.test.ts` around lines 5 - 64, The MockWebContents class in BrowserTab.test.ts is duplicated in YoBrowserPresenter.test.ts; extract the shared implementation into a single exported class (e.g., MockWebContents) in a test utility module (suggested: test/main/__mocks__/MockWebContents.ts) and replace the local class definitions by importing that exported MockWebContents in both BrowserTab.test.ts and YoBrowserPresenter.test.ts; ensure the exported class preserves all members used by tests (url, title, destroyed, loading, pendingLoad, debugger, session, navigationHistory, loadURL, isLoading, finishLoad, emitDomReady, getURL, getTitle, isDestroyed, reload, goBack, goForward, sendInputEvent) so tests continue to work.test/renderer/components/ChatSidePanel.test.ts (1)
1-83: Consider adding test cleanup and additional edge case coverage.The test correctly validates the happy path for OPEN_REQUESTED handling. However, there are a few considerations:
- The test doesn't clean up
window.apiandwindow.electronmocks after completion, which could affect other tests in the suite.- Consider adding a test case for when
windowIddoesn't match (should not triggeropenBrowser).♻️ Suggested cleanup and edge case test
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { describe, expect, it, vi } from 'vitest' describe('ChatSidePanel', () => { + afterEach(() => { + vi.resetModules() + delete (window as any).api + delete (window as any).electron + }) + it('opens the browser sidepanel when OPEN_REQUESTED targets the current host window', async () => { // ... existing test ... }) + + it('ignores OPEN_REQUESTED when windowId does not match the current host', async () => { + // Similar setup but call handler with windowId: 999 + // Verify openBrowser was NOT called + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/components/ChatSidePanel.test.ts` around lines 1 - 83, Add teardown and negative-case coverage: after mounting and assertions, remove/restore the globals and handlers by deleting window.api and window.electron (or restoring originals if saved), clear the handlers Map, and call the mocked ipcRenderer.removeListener if needed to mimic cleanup for the ChatSidePanel test (references: handlers Map, window.api.getWindowId, window.electron.ipcRenderer.on/removeListener, sidepanelStore.openBrowser). Also add a second test that imports ChatSidePanel, registers the same mocks but calls the stored handler with a non-matching payload (e.g., { windowId: 999 }) and asserts sidepanelStore.openBrowser was not called to cover the mismatch edge case.
🤖 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/browser/YoBrowserPresenter.ts`:
- Around line 396-409: The navigateWindow method currently calls
state.page.navigate which waits for full load; change it to use the same
dom-ready path as openWindow by calling state.page.navigateUntilDomReady(url,
timeoutMs) (or the equivalent signature on the Page API) so follow-up
navigations use the DOM-ready wait strategy; keep the existing lifecycle log,
state.updatedAt assignment and emitWindowUpdated call unchanged to preserve
telemetry and state updates.
In `@test/renderer/components/BrowserPanel.test.ts`:
- Around line 31-53: The test harness's setup uses the nullish coalescing
operator when assigning yoBrowserPresenter.getWindowById's mockResolvedValue, so
passing getWindowByIdResult: null still falls back to the default object; update
the assignment in setup (the getWindowById mock within yoBrowserPresenter in the
setup function) to check for undefined explicitly (e.g., if
options?.getWindowByIdResult !== undefined use that value, otherwise use the
default window object) so callers can pass null to simulate a missing window.
---
Nitpick comments:
In `@src/main/presenter/browser/YoBrowserToolHandler.ts`:
- Around line 125-139: The catch is double-logging YoBrowserNotReadyError
downstream; modify the catch in YoBrowserToolHandler around the
browserPage.sendCdpCommand call so that when error instanceof Error &&
error.name === 'YoBrowserNotReadyError' you mark the error to suppress further
logging (e.g. set a flag like error.suppressLogging = true or error.handledBy =
'YoBrowserToolHandler') before rethrowing instead of changing control flow—this
preserves retry behavior while allowing callTool()'s outer catch to skip noisy
duplicate logging by checking that flag.
In `@src/renderer/src/components/sidepanel/BrowserPanel.vue`:
- Around line 340-346: The null-vs-undefined ambiguity arises because
callPresenter<void>(...) returns undefined on success and null on IPC error;
update the navigateWindow call to explicitly check for null to detect errors
(e.g., treat result === null as failure) and add a short inline comment
explaining that callPresenter<void> returns undefined on success and null on
error, referencing the call to callPresenter with
yoBrowserPresenter.navigateWindow(browserWindowId.value, nextUrl) so future
readers understand why the null comparison is used.
In `@test/main/presenter/browser/BrowserTab.test.ts`:
- Around line 5-64: The MockWebContents class in BrowserTab.test.ts is
duplicated in YoBrowserPresenter.test.ts; extract the shared implementation into
a single exported class (e.g., MockWebContents) in a test utility module
(suggested: test/main/__mocks__/MockWebContents.ts) and replace the local class
definitions by importing that exported MockWebContents in both
BrowserTab.test.ts and YoBrowserPresenter.test.ts; ensure the exported class
preserves all members used by tests (url, title, destroyed, loading,
pendingLoad, debugger, session, navigationHistory, loadURL, isLoading,
finishLoad, emitDomReady, getURL, getTitle, isDestroyed, reload, goBack,
goForward, sendInputEvent) so tests continue to work.
In `@test/renderer/components/ChatSidePanel.test.ts`:
- Around line 1-83: Add teardown and negative-case coverage: after mounting and
assertions, remove/restore the globals and handlers by deleting window.api and
window.electron (or restoring originals if saved), clear the handlers Map, and
call the mocked ipcRenderer.removeListener if needed to mimic cleanup for the
ChatSidePanel test (references: handlers Map, window.api.getWindowId,
window.electron.ipcRenderer.on/removeListener, sidepanelStore.openBrowser). Also
add a second test that imports ChatSidePanel, registers the same mocks but calls
the stored handler with a non-matching payload (e.g., { windowId: 999 }) and
asserts sidepanelStore.openBrowser was not called to cover the mismatch edge
case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 703d1403-e26c-46a4-b9b9-e8cc9c7725af
📒 Files selected for processing (12)
src/main/events.tssrc/main/presenter/browser/BrowserTab.tssrc/main/presenter/browser/YoBrowserPresenter.tssrc/main/presenter/browser/YoBrowserToolDefinitions.tssrc/main/presenter/browser/YoBrowserToolHandler.tssrc/renderer/src/components/sidepanel/BrowserPanel.vuesrc/renderer/src/components/sidepanel/ChatSidePanel.vuesrc/renderer/src/events.tstest/main/presenter/YoBrowserPresenter.test.tstest/main/presenter/browser/BrowserTab.test.tstest/renderer/components/BrowserPanel.test.tstest/renderer/components/ChatSidePanel.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/chat/mentions/utils.ts (1)
120-128: Avoid making the empty slash menu unbounded.Because
useChatInputMentionsforwards this result straight intoSuggestionList, andSuggestionList.vuenow rendersprops.itemsverbatim, returningitemsunchanged here means typing/can create a button for every command, prompt, and tool before the user narrows the query. That is an easy UI latency path once MCP catalogs get large. Consider capping the empty state too, or virtualizing the list if showing everything is intentional.♻️ Simple cap for the empty-query path
export const filterSlashSuggestionItems = ( items: SlashSuggestionItem[], query: string, limit = MAX_FILTERED_SLASH_SUGGESTIONS ): SlashSuggestionItem[] => { const normalized = query.trim().toLowerCase() if (!normalized) { - return items + return items.slice(0, limit) } return items .filter((item) => { if (item.label.toLowerCase().includes(normalized)) return true return item.description?.toLowerCase().includes(normalized) }) .slice(0, limit) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/components/chat/mentions/utils.ts` around lines 120 - 128, filterSlashSuggestionItems currently returns the full items array when the normalized query is empty, which lets typing "/" render an unbounded list; change the empty-query path in filterSlashSuggestionItems to cap the results using the provided limit (e.g., return only the first limit entries of items) instead of returning items unchanged so SuggestionList receives a bounded set (or alternatively hook SuggestionList to a virtualization strategy if you intend to show everything).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/renderer/src/components/chat/mentions/utils.ts`:
- Around line 120-128: filterSlashSuggestionItems currently returns the full
items array when the normalized query is empty, which lets typing "/" render an
unbounded list; change the empty-query path in filterSlashSuggestionItems to cap
the results using the provided limit (e.g., return only the first limit entries
of items) instead of returning items unchanged so SuggestionList receives a
bounded set (or alternatively hook SuggestionList to a virtualization strategy
if you intend to show everything).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a213d3e4-a5a1-4303-9b25-f46b0e2eee24
📒 Files selected for processing (5)
src/renderer/src/components/chat/composables/useChatInputMentions.tssrc/renderer/src/components/chat/mentions/SuggestionList.vuesrc/renderer/src/components/chat/mentions/utils.tstest/renderer/components/SuggestionList.test.tstest/renderer/composables/useChatInputMentions.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
New Features
Bug Fixes
Documentation
UX