From 0f90f13bff3ce7260b9ed61e039afcbec47b98f2 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Tue, 15 Sep 2026 00:36:46 +0200 Subject: [PATCH] fix: settle closed browser captures and validate persisted layout fixtures Destroying a guest during frame subscription left its screenshot pending until the capture deadline and blocked queued captures. Reject at destruction, remove the listener on every exit, and return browser_tab_closed. The regression proves immediate settlement and queue release. Reuse the unchanged persisted layout schema from a UI-independent module so the recovery fixture validates storage before editing it. --- .../browser/explorer-pane-placement.spec.ts | 66 ++++++---- .../src/stores/workspace-layout-storage.ts | 114 +++++++++++++++++ .../app/src/stores/workspace-layout-store.ts | 115 +----------------- .../features/browser-automation/ipc.test.ts | 73 ++++++++++- .../src/features/browser-automation/ipc.ts | 9 +- .../features/browser-automation/service.ts | 10 ++ 6 files changed, 245 insertions(+), 142 deletions(-) create mode 100644 packages/app/src/stores/workspace-layout-storage.ts diff --git a/packages/app/e2e/browser/explorer-pane-placement.spec.ts b/packages/app/e2e/browser/explorer-pane-placement.spec.ts index b407d7673c..8c2f103100 100644 --- a/packages/app/e2e/browser/explorer-pane-placement.spec.ts +++ b/packages/app/e2e/browser/explorer-pane-placement.spec.ts @@ -6,6 +6,9 @@ // interactive in the [agent | New | explorer] state. import { expect, type Locator, type Page } from "@playwright/test"; import { test } from "../support/fixtures"; +import { z } from "zod"; +import { WorkspaceLayoutPersistedStateSchema } from "../../src/stores/workspace-layout-storage"; +import type { WorkspaceTab } from "../../src/workspace-tabs/model"; import { gotoWorkspace } from "../support/helpers/launcher"; import { seedWorkspace } from "../support/helpers/seed-client"; import { @@ -259,34 +262,49 @@ async function openAgentDraftFromLauncher(page: Page): Promise { } async function persistHiddenGeneratedExplorer(page: Page): Promise { - await page.evaluate(() => { - const stored = JSON.parse(localStorage.getItem("workspace-layout-state")!); - const key = Object.keys(stored.state.layoutByWorkspace)[0]; - const tabs = [ - { tabId: "files", target: { kind: "files" }, createdAt: 1 }, - { tabId: "changes_tree", target: { kind: "changes_tree" }, createdAt: 1 }, - ...Array.from({ length: 2250 }, (_, index) => ({ + const raw = await page.evaluate(() => localStorage.getItem("workspace-layout-state")); + if (raw === null) throw new Error("Explorer fixture: workspace layout was not persisted"); + const stored = z + .strictObject({ + state: WorkspaceLayoutPersistedStateSchema, + version: z.number().int().nonnegative().optional(), + }) + .parse(JSON.parse(raw), { error: "Explorer fixture: invalid persisted workspace layout" }); + const [key] = Object.keys(stored.state.layoutByWorkspace); + if (!key) throw new Error("Explorer fixture: persisted layout contains no workspace"); + const tabs: WorkspaceTab[] = [ + { tabId: "files", target: { kind: "files" }, createdAt: 1 }, + { tabId: "changes_tree", target: { kind: "changes_tree" }, createdAt: 1 }, + ...Array.from( + { length: 2250 }, + (_, index): WorkspaceTab => ({ tabId: `draft_saved_${index}`, target: { kind: "draft", draftId: `draft_saved_${index}` }, createdAt: index + 2, - })), - ]; - stored.state.layoutByWorkspace[key] = { - root: { - kind: "pane", - pane: { - id: "pane_generated_report_equivalent", - hidden: true, - tabIds: tabs.map((tab) => tab.tabId), - tabs, - focusedTabId: tabs[tabs.length - 1].tabId, - }, + }), + ), + ]; + stored.state.layoutByWorkspace[key] = { + root: { + kind: "pane", + pane: { + id: "pane_generated_report_equivalent", + hidden: true, + tabIds: tabs.map((tab) => tab.tabId), + tabs, + focusedTabId: tabs[tabs.length - 1].tabId, }, - focusedPaneId: null, - }; - stored.state.explorerPaneIdByWorkspace[key] = "pane_generated_report_equivalent"; - localStorage.setItem("workspace-layout-state", JSON.stringify(stored)); - }); + }, + focusedPaneId: null, + }; + stored.state.explorerPaneIdByWorkspace = { + ...stored.state.explorerPaneIdByWorkspace, + [key]: "pane_generated_report_equivalent", + }; + await page.evaluate( + (value) => localStorage.setItem("workspace-layout-state", value), + JSON.stringify(stored), + ); } // Explorer cannot replace the ordinary workspace canvas, including on restore. diff --git a/packages/app/src/stores/workspace-layout-storage.ts b/packages/app/src/stores/workspace-layout-storage.ts new file mode 100644 index 0000000000..ae916188c5 --- /dev/null +++ b/packages/app/src/stores/workspace-layout-storage.ts @@ -0,0 +1,114 @@ +import { z } from "zod"; +import type { SplitNode, WorkspaceLayout } from "./workspace-layout-actions"; + +const WorkspaceDraftTabSetupStorageSchema = z.strictObject({ + provider: z.string(), + cwd: z.string(), + modeId: z.string().nullable(), + model: z.string().nullable(), + thinkingOptionId: z.string().nullable(), + featureValues: z.record(z.string(), z.union([z.boolean(), z.string(), z.null()])), +}); +const WorkspaceTabTargetStorageSchema = z.discriminatedUnion("kind", [ + z.strictObject({ kind: z.literal("new_tab") }), + z.strictObject({ + kind: z.literal("draft"), + draftId: z.string(), + setup: WorkspaceDraftTabSetupStorageSchema.optional(), + }), + z.strictObject({ kind: z.literal("agent"), agentId: z.string() }), + z.strictObject({ + kind: z.literal("provider_subagent"), + parentAgentId: z.string(), + subagentId: z.string(), + }), + z.strictObject({ kind: z.literal("terminal"), terminalId: z.string() }), + z.strictObject({ kind: z.literal("browser"), browserId: z.string() }), + z.strictObject({ kind: z.literal("changes_tree") }), + z.strictObject({ kind: z.literal("files") }), + z.strictObject({ kind: z.literal("pull_request") }), + z.strictObject({ + kind: z.literal("file"), + path: z.string(), + lineStart: z.number().int().positive().optional(), + lineEnd: z.number().int().positive().optional(), + }), + z.strictObject({ + kind: z.literal("working_diff"), + focusPath: z.string().optional(), + focusRequestId: z.number().optional(), + // COMPAT(workingDiffTarget): accepted from pre-canonical tab ids; normalization removes them. + mode: z.enum(["uncommitted", "base"]).optional(), + baseRef: z.string().nullable().optional(), + ignoreWhitespace: z.boolean().optional(), + }), + z.strictObject({ kind: z.literal("setup"), workspaceId: z.string() }), + z.strictObject({ kind: z.literal("commit_diff"), sha: z.string() }), + z.discriminatedUnion("context", [ + z.strictObject({ + kind: z.literal("plugin"), + pluginId: z.string(), + panelId: z.string(), + context: z.literal("workspace"), + }), + z.strictObject({ + kind: z.literal("plugin"), + pluginId: z.string(), + panelId: z.string(), + context: z.literal("agent"), + agentId: z.string(), + }), + ]), +]); +const WorkspaceTabStorageSchema = z.strictObject({ + tabId: z.string(), + target: WorkspaceTabTargetStorageSchema, + createdAt: z.number(), + state: z.json().optional(), +}); +const SplitNodeStorageSchema: z.ZodType = z.lazy(() => + z.discriminatedUnion("kind", [ + z.strictObject({ + kind: z.literal("pane"), + pane: z.strictObject({ + id: z.string(), + tabIds: z.array(z.string()), + focusedTabId: z.string().nullable(), + tabs: z.array(WorkspaceTabStorageSchema).optional(), + hidden: z.boolean().optional(), + }), + }), + z.strictObject({ + kind: z.literal("group"), + group: z.strictObject({ + id: z.string(), + direction: z.enum(["horizontal", "vertical"]), + children: z.array(SplitNodeStorageSchema), + sizes: z.array(z.number()), + }), + }), + ]), +); +const WorkspaceLayoutStorageSchema: z.ZodType = z.strictObject({ + root: SplitNodeStorageSchema, + focusedPaneId: z.string().nullable(), + parentTabIdByTabId: z.record(z.string(), z.string()).optional(), +}); +export const WorkspaceLayoutPersistedStateSchema = z.strictObject({ + pinnedAgentIdsByWorkspace: z.record(z.string(), z.array(z.string())).optional(), + layoutByWorkspace: z.record(z.string(), WorkspaceLayoutStorageSchema), + splitSizesByWorkspace: z.record(z.string(), z.record(z.string(), z.array(z.number()))).optional(), + explorerSidebarWidthByWorkspace: z.record(z.string(), z.number()).optional(), + // COMPAT(explorerSidebarWidth): added in v0.6, remove after 2027-08-25. + explorerSidebarRatioByWorkspace: z.record(z.string(), z.number()).optional(), + // COMPAT(explorerSidebarNaming): accepted from builds that called this dock the Side panel. + sidePanelRatioByWorkspace: z.record(z.string(), z.number()).optional(), + // The persisted keys keep their pre-rename spelling: the schema is strict, so a + // rename here would fail every existing blob and wipe the layout it describes. + explorerPaneIdByWorkspace: z.record(z.string(), z.string().nullable()).optional(), + explorerSidebarPaneIdByWorkspace: z.record(z.string(), z.string().nullable()).optional(), + sidePaneIdByWorkspace: z.record(z.string(), z.string().nullable()).optional(), + // COMPAT(pullRequestAutoAdd): PR detection stopped opening a tab in v0.5; accepted + // and ignored so upgrading does not discard the layout. Remove after 2027-08-20. + acknowledgedPullRequestByWorkspace: z.record(z.string(), z.string()).optional(), +}); diff --git a/packages/app/src/stores/workspace-layout-store.ts b/packages/app/src/stores/workspace-layout-store.ts index a8c6220063..b7b9d57677 100644 --- a/packages/app/src/stores/workspace-layout-store.ts +++ b/packages/app/src/stores/workspace-layout-store.ts @@ -2,7 +2,8 @@ import AsyncStorage from "@react-native-async-storage/async-storage"; import { useEffect, useState } from "react"; import { create } from "zustand"; import { persist } from "zustand/middleware"; -import { z } from "zod"; +import type { z } from "zod"; +import { WorkspaceLayoutPersistedStateSchema } from "./workspace-layout-storage"; import type { JsonValue } from "@getpaseo/protocol/agent-types"; import type { WorkspaceTab, WorkspaceTabTarget } from "@/workspace-tabs/model"; import { @@ -174,118 +175,6 @@ interface WorkspaceFocusRestorationState { // Preserve four user-created split levels beneath that bookkeeping node. const MAX_TREE_DEPTH = 5; -const WorkspaceDraftTabSetupStorageSchema = z.strictObject({ - provider: z.string(), - cwd: z.string(), - modeId: z.string().nullable(), - model: z.string().nullable(), - thinkingOptionId: z.string().nullable(), - featureValues: z.record(z.string(), z.union([z.boolean(), z.string(), z.null()])), -}); -const WorkspaceTabTargetStorageSchema = z.discriminatedUnion("kind", [ - z.strictObject({ kind: z.literal("new_tab") }), - z.strictObject({ - kind: z.literal("draft"), - draftId: z.string(), - setup: WorkspaceDraftTabSetupStorageSchema.optional(), - }), - z.strictObject({ kind: z.literal("agent"), agentId: z.string() }), - z.strictObject({ - kind: z.literal("provider_subagent"), - parentAgentId: z.string(), - subagentId: z.string(), - }), - z.strictObject({ kind: z.literal("terminal"), terminalId: z.string() }), - z.strictObject({ kind: z.literal("browser"), browserId: z.string() }), - z.strictObject({ kind: z.literal("changes_tree") }), - z.strictObject({ kind: z.literal("files") }), - z.strictObject({ kind: z.literal("pull_request") }), - z.strictObject({ - kind: z.literal("file"), - path: z.string(), - lineStart: z.number().int().positive().optional(), - lineEnd: z.number().int().positive().optional(), - }), - z.strictObject({ - kind: z.literal("working_diff"), - focusPath: z.string().optional(), - focusRequestId: z.number().optional(), - // COMPAT(workingDiffTarget): accepted from pre-canonical tab ids; normalization removes them. - mode: z.enum(["uncommitted", "base"]).optional(), - baseRef: z.string().nullable().optional(), - ignoreWhitespace: z.boolean().optional(), - }), - z.strictObject({ kind: z.literal("setup"), workspaceId: z.string() }), - z.strictObject({ kind: z.literal("commit_diff"), sha: z.string() }), - z.discriminatedUnion("context", [ - z.strictObject({ - kind: z.literal("plugin"), - pluginId: z.string(), - panelId: z.string(), - context: z.literal("workspace"), - }), - z.strictObject({ - kind: z.literal("plugin"), - pluginId: z.string(), - panelId: z.string(), - context: z.literal("agent"), - agentId: z.string(), - }), - ]), -]); -const WorkspaceTabStorageSchema = z.strictObject({ - tabId: z.string(), - target: WorkspaceTabTargetStorageSchema, - createdAt: z.number(), - state: z.json().optional(), -}); -const SplitNodeStorageSchema: z.ZodType = z.lazy(() => - z.discriminatedUnion("kind", [ - z.strictObject({ - kind: z.literal("pane"), - pane: z.strictObject({ - id: z.string(), - tabIds: z.array(z.string()), - focusedTabId: z.string().nullable(), - tabs: z.array(WorkspaceTabStorageSchema).optional(), - hidden: z.boolean().optional(), - }), - }), - z.strictObject({ - kind: z.literal("group"), - group: z.strictObject({ - id: z.string(), - direction: z.enum(["horizontal", "vertical"]), - children: z.array(SplitNodeStorageSchema), - sizes: z.array(z.number()), - }), - }), - ]), -); -const WorkspaceLayoutStorageSchema: z.ZodType = z.strictObject({ - root: SplitNodeStorageSchema, - focusedPaneId: z.string().nullable(), - parentTabIdByTabId: z.record(z.string(), z.string()).optional(), -}); -const WorkspaceLayoutPersistedStateSchema = z.strictObject({ - pinnedAgentIdsByWorkspace: z.record(z.string(), z.array(z.string())).optional(), - layoutByWorkspace: z.record(z.string(), WorkspaceLayoutStorageSchema), - splitSizesByWorkspace: z.record(z.string(), z.record(z.string(), z.array(z.number()))).optional(), - explorerSidebarWidthByWorkspace: z.record(z.string(), z.number()).optional(), - // COMPAT(explorerSidebarWidth): added in v0.6, remove after 2027-08-25. - explorerSidebarRatioByWorkspace: z.record(z.string(), z.number()).optional(), - // COMPAT(explorerSidebarNaming): accepted from builds that called this dock the Side panel. - sidePanelRatioByWorkspace: z.record(z.string(), z.number()).optional(), - // The persisted keys keep their pre-rename spelling: the schema is strict, so a - // rename here would fail every existing blob and wipe the layout it describes. - explorerPaneIdByWorkspace: z.record(z.string(), z.string().nullable()).optional(), - explorerSidebarPaneIdByWorkspace: z.record(z.string(), z.string().nullable()).optional(), - sidePaneIdByWorkspace: z.record(z.string(), z.string().nullable()).optional(), - // COMPAT(pullRequestAutoAdd): PR detection stopped opening a tab in v0.5; accepted - // and ignored so upgrading does not discard the layout. Remove after 2027-08-20. - acknowledgedPullRequestByWorkspace: z.record(z.string(), z.string()).optional(), -}); - const LEGACY_EXPLORER_SIDEBAR_REFERENCE_WIDTH = 1440; const WORKSPACE_LAYOUT_PERSIST_VERSION = 2; diff --git a/packages/desktop/src/features/browser-automation/ipc.test.ts b/packages/desktop/src/features/browser-automation/ipc.test.ts index db78fe0205..ee7143adc6 100644 --- a/packages/desktop/src/features/browser-automation/ipc.test.ts +++ b/packages/desktop/src/features/browser-automation/ipc.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test, vi } from "vitest"; -import type { TabImage } from "./service.js"; +import { executeAutomationCommand, type BrowserRegistry, type TabImage } from "./service.js"; import { adaptWebContents, HostSnapshotEngineRegistry } from "./ipc.js"; import type { IsolatedKeyboardInputEvent } from "./trusted-input.js"; @@ -122,7 +122,7 @@ class FakeWebContents { public endedFrameSubscriptions = 0; public readonly invalidations: string[] = []; private consoleMessageListener: ConsoleMessageListener | null = null; - private destroyedListener: (() => void) | null = null; + public readonly destroyedListeners = new Set<() => void>(); public destroyed = false; public constructor(private readonly webContentsId: number) {} @@ -197,7 +197,12 @@ class FakeWebContents { public once(event: "destroyed", listener: () => void): void { expect(event).toBe("destroyed"); - this.destroyedListener = listener; + this.destroyedListeners.add(listener); + } + + public removeListener(event: "destroyed", listener: () => void): void { + expect(event).toBe("destroyed"); + this.destroyedListeners.delete(listener); } public emitConsoleMessage(input: { @@ -215,7 +220,11 @@ class FakeWebContents { public destroy(): void { this.destroyed = true; this.debugger.emitDetach(); - this.destroyedListener?.(); + this.frameListener = null; + for (const listener of this.destroyedListeners) { + this.destroyedListeners.delete(listener); + listener(); + } } } @@ -270,6 +279,62 @@ describe("browser automation IPC adapter", () => { expect(contents.endedFrameSubscriptions).toBe(1); }); + test("closing a guest settles its screenshot and releases the next queued capture", async () => { + vi.useFakeTimers(); + try { + const closing = new FakeWebContents(2002); + const next = new FakeWebContents(2003); + const tabs = new Map([ + ["closing", adaptWebContents(closing)], + ["next", adaptWebContents(next)], + ]); + const registry: BrowserRegistry = { + listRegisteredBrowserIds: () => [...tabs.keys()], + listRegisteredBrowserIdsForWorkspace: () => [...tabs.keys()], + getTabContents: (id) => tabs.get(id) ?? null, + getBrowserWorkspaceId: () => null, + getWorkspaceActiveBrowserId: () => null, + }; + const capture = (browserId: string) => + executeAutomationCommand( + { + type: "browser.automation.execute.request", + requestId: browserId, + command: { command: "screenshot", args: { browserId, fullPage: false } }, + }, + registry, + ); + let closedResult: unknown; + const closedCapture = Promise.resolve(capture("closing")).then((result) => { + closedResult = result; + return result; + }); + const nextCapture = capture("next"); + await vi.advanceTimersByTimeAsync(0); + expect(closing.frameListener).not.toBeNull(); + closing.destroy(); + await vi.advanceTimersByTimeAsync(0); + expect(closedResult).toEqual({ + requestId: "closing", + ok: false, + error: { + code: "browser_tab_closed", + message: "Browser tab closing has been closed", + retryable: false, + }, + }); + expect(closing.destroyedListeners.size).toBe(0); + expect(next.frameListener).not.toBeNull(); + next.frameListener?.(new FakeImage()); + await expect(nextCapture).resolves.toMatchObject({ ok: true }); + await closedCapture; + expect(next.destroyedListeners.size).toBe(1); + } finally { + await vi.runAllTimersAsync(); + vi.useRealTimers(); + } + }); + test("collects console messages until the guest is destroyed", () => { const contents = new FakeWebContents(21); const tab = adaptWebContents(contents); diff --git a/packages/desktop/src/features/browser-automation/ipc.ts b/packages/desktop/src/features/browser-automation/ipc.ts index 0d28d46004..42c38d2e2c 100644 --- a/packages/desktop/src/features/browser-automation/ipc.ts +++ b/packages/desktop/src/features/browser-automation/ipc.ts @@ -15,7 +15,7 @@ import { promptShimInstallScript, promptShimRestoreScript, } from "./dialog-handling.js"; -import { executeAutomationCommand } from "./service.js"; +import { BrowserTabClosedError, executeAutomationCommand } from "./service.js"; import { BrowserSnapshotEngine } from "./snapshot-engine.js"; import { listRegisteredPaseoBrowserIds, @@ -91,6 +91,7 @@ interface ConsoleMessageEmitter { } interface BrowserAutomationWebContents extends ConsoleMessageEmitter { + removeListener(event: "destroyed", listener: () => void): void; readonly id: number; readonly debugger: WebContentsDebugger; getURL(): string; @@ -162,13 +163,19 @@ function captureViewportFrame( return new Promise((resolve, reject) => { const stop = () => { signal.removeEventListener("abort", abort); + contents.removeListener("destroyed", destroyed); if (!contents.isDestroyed()) contents.endFrameSubscription(); }; const abort = () => { stop(); reject(signal.reason); }; + const destroyed = () => { + stop(); + reject(new BrowserTabClosedError()); + }; signal.addEventListener("abort", abort, { once: true }); + contents.once("destroyed", destroyed); try { // A resized resident guest can paint while capturePage's surface-copy // request remains pending. Subscribe to its rendered frames instead. diff --git a/packages/desktop/src/features/browser-automation/service.ts b/packages/desktop/src/features/browser-automation/service.ts index 1bcf96eec4..ca6247e543 100644 --- a/packages/desktop/src/features/browser-automation/service.ts +++ b/packages/desktop/src/features/browser-automation/service.ts @@ -94,6 +94,13 @@ async function withDialogCapture( return dialogs.length > 0 ? { ...result, dialogs } : result; } +export class BrowserTabClosedError extends Error { + public constructor() { + super("Browser tab has been closed"); + this.name = "BrowserTabClosedError"; + } +} + class ScreenshotNoFrameError extends Error { public constructor(message = SCREENSHOT_NO_FRAME_MESSAGE) { super(message); @@ -1228,6 +1235,9 @@ async function executeScreenshot( try { image = await capturePaintedViewport(target.contents); } catch (error) { + if (error instanceof BrowserTabClosedError) { + return fail(requestId, "browser_tab_closed", `Browser tab ${browserId} has been closed`); + } if (isScreenshotNoFrameError(error)) { return screenshotNoFrameFailure(requestId, error); }