fix(browser): make waitFor options-only and race-safe

Merged
AL
4mo ago
+128-11

Summary

Replaces #111. Fixes #110.

waitFor's second parameter is now an options object — waitFor(method, { predicate?, timeoutMs? }) — and the positional (method, predicate?, timeoutMs?) form is removed. The skill's navigation examples register the load waiter before Page.navigate so a fast load event can't be missed.

Why this shape instead of #111

Both real bugs from #110 are fixed here; the difference is how much API we own afterwards.

The evidence that settled the signature: in observed agent traces, a model that wanted a navigation timeout wrote waitFor("Page.loadEventFired", { timeoutMs: 15000 }).catch(() => {}) — under the positional signature the object became the predicate, the predicate throw was swallowed, the waiter ran to the default 30s, and the defensive .catch masked the timeout. A silent flat 30s tax on every navigation. The options object is the shape models assume from priors (Puppeteer/Playwright), so we make the one invented signature in session.ts match the prior instead of asking models to learn ours.

Given that, the overload machinery in #111 (positional + options forms, argument-shape validation ladder) isn't needed — there is exactly one call shape, and the only guard is a synchronous TypeError when a function is passed where options belong, so stragglers fail loud at the call site instead of stalling silently. Sync-throw also can't be swallowed by a .catch on the returned promise.

Also intentionally not carried over from #111:

  • Snippet-side void loaded.catch(() => {}) incantation — waitFor now pre-observes its own promise, so an abandoned waiter times out without an unhandled rejection. Fixed once in the session layer instead of taught in the skill forever.
  • loaderId / same-document branching in the skill examples — the canonical example stays the 3-line happy path; Page.navigate's errorText behavior is noted in one line (that's real CDP knowledge worth teaching, the branching isn't).
  • Active-session event filtering in waitFor — real question, but it's a semantics change beyond #110's scope; should be its own decision if traces show it biting.
  • navigate() wrapper — session.ts stays a transport shim; navigation composition belongs to the snippet, guided by the skill.

Predicate-throw → reject (with unsubscribe) is kept from #111, as is the mock-WS test structure (credited in the test file).

Validation

  • bun test test/cdp-session.test.ts — 4 pass
  • bun test (package) — 16 pass, 8 env-gated skips, 1 pre-existing unrelated failure (edit + cache-bust workspace import; fails identically on main)
  • bun typecheck from packages/bcode-browser — clean

Summary by cubic

Make waitFor options-only and race-safe. Prevents silent 30s stalls and ensures navigation waiters catch fast load events.

  • Bug Fixes

    • waitFor(method, { predicate?, timeoutMs? }) is the only supported form; passing a function as the second arg or a positional third-arg timeout now throws synchronously.
    • Abandoned waiters no longer create unhandled rejections; predicate errors reject immediately and unsubscribe.
    • Examples/tests now register the load waiter before Page.navigate to avoid missing a fast Page.loadEventFired.
  • Migration

    • Replace waitFor(method, predicate?, timeoutMs?) with waitFor(method, { predicate, timeoutMs }).
    • Register const loaded = session.waitFor("Page.loadEventFired", { timeoutMs }) before Page.navigate({ url }), then await loaded.
    • Remove waitFor(...).catch(() => {}) that was only used to silence unhandled rejections.