From a6bdf2dfd0b85554275187ecf1f321ec7e651294 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Fri, 13 Mar 2026 22:17:57 -0700 Subject: [PATCH] Revert "Browser: scope nested batch failures in switch" This reverts commit aaeb348bb7cbbaebe14a471776909bff129499a3. --- .../pw-tools-core.interactions.batch.test.ts | 131 -------- src/browser/pw-tools-core.interactions.ts | 313 ++---------------- 2 files changed, 32 insertions(+), 412 deletions(-) delete mode 100644 src/browser/pw-tools-core.interactions.batch.test.ts diff --git a/src/browser/pw-tools-core.interactions.batch.test.ts b/src/browser/pw-tools-core.interactions.batch.test.ts deleted file mode 100644 index f566d04bc00..00000000000 --- a/src/browser/pw-tools-core.interactions.batch.test.ts +++ /dev/null @@ -1,131 +0,0 @@ -import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; - -let page: { evaluate: ReturnType } | null = null; - -const getPageForTargetId = vi.fn(async () => { - if (!page) { - throw new Error("test: page not set"); - } - return page; -}); -const ensurePageState = vi.fn(() => {}); -const forceDisconnectPlaywrightForTarget = vi.fn(async () => {}); -const refLocator = vi.fn(() => { - throw new Error("test: refLocator should not be called"); -}); -const restoreRoleRefsForTarget = vi.fn(() => {}); - -const closePageViaPlaywright = vi.fn(async () => {}); -const resizeViewportViaPlaywright = vi.fn(async () => {}); - -vi.mock("./pw-session.js", () => ({ - ensurePageState, - forceDisconnectPlaywrightForTarget, - getPageForTargetId, - refLocator, - restoreRoleRefsForTarget, -})); - -vi.mock("./pw-tools-core.snapshot.js", () => ({ - closePageViaPlaywright, - resizeViewportViaPlaywright, -})); - -let batchViaPlaywright: typeof import("./pw-tools-core.interactions.js").batchViaPlaywright; - -describe("batchViaPlaywright", () => { - beforeAll(async () => { - ({ batchViaPlaywright } = await import("./pw-tools-core.interactions.js")); - }); - - beforeEach(() => { - vi.clearAllMocks(); - page = { - evaluate: vi.fn(async () => "ok"), - }; - }); - - it("propagates evaluate timeouts through batched execution", async () => { - const result = await batchViaPlaywright({ - cdpUrl: "http://127.0.0.1:9222", - targetId: "tab-1", - evaluateEnabled: true, - actions: [{ kind: "evaluate", fn: "() => 1", timeoutMs: 5000 }], - }); - - expect(result).toEqual({ results: [{ ok: true }] }); - expect(page?.evaluate).toHaveBeenCalledWith( - expect.any(Function), - expect.objectContaining({ - fnBody: "() => 1", - timeoutMs: 4500, - }), - ); - }); - - it("supports resize and close inside a batch", async () => { - const result = await batchViaPlaywright({ - cdpUrl: "http://127.0.0.1:9222", - targetId: "tab-1", - actions: [{ kind: "resize", width: 800, height: 600 }, { kind: "close" }], - }); - - expect(result).toEqual({ results: [{ ok: true }, { ok: true }] }); - expect(resizeViewportViaPlaywright).toHaveBeenCalledWith({ - cdpUrl: "http://127.0.0.1:9222", - targetId: "tab-1", - width: 800, - height: 600, - }); - expect(closePageViaPlaywright).toHaveBeenCalledWith({ - cdpUrl: "http://127.0.0.1:9222", - targetId: "tab-1", - }); - }); - - it("propagates nested batch failures to the parent batch result", async () => { - const result = await batchViaPlaywright({ - cdpUrl: "http://127.0.0.1:9222", - targetId: "tab-1", - actions: [ - { - kind: "batch", - actions: [{ kind: "evaluate", fn: "() => 1" }], - }, - ], - }); - - expect(result).toEqual({ - results: [ - { ok: false, error: "act:evaluate is disabled by config (browser.evaluateEnabled=false)" }, - ], - }); - }); - - it("includes all nested batch failures when stopOnError is false", async () => { - const result = await batchViaPlaywright({ - cdpUrl: "http://127.0.0.1:9222", - targetId: "tab-1", - actions: [ - { - kind: "batch", - stopOnError: false, - actions: [ - { kind: "evaluate", fn: "() => 1" }, - { kind: "evaluate", fn: "() => 2" }, - ], - }, - ], - }); - - expect(result).toEqual({ - results: [ - { - ok: false, - error: - "act:evaluate is disabled by config (browser.evaluateEnabled=false); act:evaluate is disabled by config (browser.evaluateEnabled=false)", - }, - ], - }); - }); -}); diff --git a/src/browser/pw-tools-core.interactions.ts b/src/browser/pw-tools-core.interactions.ts index 8c52301af56..852b11bb6dc 100644 --- a/src/browser/pw-tools-core.interactions.ts +++ b/src/browser/pw-tools-core.interactions.ts @@ -1,4 +1,4 @@ -import type { BrowserActRequest, BrowserFormField } from "./client-actions-core.js"; +import type { BrowserFormField } from "./client-actions-core.js"; import { DEFAULT_FILL_FIELD_TYPE } from "./form-fields.js"; import { DEFAULT_UPLOAD_DIR, resolveStrictExistingPathsWithinRoot } from "./paths.js"; import { @@ -8,32 +8,12 @@ import { refLocator, restoreRoleRefsForTarget, } from "./pw-session.js"; -import { - normalizeTimeoutMs, - requireRef, - requireRefOrSelector, - toAIFriendlyError, -} from "./pw-tools-core.shared.js"; -import { closePageViaPlaywright, resizeViewportViaPlaywright } from "./pw-tools-core.snapshot.js"; +import { normalizeTimeoutMs, requireRef, toAIFriendlyError } from "./pw-tools-core.shared.js"; type TargetOpts = { cdpUrl: string; targetId?: string; }; -const MAX_CLICK_DELAY_MS = 5_000; -const MAX_WAIT_TIME_MS = 30_000; -const MAX_BATCH_ACTIONS = 100; - -function resolveBoundedDelayMs(value: number | undefined, label: string, maxMs: number): number { - const normalized = Math.floor(value ?? 0); - if (!Number.isFinite(normalized) || normalized < 0) { - throw new Error(`${label} must be >= 0`); - } - if (normalized > maxMs) { - throw new Error(`${label} exceeds maximum of ${maxMs}ms`); - } - return normalized; -} async function getRestoredPageForTarget(opts: TargetOpts) { const page = await getPageForTargetId(opts); @@ -79,27 +59,17 @@ export async function highlightViaPlaywright(opts: { export async function clickViaPlaywright(opts: { cdpUrl: string; targetId?: string; - ref?: string; - selector?: string; + ref: string; doubleClick?: boolean; button?: "left" | "right" | "middle"; modifiers?: Array<"Alt" | "Control" | "ControlOrMeta" | "Meta" | "Shift">; - delayMs?: number; timeoutMs?: number; }): Promise { - const resolved = requireRefOrSelector(opts.ref, opts.selector); const page = await getRestoredPageForTarget(opts); - const label = resolved.ref ?? resolved.selector!; - const locator = resolved.ref - ? refLocator(page, requireRef(resolved.ref)) - : page.locator(resolved.selector); + const ref = requireRef(opts.ref); + const locator = refLocator(page, ref); const timeout = resolveInteractionTimeoutMs(opts.timeoutMs); try { - const delayMs = resolveBoundedDelayMs(opts.delayMs, "click delayMs", MAX_CLICK_DELAY_MS); - if (delayMs > 0) { - await locator.hover({ timeout }); - await new Promise((r) => setTimeout(r, delayMs)); - } if (opts.doubleClick) { await locator.dblclick({ timeout, @@ -114,84 +84,67 @@ export async function clickViaPlaywright(opts: { }); } } catch (err) { - throw toAIFriendlyError(err, label); + throw toAIFriendlyError(err, ref); } } export async function hoverViaPlaywright(opts: { cdpUrl: string; targetId?: string; - ref?: string; - selector?: string; + ref: string; timeoutMs?: number; }): Promise { - const resolved = requireRefOrSelector(opts.ref, opts.selector); + const ref = requireRef(opts.ref); const page = await getRestoredPageForTarget(opts); - const label = resolved.ref ?? resolved.selector!; - const locator = resolved.ref - ? refLocator(page, requireRef(resolved.ref)) - : page.locator(resolved.selector); try { - await locator.hover({ + await refLocator(page, ref).hover({ timeout: resolveInteractionTimeoutMs(opts.timeoutMs), }); } catch (err) { - throw toAIFriendlyError(err, label); + throw toAIFriendlyError(err, ref); } } export async function dragViaPlaywright(opts: { cdpUrl: string; targetId?: string; - startRef?: string; - startSelector?: string; - endRef?: string; - endSelector?: string; + startRef: string; + endRef: string; timeoutMs?: number; }): Promise { - const resolvedStart = requireRefOrSelector(opts.startRef, opts.startSelector); - const resolvedEnd = requireRefOrSelector(opts.endRef, opts.endSelector); + const startRef = requireRef(opts.startRef); + const endRef = requireRef(opts.endRef); + if (!startRef || !endRef) { + throw new Error("startRef and endRef are required"); + } const page = await getRestoredPageForTarget(opts); - const startLocator = resolvedStart.ref - ? refLocator(page, requireRef(resolvedStart.ref)) - : page.locator(resolvedStart.selector); - const endLocator = resolvedEnd.ref - ? refLocator(page, requireRef(resolvedEnd.ref)) - : page.locator(resolvedEnd.selector); - const startLabel = resolvedStart.ref ?? resolvedStart.selector!; - const endLabel = resolvedEnd.ref ?? resolvedEnd.selector!; try { - await startLocator.dragTo(endLocator, { + await refLocator(page, startRef).dragTo(refLocator(page, endRef), { timeout: resolveInteractionTimeoutMs(opts.timeoutMs), }); } catch (err) { - throw toAIFriendlyError(err, `${startLabel} -> ${endLabel}`); + throw toAIFriendlyError(err, `${startRef} -> ${endRef}`); } } export async function selectOptionViaPlaywright(opts: { cdpUrl: string; targetId?: string; - ref?: string; - selector?: string; + ref: string; values: string[]; timeoutMs?: number; }): Promise { - const resolved = requireRefOrSelector(opts.ref, opts.selector); + const ref = requireRef(opts.ref); if (!opts.values?.length) { throw new Error("values are required"); } const page = await getRestoredPageForTarget(opts); - const label = resolved.ref ?? resolved.selector!; - const locator = resolved.ref - ? refLocator(page, requireRef(resolved.ref)) - : page.locator(resolved.selector); try { - await locator.selectOption(opts.values, { + await refLocator(page, ref).selectOption(opts.values, { timeout: resolveInteractionTimeoutMs(opts.timeoutMs), }); } catch (err) { - throw toAIFriendlyError(err, label); + throw toAIFriendlyError(err, ref); } } @@ -215,20 +168,16 @@ export async function pressKeyViaPlaywright(opts: { export async function typeViaPlaywright(opts: { cdpUrl: string; targetId?: string; - ref?: string; - selector?: string; + ref: string; text: string; submit?: boolean; slowly?: boolean; timeoutMs?: number; }): Promise { - const resolved = requireRefOrSelector(opts.ref, opts.selector); const text = String(opts.text ?? ""); const page = await getRestoredPageForTarget(opts); - const label = resolved.ref ?? resolved.selector!; - const locator = resolved.ref - ? refLocator(page, requireRef(resolved.ref)) - : page.locator(resolved.selector); + const ref = requireRef(opts.ref); + const locator = refLocator(page, ref); const timeout = resolveInteractionTimeoutMs(opts.timeoutMs); try { if (opts.slowly) { @@ -241,7 +190,7 @@ export async function typeViaPlaywright(opts: { await locator.press("Enter", { timeout }); } } catch (err) { - throw toAIFriendlyError(err, label); + throw toAIFriendlyError(err, ref); } } @@ -418,22 +367,18 @@ export async function evaluateViaPlaywright(opts: { export async function scrollIntoViewViaPlaywright(opts: { cdpUrl: string; targetId?: string; - ref?: string; - selector?: string; + ref: string; timeoutMs?: number; }): Promise { - const resolved = requireRefOrSelector(opts.ref, opts.selector); const page = await getRestoredPageForTarget(opts); const timeout = normalizeTimeoutMs(opts.timeoutMs, 20_000); - const label = resolved.ref ?? resolved.selector!; - const locator = resolved.ref - ? refLocator(page, requireRef(resolved.ref)) - : page.locator(resolved.selector); + const ref = requireRef(opts.ref); + const locator = refLocator(page, ref); try { await locator.scrollIntoViewIfNeeded({ timeout }); } catch (err) { - throw toAIFriendlyError(err, label); + throw toAIFriendlyError(err, ref); } } @@ -454,7 +399,7 @@ export async function waitForViaPlaywright(opts: { const timeout = normalizeTimeoutMs(opts.timeoutMs, 20_000); if (typeof opts.timeMs === "number" && Number.isFinite(opts.timeMs)) { - await page.waitForTimeout(resolveBoundedDelayMs(opts.timeMs, "wait timeMs", MAX_WAIT_TIME_MS)); + await page.waitForTimeout(Math.max(0, opts.timeMs)); } if (opts.text) { await page.getByText(opts.text).first().waitFor({ @@ -703,197 +648,3 @@ export async function setInputFilesViaPlaywright(opts: { // Best-effort for sites that don't react to setInputFiles alone. } } - -const MAX_BATCH_DEPTH = 5; - -async function executeSingleAction( - action: BrowserActRequest, - cdpUrl: string, - targetId?: string, - evaluateEnabled?: boolean, - depth = 0, -): Promise { - if (depth > MAX_BATCH_DEPTH) { - throw new Error(`Batch nesting depth exceeds maximum of ${MAX_BATCH_DEPTH}`); - } - const effectiveTargetId = action.targetId ?? targetId; - switch (action.kind) { - case "click": - await clickViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - ref: action.ref, - selector: action.selector, - doubleClick: action.doubleClick, - button: action.button as "left" | "right" | "middle" | undefined, - modifiers: action.modifiers as Array< - "Alt" | "Control" | "ControlOrMeta" | "Meta" | "Shift" - >, - delayMs: action.delayMs, - timeoutMs: action.timeoutMs, - }); - break; - case "type": - await typeViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - ref: action.ref, - selector: action.selector, - text: action.text, - submit: action.submit, - slowly: action.slowly, - timeoutMs: action.timeoutMs, - }); - break; - case "press": - await pressKeyViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - key: action.key, - delayMs: action.delayMs, - }); - break; - case "hover": - await hoverViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - ref: action.ref, - selector: action.selector, - timeoutMs: action.timeoutMs, - }); - break; - case "scrollIntoView": - await scrollIntoViewViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - ref: action.ref, - selector: action.selector, - timeoutMs: action.timeoutMs, - }); - break; - case "drag": - await dragViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - startRef: action.startRef, - startSelector: action.startSelector, - endRef: action.endRef, - endSelector: action.endSelector, - timeoutMs: action.timeoutMs, - }); - break; - case "select": - await selectOptionViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - ref: action.ref, - selector: action.selector, - values: action.values, - timeoutMs: action.timeoutMs, - }); - break; - case "fill": - await fillFormViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - fields: action.fields, - timeoutMs: action.timeoutMs, - }); - break; - case "resize": - await resizeViewportViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - width: action.width, - height: action.height, - }); - break; - case "wait": - if (action.fn && !evaluateEnabled) { - throw new Error("wait --fn is disabled by config (browser.evaluateEnabled=false)"); - } - await waitForViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - timeMs: action.timeMs, - text: action.text, - textGone: action.textGone, - selector: action.selector, - url: action.url, - loadState: action.loadState, - fn: action.fn, - timeoutMs: action.timeoutMs, - }); - break; - case "evaluate": - if (!evaluateEnabled) { - throw new Error("act:evaluate is disabled by config (browser.evaluateEnabled=false)"); - } - await evaluateViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - fn: action.fn, - ref: action.ref, - timeoutMs: action.timeoutMs, - }); - break; - case "close": - await closePageViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - }); - break; - case "batch": { - // Nested batches: delegate recursively - const nestedFailures = ( - await batchViaPlaywright({ - cdpUrl, - targetId: effectiveTargetId, - actions: action.actions, - stopOnError: action.stopOnError, - evaluateEnabled, - depth: depth + 1, - }) - ).results.filter((result) => !result.ok); - if (nestedFailures.length > 0) { - throw new Error( - nestedFailures.map((result) => result.error ?? "Nested batch action failed").join("; "), - ); - } - break; - } - default: - throw new Error(`Unsupported batch action kind: ${(action as { kind: string }).kind}`); - } -} - -export async function batchViaPlaywright(opts: { - cdpUrl: string; - targetId?: string; - actions: BrowserActRequest[]; - stopOnError?: boolean; - evaluateEnabled?: boolean; - depth?: number; -}): Promise<{ results: Array<{ ok: boolean; error?: string }> }> { - const depth = opts.depth ?? 0; - if (depth > MAX_BATCH_DEPTH) { - throw new Error(`Batch nesting depth exceeds maximum of ${MAX_BATCH_DEPTH}`); - } - if (opts.actions.length > MAX_BATCH_ACTIONS) { - throw new Error(`Batch exceeds maximum of ${MAX_BATCH_ACTIONS} actions`); - } - const results: Array<{ ok: boolean; error?: string }> = []; - for (const action of opts.actions) { - try { - await executeSingleAction(action, opts.cdpUrl, opts.targetId, opts.evaluateEnabled, depth); - results.push({ ok: true }); - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - results.push({ ok: false, error: message }); - if (opts.stopOnError !== false) { - break; - } - } - } - return { results }; -}