diff --git a/docs/browser-automation-review-findings.md b/docs/browser-automation-review-findings.md index 47e62b09..66cf2271 100644 --- a/docs/browser-automation-review-findings.md +++ b/docs/browser-automation-review-findings.md @@ -28,9 +28,12 @@ Last updated: 2026-04-20 Resolution: the close request now carries `worktreeId`, and the renderer falls back to that worktree's active browser workspace instead of the global active browser tab. -- `open` `src/main/runtime/orca-runtime.ts:2153` - Issue: `tab close --page ` trusts the page id directly and does not verify that it exists - or belongs to the supplied `--worktree`. +- `fixed` `src/main/runtime/orca-runtime.ts:2153`, `src/main/runtime/orca-runtime.test.ts`, `src/renderer/src/hooks/useIpcEvents.ts`, `src/renderer/src/hooks/useIpcEvents.test.ts` + Issue: `tab close --page ` trusted the page id directly, did not verify that it existed in + the supplied `--worktree`, and the renderer close path collapsed page ids into workspace closes. + Resolution: main now validates explicit page ids against the scoped registered tab set before it + sends any close request, and the renderer now closes just the targeted page unless it is the last + page in that workspace. Unknown explicit page ids now fail instead of replying success. - `fixed` `src/main/browser/agent-browser-bridge.ts:247`, `src/renderer/src/store/slices/browser.ts:598`, `src/renderer/src/store/slices/browser.ts:810`, `src/main/ipc/browser.ts:148`, `src/main/ipc/browser.test.ts` Issue: per-worktree active-tab routing is not updated when the renderer changes the active @@ -38,6 +41,13 @@ Last updated: 2026-04-20 Resolution: `browser:activeTabChanged` now forwards the owning worktree id into the bridge so renderer tab switches update both the global active guest and the per-worktree active guest. +- `fixed` `src/main/browser/agent-browser-bridge.ts`, `src/main/browser/agent-browser-bridge.test.ts`, `src/main/ipc/browser.ts` + Issue: per-worktree active-tab routing could also go stale after a tab closed or a guest process + swap, leaving the bridge pointed at dead webContents ids. + Resolution: the bridge now repairs the per-worktree active guest when a tab closes, updates the + owning worktree slot on process swaps, and keeps `tab list` read-only so discovery does not + mutate future routing state. + ## Screenshot behavior - `fixed` `src/main/browser/agent-browser-bridge.ts:828`, `src/main/browser/agent-browser-bridge.test.ts` @@ -65,6 +75,13 @@ Last updated: 2026-04-20 Resolution: screenshot prep no longer forces the host BrowserWindow to the foreground. Guest background throttling remains disabled, so capture prep can avoid stealing app focus entirely. +- `fixed` `src/main/browser/browser-manager.ts`, `src/main/browser/browser-manager.test.ts` + Issue: screenshot prep changed Orca's remembered browser workspace/page even when the visible + pane was terminal/editor, so the next browser activation landed on the screenshot target instead + of the user's prior hidden browser state. + Resolution: restore now reapplies the previous browser workspace/page selection independently of + the visible pane type, then restores the non-browser pane focus. + - `fixed` `src/main/browser/cdp-screenshot.ts:68`, `src/main/browser/cdp-bridge.ts:636`, `src/main/browser/cdp-screenshot.test.ts` Issue: the screenshot timeout fallback ignores clip/full-page geometry, so a timed-out clipped or full-page request can return the wrong image. @@ -130,6 +147,12 @@ Last updated: 2026-04-20 ## Session lifecycle and process swaps +- `fixed` `src/main/browser/agent-browser-bridge.ts` + Issue: packaged macOS builds could fail to resolve the bundled `agent-browser` binary because the + path logic assumed a lowercase `resources` directory next to the executable. + Resolution: bundled binary lookup now uses `process.resourcesPath` (with a platform-aware test + fallback), which matches Electron packaging on macOS and other platforms. + - `fixed` `src/main/browser/agent-browser-bridge.ts:264`, `src/main/browser/agent-browser-bridge.ts:1148`, `src/main/browser/agent-browser-bridge.ts:1169`, `src/main/browser/agent-browser-bridge.ts:1559`, `src/main/browser/agent-browser-bridge.test.ts` Issue: intercept restore after a process swap replays old intercept patterns after the first successful command on the new session, which can re-enable routes the caller just disabled. diff --git a/src/cli/index.ts b/src/cli/index.ts index 02db4803..f41fc823 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -2164,9 +2164,9 @@ Terminals: Browser Automation: tab create Create a new browser tab (navigates to --url) tab list List open browser tabs - tab switch Switch the active browser tab by --index - tab close Close the active browser tab - snapshot Accessibility snapshot with element refs (e.g. e1, e2) + tab switch Switch the active browser tab by --index or --page + tab close Close a browser tab by --index/--page or the current tab + snapshot Accessibility snapshot with element refs (e.g. @e1, @e2) goto Navigate the active tab to --url click Click element by --element ref fill Clear and fill input by --element ref with --value @@ -2174,7 +2174,7 @@ Browser Automation: select Select dropdown option by --element ref and --value hover Hover element by --element ref keypress Press a key (e.g. --key Enter, --key Tab) - scroll Scroll --direction (up/down/left/right) by --amount pixels + scroll Scroll --direction (up/down) by --amount pixels back Navigate back in browser history reload Reload the active browser tab screenshot Capture viewport screenshot (--format png|jpeg) @@ -2191,7 +2191,7 @@ Browser Automation: scrollintoview Scroll --element into view get Get element property (--what: text, html, value, url, title) is Check element state (--what: visible, enabled, checked) - keyboard inserttext Insert text without key events + inserttext Insert text without key events mouse move Move mouse to --x --y coordinates mouse down Press mouse button mouse up Release mouse button @@ -2201,7 +2201,7 @@ Browser Automation: set offline Toggle offline mode (--state on|off) set headers Set HTTP headers (--headers '{"key":"val"}') set credentials Set HTTP auth (--user --pass

) - set media Set color scheme (--scheme dark|light) + set media Set color scheme (--color-scheme dark|light) clipboard read Read clipboard contents clipboard write Write --text to clipboard dialog accept Accept browser dialog (--text for prompt response) @@ -2212,8 +2212,8 @@ Browser Automation: storage session get Get sessionStorage value by --key storage session set Set sessionStorage --key --value storage session clear Clear sessionStorage - download Download file via --element to --path - highlight Highlight --element on page + download Download file via --selector to --path + highlight Highlight --selector on page exec Run any agent-browser command (--command "...") Common Commands: @@ -2274,13 +2274,13 @@ Browser Workflow: (Element refs change after navigation — always re-snapshot before interacting) Browser Options: - --element Element ref from snapshot (e.g. e3, not @e3) + --element Element ref from snapshot (e.g. @e3) --url URL to navigate to --value Value to fill or select --input Text to type at current focus (no element needed) --expression JavaScript expression to evaluate --key Key to press (Enter, Tab, Escape, Control+a, etc.) - --direction

Scroll direction: up, down, left, right + --direction Scroll direction: up or down --amount Scroll distance in pixels (default: viewport height) --index Tab index (from \`tab list\`) --page Stable browser page id (preferred for concurrent workflows) diff --git a/src/main/browser/agent-browser-bridge.test.ts b/src/main/browser/agent-browser-bridge.test.ts index 1edf6617..29122c25 100644 --- a/src/main/browser/agent-browser-bridge.test.ts +++ b/src/main/browser/agent-browser-bridge.test.ts @@ -264,6 +264,25 @@ describe('AgentBrowserBridge', () => { expect(result.tabs[0].browserPageId).toBe('tab-a') expect(result.tabs[0].url).toBe('https://a.com') }) + + it('does not mutate active-tab routing when tab-list infers the first live tab', () => { + const tabs = new Map([ + ['tab-a', 1], + ['tab-b', 2] + ]) + const wc1 = mockWebContents(1, 'https://a.com', 'A') + const wc2 = mockWebContents(2, 'https://b.com', 'B') + webContentsFromIdMock.mockImplementation((id: number) => (id === 1 ? wc1 : wc2)) + + const b = new AgentBrowserBridge(mockBrowserManager(tabs)) + + const result = b.tabList() + expect(result.tabs).toMatchObject([ + { browserPageId: 'tab-a', active: true }, + { browserPageId: 'tab-b', active: false } + ]) + expect(b.getActiveWebContentsId()).toBeNull() + }) }) // ── Tab switch ── @@ -672,6 +691,42 @@ describe('AgentBrowserBridge', () => { expect(bridge.getActiveWebContentsId()).toBeNull() }) + it('repairs per-worktree active routing when the active tab closes', async () => { + const tabs = new Map([ + ['tab-a', 1], + ['tab-b', 2] + ]) + const worktrees = new Map([ + ['tab-a', 'wt-1'], + ['tab-b', 'wt-1'] + ]) + const wc2 = mockWebContents(2, 'https://b.com', 'B') + webContentsFromIdMock.mockImplementation((id: number) => (id === 2 ? wc2 : null)) + + const b = new AgentBrowserBridge(mockBrowserManager(tabs, worktrees)) + b.setActiveTab(1, 'wt-1') + + await b.onTabClosed(1) + + expect(b.getActiveWebContentsId()).toBe(2) + expect(b.tabList('wt-1').tabs).toMatchObject([{ browserPageId: 'tab-b', active: true }]) + }) + + it('repairs per-worktree active routing when an active tab swaps processes', async () => { + const tabs = new Map([['tab-a', 200]]) + const worktrees = new Map([['tab-a', 'wt-1']]) + const wc = mockWebContents(200, 'https://a.com', 'A') + webContentsFromIdMock.mockImplementation((id: number) => (id === 200 ? wc : null)) + + const b = new AgentBrowserBridge(mockBrowserManager(tabs, worktrees)) + b.setActiveTab(100, 'wt-1') + + await b.onProcessSwap('tab-a', 200, 100) + + expect(b.getActiveWebContentsId()).toBe(200) + expect(b.tabList('wt-1').tabs).toMatchObject([{ browserPageId: 'tab-a', active: true }]) + }) + // ── tabSwitch success ── it('switches active tab and returns switched index', async () => { diff --git a/src/main/browser/agent-browser-bridge.ts b/src/main/browser/agent-browser-bridge.ts index 7e1b05b0..5dbc51aa 100644 --- a/src/main/browser/agent-browser-bridge.ts +++ b/src/main/browser/agent-browser-bridge.ts @@ -92,8 +92,15 @@ function agentBrowserNativeName(): string { function resolveAgentBrowserBinary(): string { // Why: production builds copy the platform-specific binary into resources/ - // via electron-builder extraResources. Check there first. - const bundled = join(app.getPath('exe'), '..', 'resources', agentBrowserNativeName()) + // via electron-builder extraResources. Use Electron's resolved resourcesPath + // instead of hand-rolling ../resources so packaged macOS builds keep working + // on case-sensitive filesystems where Contents/Resources casing matters. + const bundledResourcesPath = + process.resourcesPath ?? + (process.platform === 'darwin' + ? join(app.getPath('exe'), '..', '..', 'Resources') + : join(app.getPath('exe'), '..', 'resources')) + const bundled = join(bundledResourcesPath, agentBrowserNativeName()) if (existsSync(bundled)) { return bundled } @@ -230,6 +237,23 @@ export class AgentBrowserBridge { } } + private selectFallbackActiveWebContents( + worktreeId: string, + excludedWebContentsId?: number + ): number | null { + for (const [, wcId] of this.getRegisteredTabs(worktreeId)) { + if (wcId === excludedWebContentsId) { + continue + } + if (this.getWebContents(wcId)) { + this.activeWebContentsPerWorktree.set(worktreeId, wcId) + return wcId + } + } + this.activeWebContentsPerWorktree.delete(worktreeId) + return null + } + getActiveWebContentsId(): number | null { return this.activeWebContentsId } @@ -262,27 +286,55 @@ export class AgentBrowserBridge { } async onTabClosed(webContentsId: number): Promise { - if (this.activeWebContentsId === webContentsId) { - this.activeWebContentsId = null - } const browserPageId = this.resolveTabIdSafe(webContentsId) + const owningWorktreeId = browserPageId + ? this.browserManager.getWorktreeIdForTab(browserPageId) + : undefined + let nextWorktreeActiveWebContentsId: number | null = null + if ( + owningWorktreeId && + this.activeWebContentsPerWorktree.get(owningWorktreeId) === webContentsId + ) { + nextWorktreeActiveWebContentsId = this.selectFallbackActiveWebContents( + owningWorktreeId, + webContentsId + ) + } + if (this.activeWebContentsId === webContentsId) { + this.activeWebContentsId = nextWorktreeActiveWebContentsId + } if (browserPageId) { await this.destroySession(`orca-tab-${browserPageId}`) } } - async onProcessSwap(browserPageId: string, newWebContentsId: number): Promise { + async onProcessSwap( + browserPageId: string, + newWebContentsId: number, + previousWebContentsId?: number + ): Promise { // Why: Electron process swaps give same browserPageId but new webContentsId. // Old proxy's webContents is destroyed, so destroy session and let next command recreate. const sessionName = `orca-tab-${browserPageId}` const session = this.sessions.get(sessionName) + const oldWebContentsId = previousWebContentsId ?? session?.webContentsId + const owningWorktreeId = this.browserManager.getWorktreeIdForTab(browserPageId) // Why: save active intercept patterns before destroying so they can be restored // on the new session after the next successful init command. if (session && session.activeInterceptPatterns.length > 0) { this.pendingInterceptRestore.set(sessionName, [...session.activeInterceptPatterns]) } await this.destroySession(sessionName) - this.activeWebContentsId = newWebContentsId + if (oldWebContentsId != null && this.activeWebContentsId === oldWebContentsId) { + this.activeWebContentsId = newWebContentsId + } + if ( + owningWorktreeId && + oldWebContentsId != null && + this.activeWebContentsPerWorktree.get(owningWorktreeId) === oldWebContentsId + ) { + this.activeWebContentsPerWorktree.set(owningWorktreeId, newWebContentsId) + } } // ── Worktree-scoped tab queries ── @@ -308,6 +360,8 @@ export class AgentBrowserBridge { const tabs = this.getRegisteredTabs(worktreeId) // Why: use per-worktree active tab for the "active" flag so tab-list is // consistent with what resolveActiveTab would pick for command routing. + // Keep this read-only though: discovery commands must not mutate the + // active-tab state that later bare commands rely on. let activeWcId = (worktreeId && this.activeWebContentsPerWorktree.get(worktreeId)) ?? this.activeWebContentsId const result: BrowserTabInfo[] = [] @@ -329,14 +383,12 @@ export class AgentBrowserBridge { active: wcId === activeWcId }) } - // Why: if no tab has been explicitly activated (e.g. first use after app - // start), auto-activate the first live tab so the active flag is never - // all-false when tabs exist. + // Why: if no tab has been explicitly activated yet, surface the first live + // tab as active in the listing without mutating bridge state. That keeps + // `tab list` side-effect free while still showing users which tab a bare + // command would select next. if (activeWcId == null && firstLiveWcId !== null) { - this.activeWebContentsId = firstLiveWcId - if (worktreeId) { - this.activeWebContentsPerWorktree.set(worktreeId, firstLiveWcId) - } + activeWcId = firstLiveWcId if (result.length > 0) { result[0].active = true } diff --git a/src/main/browser/browser-manager.test.ts b/src/main/browser/browser-manager.test.ts index 3eccfb77..e92f5c55 100644 --- a/src/main/browser/browser-manager.test.ts +++ b/src/main/browser/browser-manager.test.ts @@ -332,6 +332,67 @@ describe('browserManager', () => { expect(restoreScript).toContain('"page-prev"') }) + it('restores remembered browser workspace/page even when the visible pane was terminal', async () => { + const rendererExecuteJavaScriptMock = vi + .fn() + .mockResolvedValueOnce({ + prevTabType: 'terminal', + prevActiveWorktreeId: 'wt-target', + prevActiveBrowserWorkspaceId: 'workspace-prev', + prevActiveBrowserPageId: 'page-prev', + prevFocusedGroupTabId: 'tab-prev', + targetWorktreeId: 'wt-target', + targetBrowserWorkspaceId: 'workspace-target', + targetBrowserPageId: 'page-target' + }) + .mockResolvedValueOnce(undefined) + const guest = { + id: 7091, + isDestroyed: vi.fn(() => false), + getType: vi.fn(() => 'webview'), + setBackgroundThrottling: guestSetBackgroundThrottlingMock, + setWindowOpenHandler: guestSetWindowOpenHandlerMock, + on: guestOnMock, + off: guestOffMock, + openDevTools: guestOpenDevToolsMock + } + const renderer = { + id: rendererWebContentsId, + isDestroyed: vi.fn(() => false), + executeJavaScript: rendererExecuteJavaScriptMock + } + browserWindowFromWebContentsMock.mockReturnValue({ isFocused: vi.fn(() => true) }) + webContentsFromIdMock.mockImplementation((id: number) => { + if (id === guest.id) { + return guest + } + if (id === rendererWebContentsId) { + return renderer + } + return null + }) + + browserManager.attachGuestPolicies(guest as never) + browserManager.registerGuest({ + browserPageId: 'page-target', + workspaceId: 'workspace-target', + worktreeId: 'wt-target', + webContentsId: guest.id, + rendererWebContentsId + }) + + const restore = await browserManager.ensureWebviewVisible(guest.id) + restore() + + const restoreScript = rendererExecuteJavaScriptMock.mock.calls[1]?.[0] + expect(restoreScript).toContain('state.setActiveBrowserTab("workspace-prev");') + expect(restoreScript).toContain('state.setActiveBrowserPage(') + expect(restoreScript).toContain('"workspace-prev"') + expect(restoreScript).toContain('"page-prev"') + expect(restoreScript).toContain('state.activateTab("tab-prev");') + expect(restoreScript).toContain('state.setActiveTabType("terminal");') + }) + it('does not focus the Orca window while preparing a screenshot', async () => { const rendererExecuteJavaScriptMock = vi.fn().mockResolvedValueOnce({ prevTabType: 'terminal', diff --git a/src/main/browser/browser-manager.ts b/src/main/browser/browser-manager.ts index 5afdae6e..d8bf41ab 100644 --- a/src/main/browser/browser-manager.ts +++ b/src/main/browser/browser-manager.ts @@ -288,7 +288,6 @@ export class BrowserManager { state = store.getState(); } if ( - ${JSON.stringify(prev?.prevTabType)} === 'browser' && ${JSON.stringify(prev?.prevActiveBrowserWorkspaceId)} && ${JSON.stringify(prev?.prevActiveBrowserWorkspaceId)} !== ${JSON.stringify(prev?.targetBrowserWorkspaceId)} && @@ -298,19 +297,26 @@ export class BrowserManager { state = store.getState(); } if ( - ${JSON.stringify(prev?.prevTabType)} === 'browser' && ${JSON.stringify(prev?.prevActiveBrowserWorkspaceId)} && ${JSON.stringify(prev?.prevActiveBrowserPageId)} && ${JSON.stringify(prev?.prevActiveBrowserPageId)} !== ${JSON.stringify(prev?.targetBrowserPageId)} && typeof state.setActiveBrowserPage === 'function' ) { + // Why: Orca remembers the last browser workspace/page even when + // the user is currently in terminal/editor view. Screenshot prep + // temporarily switches that hidden browser selection state, so + // restore it independently of the visible tab type. state.setActiveBrowserPage( ${JSON.stringify(prev?.prevActiveBrowserWorkspaceId)}, ${JSON.stringify(prev?.prevActiveBrowserPageId)} ); state = store.getState(); - } else if (${JSON.stringify(prev?.prevFocusedGroupTabId)}) { + } + if ( + ${JSON.stringify(prev?.prevTabType)} !== 'browser' && + ${JSON.stringify(prev?.prevFocusedGroupTabId)} + ) { state.activateTab(${JSON.stringify(prev?.prevFocusedGroupTabId)}); } if (${JSON.stringify(prev?.prevTabType)} !== 'browser') { diff --git a/src/main/ipc/browser.ts b/src/main/ipc/browser.ts index da2eda25..bd0c3ccb 100644 --- a/src/main/ipc/browser.ts +++ b/src/main/ipc/browser.ts @@ -117,7 +117,7 @@ export function registerBrowserHandlers(): void { rendererWebContentsId: event.sender.id }) if (agentBrowserBridgeRef && previousWcId !== null && previousWcId !== args.webContentsId) { - agentBrowserBridgeRef.onProcessSwap(args.browserPageId, args.webContentsId) + agentBrowserBridgeRef.onProcessSwap(args.browserPageId, args.webContentsId, previousWcId) } const pendingResolve = pendingTabRegistrations.get(args.browserPageId) if (pendingResolve) { diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index b704c7a1..39fa7465 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -873,5 +873,49 @@ describe('OrcaRuntimeService', () => { ).rejects.toThrow('selector_not_found') expect(tabListMock).not.toHaveBeenCalled() }) + + it('rejects closing an unknown page id instead of treating it as success', async () => { + vi.mocked(listWorktrees).mockResolvedValue(MOCK_GIT_WORKTREES) + const runtime = createRuntime() + + runtime.setAgentBrowserBridge({ + getRegisteredTabs: vi.fn(() => new Map([['page-1', 1]])) + } as never) + + await expect( + runtime.browserTabClose({ + page: 'missing-page' + }) + ).rejects.toThrow('Browser page missing-page was not found') + }) + + it('rejects closing a page outside the explicitly scoped worktree', async () => { + vi.mocked(listWorktrees).mockResolvedValue([ + ...MOCK_GIT_WORKTREES, + { + path: '/tmp/worktree-b', + head: 'def', + branch: 'feature/bar', + isBare: false, + isMainWorktree: false + } + ]) + const runtime = createRuntime() + const getRegisteredTabsMock = vi.fn((worktreeId?: string) => + worktreeId === `${TEST_REPO_ID}::/tmp/worktree-b` ? new Map() : new Map([['page-1', 1]]) + ) + + runtime.setAgentBrowserBridge({ + getRegisteredTabs: getRegisteredTabsMock + } as never) + + await expect( + runtime.browserTabClose({ + page: 'page-1', + worktree: 'path:/tmp/worktree-b' + }) + ).rejects.toThrow('Browser page page-1 was not found in this worktree') + expect(getRegisteredTabsMock).toHaveBeenCalledWith(`${TEST_REPO_ID}::/tmp/worktree-b`) + }) }) }) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 3103d5ad..adafb4a1 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -2148,12 +2148,18 @@ export class OrcaRuntimeService { page?: string worktree?: string }): Promise<{ closed: boolean }> { - const win = this.getAuthoritativeWindow() const bridge = this.requireAgentBrowserBridge() const worktreeId = await this.resolveBrowserWorktreeId(params.worktree) let tabId: string | null = null if (typeof params.page === 'string' && params.page.length > 0) { + if (!bridge.getRegisteredTabs(worktreeId).has(params.page)) { + const scope = worktreeId ? ' in this worktree' : '' + throw new BrowserError( + 'browser_tab_not_found', + `Browser page ${params.page} was not found${scope}` + ) + } tabId = params.page } else if (params.index !== undefined) { const tabs = bridge.getRegisteredTabs(worktreeId) @@ -2174,6 +2180,7 @@ export class OrcaRuntimeService { } } + const win = this.getAuthoritativeWindow() const requestId = randomUUID() await new Promise((resolve, reject) => { const timer = setTimeout(() => { diff --git a/src/renderer/src/hooks/useIpcEvents.test.ts b/src/renderer/src/hooks/useIpcEvents.test.ts index 61e87776..4d33b1d5 100644 --- a/src/renderer/src/hooks/useIpcEvents.test.ts +++ b/src/renderer/src/hooks/useIpcEvents.test.ts @@ -392,6 +392,7 @@ describe('useIpcEvents browser tab close routing', () => { it('closes the active browser tab for the requested worktree when main does not provide a tab id', async () => { const closeBrowserTab = vi.fn() + const closeBrowserPage = vi.fn() const replyTabClose = vi.fn() const tabCloseListenerRef: { current: @@ -447,7 +448,8 @@ describe('useIpcEvents browser tab close routing', () => { 'wt-2': [{ id: 'workspace-target' }] }, browserPagesByWorkspace: {}, - closeBrowserTab + closeBrowserTab, + closeBrowserPage }) } })) @@ -547,8 +549,333 @@ describe('useIpcEvents browser tab close routing', () => { }) expect(closeBrowserTab).toHaveBeenCalledWith('workspace-target') + expect(closeBrowserPage).not.toHaveBeenCalled() expect(replyTabClose).toHaveBeenCalledWith({ requestId: 'req-1' }) }) + + it('closes only the requested browser page when a workspace has multiple pages', async () => { + const closeBrowserTab = vi.fn() + const closeBrowserPage = vi.fn() + const replyTabClose = vi.fn() + const tabCloseListenerRef: { + current: + | ((data: { requestId: string; tabId: string | null; worktreeId?: string }) => void) + | null + } = { + current: null + } + + vi.doMock('react', async () => { + const actual = await vi.importActual('react') + return { + ...actual, + useEffect: (effect: () => void | (() => void)) => { + effect() + } + } + }) + + vi.doMock('../store', () => ({ + useAppStore: { + getState: () => ({ + setUpdateStatus: vi.fn(), + fetchRepos: vi.fn(), + fetchWorktrees: vi.fn(), + setActiveView: vi.fn(), + activeModal: null, + closeModal: vi.fn(), + openModal: vi.fn(), + activeWorktreeId: 'wt-1', + activeView: 'terminal', + setActiveRepo: vi.fn(), + setActiveWorktree: vi.fn(), + revealWorktreeInSidebar: vi.fn(), + setIsFullScreen: vi.fn(), + updateBrowserTabPageState: vi.fn(), + activeTabType: 'browser', + editorFontZoomLevel: 0, + setEditorFontZoomLevel: vi.fn(), + setRateLimitsFromPush: vi.fn(), + setSshConnectionState: vi.fn(), + setSshTargetLabels: vi.fn(), + enqueueSshCredentialRequest: vi.fn(), + removeSshCredentialRequest: vi.fn(), + settings: { terminalFontSize: 13 }, + activeBrowserTabId: 'workspace-1', + activeBrowserTabIdByWorktree: { 'wt-1': 'workspace-1' }, + browserTabsByWorktree: { + 'wt-1': [{ id: 'workspace-1' }] + }, + browserPagesByWorkspace: { + 'workspace-1': [ + { id: 'page-1', workspaceId: 'workspace-1' }, + { id: 'page-2', workspaceId: 'workspace-1' } + ] + }, + closeBrowserTab, + closeBrowserPage + }) + } + })) + + vi.doMock('@/lib/ui-zoom', () => ({ + applyUIZoom: vi.fn() + })) + vi.doMock('@/lib/worktree-activation', () => ({ + activateAndRevealWorktree: vi.fn(), + ensureWorktreeHasInitialTerminal: vi.fn() + })) + vi.doMock('@/components/sidebar/visible-worktrees', () => ({ + getVisibleWorktreeIds: () => [] + })) + vi.doMock('@/lib/editor-font-zoom', () => ({ + nextEditorFontZoomLevel: vi.fn(() => 0), + computeEditorFontSize: vi.fn(() => 13) + })) + vi.doMock('@/components/settings/SettingsConstants', () => ({ + zoomLevelToPercent: vi.fn(() => 100), + ZOOM_MIN: -3, + ZOOM_MAX: 3 + })) + vi.doMock('@/lib/zoom-events', () => ({ + dispatchZoomLevelChanged: vi.fn() + })) + + vi.stubGlobal('window', { + dispatchEvent: vi.fn(), + api: { + repos: { onChanged: () => () => {} }, + worktrees: { onChanged: () => () => {} }, + ui: { + onOpenSettings: () => () => {}, + onToggleLeftSidebar: () => () => {}, + onToggleRightSidebar: () => () => {}, + onToggleWorktreePalette: () => () => {}, + onOpenQuickOpen: () => () => {}, + onJumpToWorktreeIndex: () => () => {}, + onActivateWorktree: () => () => {}, + onNewBrowserTab: () => () => {}, + onRequestTabCreate: () => () => {}, + replyTabCreate: () => {}, + onRequestTabClose: ( + listener: (data: { + requestId: string + tabId: string | null + worktreeId?: string + }) => void + ) => { + tabCloseListenerRef.current = listener + return () => {} + }, + replyTabClose, + onNewTerminalTab: () => () => {}, + onCloseActiveTab: () => () => {}, + onSwitchTab: () => () => {}, + onToggleStatusBar: () => () => {}, + onFullscreenChanged: () => () => {}, + onTerminalZoom: () => () => {}, + getZoomLevel: () => 0, + set: vi.fn() + }, + updater: { + getStatus: () => Promise.resolve({ state: 'idle' }), + onStatus: () => () => {}, + onClearDismissal: () => () => {} + }, + browser: { + onGuestLoadFailed: () => () => {}, + onOpenLinkInOrcaTab: () => () => {}, + onNavigationUpdate: () => () => {}, + onActivateView: () => () => {} + }, + rateLimits: { + get: () => Promise.resolve({ limits: {}, lastUpdatedAt: Date.now() }), + onUpdate: () => () => {} + }, + ssh: { + listTargets: () => Promise.resolve([]), + getState: () => Promise.resolve(null), + onStateChanged: () => () => {}, + onCredentialRequest: () => () => {}, + onCredentialResolved: () => () => {} + } + } + }) + + const { useIpcEvents } = await import('./useIpcEvents') + useIpcEvents() + + tabCloseListenerRef.current?.({ + requestId: 'req-2', + tabId: 'page-2' + }) + + expect(closeBrowserPage).toHaveBeenCalledWith('page-2') + expect(closeBrowserTab).not.toHaveBeenCalled() + expect(replyTabClose).toHaveBeenCalledWith({ requestId: 'req-2' }) + }) + + it('rejects explicit unknown browser page ids instead of reporting success', async () => { + const closeBrowserTab = vi.fn() + const closeBrowserPage = vi.fn() + const replyTabClose = vi.fn() + const tabCloseListenerRef: { + current: + | ((data: { requestId: string; tabId: string | null; worktreeId?: string }) => void) + | null + } = { + current: null + } + + vi.doMock('react', async () => { + const actual = await vi.importActual('react') + return { + ...actual, + useEffect: (effect: () => void | (() => void)) => { + effect() + } + } + }) + + vi.doMock('../store', () => ({ + useAppStore: { + getState: () => ({ + setUpdateStatus: vi.fn(), + fetchRepos: vi.fn(), + fetchWorktrees: vi.fn(), + setActiveView: vi.fn(), + activeModal: null, + closeModal: vi.fn(), + openModal: vi.fn(), + activeWorktreeId: 'wt-1', + activeView: 'terminal', + setActiveRepo: vi.fn(), + setActiveWorktree: vi.fn(), + revealWorktreeInSidebar: vi.fn(), + setIsFullScreen: vi.fn(), + updateBrowserTabPageState: vi.fn(), + activeTabType: 'browser', + editorFontZoomLevel: 0, + setEditorFontZoomLevel: vi.fn(), + setRateLimitsFromPush: vi.fn(), + setSshConnectionState: vi.fn(), + setSshTargetLabels: vi.fn(), + enqueueSshCredentialRequest: vi.fn(), + removeSshCredentialRequest: vi.fn(), + settings: { terminalFontSize: 13 }, + activeBrowserTabId: 'workspace-1', + activeBrowserTabIdByWorktree: { 'wt-1': 'workspace-1' }, + browserTabsByWorktree: { + 'wt-1': [{ id: 'workspace-1' }] + }, + browserPagesByWorkspace: { + 'workspace-1': [{ id: 'page-1', workspaceId: 'workspace-1' }] + }, + closeBrowserTab, + closeBrowserPage + }) + } + })) + + vi.doMock('@/lib/ui-zoom', () => ({ + applyUIZoom: vi.fn() + })) + vi.doMock('@/lib/worktree-activation', () => ({ + activateAndRevealWorktree: vi.fn(), + ensureWorktreeHasInitialTerminal: vi.fn() + })) + vi.doMock('@/components/sidebar/visible-worktrees', () => ({ + getVisibleWorktreeIds: () => [] + })) + vi.doMock('@/lib/editor-font-zoom', () => ({ + nextEditorFontZoomLevel: vi.fn(() => 0), + computeEditorFontSize: vi.fn(() => 13) + })) + vi.doMock('@/components/settings/SettingsConstants', () => ({ + zoomLevelToPercent: vi.fn(() => 100), + ZOOM_MIN: -3, + ZOOM_MAX: 3 + })) + vi.doMock('@/lib/zoom-events', () => ({ + dispatchZoomLevelChanged: vi.fn() + })) + + vi.stubGlobal('window', { + dispatchEvent: vi.fn(), + api: { + repos: { onChanged: () => () => {} }, + worktrees: { onChanged: () => () => {} }, + ui: { + onOpenSettings: () => () => {}, + onToggleLeftSidebar: () => () => {}, + onToggleRightSidebar: () => () => {}, + onToggleWorktreePalette: () => () => {}, + onOpenQuickOpen: () => () => {}, + onJumpToWorktreeIndex: () => () => {}, + onActivateWorktree: () => () => {}, + onNewBrowserTab: () => () => {}, + onRequestTabCreate: () => () => {}, + replyTabCreate: () => {}, + onRequestTabClose: ( + listener: (data: { + requestId: string + tabId: string | null + worktreeId?: string + }) => void + ) => { + tabCloseListenerRef.current = listener + return () => {} + }, + replyTabClose, + onNewTerminalTab: () => () => {}, + onCloseActiveTab: () => () => {}, + onSwitchTab: () => () => {}, + onToggleStatusBar: () => () => {}, + onFullscreenChanged: () => () => {}, + onTerminalZoom: () => () => {}, + getZoomLevel: () => 0, + set: vi.fn() + }, + updater: { + getStatus: () => Promise.resolve({ state: 'idle' }), + onStatus: () => () => {}, + onClearDismissal: () => () => {} + }, + browser: { + onGuestLoadFailed: () => () => {}, + onOpenLinkInOrcaTab: () => () => {}, + onNavigationUpdate: () => () => {}, + onActivateView: () => () => {} + }, + rateLimits: { + get: () => Promise.resolve({ limits: {}, lastUpdatedAt: Date.now() }), + onUpdate: () => () => {} + }, + ssh: { + listTargets: () => Promise.resolve([]), + getState: () => Promise.resolve(null), + onStateChanged: () => () => {}, + onCredentialRequest: () => () => {}, + onCredentialResolved: () => () => {} + } + } + }) + + const { useIpcEvents } = await import('./useIpcEvents') + useIpcEvents() + + tabCloseListenerRef.current?.({ + requestId: 'req-3', + tabId: 'missing-page' + }) + + expect(closeBrowserPage).not.toHaveBeenCalled() + expect(closeBrowserTab).not.toHaveBeenCalled() + expect(replyTabClose).toHaveBeenCalledWith({ + requestId: 'req-3', + error: 'Browser tab missing-page not found' + }) + }) }) describe('useIpcEvents shortcut hint clearing', () => { diff --git a/src/renderer/src/hooks/useIpcEvents.ts b/src/renderer/src/hooks/useIpcEvents.ts index d9cb89be..f7f18211 100644 --- a/src/renderer/src/hooks/useIpcEvents.ts +++ b/src/renderer/src/hooks/useIpcEvents.ts @@ -241,8 +241,9 @@ export function useIpcEvents(): void { window.api.ui.onRequestTabClose((data) => { try { const store = useAppStore.getState() + const explicitTargetId = data.tabId ?? null let tabToClose = - data.tabId ?? + explicitTargetId ?? (data.worktreeId ? (store.activeBrowserTabIdByWorktree?.[data.worktreeId] ?? null) : store.activeBrowserTabId) @@ -255,7 +256,9 @@ export function useIpcEvents(): void { } // Why: the bridge stores tabs keyed by browserPageId (which is the page // ID from registerGuest), but closeBrowserTab expects a workspace ID. If - // tabToClose is a page ID, resolve it to its owning workspace ID. + // tabToClose is a page ID, close only that page unless it is the + // last page in its workspace. The CLI's `tab close --page` contract + // targets one browser page, not the entire workspace tab. const isWorkspaceId = Object.values(store.browserTabsByWorktree) .flat() .some((ws) => ws.id === tabToClose) @@ -264,9 +267,23 @@ export function useIpcEvents(): void { ([, pages]) => pages.some((p) => p.id === tabToClose) ) if (owningWorkspace) { - tabToClose = owningWorkspace[0] + const [workspaceId, pages] = owningWorkspace + if (pages.length <= 1) { + store.closeBrowserTab(workspaceId) + } else { + store.closeBrowserPage(tabToClose) + } + window.api.ui.replyTabClose({ requestId: data.requestId }) + return } } + if (explicitTargetId) { + window.api.ui.replyTabClose({ + requestId: data.requestId, + error: `Browser tab ${explicitTargetId} not found` + }) + return + } store.closeBrowserTab(tabToClose) window.api.ui.replyTabClose({ requestId: data.requestId }) } catch (err) { diff --git a/src/shared/runtime-types.ts b/src/shared/runtime-types.ts index dcbf9f0d..6162f921 100644 --- a/src/shared/runtime-types.ts +++ b/src/shared/runtime-types.ts @@ -385,6 +385,7 @@ export type BrowserTabCloseResult = { export type BrowserErrorCode = | 'browser_no_tab' | 'browser_tab_not_found' + | 'browser_tab_closed' | 'browser_stale_ref' | 'browser_ref_not_found' | 'browser_navigation_failed'