fix(browser): make waitFor options-only and race-safe#114
Open
Alezander9 wants to merge 1 commit into
Open
Conversation
This was referenced Jul 23, 2026
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/bcode-browser/src/cdp/session.ts">
<violation number="1" location="packages/bcode-browser/src/cdp/session.ts:199">
P2: The removed positional form with an omitted predicate is not rejected: `waitFor(method, undefined, timeoutMs)` falls through the function-only guard, defaults to `{}`, and silently ignores `timeoutMs`, restoring the 30-second default. This makes JavaScript/snippet callers fail with an unexpectedly long timeout instead of getting the documented migration error; checking for extra arguments (for example `arguments.length > 2`) would make the old form consistently fail.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| * await loaded | ||
| */ | ||
| waitFor<T = unknown>(method: string, opts: { predicate?: (params: T) => boolean; timeoutMs?: number } = {}): Promise<T> { | ||
| if (typeof opts === 'function') { |
There was a problem hiding this comment.
P2: The removed positional form with an omitted predicate is not rejected: waitFor(method, undefined, timeoutMs) falls through the function-only guard, defaults to {}, and silently ignores timeoutMs, restoring the 30-second default. This makes JavaScript/snippet callers fail with an unexpectedly long timeout instead of getting the documented migration error; checking for extra arguments (for example arguments.length > 2) would make the old form consistently fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bcode-browser/src/cdp/session.ts, line 199:
<comment>The removed positional form with an omitted predicate is not rejected: `waitFor(method, undefined, timeoutMs)` falls through the function-only guard, defaults to `{}`, and silently ignores `timeoutMs`, restoring the 30-second default. This makes JavaScript/snippet callers fail with an unexpectedly long timeout instead of getting the documented migration error; checking for extra arguments (for example `arguments.length > 2`) would make the old form consistently fail.</comment>
<file context>
@@ -188,21 +188,42 @@ export class Session implements Transport {
+ * await loaded
+ */
+ waitFor<T = unknown>(method: string, opts: { predicate?: (params: T) => boolean; timeoutMs?: number } = {}): Promise<T> {
+ if (typeof opts === 'function') {
+ throw new TypeError('waitFor(method, { predicate, timeoutMs }) — pass the predicate in the options object');
+ }
</file context>
Suggested change
| if (typeof opts === 'function') { | |
| if (arguments.length > 2 || typeof opts === 'function') { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 beforePage.navigateso 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.catchmasked 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 insession.tsmatch 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
TypeErrorwhen 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.catchon the returned promise.Also intentionally not carried over from #111:
void loaded.catch(() => {})incantation —waitFornow 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'serrorTextbehavior is noted in one line (that's real CDP knowledge worth teaching, the branching isn't).waitFor— real question, but it's a semantics change beyond fix(browser): make waitFor options explicit and race-safe #110's scope; should be its own decision if traces show it biting.navigate()wrapper —session.tsstays 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 passbun test(package) — 16 pass, 8 env-gated skips, 1 pre-existing unrelated failure (edit + cache-bustworkspace import; fails identically onmain)bun typecheckfrompackages/bcode-browser— cleanSummary by cubic
Make
waitForoptions-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 now throws synchronously.Page.navigateto avoid missing a fastPage.loadEventFired.Migration
waitFor(method, predicate?, timeoutMs?)withwaitFor(method, { predicate, timeoutMs }).const loaded = session.waitFor("Page.loadEventFired", { timeoutMs })beforePage.navigate({ url }), thenawait loaded.waitFor(...).catch(() => {})that was only used to silence unhandled rejections.Written for commit 089681b. Summary will update on new commits.