Skip to content

fix: honor waitUntil when opening a tab, not just when navigating - #212

Open
rajarshidattapy wants to merge 4 commits into
agentrhq:mainfrom
rajarshidattapy:fix/newpage-wait-until
Open

fix: honor waitUntil when opening a tab, not just when navigating#212
rajarshidattapy wants to merge 4 commits into
agentrhq:mainfrom
rajarshidattapy:fix/newpage-wait-until

Conversation

@rajarshidattapy

@rajarshidattapy rajarshidattapy commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Description

waitUntil was plumbed through every layer of the local Cloak runtime except one: newPage. #106 reported navigate hard-coding waitUntil: 'load' and that fix landed in actions.ts, but the same root cause had a second call site that was never covered.

Before this PR:

// actions.ts — navigate, fixed by #106/#107
await lease.page.goto(command.url, { waitUntil: command.waitUntil === 'none' ? 'commit' : 'load' });

// session-manager.ts — newPage, still hardcoded
await acquired.page.goto(input.url, { waitUntil: 'load' });

On a site that never goes idle — a streaming dashboard, a long-poll app shell, a page with a hanging subresource — opening a page still blocked on the load event even when the caller explicitly asked to skip it.

Closes #210

Why it survived the first fix

newPage had no waitUntil in its signature, and its only caller dropped the field even though it was reading it off the same command object:

// actions.ts — 'tabs' / 'new'
const lease = await manager.newPage({
  profileId: resolveCloakCommandProfileId(manager, command),
  session: command.session,
  surface: command.surface,
  siteSession: command.siteSession,
  idleTimeout: command.idleTimeout,
  url: command.url,
  windowMode: command.windowMode,
  // command.waitUntil was never passed
});

Every other layer was already plumbed — protocol.ts carries waitUntil?: 'load' | 'none' on BrowserRuntimeCommand, and base-page.ts, cdp.ts, and page.ts all accept and honor it. newPage's input type was the only link in the chain missing it.

Approach

The three-line version of this fix would copy the 'none' ? 'commit' : 'load' ternary into newPage. That mapping duplicated across two files is exactly what let this bug survive the #106 fix, so instead it lives in one exported helper that both call sites use:

/**
 * Map the protocol's navigation wait condition onto Playwright's `goto` option.
 * 'none' becomes 'commit': sites that stream analytics forever never fire the
 * load event, so callers gating readiness on their own selector waits must be
 * able to skip it. Every `goto` in this runtime routes through here so a new
 * call site cannot quietly reintroduce a hardcoded 'load'.
 */
export function toGotoWaitUntil(waitUntil?: 'load' | 'none'): 'load' | 'commit'

Scope change after merging main

This PR was opened against a main that still exposed webcmd browser tab new --url <url> as a CLI command. That surface no longer exists: the public browser surface is now tabs, bind, run, snapshot, and close, and tabs is list-only.

The CLI-facing half of the original PR has therefore been dropped as obsolete:

  • the --wait-until option on tab new and its resolveBrowserWaitUntil helper in cli.ts
  • the matching cli.test.ts cases
  • the tab/new option contract in command-catalog.ts
  • the docs/cli-reference.mdx "Browser Runtime" section and the SKILL.md browser tab new row

The runtime bug itself is unaffected by that restructure and is still live on main today (session-manager.ts still hardcodes waitUntil: 'load', actions.ts still drops command.waitUntil). The path is now reached through the adapter-facing IPage.newTab() API rather than from the CLI, so the fix still matters — it just no longer needs a CLI flag attached to it.

Changes

File Change
session-manager.ts Added toGotoWaitUntil(); widened newPage input with waitUntil?: 'load' | 'none'; replaced the hardcoded 'load'
actions.ts tabs/new now forwards command.waitUntil; navigate switched to the shared helper
page.ts, types.ts newTab(url?, options?: { waitUntil?: 'load' | 'none' })
page.test.ts, provider.test.ts Tests for the forwarding and the commit-only wait

Type of Change

  • 🐛 Bug fix
  • ✨ New feature
  • 🌐 New site adapter
  • 📝 Documentation
  • ♻️ Refactor
  • 🔧 CI / build / tooling

Hosted mode

This fix is local-Cloak only. Hosted newTab ignores the wait condition entirely, and hosted navigate maps 'none' to domcontentloaded rather than commit, so hosted behavior still diverges after this merges. Tracked separately in agentrhq/webcmd-cloud#30 — this PR should not be read as closing that gap.

Verification

Run against main merged into this branch:

tsc --noEmit                clean
npm test                    4532 passed | 1 skipped (378 files)
npm run build               OK
check:hosted-contract       PASS
check:typed-error-lint      PASS
check:silent-column-drop    PASS
check:package-bin           PASS

check:hosted-contract is unchanged because command-catalog.ts now matches main exactly.

No behavior change for the default path: omitting waitUntil, or passing 'load', still waits for the load event exactly as before. Only the explicit 'none' case differs.

The Cannot find module 'impit' error mentioned in the original description was a stale local node_modules and does not reproduce on a clean install. No action needed.

…duce the bug:

- session-manager.ts:24-33 — new exported toGotoWaitUntil() carrying the 'none' → 'commit' mapping and the explanatory comment that previously lived inline at the navigate site.
- session-manager.ts:248 — newPage input widened with waitUntil?: 'load' | 'none'.
- session-manager.ts:260 — the hardcoded 'load' replaced with toGotoWaitUntil(input.waitUntil).
- actions.ts:161 — tabs/new now passes waitUntil: command.waitUntil through, which it was silently dropping.
- actions.ts:113 — navigate switched to the same helper, so both paths share one implementation.

I used a shared helper rather than copying the ternary into newPage. Duplicating it would have been a two-line diff, but a duplicated mapping in two files is precisely what let this bug survive the agentrhq#106 fix.

Verification

- npx vitest run --project unit src/browser/runtime/local-cloak/provider.test.ts — 27 passed. That includes a new test mirroring the existing navigate pair: tabs/new with waitUntil: 'none' now asserts goto receives 'commit'. The pre-existing test at line 318 still asserts the default is 'load', so both branches are covered.
- npx tsc --noEmit reports one error, and it is not from this change: src/fetch/client.ts(2,23): Cannot find module 'impit'. impit@0.14.3 is in package.json dependencies but absent from node_modules here — a stale local install, not a code problem. Run npm install and it should clear; worth confirming on your side before you push, since I can't distinguish "not installed locally" from "genuinely broken on main" without it.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🟢 No documentation gap found — medium confidence

The automated review found no documentation gap in the supplied changes.

This review is advisory and does not block merging.

rajarshidattapy and others added 2 commits August 5, 2026 02:47
# Conflicts:
#	docs/cli-reference.mdx
#	skills/webcmd-browser/SKILL.md
#	src/browser/command-catalog.ts
#	src/cli.test.ts
#	src/cli.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

``waitUntil: 'load'is still hardcoded innewPage— the #106 fix reachednavigateonly, sotab new --url still hangs on never-idle sites

2 participants