spinner-waiter: an in-flight navigation counts as loading

README.md

@@ -110,6 +110,8 @@ Fair enough — *inside* Playwright's watchdog architecture it may not be. But i
 
 The flagship. Before each action, if the target element isn't visible but a spinner is, waits (up to `spinnerTimeout`) for the element — bailing out early if the spinner disappears without producing it. If there's no spinner and the action fails, the error message suggests adding one:
 
+A document that hasn't finished loading counts as a spinner too: right after a cross-server hop (an OAuth popup, say) the page may still be fetching and rendering before its `load` event — no app spinner exists yet, but the browser's tab spinner is on — so actions wait through it the same way. That covers auto-wrapped OAuth popups without explicit timeouts.
+
 ```
 Timeout 1000ms exceeded.
   If this is a slow operation, update the product code to add a spinner while it's running.

spec/spinner-waiter.spec.ts

@@ -256,3 +256,70 @@ test("disappearance waits pass through untouched", async ({ page }) => {
   expect(String(error)).toContain("Timeout 1000ms exceeded");
   expect(Date.now() - start).toBeGreaterThan(500);
 });
+
+test("waits while a freshly navigated document is still loading, with no spinner", async ({
+  page,
+}) => {
+  // A cross-server hop lands on a page that renders its UI on `load`, and a
+  // slow subresource keeps `load` 2.5s away. The app draws no spinner — the
+  // browser's own tab spinner is the only loading UI. Playwright itself waits
+  // for the navigation to commit; spinner-waiter must keep waiting until the
+  // document finishes loading instead of fast-failing at commit.
+  await page.route("https://app.middlewright.test/**", async (route) => {
+    await route.fulfill({ body: `<h1>middlewright dashboard</h1>`, contentType: "text/html" });
+  });
+  await page.route("https://auth.middlewright.test/slow.png", async (route) => {
+    await new Promise((resolve) => setTimeout(resolve, 2500));
+    await route.fulfill({ body: Buffer.alloc(0), contentType: "image/png" });
+  });
+  await page.route("https://auth.middlewright.test/consent", async (route) => {
+    await route.fulfill({
+      body: `
+        <img src="https://auth.middlewright.test/slow.png" alt="" />
+        <script>
+          window.addEventListener("load", () => {
+            document.body.insertAdjacentHTML("beforeend", '<button id="allow">Allow access</button>');
+          });
+        </script>
+      `,
+      contentType: "text/html",
+    });
+  });
+  await page.goto("https://app.middlewright.test/");
+  // Kick the hop off without a Playwright action waiting on it, the way a
+  // popup arrives already navigating.
+  await page.evaluate(() => {
+    location.assign("https://auth.middlewright.test/consent");
+  });
+
+  await page.getByRole("button", { name: "Allow access" }).click();
+});
+
+test("fails fast once a navigation has fully loaded without the expected element", async ({
+  page,
+}) => {
+  await page.route("https://app.middlewright.test/**", async (route) => {
+    await route.fulfill({ body: `<h1>middlewright dashboard</h1>`, contentType: "text/html" });
+  });
+  await page.route("https://auth.middlewright.test/**", async (route) => {
+    await new Promise((resolve) => setTimeout(resolve, 2000));
+    await route.fulfill({ body: `<h1>Something went wrong</h1>`, contentType: "text/html" });
+  });
+  await page.goto("https://app.middlewright.test/");
+  await page.evaluate(() => {
+    location.assign("https://auth.middlewright.test/consent");
+  });
+
+  const start = Date.now();
+  const error = await page
+    .getByRole("button", { name: "Allow access" })
+    .click()
+    .catch((e: Error) => e);
+
+  // The navigation is over and the document is complete: nothing is loading,
+  // so this is the ordinary no-spinner fast-fail — not a long wait.
+  expect(error).toBeInstanceOf(Error);
+  expect(String(error)).toMatch(/Timeout 1ms exceeded/);
+  expect(String(error)).toMatch(/add a spinner/i);
+  expect(Date.now() - start).toBeLessThan(6_000);
+});

src/plugins/spinner-waiter.ts

@@ -7,7 +7,7 @@
  * a different effective action timeout while the app is visibly loading.
  */
 import { AsyncLocalStorage } from "node:async_hooks";
-import type { Locator } from "@playwright/test";
+import type { Locator, Page } from "@playwright/test";
 import type { ActionContext, LocatorWithOriginal, Plugin } from "../plugin-system.ts";
 import { adjustError, oneArgMethods } from "../plugin-system.ts";
 
@@ -127,14 +127,15 @@ export const spinnerWaiter = Object.assign(
           return next();
         }
 
-        // Check for spinner
+        // Check for loading UI: an app spinner, or a navigation in flight
+        // (loading UI the app cannot draw itself — see loadingVisible).
         const spinnerSelector = settings.spinnerSelectors.join(",");
         const spinnerLocator = page.locator(spinnerSelector) as LocatorWithOriginal;
-        const spinnerVisible = await anySpinnerVisible(spinnerLocator);
+        const loading = await loadingVisible(page, spinnerLocator);
 
-        if (!spinnerVisible) {
-          // No spinner - call action, suggest adding one if it fails
-          settings.log(`${locator} not ready, no spinner, failing fast`);
+        if (!loading) {
+          // No spinner, no navigation - call action, suggest a spinner if it fails
+          settings.log(`${locator} not ready, nothing loading, failing fast`);
           try {
             return await next(withTimeoutOption(method, args, 1));
           } catch (error) {
@@ -144,12 +145,12 @@ export const spinnerWaiter = Object.assign(
         }
 
         settings.log(
-          `Spinner visible, waiting up to ${settings.spinnerTimeout - 2000}ms for ${locator}`,
+          `Loading (spinner or navigation), waiting up to ${settings.spinnerTimeout - 2000}ms for ${locator}`,
         );
 
-        // Spinner is visible — wait for the element, but bail early if the spinner
-        // disappears (the loading operation finished without producing the expected element).
-        const waitResult = await waitForReadyWhileSpinning(locator, method, spinnerLocator, {
+        // Something is loading — wait for the element, but bail early once loading
+        // finishes (the operation completed without producing the expected element).
+        const waitResult = await waitForReadyWhileSpinning(locator, method, page, spinnerLocator, {
           timeout: settings.spinnerTimeout - 2000,
         });
 
@@ -160,10 +161,10 @@ export const spinnerWaiter = Object.assign(
 
         if (waitResult === "spinner-gone") {
           settings.log(
-            `Spinner disappeared but element not ready — loading finished without expected result`,
+            `Loading finished but element not ready — the operation completed without the expected result`,
           );
         } else {
-          settings.log(`Spinner still visible after ${settings.spinnerTimeout}ms, UI likely stuck`);
+          settings.log(`Still loading after ${settings.spinnerTimeout}ms, UI likely stuck`);
         }
 
         // Call action anyway (will likely fail), adjust error message
@@ -172,8 +173,8 @@ export const spinnerWaiter = Object.assign(
         } catch (error) {
           const message =
             waitResult === "spinner-gone"
-              ? `Loading finished (spinner disappeared after ${Date.now() - start}ms) but the expected element was not ready.`
-              : `Spinner was still visible after ${settings.spinnerTimeout}ms, the UI is likely stuck.`;
+              ? `Loading finished (spinner disappeared / navigation completed after ${Date.now() - start}ms) but the expected element was not ready.`
+              : `Spinner was still visible after ${settings.spinnerTimeout}ms (or a navigation was still in flight), the UI is likely stuck.`;
           adjustError(error as Error, [message], "spinner-waiter.ts");
           throw error;
         }
@@ -242,28 +243,60 @@ function isOptionsObject(value: unknown): value is Record<string, unknown> {
 }
 
 /**
- * Wait for `target` to become ready, but bail early if `spinner` disappears.
- * Returns "appeared" if target became ready, "spinner-gone" if loading finished
- * without the target, or "timeout" if spinner was still visible at deadline.
+ * Wait for `target` to become ready, but bail early once loading finishes
+ * (spinner gone and no navigation in flight). Returns "appeared" if target
+ * became ready, "spinner-gone" if loading finished without the target, or
+ * "timeout" if still loading at the deadline.
  */
 async function waitForReadyWhileSpinning(
   target: Locator,
   method: ActionContext["method"],
+  page: Page,
   spinner: Locator,
   { timeout = 1000 } = {},
 ): Promise<"appeared" | "spinner-gone" | "timeout"> {
   const start = Date.now();
-  // Give the spinner a grace period before checking — it may flicker during transitions
+  // Give loading a grace period before checking — spinners flicker during
+  // transitions, and one navigation can hand off to another.
   const spinnerGracePeriodMs = 3000;
   while (Date.now() - start < timeout) {
     if (await locatorIsReady(target, method)) return "appeared";
     const elapsed = Date.now() - start;
-    if (elapsed > spinnerGracePeriodMs && !(await anySpinnerVisible(spinner))) return "spinner-gone";
+    if (elapsed > spinnerGracePeriodMs && !(await loadingVisible(page, spinner))) return "spinner-gone";
     await new Promise((resolve) => setTimeout(resolve, 250));
   }
   return "timeout";
 }
 
+/** Loading UI: a document the browser is still loading, or an app spinner. */
+async function loadingVisible(page: Page, spinnerLocator: Locator): Promise<boolean> {
+  return (await pageIsNavigating(page)) || (await anySpinnerVisible(spinnerLocator));
+}
+
+/**
+ * A document still loading is loading UI the app cannot draw itself. The
+ * reference is the browser's own tab spinner: it stays on until the document
+ * fires `load` (readyState !== "complete"), and no execution context to ask
+ * (the gap while a navigation commits) counts too. Playwright's locator
+ * queries already wait for a pending navigation to commit, so this covers the
+ * window after that: a cold page rendering its UI client-side before `load`.
+ * The spinner grace period and timeout bound it, as for an app spinner.
+ */
+async function pageIsNavigating(page: Page): Promise<boolean> {
+  if (page.isClosed()) return false;
+  // page.evaluate waits for an execution context; while a navigation is
+  // committing there is none, so bound the wait and count a stall as loading.
+  const readyState = await Promise.race([
+    page
+      .evaluate(() => document.readyState)
+      .catch((error) =>
+        /execution context was destroyed|navigat/i.test(String(error)) ? "loading" : "complete",
+      ),
+    new Promise<"no-context">((resolve) => setTimeout(() => resolve("no-context"), 250)),
+  ]);
+  return readyState !== "complete";
+}
+
 /**
  * Multi-element-safe "is any spinner visible": the spinner selector union can
  * legitimately match several loading indicators at once (e.g. two panels each

tasks/complete/2026-08-18-spinner-waiter-navigation-loading.md

@@ -0,0 +1,66 @@
+---
+status: done
+size: small
+branch: spinner-waiter-navigation
+---
+
+# spinner-waiter: an in-flight navigation counts as loading
+
+**Status summary**: done. spinner-waiter now treats a document that hasn't fired `load` (or has no execution context yet) as loading UI. Two specs; README updated. One limitation recorded below (an action that *initiates* a slow navigation).
+
+## Why
+
+`../iterate`'s mobile specs still carry `{ timeout: 15_000 }` on popup actions
+even though popups auto-wrap now. The comments say why: the popup is
+*mid-navigation* to the auth worker ("the popup event fires before its
+cross-server auth navigation mounts the login choices"; "clicks land after
+auth-worker navigations that run cold on fresh preview deploys — CI-proven
+>1s"). During a navigation the app cannot render a spinner — there is no
+document yet — so spinner-waiter sees no loading UI and fast-fails in 1ms.
+But the browser itself is visibly loading. That is loading UI by any honest
+reading of the plugin's rule ("if the app is visibly loading, wait longer").
+
+## Decision
+
+Treat an in-flight navigation exactly like a visible spinner:
+
+- the current document hasn't fired `load` yet (`readyState !== "complete"` —
+  the browser's own tab spinner is the reference; the grace period and
+  spinner timeout bound it as they do an app spinner), or
+- no execution context to ask — the gap while a navigation commits.
+
+Not needed (found out by instrumenting): tracking pending main-frame requests.
+Playwright's locator queries (`isVisible`, `count`) already block until a
+pending navigation commits, so spinner-waiter never observes that phase; the
+reachable window is *after* commit, before `load` — a cold page rendering its
+UI client-side. A first cut carried a request tracker; it was dead code and is
+gone.
+
+A closed page is not loading. The same predicate feeds both the initial
+"is anything loading?" check and the "loading finished without the target"
+bail-out, so error hints stay accurate ("Loading finished (spinner gone /
+navigation done)").
+
+Out of scope:
+
+- `page.waitForEvent("popup", { timeout })` — an event wait, not a locator
+  action; middlewright never sees it and the fix there is product loading UI
+  on the button that triggers the auth round-trips.
+- An action that *initiates* a slow navigation (clicking a link to a cold
+  server): Playwright's click waits for the navigation it started, inside the
+  action's own timeout, after spinner-waiter has already let it through. Found
+  while writing the spec (the anchor click itself hit the 1s budget). Worth its
+  own task if it bites for real; the popup case doesn't trigger it because the
+  popup arrives already navigating.
+
+## Checklist
+
+- [x] `pageIsNavigating(page)` predicate; fold into the loading checks _(`loadingVisible` = navigating || spinner, checked navigation-first; used for both the initial check and the loading-finished bail-out)_
+- [x] spec: a click whose target only exists after a slow cross-page navigation (no app spinner) succeeds without an explicit timeout _("waits while a freshly navigated document is still loading" — proven load-bearing by disabling the check: fast-fails at 1ms without it)_
+- [x] spec: navigation completes but the target never appears → fast fail after the navigation _(the ordinary no-spinner fast-fail once the document is complete — Playwright had already waited out the commit, so the loading branch isn't what's exercised there)_
+- [x] README: mention navigation counts as loading under spinnerWaiter
+
+## Implementation log
+
+- 2026-08-18: found via `../iterate` specs (`specs/mobile/{notifications,approvals}.spec.ts`).
+- 2026-08-18: implemented; instrumented to learn Playwright already blocks locator queries until commit, dropped the request tracker accordingly. 154 tests green.