From a5bcc4c3e1cb814ad9ee742b6ed3cdd8df236469 Mon Sep 17 00:00:00 2001 From: mforce Date: Wed, 16 Sep 2026 19:15:07 +0000 Subject: [PATCH 1/3] test(e2e): retarget the canary and capture specs from the retired Dashboard tiles to the ruled rows #883 replaced the Dashboard's .capture-grid and .capture-tile markup with one role="group" row per flock, but only the quick smoke suite runs on pull requests, so the dispatch-only canary and the two capture specs kept waiting for a class that no longer renders (workflow run 35136083182). They now key on the page heading and the group rows. --- tools/simulation/ui/specs-canary/canary.spec.ts | 9 +++++---- tools/simulation/ui/specs-screenshots/palettes.spec.ts | 2 +- .../simulation/ui/specs-screenshots/screenshots.spec.ts | 2 +- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/tools/simulation/ui/specs-canary/canary.spec.ts b/tools/simulation/ui/specs-canary/canary.spec.ts index 37b1584b..09198937 100644 --- a/tools/simulation/ui/specs-canary/canary.spec.ts +++ b/tools/simulation/ui/specs-canary/canary.spec.ts @@ -48,10 +48,11 @@ const SCREENS = [ { name: "dashboard", path: "/", - ready: (page: CanaryPage) => page.locator(".capture-grid"), - // The dashboard carries no table at all since #654 — its per-flock capture - // tiles are the rows, and they are what a lost `/api/v1/flocks` empties. - rows: (ready: CanaryLocator) => ready.locator(".capture-tile"), + ready: (page: CanaryPage) => page.getByRole("heading", { name: tEn("dashboard:title") }), + // The dashboard carries no table: since #829 its Today list is one + // `role="group"` per flock (named by the flock), and those rows are what a + // lost `/api/v1/flocks` empties. + rows: (ready: CanaryLocator) => ready.page().getByRole("group"), // All three data panels, not just the flock one. A tile renders for a flock // that filed nothing today, so the tiles alone cannot tell a working stock // or sales read from a failed one. diff --git a/tools/simulation/ui/specs-screenshots/palettes.spec.ts b/tools/simulation/ui/specs-screenshots/palettes.spec.ts index 5e7a3f76..0c209f37 100644 --- a/tools/simulation/ui/specs-screenshots/palettes.spec.ts +++ b/tools/simulation/ui/specs-screenshots/palettes.spec.ts @@ -82,7 +82,7 @@ test.describe("palette x theme visual matrix (#664)", () => { await signIn(castMember("Manager")); await page.goto("/"); - await expect(page.locator(".capture-tile").first()).toBeVisible(); + await expect(page.getByRole("group").first()).toBeVisible(); await setPalette(page, brand, theme); await capture(page, `dashboard-${brand}-${theme}.png`); diff --git a/tools/simulation/ui/specs-screenshots/screenshots.spec.ts b/tools/simulation/ui/specs-screenshots/screenshots.spec.ts index ccc93007..f71a3fcf 100644 --- a/tools/simulation/ui/specs-screenshots/screenshots.spec.ts +++ b/tools/simulation/ui/specs-screenshots/screenshots.spec.ts @@ -105,7 +105,7 @@ test.describe("README screenshots", () => { // Tiles: at least one rendered. Class locator — the tile's accessible // name interpolates a flock name this spec does not know. - await expect(page.locator(".capture-tile").first()).toBeVisible(); + await expect(page.getByRole("group").first()).toBeVisible(); // Trend: the day strip is there AND not flat. Fourteen slots are drawn // whatever the figures (#777), so counting slots would pass on a missing From 9b41bfdb302e377b8bc9b654968857e03cc0d227 Mon Sep 17 00:00:00 2001 From: mforce Date: Wed, 16 Sep 2026 19:26:48 +0000 Subject: [PATCH 2/3] test(web): guard the harness's class selectors against the app, and fix the README capture's stale one A harness locator whose class the app no longer renders fails only on the dispatch-only runs, weeks after the deletion (#883, run 35136083182). styles.harness-selectors.test.ts walks every locator() string under tools/simulation/ui and fails the unit suite when a class is neither a styles.css rule (comments stripped) nor a className token in the SPA source. Red first on main's .capture-grid/.capture-tile, and it found a second live one: the README capture's .dash-list, retired by #883 too, now the role-named sales list. AGENTS.md's call-site rule says to grep the whole repo before deleting a class. --- AGENTS.md | 2 +- .../ui/specs-screenshots/screenshots.spec.ts | 2 +- web/src/styles.harness-selectors.test.ts | 62 +++++++++++++++++++ 3 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 web/src/styles.harness-selectors.test.ts diff --git a/AGENTS.md b/AGENTS.md index ff7f482e..b4101148 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -174,7 +174,7 @@ A guard is a test whose job is to *fail* when someone later does the wrong thing - **Adding an entry to a registry? Find its guards by grepping the registry's READERS, never by recall.** A registry here is any list other code walks — `CliDispatcher.Commands`, `AuditActions`, an enum mirrored into `web/src/i18n/enums.ts`. Adding #534's two verbs, a recalled list produced `CliDispatcherTests` and `ProcessRoleRegistryTests` and missed `OneShotVerbMinimalConfigTests`, which walks `ProcessRoles.OneShotVerbs` and fails any verb with no minimal-config case; `grep -rn "CliDispatcher.Commands\|ProcessRoles.OneShotVerbs" tests/` returns all three in a second. This is the bullet above turned on the guards themselves: a remembered list of guards is exactly the hand-maintained list they exist to stop anyone trusting. - **A guard that inspects call-site SYNTAX has to be read before you author the call site.** Its rule is not inferable from the code it guards, and violating it is a build failure rather than a review comment. `AuditVocabularyCoverageTests` accepts only `AuditActions.X`, or a ternary of two such references, as the action argument of an `IAuditWriter.WriteAsync` call — it fails closed on everything else and carries exactly one bespoke exemption (`IdentityProvider`'s forwarded parameter) with three companion assertions holding that exemption honest. So the obvious refactor, forwarding the action through a shared private helper's parameter, goes red on a test the author never opened; #534 caught that in a pre-dispatch review and switched to the ternary. - **A guard that walks every TRACKED file starts applying to a document the moment you commit it (#508).** `SchemaDocsTests.PostgresImagePin_IsOneIdenticalStringAcrossEveryTrackedFile` went red on a plan document, because its prose named a bare `postgres:` while describing a probe. The document was untracked while it was written and tracked one commit later, so the guard's scope changed under an artifact nobody thought of as code — and it cost a full implementer stop one increment from the finish line. Two rules follow. Before committing a document, check it against the guards that walk tracked files, not just against the prose you meant to write. And when a guard fires on the CONTENT of a copied document, that is a defect in the source document: fix it there and re-copy — never edit the committed copy (it breaks whatever "verbatim" meant) and never allow-list the file (it relaxes a pin guard to spare a comment). -- **Count a selector's call sites before styling it (#662).** Three selectors named across #651/#652 had moved on before the work started: `.stat`/`.stat-label` no longer existed, `.eyebrow` had zero call sites, and `.toolbar` had zero call sites too but was restyled anyway — so half of #651 changed nothing on screen, caught only when the owner compared before/after screenshots and said they looked the same. Before styling a selector an issue names, run `grep -rn "" web/src --include='*.tsx'` and record the count in the design; zero is a legitimate answer — deliberate groundwork counts — but it has to be a stated decision, not a discovery after merge. +- **Count a selector's call sites before styling it (#662).** Three selectors named across #651/#652 had moved on before the work started: `.stat`/`.stat-label` no longer existed, `.eyebrow` had zero call sites, and `.toolbar` had zero call sites too but was restyled anyway — so half of #651 changed nothing on screen, caught only when the owner compared before/after screenshots and said they looked the same. Before styling a selector an issue names, run `grep -rn "" web/src --include='*.tsx'` and record the count in the design; before DELETING one, grep the whole repo (`git grep -n "" -- ':!web/src/styles.css'`), because the Playwright harness under `tools/simulation/ui` selects by class too and its canary and capture specs run only on dispatch, so a stale selector there surfaces weeks later on a release branch (#883, run 35136083182); `web/src/styles.harness-selectors.test.ts` now fails the unit suite on such a selector; zero is a legitimate answer — deliberate groundwork counts — but it has to be a stated decision, not a discovery after merge. ## Pre-commit hook (opt-in) diff --git a/tools/simulation/ui/specs-screenshots/screenshots.spec.ts b/tools/simulation/ui/specs-screenshots/screenshots.spec.ts index f71a3fcf..13328ea1 100644 --- a/tools/simulation/ui/specs-screenshots/screenshots.spec.ts +++ b/tools/simulation/ui/specs-screenshots/screenshots.spec.ts @@ -126,7 +126,7 @@ test.describe("README screenshots", () => { // Sales: the Owner sees the panel and the demo fixture has one confirmed // order and one draft — scoped to the sales list, never the shell's own // list items. - await expect(page.locator(".dash-list li").first()).toBeVisible(); + await expect(page.getByRole("list", { name: tEn("dashboard:salesPanelTitle") }).getByRole("listitem").first()).toBeVisible(); // CAPTURED BEFORE THE INTERACTION BELOW, and that ordering is the whole // reason the #780 block moved down here. Focusing a day leaves the strip diff --git a/web/src/styles.harness-selectors.test.ts b/web/src/styles.harness-selectors.test.ts new file mode 100644 index 00000000..7fbf0f4d --- /dev/null +++ b/web/src/styles.harness-selectors.test.ts @@ -0,0 +1,62 @@ +import { readFileSync, readdirSync, statSync } from "node:fs"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; + +// Every class the Playwright harness selects by must still exist in the app, +// either as a rule in styles.css or as a className token in a non-test .tsx. +// The canary and the capture specs are dispatch-only, so a class retired by a +// screen conversion leaves them waiting for markup that no longer renders and +// nothing on a pull request notices: #883 retired `.capture-grid` and +// `.capture-tile` and the canary failed weeks of pull requests later, on the +// release branch's full run (workflow run 35136083182). This walks the harness +// the way styles.declared-tokens.test.ts walks the app, and it runs in the +// unit suite, so the deletion and the stale selector meet on the same PR. + +const WEB = __dirname; +const HARNESS = join(WEB, "..", "..", "tools", "simulation", "ui"); + +function walk(dir: string, out: string[] = []): string[] { + for (const entry of readdirSync(dir)) { + if (entry === "node_modules" || entry === "test-results" || entry.startsWith("out")) continue; + const p = join(dir, entry); + if (statSync(p).isDirectory()) walk(p, out); + else if (/\.tsx?$/.test(entry)) out.push(p); + } + return out; +} + +// Comments come out first: a rule family's obituary ("ul.dash-list is gone") +// would otherwise keep the class alive for the harness. +const css = readFileSync(join(WEB, "styles.css"), "utf8").replace(/\/\*[\s\S]*?\*\//g, ""); +const declared = new Set([...css.matchAll(/\.([A-Za-z_][\w-]*)/g)].map((m) => m[1])); + +const markupHooks = new Set(); +for (const file of walk(WEB)) { + if (/\.test\.tsx?$/.test(file) || file.includes(`${join(WEB, "test")}`)) continue; + const text = readFileSync(file, "utf8"); + for (const m of text.matchAll(/className=(?:"([^"]*)"|\{`([^`]*)`\}|\{"([^"]*)"\})/g)) { + for (const token of (m[1] ?? m[2] ?? m[3] ?? "").split(/\s+/)) { + const bare = token.replace(/\$\{[^}]*\}/g, "").trim(); + if (bare) markupHooks.add(bare); + } + } +} + +describe("the Playwright harness selects only classes the app still renders", () => { + it("every .class inside a locator() string is a stylesheet rule or a markup hook", () => { + expect(declared.size).toBeGreaterThan(100); + const stale = new Map>(); + for (const file of walk(HARNESS)) { + const text = readFileSync(file, "utf8"); + for (const call of text.matchAll(/locator\((["'`])([^"'`]+)\1/g)) { + for (const cls of call[2].matchAll(/\.([A-Za-z_][\w-]*)/g)) { + const name = cls[1]; + if (declared.has(name) || markupHooks.has(name)) continue; + stale.set(name, (stale.get(name) ?? new Set()).add(file.slice(HARNESS.length + 1))); + } + } + } + const report = [...stale].map(([name, files]) => `.${name} in ${[...files].join(", ")}`).join("\n"); + expect(report, `harness selectors the app no longer renders:\n${report}`).toBe(""); + }); +}); From 74c00d0a5f138c4224137258dbd51a7327dd49cd Mon Sep 17 00:00:00 2001 From: mforce Date: Wed, 16 Sep 2026 19:31:55 +0000 Subject: [PATCH 3/3] test(e2e): scope the flock rows to the Today section and widen the harness-selector guard Codex review of #890: an unscoped getByRole("group") also matched the DayStrip, so the canary and the captures could pass with no flock rows; the guard read only locator() literals and only plain className strings, and its comment stripper could not see quotes. The rows are now scoped to the section headed by the Today title; the guard also reads querySelector, querySelectorAll, $ and $$ literals, every quoted string inside a className expression, skips MUI's own classes, and pins that styles.css carries no quoted comment opener. --- .../simulation/ui/specs-canary/canary.spec.ts | 15 +++++--- .../ui/specs-screenshots/palettes.spec.ts | 2 +- .../ui/specs-screenshots/screenshots.spec.ts | 2 +- web/src/styles.harness-selectors.test.ts | 34 +++++++++++++------ 4 files changed, 36 insertions(+), 17 deletions(-) diff --git a/tools/simulation/ui/specs-canary/canary.spec.ts b/tools/simulation/ui/specs-canary/canary.spec.ts index 09198937..4fe364e7 100644 --- a/tools/simulation/ui/specs-canary/canary.spec.ts +++ b/tools/simulation/ui/specs-canary/canary.spec.ts @@ -48,11 +48,16 @@ const SCREENS = [ { name: "dashboard", path: "/", - ready: (page: CanaryPage) => page.getByRole("heading", { name: tEn("dashboard:title") }), - // The dashboard carries no table: since #829 its Today list is one - // `role="group"` per flock (named by the flock), and those rows are what a - // lost `/api/v1/flocks` empties. - rows: (ready: CanaryLocator) => ready.page().getByRole("group"), + // The Today section, found by its own heading: since #829 the dashboard + // carries no table, its Today list is one `role="group"` per flock (named + // by the flock), and those rows are what a lost `/api/v1/flocks` empties. + // Scoped to the section because the DayStrip is a `role="group"` too, so an + // unscoped query would count it as a row and pass with no flocks at all. + ready: (page: CanaryPage) => + page.locator("section").filter({ + has: page.getByRole("heading", { name: tEn("dashboard:todayPanelTitle") }), + }), + rows: (ready: CanaryLocator) => ready.getByRole("group"), // All three data panels, not just the flock one. A tile renders for a flock // that filed nothing today, so the tiles alone cannot tell a working stock // or sales read from a failed one. diff --git a/tools/simulation/ui/specs-screenshots/palettes.spec.ts b/tools/simulation/ui/specs-screenshots/palettes.spec.ts index 0c209f37..509367c9 100644 --- a/tools/simulation/ui/specs-screenshots/palettes.spec.ts +++ b/tools/simulation/ui/specs-screenshots/palettes.spec.ts @@ -82,7 +82,7 @@ test.describe("palette x theme visual matrix (#664)", () => { await signIn(castMember("Manager")); await page.goto("/"); - await expect(page.getByRole("group").first()).toBeVisible(); + await expect(page.locator("section").filter({ has: page.getByRole("heading", { name: tEn("dashboard:todayPanelTitle") }) }).getByRole("group").first()).toBeVisible(); await setPalette(page, brand, theme); await capture(page, `dashboard-${brand}-${theme}.png`); diff --git a/tools/simulation/ui/specs-screenshots/screenshots.spec.ts b/tools/simulation/ui/specs-screenshots/screenshots.spec.ts index 13328ea1..ae817dc5 100644 --- a/tools/simulation/ui/specs-screenshots/screenshots.spec.ts +++ b/tools/simulation/ui/specs-screenshots/screenshots.spec.ts @@ -105,7 +105,7 @@ test.describe("README screenshots", () => { // Tiles: at least one rendered. Class locator — the tile's accessible // name interpolates a flock name this spec does not know. - await expect(page.getByRole("group").first()).toBeVisible(); + await expect(page.locator("section").filter({ has: page.getByRole("heading", { name: tEn("dashboard:todayPanelTitle") }) }).getByRole("group").first()).toBeVisible(); // Trend: the day strip is there AND not flat. Fourteen slots are drawn // whatever the figures (#777), so counting slots would pass on a missing diff --git a/web/src/styles.harness-selectors.test.ts b/web/src/styles.harness-selectors.test.ts index 7fbf0f4d..b0029913 100644 --- a/web/src/styles.harness-selectors.test.ts +++ b/web/src/styles.harness-selectors.test.ts @@ -11,6 +11,11 @@ import { describe, expect, it } from "vitest"; // release branch's full run (workflow run 35136083182). This walks the harness // the way styles.declared-tokens.test.ts walks the app, and it runs in the // unit suite, so the deletion and the stale selector meet on the same PR. +// +// Scope, stated so nobody trusts it for more: it reads selector STRING +// LITERALS passed to locator(), querySelector(), querySelectorAll(), $() and +// $$(). A selector built from a variable or by concatenation is invisible to +// it. MUI's own `.Mui*` classes are framework-owned and skipped. const WEB = __dirname; const HARNESS = join(WEB, "..", "..", "tools", "simulation", "ui"); @@ -25,33 +30,42 @@ function walk(dir: string, out: string[] = []): string[] { return out; } +const rawCss = readFileSync(join(WEB, "styles.css"), "utf8"); // Comments come out first: a rule family's obituary ("ul.dash-list is gone") -// would otherwise keep the class alive for the harness. -const css = readFileSync(join(WEB, "styles.css"), "utf8").replace(/\/\*[\s\S]*?\*\//g, ""); +// would otherwise keep the class alive for the harness. The stripper is a +// plain regex, so a `/*` inside a quoted CSS string would swallow real rules; +// the stylesheet has none, and this pins that. +expect(rawCss, "styles.css carries a /* inside a quoted string; the comment stripper below cannot see quotes") + .not.toMatch(/["'][^"'\n]*\/\*/); +const css = rawCss.replace(/\/\*[\s\S]*?\*\//g, ""); const declared = new Set([...css.matchAll(/\.([A-Za-z_][\w-]*)/g)].map((m) => m[1])); +// Every quoted string inside a className attribute counts, whichever +// expression carries it: a plain string, a template literal, a ternary or a +// clsx() call. Tokens are split on whitespace and template holes dropped. const markupHooks = new Set(); for (const file of walk(WEB)) { - if (/\.test\.tsx?$/.test(file) || file.includes(`${join(WEB, "test")}`)) continue; + if (/\.test\.tsx?$/.test(file) || file.startsWith(join(WEB, "test"))) continue; const text = readFileSync(file, "utf8"); - for (const m of text.matchAll(/className=(?:"([^"]*)"|\{`([^`]*)`\}|\{"([^"]*)"\})/g)) { - for (const token of (m[1] ?? m[2] ?? m[3] ?? "").split(/\s+/)) { - const bare = token.replace(/\$\{[^}]*\}/g, "").trim(); - if (bare) markupHooks.add(bare); + for (const attr of text.matchAll(/className=(?:"([^"]*)"|\{([\s\S]*?)\}(?=\s|\/?>))/g)) { + const strings = attr[1] !== undefined ? [attr[1]] : [...attr[2].matchAll(/["'`]([^"'`]*)["'`]/g)].map((m) => m[1]); + for (const s of strings) { + for (const token of s.replace(/\$\{[^}]*\}/g, " ").split(/\s+/)) if (token) markupHooks.add(token); } } } describe("the Playwright harness selects only classes the app still renders", () => { - it("every .class inside a locator() string is a stylesheet rule or a markup hook", () => { + it("every .class inside a selector string literal is a stylesheet rule or a markup hook", () => { expect(declared.size).toBeGreaterThan(100); + expect(markupHooks.size).toBeGreaterThan(20); const stale = new Map>(); for (const file of walk(HARNESS)) { const text = readFileSync(file, "utf8"); - for (const call of text.matchAll(/locator\((["'`])([^"'`]+)\1/g)) { + for (const call of text.matchAll(/(?:locator|querySelectorAll|querySelector|\$\$|\$)\((["'`])([^"'`]+)\1/g)) { for (const cls of call[2].matchAll(/\.([A-Za-z_][\w-]*)/g)) { const name = cls[1]; - if (declared.has(name) || markupHooks.has(name)) continue; + if (name.startsWith("Mui") || declared.has(name) || markupHooks.has(name)) continue; stale.set(name, (stale.get(name) ?? new Set()).add(file.slice(HARNESS.length + 1))); } }