fix(browser): harden waitFor event handling#111
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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:206">
P3: Invalid runtime `method` values still create a waiter that can never match a CDP event and only fail at timeout. Validate the required method string alongside the other arguments so invalid JavaScript callers fail immediately.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| predicateOrOptions?: ((params: T) => boolean) | WaitForOptions<T>, | ||
| positionalTimeoutMs?: number, | ||
| ): Promise<T> { | ||
| if ( |
There was a problem hiding this comment.
P3: Invalid runtime method values still create a waiter that can never match a CDP event and only fail at timeout. Validate the required method string alongside the other arguments so invalid JavaScript callers fail immediately.
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 206:
<comment>Invalid runtime `method` values still create a waiter that can never match a CDP event and only fail at timeout. Validate the required method string alongside the other arguments so invalid JavaScript callers fail immediately.</comment>
<file context>
@@ -189,15 +196,45 @@ export class Session implements Transport {
+ predicateOrOptions?: ((params: T) => boolean) | WaitForOptions<T>,
+ positionalTimeoutMs?: number,
+ ): Promise<T> {
+ if (
+ predicateOrOptions !== undefined &&
+ typeof predicateOrOptions !== 'function' &&
</file context>
| if ( | |
| if (typeof method !== 'string') { | |
| throw new TypeError('waitFor method must be a string'); | |
| } | |
| if ( |
There was a problem hiding this comment.
Thanks. I’m intentionally not adding this check: method is already a required TypeScript string, while this PR fixes the overloaded second argument that is valid in the documented JavaScript API. Expanding runtime validation to an unrelated required parameter would add scope without changing the event or navigation race.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
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/skills/browser-execute/SKILL.md">
<violation number="1" location="packages/bcode-browser/skills/browser-execute/SKILL.md:119">
P2: The new `Promise.all` pattern drops the `if (navigation.errorText) throw new Error(...)` check from the navigation pattern. When a page fails to load (e.g., DNS failure, network error), `Page.navigate` returns `{ errorText: 'net::ERR_FAILED' }` — the error text is the only way to detect navigation failure. The new code either lets it silently succeed (if the error page happens to fire a load event) or surfaces a generic timeout error instead of the actionable `errorText`. The PR description claims to "surface Page.navigate.errorText" but this change actively removes that surfacing.</violation>
<violation number="2" location="packages/bcode-browser/skills/browser-execute/SKILL.md:119">
P1: The new `Promise.all` pattern removes the `if (navigation.loaderId) await loaded` guard for same-document navigations. Per the CDP protocol spec, `Page.navigate` omits `loaderId` for same-document navigations (hash changes, `history.pushState`/`replaceState`), and `Page.loadEventFired` does NOT fire in that case. The old code correctly only waited for the load event when `loaderId` was 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 the `scrapeTitles` example which iterates arbitrary URLs, any of which could be a hash/SPA navigation.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| 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.
P1: The new Promise.all pattern removes the if (navigation.loaderId) await loaded guard for same-document navigations. Per the CDP protocol spec, Page.navigate omits loaderId for same-document navigations (hash changes, history.pushState/replaceState), and Page.loadEventFired does NOT fire in that case. The old code correctly only waited for the load event when loaderId was 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 the scrapeTitles example which iterates arbitrary URLs, any of which could be a hash/SPA navigation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bcode-browser/skills/browser-execute/SKILL.md, line 119:
<comment>The new `Promise.all` pattern removes the `if (navigation.loaderId) await loaded` guard for same-document navigations. Per the CDP protocol spec, `Page.navigate` omits `loaderId` for same-document navigations (hash changes, `history.pushState`/`replaceState`), and `Page.loadEventFired` does NOT fire in that case. The old code correctly only waited for the load event when `loaderId` was 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 the `scrapeTitles` example which iterates arbitrary URLs, any of which could be a hash/SPA navigation.</comment>
<file context>
@@ -115,13 +115,10 @@ Common moves:
-// Same-document navigations have no loaderId and do not fire a new load event.
-if (navigation.loaderId) await loaded
+await Promise.all([
+ session.waitFor("Page.loadEventFired", { timeoutMs: 15_000 }),
+ session.Page.navigate({ url: "https://example.com" }),
+])
</file context>
There was a problem hiding this comment.
Fixed in 5f8f76b by moving the protocol-specific branches behind session.navigate(url). The skill now uses one call; the helper waits only when loaderId is present and isDownload is false, and it surfaces errorText immediately. Deterministic tests cover early load events, same-document navigation, downloads, and navigation errors.
| 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.
P2: The new Promise.all pattern drops the if (navigation.errorText) throw new Error(...) check from the navigation pattern. When a page fails to load (e.g., DNS failure, network error), Page.navigate returns { errorText: 'net::ERR_FAILED' } — the error text is the only way to detect navigation failure. The new code either lets it silently succeed (if the error page happens to fire a load event) or surfaces a generic timeout error instead of the actionable errorText. The PR description claims to "surface Page.navigate.errorText" but this change actively removes that surfacing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/bcode-browser/skills/browser-execute/SKILL.md, line 119:
<comment>The new `Promise.all` pattern drops the `if (navigation.errorText) throw new Error(...)` check from the navigation pattern. When a page fails to load (e.g., DNS failure, network error), `Page.navigate` returns `{ errorText: 'net::ERR_FAILED' }` — the error text is the only way to detect navigation failure. The new code either lets it silently succeed (if the error page happens to fire a load event) or surfaces a generic timeout error instead of the actionable `errorText`. The PR description claims to "surface Page.navigate.errorText" but this change actively removes that surfacing.</comment>
<file context>
@@ -115,13 +115,10 @@ Common moves:
-// Same-document navigations have no loaderId and do not fire a new load event.
-if (navigation.loaderId) await loaded
+await Promise.all([
+ session.waitFor("Page.loadEventFired", { timeoutMs: 15_000 }),
+ session.Page.navigate({ url: "https://example.com" }),
+])
</file context>
There was a problem hiding this comment.
Fixed in 5f8f76b by moving the protocol-specific branches behind session.navigate(url). The skill now uses one call; the helper waits only when loaderId is present and isDownload is false, and it surfaces errorText immediately. Deterministic tests cover early load events, same-document navigation, downloads, and navigation errors.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
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:258">
P2: Same-document, download, and failed navigations leave a `Page.loadEventFired` waiter alive for up to 30 seconds. Repeated calls accumulate timers/listeners (and every later page event iterates them); expose cancellation/cleanup for this pre-registered waiter and invoke it on all non-waiting paths.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| */ | ||
| async navigate(url: string, options: NavigateOptions = {}): Promise<Page.NavigateReturn> { | ||
| await this.domains.Page.enable(); | ||
| const loaded = this.waitFor('Page.loadEventFired', { timeoutMs: options.timeoutMs }); |
There was a problem hiding this comment.
P2: Same-document, download, and failed navigations leave a Page.loadEventFired waiter alive for up to 30 seconds. Repeated calls accumulate timers/listeners (and every later page event iterates them); expose cancellation/cleanup for this pre-registered waiter and invoke it on all non-waiting paths.
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 258:
<comment>Same-document, download, and failed navigations leave a `Page.loadEventFired` waiter alive for up to 30 seconds. Repeated calls accumulate timers/listeners (and every later page event iterates them); expose cancellation/cleanup for this pre-registered waiter and invoke it on all non-waiting paths.</comment>
<file context>
@@ -244,6 +249,20 @@ export class Session implements Transport {
+ */
+ async navigate(url: string, options: NavigateOptions = {}): Promise<Page.NavigateReturn> {
+ await this.domains.Page.enable();
+ const loaded = this.waitFor('Page.loadEventFired', { timeoutMs: options.timeoutMs });
+ void loaded.catch(() => {});
+ const navigation = await this.domains.Page.navigate({ url });
</file context>
1 participant
Summary
{ predicate?, timeoutMs? }form while preserving the positionalwaitForAPIsession.navigate(url)so the agent uses one simple call while the implementation registers the load waiter beforePage.navigatePage.navigate.errorText, and wait only for real cross-document loadsCloses #110
Validation
cdp-sessiontest file is lint-clean