test(e2e): gate every measurement on its element, and keep it that way

Four specs navigated and then read the DOM once, with nothing in between
that waits for anything but the network. An idle network says nothing about
whether React has rendered, so the read can sample a tree that is still
mounting and the assertion then blames the application for a race inside the
test. Two such reads went red on consecutive CI runs, on different specs.

The settings over-scroll guard also slept a fixed 500 ms, which is the wrong
shape twice over: too long when the page is ready, too short on a loaded
runner, and it never says what it is waiting for.

The structural guard runs in the unit suite rather than in Playwright,
because e2e runs at PR-CI only and flaky counts as failed there — one
unguarded read otherwise costs a whole pipeline before anyone sees it. It
refuses to count a wait whose rejection is thrown away, since that reads
like a guarantee and is not one, and would leave the check defeatable by
appending a catch. The one deliberate swallow is on the allowlist with its
reason: a genuine regression there still fails at the width assertion below
it, which reports the measured number instead of a timeout message.

e2e/mobile-density.spec.ts

@@ -1,6 +1,7 @@
 import { expect, test } from "./setup/test";
 
 import { STORAGE_STATE_PATH } from "./setup/global-setup";
+import { settleBeforeMeasure } from "./utils/settle";
 import {
   LONG_HEADLINE_BRIEFING,
   MOCK_SCORE_RINGS,
@@ -162,7 +163,16 @@ test.describe("phone-width density guard", () => {
           await mockMoodInsights(page);
         }
         await page.goto(route, { waitUntil: "domcontentloaded" });
-        await page.waitForLoadState("networkidle");
+
+        if (route === "/insights") {
+          // The one route here that had no readiness signal, while its two
+          // siblings below both have one. An overflow guard against a page
+          // that has not finished rendering passes for the same reason an
+          // empty page passes: there is nothing wide on it yet.
+          await expect(
+            page.locator('[data-slot="section-heading"]').first(),
+          ).toBeVisible();
+        }
 
         if (route === "/") {
           // The guard must not pass vacuously — the Today hero and its
@@ -186,6 +196,8 @@ test.describe("phone-width density guard", () => {
           ).toBeVisible();
         }
 
+        await settleBeforeMeasure(page, page.locator("main#main-content"));
+
         // The document/body pair alone cannot see the app's sideways scroll:
         // `AuthShell` scrolls inside `<main id="main-content">`, whose
         // `overflow-y:auto` makes `overflow-x` compute to `auto`, so an

e2e/mobile-viewport.spec.ts

@@ -1,6 +1,7 @@
 import { expect, test } from "./setup/test";
 
 import { STORAGE_STATE_PATH } from "./setup/global-setup";
+import { settleBeforeMeasure } from "./utils/settle";
 import {
   mockDashboardSnapshot,
   WEIGHT_ONLY_SUMMARIES,
@@ -78,7 +79,10 @@ test.describe("mobile-viewport smoke", () => {
     );
 
     await page.goto("/", { waitUntil: "domcontentloaded" });
-    await page.waitForLoadState("networkidle");
+    // Gate on the dashboard's own content, not on the network. Every read
+    // below is geometric and happens exactly once, so measuring a tree that
+    // is still mounting reports a layout nobody will ever see.
+    await settleBeforeMeasure(page, page.locator("main#main-content"));
 
     // 1) No horizontal scroll
     const dims = await page.evaluate(() => ({

e2e/settings-overscroll.spec.ts

@@ -1,6 +1,7 @@
 import { expect, test } from "./setup/test";
 
 import { STORAGE_STATE_PATH } from "./setup/global-setup";
+import { settleBeforeMeasure } from "./utils/settle";
 
 /**
  * Vertical over-scroll regression guard (the recurring class behind the
@@ -63,9 +64,11 @@ test.describe("settings + admin vertical over-scroll guard", () => {
       test(`no over-scroll on ${route} (${vp.name})`, async ({ page }) => {
         await page.setViewportSize({ width: vp.width, height: vp.height });
         await page.goto(route, { waitUntil: "domcontentloaded" });
-        await page.waitForLoadState("networkidle");
-        // Let entry transitions / late queries settle before measuring.
-        await page.waitForTimeout(500);
+        // Was a fixed 500 ms sleep, which is the wrong shape twice over: too
+        // long when the page is ready and too short on a loaded runner, and
+        // it never says what it is waiting for. Gate on the element the
+        // measurement reads instead.
+        await settleBeforeMeasure(page, page.locator("#main-content"));
 
         const dims = await page.evaluate(() => {
           const main = document.getElementById("main-content");

e2e/utils/settle.ts

@@ -0,0 +1,39 @@
+import { expect, type Locator, type Page } from "@playwright/test";
+
+/**
+ * Gate a one-shot DOM read on the element it is about to measure.
+ *
+ * The failure this exists to prevent: a spec navigates with
+ * `waitUntil: "networkidle"`, then reads the DOM in a single
+ * `page.evaluate` / `evaluateAll` that does not retry. An idle network says
+ * nothing about whether React has rendered, so the read can land on a tree
+ * that is still mounting and the assertion then blames the application for
+ * something the test raced — "language + dob fields must exist" when the
+ * fields were simply not there yet, or a measurement helper accused of
+ * measuring the wrong element when it was measuring the right one too early.
+ *
+ * Two such tests went red on consecutive CI runs during v1.33.0. Neither
+ * reproduces locally: a developer machine is fast enough that the render
+ * always wins the race, so the only place this shows up is a loaded runner,
+ * and `failOnFlakyTests` means every occurrence costs a whole red pipeline
+ * even when the retry passes.
+ *
+ * `sentinel` must be something the measurement actually depends on, not just
+ * any element on the page. Gating on a layout shell that renders before the
+ * data proves nothing.
+ */
+export async function settleBeforeMeasure(
+  page: Page,
+  sentinel: Locator,
+  options: { attachedOnly?: boolean } = {},
+): Promise<void> {
+  if (options.attachedOnly) {
+    await expect(sentinel).toBeAttached();
+  } else {
+    await expect(sentinel).toBeVisible();
+  }
+  // Fonts and late CSS change geometry after the element is visible, and
+  // every measurement in this suite is geometric.
+  await page.waitForLoadState("networkidle").catch(() => {});
+  await page.evaluate(() => document.fonts?.ready);
+}

src/__tests__/e2e-measurement-gating-guard.test.ts

@@ -0,0 +1,175 @@
+import { readdirSync, readFileSync } from "node:fs";
+import { join, resolve } from "node:path";
+
+import { describe, expect, it } from "vitest";
+
+/**
+ * A one-shot DOM read must be gated on the element it measures, not on the
+ * network.
+ *
+ * The failure this catches, twice on consecutive CI runs during v1.33.0:
+ * a spec navigates with `waitUntil: "networkidle"` and then reads the DOM in
+ * a single `page.evaluate` / `evaluateAll` that never retries. An idle
+ * network says nothing about whether React has rendered, so the read can land
+ * on a tree that is still mounting. The assertion then blames the application
+ * — "language + dob fields must exist", "the helper is measuring the wrong
+ * element" — for what is a race inside the test.
+ *
+ * Why this guard lives in the unit suite rather than in Playwright: e2e runs
+ * at PR-CI only, and `playwright.config.ts` sets `failOnFlakyTests` on CI, so
+ * one unguarded read costs a whole red pipeline before anyone sees it. Here it
+ * is red in seconds, on the machine that wrote it.
+ *
+ * It is deliberately a source-shape check rather than a runtime one. The
+ * shape is what a developer gets wrong; the runtime symptom only appears on a
+ * loaded runner, which is precisely where it is most expensive to discover.
+ */
+
+const E2E_DIR = resolve(__dirname, "../../e2e");
+
+/**
+ * Sites where a `goto` → `evaluate` with no locator gate is correct, each with
+ * the reason. Keep this list short: it is an inventory of exceptions, not a
+ * place to put anything inconvenient. Past ~15 entries the guard is asking the
+ * wrong question and should be rethought rather than extended.
+ */
+const ALLOWLIST: ReadonlyArray<{ file: string; test: string; why: string }> = [
+  {
+    file: "ipad-viewport.spec.ts",
+    test: "no horizontal overflow and full-width content on",
+    why: "Deliberate swallowed waitForFunction. The rail settle can land just after networkidle on a loaded runner, but a genuine regression — the rail never engaging — still fails at the width assertion below, which reports the actual measured number. A hard gate here would replace that number with a generic timeout message, so the swallow buys a better diagnostic rather than hiding a race.",
+  },
+];
+
+interface Offence {
+  file: string;
+  testName: string;
+  line: number;
+}
+
+/** Source positions of every `test(` / `test.only(` block opener. */
+function testBlocks(
+  source: string,
+): Array<{ name: string; start: number; end: number }> {
+  const blocks: Array<{ name: string; start: number; end: number }> = [];
+  const opener = /\btest(?:\.only|\.fixme|\.skip)?\s*\(\s*(`|"|')([\s\S]*?)\1/g;
+  const starts: Array<{ name: string; index: number }> = [];
+  for (let m = opener.exec(source); m !== null; m = opener.exec(source)) {
+    starts.push({ name: m[2], index: m.index });
+  }
+  for (let i = 0; i < starts.length; i += 1) {
+    blocks.push({
+      name: starts[i].name,
+      start: starts[i].index,
+      end: i + 1 < starts.length ? starts[i + 1].index : source.length,
+    });
+  }
+  return blocks;
+}
+
+const GOTO = /\bpage\.goto\s*\(/g;
+/** A read that samples the DOM once and does not retry. */
+const ONE_SHOT_READ =
+  /\b(?:page|\w+)\.(?:evaluate|evaluateAll|evaluateHandle|\$\$eval|\$eval)\s*\(/;
+/**
+ * Anything that waits for the DOM to reach a state before the read.
+ *
+ * No leading `\b`: the common form is `page.locator(…).first().waitFor(…)`,
+ * where the character before the dot is `)`. There is no word boundary
+ * between `)` and `.`, so a `\b` here silently fails to match the most
+ * frequent gate in the suite and reports correct tests as offenders. That is
+ * worse than no guard, because the fix someone reaches for is to allowlist a
+ * test that was never wrong.
+ */
+const GATE =
+  /(?:await\s+expect(?:\.poll|\.soft)?\s*\(|\.waitFor\s*\(|waitForSelector\s*\(|waitForFunction\s*\(|settleBeforeMeasure\s*\(|settleForOverflowMeasurement\s*\(|toBeVisible\s*\(|toBeAttached\s*\()/;
+
+function lineOf(source: string, index: number): number {
+  return source.slice(0, index).split("\n").length;
+}
+
+/**
+ * A wait whose rejection is thrown away is not a gate.
+ *
+ * `await page.waitForFunction(...).catch(() => {})` reads like a guarantee and
+ * is not one: on timeout it proceeds to the measurement exactly as if it had
+ * never waited. Counting it would also leave the guard trivially defeatable —
+ * append `.catch(() => {})` to any wait and the check goes quiet. Where the
+ * swallow is deliberate, the allowlist is the place to say so.
+ */
+const SWALLOWED_WAIT =
+  /(?:waitForFunction|waitForSelector|waitFor)\s*\([\s\S]*?\)\s*\n?\s*\.catch\s*\(/;
+
+function stripSwallowedWaits(segment: string): string {
+  return segment.replace(new RegExp(SWALLOWED_WAIT, "g"), "«swallowed»");
+}
+
+function scan(file: string, source: string): Offence[] {
+  const offences: Offence[] = [];
+  for (const block of testBlocks(source)) {
+    const body = source.slice(block.start, block.end);
+    GOTO.lastIndex = 0;
+    for (let g = GOTO.exec(body); g !== null; g = GOTO.exec(body)) {
+      const after = body.slice(g.index);
+      const read = ONE_SHOT_READ.exec(after);
+      if (!read) continue;
+      const between = stripSwallowedWaits(after.slice(0, read.index));
+      if (GATE.test(between)) continue;
+      offences.push({
+        file,
+        testName: block.name,
+        line: lineOf(source, block.start + g.index),
+      });
+      break;
+    }
+  }
+  return offences;
+}
+
+describe("e2e specs gate their measurements on the element, not the network", () => {
+  const specs = readdirSync(E2E_DIR).filter((f) => f.endsWith(".spec.ts"));
+
+  it("finds spec files to check (the guard must not pass vacuously)", () => {
+    expect(specs.length).toBeGreaterThan(10);
+  });
+
+  it("has no ungated goto → one-shot DOM read outside the allowlist", () => {
+    const offences: Offence[] = [];
+    for (const spec of specs) {
+      const source = readFileSync(join(E2E_DIR, spec), "utf8");
+      offences.push(...scan(spec, source));
+    }
+
+    const unexplained = offences.filter(
+      (o) =>
+        !ALLOWLIST.some(
+          (a) => a.file === o.file && o.testName.includes(a.test),
+        ),
+    );
+
+    if (unexplained.length > 0) {
+      throw new Error(
+        `${unexplained.length} e2e test(s) navigate and then read the DOM once, with nothing in between that waits for the element:\n\n` +
+          unexplained
+            .map((o) => `  ${o.file}:${o.line} — "${o.testName}"`)
+            .join("\n") +
+          "\n\nAn idle network does not mean React has rendered. Gate on the element the " +
+          "measurement is about (await expect(locator).toBeVisible(), or settleBeforeMeasure " +
+          "from e2e/utils/settle.ts). If the read genuinely does not need a gate, add it to " +
+          "ALLOWLIST in this file with the reason.",
+      );
+    }
+  });
+
+  it("carries no allowlist entry for a spec file that no longer exists", () => {
+    for (const entry of ALLOWLIST) {
+      expect(specs, `stale allowlist entry: ${entry.file}`).toContain(
+        entry.file,
+      );
+    }
+  });
+
+  it("keeps the allowlist small enough to still be an inventory of exceptions", () => {
+    expect(ALLOWLIST.length).toBeLessThanOrEqual(15);
+  });
+});