From f2ac9f29b20fc0f8d49bbb82dbb1538efdb7e4d4 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 23 Sep 2026 21:17:46 -0400 Subject: [PATCH] fix(browser): let pixel capture hold its own page drawn, without the desktop window (#22534) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(browser): let pixel capture hold its own page drawn, without the desktop window Screenshots were the last browser commands that still borrowed the desktop window: they took the per-page automation-visibility lease, which waits for two desktop-window animation frames (capped at 2 s) and never arrives when the window is minimized or throttled. Only pixel capture actually needs a page drawn — input, scripts, layout, the accessibility tree and PDF all work on a hidden page. Capture now takes a main-owned paint hold: a synchronous, per-page ref-count that tells the renderer one way (no reply awaited) to keep the page drawn and keeps the desktop renderer unthrottled while held. Both Orca's full-page capture and the agent-browser helper's screenshots take it in cdp-screenshot.ts and retry on a bounded schedule until the page answers with a frame; a CDP error fails fast. Deleted: the queue's needsPaint lease, the executeJavaScript acquire path and its two racing 2 s timeouts and late-token cleanup, the renderer's rAF wait and window bridge, the capture commands' own leases, the fixed 300/500 ms settle waits, and the global one-screenshot-at-a-time lock. Rebased onto main after #22528 landed; content identical to the reviewed branch head 7b390ed6a8. * fix(browser): probe for a frame instead of repeating the full capture Retrying a capture resent the caller's full request, so on an already drawn tall page (a full-page capture takes ~0.5 s) the 250 ms retry started a second full beyond-viewport capture while the first was still running. Measured on Electron 43: any later request makes a held page produce a frame, and that frame answers every pending capture with a full, correct image. So the capture is sent once and 1x1 probes follow until it answers; their results are ignored. Also report a detached debugger as detached rather than destroyed, and give the layout-metrics timeout its own "did not respond" message, since that request doesn't need a drawn page. --- config/scripts/ci-shard-timings.json | 2 +- ...owser-bridge-automation-visibility.test.ts | 420 ------------------ .../agent-browser-bridge-capture-commands.ts | 84 +--- ...-browser-bridge-capture-paint-hold.test.ts | 165 +++++++ ...ent-browser-bridge-interaction-commands.ts | 28 +- .../browser/agent-browser-bridge-lifecycle.ts | 2 +- .../browser/agent-browser-bridge-queue.ts | 20 +- ...t-browser-bridge-session-lifecycle.test.ts | 60 +++ .../agent-browser-bridge-state-commands.ts | 16 +- .../browser/agent-browser-bridge-state.ts | 2 - .../agent-browser-bridge-test-harness.ts | 1 - .../browser/agent-browser-bridge-types.ts | 2 - .../browser-manager-guest-visibility.test.ts | 150 ++----- src/main/browser/browser-manager-types.ts | 69 --- .../browser/browser-manager-visibility.ts | 64 ++- src/main/browser/cdp-page-capture-commands.ts | 23 +- src/main/browser/cdp-screenshot.test.ts | 337 +++++++++----- src/main/browser/cdp-screenshot.ts | 262 +++++------ .../browser/cdp-ws-proxy-focus-replay.test.ts | 4 +- src/main/browser/cdp-ws-proxy-test-harness.ts | 3 +- src/main/browser/cdp-ws-proxy.test.ts | 4 +- src/main/browser/cdp-ws-proxy.ts | 10 +- .../orca-runtime-close-mobile-session-tab.ts | 3 +- ...time-run-create-mobile-session-terminal.ts | 3 +- src/main/runtime/orca-runtime-runtime-id.ts | 4 - .../window/renderer-publication-throttle.ts | 3 + src/preload/api/browser-api.ts | 3 + ...er-bridge-page-interaction-and-sessions.ts | 10 + .../browser-automation-visibility.test.ts | 71 +-- .../browser-automation-visibility.ts | 66 --- ...rowser-guest-retention-site-census.test.ts | 3 +- .../browser-state-capture-paint-hold.test.ts | 79 ++++ .../ipc-events/browser-state-ipc-bridge.ts | 27 ++ .../src/hooks/useIpcEvents-lifecycle.test.ts | 2 + 34 files changed, 814 insertions(+), 1188 deletions(-) delete mode 100644 src/main/browser/agent-browser-bridge-automation-visibility.test.ts create mode 100644 src/main/browser/agent-browser-bridge-capture-paint-hold.test.ts create mode 100644 src/renderer/src/hooks/ipc-events/browser-state-capture-paint-hold.test.ts diff --git a/config/scripts/ci-shard-timings.json b/config/scripts/ci-shard-timings.json index 722ff71be5c..4b4b08f97b2 100644 --- a/config/scripts/ci-shard-timings.json +++ b/config/scripts/ci-shard-timings.json @@ -587,7 +587,7 @@ "src/main/bitbucket/pull-request-mappers.test.ts": 7, "src/main/bitbucket/repository-ref.test.ts": 24, "src/main/bitbucket/status-no-decrypt.test.ts": 171, - "src/main/browser/agent-browser-bridge-automation-visibility.test.ts": 91, + "src/main/browser/agent-browser-bridge-capture-paint-hold.test.ts": 91, "src/main/browser/agent-browser-bridge-command-transport.test.ts": 102, "src/main/browser/agent-browser-bridge-mouse-input.test.ts": 24, "src/main/browser/agent-browser-bridge-navigation.test.ts": 156, diff --git a/src/main/browser/agent-browser-bridge-automation-visibility.test.ts b/src/main/browser/agent-browser-bridge-automation-visibility.test.ts deleted file mode 100644 index 863364f9248..00000000000 --- a/src/main/browser/agent-browser-bridge-automation-visibility.test.ts +++ /dev/null @@ -1,420 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest' - -const { execFileMock, webContentsFromIdMock, existsSyncMock, readFileSyncMock, stdinWrites } = - vi.hoisted(() => ({ - execFileMock: vi.fn(), - webContentsFromIdMock: vi.fn(), - existsSyncMock: vi.fn(() => false), - readFileSyncMock: vi.fn(() => Buffer.from('')), - stdinWrites: [] as string[] - })) - -vi.mock('child_process', () => ({ execFile: execFileMock })) -vi.mock('fs', () => ({ - existsSync: existsSyncMock, - readFileSync: readFileSyncMock, - accessSync: vi.fn(), - chmodSync: vi.fn(), - constants: { X_OK: 1 } -})) -vi.mock('os', () => ({ platform: () => 'darwin', arch: () => 'arm64' })) -vi.mock('electron', () => { - return { - app: { getPath: vi.fn(() => '/app'), getAppPath: vi.fn(() => '/project'), isPackaged: false }, - webContents: { fromId: webContentsFromIdMock } - } -}) -const { CdpWsProxyMock } = vi.hoisted(() => { - const instances: unknown[] = [] - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const MockClass = vi.fn().mockImplementation(function (this: any, _wc: unknown) { - this._wc = _wc - this.start = vi.fn(async () => 'ws://127.0.0.1:9222') - this.stop = vi.fn(async () => {}) - this.getPort = vi.fn(() => 9222) - instances.push(this) - }) - return { CdpWsProxyMock: Object.assign(MockClass, { instances }) } -}) - -vi.mock('./cdp-ws-proxy', () => ({ - CdpWsProxy: CdpWsProxyMock -})) -vi.mock('./cdp-bridge', () => ({ - BrowserError: class BrowserError extends Error { - code: string - constructor(code: string, message: string) { - super(message) - this.code = code - } - } -})) - -import { AgentBrowserBridge } from './agent-browser-bridge' -import { - createSucceedWith, - mockBrowserManager, - mockWebContents, - overrideBridgeWebContentsLookup, - resetAgentBrowserBridgeMocks, - type ExecFileCallback -} from './agent-browser-bridge-test-harness' - -overrideBridgeWebContentsLookup(AgentBrowserBridge.prototype, webContentsFromIdMock) - -const succeedWith = createSucceedWith(execFileMock, stdinWrites) - -describe('AgentBrowserBridge', () => { - let bridge: AgentBrowserBridge - - beforeEach(() => { - resetAgentBrowserBridgeMocks({ - webContentsFromIdMock, - existsSyncMock, - readFileSyncMock, - stdinWrites, - cdpWsProxyInstances: CdpWsProxyMock.instances - }) - bridge = new AgentBrowserBridge(mockBrowserManager()) - bridge.setActiveTab(100) - }) - - it('does not lease automation visibility for ordinary commands', async () => { - const acquireAutomationVisibility = vi.fn(async () => () => {}) - const b = new AgentBrowserBridge( - mockBrowserManager(undefined, undefined, { acquireAutomationVisibility }) - ) - b.setActiveTab(100) - webContentsFromIdMock.mockReturnValue(mockWebContents(100)) - - succeedWith({ snapshot: 'tree' }) - await b.snapshot() - await b.click('@e1') - await b.mouseClick(10, 20) - - expect(acquireAutomationVisibility).not.toHaveBeenCalled() - }) - - it('acquires an automation visibility lease while running exec 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 releaseExec: (() => void) | null = null - execFileMock.mockImplementation( - (_bin: string, args: string[], _opts: unknown, cb: ExecFileCallback) => { - if (args.includes('close')) { - cb(null, JSON.stringify({ success: true, data: null }), '') - return - } - if (args.includes('screenshot')) { - lifecycleEvents.push('command-exec') - releaseExec = () => { - cb(null, JSON.stringify({ success: true, data: { ok: true } }), '') - } - return - } - cb(null, JSON.stringify({ success: true, data: { ok: true } }), '') - } - ) - - const exec = b.exec('screenshot') - - await vi.waitFor(() => { - expect(releaseExec).not.toBeNull() - }) - expect(lifecycleEvents).toEqual(['acquire-100', 'command-exec']) - expect(restore).not.toHaveBeenCalled() - - releaseExec!() - - await expect(exec).resolves.toEqual({ ok: true }) - expect(lifecycleEvents).toEqual(['acquire-100', 'command-exec', 'restore-100']) - }) - - it('re-resolves the page when a pdf lease re-registers the webview', async () => { - const tabs = new Map([['tab-1', 100]]) - const wc100 = mockWebContents(100) - const printToPDF = vi.fn(async () => Buffer.from('pdf')) - const wc200 = { - ...mockWebContents(200, 'https://example.com/reloaded', 'Reloaded'), - printToPDF - } - webContentsFromIdMock.mockImplementation((id: number) => { - if (id === 100) { - return wc100 - } - if (id === 200) { - return wc200 - } - return null - }) - - const acquireAutomationVisibility = vi.fn(async () => { - tabs.set('tab-1', 200) - return vi.fn() - }) - const b = new AgentBrowserBridge( - mockBrowserManager(tabs, undefined, { - acquireAutomationVisibility - }) - ) - b.setActiveTab(100) - - succeedWith(null) - await expect(b.pdf()).resolves.toEqual({ data: Buffer.from('pdf').toString('base64') }) - - expect(acquireAutomationVisibility).toHaveBeenCalledWith(100) - expect(printToPDF).toHaveBeenCalled() - const createdProxyIds = CdpWsProxyMock.instances.map( - (instance) => (instance as { _wc?: { id?: number } })._wc?.id - ) - // Why: the session is created when the command runs, after the lease, so the old guest never gets one. - expect(createdProxyIds).toEqual([200]) - }) - - it('rejects commands queued behind a leased command whose page swaps guests', async () => { - const tabs = new Map([['tab-1', 100]]) - const wc100 = mockWebContents(100) - const wc200 = { - ...mockWebContents(200, 'https://example.com/reloaded', 'Reloaded'), - printToPDF: vi.fn(async () => Buffer.from('pdf')) - } - webContentsFromIdMock.mockImplementation((id: number) => - id === 100 ? wc100 : id === 200 ? wc200 : null - ) - - let releaseLease: (() => void) | null = null - const acquireAutomationVisibility = vi.fn( - () => - new Promise<() => void>((resolve) => { - releaseLease = () => { - // Why: mirrors browser:registerGuest, which reports the new guest to the bridge in the same tick. - tabs.set('tab-1', 200) - void b.onProcessSwap('tab-1', 200, 100) - resolve(() => {}) - } - }) - ) - const b = new AgentBrowserBridge( - mockBrowserManager(tabs, undefined, { acquireAutomationVisibility }) - ) - b.setActiveTab(100) - succeedWith(null) - - const pdf = b.pdf(undefined, 'tab-1') - await vi.waitFor(() => expect(releaseLease).not.toBeNull()) - const click = b.mouseClick(10, 20, 'left', undefined, 'tab-1') - releaseLease!() - - await expect(click).rejects.toMatchObject({ code: 'browser_tab_closed' }) - await expect(pdf).resolves.toEqual({ data: Buffer.from('pdf').toString('base64') }) - expect(wc200.printToPDF).toHaveBeenCalled() - }) - - it('preserves intercept routes when automation visibility re-registers the webview', async () => { - const tabs = new Map([['tab-1', 100]]) - const wc100 = mockWebContents(100) - const wc200 = mockWebContents(200, 'https://example.com/reloaded', 'Reloaded') - webContentsFromIdMock.mockImplementation((id: number) => { - if (id === 100) { - return wc100 - } - if (id === 200) { - return wc200 - } - return null - }) - - let reregisterOnVisibility = false - const acquireAutomationVisibility = vi.fn(async () => { - if (reregisterOnVisibility) { - tabs.set('tab-1', 200) - void b.onProcessSwap('tab-1', 200, 100) - } - return vi.fn() - }) - const b = new AgentBrowserBridge( - mockBrowserManager(tabs, undefined, { - acquireAutomationVisibility - }) - ) - b.setActiveTab(100) - - const commandCalls: string[][] = [] - execFileMock.mockImplementation( - (_bin: string, args: string[], _opts: unknown, cb: ExecFileCallback) => { - commandCalls.push(args) - cb(null, JSON.stringify({ success: true, data: { ok: true } }), '') - } - ) - - await b.interceptEnable(['https://old.example/**']) - reregisterOnVisibility = true - await expect(b.exec('get title')).resolves.toEqual({ ok: true }) - - const routeCalls = commandCalls.filter( - (args) => args.includes('network') && args.includes('route') - ) - expect(routeCalls).toHaveLength(2) - expect(routeCalls.at(-1)).toContain('https://old.example/**') - expect(routeCalls.at(-1)).toContain('--cdp') - expect(routeCalls.at(-1)).toContain('9222') - }) - - it('serializes screenshot visibility prep across sessions', async () => { - vi.useFakeTimers() - try { - const tabs = new Map([ - ['tab-1', 1], - ['tab-2', 2] - ]) - const worktrees = new Map([ - ['tab-1', 'wt-1'], - ['tab-2', 'wt-2'] - ]) - const lifecycleEvents: string[] = [] - const acquireAutomationVisibilityMock = vi.fn(async (webContentsId: number) => { - lifecycleEvents.push(`acquire-${webContentsId}`) - return () => { - lifecycleEvents.push(`restore-${webContentsId}`) - } - }) - const wc1 = mockWebContents(1) - const wc2 = mockWebContents(2) - webContentsFromIdMock.mockImplementation((id: number) => - id === 1 ? wc1 : id === 2 ? wc2 : null - ) - existsSyncMock.mockReturnValue(true) - const screenshotBytes = Buffer.from('serialized-screenshot') - readFileSyncMock.mockReturnValue(screenshotBytes) - - const b = new AgentBrowserBridge( - mockBrowserManager(tabs, worktrees, { - acquireAutomationVisibility: acquireAutomationVisibilityMock - }) - ) - b.setActiveTab(1, 'wt-1') - b.setActiveTab(2, 'wt-2') - - let releaseFirstScreenshot: (() => void) | null = null - execFileMock.mockImplementation( - (_bin: string, args: string[], _opts: unknown, cb: ExecFileCallback) => { - if (args.includes('close')) { - cb(null, JSON.stringify({ success: true, data: null }), '') - return - } - if (args.includes('screenshot')) { - const sessionName = args[args.indexOf('--session') + 1] - lifecycleEvents.push(`command-${sessionName}`) - if (sessionName === 'orca-tab-tab-1' && !releaseFirstScreenshot) { - releaseFirstScreenshot = () => { - cb(null, JSON.stringify({ success: true, data: { path: '/tmp/tab-1.png' } }), '') - } - return - } - cb( - null, - JSON.stringify({ success: true, data: { path: `/tmp/${sessionName}.png` } }), - '' - ) - return - } - cb(null, JSON.stringify({ success: true, data: { ok: true } }), '') - } - ) - - const first = b.screenshot('png', 'wt-1') - const second = b.screenshot('png', 'wt-2') - - await Promise.resolve() - await Promise.resolve() - await vi.advanceTimersByTimeAsync(300) - - expect(lifecycleEvents).toContain('acquire-1') - expect(lifecycleEvents).toContain('command-orca-tab-tab-1') - expect(lifecycleEvents).not.toContain('acquire-2') - - expect(releaseFirstScreenshot).not.toBeNull() - releaseFirstScreenshot!() - await expect(first).resolves.toEqual({ - data: screenshotBytes.toString('base64'), - format: 'png' - }) - - await Promise.resolve() - await Promise.resolve() - - expect(lifecycleEvents.indexOf('restore-1')).toBeLessThan( - lifecycleEvents.indexOf('acquire-2') - ) - - await vi.advanceTimersByTimeAsync(300) - await expect(second).resolves.toEqual({ - data: screenshotBytes.toString('base64'), - format: 'png' - }) - } finally { - vi.useRealTimers() - } - }) - - it('captures full-page screenshots directly through CDP using CSS layout bounds', async () => { - vi.useFakeTimers() - try { - const wc = mockWebContents(100) - wc.debugger.sendCommand.mockImplementation((method: string) => { - if (method === 'Page.getLayoutMetrics') { - return Promise.resolve({ - cssContentSize: { width: 600.2, height: 900.4 }, - contentSize: { width: 1200.4, height: 1800.8 } - }) - } - if (method === 'Page.captureScreenshot') { - return Promise.resolve({ data: 'full-cdp-shot' }) - } - return Promise.resolve({}) - }) - webContentsFromIdMock.mockReturnValue(wc) - - execFileMock.mockImplementation( - (_bin: string, _args: string[], _opts: unknown, cb: ExecFileCallback) => { - cb(null, JSON.stringify({ success: true, data: null }), '') - } - ) - - const screenshotPromise = bridge.fullPageScreenshot('png') - await vi.advanceTimersByTimeAsync(500) - - await expect(screenshotPromise).resolves.toEqual({ - data: 'full-cdp-shot', - format: 'png' - }) - - expect(wc.debugger.sendCommand).toHaveBeenNthCalledWith(1, 'Page.getLayoutMetrics', {}) - expect(wc.debugger.sendCommand).toHaveBeenNthCalledWith(2, 'Page.captureScreenshot', { - format: 'png', - captureBeyondViewport: true, - clip: { x: 0, y: 0, width: 601, height: 901, scale: 1 } - }) - const screenshotCall = execFileMock.mock.calls.find((call: unknown[]) => - (call[1] as string[]).includes('screenshot') - ) - expect(screenshotCall).toBeUndefined() - } finally { - vi.useRealTimers() - } - }) -}) diff --git a/src/main/browser/agent-browser-bridge-capture-commands.ts b/src/main/browser/agent-browser-bridge-capture-commands.ts index 088e1d62c54..1e823a4f827 100644 --- a/src/main/browser/agent-browser-bridge-capture-commands.ts +++ b/src/main/browser/agent-browser-bridge-capture-commands.ts @@ -14,7 +14,10 @@ export abstract class AgentBrowserBridgeCaptureCommands extends AgentBrowserBrid ): Promise { // Why: agent-browser writes the screenshot to a temp file and returns its path; read it and return base64. return this.enqueueTargetedCommand(worktreeId, browserPageId, async (sessionName) => { - return this.captureScreenshotCommand(sessionName, ['screenshot'], 300, format) + return this.readScreenshotFromResult( + await this.execAgentBrowser(sessionName, ['screenshot']), + format + ) }) } @@ -23,13 +26,15 @@ export abstract class AgentBrowserBridgeCaptureCommands extends AgentBrowserBrid 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) => { + const wc = this.requireTargetWebContents(target) + try { + return await captureFullPageScreenshot(wc, format === 'jpeg' ? 'jpeg' : 'png', () => + this.browserManager.holdPaintForCapture(target.webContentsId) + ) + } catch (error) { + throw new BrowserError('browser_error', (error as Error).message) + } }) } @@ -45,69 +50,6 @@ export abstract class AgentBrowserBridgeCaptureCommands extends AgentBrowserBrid return { data, format: format === 'jpeg' ? 'jpeg' : 'png' } as BrowserScreenshotResult } - private async captureScreenshotCommand( - sessionName: string, - commandArgs: string[], - settleMs: number, - format?: string - ): Promise { - return this.withSerializedScreenshotAccess(async () => { - const session = this.sessions.get(sessionName) - const restore = session - ? await this.browserManager.acquireAutomationVisibility(session.webContentsId) - : () => {} - try { - // Why: let the compositor settle to a painted frame after the lease, inside the screenshot lock so another tab can't change lease state first. - await new Promise((r) => setTimeout(r, settleMs)) - const raw = await this.execAgentBrowser(sessionName, commandArgs) - return this.readScreenshotFromResult(raw, format) - } finally { - restore() - } - }) - } - - private async captureFullPageScreenshotCommand( - sessionName: string, - webContentsId: number, - settleMs: number, - format: 'png' | 'jpeg' - ): Promise { - return this.withSerializedScreenshotAccess(async () => { - const session = this.sessions.get(sessionName) - const restore = session - ? await this.browserManager.acquireAutomationVisibility(session.webContentsId) - : () => {} - try { - // Why: the guest compositor needs a beat to paint a fresh frame after becoming paintable, or CDP captures a stale surface. - await new Promise((r) => setTimeout(r, settleMs)) - const wc = this.getWebContents(webContentsId) - if (!wc) { - throw new BrowserError('browser_tab_not_found', 'Tab is no longer available') - } - return await captureFullPageScreenshot(wc, format) - } catch (error) { - throw new BrowserError('browser_error', (error as Error).message) - } finally { - restore() - } - }) - } - - private async withSerializedScreenshotAccess(execute: () => Promise): Promise { - const previousTurn = this.screenshotTurn.catch(() => {}) - let releaseTurn!: () => void - this.screenshotTurn = new Promise((resolve) => { - releaseTurn = resolve - }) - await previousTurn - try { - return await execute() - } finally { - releaseTurn() - } - } - async evaluate( expression: string, worktreeId?: string, diff --git a/src/main/browser/agent-browser-bridge-capture-paint-hold.test.ts b/src/main/browser/agent-browser-bridge-capture-paint-hold.test.ts new file mode 100644 index 00000000000..8871e481d6c --- /dev/null +++ b/src/main/browser/agent-browser-bridge-capture-paint-hold.test.ts @@ -0,0 +1,165 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +const { execFileMock, webContentsFromIdMock, existsSyncMock, readFileSyncMock, stdinWrites } = + vi.hoisted(() => ({ + execFileMock: vi.fn(), + webContentsFromIdMock: vi.fn(), + existsSyncMock: vi.fn(() => false), + readFileSyncMock: vi.fn(() => Buffer.from('')), + stdinWrites: [] as string[] + })) + +vi.mock('child_process', () => ({ execFile: execFileMock })) +vi.mock('fs', () => ({ + existsSync: existsSyncMock, + readFileSync: readFileSyncMock, + accessSync: vi.fn(), + chmodSync: vi.fn(), + constants: { X_OK: 1 } +})) +vi.mock('os', () => ({ platform: () => 'darwin', arch: () => 'arm64' })) +vi.mock('electron', () => { + return { + app: { getPath: vi.fn(() => '/app'), getAppPath: vi.fn(() => '/project'), isPackaged: false }, + webContents: { fromId: webContentsFromIdMock } + } +}) +const { CdpWsProxyMock } = vi.hoisted(() => { + const instances: unknown[] = [] + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const MockClass = vi.fn().mockImplementation(function (this: any, _wc: unknown) { + this._wc = _wc + this.start = vi.fn(async () => 'ws://127.0.0.1:9222') + this.stop = vi.fn(async () => {}) + this.getPort = vi.fn(() => 9222) + instances.push(this) + }) + return { CdpWsProxyMock: Object.assign(MockClass, { instances }) } +}) + +vi.mock('./cdp-ws-proxy', () => ({ + CdpWsProxy: CdpWsProxyMock +})) +vi.mock('./cdp-bridge', () => ({ + BrowserError: class BrowserError extends Error { + code: string + constructor(code: string, message: string) { + super(message) + this.code = code + } + } +})) + +import { AgentBrowserBridge } from './agent-browser-bridge' +import { + createSucceedWith, + mockBrowserManager, + mockWebContents, + overrideBridgeWebContentsLookup, + resetAgentBrowserBridgeMocks, + type ExecFileCallback +} from './agent-browser-bridge-test-harness' + +overrideBridgeWebContentsLookup(AgentBrowserBridge.prototype, webContentsFromIdMock) + +const succeedWith = createSucceedWith(execFileMock, stdinWrites) + +describe('AgentBrowserBridge', () => { + let bridge: AgentBrowserBridge + + beforeEach(() => { + resetAgentBrowserBridgeMocks({ + webContentsFromIdMock, + existsSyncMock, + readFileSyncMock, + stdinWrites, + cdpWsProxyInstances: CdpWsProxyMock.instances + }) + bridge = new AgentBrowserBridge(mockBrowserManager()) + bridge.setActiveTab(100) + }) + + it('never holds paint for commands that do not capture pixels', async () => { + const holdPaintForCapture = vi.fn(() => () => {}) + const printToPDF = vi.fn(async () => Buffer.from('pdf')) + const b = new AgentBrowserBridge( + mockBrowserManager(undefined, undefined, { holdPaintForCapture }) + ) + b.setActiveTab(100) + webContentsFromIdMock.mockReturnValue({ ...mockWebContents(100), printToPDF }) + + succeedWith({ snapshot: 'tree' }) + await b.snapshot() + await b.click('@e1') + await b.mouseClick(10, 20) + await b.exec('get title') + await expect(b.pdf()).resolves.toEqual({ data: Buffer.from('pdf').toString('base64') }) + + expect(holdPaintForCapture).not.toHaveBeenCalled() + }) + + it('gives the helper proxy a paint hold scoped to its page', async () => { + const release = vi.fn() + const holdPaintForCapture = vi.fn(() => release) + const b = new AgentBrowserBridge( + mockBrowserManager(undefined, undefined, { holdPaintForCapture }) + ) + b.setActiveTab(100) + webContentsFromIdMock.mockReturnValue(mockWebContents(100)) + + succeedWith({ snapshot: 'tree' }) + await b.snapshot() + + const holdPaint = CdpWsProxyMock.mock.calls[0]?.[1] + expect(holdPaint()).toBe(release) + expect(holdPaintForCapture).toHaveBeenCalledWith(100) + }) + + it('captures full-page screenshots directly through CDP using CSS layout bounds', async () => { + const wc = mockWebContents(100) + wc.debugger.sendCommand.mockImplementation((method: string) => { + if (method === 'Page.getLayoutMetrics') { + return Promise.resolve({ + cssContentSize: { width: 600.2, height: 900.4 }, + contentSize: { width: 1200.4, height: 1800.8 } + }) + } + if (method === 'Page.captureScreenshot') { + return Promise.resolve({ data: 'full-cdp-shot' }) + } + return Promise.resolve({}) + }) + webContentsFromIdMock.mockReturnValue(wc) + + execFileMock.mockImplementation( + (_bin: string, _args: string[], _opts: unknown, cb: ExecFileCallback) => { + cb(null, JSON.stringify({ success: true, data: null }), '') + } + ) + + const release = vi.fn() + const holdPaintForCapture = vi.fn(() => release) + const b = new AgentBrowserBridge( + mockBrowserManager(undefined, undefined, { holdPaintForCapture }) + ) + b.setActiveTab(100) + + await expect(b.fullPageScreenshot('png')).resolves.toEqual({ + data: 'full-cdp-shot', + format: 'png' + }) + + expect(wc.debugger.sendCommand).toHaveBeenNthCalledWith(1, 'Page.getLayoutMetrics', {}) + expect(wc.debugger.sendCommand).toHaveBeenNthCalledWith(2, 'Page.captureScreenshot', { + format: 'png', + captureBeyondViewport: true, + clip: { x: 0, y: 0, width: 601, height: 901, scale: 1 } + }) + expect(holdPaintForCapture).toHaveBeenCalledWith(100) + expect(release).toHaveBeenCalledTimes(1) + const screenshotCall = execFileMock.mock.calls.find((call: unknown[]) => + (call[1] as string[]).includes('screenshot') + ) + expect(screenshotCall).toBeUndefined() + }) +}) diff --git a/src/main/browser/agent-browser-bridge-interaction-commands.ts b/src/main/browser/agent-browser-bridge-interaction-commands.ts index a4c32f39f67..73445dbe0dc 100644 --- a/src/main/browser/agent-browser-bridge-interaction-commands.ts +++ b/src/main/browser/agent-browser-bridge-interaction-commands.ts @@ -240,21 +240,17 @@ export abstract class AgentBrowserBridgeInteractionCommands extends AgentBrowser async pdf(worktreeId?: string, browserPageId?: string): Promise { // Why: agent-browser's CDP printToPDF hangs in Electron webviews — use the native webContents.printToPDF(). - return this.enqueueTargetedCommand( - worktreeId, - browserPageId, - async (_sessionName, target) => { - const wc = this.getWebContents(target.webContentsId) - if (!wc) { - throw new BrowserError('browser_no_tab', 'Tab is no longer available') - } - const buffer = await wc.printToPDF({ - printBackground: true, - preferCSSPageSize: true - }) - return { data: buffer.toString('base64') } - }, - { needsPaint: true } - ) + // Printing lays the page out afresh, so it works on an undrawn page and needs no paint hold. + return this.enqueueTargetedCommand(worktreeId, browserPageId, async (_sessionName, target) => { + const wc = this.getWebContents(target.webContentsId) + if (!wc) { + throw new BrowserError('browser_no_tab', 'Tab is no longer available') + } + const buffer = await wc.printToPDF({ + printBackground: true, + preferCSSPageSize: true + }) + return { data: buffer.toString('base64') } + }) } } diff --git a/src/main/browser/agent-browser-bridge-lifecycle.ts b/src/main/browser/agent-browser-bridge-lifecycle.ts index 89e4bb2448f..fa7a0dbf0c3 100644 --- a/src/main/browser/agent-browser-bridge-lifecycle.ts +++ b/src/main/browser/agent-browser-bridge-lifecycle.ts @@ -106,7 +106,7 @@ export abstract class AgentBrowserBridgeLifecycle extends AgentBrowserBridgeRawP // Why: the daemon persists sessions (incl. CDP port) across restarts; close the stale one first or it ignores --cdp and hits the dead port. await this.closeStaleAgentBrowserSession(sessionName) - const proxy = new CdpWsProxy(wc) + const proxy = new CdpWsProxy(wc, () => this.browserManager.holdPaintForCapture(webContentsId)) const cdpEndpoint = await proxy.start() this.sessions.set(sessionName, { diff --git a/src/main/browser/agent-browser-bridge-queue.ts b/src/main/browser/agent-browser-bridge-queue.ts index 7678b6b7c7c..7c6bfe919e5 100644 --- a/src/main/browser/agent-browser-bridge-queue.ts +++ b/src/main/browser/agent-browser-bridge-queue.ts @@ -89,22 +89,12 @@ export abstract class AgentBrowserBridgeQueue extends AgentBrowserBridgeShutdown ): Promise { this.assertCommandAdmission() const sessionName = `${ORCA_TAB_SESSION_PREFIX}${browserPageId}` - // Why: inactive panes are display:none; the automation lease makes only this target paintable without selecting it. - const restore = options.needsPaint - ? await this.browserManager.acquireAutomationVisibility( - this.resolveCommandTarget(worktreeId, browserPageId).webContentsId - ) - : undefined - try { - // Why: the page's guest can change while queued; bind to the one current at execution. - const target = this.resolveCommandTarget(worktreeId, browserPageId) - if (options.ensureSession !== false) { - await this.ensureSession(sessionName, browserPageId, target.webContentsId) - } - return await execute(sessionName, target) - } finally { - restore?.() + // Why: the page's guest can change while queued; bind to the one current at execution. + const target = this.resolveCommandTarget(worktreeId, browserPageId) + if (options.ensureSession !== false) { + await this.ensureSession(sessionName, browserPageId, target.webContentsId) } + return execute(sessionName, target) } protected async processQueue(sessionName: string): Promise { diff --git a/src/main/browser/agent-browser-bridge-session-lifecycle.test.ts b/src/main/browser/agent-browser-bridge-session-lifecycle.test.ts index c122844eb43..8cb8dbe117a 100644 --- a/src/main/browser/agent-browser-bridge-session-lifecycle.test.ts +++ b/src/main/browser/agent-browser-bridge-session-lifecycle.test.ts @@ -374,6 +374,66 @@ describe('AgentBrowserBridge', () => { expect(lastSnapshotArgs).toContain('--cdp') }) + it('rejects commands queued behind a running command when the page swaps guests', async () => { + const tabs = new Map([['tab-1', 100]]) + const b = new AgentBrowserBridge(mockBrowserManager(tabs)) + b.setActiveTab(100) + webContentsFromIdMock.mockImplementation((id: number) => + id === 100 || id === 200 ? mockWebContents(id) : null + ) + + let finishSnapshot: (() => void) | null = null + execFileMock.mockImplementation( + (_bin: string, args: string[], _opts: unknown, cb: ExecFileCallback) => { + if (args.includes('snapshot')) { + finishSnapshot = () => + cb(null, JSON.stringify({ success: true, data: { snapshot: 'x' } }), '') + return { kill: vi.fn(() => finishSnapshot?.()) } + } + cb(null, JSON.stringify({ success: true, data: null }), '') + return { kill: vi.fn() } + } + ) + + const snapshot = b.snapshot(undefined, 'tab-1') + await vi.waitFor(() => expect(finishSnapshot).not.toBeNull()) + const click = b.mouseClick(10, 20, 'left', undefined, 'tab-1') + + tabs.set('tab-1', 200) + await b.onProcessSwap('tab-1', 200, 100) + + await expect(click).rejects.toMatchObject({ code: 'browser_tab_closed' }) + await snapshot.catch(() => {}) + }) + + it('replays saved intercept routes onto the new guest after a process swap', async () => { + const tabs = new Map([['tab-1', 100]]) + const b = new AgentBrowserBridge(mockBrowserManager(tabs)) + b.setActiveTab(100) + webContentsFromIdMock.mockImplementation((id: number) => + id === 100 || id === 200 ? mockWebContents(id) : null + ) + const commandCalls: string[][] = [] + execFileMock.mockImplementation( + (_bin: string, args: string[], _opts: unknown, cb: ExecFileCallback) => { + commandCalls.push(args) + cb(null, JSON.stringify({ success: true, data: { ok: true } }), '') + } + ) + + await b.interceptEnable(['https://old.example/**']) + tabs.set('tab-1', 200) + await b.onProcessSwap('tab-1', 200, 100) + await expect(b.exec('get title')).resolves.toEqual({ ok: true }) + + const routeCalls = commandCalls.filter( + (args) => args.includes('network') && args.includes('route') + ) + expect(routeCalls).toHaveLength(2) + expect(routeCalls.at(-1)).toContain('https://old.example/**') + expect(routeCalls.at(-1)).toContain('--cdp') + }) + it('does not replay stale intercept routes after process swap when the first command disables routing', async () => { const tabs = new Map([['tab-1', 100]]) const mgr = mockBrowserManager(tabs) diff --git a/src/main/browser/agent-browser-bridge-state-commands.ts b/src/main/browser/agent-browser-bridge-state-commands.ts index 597b56223c5..4e03055da57 100644 --- a/src/main/browser/agent-browser-bridge-state-commands.ts +++ b/src/main/browser/agent-browser-bridge-state-commands.ts @@ -257,16 +257,10 @@ export abstract class AgentBrowserBridgeStateCommands extends AgentBrowserBridge // ── Generic passthrough ── async exec(command: string, worktreeId?: string, browserPageId?: string): Promise { - return this.enqueueTargetedCommand( - worktreeId, - browserPageId, - async (sessionName) => { - // Why: strip target/session flags from passthrough so a caller can't override Orca's selected page or CDP proxy. - const args = stripAgentBrowserTargetArgs(parseShellArgs(command.trim())) - return await this.execAgentBrowser(sessionName, args) - }, - // Why: passthrough can run screenshot/record. - { needsPaint: true } - ) + return this.enqueueTargetedCommand(worktreeId, browserPageId, async (sessionName) => { + // Why: strip target/session flags from passthrough so a caller can't override Orca's selected page or CDP proxy. + const args = stripAgentBrowserTargetArgs(parseShellArgs(command.trim())) + return await this.execAgentBrowser(sessionName, args) + }) } } diff --git a/src/main/browser/agent-browser-bridge-state.ts b/src/main/browser/agent-browser-bridge-state.ts index c23cd8e5ac5..c309689cd8b 100644 --- a/src/main/browser/agent-browser-bridge-state.ts +++ b/src/main/browser/agent-browser-bridge-state.ts @@ -16,8 +16,6 @@ export abstract class AgentBrowserBridgeState { protected readonly sessions = new Map() protected readonly commandQueues = new Map() protected readonly processingQueues = new Set() - // Why: screenshot prep mutates shared paintability across tabs; serialize globally so concurrent captures don't blank each other. - protected screenshotTurn: Promise = Promise.resolve() protected readonly agentBrowserBin: string protected readonly agentBrowserEnv: NodeJS.ProcessEnv protected readonly ownsAgentBrowserSocketDirectory: boolean diff --git a/src/main/browser/agent-browser-bridge-test-harness.ts b/src/main/browser/agent-browser-bridge-test-harness.ts index 7aa38364a3c..5a861f3f552 100644 --- a/src/main/browser/agent-browser-bridge-test-harness.ts +++ b/src/main/browser/agent-browser-bridge-test-harness.ts @@ -33,7 +33,6 @@ export function mockBrowserManager( getBrowserPageCertificateFailure: vi.fn(() => null), unregisterGuest: vi.fn(), ensureWebviewVisible: vi.fn(async () => () => {}), - acquireAutomationVisibility: vi.fn(async () => () => {}), ...overrides } as unknown as BrowserManager } diff --git a/src/main/browser/agent-browser-bridge-types.ts b/src/main/browser/agent-browser-bridge-types.ts index 9c2edf56100..5d6e10e3078 100644 --- a/src/main/browser/agent-browser-bridge-types.ts +++ b/src/main/browser/agent-browser-bridge-types.ts @@ -55,8 +55,6 @@ export type AgentBrowserExecOptions = { export type EnqueueTargetedCommandOptions = { ensureSession?: boolean - // Why: only pixel capture needs a drawn page; input, JS, layout and snapshots work on a display:none page. Screenshots lease inside the screenshot lock instead. - needsPaint?: boolean // Why: text-mutating commands must never fall back to the global tab (may be a worktree the user is viewing). requireScopedTarget?: boolean } diff --git a/src/main/browser/browser-manager-guest-visibility.test.ts b/src/main/browser/browser-manager-guest-visibility.test.ts index 9f7f5013b82..17e90143a44 100644 --- a/src/main/browser/browser-manager-guest-visibility.test.ts +++ b/src/main/browser/browser-manager-guest-visibility.test.ts @@ -129,11 +129,7 @@ describe('browserManager', () => { restore() }) - it('acquires renderer automation visibility without changing active browser state', async () => { - const rendererExecuteJavaScriptMock = vi - .fn() - .mockResolvedValueOnce('lease-1') - .mockResolvedValueOnce(true) + it('holds capture paint one-way and keeps the renderer unthrottled until the last release', () => { const guest = { id: 1707, isDestroyed: vi.fn(() => false), @@ -147,7 +143,9 @@ describe('browserManager', () => { const renderer = { id: rendererWebContentsId, isDestroyed: vi.fn(() => false), - executeJavaScript: rendererExecuteJavaScriptMock + executeJavaScript: vi.fn(() => new Promise(() => {})), + send: vi.fn(), + setBackgroundThrottling: vi.fn() } webContentsFromIdMock.mockImplementation((id: number) => { if (id === guest.id) { @@ -158,135 +156,41 @@ describe('browserManager', () => { } return null }) - browserManager.attachGuestPolicies(guest as never) browserManager.registerGuest({ - browserPageId: 'page-automation', + browserPageId: 'page-capture', 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') + // Synchronous: a capture never waits on the desktop renderer. + const releaseFirst = browserManager.holdPaintForCapture(guest.id) + const releaseSecond = browserManager.holdPaintForCapture(guest.id) - restore() + expect(renderer.executeJavaScript).not.toHaveBeenCalled() + expect(renderer.send.mock.calls).toEqual([ + ['browser:capturePaintHold', { browserPageId: 'page-capture', held: true }] + ]) + expect(renderer.setBackgroundThrottling.mock.calls).toEqual([[false]]) - const releaseScript = rendererExecuteJavaScriptMock.mock.calls[1]?.[0] - expect(releaseScript).toContain('bridge.release("lease-1")') + releaseFirst() + releaseFirst() + expect(renderer.send).toHaveBeenCalledTimes(1) + expect(renderer.setBackgroundThrottling.mock.calls).toEqual([[false]]) + + releaseSecond() + expect(renderer.send.mock.calls.at(-1)).toEqual([ + 'browser:capturePaintHold', + { browserPageId: 'page-capture', held: false } + ]) + expect(renderer.setBackgroundThrottling.mock.calls).toEqual([[false], [true]]) }) - 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('returns a no-op capture hold for a guest no page owns', () => { + const release = browserManager.holdPaintForCapture(424242) + expect(() => release()).not.toThrow() }) it('restores the previously focused browser workspace after screenshot prep changes tabs', async () => { diff --git a/src/main/browser/browser-manager-types.ts b/src/main/browser/browser-manager-types.ts index 99f5280dd11..3d3825d6ebe 100644 --- a/src/main/browser/browser-manager-types.ts +++ b/src/main/browser/browser-manager-types.ts @@ -22,79 +22,10 @@ import type { import type { BrowserAnnotationViewportBridgeOptions } from '../../shared/browser-annotation-viewport-bridge' import type { KeybindingOverrides } from '../../shared/keybindings' -export const AUTOMATION_VISIBILITY_ACQUIRE_TIMEOUT_MS = 2_000 - export function isChromiumInternalErrorUrl(url: string): boolean { return url.startsWith('chrome-error://') } -export 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) - } - }) -} - -export 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(() => {}) -} - -export function cleanupLateAutomationVisibilityToken( - renderer: Electron.WebContents, - acquirePromise: Promise -): void { - acquirePromise - .then((lateToken) => { - if (typeof lateToken !== 'string' || lateToken.length === 0) { - return - } - // Why: the lease is created before paint; if main's acquire timed out, release the late token so hidden webviews don't stay paintable. - releaseAutomationVisibilityToken(renderer, lateToken) - }) - .catch(() => {}) -} - -export function createNoopRestoreForTimedOutAutomationAcquire( - renderer: Electron.WebContents, - acquirePromise: Promise, - timedOut: boolean -): () => void { - if (timedOut) { - cleanupLateAutomationVisibilityToken(renderer, acquirePromise) - } - return () => {} -} - -export function isAutomationVisibilityToken(token: unknown): token is string { - return typeof token === 'string' && token.length > 0 -} - export type BrowserGuestRegistration = { browserPageId?: string browserTabId?: string diff --git a/src/main/browser/browser-manager-visibility.ts b/src/main/browser/browser-manager-visibility.ts index c3566cc0d48..c51324b70db 100644 --- a/src/main/browser/browser-manager-visibility.ts +++ b/src/main/browser/browser-manager-visibility.ts @@ -1,10 +1,4 @@ -import { - AUTOMATION_VISIBILITY_ACQUIRE_TIMEOUT_MS, - createNoopRestoreForTimedOutAutomationAcquire, - isAutomationVisibilityToken, - releaseAutomationVisibilityToken, - resolveWithTimeout -} from './browser-manager-types' +import { rendererPublicationThrottle } from '../window/renderer-publication-throttle' import { BrowserManagerState } from './browser-manager-state' export abstract class BrowserManagerVisibility extends BrowserManagerState { @@ -219,38 +213,40 @@ export abstract class BrowserManagerVisibility extends BrowserManagerState { } } - async acquireAutomationVisibility(guestWebContentsId: number): Promise<() => void> { + // Why: page id -> active capture count; the renderer hears only the first hold and the last release. + private readonly capturePaintHolds = new Map() + + // Why: only pixel capture needs a drawn guest. One-way so a minimized or throttled desktop renderer + // can't stall the capture; the capture itself retries until the page produces a frame. + holdPaintForCapture(guestWebContentsId: number): () => void { const browserPageId = this.resolveBrowserTabIdForGuestWebContentsId(guestWebContentsId) - if (!browserPageId) { + const renderer = browserPageId ? this.resolveRendererForBrowserTab(browserPageId) : null + if (!browserPageId || !renderer || renderer.isDestroyed()) { return () => {} } - const renderer = this.resolveRendererForBrowserTab(browserPageId) - if (!renderer || renderer.isDestroyed()) { - return () => {} + const holds = this.capturePaintHolds.get(browserPageId) ?? 0 + this.capturePaintHolds.set(browserPageId, holds + 1) + if (holds === 0) { + renderer.send('browser:capturePaintHold', { browserPageId, held: true }) } - - // Why: pixel-capturing agent commands need a drawn webview without stealing the user's visible tab. - 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) - } - + // Why: a throttled renderer applies the parking change late, which every retry would pay for. + const releaseThrottle = rendererPublicationThrottle.acquire(renderer) + let released = false return () => { - releaseAutomationVisibilityToken(renderer, token) + if (released) { + return + } + released = true + releaseThrottle() + const remaining = (this.capturePaintHolds.get(browserPageId) ?? 1) - 1 + if (remaining > 0) { + this.capturePaintHolds.set(browserPageId, remaining) + return + } + this.capturePaintHolds.delete(browserPageId) + if (!renderer.isDestroyed()) { + renderer.send('browser:capturePaintHold', { browserPageId, held: false }) + } } } } diff --git a/src/main/browser/cdp-page-capture-commands.ts b/src/main/browser/cdp-page-capture-commands.ts index 599caffb0ff..4b8edd01121 100644 --- a/src/main/browser/cdp-page-capture-commands.ts +++ b/src/main/browser/cdp-page-capture-commands.ts @@ -1,6 +1,6 @@ import type { WebSocket } from 'ws' import type { WebContents } from 'electron' -import { captureScreenshot } from './cdp-screenshot' +import { captureScreenshot, type CapturePaintHold } from './cdp-screenshot' import { buildPrintToPdfOptions, CdpPdfStreamStore } from './cdp-print-to-pdf' import type { CdpClientResponseWriter } from './cdp-client-response-writer' @@ -13,7 +13,8 @@ export class CdpPageCaptureCommands { constructor( private readonly webContents: WebContents, - private readonly responder: CdpClientResponseWriter + private readonly responder: CdpClientResponseWriter, + private readonly holdPaint: CapturePaintHold ) {} clear(): void { @@ -71,12 +72,16 @@ export class CdpPageCaptureCommands { this.responder.sendResult(clientId, {}, client) } - handleScreenshot(client: WebSocket, clientId: number, params?: Record): void { - captureScreenshot( - this.webContents, - params, - (result) => this.responder.sendResult(clientId, result, client), - (message) => this.responder.sendError(clientId, message, client) - ) + async handleScreenshot( + client: WebSocket, + clientId: number, + params?: Record + ): Promise { + try { + const result = await captureScreenshot(this.webContents, params, this.holdPaint) + this.responder.sendResult(clientId, result, client) + } catch (err) { + this.responder.sendError(clientId, err instanceof Error ? err.message : String(err), client) + } } } diff --git a/src/main/browser/cdp-screenshot.test.ts b/src/main/browser/cdp-screenshot.test.ts index 97859f7297f..a304fd27b9c 100644 --- a/src/main/browser/cdp-screenshot.test.ts +++ b/src/main/browser/cdp-screenshot.test.ts @@ -1,9 +1,10 @@ +import type { WebContents } from 'electron' import { afterEach, describe, expect, it, vi } from 'vitest' import { captureFullPageScreenshot, captureScreenshot } from './cdp-screenshot' function createMockWebContents() { - return { + const mock = { isDestroyed: vi.fn(() => false), invalidate: vi.fn(), capturePage: vi.fn(), @@ -12,8 +13,18 @@ function createMockWebContents() { sendCommand: vi.fn() } } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the capture calls. + return Object.assign(mock, { guest: mock as unknown as WebContents }) } +const noHold = (): (() => void) => () => {} +const PROBE = { + format: 'jpeg', + quality: 1, + clip: { x: 0, y: 0, width: 1, height: 1, scale: 1 } +} +const TIMEOUT_MESSAGE = 'Screenshot timed out — the browser page did not draw a frame.' + describe('captureScreenshot', () => { afterEach(() => { vi.useRealTimers() @@ -22,82 +33,209 @@ describe('captureScreenshot', () => { it('invalidates the guest before forwarding Page.captureScreenshot', async () => { const webContents = createMockWebContents() webContents.debugger.sendCommand.mockResolvedValueOnce({ data: 'png-data' }) - const onResult = vi.fn() - const onError = vi.fn() - captureScreenshot(webContents as never, { format: 'png' }, onResult, onError) - await Promise.resolve() + await expect(captureScreenshot(webContents.guest, { format: 'png' }, noHold)).resolves.toEqual({ + data: 'png-data' + }) expect(webContents.invalidate).toHaveBeenCalledTimes(1) expect(webContents.debugger.sendCommand).toHaveBeenCalledWith('Page.captureScreenshot', { format: 'png' }) - expect(onResult).toHaveBeenCalledWith({ data: 'png-data' }) - expect(onError).not.toHaveBeenCalled() + }) + + it('holds paint for the capture and releases it when the capture fails', async () => { + vi.useFakeTimers() + const events: string[] = [] + const holdPaint = vi.fn(() => { + events.push('hold') + return () => events.push('release') + }) + const webContents = createMockWebContents() + webContents.debugger.sendCommand.mockImplementation(() => { + events.push('capture') + return new Promise(() => {}) + }) + webContents.capturePage.mockImplementation(() => new Promise(() => {})) + + const capture = captureScreenshot(webContents.guest, { format: 'png' }, holdPaint) + const settled = expect(capture).rejects.toThrow(TIMEOUT_MESSAGE) + await vi.advanceTimersByTimeAsync(9000) + await settled + + expect(events[0]).toBe('hold') + expect(events.at(-1)).toBe('release') + expect(events.filter((event) => event === 'release')).toHaveLength(1) + }) + + it('sends the capture once and probes until the held page produces a frame', async () => { + vi.useFakeTimers() + const webContents = createMockWebContents() + // Undrawn: the capture hangs until a later request makes the page draw a frame. + let drawFrame: (() => void) | null = null + webContents.debugger.sendCommand + .mockImplementationOnce( + () => + new Promise((resolve) => { + drawFrame = () => resolve({ data: 'drawn-png' }) + }) + ) + .mockImplementationOnce(() => new Promise(() => {})) + .mockImplementationOnce(() => { + drawFrame?.() + return Promise.resolve({ data: 'probe' }) + }) + + const capture = captureScreenshot(webContents.guest, { format: 'png' }, noHold) + await vi.advanceTimersByTimeAsync(750) + + await expect(capture).resolves.toEqual({ data: 'drawn-png' }) + expect(webContents.debugger.sendCommand.mock.calls).toEqual([ + ['Page.captureScreenshot', { format: 'png' }], + ['Page.captureScreenshot', PROBE], + ['Page.captureScreenshot', PROBE] + ]) + expect(webContents.capturePage).not.toHaveBeenCalled() + }) + + it('never repeats the full capture while a slow one is in flight', async () => { + vi.useFakeTimers() + let resolveCapture: ((value: unknown) => void) | null = null + const webContents = createMockWebContents() + webContents.debugger.sendCommand + .mockImplementationOnce( + () => + new Promise((resolve) => { + resolveCapture = resolve + }) + ) + .mockImplementation(() => new Promise(() => {})) + const fullPage = { format: 'png', captureBeyondViewport: true } + + const capture = captureScreenshot(webContents.guest, fullPage, noHold) + await vi.advanceTimersByTimeAsync(480) + resolveCapture!({ data: 'slow-png' }) + + await expect(capture).resolves.toEqual({ data: 'slow-png' }) + expect(webContents.debugger.sendCommand.mock.calls).toEqual([ + ['Page.captureScreenshot', fullPage], + ['Page.captureScreenshot', PROBE] + ]) + }) + + it('stops probing at the deadline', async () => { + vi.useFakeTimers() + const webContents = createMockWebContents() + webContents.debugger.sendCommand.mockImplementation(() => new Promise(() => {})) + webContents.capturePage.mockResolvedValue({ isEmpty: () => true }) + + const capture = captureScreenshot(webContents.guest, { format: 'png' }, noHold) + const settled = expect(capture).rejects.toThrow(TIMEOUT_MESSAGE) + await vi.advanceTimersByTimeAsync(8000) + await settled + + await vi.advanceTimersByTimeAsync(60_000) + expect(webContents.debugger.sendCommand.mock.calls).toEqual([ + ['Page.captureScreenshot', { format: 'png' }], + ['Page.captureScreenshot', PROBE], + ['Page.captureScreenshot', PROBE], + ['Page.captureScreenshot', PROBE], + ['Page.captureScreenshot', PROBE] + ]) + }) + + it('fails at once on a CDP error, without retrying or falling back', async () => { + vi.useFakeTimers() + const webContents = createMockWebContents() + webContents.debugger.sendCommand.mockRejectedValue(new Error('Target closed')) + + const capture = captureScreenshot(webContents.guest, { format: 'png' }, noHold) + const settled = expect(capture).rejects.toThrow('Target closed') + await vi.advanceTimersByTimeAsync(0) + await settled + + await vi.advanceTimersByTimeAsync(8000) + expect(webContents.debugger.sendCommand).toHaveBeenCalledTimes(1) + expect(webContents.capturePage).not.toHaveBeenCalled() + }) + + it('stops retrying once the guest is destroyed', async () => { + vi.useFakeTimers() + const webContents = createMockWebContents() + webContents.debugger.sendCommand.mockImplementation(() => new Promise(() => {})) + + const capture = captureScreenshot(webContents.guest, { format: 'png' }, noHold) + const settled = expect(capture).rejects.toThrow('WebContents destroyed') + await vi.advanceTimersByTimeAsync(0) + webContents.isDestroyed.mockReturnValue(true) + await vi.advanceTimersByTimeAsync(250) + await settled + + expect(webContents.debugger.sendCommand).toHaveBeenCalledTimes(1) + }) + + it('reports a detached debugger as detached', async () => { + vi.useFakeTimers() + const webContents = createMockWebContents() + webContents.debugger.sendCommand.mockImplementation(() => new Promise(() => {})) + + const capture = captureScreenshot(webContents.guest, { format: 'png' }, noHold) + const settled = expect(capture).rejects.toThrow('Debugger detached') + await vi.advanceTimersByTimeAsync(0) + webContents.debugger.isAttached.mockReturnValue(false) + await vi.advanceTimersByTimeAsync(250) + await settled }) it('falls back to capturePage when Page.captureScreenshot stalls', async () => { vi.useFakeTimers() - const webContents = createMockWebContents() webContents.debugger.sendCommand.mockImplementation(() => new Promise(() => {})) webContents.capturePage.mockResolvedValueOnce({ isEmpty: () => false, toPNG: () => Buffer.from('fallback-png') }) - const onResult = vi.fn() - const onError = vi.fn() - captureScreenshot(webContents as never, { format: 'png' }, onResult, onError) + const capture = captureScreenshot(webContents.guest, { format: 'png' }, noHold) await vi.advanceTimersByTimeAsync(8000) - expect(webContents.capturePage).toHaveBeenCalledTimes(1) - expect(onResult).toHaveBeenCalledWith({ + await expect(capture).resolves.toEqual({ data: Buffer.from('fallback-png').toString('base64') }) - expect(onError).not.toHaveBeenCalled() + expect(webContents.capturePage).toHaveBeenCalledTimes(1) }) it('crops the fallback image when the request includes a visible clip rect', async () => { vi.useFakeTimers() - const croppedImage = { isEmpty: () => false, toPNG: () => Buffer.from('cropped-png') } const webContents = createMockWebContents() webContents.debugger.sendCommand.mockImplementation(() => new Promise(() => {})) + const crop = vi.fn(() => croppedImage) webContents.capturePage.mockResolvedValueOnce({ isEmpty: () => false, getSize: () => ({ width: 400, height: 300 }), - crop: vi.fn(() => croppedImage), + crop, toPNG: () => Buffer.from('full-png') }) - const onResult = vi.fn() - const onError = vi.fn() - captureScreenshot( - webContents as never, - { - format: 'png', - clip: { x: 10, y: 20, width: 30, height: 40, scale: 2 } - }, - onResult, - onError + const capture = captureScreenshot( + webContents.guest, + { format: 'png', clip: { x: 10, y: 20, width: 100, height: 50, scale: 2 } }, + noHold ) await vi.advanceTimersByTimeAsync(8000) - const fallbackImage = await webContents.capturePage.mock.results[0]?.value - expect(fallbackImage.crop).toHaveBeenCalledWith({ x: 20, y: 40, width: 60, height: 80 }) - expect(onResult).toHaveBeenCalledWith({ + await expect(capture).resolves.toEqual({ data: Buffer.from('cropped-png').toString('base64') }) - expect(onError).not.toHaveBeenCalled() + expect(crop).toHaveBeenCalledWith({ x: 20, y: 40, width: 200, height: 100 }) }) it('keeps the timeout error when the request needs beyond-viewport pixels', async () => { vi.useFakeTimers() - const webContents = createMockWebContents() webContents.debugger.sendCommand.mockImplementation(() => new Promise(() => {})) webContents.capturePage.mockResolvedValueOnce({ @@ -106,93 +244,35 @@ describe('captureScreenshot', () => { crop: vi.fn(), toPNG: () => Buffer.from('full-png') }) - const onResult = vi.fn() - const onError = vi.fn() - captureScreenshot( - webContents as never, + const capture = captureScreenshot( + webContents.guest, { format: 'png', captureBeyondViewport: true, clip: { x: 0, y: 0, width: 800, height: 1200, scale: 1 } }, - onResult, - onError + noHold ) + const settled = expect(capture).rejects.toThrow(TIMEOUT_MESSAGE) await vi.advanceTimersByTimeAsync(8000) - - expect(onResult).not.toHaveBeenCalled() - expect(onError).toHaveBeenCalledWith( - 'Screenshot timed out — the browser tab may not be visible or the window may not have focus.' - ) + await settled }) - it('ignores the fallback result when CDP settles first after the timeout fires', async () => { + it('reports the original timeout when the fallback capture is empty', async () => { vi.useFakeTimers() - - let resolveCapturePage: ((value: unknown) => void) | null = null - let resolveSendCommand: ((value: unknown) => void) | null = null - const webContents = createMockWebContents() - webContents.debugger.sendCommand.mockImplementation( - () => - new Promise((resolve) => { - resolveSendCommand = resolve - }) - ) - webContents.capturePage.mockImplementation( - () => - new Promise((resolve) => { - resolveCapturePage = resolve - }) - ) - const onResult = vi.fn() - const onError = vi.fn() - - captureScreenshot(webContents as never, { format: 'png' }, onResult, onError) - await vi.advanceTimersByTimeAsync(8000) - - expect(resolveSendCommand).toBeTypeOf('function') - resolveSendCommand!({ data: 'cdp-png' }) - await Promise.resolve() - - expect(resolveCapturePage).toBeTypeOf('function') - resolveCapturePage!({ - isEmpty: () => false, - getSize: () => ({ width: 100, height: 100 }), - crop: vi.fn(), - toPNG: () => Buffer.from('fallback-png') - }) - await Promise.resolve() - - expect(onResult).toHaveBeenCalledTimes(1) - expect(onResult).toHaveBeenCalledWith({ data: 'cdp-png' }) - expect(onError).not.toHaveBeenCalled() - }) - - it('reports the original timeout when the fallback capture is unavailable', async () => { - vi.useFakeTimers() - const webContents = createMockWebContents() webContents.debugger.sendCommand.mockImplementation(() => new Promise(() => {})) - webContents.capturePage.mockResolvedValueOnce({ - isEmpty: () => true, - toPNG: () => Buffer.from('unused') - }) - const onResult = vi.fn() - const onError = vi.fn() + webContents.capturePage.mockResolvedValueOnce({ isEmpty: () => true }) - captureScreenshot(webContents as never, { format: 'png' }, onResult, onError) + const capture = captureScreenshot(webContents.guest, { format: 'png' }, noHold) + const settled = expect(capture).rejects.toThrow(TIMEOUT_MESSAGE) await vi.advanceTimersByTimeAsync(8000) - - expect(onResult).not.toHaveBeenCalled() - expect(onError).toHaveBeenCalledWith( - 'Screenshot timed out — the browser tab may not be visible or the window may not have focus.' - ) + await settled }) it('reports the original timeout when fallback encoding fails', async () => { vi.useFakeTimers() - const webContents = createMockWebContents() webContents.debugger.sendCommand.mockImplementation(() => new Promise(() => {})) webContents.capturePage.mockResolvedValueOnce({ @@ -200,43 +280,60 @@ describe('captureScreenshot', () => { throw new Error('native image unavailable') } }) - const onResult = vi.fn() - const onError = vi.fn() - captureScreenshot(webContents as never, { format: 'png' }, onResult, onError) + const capture = captureScreenshot(webContents.guest, { format: 'png' }, noHold) + const settled = expect(capture).rejects.toThrow(TIMEOUT_MESSAGE) await vi.advanceTimersByTimeAsync(8000) - - expect(onResult).not.toHaveBeenCalled() - expect(onError).toHaveBeenCalledWith( - 'Screenshot timed out — the browser tab may not be visible or the window may not have focus.' - ) + await settled }) it('reports the timeout when both CDP and fallback capture stall', async () => { vi.useFakeTimers() - const webContents = createMockWebContents() webContents.debugger.sendCommand.mockImplementation(() => new Promise(() => {})) webContents.capturePage.mockImplementation(() => new Promise(() => {})) - const onResult = vi.fn() - const onError = vi.fn() + const onSettled = vi.fn() - captureScreenshot(webContents as never, { format: 'png' }, onResult, onError) + const capture = captureScreenshot(webContents.guest, { format: 'png' }, noHold) + capture.catch(onSettled) await vi.advanceTimersByTimeAsync(8000) - expect(webContents.capturePage).toHaveBeenCalledTimes(1) - expect(onResult).not.toHaveBeenCalled() - expect(onError).not.toHaveBeenCalled() + expect(onSettled).not.toHaveBeenCalled() await vi.advanceTimersByTimeAsync(1000) - - expect(onError).toHaveBeenCalledWith( - 'Screenshot timed out — the browser tab may not be visible or the window may not have focus.' - ) + await expect(capture).rejects.toThrow(TIMEOUT_MESSAGE) }) }) describe('captureFullPageScreenshot', () => { + afterEach(() => { + vi.useRealTimers() + }) + + it('reports an unanswered layout request as unresponsive, not undrawn', async () => { + vi.useFakeTimers() + const webContents = createMockWebContents() + webContents.debugger.sendCommand.mockImplementation(() => new Promise(() => {})) + + const capture = captureFullPageScreenshot(webContents.guest, 'png', noHold) + const settled = expect(capture).rejects.toThrow( + 'Screenshot timed out — the browser page did not respond.' + ) + await vi.advanceTimersByTimeAsync(8000) + await settled + }) + + it('releases its paint hold when the page cannot be measured', async () => { + const release = vi.fn() + const webContents = createMockWebContents() + webContents.debugger.sendCommand.mockRejectedValue(new Error('Target closed')) + + await expect( + captureFullPageScreenshot(webContents.guest, 'png', () => release) + ).rejects.toThrow('Target closed') + expect(release).toHaveBeenCalledTimes(1) + }) + it('uses cssContentSize so HiDPI pages are captured at the real page size', async () => { const webContents = createMockWebContents() webContents.debugger.sendCommand.mockImplementation((method: string) => { @@ -252,7 +349,7 @@ describe('captureFullPageScreenshot', () => { return Promise.resolve({}) }) - await expect(captureFullPageScreenshot(webContents as never, 'png')).resolves.toEqual({ + await expect(captureFullPageScreenshot(webContents.guest, 'png', noHold)).resolves.toEqual({ data: 'full-page-data', format: 'png' }) @@ -278,7 +375,7 @@ describe('captureFullPageScreenshot', () => { return Promise.resolve({}) }) - await expect(captureFullPageScreenshot(webContents as never, 'jpeg')).resolves.toEqual({ + await expect(captureFullPageScreenshot(webContents.guest, 'jpeg', noHold)).resolves.toEqual({ data: 'legacy-full-page-data', format: 'jpeg' }) diff --git a/src/main/browser/cdp-screenshot.ts b/src/main/browser/cdp-screenshot.ts index 7ba4315d7d9..dc6fcc1d6fd 100644 --- a/src/main/browser/cdp-screenshot.ts +++ b/src/main/browser/cdp-screenshot.ts @@ -1,9 +1,19 @@ import type { WebContents } from 'electron' +/** Draws a hidden guest for the duration of a capture; returns its release. */ +export type CapturePaintHold = () => () => void + const SCREENSHOT_TIMEOUT_MS = 8000 +// Why: offsets from the capture start; the last leaves a full-page capture (~0.5 s on a tall page) time before the deadline. +const FRAME_PROBE_OFFSETS_MS = [250, 750, 1750, 3750] +// Why: a 1x1 request is cheap; the frame it makes the page produce also answers the pending capture. +const FRAME_PROBE_PARAMS = { + format: 'jpeg', + quality: 1, + clip: { x: 0, y: 0, width: 1, height: 1, scale: 1 } +} const FALLBACK_CAPTURE_TIMEOUT_MS = 1000 -const SCREENSHOT_TIMEOUT_MESSAGE = - 'Screenshot timed out — the browser tab may not be visible or the window may not have focus.' +const SCREENSHOT_TIMEOUT_MESSAGE = 'Screenshot timed out — the browser page did not draw a frame.' function applyFallbackClip( image: Electron.NativeImage, @@ -109,88 +119,122 @@ function getLayoutClip(metrics: { } } -async function sendCommandWithTimeout( - webContents: WebContents, - method: string, - params: Record | undefined, - timeoutMessage: string -): Promise { +function withTimeout(promise: Promise, timeoutMs: number, message: string): Promise { let timer: NodeJS.Timeout | null = null - try { - return await Promise.race([ - webContents.debugger.sendCommand(method, params ?? {}) as Promise, - new Promise((_, reject) => { - timer = setTimeout(() => reject(new Error(timeoutMessage)), SCREENSHOT_TIMEOUT_MS) - }) - ]) - } finally { + return Promise.race([ + promise, + new Promise((_, reject) => { + timer = setTimeout(() => reject(new Error(message)), timeoutMs) + }) + ]).finally(() => { if (timer) { clearTimeout(timer) } - } + }) +} + +// Why: a request made before the held page is drawn never resolves, and an offscreen drawn page can +// skip one; a later request makes the page produce a frame, which answers every pending request. +// So the capture is sent once and cheap probes follow until it answers. Resolves null when no frame +// arrives by the deadline; a CDP error is an answer. Unanswered probes settle on the next frame or +// reject on detach. +function captureUntilDrawn( + webContents: WebContents, + params: Record +): Promise<{ data: string } | null> { + return new Promise((resolve, reject) => { + let settled = false + const finish = (settle: () => void): void => { + if (settled) { + return + } + settled = true + clearTimeout(deadline) + probes.forEach(clearTimeout) + settle() + } + const send = ( + requestParams: Record + ): Promise<{ data?: string } | undefined> | null => { + if (webContents.isDestroyed()) { + finish(() => reject(new Error('WebContents destroyed'))) + return null + } + if (!webContents.debugger.isAttached()) { + finish(() => reject(new Error('Debugger detached'))) + return null + } + try { + webContents.invalidate() + } catch { + // Some guest teardown paths reject repaint requests. Fall through to CDP. + } + return webContents.debugger.sendCommand('Page.captureScreenshot', requestParams) + } + const deadline = setTimeout(() => finish(() => resolve(null)), SCREENSHOT_TIMEOUT_MS) + const probes = FRAME_PROBE_OFFSETS_MS.map((offsetMs) => + setTimeout(() => send(FRAME_PROBE_PARAMS)?.catch(() => {}), offsetMs) + ) + send(params)?.then( + (result) => finish(() => resolve(result?.data ? { data: result.data } : null)), + (error: unknown) => + finish(() => reject(error instanceof Error ? error : new Error(String(error)))) + ) + }) } export async function captureFullPageScreenshot( webContents: WebContents, - format: 'png' | 'jpeg' = 'png' + format: 'png' | 'jpeg', + holdPaint: CapturePaintHold ): Promise<{ data: string; format: 'png' | 'jpeg' }> { if (webContents.isDestroyed()) { throw new Error('WebContents destroyed') } - const dbg = webContents.debugger - if (!dbg.isAttached()) { + if (!webContents.debugger.isAttached()) { throw new Error('Debugger not attached') } + const release = holdPaint() try { - webContents.invalidate() - } catch { - // Some guest teardown paths reject repaint requests. Fall through to CDP. - } - - const metrics = await sendCommandWithTimeout<{ - cssContentSize?: { width?: number; height?: number } - contentSize?: { width?: number; height?: number } - }>(webContents, 'Page.getLayoutMetrics', undefined, SCREENSHOT_TIMEOUT_MESSAGE) - const clip = getLayoutClip(metrics) - if (!clip) { - throw new Error('Unable to determine full-page screenshot bounds') - } - - const { data } = await sendCommandWithTimeout<{ data: string }>( - webContents, - 'Page.captureScreenshot', - { + // Why: layout works on an undrawn page, so only the pixel capture waits for a frame. + const layoutMetrics: Promise[0]> = + webContents.debugger.sendCommand('Page.getLayoutMetrics', {}) + const metrics = await withTimeout( + layoutMetrics, + SCREENSHOT_TIMEOUT_MS, + 'Screenshot timed out — the browser page did not respond.' + ) + const clip = getLayoutClip(metrics) + if (!clip) { + throw new Error('Unable to determine full-page screenshot bounds') + } + const frame = await captureUntilDrawn(webContents, { format, captureBeyondViewport: true, clip - }, - SCREENSHOT_TIMEOUT_MESSAGE - ) - - return { data, format } + }) + if (!frame) { + throw new Error(SCREENSHOT_TIMEOUT_MESSAGE) + } + return { data: frame.data, format } + } finally { + release() + } } -// Why: Electron's capturePage() is unreliable on webview guests — the compositor -// may not produce frames when the webview panel is inactive, unfocused, or in a -// split-pane layout. Instead, use the debugger's Page.captureScreenshot which -// renders server-side in the Blink compositor and doesn't depend on OS-level -// window focus or display state. Guard with a timeout so agent-browser doesn't -// hang on its 30s CDP timeout if the debugger stalls. -export function captureScreenshot( +// Why: Page.captureScreenshot honours clip and beyond-viewport params that capturePage() can't. +// Bounded so agent-browser doesn't hang on its 30s CDP timeout if the debugger stalls. +export async function captureScreenshot( webContents: WebContents, params: Record | undefined, - onResult: (result: unknown) => void, - onError: (message: string) => void -): void { + holdPaint: CapturePaintHold +): Promise<{ data: string }> { if (webContents.isDestroyed()) { - onError('WebContents destroyed') - return + throw new Error('WebContents destroyed') } - const dbg = webContents.debugger - if (!dbg.isAttached()) { - onError('Debugger not attached') - return + if (!webContents.debugger.isAttached()) { + throw new Error('Debugger not attached') } const screenshotParams: Record = {} @@ -210,89 +254,25 @@ export function captureScreenshot( screenshotParams.fromSurface = params.fromSurface } - let settled = false - let timeoutTimer: ReturnType | null = null - let fallbackTimer: ReturnType | null = null - const clearTimers = (): void => { - if (timeoutTimer) { - clearTimeout(timeoutTimer) - timeoutTimer = null - } - if (fallbackTimer) { - clearTimeout(fallbackTimer) - fallbackTimer = null - } - } - const settleResult = (result: unknown): void => { - if (settled) { - return - } - settled = true - clearTimers() - onResult(result) - } - const settleError = (message: string): void => { - if (settled) { - return - } - settled = true - clearTimers() - onError(message) - } - // Why: a compositor invalidate is cheap and can recover guest instances that - // are visible but have not produced a fresh frame since being reclaimed into - // the active browser tab. + const release = holdPaint() try { - webContents.invalidate() - } catch { - // Some guest teardown paths reject repaint requests. Fall through to CDP. - } - timeoutTimer = setTimeout(() => { - if (settled) { - return + const frame = await captureUntilDrawn(webContents, screenshotParams) + if (frame) { + return frame } - // Why: capturePage is only a best-effort fallback. If it also stalls, the - // CDP proxy must still settle instead of inheriting the compositor hang. - fallbackTimer = setTimeout( - () => settleError(SCREENSHOT_TIMEOUT_MESSAGE), - FALLBACK_CAPTURE_TIMEOUT_MS + // Why: capturePage is only a best-effort fallback for a page that never answered. + const fallback = await withTimeout( + Promise.resolve().then(() => webContents.capturePage()), + FALLBACK_CAPTURE_TIMEOUT_MS, + SCREENSHOT_TIMEOUT_MESSAGE ) - void Promise.resolve() - .then(() => webContents.capturePage()) - .then( - (image) => { - if (settled) { - return - } - if (fallbackTimer) { - clearTimeout(fallbackTimer) - fallbackTimer = null - } - let fallback: { data: string } | null = null - try { - fallback = encodeNativeImageScreenshot(image, params) - } catch { - settleError(SCREENSHOT_TIMEOUT_MESSAGE) - return - } - if (fallback) { - settleResult(fallback) - return - } - settleError(SCREENSHOT_TIMEOUT_MESSAGE) - }, - () => { - if (fallbackTimer) { - clearTimeout(fallbackTimer) - fallbackTimer = null - } - settleError(SCREENSHOT_TIMEOUT_MESSAGE) - } - ) - }, SCREENSHOT_TIMEOUT_MS) - - dbg - .sendCommand('Page.captureScreenshot', screenshotParams) - .then((result) => settleResult(result)) - .catch((err) => settleError((err as Error).message)) + .then((image) => encodeNativeImageScreenshot(image, params)) + .catch(() => null) + if (fallback) { + return fallback + } + throw new Error(SCREENSHOT_TIMEOUT_MESSAGE) + } finally { + release() + } } diff --git a/src/main/browser/cdp-ws-proxy-focus-replay.test.ts b/src/main/browser/cdp-ws-proxy-focus-replay.test.ts index 8404614ef8d..cc87d2c7b90 100644 --- a/src/main/browser/cdp-ws-proxy-focus-replay.test.ts +++ b/src/main/browser/cdp-ws-proxy-focus-replay.test.ts @@ -23,8 +23,8 @@ describe('CdpWsProxy DOM.focus replay', () => { beforeEach(async () => { mock = createMockWebContents() - // eslint-disable-next-line @typescript-eslint/no-explicit-any - proxy = new CdpWsProxy(mock.webContents as any) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the proxy calls. + proxy = new CdpWsProxy(mock.webContents as never, () => () => {}) endpoint = await proxy.start() }) diff --git a/src/main/browser/cdp-ws-proxy-test-harness.ts b/src/main/browser/cdp-ws-proxy-test-harness.ts index 934c859c384..c66a3cbf6b1 100644 --- a/src/main/browser/cdp-ws-proxy-test-harness.ts +++ b/src/main/browser/cdp-ws-proxy-test-harness.ts @@ -51,7 +51,8 @@ export function createMockWebContents(): MockWebContents { debuggerAttached = false }), sendCommand: vi.fn( - async (_method?: string, _params?: Record, _sessionId?: string) => ({}) + async (method?: string, _params?: Record, _sessionId?: string) => + method === 'Page.captureScreenshot' ? { data: 'png' } : {} ), on: vi.fn((event: string, handler: DebuggerListener) => { const arr = listeners.get(event) ?? [] diff --git a/src/main/browser/cdp-ws-proxy.test.ts b/src/main/browser/cdp-ws-proxy.test.ts index d5a2d14a05b..f744998ab24 100644 --- a/src/main/browser/cdp-ws-proxy.test.ts +++ b/src/main/browser/cdp-ws-proxy.test.ts @@ -21,8 +21,8 @@ describe('CdpWsProxy', () => { beforeEach(async () => { mock = createMockWebContents() - // eslint-disable-next-line @typescript-eslint/no-explicit-any - proxy = new CdpWsProxy(mock.webContents as any) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mock implements every WebContents member the proxy calls. + proxy = new CdpWsProxy(mock.webContents as never, () => () => {}) endpoint = await proxy.start() }) diff --git a/src/main/browser/cdp-ws-proxy.ts b/src/main/browser/cdp-ws-proxy.ts index 32ea35be12c..4bc18eb4112 100644 --- a/src/main/browser/cdp-ws-proxy.ts +++ b/src/main/browser/cdp-ws-proxy.ts @@ -8,6 +8,7 @@ import { CdpDebuggerChannel } from './cdp-debugger-channel' import { CdpPageNavigationCommands } from './cdp-page-navigation-commands' import { CdpDomFocusReplay } from './cdp-dom-focus-replay' import { CdpPageCaptureCommands } from './cdp-page-capture-commands' +import type { CapturePaintHold } from './cdp-screenshot' export class CdpWsProxy { private httpServer: Server | null = null @@ -23,7 +24,10 @@ export class CdpWsProxy { private readonly domFocusReplay: CdpDomFocusReplay private readonly pageCapture: CdpPageCaptureCommands - constructor(private readonly webContents: WebContents) { + constructor( + private readonly webContents: WebContents, + holdPaint: CapturePaintHold + ) { this.discovery = new CdpTargetDiscovery( webContents, this.responder, @@ -44,7 +48,7 @@ export class CdpWsProxy { this.debuggerChannel ) this.domFocusReplay = new CdpDomFocusReplay(webContents, this.responder, this.debuggerChannel) - this.pageCapture = new CdpPageCaptureCommands(webContents, this.responder) + this.pageCapture = new CdpPageCaptureCommands(webContents, this.responder, holdPaint) } async start(): Promise { @@ -171,7 +175,7 @@ export class CdpWsProxy { } // Why: Page.captureScreenshot via debugger.sendCommand hangs on Electron webview guests. if (msg.method === 'Page.captureScreenshot') { - this.pageCapture.handleScreenshot(client, clientId, msg.params) + void this.pageCapture.handleScreenshot(client, clientId, msg.params) return } // Why: CDP Page.printToPDF is not available for Electron webview guests. diff --git a/src/main/runtime/orca-runtime-close-mobile-session-tab.ts b/src/main/runtime/orca-runtime-close-mobile-session-tab.ts index 15ba06ba386..2ef5b77069d 100644 --- a/src/main/runtime/orca-runtime-close-mobile-session-tab.ts +++ b/src/main/runtime/orca-runtime-close-mobile-session-tab.ts @@ -21,6 +21,7 @@ import type { RuntimeCommandSurfaceHost } from './orca-runtime-core' import { structuredAgentSessionTabId } from '../../shared/structured-agent-session-projection' import { SESSION_TAB_NOT_FOUND_ERROR } from '../../shared/session-tab-close' import { captureAcknowledgedTerminalTabRetirement } from './workspace-session-terminal-tab-retirement-identity' +import { rendererPublicationThrottle } from '../window/renderer-publication-throttle' export class OrcaRuntimeWithCloseMobileSessionTab extends OrcaRuntimeWithRefuseUnattributedMobileSessionTabClose { async closeMobileSessionTab( @@ -196,7 +197,7 @@ export class OrcaRuntimeWithCloseMobileSessionTab extends OrcaRuntimeWithRefuseU } const releasePublicationThrottle = options.clientNavigationId && win - ? this.rendererPublicationThrottle.acquire(win.webContents) + ? rendererPublicationThrottle.acquire(win.webContents) : () => {} try { await (options.localPtyTeardownOwnedExternally diff --git a/src/main/runtime/orca-runtime-run-create-mobile-session-terminal.ts b/src/main/runtime/orca-runtime-run-create-mobile-session-terminal.ts index 05621ac9792..193bf3cb12d 100644 --- a/src/main/runtime/orca-runtime-run-create-mobile-session-terminal.ts +++ b/src/main/runtime/orca-runtime-run-create-mobile-session-terminal.ts @@ -12,6 +12,7 @@ import { MOBILE_TERMINAL_SURFACE_TIMEOUT_MS, isClientDisconnectedError } from './orca-runtime-core' +import { rendererPublicationThrottle } from '../window/renderer-publication-throttle' export class OrcaRuntimeWithRunCreateMobileSessionTerminal extends OrcaRuntimeWithCreateMobileSessionTerminal { protected async runCreateMobileSessionTerminal( @@ -90,7 +91,7 @@ export class OrcaRuntimeWithRunCreateMobileSessionTerminal extends OrcaRuntimeWi throw new Error('runtime_unavailable') } const releasePublicationThrottle = pairedCreate - ? this.rendererPublicationThrottle.acquire(win.webContents) + ? rendererPublicationThrottle.acquire(win.webContents) : () => {} try { const requestId = randomUUID() diff --git a/src/main/runtime/orca-runtime-runtime-id.ts b/src/main/runtime/orca-runtime-runtime-id.ts index a93250f5d51..7b1811d9255 100644 --- a/src/main/runtime/orca-runtime-runtime-id.ts +++ b/src/main/runtime/orca-runtime-runtime-id.ts @@ -24,7 +24,6 @@ import { RUNTIME_GRAPH_RELOAD_TIMEOUT_MS, RuntimeGraphReloadLifecycle } from './runtime-graph-reload-lifecycle' -import { RendererPublicationThrottle } from '../window/renderer-publication-throttle' import { ClientHostedPageReconciliationWindow } from './client-hosted-page-reconciliation-window' import { ClientSessionTabSelectionStore } from './client-session-tab-selection' import { WorktreeTerminalMutationLock } from './worktree-terminal-mutation-lock' @@ -93,9 +92,6 @@ export class OrcaRuntimeWithRuntimeId { onTimeout: (_revision, windowId) => this.handleGraphReloadTimeout(windowId) }) - // Why: paired graph transactions need foreground timer cadence only until their publication settles. - protected readonly rendererPublicationThrottle = new RendererPublicationThrottle() - protected tabs = new Map() protected mobileSessionTabsByWorktree = new Map() diff --git a/src/main/window/renderer-publication-throttle.ts b/src/main/window/renderer-publication-throttle.ts index c7a63d74597..e53d80ca4bc 100644 --- a/src/main/window/renderer-publication-throttle.ts +++ b/src/main/window/renderer-publication-throttle.ts @@ -30,3 +30,6 @@ export class RendererPublicationThrottle { } } } + +// Why: one instance per process, or two owners of the same window would re-throttle it under each other's lease. +export const rendererPublicationThrottle = new RendererPublicationThrottle() diff --git a/src/preload/api/browser-api.ts b/src/preload/api/browser-api.ts index 5aff6728abf..460cabe8ee4 100644 --- a/src/preload/api/browser-api.ts +++ b/src/preload/api/browser-api.ts @@ -119,6 +119,9 @@ export type BrowserApi = { onActivateView: ( callback: (data: { worktreeId?: string; browserPageId?: string }) => void ) => () => void + onCapturePaintHold: ( + callback: (data: { browserPageId: string; held: boolean }) => void + ) => () => void onPaneFocus: ( callback: (data: { worktreeId: string | null; browserPageId: string }) => void ) => () => void diff --git a/src/preload/api/browser-bridge-page-interaction-and-sessions.ts b/src/preload/api/browser-bridge-page-interaction-and-sessions.ts index 0e07440f245..319207175a3 100644 --- a/src/preload/api/browser-bridge-page-interaction-and-sessions.ts +++ b/src/preload/api/browser-bridge-page-interaction-and-sessions.ts @@ -61,6 +61,16 @@ export const browserPageInteractionAndSessionsApi = { ipcRenderer.on('browser:activateView', listener) return () => ipcRenderer.removeListener('browser:activateView', listener) }, + onCapturePaintHold: ( + callback: (data: { browserPageId: string; held: boolean }) => void + ): (() => void) => { + const listener = ( + _event: Electron.IpcRendererEvent, + data: { browserPageId: string; held: boolean } + ) => callback(data) + ipcRenderer.on('browser:capturePaintHold', listener) + return () => ipcRenderer.removeListener('browser:capturePaintHold', listener) + }, onPaneFocus: ( callback: (data: { worktreeId: string | null; browserPageId: string }) => void ): (() => void) => { diff --git a/src/renderer/src/components/browser-pane/host-guest/browser-automation-visibility.test.ts b/src/renderer/src/components/browser-pane/host-guest/browser-automation-visibility.test.ts index 14f4d8c9976..c07e8b53413 100644 --- a/src/renderer/src/components/browser-pane/host-guest/browser-automation-visibility.test.ts +++ b/src/renderer/src/components/browser-pane/host-guest/browser-automation-visibility.test.ts @@ -1,18 +1,8 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' describe('browser automation visibility leases', () => { beforeEach(() => { vi.resetModules() - vi.stubGlobal('window', { - requestAnimationFrame: (callback: FrameRequestCallback) => { - callback(0) - return 1 - } - }) - }) - - afterEach(() => { - vi.unstubAllGlobals() }) it('keeps a page visible until every lease is released', async () => { @@ -41,63 +31,4 @@ describe('browser automation visibility leases', () => { acquireBrowserAutomationVisibility('page-2') expect(listener).toHaveBeenCalledTimes(4) }) - - it('installs a main-process bridge that keeps the page visible while waiting for paint', async () => { - const animationFrameCallbacks: FrameRequestCallback[] = [] - vi.stubGlobal('window', { - requestAnimationFrame: (callback: FrameRequestCallback) => { - animationFrameCallbacks.push(callback) - return animationFrameCallbacks.length - } - }) - const { isBrowserAutomationVisible } = await import('./browser-automation-visibility') - - const bridge = window.__orcaBrowserAutomationVisibility - expect(bridge).toBeTruthy() - - const acquirePromise = bridge?.acquire('page-2') - await Promise.resolve() - - expect(isBrowserAutomationVisible('page-2')).toBe(true) - expect(animationFrameCallbacks).toHaveLength(1) - - animationFrameCallbacks.shift()?.(0) - await Promise.resolve() - - expect(isBrowserAutomationVisible('page-2')).toBe(true) - expect(animationFrameCallbacks).toHaveLength(1) - - animationFrameCallbacks.shift()?.(16) - const token = await acquirePromise - - expect(typeof token).toBe('string') - expect(isBrowserAutomationVisible('page-2')).toBe(true) - expect(bridge?.release(token ?? '')).toBe(true) - expect(isBrowserAutomationVisible('page-2')).toBe(false) - }) - - it('releases the main-process bridge lease when the paint wait hangs', async () => { - vi.useFakeTimers() - vi.stubGlobal('window', { - requestAnimationFrame: () => 1 - }) - try { - const { isBrowserAutomationVisible } = await import('./browser-automation-visibility') - - const bridge = window.__orcaBrowserAutomationVisibility - expect(bridge).toBeTruthy() - - const acquirePromise = bridge?.acquire('page-hung-paint') - await Promise.resolve() - - expect(isBrowserAutomationVisible('page-hung-paint')).toBe(true) - - await vi.advanceTimersByTimeAsync(2_000) - await expect(acquirePromise).resolves.toBeNull() - - expect(isBrowserAutomationVisible('page-hung-paint')).toBe(false) - } finally { - vi.useRealTimers() - } - }) }) diff --git a/src/renderer/src/components/browser-pane/host-guest/browser-automation-visibility.ts b/src/renderer/src/components/browser-pane/host-guest/browser-automation-visibility.ts index f810d22f024..db1401751ae 100644 --- a/src/renderer/src/components/browser-pane/host-guest/browser-automation-visibility.ts +++ b/src/renderer/src/components/browser-pane/host-guest/browser-automation-visibility.ts @@ -1,24 +1,11 @@ import { useSyncExternalStore } from 'react' -type BrowserAutomationVisibilityBridge = { - acquire: (browserPageId: string) => Promise - release: (token: string) => boolean -} - -declare global { - // oxlint-disable-next-line typescript-eslint/consistent-type-definitions -- declaration merging requires interface - interface Window { - __orcaBrowserAutomationVisibility?: BrowserAutomationVisibilityBridge - } -} - const leaseCountsByPageId = new Map() const pageIdByToken = new Map() const listeners = new Set<() => void>() let version = 0 let nextLeaseId = 0 -const AUTOMATION_VISIBILITY_PAINT_TIMEOUT_MS = 2_000 function emitChange(): void { version += 1 @@ -46,32 +33,6 @@ function getServerSnapshot(): number { return 0 } -function nextAnimationFrame(): Promise { - if (typeof window === 'undefined' || typeof window.requestAnimationFrame !== 'function') { - return Promise.resolve() - } - return new Promise((resolve) => window.requestAnimationFrame(() => resolve())) -} - -async function waitForAutomationVisiblePaint(): Promise { - let timeout: ReturnType | null = null - const paint = (async () => { - await nextAnimationFrame() - await nextAnimationFrame() - return true - })() - const timedOut = new Promise((resolve) => { - timeout = setTimeout(() => resolve(false), AUTOMATION_VISIBILITY_PAINT_TIMEOUT_MS) - }) - try { - return await Promise.race([paint, timedOut]) - } finally { - if (timeout !== null) { - clearTimeout(timeout) - } - } -} - export function isBrowserAutomationVisible(browserPageId: string): boolean { return (leaseCountsByPageId.get(browserPageId) ?? 0) > 0 } @@ -121,30 +82,3 @@ export function releaseBrowserAutomationVisibility(token: string): boolean { emitChange() return true } - -async function acquireForMainProcess(browserPageId: string): Promise { - if (typeof browserPageId !== 'string' || browserPageId.length === 0) { - return null - } - const token = acquireBrowserAutomationVisibility(browserPageId) - // Why: the hidden pane only becomes paintable after the visibility lease - // exists. Wait after acquiring it so agent-browser commands do not race a - // still-hidden webview; release locally if paint never arrives. - if (await waitForAutomationVisiblePaint()) { - return token - } - releaseBrowserAutomationVisibility(token) - return null -} - -export function installBrowserAutomationVisibilityBridge(): void { - if (typeof window === 'undefined') { - return - } - window.__orcaBrowserAutomationVisibility = { - acquire: acquireForMainProcess, - release: releaseBrowserAutomationVisibility - } -} - -installBrowserAutomationVisibilityBridge() diff --git a/src/renderer/src/components/browser-pane/host-guest/browser-guest-retention-site-census.test.ts b/src/renderer/src/components/browser-pane/host-guest/browser-guest-retention-site-census.test.ts index 67063ff8805..5d8c6028329 100644 --- a/src/renderer/src/components/browser-pane/host-guest/browser-guest-retention-site-census.test.ts +++ b/src/renderer/src/components/browser-pane/host-guest/browser-guest-retention-site-census.test.ts @@ -94,12 +94,11 @@ const NON_RETENTION_TERM_READERS = new Map([ ] ]) -// Writers, hydrators, the bridge installer and the idle sentinel: they set or seed a term rather +// Writers, hydrators and the idle sentinel: they set or seed a term rather // than read it, so naming one is not a retention decision. const NON_READER_TERM_EXPORTS = [ 'acquireBrowserAutomationVisibility', 'releaseBrowserAutomationVisibility', - 'installBrowserAutomationVisibilityBridge', 'setDriverForBrowserPage', 'hydrateBrowserDrivers', 'IDLE_BROWSER_DRIVER', diff --git a/src/renderer/src/hooks/ipc-events/browser-state-capture-paint-hold.test.ts b/src/renderer/src/hooks/ipc-events/browser-state-capture-paint-hold.test.ts new file mode 100644 index 00000000000..90cf727326d --- /dev/null +++ b/src/renderer/src/hooks/ipc-events/browser-state-capture-paint-hold.test.ts @@ -0,0 +1,79 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' + +vi.mock('../../store', () => ({ + useAppStore: { getState: () => ({}) } +})) +vi.mock('@/lib/worktree-runtime-owner', () => ({ + getRuntimeEnvironmentIdForWorktree: () => null +})) +vi.mock('@/components/browser-pane/describe-page/live-browser-url-registry', () => ({ + rememberLiveBrowserUrl: vi.fn() +})) +vi.mock('./browser-automation-bootstrap-lease', () => ({ + acquireBrowserAutomationBootstrapLease: vi.fn() +})) + +import { isBrowserAutomationVisible } from '@/components/browser-pane/host-guest/browser-automation-visibility' +import { registerBrowserStateIpcBridge } from './browser-state-ipc-bridge' + +type CapturePaintHoldEvent = { browserPageId: string; held: boolean } + +function captureHoldHandler(unsubs: (() => void)[] = []): (event: CapturePaintHoldEvent) => void { + let handler: ((event: CapturePaintHoldEvent) => void) | null = null + const subscribe = vi.fn(() => () => {}) + vi.stubGlobal('window', { + api: { + ui: { onFullscreenChanged: subscribe }, + browser: { + onGuestLoadFailed: subscribe, + onNavigationUpdate: subscribe, + onActivateView: subscribe, + onPaneFocus: subscribe, + onOpenLinkInOrcaTab: subscribe, + onCapturePaintHold: (callback: (event: CapturePaintHoldEvent) => void) => { + handler = callback + return () => {} + } + } + } + }) + + registerBrowserStateIpcBridge(unsubs, () => false) + if (!handler) { + throw new Error('Expected the bridge to subscribe to browser:capturePaintHold') + } + return handler +} + +describe('capture paint holds from main', () => { + afterEach(() => { + vi.unstubAllGlobals() + }) + + it('keeps the page drawn from the hold until its release, ignoring repeats', () => { + const onHold = captureHoldHandler() + + onHold({ browserPageId: 'page-1', held: true }) + onHold({ browserPageId: 'page-1', held: true }) + expect(isBrowserAutomationVisible('page-1')).toBe(true) + + onHold({ browserPageId: 'page-1', held: false }) + expect(isBrowserAutomationVisible('page-1')).toBe(false) + + onHold({ browserPageId: 'page-1', held: false }) + expect(isBrowserAutomationVisible('page-1')).toBe(false) + }) + + it('releases a live hold when the bridge is disposed', () => { + const unsubs: (() => void)[] = [] + const onHold = captureHoldHandler(unsubs) + + onHold({ browserPageId: 'page-2', held: true }) + expect(isBrowserAutomationVisible('page-2')).toBe(true) + + for (const unsubscribe of unsubs) { + unsubscribe() + } + expect(isBrowserAutomationVisible('page-2')).toBe(false) + }) +}) diff --git a/src/renderer/src/hooks/ipc-events/browser-state-ipc-bridge.ts b/src/renderer/src/hooks/ipc-events/browser-state-ipc-bridge.ts index b4fec87e379..47da67cdc85 100644 --- a/src/renderer/src/hooks/ipc-events/browser-state-ipc-bridge.ts +++ b/src/renderer/src/hooks/ipc-events/browser-state-ipc-bridge.ts @@ -2,6 +2,10 @@ import { rememberLiveBrowserUrl } from '@/components/browser-pane/describe-page/ import { getRuntimeEnvironmentIdForWorktree } from '@/lib/worktree-runtime-owner' import { redactKagiSessionToken } from '../../../../shared/browser-url' import { useAppStore } from '../../store' +import { + acquireBrowserAutomationVisibility, + releaseBrowserAutomationVisibility +} from '@/components/browser-pane/host-guest/browser-automation-visibility' import { acquireBrowserAutomationBootstrapLease } from './browser-automation-bootstrap-lease' /** @@ -68,6 +72,29 @@ export function registerBrowserStateIpcBridge( } }) ) + // Why: main owns capture holds and sends each page's first hold and last release; no reply is awaited. + const capturePaintHoldTokens = new Map() + const unsubscribeCapturePaintHold = window.api.browser.onCapturePaintHold?.( + ({ browserPageId, held }) => { + const token = capturePaintHoldTokens.get(browserPageId) + if (held && !token) { + capturePaintHoldTokens.set(browserPageId, acquireBrowserAutomationVisibility(browserPageId)) + } else if (!held && token) { + capturePaintHoldTokens.delete(browserPageId) + releaseBrowserAutomationVisibility(token) + } + } + ) + if (unsubscribeCapturePaintHold) { + unsubs.push(() => { + unsubscribeCapturePaintHold() + // Why: the release for a live hold can no longer arrive, so it must not leave the page drawn. + for (const token of capturePaintHoldTokens.values()) { + releaseBrowserAutomationVisibility(token) + } + capturePaintHoldTokens.clear() + }) + } unsubs.push( window.api.browser.onPaneFocus(({ worktreeId, browserPageId }) => { if (isRuntimeEnvironmentActive()) { diff --git a/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts b/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts index 2e13d9382e4..d0572bd847d 100644 --- a/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts +++ b/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts @@ -10,6 +10,7 @@ const EXPECTED_DIRECT_CALLBACK_METHODS = [ 'agentStatus.onSet', 'automations.onChanged', 'browser.onActivateView', + 'browser.onCapturePaintHold', 'browser.onCertificateFailureChanged', 'browser.onGuestLoadFailed', 'browser.onNavigationUpdate', @@ -165,6 +166,7 @@ const EXPECTED_CALLBACK_REGISTRATION_SEQUENCE = [ 'browser.onCertificateFailureChanged', 'browser.onNavigationUpdate', 'browser.onActivateView', + 'browser.onCapturePaintHold', 'browser.onPaneFocus', 'browser.onOpenLinkInOrcaTab', 'ui.onNewBrowserTab',