From c134b1c31dcdca26cd0724cbbc892b15d32dee06 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 26 May 2026 19:27:23 -0400 Subject: [PATCH] Fix hidden browser automation paintability (#2851) Co-authored-by: Orca --- src/cli/browser.test.ts | 5 +- src/cli/handlers/computer.test.ts | 9 +- src/cli/handlers/file.test.ts | 11 +- src/cli/index.test.ts | 31 +-- src/cli/selectors.ts | 8 +- src/main/browser/agent-browser-bridge.test.ts | 68 ++++- src/main/browser/agent-browser-bridge.ts | 92 +++++-- src/main/browser/browser-manager.test.ts | 160 ++++++++++++ src/main/browser/browser-manager.ts | 104 ++++++++ src/main/runtime/orca-runtime-browser.ts | 19 +- src/main/runtime/orca-runtime.test.ts | 39 +++ src/main/runtime/orca-runtime.ts | 7 + src/preload/api-types.ts | 2 +- src/preload/index.ts | 4 +- src/renderer/src/components/Terminal.tsx | 17 +- .../components/browser-pane/BrowserPane.tsx | 55 +++- .../browser-pane/BrowserPaneOverlayLayer.tsx | 14 +- .../browser-automation-visibility.test.ts | 85 +++++++ .../browser-automation-visibility.ts | 130 ++++++++++ src/renderer/src/hooks/useIpcEvents.test.ts | 235 ++++++++++++++++++ src/renderer/src/hooks/useIpcEvents.ts | 67 ++++- src/renderer/src/store/slices/browser.test.ts | 14 ++ src/renderer/src/store/slices/browser.ts | 3 +- src/renderer/src/store/slices/tabs.test.ts | 10 + src/renderer/src/store/slices/tabs.ts | 19 +- 25 files changed, 1094 insertions(+), 114 deletions(-) create mode 100644 src/renderer/src/components/browser-pane/browser-automation-visibility.test.ts create mode 100644 src/renderer/src/components/browser-pane/browser-automation-visibility.ts diff --git a/src/cli/browser.test.ts b/src/cli/browser.test.ts index 161368f7a0e..86bbee3039a 100644 --- a/src/cli/browser.test.ts +++ b/src/cli/browser.test.ts @@ -1,5 +1,4 @@ /* eslint-disable max-lines -- Why: this file groups every CLI browser-command test (page targeting, profiles, waits, viewport) so test-fixture imports and the runtime-client mock stay shared in one place. */ -import path from 'path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const callMock = vi.fn() @@ -90,7 +89,7 @@ describe('orca cli browser page targeting', () => { expect(callMock).toHaveBeenNthCalledWith(1, 'worktree.list', { limit: 10_000 }) expect(callMock).toHaveBeenNthCalledWith(2, 'browser.snapshot', { page: 'page-1', - worktree: `path:${path.resolve('/tmp/repo/feature')}` + worktree: 'id:repo::/tmp/repo/feature' }) }) @@ -124,7 +123,7 @@ describe('orca cli browser page targeting', () => { expect(callMock).toHaveBeenNthCalledWith(2, 'browser.tabSwitch', { index: undefined, page: 'page-2', - worktree: `path:${path.resolve('/tmp/repo/feature')}` + worktree: 'id:repo::/tmp/repo/feature' }) }) diff --git a/src/cli/handlers/computer.test.ts b/src/cli/handlers/computer.test.ts index dca8f050ba7..62f86363a48 100644 --- a/src/cli/handlers/computer.test.ts +++ b/src/cli/handlers/computer.test.ts @@ -1,5 +1,4 @@ /* eslint-disable max-lines -- Why: computer CLI coverage shares one mocked runtime setup across command contracts. */ -import path from 'path' import { beforeEach, describe, expect, it, vi } from 'vitest' const callMock = vi.fn() @@ -71,7 +70,7 @@ describe('orca computer CLI handlers', () => { expect(callMock).toHaveBeenNthCalledWith(1, 'worktree.list', { limit: 10_000 }) expect(callMock).toHaveBeenNthCalledWith(2, 'computer.listApps', { - worktree: `path:${path.resolve('/tmp/repo')}` + worktree: 'id:repo::/tmp/repo' }) }) @@ -142,7 +141,7 @@ describe('orca computer CLI handlers', () => { expect(callMock).toHaveBeenNthCalledWith(2, 'computer.getAppState', { app: 'Finder', - worktree: `path:${path.resolve('/tmp/repo')}`, + worktree: 'id:repo::/tmp/repo', noScreenshot: true, restoreWindow: true }) @@ -176,7 +175,7 @@ describe('orca computer CLI handlers', () => { expect(callMock).toHaveBeenNthCalledWith(2, 'computer.listWindows', { app: 'Finder', - worktree: `path:${path.resolve('/tmp/repo')}` + worktree: 'id:repo::/tmp/repo' }) const output = vi.mocked(console.log).mock.calls[0][0] expect(output).toContain('[0] id:42 "Recents"') @@ -241,7 +240,7 @@ describe('orca computer CLI handlers', () => { expect(callMock).toHaveBeenNthCalledWith(2, 'computer.drag', { app: 'Finder', - worktree: `path:${path.resolve('/tmp/repo')}`, + worktree: 'id:repo::/tmp/repo', fromElementIndex: undefined, toElementIndex: undefined, fromX: 1, diff --git a/src/cli/handlers/file.test.ts b/src/cli/handlers/file.test.ts index b351f0420cb..b4e11d020db 100644 --- a/src/cli/handlers/file.test.ts +++ b/src/cli/handlers/file.test.ts @@ -1,5 +1,4 @@ /* eslint-disable max-lines -- Why: file CLI coverage shares one mocked runtime setup across command contracts. */ -import path from 'path' import { beforeEach, describe, expect, it, vi } from 'vitest' const callMock = vi.fn() @@ -83,7 +82,7 @@ describe('orca file CLI handlers', () => { expect(callMock).toHaveBeenNthCalledWith(1, 'worktree.list', { limit: 10_000 }) expect(callMock).toHaveBeenNthCalledWith(2, 'files.open', { - worktree: `path:${path.resolve('/tmp/repo')}`, + worktree: 'id:repo::/tmp/repo', relativePath: 'src/App.tsx' }) expect(vi.mocked(console.log).mock.calls[0][0]).toBe('Opened src/App.tsx.') @@ -160,20 +159,20 @@ describe('orca file CLI handlers', () => { await main(['file', 'open-changed'], '/tmp/repo/src') expect(callMock).toHaveBeenNthCalledWith(2, 'git.status', { - worktree: `path:${path.resolve('/tmp/repo')}` + worktree: 'id:repo::/tmp/repo' }) expect(callMock).toHaveBeenNthCalledWith(3, 'files.openDiff', { - worktree: `path:${path.resolve('/tmp/repo')}`, + worktree: 'id:repo::/tmp/repo', relativePath: 'src/App.tsx', staged: false }) expect(callMock).toHaveBeenNthCalledWith(4, 'files.openDiff', { - worktree: `path:${path.resolve('/tmp/repo')}`, + worktree: 'id:repo::/tmp/repo', relativePath: 'package.json', staged: true }) expect(callMock).toHaveBeenNthCalledWith(5, 'files.openDiff', { - worktree: `path:${path.resolve('/tmp/repo')}`, + worktree: 'id:repo::/tmp/repo', relativePath: 'docs/new.md', staged: false }) diff --git a/src/cli/index.test.ts b/src/cli/index.test.ts index ec97b542096..6446680b142 100644 --- a/src/cli/index.test.ts +++ b/src/cli/index.test.ts @@ -182,7 +182,10 @@ describe('orca cli worktree awareness', () => { it('shows the enclosing worktree for `worktree current`', async () => { queueFixtures( callMock, - worktreeListFixture([buildWorktree('/tmp/repo/feature', 'feature/foo')]), + worktreeListFixture([ + buildWorktree('/tmp/repo/feature', 'feature/foo'), + buildWorktree('/tmp/repo/feature', 'feature/foo', 'abc', 'duplicate-repo') + ]), okFixture('req_1', { worktree: { id: 'repo::/tmp/repo/feature', @@ -197,7 +200,7 @@ describe('orca cli worktree awareness', () => { expect(callMock).toHaveBeenNthCalledWith(1, 'worktree.list', { limit: 10_000 }) expect(callMock).toHaveBeenNthCalledWith(2, 'worktree.show', { - worktree: `path:${path.resolve('/tmp/repo/feature')}` + worktree: 'id:repo::/tmp/repo/feature' }) expect(logSpy).toHaveBeenCalledTimes(1) }) @@ -245,7 +248,7 @@ describe('orca cli worktree awareness', () => { ) expect(callMock).toHaveBeenNthCalledWith(2, 'worktree.set', { - worktree: `path:${path.resolve('/tmp/repo/feature')}`, + worktree: 'id:repo::/tmp/repo/feature', displayName: undefined, linkedIssue: undefined, comment: 'hello', @@ -332,7 +335,7 @@ describe('orca cli worktree awareness', () => { displayName: undefined, linkedIssue: undefined, comment: undefined, - parentWorktree: `path:${path.resolve('/tmp/repo/parent')}`, + parentWorktree: 'id:repo::/tmp/repo/parent', noParent: false }) }) @@ -437,7 +440,7 @@ describe('orca cli worktree awareness', () => { runHooks: false, activate: true, parentWorktree: undefined, - cwdParentWorktree: `path:${path.resolve('/tmp/repo')}`, + cwdParentWorktree: 'id:repo-1::/tmp/repo', noParent: false, callerTerminalHandle: undefined }) @@ -541,7 +544,7 @@ describe('orca cli worktree awareness', () => { comment: undefined, runHooks: false, activate: false, - parentWorktree: `path:${path.resolve('/tmp/repo/parent')}`, + parentWorktree: 'id:repo-1::/tmp/repo/parent', noParent: false, callerTerminalHandle: undefined }) @@ -666,7 +669,7 @@ describe('orca cli worktree awareness', () => { runHooks: false, activate: false, parentWorktree: undefined, - cwdParentWorktree: `path:${path.resolve('/tmp/repo')}`, + cwdParentWorktree: 'id:repo-1::/tmp/repo', noParent: false, callerTerminalHandle: 'term_parent' }) @@ -882,7 +885,7 @@ describe('orca cli worktree awareness', () => { runHooks: true, activate: true, parentWorktree: undefined, - cwdParentWorktree: `path:${path.resolve('/tmp/repo')}`, + cwdParentWorktree: 'id:repo-1::/tmp/repo', noParent: false, callerTerminalHandle: undefined }) @@ -1273,7 +1276,7 @@ describe('orca cli worktree awareness', () => { await main(['worktree', 'show', '--worktree', 'current', '--json'], '/tmp/repo/feature/src') expect(callMock).toHaveBeenNthCalledWith(2, 'worktree.show', { - worktree: `path:${path.resolve('/tmp/repo/feature')}` + worktree: 'id:repo::/tmp/repo/feature' }) }) @@ -1397,7 +1400,7 @@ describe('orca cli worktree awareness', () => { await main(['terminal', 'list', '--worktree', 'active', '--json'], '/tmp/repo/feature/src') expect(callMock).toHaveBeenNthCalledWith(2, 'terminal.list', { - worktree: `path:${path.resolve('/tmp/repo/feature')}`, + worktree: 'id:repo::/tmp/repo/feature', limit: undefined }) }) @@ -1610,7 +1613,7 @@ describe('orca cli worktree awareness', () => { prompt: 'Review open changes', agentId: 'codex', repo: undefined, - workspace: `path:${path.resolve('/tmp/repo/feature')}`, + workspace: 'id:repo-1::/tmp/repo/feature', workspaceMode: 'existing', baseBranch: undefined, reuseSession: undefined, @@ -1823,7 +1826,7 @@ describe('orca cli worktree awareness', () => { 2, 'automation.create', expect.objectContaining({ - workspace: `path:${path.resolve('/tmp/repo/feature')}`, + workspace: 'id:repo-1::/tmp/repo/feature', workspaceMode: 'existing', reuseSession: true }) @@ -1950,7 +1953,7 @@ describe('orca cli worktree awareness', () => { prompt: 'Review open changes', agentId: 'codex', repo: undefined, - workspace: `path:${path.resolve('/tmp/repo/feature')}`, + workspace: 'id:repo-1::/tmp/repo/feature', workspaceMode: 'existing', baseBranch: undefined, timezone: undefined, @@ -1982,7 +1985,7 @@ describe('orca cli worktree awareness', () => { prompt: undefined, agentId: undefined, repo: undefined, - workspace: `path:${path.resolve('/tmp/repo/feature')}`, + workspace: 'id:repo-1::/tmp/repo/feature', workspaceMode: undefined, baseBranch: undefined, reuseSession: undefined, diff --git a/src/cli/selectors.ts b/src/cli/selectors.ts index c56d4f152e0..a94878ca87f 100644 --- a/src/cli/selectors.ts +++ b/src/cli/selectors.ts @@ -69,10 +69,10 @@ export async function resolveCurrentWorktreeSelector( } // Why: users expect "active/current" to mean the enclosing managed worktree - // even from nested subdirectories. The CLI resolves that shell-local concept - // to the deepest matching worktree root, then hands the runtime a normal - // path selector so selector semantics stay centralized in one layer. - return buildCurrentWorktreeSelector(enclosingWorktree.path) + // even from nested subdirectories. Resolve to the concrete runtime id here: + // duplicate repo registrations can expose the same Git worktree path, and a + // path selector would throw selector_ambiguous after losing the repo id. + return `id:${enclosingWorktree.id}` } export async function getOptionalWorktreeSelector( diff --git a/src/main/browser/agent-browser-bridge.test.ts b/src/main/browser/agent-browser-bridge.test.ts index be6c0c693c6..5a1937ec35f 100644 --- a/src/main/browser/agent-browser-bridge.test.ts +++ b/src/main/browser/agent-browser-bridge.test.ts @@ -70,6 +70,7 @@ function mockBrowserManager( getWorktreeIdForTab: (tabId: string) => worktrees.get(tabId), getGuestWebContentsId: vi.fn(() => null), ensureWebviewVisible: vi.fn(async () => () => {}), + acquireAutomationVisibility: vi.fn(async () => () => {}), ...overrides } as unknown as BrowserManager } @@ -420,6 +421,55 @@ describe('AgentBrowserBridge', () => { expect(snapshotIdx).toBeLessThan(clickIdx) }) + it('acquires an automation visibility lease while running snapshot commands', async () => { + const lifecycleEvents: string[] = [] + const restore = vi.fn(() => { + lifecycleEvents.push('restore-100') + }) + const acquireAutomationVisibility = vi.fn(async (webContentsId: number) => { + lifecycleEvents.push(`acquire-${webContentsId}`) + return restore + }) + + const b = new AgentBrowserBridge( + mockBrowserManager(undefined, undefined, { + acquireAutomationVisibility + }) + ) + b.setActiveTab(100) + + let releaseSnapshot: (() => void) | null = null + execFileMock.mockImplementation( + (_bin: string, args: string[], _opts: unknown, cb: Function) => { + if (args.includes('close')) { + cb(null, JSON.stringify({ success: true, data: null }), '') + return + } + if (args.includes('snapshot')) { + lifecycleEvents.push('command-snapshot') + releaseSnapshot = () => { + cb(null, JSON.stringify({ success: true, data: { snapshot: 'tree' } }), '') + } + return + } + cb(null, JSON.stringify({ success: true, data: { ok: true } }), '') + } + ) + + const snapshot = b.snapshot() + + await vi.waitFor(() => { + expect(releaseSnapshot).not.toBeNull() + }) + expect(lifecycleEvents).toEqual(['acquire-100', 'command-snapshot']) + expect(restore).not.toHaveBeenCalled() + + releaseSnapshot!() + + await expect(snapshot).resolves.toEqual({ browserPageId: 'tab-1', snapshot: 'tree' }) + expect(lifecycleEvents).toEqual(['acquire-100', 'command-snapshot', 'restore-100']) + }) + it('serializes screenshot visibility prep across sessions', async () => { vi.useFakeTimers() try { @@ -432,8 +482,8 @@ describe('AgentBrowserBridge', () => { ['tab-2', 'wt-2'] ]) const lifecycleEvents: string[] = [] - const ensureWebviewVisibleMock = vi.fn(async (webContentsId: number) => { - lifecycleEvents.push(`ensure-${webContentsId}`) + const acquireAutomationVisibilityMock = vi.fn(async (webContentsId: number) => { + lifecycleEvents.push(`acquire-${webContentsId}`) return () => { lifecycleEvents.push(`restore-${webContentsId}`) } @@ -449,7 +499,7 @@ describe('AgentBrowserBridge', () => { const b = new AgentBrowserBridge( mockBrowserManager(tabs, worktrees, { - ensureWebviewVisible: ensureWebviewVisibleMock + acquireAutomationVisibility: acquireAutomationVisibilityMock }) ) b.setActiveTab(1, 'wt-1') @@ -489,9 +539,9 @@ describe('AgentBrowserBridge', () => { await Promise.resolve() await vi.advanceTimersByTimeAsync(300) - expect(lifecycleEvents).toContain('ensure-1') + expect(lifecycleEvents).toContain('acquire-1') expect(lifecycleEvents).toContain('command-orca-tab-tab-1') - expect(lifecycleEvents).not.toContain('ensure-2') + expect(lifecycleEvents).not.toContain('acquire-2') expect(releaseFirstScreenshot).not.toBeNull() releaseFirstScreenshot!() @@ -503,7 +553,9 @@ describe('AgentBrowserBridge', () => { await Promise.resolve() await Promise.resolve() - expect(lifecycleEvents.indexOf('restore-1')).toBeLessThan(lifecycleEvents.indexOf('ensure-2')) + expect(lifecycleEvents.indexOf('restore-1')).toBeLessThan( + lifecycleEvents.indexOf('acquire-2') + ) await vi.advanceTimersByTimeAsync(300) await expect(second).resolves.toEqual({ @@ -665,7 +717,9 @@ describe('AgentBrowserBridge', () => { ) const runningSnapshot = bridge.snapshot() - await Promise.resolve() + await vi.waitFor(() => { + expect(resolveRunningCommand).not.toBeNull() + }) const destroyPromise = ( bridge as unknown as { destroySession: (name: string) => Promise } diff --git a/src/main/browser/agent-browser-bridge.ts b/src/main/browser/agent-browser-bridge.ts index 41c7f2a4943..21bce47e4c1 100644 --- a/src/main/browser/agent-browser-bridge.ts +++ b/src/main/browser/agent-browser-bridge.ts @@ -86,6 +86,11 @@ type AgentBrowserExecOptions = { timeoutError?: BrowserError } +type EnqueueTargetedCommandOptions = { + ensureSession?: boolean + ensureVisible?: boolean +} + type AgentBrowserBridgeOptions = { onTabsChanged?: (worktreeId?: string) => void } @@ -388,10 +393,10 @@ export class AgentBrowserBridge { private readonly sessions = new Map() private readonly commandQueues = new Map() private readonly processingQueues = new Set() - // Why: screenshot prep temporarily changes shared renderer visibility/focus - // state. Per-session queues only serialize commands within one browser tab, so - // concurrent screenshots on different tabs can otherwise interleave - // ensureWebviewVisible()/restore and blank each other's capture. + // Why: screenshot prep temporarily changes shared renderer paintability state. + // Per-session queues only serialize commands within one browser tab, so + // concurrent screenshots on different tabs can otherwise interleave hidden + // surface leases and blank each other's capture. private screenshotTurn: Promise = Promise.resolve() private readonly agentBrowserBin: string // Why: when a process swap destroys a session that had active intercept patterns, @@ -1139,9 +1144,14 @@ export class AgentBrowserBridge { ): Promise { // Why: agent-browser writes the screenshot to a temp file and returns // { "path": "/tmp/screenshot-xxx.png" }. We read the file and return base64. - return this.enqueueTargetedCommand(worktreeId, browserPageId, async (sessionName) => { - return this.captureScreenshotCommand(sessionName, ['screenshot'], 300, format) - }) + return this.enqueueTargetedCommand( + worktreeId, + browserPageId, + async (sessionName) => { + return this.captureScreenshotCommand(sessionName, ['screenshot'], 300, format) + }, + { ensureVisible: false } + ) } async fullPageScreenshot( @@ -1149,14 +1159,19 @@ export class AgentBrowserBridge { worktreeId?: string, browserPageId?: string ): Promise { - return this.enqueueTargetedCommand(worktreeId, browserPageId, async (sessionName, target) => { - return this.captureFullPageScreenshotCommand( - sessionName, - target.webContentsId, - 500, - format === 'jpeg' ? 'jpeg' : 'png' - ) - }) + return this.enqueueTargetedCommand( + worktreeId, + browserPageId, + async (sessionName, target) => { + return this.captureFullPageScreenshotCommand( + sessionName, + target.webContentsId, + 500, + format === 'jpeg' ? 'jpeg' : 'png' + ) + }, + { ensureVisible: false } + ) } private readScreenshotFromResult(raw: unknown, format?: string): BrowserScreenshotResult { @@ -1180,12 +1195,12 @@ export class AgentBrowserBridge { return this.withSerializedScreenshotAccess(async () => { const session = this.sessions.get(sessionName) const restore = session - ? await this.browserManager.ensureWebviewVisible(session.webContentsId) + ? await this.browserManager.acquireAutomationVisibility(session.webContentsId) : () => {} try { - // Why: after focusing the window and unhiding the webview, the compositor + // Why: after acquiring the hidden paintability lease, the compositor // needs a short settle period to produce a painted frame. Waiting inside - // the global screenshot lock prevents another tab from stealing visible + // the global screenshot lock prevents another tab from changing lease // state before the current capture actually hits CDP. await new Promise((r) => setTimeout(r, settleMs)) const raw = await this.execAgentBrowser(sessionName, commandArgs) @@ -1205,11 +1220,11 @@ export class AgentBrowserBridge { return this.withSerializedScreenshotAccess(async () => { const session = this.sessions.get(sessionName) const restore = session - ? await this.browserManager.ensureWebviewVisible(session.webContentsId) + ? await this.browserManager.acquireAutomationVisibility(session.webContentsId) : () => {} try { // Why: full-page capture still depends on the guest compositor producing - // a fresh frame. Wait after activating the target webview so the direct + // a fresh frame. Wait after the target webview is paintable so the direct // CDP capture sees the live page instead of a stale surface. await new Promise((r) => setTimeout(r, settleMs)) const wc = this.getWebContents(webContentsId) @@ -1691,8 +1706,11 @@ export class AgentBrowserBridge { worktreeId: string | undefined, execute: (sessionName: string) => Promise ): Promise { - return this.enqueueTargetedCommand(worktreeId, undefined, async (sessionName) => - execute(sessionName) + return this.enqueueTargetedCommand( + worktreeId, + undefined, + async (sessionName) => execute(sessionName), + { ensureVisible: false } ) } @@ -1700,7 +1718,7 @@ export class AgentBrowserBridge { worktreeId: string | undefined, browserPageId: string | undefined, execute: (sessionName: string, target: ResolvedBrowserCommandTarget) => Promise, - options: { ensureSession?: boolean } = {} + options: EnqueueTargetedCommandOptions = {} ): Promise { const target = this.resolveCommandTarget(worktreeId, browserPageId) const sessionName = `orca-tab-${target.browserPageId}` @@ -1716,7 +1734,13 @@ export class AgentBrowserBridge { this.commandQueues.set(sessionName, queue) } queue.push({ - execute: (() => execute(sessionName, target)) as () => Promise, + execute: (() => + this.executeWithVisibleTarget( + sessionName, + target, + execute, + options + )) as () => Promise, resolve: resolve as (value: unknown) => void, reject }) @@ -1724,6 +1748,26 @@ export class AgentBrowserBridge { }) } + private async executeWithVisibleTarget( + sessionName: string, + target: ResolvedBrowserCommandTarget, + execute: (sessionName: string, target: ResolvedBrowserCommandTarget) => Promise, + options: EnqueueTargetedCommandOptions + ): Promise { + if (options.ensureVisible === false) { + return execute(sessionName, target) + } + + // Why: inactive browser panes are display:none in the renderer; the + // automation lease makes only this target paintable without selecting it. + const restore = await this.browserManager.acquireAutomationVisibility(target.webContentsId) + try { + return await execute(sessionName, target) + } finally { + restore() + } + } + private async processQueue(sessionName: string): Promise { if (this.processingQueues.has(sessionName)) { return diff --git a/src/main/browser/browser-manager.test.ts b/src/main/browser/browser-manager.test.ts index 89ec920799c..bbdefb15c76 100644 --- a/src/main/browser/browser-manager.test.ts +++ b/src/main/browser/browser-manager.test.ts @@ -249,6 +249,166 @@ describe('browserManager', () => { restore() }) + it('acquires renderer automation visibility without changing active browser state', async () => { + const rendererExecuteJavaScriptMock = vi + .fn() + .mockResolvedValueOnce('lease-1') + .mockResolvedValueOnce(true) + const guest = { + id: 1707, + 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 + } + 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-automation', + workspaceId: 'workspace-1', + worktreeId: 'wt-1', + webContentsId: guest.id, + rendererWebContentsId + }) + + const restore = await browserManager.acquireAutomationVisibility(guest.id) + const acquireScript = rendererExecuteJavaScriptMock.mock.calls[0]?.[0] + expect(acquireScript).toContain('__orcaBrowserAutomationVisibility') + expect(acquireScript).toContain('bridge.acquire("page-automation")') + expect(acquireScript).not.toContain('setActiveBrowserTab') + expect(acquireScript).not.toContain('setActiveTabType') + + restore() + + const releaseScript = rendererExecuteJavaScriptMock.mock.calls[1]?.[0] + expect(releaseScript).toContain('bridge.release("lease-1")') + }) + + it('returns a no-op automation visibility restore when renderer acquire hangs', async () => { + vi.useFakeTimers() + + const rendererExecuteJavaScriptMock = vi.fn().mockReturnValueOnce(new Promise(() => {})) + const guest = { + id: 1708, + 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 + } + 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-hung-acquire', + workspaceId: 'workspace-1', + worktreeId: 'wt-1', + webContentsId: guest.id, + rendererWebContentsId + }) + + const restorePromise = browserManager.acquireAutomationVisibility(guest.id) + await vi.advanceTimersByTimeAsync(2_000) + const restore = await restorePromise + + restore() + + expect(rendererExecuteJavaScriptMock).toHaveBeenCalledTimes(1) + }) + + it('releases a delayed automation visibility token after acquire timeout', async () => { + vi.useFakeTimers() + + let resolveAcquire: (token: string) => void = () => {} + const acquirePromise = new Promise((resolve) => { + resolveAcquire = resolve + }) + const rendererExecuteJavaScriptMock = vi + .fn() + .mockReturnValueOnce(acquirePromise) + .mockResolvedValueOnce(true) + const guest = { + id: 1709, + 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 + } + 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-delayed-acquire', + workspaceId: 'workspace-1', + worktreeId: 'wt-1', + webContentsId: guest.id, + rendererWebContentsId + }) + + const restorePromise = browserManager.acquireAutomationVisibility(guest.id) + await vi.advanceTimersByTimeAsync(2_000) + const restore = await restorePromise + + restore() + expect(rendererExecuteJavaScriptMock).toHaveBeenCalledTimes(1) + + resolveAcquire('late-lease-1') + await Promise.resolve() + await Promise.resolve() + + expect(rendererExecuteJavaScriptMock).toHaveBeenCalledTimes(2) + const releaseScript = rendererExecuteJavaScriptMock.mock.calls[1]?.[0] + expect(releaseScript).toContain('bridge.release("late-lease-1")') + }) + it('restores the previously focused browser workspace after screenshot prep changes tabs', async () => { const rendererExecuteJavaScriptMock = vi .fn() diff --git a/src/main/browser/browser-manager.ts b/src/main/browser/browser-manager.ts index 1f02c82239f..c7d7f958fbe 100644 --- a/src/main/browser/browser-manager.ts +++ b/src/main/browser/browser-manager.ts @@ -44,6 +44,74 @@ import { } from '../../shared/browser-annotation-viewport-bridge' import type { KeybindingOverrides } from '../../shared/keybindings' +const AUTOMATION_VISIBILITY_ACQUIRE_TIMEOUT_MS = 2_000 + +function resolveWithTimeout( + promise: Promise, + timeoutMs: number, + fallbackValue: T +): Promise<{ value: T; timedOut: boolean }> { + let timeoutId: ReturnType | null = null + const timeoutPromise = new Promise<{ value: T; timedOut: boolean }>((resolve) => { + timeoutId = setTimeout(() => resolve({ value: fallbackValue, timedOut: true }), timeoutMs) + }) + return Promise.race([ + promise.then((value) => ({ value, timedOut: false })), + timeoutPromise + ]).finally(() => { + if (timeoutId) { + clearTimeout(timeoutId) + } + }) +} + +function releaseAutomationVisibilityToken(renderer: Electron.WebContents, token: string): void { + if (renderer.isDestroyed()) { + return + } + renderer + .executeJavaScript( + `(function() { + var bridge = window.__orcaBrowserAutomationVisibility; + if (!bridge || typeof bridge.release !== 'function') return false; + return bridge.release(${JSON.stringify(token)}); + })()` + ) + .catch(() => {}) +} + +function cleanupLateAutomationVisibilityToken( + renderer: Electron.WebContents, + acquirePromise: Promise +): void { + acquirePromise + .then((lateToken) => { + if (typeof lateToken !== 'string' || lateToken.length === 0) { + return + } + // Why: the renderer creates the lease before waiting for paint; if main's + // acquire timeout wins, release the eventual token so hidden webviews do + // not stay paintable indefinitely. + releaseAutomationVisibilityToken(renderer, lateToken) + }) + .catch(() => {}) +} + +function createNoopRestoreForTimedOutAutomationAcquire( + renderer: Electron.WebContents, + acquirePromise: Promise, + timedOut: boolean +): () => void { + if (timedOut) { + cleanupLateAutomationVisibilityToken(renderer, acquirePromise) + } + return () => {} +} + +function isAutomationVisibilityToken(token: unknown): token is string { + return typeof token === 'string' && token.length > 0 +} + // Why: mobile presets need a touch-capable UA or responsive sites serve the // desktop variant based on UA sniffing. This is the Chrome DevTools default // iPhone UA template; we splice in the guest session's real Chrome major so @@ -437,6 +505,42 @@ export class BrowserManager { } } + async acquireAutomationVisibility(guestWebContentsId: number): Promise<() => void> { + const browserPageId = this.resolveBrowserTabIdForGuestWebContentsId(guestWebContentsId) + if (!browserPageId) { + return () => {} + } + const renderer = this.resolveRendererForBrowserTab(browserPageId) + if (!renderer || renderer.isDestroyed()) { + return () => {} + } + + // Why: agent browser commands need a paintable webview for lazy-loading + // sites, but must not steal the user's visible Orca tab/worktree. + const acquirePromise = renderer + .executeJavaScript( + `(async function() { + var bridge = window.__orcaBrowserAutomationVisibility; + if (!bridge || typeof bridge.acquire !== 'function') return null; + return await bridge.acquire(${JSON.stringify(browserPageId)}); + })()` + ) + .catch(() => null) + const { value: token, timedOut } = await resolveWithTimeout( + acquirePromise, + AUTOMATION_VISIBILITY_ACQUIRE_TIMEOUT_MS, + null + ) + + if (!isAutomationVisibilityToken(token)) { + return createNoopRestoreForTimedOutAutomationAcquire(renderer, acquirePromise, timedOut) + } + + return () => { + releaseAutomationVisibilityToken(renderer, token) + } + } + attachGuestPolicies(guest: Electron.WebContents): void { if (this.policyAttachedGuestIds.has(guest.id)) { return diff --git a/src/main/runtime/orca-runtime-browser.ts b/src/main/runtime/orca-runtime-browser.ts index 75e83286713..b6ef370e796 100644 --- a/src/main/runtime/orca-runtime-browser.ts +++ b/src/main/runtime/orca-runtime-browser.ts @@ -1,7 +1,6 @@ /* eslint-disable max-lines -- Why: this file is a command adapter for one external surface, Agent Browser automation. It stays separate from OrcaRuntimeService so runtime state does not grow further while browser routing remains easy to scan in one place. */ import { randomUUID } from 'crypto' import { ipcMain, webContents, type BrowserWindow } from 'electron' -import { getRepoIdFromWorktreeId } from '../../shared/worktree-id' import type { BrowserBackResult, BrowserCaptureStartResult, @@ -248,22 +247,14 @@ export class RuntimeBrowserCommands { return { browserPageId: resolvedPageId, webContents: guest } } - // Why: browser tabs only mount (and become operable) when their worktree is - // the active worktree in the renderer AND activeTabType is 'browser'. If either - // condition is false, the webview stays in display:none and Electron won't start - // its guest process — dom-ready never fires, registerGuest never runs, and CLI - // browser commands fail with "CDP connection refused". + // Why: browser tabs must become paintable before their webview guest starts + // and registerGuest fires, but automation must not steal the user's visible + // worktree/browser pane. Ask the renderer to background-mount the worktree and + // acquire a hidden automation visibility lease instead of activating the UI. private async ensureBrowserWorktreeActive(worktreeId: string): Promise { const win = this.host.getAuthoritativeWindow() - const repoId = getRepoIdFromWorktreeId(worktreeId) - if (!repoId) { - return - } - win.webContents.send('ui:activateWorktree', { repoId, worktreeId }) - // Why: switching worktree alone sets activeView='terminal'. Browser webviews - // won't mount until activeTabType is 'browser'. Send a second IPC to flip it. win.webContents.send('browser:activateView', { worktreeId }) - // Why: give the renderer time to mount the webview after switching worktrees. + // Why: give the renderer time to mount the hidden paintable webview. // The webview needs to attach and fire dom-ready before registerGuest runs. await new Promise((resolve) => setTimeout(resolve, 500)) } diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 34bd5fe8ad5..934c4914cff 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -7854,6 +7854,45 @@ describe('OrcaRuntimeService', () => { } }) + it('resolves an exact path selector when duplicate repo entries expose the same path', async () => { + const runtime = new OrcaRuntimeService(store) + const duplicatePath = '/tmp/workspaces/runtime-duplicate-selector' + const getRepos = vi.spyOn(store, 'getRepos').mockReturnValue([ + { + id: TEST_REPO_ID, + path: TEST_REPO_PATH, + displayName: 'repo', + badgeColor: 'blue', + addedAt: 1 + }, + { + id: 'repo-duplicate-entry', + path: '/tmp/repo-secondary-worktree', + displayName: 'repo-secondary-worktree', + badgeColor: 'red', + addedAt: 2 + } + ]) + vi.mocked(listWorktrees).mockResolvedValue([ + { + path: duplicatePath, + head: 'def', + branch: 'runtime-duplicate-selector', + isBare: false, + isMainWorktree: false + } + ]) + + try { + const worktree = await runtime.showManagedWorktree(`path:${duplicatePath}`) + + expect(worktree.id).toBe(`${TEST_REPO_ID}::${duplicatePath}`) + expect(worktree.path).toBe(duplicatePath) + } finally { + getRepos.mockRestore() + } + }) + it('keeps CLI-created worktrees successful when initial terminal creation fails', async () => { const runtime = new OrcaRuntimeService(store) const spawn = vi.fn().mockRejectedValue(new Error('pty unavailable')) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index e52cea9be6c..75967944fc3 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -9491,6 +9491,13 @@ export class OrcaRuntimeService { candidates = worktrees.filter((worktree) => runtimePathsEqual(worktree.path, selector.slice(5)) ) + if (candidates.length > 1) { + // Why: registering another worktree from the same Git repo makes git + // report the same physical worktree path under multiple repo IDs. + // A path selector is already exact, so prefer the first resolved row + // instead of surfacing a duplicate-registration ambiguity. + candidates = [candidates[0]] + } } else if (selector.startsWith('branch:')) { const branchSelector = selector.slice(7) candidates = worktrees.filter((worktree) => diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index 6d899a3dd62..ef0026a5920 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -328,7 +328,7 @@ export type BrowserApi = { onNavigationUpdate: ( callback: (event: { browserPageId: string; url: string; title: string }) => void ) => () => void - onActivateView: (callback: (data: { worktreeId: string }) => void) => () => void + onActivateView: (callback: (data: { worktreeId?: string }) => void) => () => void onPaneFocus: ( callback: (data: { worktreeId: string | null; browserPageId: string }) => void ) => () => void diff --git a/src/preload/index.ts b/src/preload/index.ts index d37ffd07723..ececccf1fd0 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -1712,8 +1712,8 @@ const api = { return () => ipcRenderer.removeListener('browser:navigation-update', listener) }, - onActivateView: (callback: (data: { worktreeId: string }) => void): (() => void) => { - const listener = (_event: Electron.IpcRendererEvent, data: { worktreeId: string }) => + onActivateView: (callback: (data: { worktreeId?: string }) => void): (() => void) => { + const listener = (_event: Electron.IpcRendererEvent, data: { worktreeId?: string }) => callback(data) ipcRenderer.on('browser:activateView', listener) return () => ipcRenderer.removeListener('browser:activateView', listener) diff --git a/src/renderer/src/components/Terminal.tsx b/src/renderer/src/components/Terminal.tsx index a4b431af48c..f10e94a9e5a 100644 --- a/src/renderer/src/components/Terminal.tsx +++ b/src/renderer/src/components/Terminal.tsx @@ -3,6 +3,7 @@ import React, { useEffect, useCallback, useMemo, useRef, useState, lazy, Suspense } from 'react' import { createPortal } from 'react-dom' import { toast } from 'sonner' +import { useShallow } from 'zustand/react/shallow' import { BACKGROUND_MOUNT_TERMINAL_WORKTREE_EVENT, TOGGLE_TERMINAL_PANE_EXPAND_EVENT, @@ -36,6 +37,7 @@ import type { TabGroupLayoutNode } from '../../../shared/types' import BrowserPane from './browser-pane/BrowserPane' import { destroyPersistentWebview } from './browser-pane/webview-registry' import BrowserPaneOverlayLayer from './browser-pane/BrowserPaneOverlayLayer' +import { useBrowserAutomationVisibilityForAny } from './browser-pane/browser-automation-visibility' import TerminalPaneOverlayLayer from './terminal-pane/TerminalPaneOverlayLayer' import { collectBrowserWebviewIds, @@ -1788,15 +1790,28 @@ const WorktreeSplitSurface = React.memo(function WorktreeSplitSurface({ shouldMeasureHiddenWorktree: boolean activityTerminalPortals: ActivityTerminalPortalTarget[] }): React.JSX.Element { + const browserPageIds = useAppStore( + useShallow((state) => + (state.browserTabsByWorktree[worktreeId] ?? []).flatMap((tab) => + tab.pageIds && tab.pageIds.length > 0 ? tab.pageIds : [tab.activePageId ?? tab.id] + ) + ) + ) + const hasAutomationVisibleBrowser = useBrowserAutomationVisibilityForAny(browserPageIds) + const shouldKeepPaintable = shouldMeasureHiddenWorktree || hasAutomationVisibleBrowser + return (
diff --git a/src/renderer/src/components/browser-pane/BrowserPane.tsx b/src/renderer/src/components/browser-pane/BrowserPane.tsx index 577d4e2180f..088156246bb 100644 --- a/src/renderer/src/components/browser-pane/BrowserPane.tsx +++ b/src/renderer/src/components/browser-pane/BrowserPane.tsx @@ -83,6 +83,7 @@ import { registeredWebContentsIds, webviewRegistry } from './webview-registry' +import { useBrowserAutomationVisiblePageIds } from './browser-automation-visibility' import type { BrowserDownloadRequestedEvent, BrowserDownloadProgressEvent, @@ -784,6 +785,20 @@ export default function BrowserPane({ const setBrowserPageUrl = useAppStore((s) => s.setBrowserPageUrl) const runtimeEnvironmentActive = Boolean(activeRuntimeEnvironmentId?.trim()) const activeBrowserPageId = activeBrowserPage?.id ?? null + const browserPageIds = useMemo(() => browserPages.map((page) => page.id), [browserPages]) + const automationVisiblePageIds = useBrowserAutomationVisiblePageIds(browserPageIds) + const renderedBrowserPages = useMemo(() => { + const pages: BrowserPageState[] = [] + if (activeBrowserPage) { + pages.push(activeBrowserPage) + } + for (const page of browserPages) { + if (page.id !== activeBrowserPage?.id && automationVisiblePageIds.has(page.id)) { + pages.push(page) + } + } + return pages + }, [activeBrowserPage, automationVisiblePageIds, browserPages]) const [activeBrowserDriver, setActiveBrowserDriver] = useState({ kind: 'idle' }) @@ -834,19 +849,22 @@ export default function BrowserPane({ return (
- {activeBrowserPage ? ( + {renderedBrowserPages.length > 0 ? (
- + {renderedBrowserPages.map((page) => ( + + ))} void onSetUrl: (tabId: string, url: string) => void }): React.JSX.Element { + const isPaintable = isActive || isAutomationVisible const containerRef = useRef(null) const addressBarInputRef = useRef(null) const webviewRef = useRef(null) @@ -4171,8 +4192,16 @@ function BrowserPagePane({