diff --git a/src/main/browser/browser-backend.ts b/src/main/browser/browser-backend.ts index e55c8dc7a16..e06b7f54b2e 100644 --- a/src/main/browser/browser-backend.ts +++ b/src/main/browser/browser-backend.ts @@ -6,6 +6,8 @@ // WebContents uniformly regardless of how the page was created. This interface // isolates the only step that actually differs: tab creation and teardown. +import type { BrowserLoadError } from '../../shared/browser-workspace-types' + export type BrowserBackendCreateTab = { url: string worktreeId?: string @@ -22,6 +24,9 @@ export type ParkedBrowserPage = { title: string /** Whether the page was its worktree's active tab when it parked. */ active?: boolean + /** The failure the page last reported. Reclaiming a renderer does not make a + * page that failed to load healthy again. */ + loadError?: BrowserLoadError | null } export type BrowserBackend = { @@ -40,7 +45,7 @@ export type BrowserBackend = { wakeTab?(browserPageId: string): Promise /** Pages this backend still owns whose renderer is currently reclaimed. */ listParkedPages?(worktreeId?: string): ParkedBrowserPage[] - /** Most-recently-used parked page, for commands that target a worktree - * rather than a page id. */ - getMostRecentlyUsedParkedPageId?(worktreeId?: string): string | null + /** The parked page a page-less command should target: the one that was + * active when it parked, else the most recently used. */ + getParkedPageIdForImplicitTarget?(worktreeId?: string): string | null } diff --git a/src/main/browser/headless-browser-tab-listing.test.ts b/src/main/browser/headless-browser-tab-listing.test.ts index d5ee2a21216..d04580b2fbe 100644 --- a/src/main/browser/headless-browser-tab-listing.test.ts +++ b/src/main/browser/headless-browser-tab-listing.test.ts @@ -60,4 +60,17 @@ describe('mergeParkedBrowserTabs', () => { parked: true }) }) + + it('reports at most one active tab when several parked pages claim it', () => { + // Why: `active` is a single selection. Parking the active page promotes + // another live tab, which can later park claiming the flag as well. + const merged = mergeParkedBrowserTabs([], [parked('b', true), parked('c', true)]) + expect(merged.filter((tab) => tab.active).map((tab) => tab.browserPageId)).toEqual(['b']) + }) + + it('carries a parked page load failure into the listing', () => { + const loadError = { code: -105, description: 'NAME_NOT_RESOLVED', validatedUrl: 'https://nope' } + const [tab] = mergeParkedBrowserTabs([], [{ ...parked('b'), loadError }]) + expect(tab.loadError).toEqual(loadError) + }) }) diff --git a/src/main/browser/headless-browser-tab-listing.ts b/src/main/browser/headless-browser-tab-listing.ts index 8c1012f4baa..e2b1037a0bf 100644 --- a/src/main/browser/headless-browser-tab-listing.ts +++ b/src/main/browser/headless-browser-tab-listing.ts @@ -16,17 +16,23 @@ export function mergeParkedBrowserTabs( } // Why: parking clears the bridge's active pointer, which then lands on a // surviving live tab. A parked page may only claim active when nothing live - // does, or a listing can report two active tabs. - const liveClaimsActive = live.some((tab) => tab.active) + // does, and at most one may — `active` is a single selection, so a listing + // that reports two is not something any caller can act on. + let activeClaimed = live.some((tab) => tab.active) const merged = [...live] for (const page of parked) { + const active = !activeClaimed && page.active === true + activeClaimed = activeClaimed || active merged.push({ browserPageId: page.browserPageId, index: merged.length, url: page.url, title: page.title, - active: !liveClaimsActive && page.active === true, + active, parked: true, + // Why: a page that failed to load is still failed while parked; hiding + // that would show a broken page as healthy at its address. + loadError: page.loadError ?? null, worktreeId: page.worktreeId ?? null, profileId: page.profileId ?? null }) diff --git a/src/main/browser/offscreen-browser-backend-reclaim.test.ts b/src/main/browser/offscreen-browser-backend-reclaim.test.ts index b56ff27247b..2c013482d1e 100644 --- a/src/main/browser/offscreen-browser-backend-reclaim.test.ts +++ b/src/main/browser/offscreen-browser-backend-reclaim.test.ts @@ -25,6 +25,8 @@ type FakeWindow = BrowserWindow & { __destroyed: boolean __url: string __destroyedListeners: (() => void)[] + __navigationListeners: ((e: unknown, url: string) => void)[] + navigateTo: (url: string) => void } let nextWebContentsId = 100 @@ -36,7 +38,14 @@ function makeWindow(): FakeWindow { __destroyed: false, __url: 'about:blank', __destroyedListeners: [] as (() => void)[], + __navigationListeners: [] as ((e: unknown, url: string) => void)[], isDestroyed: () => win.__destroyed, + navigateTo: (url: string) => { + win.__url = url + for (const listener of win.__navigationListeners) { + listener(null, url) + } + }, destroy: () => { win.__destroyed = true for (const listener of win.__destroyedListeners) { @@ -52,6 +61,11 @@ function makeWindow(): FakeWindow { if (event === 'destroyed') { win.__destroyedListeners.push(listener) } + }, + on: (event: string, listener: (e: unknown, url: string) => void) => { + if (event === 'did-navigate' || event === 'did-navigate-in-page') { + win.__navigationListeners.push(listener) + } } } } as unknown as FakeWindow @@ -66,9 +80,17 @@ type Harness = { registered: Map windows: FakeWindow[] clock: { value: number } + activePageId: string | undefined } -function createHarness(overrides: { pinned?: Set; activePageId?: string } = {}): Harness { +function createHarness( + overrides: { + pinned?: Set + activePageId?: string + loadError?: { code: number; description: string; validatedUrl: string } | null + } = {} +): Harness { + const state = { activePageId: overrides.activePageId } const order: string[] = [] const registered = new Map() const windows: FakeWindow[] = [] @@ -95,7 +117,8 @@ function createHarness(overrides: { pinned?: Set; activePageId?: string order.push(`unregister:${browserPageId}`) registered.delete(browserPageId) }, - getGuestWebContentsId: (browserPageId: string) => registered.get(browserPageId) ?? null + getGuestWebContentsId: (browserPageId: string) => registered.get(browserPageId) ?? null, + getBrowserPageLoadError: () => overrides.loadError ?? null } as unknown as BrowserManager const bridge = { @@ -105,7 +128,7 @@ function createHarness(overrides: { pinned?: Set; activePageId?: string onProcessSwap: vi.fn(async (browserPageId: string, webContentsId: number) => { order.push(`process-swap:${browserPageId}:${webContentsId}`) }), - isActiveBrowserPage: (browserPageId: string) => overrides.activePageId === browserPageId + isActiveBrowserPage: (browserPageId: string) => state.activePageId === browserPageId } as unknown as AgentBrowserBridge const backend = new OffscreenBrowserBackend(manager, { @@ -114,7 +137,18 @@ function createHarness(overrides: { pinned?: Set; activePageId?: string now: () => clock.value }) - return { backend, manager, bridge, order, registered, windows, clock } + return { + backend, + manager, + bridge, + order, + registered, + windows, + clock, + set activePageId(value: string | undefined) { + state.activePageId = value + } + } } beforeEach(() => { @@ -145,7 +179,8 @@ describe('OffscreenBrowserBackend reclamation', () => { profileId: 'default', url: 'https://example.test/a', title: `title-${h.windows[0].webContents.id}`, - active: false + active: false, + loadError: null } ]) }) @@ -186,7 +221,7 @@ describe('OffscreenBrowserBackend reclamation', () => { const h = createHarness() await h.backend.createTab({ url: 'https://example.test/a' }) const [pageId] = [...h.registered.keys()] - h.windows[0].__url = 'https://example.test/moved' + h.windows[0].navigateTo('https://example.test/moved') h.clock.value += 120_000 await h.backend.reclaimIdlePages() @@ -204,11 +239,10 @@ describe('OffscreenBrowserBackend reclamation', () => { }) it('keeps the requested address when a page parks before committing one', async () => { - // Why: an uncommitted window still reports about:blank, and adopting that - // would send the wake to a blank page instead of the agent's URL. + // Why: a page whose load never committed must still wake to the address the + // agent asked for, not to the blank page the window started on. const h = createHarness() await h.backend.createTab({ url: 'https://example.test/slow', browserPageId: 'a' }) - h.windows[0].__url = 'about:blank' h.clock.value += 120_000 await h.backend.reclaimIdlePages() @@ -298,6 +332,72 @@ describe('OffscreenBrowserBackend reclamation', () => { expect(h.backend.listParkedPages().map((page) => page.browserPageId)).toEqual(['b']) }) + it('keeps an intentional navigation to about:blank across a park', async () => { + // Why: an in-page `location.href = "about:blank"` is a real destination. + // Sniffing the address at park time could not tell it apart from the blank + // page a window starts on, so the record follows navigation instead. + const h = createHarness() + await h.backend.createTab({ url: 'https://example.test/a', browserPageId: 'a' }) + h.windows[0].navigateTo('about:blank') + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + expect(h.backend.listParkedPages()[0]?.url).toBe('about:blank') + }) + + it('ignores a chrome-error address so a wake retries the real one', async () => { + const h = createHarness() + await h.backend.createTab({ url: 'https://example.test/a', browserPageId: 'a' }) + h.windows[0].navigateTo('chrome-error://chromewebdata/') + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + expect(h.backend.listParkedPages()[0]?.url).toBe('https://example.test/a') + }) + + it('carries a load failure onto the parked record', async () => { + // Why: reclaiming a renderer does not make a page that failed to load + // healthy, and the failure is unreadable once the guest is unregistered. + const loadError = { code: -105, description: 'NAME_NOT_RESOLVED', validatedUrl: 'https://nope' } + const h = createHarness({ loadError }) + await h.backend.createTab({ url: 'https://nope', browserPageId: 'a' }) + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + expect(h.backend.listParkedPages()[0]?.loadError).toEqual(loadError) + }) + + it('lets only the newest claim hold the parked active flag', async () => { + // Why: parking the active page promotes another live tab to active, which + // then parks claiming the flag too. `active` is a single selection. + const h = createHarness({ activePageId: 'a' }) + await h.backend.createTab({ url: 'https://a', browserPageId: 'a', worktreeId: 'wt-1' }) + await h.backend.createTab({ url: 'https://b', browserPageId: 'b', worktreeId: 'wt-1' }) + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + // Simulate the promotion: b is now the active page and parks claiming it. + h.activePageId = 'b' + await h.backend.wakeTab('b') + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + const claimed = h.backend.listParkedPages().filter((page) => page.active) + expect(claimed.map((page) => page.browserPageId)).toEqual(['b']) + }) + + it('targets the page that was active, not merely the most recently used', async () => { + // Why: an explicit `--page b` command makes b the most recently used while + // a is still the active tab, and a page-less command means "the active tab". + const h = createHarness({ activePageId: 'a' }) + await h.backend.createTab({ url: 'https://a', browserPageId: 'a', worktreeId: 'wt-1' }) + h.clock.value += 1_000 + await h.backend.createTab({ url: 'https://b', browserPageId: 'b', worktreeId: 'wt-1' }) + h.clock.value += 120_000 + await h.backend.reclaimIdlePages() + + expect(h.backend.getParkedPageIdForImplicitTarget('wt-1')).toBe('a') + }) + it('reports the most recently used parked page for implicit targeting', async () => { const h = createHarness() for (const id of ['a', 'b', 'c', 'd']) { @@ -307,7 +407,7 @@ describe('OffscreenBrowserBackend reclamation', () => { h.clock.value += 10_000 await h.backend.reclaimIdlePages() - expect(h.backend.getMostRecentlyUsedParkedPageId()).toBe('b') + expect(h.backend.getParkedPageIdForImplicitTarget()).toBe('b') }) it('restarts the reclaim clock when a resident page is used', async () => { @@ -395,7 +495,7 @@ describe('OffscreenBrowserBackend reclamation', () => { await h.backend.reclaimIdlePages() expect(h.backend.listParkedPages('wt-1').map((page) => page.browserPageId)).toEqual(['a']) - expect(h.backend.getMostRecentlyUsedParkedPageId('wt-2')).toBe('b') + expect(h.backend.getParkedPageIdForImplicitTarget('wt-2')).toBe('b') }) it('destroys every page it owns on shutdown', async () => { diff --git a/src/main/browser/offscreen-browser-backend.ts b/src/main/browser/offscreen-browser-backend.ts index 96904d61903..bc974860ecc 100644 --- a/src/main/browser/offscreen-browser-backend.ts +++ b/src/main/browser/offscreen-browser-backend.ts @@ -68,6 +68,7 @@ export class OffscreenBrowserBackend implements BrowserBackend { window: null, activeWhenParked: false, loading: false, + loadError: null, lastActivityAt: this.now() } this.pages.add(page) @@ -151,9 +152,9 @@ export class OffscreenBrowserBackend implements BrowserBackend { return this.pages.listParked(worktreeId) } - /** Most-recently-used parked page in a worktree, for implicit targeting. */ - getMostRecentlyUsedParkedPageId(worktreeId?: string): string | null { - return this.pages.mostRecentlyUsedParkedId(worktreeId) + /** The parked page a page-less command should target in this worktree. */ + getParkedPageIdForImplicitTarget(worktreeId?: string): string | null { + return this.pages.parkedIdForImplicitTarget(worktreeId) } destroyAll(): void { @@ -217,23 +218,25 @@ export class OffscreenBrowserBackend implements BrowserBackend { if (!page?.window || page.window.isDestroyed()) { return } - // Why: capture the committed address before teardown so a woken page - // returns to where the agent left it, not to its original create URL. A - // page that never committed still reports the empty or blank address the - // window started on; adopting that would send the wake to the wrong page. - const wc = page.window.webContents - const committed = wc.getURL() - const uncommitted = !committed || committed === 'about:blank' - if (!uncommitted && !committed.startsWith('chrome-error://')) { - page.url = committed - } - page.title = wc.getTitle() ?? page.title + // Why: the record's address is kept current by did-navigate, so parking + // never has to guess it from a WebContents that may be sitting on the blank + // page a failed load left behind. + page.title = page.window.webContents.getTitle() ?? page.title + // Why: a reclaimed renderer does not make a page that failed to load + // healthy, and the failure becomes unreadable once the guest is + // unregistered — so carry it onto the record. + page.loadError = this.browserManager.getBrowserPageLoadError(browserPageId) page.activeWhenParked = this.options .getAgentBrowserBridge?.() ?.isActiveBrowserPage(browserPageId, page.worktreeId) === true await this.releaseRenderer(page, browserPageId) page.window = null + if (page.activeWhenParked) { + // Why: teardown promotes another live tab to active, which then parks + // claiming the flag too. Only the newest claim may hold it. + this.pages.claimParkedActive(browserPageId, page.worktreeId) + } } // Why: teardown order matters — the bridge must destroy the helper session and @@ -281,6 +284,17 @@ export class OffscreenBrowserBackend implements BrowserBackend { // the throw would escape the 'destroyed' listener into the main process. // Capture the id while it is still safe to read. const webContentsId = win.webContents.id + // Why: the record's address must follow the page, not the create call — an + // agent that navigates with `goto` or in-page script has to be woken back + // to where it actually is. A chrome-error address is the failure, not a + // destination, so it never replaces the address that produced it. + const trackNavigation = (_event: unknown, url: string): void => { + if (page.window === win && url && !url.startsWith('chrome-error://')) { + page.url = url + } + } + win.webContents.on('did-navigate', trackNavigation) + win.webContents.on('did-navigate-in-page', trackNavigation) // Why: if the window is destroyed out from under us (crash, app teardown), // drop the page so commands fail cleanly instead of resolving a dead // WebContents. Parking destroys it deliberately, so it opts out here. diff --git a/src/main/browser/offscreen-browser-open-pages.ts b/src/main/browser/offscreen-browser-open-pages.ts index 5508ec66b90..5cf74760504 100644 --- a/src/main/browser/offscreen-browser-open-pages.ts +++ b/src/main/browser/offscreen-browser-open-pages.ts @@ -1,4 +1,5 @@ import type { BrowserWindow } from 'electron' +import type { BrowserLoadError } from '../../shared/browser-workspace-types' import type { ParkedBrowserPage } from './browser-backend' // Why (STA-4341): a headless browser page and the renderer behind it have @@ -20,6 +21,8 @@ export type OffscreenBrowserPage = { activeWhenParked: boolean /** True while the initial or post-wake navigation is still in flight. */ loading: boolean + /** The failure the page reported when its renderer was reclaimed. */ + loadError: BrowserLoadError | null lastActivityAt: number } @@ -77,18 +80,37 @@ export class OffscreenBrowserOpenPages { profileId: page.profileId, url: page.url, title: page.title, - active: page.activeWhenParked + active: page.activeWhenParked, + loadError: page.loadError })) } - /** Most-recently-used parked page, for commands that name a worktree. */ - mostRecentlyUsedParkedId(worktreeId?: string): string | null { + /** + * The parked page a page-less command should target. Activity order alone is + * wrong: an explicit `--page B` command makes B the most recently used while + * A is still the active tab, and a page-less command has always meant "the + * active tab". So the retained active page wins, and recency only breaks the + * tie when none is recorded. + */ + parkedIdForImplicitTarget(worktreeId?: string): string | null { let best: OffscreenBrowserPage | null = null for (const page of this.parked(worktreeId)) { + if (page.activeWhenParked) { + return page.browserPageId + } if (!best || page.lastActivityAt > best.lastActivityAt) { best = page } } return best?.browserPageId ?? null } + + /** Only one page per worktree may carry the active flag across a park. */ + claimParkedActive(browserPageId: string, worktreeId?: string): void { + for (const page of this.parked(worktreeId)) { + if (page.browserPageId !== browserPageId) { + page.activeWhenParked = false + } + } + } } diff --git a/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts b/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts index 6a7914bc22a..e2ef723a1ee 100644 --- a/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts +++ b/src/main/runtime/orca-runtime-browser-parked-targeting.test.ts @@ -109,7 +109,7 @@ function createFakeHeadlessHost(live: string[], parked: string[]): FakeHeadlessH url: `https://example.test/${browserPageId}`, title: browserPageId })), - getMostRecentlyUsedParkedPageId: () => state.parkedPageIds.at(-1) ?? null + getParkedPageIdForImplicitTarget: () => state.parkedPageIds.at(-1) ?? null } as unknown as BrowserBackend state.host = { diff --git a/src/main/runtime/orca-runtime-browser.ts b/src/main/runtime/orca-runtime-browser.ts index e02958b7d83..83e234b43a4 100644 --- a/src/main/runtime/orca-runtime-browser.ts +++ b/src/main/runtime/orca-runtime-browser.ts @@ -327,7 +327,7 @@ export class RuntimeBrowserCommands { // Read-only callers opt out — listing tabs must never cost a renderer. if (options.wakeParkedPage !== false && !this.host.getAvailableAuthoritativeWindow()) { const offscreen = this.host.getOffscreenBrowserBackend() - const parkedPageId = offscreen?.getMostRecentlyUsedParkedPageId?.(worktreeId) + const parkedPageId = offscreen?.getParkedPageIdForImplicitTarget?.(worktreeId) if (parkedPageId && (await offscreen?.wakeTab?.(parkedPageId))) { return } @@ -1768,7 +1768,7 @@ export class RuntimeBrowserCommands { const resolvedTabId = tabId ?? bridge.getActivePageId(worktreeId) ?? - offscreen.getMostRecentlyUsedParkedPageId?.(worktreeId) ?? + offscreen.getParkedPageIdForImplicitTarget?.(worktreeId) ?? null if (!resolvedTabId) { return { closed: false }