Wait for browser registration after hidden wake (#4413)
This commit is contained in:
parent
163cf8da6f
commit
d044a2be9e
|
|
@ -116,3 +116,16 @@ The follow-up changes the order: create the automation visibility lease first,
|
|||
then wait for paint while the pane is actually visible to automation. A
|
||||
renderer-side timeout releases the lease if paint never arrives, so a hung RAF
|
||||
does not pin an inactive browser pane indefinitely.
|
||||
|
||||
## Follow-up: Browser Registration Readiness
|
||||
|
||||
One remaining automation race was the wake path for parked or restored browser
|
||||
tabs. Runtime browser commands asked the renderer to mount a hidden browser
|
||||
pane, then waited a fixed 500 ms before reading the agent-browser tab registry.
|
||||
On slow webview startup, that could still race `registerGuest` and make
|
||||
agent-browser report no tab even though the tab was in the process of mounting.
|
||||
|
||||
The follow-up extends the existing tab-registration wait from page-specific
|
||||
creation to worktree/global wake flows. Runtime commands now wait for the
|
||||
renderer's actual `browser:registerGuest` IPC before routing automation, with
|
||||
the same timeout fallback used by tab creation.
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
/* eslint-disable max-lines -- Why: browser IPC tests share one mocked trust-boundary handler registry plus registration waiters; splitting would duplicate setup and weaken coverage. */
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
const {
|
||||
|
|
@ -6,6 +7,7 @@ const {
|
|||
registerGuestMock,
|
||||
unregisterGuestMock,
|
||||
getGuestWebContentsIdMock,
|
||||
getWebContentsIdByTabIdMock,
|
||||
getWorktreeIdForTabMock,
|
||||
openDevToolsMock,
|
||||
setAnnotationViewportBridgeMock,
|
||||
|
|
@ -20,6 +22,7 @@ const {
|
|||
registerGuestMock: vi.fn(),
|
||||
unregisterGuestMock: vi.fn(),
|
||||
getGuestWebContentsIdMock: vi.fn(),
|
||||
getWebContentsIdByTabIdMock: vi.fn(() => new Map()),
|
||||
getWorktreeIdForTabMock: vi.fn(),
|
||||
openDevToolsMock: vi.fn().mockResolvedValue(true),
|
||||
setAnnotationViewportBridgeMock: vi.fn().mockResolvedValue(true),
|
||||
|
|
@ -48,6 +51,7 @@ vi.mock('../browser/browser-manager', () => ({
|
|||
registerGuest: registerGuestMock,
|
||||
unregisterGuest: unregisterGuestMock,
|
||||
getGuestWebContentsId: getGuestWebContentsIdMock,
|
||||
getWebContentsIdByTabId: getWebContentsIdByTabIdMock,
|
||||
getWorktreeIdForTab: getWorktreeIdForTabMock,
|
||||
openDevTools: openDevToolsMock,
|
||||
setAnnotationViewportBridge: setAnnotationViewportBridgeMock,
|
||||
|
|
@ -60,7 +64,9 @@ vi.mock('../browser/browser-manager', () => ({
|
|||
import {
|
||||
registerBrowserHandlers,
|
||||
setAgentBrowserBridgeRef,
|
||||
waitForTabRegistration
|
||||
waitForAnyTabRegistration,
|
||||
waitForTabRegistration,
|
||||
waitForWorktreeTabRegistration
|
||||
} from './browser'
|
||||
|
||||
describe('registerBrowserHandlers', () => {
|
||||
|
|
@ -71,6 +77,8 @@ describe('registerBrowserHandlers', () => {
|
|||
registerGuestMock.mockReset()
|
||||
unregisterGuestMock.mockReset()
|
||||
getGuestWebContentsIdMock.mockReset()
|
||||
getWebContentsIdByTabIdMock.mockReset()
|
||||
getWebContentsIdByTabIdMock.mockReturnValue(new Map())
|
||||
getWorktreeIdForTabMock.mockReset()
|
||||
openDevToolsMock.mockReset()
|
||||
setAnnotationViewportBridgeMock.mockReset()
|
||||
|
|
@ -219,6 +227,71 @@ describe('registerBrowserHandlers', () => {
|
|||
}
|
||||
})
|
||||
|
||||
it('resolves worktree and any-tab registration waiters when a guest registers', async () => {
|
||||
vi.useFakeTimers()
|
||||
try {
|
||||
getWebContentsIdByTabIdMock.mockReturnValue(new Map())
|
||||
const worktreeWait = waitForWorktreeTabRegistration('worktree-1', 1000)
|
||||
const anyWait = waitForAnyTabRegistration(1000)
|
||||
const settled = Promise.allSettled([worktreeWait, anyWait])
|
||||
|
||||
registerBrowserHandlers()
|
||||
|
||||
const registerHandler = handleMock.mock.calls.find(
|
||||
([channel]) => channel === 'browser:registerGuest'
|
||||
)?.[1] as (
|
||||
event: { sender: Electron.WebContents },
|
||||
args: {
|
||||
browserPageId: string
|
||||
workspaceId: string
|
||||
worktreeId: string
|
||||
webContentsId: number
|
||||
}
|
||||
) => boolean
|
||||
|
||||
const result = registerHandler(
|
||||
{
|
||||
sender: {
|
||||
id: 91,
|
||||
isDestroyed: () => false,
|
||||
getType: () => 'window',
|
||||
getURL: () => 'file:///renderer/index.html'
|
||||
} as Electron.WebContents
|
||||
},
|
||||
{
|
||||
browserPageId: 'page-worktree-1',
|
||||
workspaceId: 'workspace-1',
|
||||
worktreeId: 'worktree-1',
|
||||
webContentsId: 123
|
||||
}
|
||||
)
|
||||
|
||||
expect(result).toBe(true)
|
||||
await vi.advanceTimersByTimeAsync(1001)
|
||||
expect(await settled).toEqual([
|
||||
{ status: 'fulfilled', value: undefined },
|
||||
{ status: 'fulfilled', value: undefined }
|
||||
])
|
||||
} finally {
|
||||
vi.useRealTimers()
|
||||
}
|
||||
})
|
||||
|
||||
it('resolves worktree registration waits immediately when a tab is already registered', async () => {
|
||||
getWebContentsIdByTabIdMock.mockReturnValue(new Map([['page-1', 123]]))
|
||||
getWorktreeIdForTabMock.mockReturnValue('worktree-1')
|
||||
|
||||
await expect(waitForWorktreeTabRegistration('worktree-1', 1000)).resolves.toBeUndefined()
|
||||
|
||||
expect(getWorktreeIdForTabMock).toHaveBeenCalledWith('page-1')
|
||||
})
|
||||
|
||||
it('resolves any-tab registration waits immediately when a tab is already registered', async () => {
|
||||
getWebContentsIdByTabIdMock.mockReturnValue(new Map([['page-1', 123]]))
|
||||
|
||||
await expect(waitForAnyTabRegistration(1000)).resolves.toBeUndefined()
|
||||
})
|
||||
|
||||
it('validates annotation viewport bridge requests before syncing to the guest', async () => {
|
||||
registerBrowserHandlers()
|
||||
|
||||
|
|
|
|||
|
|
@ -42,17 +42,15 @@ let agentBrowserBridgeRef: AgentBrowserBridge | null = null
|
|||
// subsequent commands. Multiple commands can wait for the same page during
|
||||
// startup, so keep all one-shot resolvers keyed by browserPageId.
|
||||
const pendingTabRegistrations = new Map<string, Set<() => void>>()
|
||||
const pendingWorktreeTabRegistrations = new Map<string, Set<() => void>>()
|
||||
const pendingAnyTabRegistrations = new Set<() => void>()
|
||||
|
||||
export function waitForTabRegistration(browserPageId: string, timeoutMs = 8_000): Promise<void> {
|
||||
if (browserManager.getGuestWebContentsId(browserPageId) !== null) {
|
||||
return Promise.resolve()
|
||||
}
|
||||
function waitForRegistrationSet(
|
||||
registrationResolvers: Set<() => void>,
|
||||
timeoutMs: number,
|
||||
onEmpty: () => void
|
||||
): Promise<void> {
|
||||
return new Promise<void>((resolve, reject) => {
|
||||
let registrationResolvers = pendingTabRegistrations.get(browserPageId)
|
||||
if (!registrationResolvers) {
|
||||
registrationResolvers = new Set()
|
||||
pendingTabRegistrations.set(browserPageId, registrationResolvers)
|
||||
}
|
||||
const resolveRegistration = (): void => {
|
||||
clearTimeout(timer)
|
||||
resolve()
|
||||
|
|
@ -60,7 +58,7 @@ export function waitForTabRegistration(browserPageId: string, timeoutMs = 8_000)
|
|||
const timer = setTimeout(() => {
|
||||
registrationResolvers.delete(resolveRegistration)
|
||||
if (registrationResolvers.size === 0) {
|
||||
pendingTabRegistrations.delete(browserPageId)
|
||||
onEmpty()
|
||||
}
|
||||
reject(new Error('Tab registration timed out'))
|
||||
}, timeoutMs)
|
||||
|
|
@ -68,6 +66,65 @@ export function waitForTabRegistration(browserPageId: string, timeoutMs = 8_000)
|
|||
})
|
||||
}
|
||||
|
||||
function resolvePendingRegistrations(registrationResolvers: Set<() => void> | undefined): void {
|
||||
if (!registrationResolvers) {
|
||||
return
|
||||
}
|
||||
for (const pendingResolve of registrationResolvers) {
|
||||
pendingResolve()
|
||||
}
|
||||
}
|
||||
|
||||
function hasRegisteredTabForWorktree(worktreeId: string): boolean {
|
||||
for (const browserPageId of browserManager.getWebContentsIdByTabId().keys()) {
|
||||
if (browserManager.getWorktreeIdForTab(browserPageId) === worktreeId) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
export function waitForTabRegistration(browserPageId: string, timeoutMs = 8_000): Promise<void> {
|
||||
if (browserManager.getGuestWebContentsId(browserPageId) !== null) {
|
||||
return Promise.resolve()
|
||||
}
|
||||
let registrationResolvers = pendingTabRegistrations.get(browserPageId)
|
||||
if (!registrationResolvers) {
|
||||
registrationResolvers = new Set()
|
||||
pendingTabRegistrations.set(browserPageId, registrationResolvers)
|
||||
}
|
||||
return waitForRegistrationSet(registrationResolvers, timeoutMs, () => {
|
||||
pendingTabRegistrations.delete(browserPageId)
|
||||
})
|
||||
}
|
||||
|
||||
export function waitForWorktreeTabRegistration(
|
||||
worktreeId: string | undefined,
|
||||
timeoutMs = 8_000
|
||||
): Promise<void> {
|
||||
if (!worktreeId) {
|
||||
return waitForAnyTabRegistration(timeoutMs)
|
||||
}
|
||||
if (hasRegisteredTabForWorktree(worktreeId)) {
|
||||
return Promise.resolve()
|
||||
}
|
||||
let registrationResolvers = pendingWorktreeTabRegistrations.get(worktreeId)
|
||||
if (!registrationResolvers) {
|
||||
registrationResolvers = new Set()
|
||||
pendingWorktreeTabRegistrations.set(worktreeId, registrationResolvers)
|
||||
}
|
||||
return waitForRegistrationSet(registrationResolvers, timeoutMs, () => {
|
||||
pendingWorktreeTabRegistrations.delete(worktreeId)
|
||||
})
|
||||
}
|
||||
|
||||
export function waitForAnyTabRegistration(timeoutMs = 8_000): Promise<void> {
|
||||
if (browserManager.getWebContentsIdByTabId().size > 0) {
|
||||
return Promise.resolve()
|
||||
}
|
||||
return waitForRegistrationSet(pendingAnyTabRegistrations, timeoutMs, () => {})
|
||||
}
|
||||
|
||||
export function setTrustedBrowserRendererWebContentsId(webContentsId: number | null): void {
|
||||
trustedBrowserRendererWebContentsId = webContentsId
|
||||
}
|
||||
|
|
@ -139,12 +196,14 @@ export function registerBrowserHandlers(): void {
|
|||
agentBrowserBridgeRef.onProcessSwap(args.browserPageId, args.webContentsId, previousWcId)
|
||||
}
|
||||
const pendingResolves = pendingTabRegistrations.get(args.browserPageId)
|
||||
if (pendingResolves) {
|
||||
pendingTabRegistrations.delete(args.browserPageId)
|
||||
for (const pendingResolve of pendingResolves) {
|
||||
pendingResolve()
|
||||
}
|
||||
}
|
||||
pendingTabRegistrations.delete(args.browserPageId)
|
||||
resolvePendingRegistrations(pendingResolves)
|
||||
const pendingWorktreeResolves = pendingWorktreeTabRegistrations.get(args.worktreeId)
|
||||
pendingWorktreeTabRegistrations.delete(args.worktreeId)
|
||||
resolvePendingRegistrations(pendingWorktreeResolves)
|
||||
const pendingAnyResolves = new Set(pendingAnyTabRegistrations)
|
||||
pendingAnyTabRegistrations.clear()
|
||||
resolvePendingRegistrations(pendingAnyResolves)
|
||||
return true
|
||||
}
|
||||
)
|
||||
|
|
|
|||
|
|
@ -1,11 +1,13 @@
|
|||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { AgentBrowserBridge } from '../browser/agent-browser-bridge'
|
||||
import type { RuntimeBrowserCommandHost } from './orca-runtime-browser'
|
||||
|
||||
const { webContentsFromIdMock, startBrowserScreencastMock } = vi.hoisted(() => ({
|
||||
webContentsFromIdMock: vi.fn(),
|
||||
startBrowserScreencastMock: vi.fn()
|
||||
}))
|
||||
const { webContentsFromIdMock, startBrowserScreencastMock, waitForWorktreeTabRegistrationMock } =
|
||||
vi.hoisted(() => ({
|
||||
webContentsFromIdMock: vi.fn(),
|
||||
startBrowserScreencastMock: vi.fn(),
|
||||
waitForWorktreeTabRegistrationMock: vi.fn()
|
||||
}))
|
||||
|
||||
vi.mock('electron', () => ({
|
||||
ipcMain: { on: vi.fn(), removeListener: vi.fn() },
|
||||
|
|
@ -16,6 +18,11 @@ vi.mock('../browser/browser-screencast-stream', () => ({
|
|||
startBrowserScreencast: startBrowserScreencastMock
|
||||
}))
|
||||
|
||||
vi.mock('../ipc/browser', () => ({
|
||||
waitForTabRegistration: vi.fn(),
|
||||
waitForWorktreeTabRegistration: waitForWorktreeTabRegistrationMock
|
||||
}))
|
||||
|
||||
function deferred<T>() {
|
||||
let resolve!: (value: T) => void
|
||||
let reject!: (reason?: unknown) => void
|
||||
|
|
@ -26,31 +33,83 @@ function deferred<T>() {
|
|||
return { promise, resolve, reject }
|
||||
}
|
||||
|
||||
function createHost(): RuntimeBrowserCommandHost {
|
||||
const bridge = {
|
||||
getRegisteredTabs: vi.fn(() => new Map([['page-1', 100]])),
|
||||
getActivePageId: vi.fn(() => 'page-1'),
|
||||
tabList: vi.fn(() => ({
|
||||
tabs: [
|
||||
{
|
||||
browserPageId: 'page-1',
|
||||
index: 0,
|
||||
url: 'about:blank',
|
||||
title: 'Browser',
|
||||
active: true
|
||||
}
|
||||
]
|
||||
}))
|
||||
} as unknown as AgentBrowserBridge
|
||||
function createHost(overrides: Partial<RuntimeBrowserCommandHost> = {}): RuntimeBrowserCommandHost {
|
||||
const bridge =
|
||||
overrides.getAgentBrowserBridge?.() ??
|
||||
({
|
||||
getRegisteredTabs: vi.fn(() => new Map([['page-1', 100]])),
|
||||
getActivePageId: vi.fn(() => 'page-1'),
|
||||
tabList: vi.fn(() => ({
|
||||
tabs: [
|
||||
{
|
||||
browserPageId: 'page-1',
|
||||
index: 0,
|
||||
url: 'about:blank',
|
||||
title: 'Browser',
|
||||
active: true
|
||||
}
|
||||
]
|
||||
}))
|
||||
} as unknown as AgentBrowserBridge)
|
||||
return {
|
||||
getAgentBrowserBridge: () => bridge,
|
||||
resolveWorktreeSelector: async (selector) => ({ id: selector.replace(/^id:/, '') }),
|
||||
getAuthoritativeWindow: vi.fn(),
|
||||
getAvailableAuthoritativeWindow: vi.fn(() => null)
|
||||
getAvailableAuthoritativeWindow: vi.fn(() => null),
|
||||
...overrides,
|
||||
getAgentBrowserBridge: () => bridge
|
||||
} as unknown as RuntimeBrowserCommandHost
|
||||
}
|
||||
|
||||
describe('RuntimeBrowserCommands browser screencast', () => {
|
||||
beforeEach(() => {
|
||||
webContentsFromIdMock.mockReset()
|
||||
startBrowserScreencastMock.mockReset()
|
||||
waitForWorktreeTabRegistrationMock.mockReset()
|
||||
waitForWorktreeTabRegistrationMock.mockResolvedValue(undefined)
|
||||
})
|
||||
|
||||
it('waits for explicit worktree browser registration after requesting a hidden mount', async () => {
|
||||
const { RuntimeBrowserCommands } = await import('./orca-runtime-browser')
|
||||
const send = vi.fn()
|
||||
const bridge = {
|
||||
getRegisteredTabs: vi.fn(() => new Map()),
|
||||
tabList: vi.fn(() => ({ tabs: [] }))
|
||||
} as unknown as AgentBrowserBridge
|
||||
const commands = new RuntimeBrowserCommands(
|
||||
createHost({
|
||||
getAgentBrowserBridge: () => bridge,
|
||||
getAuthoritativeWindow: vi.fn(() => ({ webContents: { send } }) as never)
|
||||
})
|
||||
)
|
||||
|
||||
await commands.browserTabList({ worktree: 'id:wt-1' })
|
||||
|
||||
expect(send).toHaveBeenCalledWith('browser:activateView', { worktreeId: 'wt-1' })
|
||||
expect(waitForWorktreeTabRegistrationMock).toHaveBeenCalledWith('wt-1')
|
||||
expect(bridge.tabList).toHaveBeenCalledWith('wt-1')
|
||||
})
|
||||
|
||||
it('waits for any browser registration after requesting a hidden mount without worktree scope', async () => {
|
||||
const { RuntimeBrowserCommands } = await import('./orca-runtime-browser')
|
||||
const send = vi.fn()
|
||||
const bridge = {
|
||||
getRegisteredTabs: vi.fn(() => new Map()),
|
||||
tabList: vi.fn(() => ({ tabs: [] }))
|
||||
} as unknown as AgentBrowserBridge
|
||||
const commands = new RuntimeBrowserCommands(
|
||||
createHost({
|
||||
getAgentBrowserBridge: () => bridge,
|
||||
getAuthoritativeWindow: vi.fn(() => ({ webContents: { send } }) as never)
|
||||
})
|
||||
)
|
||||
|
||||
await commands.browserTabList({})
|
||||
|
||||
expect(send).toHaveBeenCalledWith('browser:activateView', {})
|
||||
expect(waitForWorktreeTabRegistrationMock).toHaveBeenCalledWith(undefined)
|
||||
expect(bridge.tabList).toHaveBeenCalledWith(undefined)
|
||||
})
|
||||
|
||||
it('lets a new same-page stream take over an active stale stream', async () => {
|
||||
const { RuntimeBrowserCommands } = await import('./orca-runtime-browser')
|
||||
webContentsFromIdMock.mockReturnValue({ isDestroyed: () => false })
|
||||
|
|
|
|||
|
|
@ -62,7 +62,7 @@ import {
|
|||
importCookiesFromBrowser,
|
||||
selectBrowserProfile
|
||||
} from '../browser/browser-cookie-import'
|
||||
import { waitForTabRegistration } from '../ipc/browser'
|
||||
import { waitForTabRegistration, waitForWorktreeTabRegistration } from '../ipc/browser'
|
||||
|
||||
export type BrowserCommandTargetParams = {
|
||||
worktree?: string
|
||||
|
|
@ -171,9 +171,7 @@ export class RuntimeBrowserCommands {
|
|||
const bridge = this.host.getAgentBrowserBridge()
|
||||
if (bridge && bridge.getRegisteredTabs().size === 0) {
|
||||
try {
|
||||
const win = this.host.getAuthoritativeWindow()
|
||||
win.webContents.send('browser:activateView', {})
|
||||
await new Promise((resolve) => setTimeout(resolve, 500))
|
||||
await this.ensureBrowserWorktreeActive(undefined)
|
||||
} catch {
|
||||
// Window may not exist yet (e.g. during startup or in tests)
|
||||
}
|
||||
|
|
@ -251,12 +249,13 @@ export class RuntimeBrowserCommands {
|
|||
// and registerGuest fires, but automation must not steal the user's visible
|
||||
// worktree/browser pane. Ask the renderer to background-mount the worktree and
|
||||
// acquire a hidden automation visibility lease instead of activating the UI.
|
||||
private async ensureBrowserWorktreeActive(worktreeId: string): Promise<void> {
|
||||
private async ensureBrowserWorktreeActive(worktreeId: string | undefined): Promise<void> {
|
||||
const win = this.host.getAuthoritativeWindow()
|
||||
win.webContents.send('browser:activateView', { worktreeId })
|
||||
// Why: give the renderer time to mount the hidden paintable webview.
|
||||
// The webview needs to attach and fire dom-ready before registerGuest runs.
|
||||
await new Promise((resolve) => setTimeout(resolve, 500))
|
||||
win.webContents.send('browser:activateView', worktreeId ? { worktreeId } : {})
|
||||
// Why: parked/restored browser panes become operable only after the
|
||||
// renderer's webview mounts and calls registerGuest. Waiting on that IPC is
|
||||
// both faster and less flaky than sleeping for an arbitrary fixed delay.
|
||||
await waitForWorktreeTabRegistration(worktreeId)
|
||||
}
|
||||
|
||||
// Why: agent-browser drives navigation via CDP, which bypasses Electron's
|
||||
|
|
|
|||
Loading…
Reference in New Issue