diff --git a/README.md b/README.md index d1a42f4..98b8860 100644 --- a/README.md +++ b/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. diff --git a/spec/spinner-waiter.spec.ts b/spec/spinner-waiter.spec.ts index b55aba3..16f87f8 100644 --- a/spec/spinner-waiter.spec.ts +++ b/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: `

middlewright dashboard

`, 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: ` + + + `, + 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: `

middlewright dashboard

`, contentType: "text/html" }); + }); + await page.route("https://auth.middlewright.test/**", async (route) => { + await new Promise((resolve) => setTimeout(resolve, 2000)); + await route.fulfill({ body: `

Something went wrong

`, 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); +}); diff --git a/src/plugins/spinner-waiter.ts b/src/plugins/spinner-waiter.ts index 7dc7d59..43a5558 100644 --- a/src/plugins/spinner-waiter.ts +++ b/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 { } /** - * 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 { + 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 { + 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 diff --git a/tasks/complete/2026-08-18-spinner-waiter-navigation-loading.md b/tasks/complete/2026-08-18-spinner-waiter-navigation-loading.md new file mode 100644 index 0000000..4eb0b68 --- /dev/null +++ b/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.