From 089681b1f99864088c1d024e25adbbdbb6859de6 Mon Sep 17 00:00:00 2001 From: Alexander Yue Date: Thu, 23 Jul 2026 13:17:29 -0700 Subject: [PATCH] fix(browser): make waitFor options-only and register load waiters before navigate --- .../skills/browser-execute/SKILL.md | 9 ++- packages/bcode-browser/src/cdp/session.ts | 31 +++++++-- .../test/browser-execute.test.ts | 6 +- .../bcode-browser/test/cdp-session.test.ts | 66 +++++++++++++++++++ packages/bcode-browser/test/cdp-smoke.test.ts | 3 +- 5 files changed, 104 insertions(+), 11 deletions(-) create mode 100644 packages/bcode-browser/test/cdp-session.test.ts diff --git a/packages/bcode-browser/skills/browser-execute/SKILL.md b/packages/bcode-browser/skills/browser-execute/SKILL.md index 1e6ae048b0..ff8c9c6228 100644 --- a/packages/bcode-browser/skills/browser-execute/SKILL.md +++ b/packages/bcode-browser/skills/browser-execute/SKILL.md @@ -113,10 +113,12 @@ For unknown param shapes, call with `{}` and inspect the thrown `CdpError` — ` Common moves: ```js -// Navigate. +// Navigate. Register the load waiter BEFORE navigate so a fast load isn't missed. await session.Page.enable() +const loaded = session.waitFor("Page.loadEventFired", { timeoutMs: 15_000 }) await session.Page.navigate({ url: "https://example.com" }) -await session.waitFor("Page.loadEventFired") +await loaded +// Page.navigate resolves even on network errors — its result carries `errorText` when the load failed. // Evaluate JS in the page. const r = await session.Runtime.evaluate({ @@ -153,8 +155,9 @@ export async function scrapeTitles(session: any, urls: string[]) { const titles: string[] = [] await session.Page.enable() for (const url of urls) { + const loaded = session.waitFor("Page.loadEventFired", { timeoutMs: 15_000 }) await session.Page.navigate({ url }) - await session.waitFor("Page.loadEventFired") + await loaded const r = await session.Runtime.evaluate({ expression: "document.title", returnByValue: true }) titles.push(r.result.value) } diff --git a/packages/bcode-browser/src/cdp/session.ts b/packages/bcode-browser/src/cdp/session.ts index 6267868276..18084d900f 100644 --- a/packages/bcode-browser/src/cdp/session.ts +++ b/packages/bcode-browser/src/cdp/session.ts @@ -188,21 +188,42 @@ export class Session implements Transport { }; } - /** Wait for the next event matching `method` (and optional predicate). */ - waitFor(method: string, predicate?: (params: T) => boolean, timeoutMs = 30_000): Promise { - return new Promise((resolve, reject) => { + /** + * Wait for the next event matching `method` (and optional predicate). + * Register the waiter before the call that triggers the event: + * const loaded = session.waitFor("Page.loadEventFired", { timeoutMs: 15_000 }) + * await session.Page.navigate({ url }) + * await loaded + */ + waitFor(method: string, opts: { predicate?: (params: T) => boolean; timeoutMs?: number } = {}): Promise { + if (typeof opts === 'function') { + throw new TypeError('waitFor(method, { predicate, timeoutMs }) — pass the predicate in the options object'); + } + const p = new Promise((resolve, reject) => { const timer = setTimeout(() => { unsub(); reject(new Error(`Timeout waiting for ${method}`)); - }, timeoutMs); + }, opts.timeoutMs ?? 30_000); const unsub = this.onEvent((m, params) => { if (m !== method) return; - if (predicate && !predicate(params as T)) return; + try { + if (opts.predicate && !opts.predicate(params as T)) return; + } catch (e) { + clearTimeout(timer); + unsub(); + reject(e); + return; + } clearTimeout(timer); unsub(); resolve(params as T); }); }); + // Pre-observe so an abandoned waiter (snippet returned or threw before + // awaiting it) times out without an unhandled rejection. Awaiting + // callers still see the rejection. + p.catch(() => {}); + return p; } // Transport implementation. Called by the generated domain bindings. diff --git a/packages/bcode-browser/test/browser-execute.test.ts b/packages/bcode-browser/test/browser-execute.test.ts index fa11b6c6eb..5b1ca9eb49 100644 --- a/packages/bcode-browser/test/browser-execute.test.ts +++ b/packages/bcode-browser/test/browser-execute.test.ts @@ -87,8 +87,9 @@ test.skipIf(!enabled)("workspace import inside a snippet", async () => { await session.use(page.targetId) } await session.Page.enable() + const loaded = session.waitFor("Page.loadEventFired", { timeoutMs: 5000 }) await session.Page.navigate({ url: "data:text/html,bcode-be" }) - await session.waitFor("Page.loadEventFired", undefined, 5000) + await loaded const r = await session.Runtime.evaluate({ expression: "document.title", returnByValue: true }) return r.result.value }`, @@ -125,8 +126,9 @@ test.skipIf(!enabled)("Page.captureScreenshot is collected into result.screensho { description: "Capture two screenshots", code: `await session.Page.enable(); + const loaded = session.waitFor("Page.loadEventFired", { timeoutMs: 5000 }); await session.Page.navigate({ url: "data:text/html,shothi" }); - await session.waitFor("Page.loadEventFired", undefined, 5000); + await loaded; const a = await session.Page.captureScreenshot({ format: "png" }); const b = await session.Page.captureScreenshot({ format: "jpeg", quality: 50 }); return { aLen: a.data.length, bLen: b.data.length };`, diff --git a/packages/bcode-browser/test/cdp-session.test.ts b/packages/bcode-browser/test/cdp-session.test.ts new file mode 100644 index 0000000000..6966d95964 --- /dev/null +++ b/packages/bcode-browser/test/cdp-session.test.ts @@ -0,0 +1,66 @@ +// waitFor semantics against a bare WebSocket server (no Chrome needed). +// Test structure adapted from PR #111 by @MagMueller. +import { afterAll, beforeAll, expect, test } from "bun:test" +import { Session } from "../src/cdp/session" + +const channel = "cdp-events" +const server = Bun.serve({ + port: 0, + fetch(req, srv) { + return srv.upgrade(req) ? undefined : new Response("nope", { status: 400 }) + }, + websocket: { + open(ws) { + ws.subscribe(channel) + }, + message() {}, + }, +}) +const session = new Session() +const emit = (method: string, params: unknown) => { + server.publish(channel, JSON.stringify({ method, params })) +} + +beforeAll(async () => { + await session.connect({ wsUrl: `ws://127.0.0.1:${server.port}/` }) +}) + +afterAll(() => { + session.close() + server.stop(true) +}) + +test("waitFor resolves on a matching event, respecting the predicate", async () => { + const waiting = session.waitFor<{ ready: boolean }>("Test.event", { + predicate: (params) => params.ready, + timeoutMs: 1_000, + }) + emit("Test.event", { ready: false }) + emit("Test.event", { ready: true }) + expect(await waiting).toEqual({ ready: true }) +}) + +test("waitFor honors timeoutMs", async () => { + await expect(session.waitFor("Test.timeout", { timeoutMs: 20 })).rejects.toThrow("Timeout waiting for Test.timeout") +}) + +test("waitFor rejects and unsubscribes when a predicate throws", async () => { + let calls = 0 + const waiting = session.waitFor("Test.bad", { + predicate: () => { + calls++ + throw new Error("predicate failed") + }, + timeoutMs: 1_000, + }) + emit("Test.bad", {}) + await expect(waiting).rejects.toThrow("predicate failed") + emit("Test.bad", {}) + await Bun.sleep(10) + expect(calls).toBe(1) +}) + +test("waitFor throws synchronously on the removed positional-predicate form", () => { + // @ts-expect-error old signature: waitFor(method, predicate, timeoutMs) + expect(() => session.waitFor("Test.positional", () => true, 1_000)).toThrow(TypeError) +}) diff --git a/packages/bcode-browser/test/cdp-smoke.test.ts b/packages/bcode-browser/test/cdp-smoke.test.ts index f1fe257798..426f767eb3 100644 --- a/packages/bcode-browser/test/cdp-smoke.test.ts +++ b/packages/bcode-browser/test/cdp-smoke.test.ts @@ -32,8 +32,9 @@ test.skipIf(!enabled)("Session connects, navigates, reads title", async () => { } await session.domains.Page.enable() + const loaded = session.waitFor("Page.loadEventFired", { timeoutMs: 5000 }) await session.domains.Page.navigate({ url: "data:text/html,bcode-smoke" }) - await session.waitFor("Page.loadEventFired", undefined, 5000) + await loaded const r = (await session.domains.Runtime.evaluate({ expression: "document.title",