diff --git a/apps/server/src/mcp/PreviewAutomationBroker.test.ts b/apps/server/src/mcp/PreviewAutomationBroker.test.ts index 0fb3b610e5d59..1c193096db273 100644 --- a/apps/server/src/mcp/PreviewAutomationBroker.test.ts +++ b/apps/server/src/mcp/PreviewAutomationBroker.test.ts @@ -335,6 +335,43 @@ it.effect.each([ ), ); +it.effect("names the click target that matched nothing when the server browser times out", () => + Effect.scoped( + Effect.gen(function* () { + const broker = yield* makeBroker; + const clientId = SERVER_BROWSER_AUTOMATION_CLIENT_ID; + const requests = requestsFrom(yield* broker.connect(makeHost({ clientId }))); + yield* Stream.runForEach(requests, (request) => + broker.respond({ + clientId, + connectionId: request.connectionId, + requestId: request.requestId, + ok: false, + error: { + _tag: "PreviewAutomationTimeoutError", + message: "No element matches the target.", + detail: { reason: 'No element matches ".definitely-no-such-t3-target".' }, + }, + }), + ).pipe(Effect.forkScoped); + yield* Effect.yieldNow; + + const error = yield* broker + .invoke({ + scope, + operation: "click", + input: { selector: ".definitely-no-such-t3-target", timeoutMs: 15_000 }, + }) + .pipe(Effect.flip); + + expect(error._tag).toBe("PreviewAutomationTimeoutError"); + expect(error.message).toBe( + 'Preview automation click timed out after 15000ms. No element matches ".definitely-no-such-t3-target".', + ); + }), + ), +); + it.effect("distinguishes malformed remote failures", () => Effect.scoped( Effect.gen(function* () { diff --git a/apps/server/src/mcp/PreviewAutomationBroker.ts b/apps/server/src/mcp/PreviewAutomationBroker.ts index cc34ebbce69f3..85646d387160d 100644 --- a/apps/server/src/mcp/PreviewAutomationBroker.ts +++ b/apps/server/src/mcp/PreviewAutomationBroker.ts @@ -205,6 +205,14 @@ const readRemoteTimeoutMs = (detail: unknown): number | undefined => { : undefined; }; +const readRemoteReason = (detail: unknown): string | undefined => + typeof detail === "object" && + detail !== null && + "reason" in detail && + typeof detail.reason === "string" + ? detail.reason + : undefined; + const MAX_REASON_CHARS = 500; const classifyResponseError = ( @@ -255,9 +263,13 @@ const classifyResponseError = ( }); case "PreviewAutomationTimeoutError": { const remoteTimeoutMs = readRemoteTimeoutMs(error.detail); + const remoteReason = readRemoteReason(error.detail); return new PreviewAutomationTimeoutError({ ...context, ...remoteDiagnostics, + ...(context.clientId === SERVER_BROWSER_AUTOMATION_CLIENT_ID && remoteReason !== undefined + ? { reason: remoteReason.slice(0, MAX_REASON_CHARS) } + : {}), ...(remoteTimeoutMs === undefined ? {} : { timeoutMs: Math.min(context.timeoutMs, remoteTimeoutMs) }), diff --git a/apps/server/src/preview/ServerBrowserPage.test.ts b/apps/server/src/preview/ServerBrowserPage.test.ts index d58fe63047bd5..2c6d3a01bcf74 100644 --- a/apps/server/src/preview/ServerBrowserPage.test.ts +++ b/apps/server/src/preview/ServerBrowserPage.test.ts @@ -98,6 +98,27 @@ describe("server browser element refs", () => { expect(await page.locator("li").count()).toBe(5); }); + it("tells a click target that matches nothing from one that never becomes clickable", async () => { + await page.setContent(``); + const failure = (selector: string) => + ServerBrowserPage.click(page, { selector, timeoutMs: 500 }).then( + () => expect.unreachable(), + ServerBrowserPage.toOperationError, + ); + + const absent = await failure(".definitely-no-such-t3-target"); + expect(absent.tag).toBe("PreviewAutomationTimeoutError"); + expect(absent.message).toBe( + 'The click may have happened and removed its target or navigated; check the page before retrying. No element matches ".definitely-no-such-t3-target" after the timeout; take a fresh snapshot and use its locator.', + ); + expect(absent.cause).toMatchObject({ name: "TimeoutError" }); + 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.', + ); + }); + it("does not retarget a removed ref to a replacement node", async () => { await page.setContent(""); const locator = buttonLocator((await takeSnapshot()).accessibilityTree, "original"); diff --git a/apps/server/src/preview/ServerBrowserPage.ts b/apps/server/src/preview/ServerBrowserPage.ts index d573cd6387c8b..f128bf445e603 100644 --- a/apps/server/src/preview/ServerBrowserPage.ts +++ b/apps/server/src/preview/ServerBrowserPage.ts @@ -33,8 +33,8 @@ export class ServerBrowserOperationError extends Error { readonly tag: string; readonly detail: unknown; - constructor(tag: string, message: string, detail?: unknown) { - super(message); + constructor(tag: string, message: string, detail?: unknown, options?: ErrorOptions) { + super(message, options); this.tag = tag; this.detail = detail; } @@ -72,6 +72,8 @@ export const toOperationError = ( }; const DEFAULT_TIMEOUT_MS = 15_000; +const TARGET_COUNT_MS = 250; +const MAX_TARGET_CHARS = 200; const pageRefs = new WeakMap }>(); // A compact runtime namespace prevents old refs from aliasing after a server restart. @@ -634,6 +636,58 @@ export const click = async ( ): Promise<{ readonly x: number; readonly y: number }> => { const timeout = input.timeoutMs ?? DEFAULT_TIMEOUT_MS; const locator = targetLocator(page, input); + try { + return await clickTarget(page, locator, input, timeout, pointer); + } catch (cause) { + throw locator === null ? cause : await namedTargetTimeout(locator, input, cause); + } +}; + +/** + * Playwright's timeout names neither the target nor why it waited. Counting only after the + * wait failed keeps auto-wait intact, but the count describes the page after the timeout, not + * what the click did: Playwright can dispatch the click and then time out waiting for the + * navigation it started. + */ +const namedTargetTimeout = async ( + locator: Locator, + input: { readonly locator?: string | undefined; readonly selector?: string | undefined }, + cause: unknown, +) => { + if (!(cause instanceof Error && cause.name === "TimeoutError")) return cause; + const selector = JSON.stringify(input.locator ?? input.selector); + // The warning leads and the target is capped, so a reply cut short still carries the warning. + const target = + selector.length > MAX_TARGET_CHARS ? `${selector.slice(0, MAX_TARGET_CHARS)}...` : selector; + // The host's reply must still beat the broker's short grace after the operation timeout. + let timer: ReturnType | undefined; + const matches = await Promise.race([ + locator.count().catch(() => undefined), + new Promise((resolve) => { + timer = setTimeout(() => resolve(undefined), TARGET_COUNT_MS); + }), + ]).finally(() => clearTimeout(timer)); + const reason = + matches === undefined + ? `The click may have happened; check the page before retrying. Whether ${target} still matches an element is unknown.` + : 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.`; + return new ServerBrowserOperationError( + "PreviewAutomationTimeoutError", + reason, + { reason }, + { cause }, + ); +}; + +const clickTarget = async ( + page: Page, + locator: Locator | null, + input: PreviewAutomationClickInput, + timeout: number, + pointer: PointerReporter, +) => { const point = await targetPoint(page, locator, input, timeout); await pointer(point, "click"); const options = { button: input.button ?? "left", clickCount: input.clickCount ?? 1 } as const; diff --git a/packages/contracts/src/previewAutomation.ts b/packages/contracts/src/previewAutomation.ts index 5862b643e00ae..d225f47f19973 100644 --- a/packages/contracts/src/previewAutomation.ts +++ b/packages/contracts/src/previewAutomation.ts @@ -766,11 +766,12 @@ export class PreviewAutomationTimeoutError extends Schema.TaggedError