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-canary/canary.spec.ts b/tools/simulation/ui/specs-canary/canary.spec.ts index 37b1584b..4fe364e7 100644 --- a/tools/simulation/ui/specs-canary/canary.spec.ts +++ b/tools/simulation/ui/specs-canary/canary.spec.ts @@ -48,10 +48,16 @@ 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"), + // 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 5e7a3f76..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.locator(".capture-tile").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 ccc93007..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.locator(".capture-tile").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 @@ -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..b0029913 --- /dev/null +++ b/web/src/styles.harness-selectors.test.ts @@ -0,0 +1,76 @@ +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. +// +// 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"); + +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; +} + +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. 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.startsWith(join(WEB, "test"))) continue; + const text = readFileSync(file, "utf8"); + 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 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|querySelectorAll|querySelector|\$\$|\$)\((["'`])([^"'`]+)\1/g)) { + for (const cls of call[2].matchAll(/\.([A-Za-z_][\w-]*)/g)) { + const name = cls[1]; + 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))); + } + } + } + const report = [...stale].map(([name, files]) => `.${name} in ${[...files].join(", ")}`).join("\n"); + expect(report, `harness selectors the app no longer renders:\n${report}`).toBe(""); + }); +});