From 6a7990ce0b73fb1921d7ef1d38f176800113e165 Mon Sep 17 00:00:00 2001 From: Lakshmi Tanmay Date: Fri, 13 Nov 2026 10:36:24 +0530 Subject: [PATCH] fix(desktop): keep screenshot retries painting until their surface is ready --- .../src/app/DesktopPreReadyPlatform.test.ts | 26 ++- .../src/app/DesktopPreReadyPlatform.ts | 9 + .../src/preview/DesktopBrowserHost.test.ts | 177 +++++++++++++++++- .../desktop/src/preview/DesktopBrowserHost.ts | 55 +++++- 4 files changed, 258 insertions(+), 9 deletions(-) diff --git a/apps/desktop/src/app/DesktopPreReadyPlatform.test.ts b/apps/desktop/src/app/DesktopPreReadyPlatform.test.ts index 34607e08b844f..9f3d406987aa5 100644 --- a/apps/desktop/src/app/DesktopPreReadyPlatform.test.ts +++ b/apps/desktop/src/app/DesktopPreReadyPlatform.test.ts @@ -207,15 +207,29 @@ describe("DesktopPreReadyPlatform", () => { ).pipe(Effect.ensuring(Effect.sync(() => vi.unstubAllEnvs()))); }); - it.effect( - "acquires a synchronous pre-ready layer before an asynchronous Clerk-shaped layer", - () => + it.effect.each([ + { enabledFeatures: undefined, expected: "CDPScreenshotNewSurface" }, + { + enabledFeatures: "ExistingFeature Effect.gen(function* () { class ClerkShaped extends Context.Service()( "@t3tools/desktop/app/DesktopPreReadyPlatform.test/ClerkShaped", ) {} const events: Array = []; + hasSwitchMock.mockImplementation( + (name: string) => name === "enable-features" && enabledFeatures !== undefined, + ); + getSwitchValueMock.mockReturnValue(enabledFeatures ?? ""); registerSchemesMock.mockImplementation(() => { events.push("pre-ready"); }); @@ -224,6 +238,9 @@ describe("DesktopPreReadyPlatform", () => { Layer.provide(Layer.succeed(HostProcess.Platform, "darwin")), ); + const readiness = Promise.resolve().then(() => { + assert.deepEqual(appendSwitchMock.mock.calls, [["enable-features", expected]]); + }); const layerClerkShaped = Layer.effect( ClerkShaped, Effect.promise(() => Promise.resolve()).pipe( @@ -244,6 +261,7 @@ describe("DesktopPreReadyPlatform", () => { preReady: DesktopPreReadyPlatform.DesktopPreReadyElectronOptions, }).pipe(Effect.provide(layerRuntime)); + yield* Effect.promise(() => readiness); assert.deepEqual(result, { clerk: { ready: true }, preReady: { @@ -253,7 +271,7 @@ describe("DesktopPreReadyPlatform", () => { }); assert.deepEqual(events, ["pre-ready", "clerk"]); assert.equal(registerSchemesMock.mock.calls.length, 1); - assert.equal(appendSwitchMock.mock.calls.length, 0); + assert.equal(appendSwitchMock.mock.calls.length, 1); assert.equal(setDesktopNameMock.mock.calls.length, 0); }), ); diff --git a/apps/desktop/src/app/DesktopPreReadyPlatform.ts b/apps/desktop/src/app/DesktopPreReadyPlatform.ts index df8059133fc20..e538ad644d175 100644 --- a/apps/desktop/src/app/DesktopPreReadyPlatform.ts +++ b/apps/desktop/src/app/DesktopPreReadyPlatform.ts @@ -64,6 +64,15 @@ export const make = Effect.gen(function* () { const homeDirectory = yield* HostProcess.HomeDirectory; const linuxDesktopEntryConfig = yield* LinuxDesktopEntryConfig; return yield* Effect.sync((): DesktopPreReadyElectronOptions["Service"] => { + // Match Playwright's launch default: legacy CDP screenshots can wait forever for a presented frame. + const enableFeatures = new Set( + (readCommandLineSwitchValue(Electron.app.commandLine, "enable-features") ?? "") + .split(",") + .filter(Boolean), + ); + enableFeatures.add("CDPScreenshotNewSurface"); + Electron.app.commandLine.appendSwitch("enable-features", [...enableFeatures].join(",")); + const linuxPasswordStoreCommandLine = platform === "linux" ? readCommandLineSwitchValue(Electron.app.commandLine, "password-store") diff --git a/apps/desktop/src/preview/DesktopBrowserHost.test.ts b/apps/desktop/src/preview/DesktopBrowserHost.test.ts index fbb475be1424b..83236f8c164b6 100644 --- a/apps/desktop/src/preview/DesktopBrowserHost.test.ts +++ b/apps/desktop/src/preview/DesktopBrowserHost.test.ts @@ -45,7 +45,7 @@ const makeDebuggee = ( readonly deferFocus?: boolean; /** The guest is still loading its first page. */ readonly loading?: boolean; - readonly refuse?: (method: string, sessionId: string | undefined) => boolean; + readonly refuse?: (method: string, sessionId: string | undefined) => boolean | Error; } = {}, ) => { const emitter = new NodeEvents.EventEmitter(); @@ -63,8 +63,9 @@ const makeDebuggee = ( sessionId?: string, ) => new Promise((resolve, reject) => { - if (options.refuse?.(method, sessionId)) { - reject(new Error("Refused")); + const refusal = options.refuse?.(method, sessionId); + if (refusal) { + reject(refusal instanceof Error ? refusal : new Error("Refused")); return; } if (method === "Target.getTargetInfo") { @@ -330,6 +331,176 @@ describe("DesktopBrowserHost", () => { }).pipe(Effect.scoped), ); + it.effect.each(["frames", "refused wait", "timeout", "refused retry"] as const)( + "keeps painting through a missing-surface retry with %s", + (outcome) => + Effect.gen(function* () { + const host = yield* DesktopBrowserHost.make; + let captures = 0; + const debuggee = makeDebuggee({ + refuse: (method) => { + if (method === "Page.captureScreenshot") { + captures += 1; + if (captures === 1 || outcome === "refused retry") { + return new Error("Unable to capture screenshot"); + } + } + return method === "Runtime.evaluate" && outcome === "refused wait"; + }, + }); + host.attach(key, debuggee.tab); + const replies = yield* watchReplies(host); + const painting = yield* watchPainting(host); + const sent = debuggee.nextSend(); + yield* command(host, 1, "Page.captureScreenshot"); + + yield* Effect.raceFirst( + Effect.promise(() => sent), + Queue.take(replies), + ); + expect(painting).toEqual([true]); + if (outcome === "refused wait") { + yield* Effect.promise(() => sent); + } else { + yield* Effect.promise(() => sent); + expect(debuggee.methods).toEqual(["Runtime.evaluate"]); + expect(painting).toEqual([true]); + const retry = outcome === "refused retry" ? undefined : debuggee.nextSend(); + if (outcome === "timeout") yield* TestClock.adjust("1 second"); + else debuggee.releaseFirst(); + if (retry) yield* Effect.promise(() => retry); + } + if (outcome !== "refused retry") { + expect(captures).toBe(2); + expect(painting).toEqual([true]); + debuggee.release(); + } + expect(yield* Queue.take(replies)).toBe(1); + expect(captures).toBe(2); + expect(painting).toEqual([true, false]); + // A late frame callback cannot retry again after the bounded wait finished. + debuggee.release(); + yield* TestClock.adjust("10 seconds"); + expect(captures).toBe(2); + expect(debuggee.focusEmulation).toEqual([true, false]); + }).pipe(Effect.scoped), + ); + + it.effect.each(["navigation", "detach", "release", "turn end", "session detach"] as const)( + "ends retry preparation on %s without sending into a stale document", + (ending) => + Effect.gen(function* () { + const host = yield* DesktopBrowserHost.make; + let captures = 0; + const debuggee = makeDebuggee({ + refuse: (method) => { + if (method !== "Page.captureScreenshot") return false; + captures += 1; + return captures === 1 ? new Error("Unable to capture screenshot") : false; + }, + }); + host.attach(key, debuggee.tab); + const replies = yield* watchReplies(host); + const painting = yield* watchPainting(host); + const raw = (message: Record) => + host.handleCommandLine(encodeJson({ type: "cdp", ...key, message: encodeJson(message) })); + yield* raw({ id: 10, method: "Target.attachToTarget", params: { targetId: "GUEST" } }); + expect(yield* Queue.take(replies)).toBe(10); + const waiting = debuggee.nextSend(); + yield* raw({ id: 1, method: "Page.captureScreenshot", sessionId: "t3-preview-page-1" }); + yield* Effect.raceFirst( + Effect.promise(() => waiting), + Queue.take(replies), + ); + expect(painting).toEqual([true]); + + if (ending === "navigation") debuggee.navigate(); + else if (ending === "detach") host.detach(key); + else if (ending === "release") { + yield* host.handleCommandLine(encodeJson({ type: "release", ...key })); + } else if (ending === "turn end") { + yield* host.handleCommandLine( + encodeJson({ type: "turn", threadId: key.threadId, active: false }), + ); + } else { + yield* raw({ + id: 11, + method: "Target.detachFromTarget", + params: { sessionId: "t3-preview-page-1" }, + }); + expect(yield* Queue.take(replies)).toBe(11); + } + expect(painting).toEqual([true, false]); + + // New contents can capture while the old frame wait still has not answered. + const replacement = makeDebuggee(); + host.attach(key, replacement.tab); + const sent = replacement.nextSend(); + yield* command(host, 2, "Page.captureScreenshot"); + yield* Effect.promise(() => sent); + debuggee.release(); + yield* TestClock.adjust("1 second"); + expect(captures).toBe(1); + expect(painting).toEqual([true, false, true]); + replacement.release(); + expect(yield* Queue.take(replies)).toBe(2); + expect(painting).toEqual([true, false, true, false]); + }).pipe(Effect.scoped), + ); + + it.effect( + "keeps the capture slot through retry preparation without releasing another painter", + () => + Effect.gen(function* () { + const host = yield* DesktopBrowserHost.make; + let captures = 0; + const debuggee = makeDebuggee({ + refuse: (method) => { + if (method !== "Page.captureScreenshot") return false; + captures += 1; + return captures === 1 ? new Error("Unable to capture screenshot") : false; + }, + }); + host.attach(key, debuggee.tab); + const replies = yield* watchReplies(host); + const painting = yield* watchPainting(host); + const waiting = debuggee.nextSend(); + yield* command(host, 1, "Page.captureScreenshot"); + yield* Effect.raceFirst( + Effect.promise(() => waiting), + Queue.take(replies), + ); + expect(painting).toEqual([true]); + yield* command(host, 2, "Page.captureScreenshot"); + expect(captures).toBe(1); + const streaming = debuggee.nextSend(); + yield* command(host, 3, "Page.startScreencast"); + yield* Effect.promise(() => streaming); + const retry = debuggee.nextSend(); + debuggee.releaseFirst(); + yield* Effect.promise(() => retry); + expect(captures).toBe(2); + expect(painting).toEqual([true]); + const queued = debuggee.nextSend(); + debuggee.release(); + yield* Effect.promise(() => queued); + expect(captures).toBe(3); + debuggee.release(); + expect( + [ + yield* Queue.take(replies), + yield* Queue.take(replies), + yield* Queue.take(replies), + ].sort(), + ).toEqual([1, 2, 3]); + expect(painting).toEqual([true]); + const stopping = debuggee.nextSend(); + yield* command(host, 4, "Page.stopScreencast"); + yield* Effect.promise(() => stopping); + expect(painting).toEqual([true, false]); + }).pipe(Effect.scoped), + ); + it.effect("stops waiting on a screenshot the page navigated away from", () => Effect.gen(function* () { const host = yield* DesktopBrowserHost.make; diff --git a/apps/desktop/src/preview/DesktopBrowserHost.ts b/apps/desktop/src/preview/DesktopBrowserHost.ts index e5f43634d204a..714ff6a56424d 100644 --- a/apps/desktop/src/preview/DesktopBrowserHost.ts +++ b/apps/desktop/src/preview/DesktopBrowserHost.ts @@ -30,8 +30,12 @@ const AGENT_DOWNLOAD_WINDOW_MS = 5_000; const FOCUS_IDLE = "5 seconds"; /** Well inside the server's 10 s DESKTOP_ATTACH_TIMEOUT, for a first page that never reports it is up. */ const FIRST_PAGE_TIMEOUT = "5 seconds"; +const SURFACE_WAIT = "1 second"; const ERR_ABORTED = -3; +const isMissingSurface = (cause: unknown) => + cause instanceof Error && cause.message === "Unable to capture screenshot"; + export interface DesktopBrowserTabKey { readonly threadId: string; readonly tabId: string; @@ -67,6 +71,8 @@ interface AttachedTab { readonly focusPending: Map; /** Screenshots Chromium was sent and has not answered; each keeps its wake lock until then. */ readonly capturing: Set; + /** Serialized captures preparing a surface; lifecycle release prevents a late retry. */ + readonly preparingScreenshots: Map; /** Whether the client keeps the guest painting, as published on `painting`. */ painting: boolean; /** @@ -125,7 +131,9 @@ export const make = Effect.gen(function* () { readonly key: DesktopBrowserTabKey; readonly active: boolean; }>(); - const runFork = Effect.runForkWith(yield* Effect.context()); + const context = yield* Effect.context(); + const runFork = Effect.runForkWith(context); + const runPromise = Effect.runPromiseWith(context); const tabs = new Map(); /** Threads whose agent turn runs, by the server's account. */ const agentTurns = new Set(); @@ -240,6 +248,7 @@ export const make = Effect.gen(function* () { tabs.get(keyOf(tab.key)) === tab && (tab.screencasts.size > 0 || tab.capturing.size > 0 || + tab.preparingScreenshots.size > 0 || tab.waitingScreenshots.some((waiting) => waiting.live())); if (tab.painting === active) return; tab.painting = active; @@ -278,6 +287,7 @@ export const make = Effect.gen(function* () { */ const dropFocusHolds = (tab: AttachedTab) => { tab.focusPending.clear(); + tab.preparingScreenshots.clear(); releaseFocusWhenIdle(tab); syncPainting(tab); }; @@ -301,6 +311,9 @@ export const make = Effect.gen(function* () { for (const [pending, holder] of tab.focusPending) { if (holder === owner) tab.focusPending.delete(pending); } + for (const [pending, holder] of tab.preparingScreenshots) { + if (holder === owner) tab.preparingScreenshots.delete(pending); + } releaseFocusWhenIdle(tab); syncPainting(tab); }; @@ -329,6 +342,20 @@ export const make = Effect.gen(function* () { if (method === "Network.clearBrowserCookies") await cookiesCleared?.(); return result; }; + const waitForFrame = (sessionId?: string) => + runPromise( + Effect.promise(() => + send( + "Runtime.evaluate", + { + expression: + "new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(resolve)))", + awaitPromise: true, + }, + sessionId, + ).catch(() => undefined), + ).pipe(Effect.timeoutOption(SURFACE_WAIT)), + ); const detachedOwners = new Set(); /** Viewers and recordings stream; the agent's own sessions never do. */ const streamingOwners = new Set(); @@ -374,7 +401,29 @@ export const make = Effect.gen(function* () { relay, () => owner === undefined || !detachedOwners.has(owner), (current, held) => - held ? holdFocus(tab, owner, () => forward(current)) : forward(current), + held + ? holdFocus(tab, owner, () => { + const preparing = {}; + tab.preparingScreenshots.set(preparing, owner); + syncPainting(tab); + return forward(current) + .catch(async (cause: unknown) => { + if ( + !isMissingSurface(cause) || + !current() || + !tab.preparingScreenshots.has(preparing) + ) + throw cause; + await waitForFrame(sessionId); + if (!tab.preparingScreenshots.has(preparing)) throw cause; + return forward(current); + }) + .finally(() => { + tab.preparingScreenshots.delete(preparing); + syncPainting(tab); + }); + }) + : forward(current), ) : method === "Page.startScreencast" ? holdFocus(tab, owner, forward).then( @@ -471,6 +520,7 @@ export const make = Effect.gen(function* () { const announced = tab.stopWaiting === null; tab.stopWaiting?.(); tab.relay = null; + tab.preparingScreenshots.clear(); tab.debuggee.debugger.off("message", tab.onMessage); tab.debuggee.webContents.off("did-navigate", tab.onNewDocument); syncPainting(tab); @@ -495,6 +545,7 @@ export const make = Effect.gen(function* () { focusUses: 0, focusPending: new Map(), capturing: new Set(), + preparingScreenshots: new Map(), painting: false, agentUsed: false, focusReleasing: false,