From 6449644d301a6412b22da9f0d01f05c8e1fea6f7 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 12 Nov 2026 07:16:18 +0000 Subject: [PATCH 1/4] test(server): repro preview click reusing a full timeout per step and dropping its reason Refs #25557. Both tests fail on main: a click whose target appears late still succeeds after timeoutMs, and a disabled target's timeout does not say it is not enabled. AI-assisted; verified with the tests and tools listed above. --- .../server/src/preview/ServerBrowserPage.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/apps/server/src/preview/ServerBrowserPage.test.ts b/apps/server/src/preview/ServerBrowserPage.test.ts index dd1895c9b7361..1817ec36ba3c6 100644 --- a/apps/server/src/preview/ServerBrowserPage.test.ts +++ b/apps/server/src/preview/ServerBrowserPage.test.ts @@ -130,6 +130,22 @@ describe("server browser element refs", () => { expect(hidden.message).toBe( 'The click may have happened; check the page before retrying. "button" still matches an element after the timeout.', ); + await page.setContent(``); + const disabled = await failure("#save"); + expect(disabled.tag).toBe("PreviewAutomationTimeoutError"); + expect(disabled.message).toMatch(/not enabled/); + }); + + it("gives a late target only the click's remaining time, not a fresh timeout", async () => { + await page.setContent(``); + await expect( + ServerBrowserPage.click(page, { selector: "#save", timeoutMs: 2_000 }), + ).rejects.toThrow(); + expect(await page.evaluate(() => document.body.dataset.clicked)).toBeUndefined(); }); it("does not retarget a removed ref to a replacement node", async () => { From e7ddb185b11ce84005b4502c15be34556973e5e1 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 12 Nov 2026 07:25:09 +0000 Subject: [PATCH 2/4] fix(server): preview clicks fail within their timeout and say why A preview click gave scrollIntoViewIfNeeded, boundingBox and click each the full timeoutMs, so a target that appeared late could outlive the broker's grace and reach the agent as a generic timeout. Its timeout message also kept only Playwright's first line, dropping the call-log reason such as "element is not enabled". The click now shares one deadline across its steps, and timeout errors append the last "element is ..." line from Playwright's call log. Fixes pingdotgg/t3code#25557 AI-assisted; verified with the tests and tools listed above. --- .../src/preview/ServerBrowserPage.test.ts | 4 +- apps/server/src/preview/ServerBrowserPage.ts | 37 +++++++++++++------ 2 files changed, 27 insertions(+), 14 deletions(-) diff --git a/apps/server/src/preview/ServerBrowserPage.test.ts b/apps/server/src/preview/ServerBrowserPage.test.ts index 1817ec36ba3c6..b397181115e7f 100644 --- a/apps/server/src/preview/ServerBrowserPage.test.ts +++ b/apps/server/src/preview/ServerBrowserPage.test.ts @@ -128,7 +128,7 @@ describe("server browser element refs", () => { const hidden = await failure("button"); expect(hidden.tag).toBe("PreviewAutomationTimeoutError"); expect(hidden.message).toBe( - 'The click may have happened; check the page before retrying. "button" still matches an element after the timeout.', + 'The click may have happened; check the page before retrying. "button" still matches an element after the timeout. Playwright last saw: element is not visible.', ); await page.setContent(``); const disabled = await failure("#save"); @@ -145,7 +145,7 @@ describe("server browser element refs", () => { await expect( ServerBrowserPage.click(page, { selector: "#save", timeoutMs: 2_000 }), ).rejects.toThrow(); - expect(await page.evaluate(() => document.body.dataset.clicked)).toBeUndefined(); + expect(await page.locator("body").getAttribute("data-clicked")).toBeNull(); }); it("does not retarget a removed ref to a replacement node", async () => { diff --git a/apps/server/src/preview/ServerBrowserPage.ts b/apps/server/src/preview/ServerBrowserPage.ts index 29ba05c21b7ea..9cab14e70391c 100644 --- a/apps/server/src/preview/ServerBrowserPage.ts +++ b/apps/server/src/preview/ServerBrowserPage.ts @@ -60,7 +60,7 @@ export const toOperationError = ( if (cause instanceof Error && cause.name === "TimeoutError") { return new ServerBrowserOperationError({ tag: "PreviewAutomationTimeoutError", - message: firstLine, + message: withActionabilityReason(firstLine, cause), detail: timeoutMs === undefined ? undefined : { timeoutMs }, }); } @@ -82,6 +82,16 @@ export const toOperationError = ( }); }; +/** Playwright puts why an element was not actionable in its call log, below the first line. */ +const withActionabilityReason = (message: string, cause: Error) => { + const reason = cause.message + .split("\n") + .map((line) => line.trim()) + .findLast((line) => line.startsWith("- element is ")) + ?.slice(2); + return reason === undefined ? message : `${message} Playwright last saw: ${reason}.`; +}; + const DEFAULT_TIMEOUT_MS = 15_000; const TARGET_COUNT_MS = 250; const MAX_TARGET_CHARS = 200; @@ -149,11 +159,11 @@ const targetPoint = async ( page: Page, locator: Locator | null, input: { readonly x?: number | undefined; readonly y?: number | undefined }, - timeout: number, + timeout: () => number, ) => { if (locator === null) return { x: input.x ?? 0, y: input.y ?? 0 }; - await locator.scrollIntoViewIfNeeded({ timeout }); - const box = await locator.boundingBox({ timeout }); + await locator.scrollIntoViewIfNeeded({ timeout: timeout() }); + const box = await locator.boundingBox({ timeout: timeout() }); if (box) return { x: box.x + box.width / 2, y: box.y + box.height / 2 }; const viewport = page.viewportSize(); return { x: (viewport?.width ?? 0) / 2, y: (viewport?.height ?? 0) / 2 }; @@ -675,10 +685,12 @@ export const click = async ( input: PreviewAutomationClickInput, pointer: PointerReporter = noPointer, ): Promise<{ readonly x: number; readonly y: number }> => { - const timeout = input.timeoutMs ?? DEFAULT_TIMEOUT_MS; + // One deadline across steps keeps the reply inside the broker's grace; Playwright reads 0 as none. + const deadline = Date.now() + (input.timeoutMs ?? DEFAULT_TIMEOUT_MS); + const remaining = () => Math.max(1, deadline - Date.now()); const locator = targetLocator(page, input); try { - return await clickTarget(page, locator, input, timeout, pointer); + return await clickTarget(page, locator, input, remaining, pointer); } catch (cause) { throw locator === null ? cause : await namedTargetTimeout(locator, input, cause); } @@ -714,10 +726,11 @@ const namedTargetTimeout = async ( : matches === 0 ? `The click may have happened and removed its target or navigated; check the page before retrying. No element matches ${target} after the timeout; take a fresh snapshot and use its locator.` : `The click may have happened; check the page before retrying. ${target} still matches an element after the timeout.`; + const message = withActionabilityReason(reason, cause); return new ServerBrowserOperationError({ tag: "PreviewAutomationTimeoutError", - message: reason, - detail: { reason }, + message, + detail: { reason: message }, cause, }); }; @@ -726,7 +739,7 @@ const clickTarget = async ( page: Page, locator: Locator | null, input: PreviewAutomationClickInput, - timeout: number, + timeout: () => number, pointer: PointerReporter, ) => { const point = await targetPoint(page, locator, input, timeout); @@ -735,7 +748,7 @@ const clickTarget = async ( const clicked = locator === null ? page.mouse.click(point.x, point.y, options) - : locator.click({ ...options, timeout }); + : locator.click({ ...options, timeout: timeout() }); let onDialog = constVoid; const dialogOpened = new Promise<"dialog">((resolve) => { onDialog = () => resolve("dialog"); @@ -816,7 +829,7 @@ export const hover = async ( ) => { const timeout = input.timeoutMs ?? DEFAULT_TIMEOUT_MS; const locator = targetLocator(page, input); - await pointer(await targetPoint(page, locator, input, timeout), "move"); + await pointer(await targetPoint(page, locator, input, () => timeout), "move"); if (locator === null) { await page.mouse.move(input.x ?? 0, input.y ?? 0); return; @@ -867,7 +880,7 @@ export const drag = async ( const timeout = input.timeoutMs ?? DEFAULT_TIMEOUT_MS; const source = targetLocator(page, { locator: input.source })!; const target = targetLocator(page, { locator: input.target })!; - await pointer(await targetPoint(page, source, {}, timeout), "move"); + await pointer(await targetPoint(page, source, {}, () => timeout), "move"); // Observe drag rejection immediately; an unhandled rejection during cursor movement exits the server. const dragFailure = source.dragTo(target, { timeout }).then( () => null, From 26fc6b54eb9bb4d3b94f1960512a7aacf776d99e Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Thu, 12 Nov 2026 07:38:04 +0000 Subject: [PATCH 3/4] fix(server): queued preview clicks keep the broker's deadline and reason A click queued behind other work on its tab started its own timeoutMs only once it ran, so it could still outlive the broker. The click now gets the request's remaining budget. Playwright also dims call-log lines with ANSI escapes when colour is on, which hid the actionability reason; the escapes are stripped before matching. Refs pingdotgg/t3code#25557 AI-assisted; verified with the tests and tools listed above. --- apps/server/src/preview/ServerBrowser.ts | 16 ++++++++++++---- apps/server/src/preview/ServerBrowserPage.ts | 4 +++- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/apps/server/src/preview/ServerBrowser.ts b/apps/server/src/preview/ServerBrowser.ts index f06c6b34d41b5..55ed91786473a 100644 --- a/apps/server/src/preview/ServerBrowser.ts +++ b/apps/server/src/preview/ServerBrowser.ts @@ -2316,7 +2316,7 @@ const make = Effect.gen(function* () { message: "A browser dialog is pending. Read preview_status.", reason: "dialogPending", }); - return executeTabOperation(tab, request); + return executeTabOperation(tab, request, deadlineMs); }); } return tab.control.agent( @@ -2332,7 +2332,7 @@ const make = Effect.gen(function* () { tab.agentOperating = !READ_OPERATIONS.has(request.operation); updateFileChooserInterception(tab); try { - return await executeTabOperation(tab, request); + return await executeTabOperation(tab, request, deadlineMs); } finally { tab.agentOperating = false; updateFileChooserInterception(tab); @@ -2343,7 +2343,11 @@ const make = Effect.gen(function* () { ); }; - const executeTabOperation = async (tab: ServerTab, request: PreviewAutomationRequest) => { + const executeTabOperation = async ( + tab: ServerTab, + request: PreviewAutomationRequest, + deadlineMs: number, + ) => { const input = request.input; switch (request.operation) { case "navigate": { @@ -2390,7 +2394,11 @@ const make = Effect.gen(function* () { return loadError ? { ...snapshot, url: loadError.url, loadError } : snapshot; } case "click": { - const clickInput = input as PreviewAutomationClickInput; + // Time spent queued behind other work on this tab counts against the broker's deadline. + const clickInput = { + ...(input as PreviewAutomationClickInput), + timeoutMs: Math.max(1, Math.floor(deadlineMs - performance.now())), + }; await recordAction(tab, "click", () => ServerBrowserPage.click(tab.page, clickInput, pointerFor(tab)), ); diff --git a/apps/server/src/preview/ServerBrowserPage.ts b/apps/server/src/preview/ServerBrowserPage.ts index 9cab14e70391c..885ac873d1722 100644 --- a/apps/server/src/preview/ServerBrowserPage.ts +++ b/apps/server/src/preview/ServerBrowserPage.ts @@ -22,6 +22,7 @@ import { constVoid } from "effect/Function"; import * as Schema from "effect/Schema"; import type { CDPSession, ConsoleMessage, Frame, Locator, Page } from "playwright-core"; import * as NodeCrypto from "node:crypto"; +import * as NodeUtil from "node:util"; import { BrowserControlInterrupted } from "./SessionControl.ts"; const isBrowserControlInterrupted = Schema.is(BrowserControlInterrupted); @@ -84,7 +85,8 @@ export const toOperationError = ( /** Playwright puts why an element was not actionable in its call log, below the first line. */ const withActionabilityReason = (message: string, cause: Error) => { - const reason = cause.message + // Playwright dims call-log lines with ANSI escapes when colour is on. + const reason = NodeUtil.stripVTControlCharacters(cause.message) .split("\n") .map((line) => line.trim()) .findLast((line) => line.startsWith("- element is ")) From 0f02232e6cb6c22785d3c6e716c1acda3697eb32 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Fri, 13 Nov 2026 16:26:35 +0000 Subject: [PATCH 4/4] test(server): drop the disabled-button case that repeats the hidden one Co-Authored-By: Claude Opus 5.5 --- apps/server/src/preview/ServerBrowserPage.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/apps/server/src/preview/ServerBrowserPage.test.ts b/apps/server/src/preview/ServerBrowserPage.test.ts index 781ae255adcac..22365cc5671a9 100644 --- a/apps/server/src/preview/ServerBrowserPage.test.ts +++ b/apps/server/src/preview/ServerBrowserPage.test.ts @@ -175,10 +175,6 @@ describe("server browser element refs", () => { expect(hidden.message).toBe( 'The click may have happened; check the page before retrying. "button" still matches an element after the timeout. Playwright last saw: element is not visible.', ); - await page.setContent(``); - const disabled = await failure("#save"); - expect(disabled.tag).toBe("PreviewAutomationTimeoutError"); - expect(disabled.message).toMatch(/not enabled/); }); it("gives a late target only the click's remaining time, not a fresh timeout", async () => {