spinner-waiter: a document that commits mid-check gets its own quick check

[jonastemplestein]

The bug: two documents in one decision

After the 1 s quick check fails, spinnerWaiter asks one question: is anything loading? loadingVisible answers with document.readyState and a spinner count. Both describe whatever document the page shows at that moment, and a navigation can swap the document between them.

A click that sets off a redirect chain (OAuth consent → callback → app page) is the common case. pageIsNavigating reads readyState from the page the click left: "complete". The spinner count is held until the navigation commits and then runs on the new page, which has not drawn its spinner yet: 0. "Complete" plus "no spinner" reads as "nothing loading", and the action fails with Timeout 1ms exceeded while the app is mid-redirect. The later loop has a 3 s grace period against spinner flicker; this first decision had none.

What makes the hold happen is Chromium, not Playwright. Chromium's DevTools suspends every renderer-bound message for a frame from DidStartNavigation to DidFinishNavigation (content/browser/devtools/render_frame_devtools_agent_host.cc). So page.evaluate, isVisible() and count() issued during a navigation wait for it to commit and then run on the new document. This has a second consequence: the quick check itself straddles the commit. It spends its second polling the old page, the navigation commits, and the loading check runs on a new page that has had no time to draw anything. Either way the fast-fail hits a document that never got its second.

This is the failure iterate/private hit in CI in test/playwright/dash/integrations.spec.ts ("a project connects X…"): trace with readyState "complete", a spinner count that returned 1.6 s later on the returned page with 0, the 1 ms timeout, and a failure screenshot showing the page's spinner. Any click into a redirect chain is exposed. The spec there polls page.url() as a workaround; this is the root fix.

The test

spec/spinner-waiter.spec.ts › "waits through a redirect chain whose landing page draws its spinner after it commits". A real node:http server (Chromium does not pass a route-fulfilled 302's next hop back through page.route) serves a Connect button that POSTs for 400 ms, then follows consent (302, 150 ms) → callback (302, 150 ms) → project page. The project page draws aria-label="Loading" 850 ms after it commits and the result 2.5 s after. The test clicks with noWaitAfter and waits for the result.

Measured from the click with spinnerWaiter({ log }) and framenavigated, before and after:

ms	origin/main	this branch
26	waitFor starts, quick check begins on the old page	same
429 → 741	navigation requests: consent, callback, project	same
745	project page commits	same
1070	"not ready, nothing loading, failing fast" → Timeout 1ms exceeded	"a new document committed while checking, giving it its own quick check"
2127		spinner seen: "waiting up to 28000ms"
3406		target ready, waitFor passes

	result
origin/main, --repeat-each 20	20/20 failed, locator.waitFor: Timeout 1ms exceeded with the add-a-spinner hint
this branch, --repeat-each 20	20/20 passed (~3.3 s each)

A second spec, "gives up on a page that keeps navigating and blames the navigation", serves a page that reloads itself every 300 ms with spinnerTimeout: 3001 and checks that the action fails within the budget with the "navigation was still in flight" message.

The fix

Count main-frame framenavigated events per page (a WeakMap and one lazily attached listener). The quick check and the loading check run in a loop: if they conclude "not loading" but the main frame navigated since the quick check began, the document that is there now gets its own quick check. One deadline, spinnerTimeout minus the action's last second, covers the quick checks and the spinner wait together, so the total cannot exceed the configured budget. A page still navigating at the deadline counts as loading and fails with the existing "navigation was still in flight" message rather than the no-spinner hint.

Cost: the common fast-fail path (no navigation during the action) is unchanged apart from two WeakMap reads. A fast-fail after a navigation costs one more second per committed document: the existing spec "fails fast once a navigation has fully loaded without the expected element" takes 4.2 s with this change and 3.2 s without (3 runs each, --workers 1). Same-document navigations (pushState) count as commits, because framenavigated does not tell them apart; a route change during an action therefore also buys the new route a second before the fast-fail.

Not taken, and why: tracking pending navigation requests (Chromium hides the pre-commit window from every probe anyway, and it does nothing for the post-commit, pre-spinner window the spec reproduces); reading readyState after the count so both see one document (fixes only the straddled decision, not the spent quick check); a stable "not loading" window before every fast-fail (slows the common path, still misses a spinner drawn late in the new document's first second).

Codex review

Codex (gpt-6-astra, reasoning effort max, read-only sandbox) reviewed the diff against the Playwright v1.60.0 and Chromium 148 sources. Verdict: keep the navigation-counter design, with changes. Taken:

• Budget accounting. Repeated quick checks could consume close to spinnerTimeout and the spinner wait then started another spinnerTimeout - 2000. One deadline now covers both.
• Navigation-aware exhaustion. Budget exhaustion fell into the 1 ms fast-fail with the add-a-spinner hint. A page still navigating at the deadline now counts as loading and fails with the "navigation was still in flight" message. The new reload-loop spec covers it.
• Claims softened. A held query does not run on the new document: Playwright maps its destroyed-context error to false and 0. Comments, README and task note say so now. The counter is mainFrameNavigations, because framenavigated counts same-document navigations too.
• Test server. Startup rejects on error; shutdown calls close() before closeAllConnections().
• noWaitAfter comment. Playwright 1.60's click still passes waitAfter: !noWaitAfter, so the option is not a no-op; the comment states the intent only.
• Common-path cost is two WeakMap reads, not one.

Not taken: a deterministic spec for the straddled decision. It needs a navigation to start in the few milliseconds between the readyState read and the spinner count, which no real page behaviour pins down; the counter check after loadingVisible covers that ordering, and the real-server spec covers the spent quick check. Codex confirmed the edge cases: popups (wrapped synchronously, the first check gets a fresh budget), iframes (child-frame commits do not count), waitForURL, failed navigations (no event; an error-page commit counts), page.close(), and listener lifetime (one per page; the WeakMap does not root the page). It flagged two pre-existing limits this PR leaves alone: isVisible() and count() carry no deadline, so a navigation that never finishes holds them past spinnerTimeout; and the 3 s grace period in waitForReadyWhileSpinning starts once and does not restart on a later navigation.

Risk map

• Changed: the decision after a failed quick check, only when a main-frame navigation commits during it. Everything else in spinnerWaiter is untouched.
• Slower: fast-fails after a navigation, by one second per observed navigation, within the same spinnerTimeout budget. Same-document navigations (pushState, hash changes) count too.
• Listener: one framenavigated listener per page for the life of the page; the counter lives in a WeakMap keyed by the Playwright Page.
• Not covered: a spinner drawn more than a second after its document commits still fast-fails, as it does for in-page actions today. The two pre-existing limits Codex flagged (undeadlined probes, the one-shot grace period in the spinner wait) are unchanged.
• Verified: pnpm test, pnpm typecheck, pnpm lint, pnpm build; the new spec 20/20 before and after.

Pull request video

Rendered spec/todo-app.spec.ts (ffmpeg-full 9.0.2 with libass; Homebrew's core ffmpeg no longer ships the ass filter, so plain pnpm test cannot render locally on that build). This PR does not change recording or rendering.

mw-todo-app-video-rendered.webm

🤖 Generated with Claude Code

[cursor]

PR Summary

Medium Risk
Changes timing and failure semantics for locator actions during redirects and repeated navigations; common no-navigation fast-fail path only adds WeakMap reads.

Overview
Fixes spinnerWaiter falsely treating mid-redirect pages as idle: when the main frame navigates during the 1s quick check or loading probe, the plugin re-runs the quick check on the new document instead of 1ms fast-failing with the “add a spinner” hint.

Implementation tracks mainFrameNavigations via framenavigated and loops until loading is detected or the navigation count is stable. One deadline (spinnerTimeout minus the action’s last second) now covers repeated quick checks and the spinner wait; hitting it while navigation is still ongoing fails with the “navigation was still in flight” message rather than the no-spinner nudge.

Adds a real HTTP Connect/OAuth redirect spec (spinner appears ~850ms after landing commits) and a reload-loop spec; README documents the behavior.

^{Reviewed by Cursor Bugbot for commit 8deb002. Bugbot is set up for automated code reviews on this repo. Configure here.}

[pkg-pr-new]

Open in StackBlitz

pnpm add https://pkg.pr.new/middlewright@46

commit: 8deb002