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

[rajarshidattapy]

Description

#106 reported navigate hard-coding waitUntil: 'load'. That fix landed in actions.ts, but the same root cause had a second call site that was never covered: newPage, which backs webcmd browser tab new --url <url> and the hosted tab-open path.

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' });

So browser navigate --wait-until none worked while browser tab new --url still blocked on the load event. On a site that never goes idle — a streaming dashboard, a long-poll app shell, a page with a hanging subresource — opening a tab still hung. Same failure #106 was filed about, different entry point.

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. I did not do that, because a mapping duplicated across two files is exactly what let this bug survive the #106 fix.

Instead the mapping 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'

navigate and newPage now both call it. A future goto in this runtime has one obvious thing to reach for, and the reason for the 'commit' translation is documented once rather than as an inline comment at whichever site happened to be written first.

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
provider.test.ts	New test for the tab-open path

Type of Change

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

Checklist

• I ran the checks relevant to this PR
• I updated tests or docs if needed
• I included output or screenshots when useful

Notes on the checklist:

• npx tsc --noEmit reports one error that is not from this change: src/fetch/client.ts(2,23): Cannot find module 'impit'. impit@0.14.3 is listed in dependencies but was absent from my node_modules — a stale local install. No file touched by this PR is involved. Worth a second look in CI.
• 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, and only for tab-open, which previously ignored it.

Adapter Notes

Not applicable — this is runtime plumbing, no adapter is added or modified.

• Updated generated or lean docs when command discoverability changed
• Used positional args for the command's primary subject unless a named flag is clearly better
• Normalized expected adapter failures to CliError subclasses instead of raw Error

Screenshots / Output

$ npx vitest run --project unit src/browser/runtime/local-cloak/provider.test.ts

 Test Files  1 passed (1)
      Tests  27 passed (27)
   Duration  1.15s

The added test mirrors the existing navigate pair, so both branches of the tab-open path are now pinned:

it("maps waitUntil 'none' to a commit-only wait when opening a tab", async () => {
  const { provider, pages } = makeProviderWithFakePage();
  const result = await provider.dispatch({
    id: 'new',
    action: 'tabs',
    op: 'new',
    session: 'work',
    surface: 'browser',
    url: 'https://second.example/',
    waitUntil: 'none',
    profileId: 'default',
  });
  expect(result).toMatchObject({ id: 'new', ok: true, page: expect.any(String) });
  expect(pages[1].goto).toHaveBeenCalledWith('https://second.example/', expect.objectContaining({ waitUntil: 'commit' }));
});

The pre-existing assertion that tab-open defaults to waitUntil: 'load' is untouched and still passes.

Branch fix/newpage-wait-until is based on upstream/main, still uncommitted and unpushed — commit and git push -u origin fix/newpage-wait-until before opening. Base the PR on main.

One thing to settle first: run npm install and re-run npx tsc --noEmit locally. If the impit error clears, delete that checklist bullet from the body — no reason to draw a reviewer's attention to a problem that only existed on my machine. If it persists after a clean install, it's worth its own issue, and the bullet should stay.

[github-actions]

🟢 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.