-
Notifications
You must be signed in to change notification settings - Fork 35
fix(browser): harden waitFor event handling #111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
50d806f
9d674e0
576a80a
5f8f76b
a229720
b88f652
ba38df9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -115,8 +115,10 @@ Common moves: | |
| ```js | ||
| // Navigate. | ||
| await session.Page.enable() | ||
| await session.Page.navigate({ url: "https://example.com" }) | ||
| await session.waitFor("Page.loadEventFired") | ||
| await Promise.all([ | ||
| session.waitFor("Page.loadEventFired", { timeoutMs: 15_000 }), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: The new Prompt for AI agents
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 5f8f76b by moving the protocol-specific branches behind |
||
| session.Page.navigate({ url: "https://example.com" }), | ||
| ]) | ||
|
|
||
| // Evaluate JS in the page. | ||
| const r = await session.Runtime.evaluate({ | ||
|
|
@@ -153,8 +155,10 @@ export async function scrapeTitles(session: any, urls: string[]) { | |
| const titles: string[] = [] | ||
| await session.Page.enable() | ||
| for (const url of urls) { | ||
| await session.Page.navigate({ url }) | ||
| await session.waitFor("Page.loadEventFired") | ||
| await Promise.all([ | ||
| session.waitFor("Page.loadEventFired", { timeoutMs: 15_000 }), | ||
| session.Page.navigate({ url }), | ||
| ]) | ||
| const r = await session.Runtime.evaluate({ expression: "document.title", returnByValue: true }) | ||
| titles.push(r.result.value) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -25,6 +25,13 @@ export type ConnectOptions = { | |||||||||||
| timeoutMs?: number; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| export type WaitForOptions<T> = { | ||||||||||||
| /** Only resolve when the event payload matches this predicate. */ | ||||||||||||
| predicate?: (params: T) => boolean; | ||||||||||||
| /** Maximum wait in ms. Default 30000. */ | ||||||||||||
| timeoutMs?: number; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| /** A Chromium-based browser detected as running on this machine. */ | ||||||||||||
| export type DetectedBrowser = { | ||||||||||||
| /** Short label, e.g. 'Google Chrome', 'Brave', 'Comet'. */ | ||||||||||||
|
|
@@ -189,15 +196,47 @@ export class Session implements Transport { | |||||||||||
| } | ||||||||||||
|
|
||||||||||||
| /** Wait for the next event matching `method` (and optional predicate). */ | ||||||||||||
| waitFor<T = unknown>(method: string, predicate?: (params: T) => boolean, timeoutMs = 30_000): Promise<T> { | ||||||||||||
| waitFor<T = unknown>(method: string, options?: WaitForOptions<T>): Promise<T>; | ||||||||||||
| waitFor<T = unknown>(method: string, predicate?: (params: T) => boolean, timeoutMs?: number): Promise<T>; | ||||||||||||
| waitFor<T = unknown>( | ||||||||||||
| method: string, | ||||||||||||
| predicateOrOptions?: ((params: T) => boolean) | WaitForOptions<T>, | ||||||||||||
| positionalTimeoutMs?: number, | ||||||||||||
| ): Promise<T> { | ||||||||||||
| if ( | ||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: Invalid runtime Prompt for AI agents
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I’m intentionally not adding this check: |
||||||||||||
| predicateOrOptions !== undefined && | ||||||||||||
| typeof predicateOrOptions !== 'function' && | ||||||||||||
| (typeof predicateOrOptions !== 'object' || predicateOrOptions === null || Array.isArray(predicateOrOptions)) | ||||||||||||
| ) { | ||||||||||||
| throw new TypeError('waitFor expects a predicate function or an options object'); | ||||||||||||
| } | ||||||||||||
| const options = typeof predicateOrOptions === 'object' ? predicateOrOptions : undefined; | ||||||||||||
| if (options?.predicate !== undefined && typeof options.predicate !== 'function') { | ||||||||||||
| throw new TypeError('waitFor options.predicate must be a function'); | ||||||||||||
| } | ||||||||||||
| const predicate = typeof predicateOrOptions === 'function' ? predicateOrOptions : options?.predicate; | ||||||||||||
| const timeoutMs = options?.timeoutMs ?? positionalTimeoutMs ?? 30_000; | ||||||||||||
| const sessionId = isBrowserLevel(method) ? undefined : this.activeSessionId; | ||||||||||||
| if (!Number.isFinite(timeoutMs) || timeoutMs < 0) { | ||||||||||||
| throw new TypeError('waitFor timeoutMs must be a non-negative finite number'); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return new Promise((resolve, reject) => { | ||||||||||||
| const timer = setTimeout(() => { | ||||||||||||
| unsub(); | ||||||||||||
| reject(new Error(`Timeout waiting for ${method}`)); | ||||||||||||
| }, timeoutMs); | ||||||||||||
| const unsub = this.onEvent((m, params) => { | ||||||||||||
| const unsub = this.onEvent((m, params, eventSessionId) => { | ||||||||||||
| if (m !== method) return; | ||||||||||||
| if (predicate && !predicate(params as T)) return; | ||||||||||||
| if (sessionId !== undefined && eventSessionId !== sessionId) return; | ||||||||||||
| try { | ||||||||||||
| if (predicate && !predicate(params as T)) return; | ||||||||||||
| } catch (error) { | ||||||||||||
| clearTimeout(timer); | ||||||||||||
| unsub(); | ||||||||||||
| reject(error); | ||||||||||||
| return; | ||||||||||||
| } | ||||||||||||
| clearTimeout(timer); | ||||||||||||
| unsub(); | ||||||||||||
| resolve(params as T); | ||||||||||||
|
|
@@ -423,4 +462,3 @@ async function tryReadDevToolsActivePort( | |||||||||||
| return undefined; | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,107 @@ | ||
| 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(ws, raw) { | ||
| const message: unknown = JSON.parse(String(raw)) | ||
| if (typeof message !== "object" || message === null) return | ||
| const method = Reflect.get(message, "method") | ||
| const id = Reflect.get(message, "id") | ||
| if (method !== "Page.navigate" || typeof id !== "number") return | ||
| ws.send(JSON.stringify({ method: "Page.loadEventFired", params: { timestamp: 1 } })) | ||
| ws.send(JSON.stringify({ id, result: { frameId: "frame" } })) | ||
| }, | ||
| close() {}, | ||
| }, | ||
| }) | ||
| const session = new Session() | ||
|
|
||
| beforeAll(async () => { | ||
| await session.connect({ wsUrl: `ws://127.0.0.1:${server.port}/` }) | ||
| }) | ||
|
|
||
| afterAll(() => { | ||
| session.close() | ||
| server.stop(true) | ||
| }) | ||
|
|
||
| const emit = (method: string, params: unknown, sessionId?: string) => { | ||
| server.publish(channel, JSON.stringify({ method, params, sessionId })) | ||
| } | ||
|
|
||
| test("waitFor accepts predicate and timeout options", async () => { | ||
| const waiting = session.waitFor<{ ready: boolean }>("Test.options", { | ||
| predicate: (params) => params.ready, | ||
| timeoutMs: 1_000, | ||
| }) | ||
| emit("Test.options", { ready: false }) | ||
| emit("Test.options", { ready: true }) | ||
| expect(await waiting).toEqual({ ready: true }) | ||
| }) | ||
|
|
||
| test("waitFor options timeout is honored", async () => { | ||
| const started = performance.now() | ||
| await expect(session.waitFor("Test.timeout", { timeoutMs: 20 })).rejects.toThrow("Timeout waiting for Test.timeout") | ||
| expect(performance.now() - started).toBeLessThan(500) | ||
| }) | ||
|
|
||
| test("waitFor rejects and unsubscribes when a predicate throws", async () => { | ||
| let calls = 0 | ||
| const waiting = session.waitFor("Test.predicate-error", { | ||
| predicate: () => { | ||
| calls++ | ||
| throw new Error("predicate failed") | ||
| }, | ||
| timeoutMs: 1_000, | ||
| }) | ||
| emit("Test.predicate-error", {}) | ||
| await expect(waiting).rejects.toThrow("predicate failed") | ||
| emit("Test.predicate-error", {}) | ||
| await Bun.sleep(10) | ||
| expect(calls).toBe(1) | ||
| }) | ||
|
|
||
| test("waitFor retains the positional signature", async () => { | ||
| const waiting = session.waitFor<{ ready: boolean }>("Test.positional", (params) => params.ready, 1_000) | ||
| emit("Test.positional", { ready: true }) | ||
| expect(await waiting).toEqual({ ready: true }) | ||
| }) | ||
|
|
||
| test("a waiter registered before navigation catches an event emitted before the navigation response", async () => { | ||
| const [loaded] = await Promise.all([ | ||
| session.waitFor<{ timestamp: number }>("Page.loadEventFired", { timeoutMs: 1_000 }), | ||
| session.domains.Page.navigate({ url: "https://example.com" }), | ||
| ]) | ||
| expect(loaded).toEqual({ timestamp: 1 }) | ||
| }) | ||
|
|
||
| test("waitFor ignores matching events from another attached session", async () => { | ||
| session.setActiveSession("session-active") | ||
| try { | ||
| const waiting = session.waitFor<{ source: string }>("Page.loadEventFired", { timeoutMs: 1_000 }) | ||
| emit("Page.loadEventFired", { source: "background" }, "session-background") | ||
| emit("Page.loadEventFired", { source: "active" }, "session-active") | ||
| expect(await waiting).toEqual({ source: "active" }) | ||
| } finally { | ||
| session.setActiveSession(undefined) | ||
| } | ||
| }) | ||
|
|
||
| test("waitFor rejects invalid runtime arguments immediately", () => { | ||
| expect(() => | ||
| // @ts-expect-error Runtime callers can still pass invalid JavaScript. | ||
| session.waitFor("Test.invalid-predicate", { predicate: "not a function" }), | ||
| ).toThrow("waitFor options.predicate must be a function") | ||
| expect(() => | ||
| session.waitFor("Test.invalid-timeout", { timeoutMs: Number.NaN }), | ||
| ).toThrow("waitFor timeoutMs must be a non-negative finite number") | ||
| }) |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: The new
Promise.allpattern removes theif (navigation.loaderId) await loadedguard for same-document navigations. Per the CDP protocol spec,Page.navigateomitsloaderIdfor same-document navigations (hash changes,history.pushState/replaceState), andPage.loadEventFireddoes NOT fire in that case. The old code correctly only waited for the load event whenloaderIdwas present. The new code always waits, so every same-document navigation will sit for 15 seconds and then reject with a timeout — even though the navigation succeeded. This is especially impactful in thescrapeTitlesexample which iterates arbitrary URLs, any of which could be a hash/SPA navigation.Prompt for AI agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5f8f76b by moving the protocol-specific branches behind
session.navigate(url). The skill now uses one call; the helper waits only whenloaderIdis present andisDownloadis false, and it surfaceserrorTextimmediately. Deterministic tests cover early load events, same-document navigation, downloads, and navigation errors.