Skip to content

Commit 39625b5

Browse files
PoolchaosPoolchaos
committed
Walk the whole app in a browser, in both themes, and find that one was missing
Work item V8 of the M3 plan: a browser journey from an empty app to an exported report, screenshots of every screen in both themes, and CI running both. The journey adds a repository by pasting a file URL, waits out the clone, imports the protocol, picks both branches, reads the pre-flight, starts the review, dismisses one finding with a reason, confirms the rest from the keyboard, completes, and exports. It runs against a production build with the fake engine, so it spends nothing, and its answers are the ones the engine quality gate already uses. Written as one test with named steps, because Playwright gives every test a fresh page and a journey split into tests is a journey that starts over each time. Photographing both themes immediately earned itself: the file named light was dark. Tailwind 4's @theme does not honour being nested in a media query, so the dark block had been overwriting the light values unconditionally and the built CSS contained no prefers-color-scheme rule at all. The light theme had never rendered, for anyone, since the day it was written. The dark values are plain custom-property overrides now, and a test asserts the background follows the browser's preference rather than leaving it to a picture nobody opens. That assertion was wrong before it was right, which is worth recording: Chromium reports these colours as lab(), where the first channel is lightness on a 0 to 100 scale, and summing the channels read 98.8 as dark. Two smaller decisions the work forced. Screenshots capture the body element with animations frozen rather than the full page, because the rail polls for a running review so the page is never idle and full-page capture failed intermittently mid-repaint; freezing animations also makes two photographs of one screen identical, which is what evidence has to be. And the web server is never reused: reusing one would run the journey against a server started without the fake engine, which is exactly the mistake that spent real usage earlier today. CI now runs ./verify.sh --build --e2e, which is what D-20 always said it should. Verified locally, three consecutive clean runs of the browser suite and a full gate reporting for the first time that it passed including build and e2e.
1 parent d1e898e commit 39625b5

8 files changed

Lines changed: 397 additions & 6 deletions

File tree

‎.github/workflows/verify.yml‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,12 @@ jobs:
1818

1919
- run: npm ci
2020

21-
# The same gate contributors run locally, including the production
22-
# build. Nothing is checked here that ./verify.sh does not check.
23-
- run: ./verify.sh --build
21+
# The browser the journey runs in. Only chromium: the app is a local
22+
# tool, and a second engine would double the run for no question it
23+
# answers.
24+
- run: npx playwright install --with-deps chromium
25+
26+
# The same gate contributors run locally, plus the production build and
27+
# the browser journey. Nothing is checked here that ./verify.sh does not
28+
# check; the local default stays fast by leaving these two off.
29+
- run: ./verify.sh --build --e2e

‎docs/DECISIONS.md‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -787,3 +787,28 @@ verified evidence, in writing, here.
787787
timestamp, which is the real sequence, and a test reproducing the old
788788
ordering fails it about two runs in three. Recorded because the same trap
789789
applies anywhere else ULIDs are used as a tiebreak within one timestamp.
790+
- 2026-07-31 FIXED (V8): the light theme never rendered. Tailwind 4's `@theme`
791+
does not honour being nested in a media query, so the dark block overwrote
792+
the light values unconditionally and the built CSS contained no
793+
prefers-color-scheme rule at all. The dark values are plain custom-property
794+
overrides on `:root` now, which is what the components' var() references
795+
actually read. Found by photographing both themes and noticing that the file
796+
named light was dark, which is the whole reason the design doc calls both
797+
themes first-class and says both are screenshotted.
798+
- 2026-07-31 DECIDED (V8): the browser journey is one test with named steps
799+
rather than several tests. Playwright gives every test a fresh page, so a
800+
journey split into tests is a journey that starts over each time. Its steps
801+
depend on each other because the flow does.
802+
- 2026-07-31 DECIDED (V8): the e2e web server is never reused
803+
(`reuseExistingServer: false`). Reusing one would run the journey against a
804+
server started without the fake engine, which is exactly the mistake that
805+
spent real usage on 2026-07-31. A port already in use fails loudly instead.
806+
- 2026-07-31 DECIDED (V8): screenshots capture the body element with
807+
animations frozen, not the full page. The rail polls for a running review, so
808+
the page is never idle and a full-page capture intermittently failed
809+
mid-repaint; freezing animations also makes two photographs of one screen
810+
identical, which is what evidence has to be.
811+
- 2026-07-31 NOTED (V8): a test asserting the light theme was itself wrong
812+
before it was right. Chromium reports these colours as `lab()`, where the
813+
first channel is lightness on a 0 to 100 scale; summing the channels read
814+
98.8 as "dark". The helper now normalises lab, oklch and rgb to one scale.

‎docs/plans/M3-FINISH-PLAN.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,7 @@ confirms the wiring.
350350
| V5 | report renderer, report/export routes, report UI | V2 | DONE |
351351
| V6 | resume/queued/merged/delete UI, settings editor, probe buttons | V2 | DONE |
352352
| V7 | rulesets detail, enable toggle, snapshot filter, export | V1 | DONE |
353-
| V8 | e2e journey, theme screenshots, design audit, CI --e2e | V2-V7 | |
353+
| V8 | e2e journey, theme screenshots, design audit, CI --e2e | V2-V7 | DONE |
354354
| V9 | PROJECT-STATE, indexes, FG-2 checklist and evidence, G1 row | V8 | |
355355

356356
## 3. Decisions fixed by this plan (append to DECISIONS.md as work lands)

‎e2e/journey.spec.ts‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
/**
2+
* One review, from an empty app to an exported report, through a browser.
3+
*
4+
* The unit and route tests prove each part in isolation; this proves the parts
5+
* are connected, which is the one thing they cannot. It runs against a
6+
* production build with the fake engine, so it spends nothing and its answers
7+
* are the same ones the engine quality gate uses.
8+
*
9+
* Written as one test with named steps rather than several tests, because it
10+
* is one flow: each step depends on what the last one left on the screen, and
11+
* Playwright gives every separate test a fresh page.
12+
*/
13+
14+
import { readFileSync } from "node:fs";
15+
import { expect, test } from "@playwright/test";
16+
import { APP_REPO, PROTOCOL_PATH } from "./setup";
17+
18+
test("a review, from an empty app to an exported report", async ({ page }) => {
19+
await test.step("an app with nothing in it says what to do first", async () => {
20+
await page.goto("/projects");
21+
await expect(page.getByRole("heading", { name: "Projects" })).toBeVisible();
22+
// An empty state that teaches the next step, rather than an apology.
23+
await expect(page.getByText("No projects yet")).toBeVisible();
24+
await expect(page.getByText(/pick two branches and the rules/i)).toBeVisible();
25+
});
26+
27+
await test.step("a repository is added and clones in the background", async () => {
28+
await page.getByPlaceholder("git@github.com:you/your-app.git").fill(`file://${APP_REPO}`);
29+
await page.getByRole("button", { name: "Add project" }).click();
30+
// The clone runs in the background and the row appears at once, so the
31+
// name is what to wait for rather than a spinner finishing.
32+
await expect(page.getByRole("link", { name: "app", exact: true })).toBeVisible({
33+
timeout: 60_000,
34+
});
35+
});
36+
37+
await test.step("a protocol document becomes a ruleset", async () => {
38+
await page.goto("/rulesets");
39+
await page.getByLabel("Name").fill("Example protocol");
40+
await page
41+
.getByPlaceholder("Paste the markdown protocol here")
42+
.fill(readFileSync(PROTOCOL_PATH, "utf8"));
43+
await page.getByRole("button", { name: "Import" }).click();
44+
await expect(page.getByText(/Imported \d+ rule\(s\)/)).toBeVisible({ timeout: 30_000 });
45+
});
46+
47+
await test.step("the project page lists its branches with how far they have moved", async () => {
48+
await page.goto("/projects");
49+
await page.getByRole("link", { name: "app", exact: true }).click();
50+
await expect(page.getByRole("heading", { name: "Branches" })).toBeVisible({ timeout: 60_000 });
51+
await expect(page.getByRole("heading", { name: "Dependencies" })).toBeVisible();
52+
});
53+
54+
await test.step("setting up a review shows what it will examine first", async () => {
55+
await page.getByRole("link", { name: "New review" }).click();
56+
await expect(page.getByRole("heading", { name: "New review" })).toBeVisible();
57+
58+
// Both branches are chosen deliberately rather than left to the defaults.
59+
// This fixture's HEAD is the feature branch, so its detected default is
60+
// the branch under review, and taking the default would review main into
61+
// the feature branch: backwards, and a review of nothing anyone asked for.
62+
// Clicked the way a person clicks it: the radio itself is visually
63+
// hidden and the whole row is its label.
64+
await page
65+
.getByRole("main")
66+
.locator("label")
67+
.filter({ hasText: "feature/rename-prefs" })
68+
.first()
69+
.click();
70+
await page.getByLabel("Compare against").selectOption("main");
71+
await page
72+
.getByLabel(/What was this change meant to do/)
73+
.fill("Rename the prefs field and migrate every consumer.");
74+
75+
// The pre-flight is free and read-only, so it appears on its own once the
76+
// four decisions are made.
77+
await expect(page.getByText("What this review will examine")).toBeVisible({ timeout: 60_000 });
78+
await expect(page.getByText("Files changed")).toBeVisible();
79+
await expect(page.getByText(/pinned again when the review starts/)).toBeVisible();
80+
});
81+
82+
await test.step("starting it walks the stages and then stops for a person", async () => {
83+
await page.getByRole("button", { name: "Start review" }).click();
84+
await expect(page).toHaveURL(/\/reviews\/[A-Z0-9]+$/i, { timeout: 60_000 });
85+
await expect(page.getByText("awaiting confirmation")).toBeVisible({ timeout: 120_000 });
86+
await expect(page.getByText(/\d+ of \d+ decided/)).toBeVisible();
87+
});
88+
89+
await test.step("one finding is dismissed with a reason", async () => {
90+
// Nothing is reported until a person says so, so completing is refused
91+
// while anything is still undecided.
92+
await expect(page.getByRole("button", { name: "Complete review" })).toBeDisabled();
93+
94+
await page
95+
.getByPlaceholder("Why is this not a problem?")
96+
.first()
97+
.fill("Deliberate: the caller already guards this.");
98+
await page.getByRole("button", { name: "Dismiss" }).first().click();
99+
await expect(page.getByText("Deliberate: the caller already guards this.")).toBeVisible();
100+
});
101+
102+
await test.step("the rest are confirmed from the keyboard", async () => {
103+
for (let guard = 0; guard < 30; guard += 1) {
104+
if ((await page.getByRole("button", { name: "Confirm" }).count()) === 0) break;
105+
await page.locator("body").press("c");
106+
await page.waitForTimeout(200);
107+
}
108+
await expect(page.getByRole("button", { name: "Complete review" })).toBeEnabled({
109+
timeout: 30_000,
110+
});
111+
});
112+
113+
await test.step("completing it produces a report that can be exported", async () => {
114+
await page.getByRole("button", { name: "Complete review" }).click();
115+
await expect(page.getByRole("heading", { name: "Report" })).toBeVisible({ timeout: 30_000 });
116+
117+
// The report says what was examined, not only what was found, and renders
118+
// findings in the structure the protocol defines.
119+
await expect(page.getByText("## What was examined")).toBeVisible();
120+
await expect(page.getByText(/^File: app\//m).first()).toBeVisible();
121+
122+
await page.getByRole("button", { name: "Export" }).click();
123+
await expect(page.getByText(/Written to .*exports/)).toBeVisible({ timeout: 30_000 });
124+
});
125+
});

‎e2e/setup.ts‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
/**
2+
* Everything the browser journey needs before a browser opens.
3+
*
4+
* Builds the seeded repositories, clones one the way a person would add it,
5+
* and writes the answers the fake CLI hands back, so the whole journey runs
6+
* with no model and no money. The answers are generated from a reference
7+
* checkout of the same pinned commits: verification quotes the file it read,
8+
* and the bytes at a commit are the same wherever that commit is checked out.
9+
*
10+
* Paths are fixed rather than random because the Playwright config has to name
11+
* them in the server's environment before this file runs.
12+
*/
13+
14+
import { execFileSync } from "node:child_process";
15+
import { mkdirSync, readFileSync, rmSync } from "node:fs";
16+
import { tmpdir } from "node:os";
17+
import { join } from "node:path";
18+
import { fileURLToPath } from "node:url";
19+
20+
export const E2E_ROOT = join(tmpdir(), "trysquare-e2e");
21+
export const DATA_DIR = join(E2E_ROOT, "data");
22+
export const ANSWERS_DIR = join(E2E_ROOT, "answers");
23+
export const COUNTER_FILE = join(E2E_ROOT, "calls.txt");
24+
export const FIXTURE_DIR = join(E2E_ROOT, "fixture");
25+
26+
/**
27+
* The address the journey pastes in.
28+
*
29+
* A bare clone named app.git, because the app takes a project's name from the
30+
* address and its worktree directory from that name. The answers below are
31+
* written for a project called "app", so the remote is named to match rather
32+
* than the answers being bent to fit a fixture directory name.
33+
*/
34+
export const APP_REPO = join(E2E_ROOT, "app.git");
35+
36+
const REPO_ROOT = fileURLToPath(new URL("..", import.meta.url));
37+
export const FAKE_CLI = join(REPO_ROOT, "tests", "fixtures", "fake-claude.mjs");
38+
export const PROTOCOL_PATH = join(REPO_ROOT, "tests", "fixtures", "example-protocol.md");
39+
40+
export default async function globalSetup(): Promise<void> {
41+
rmSync(E2E_ROOT, { recursive: true, force: true });
42+
mkdirSync(FIXTURE_DIR, { recursive: true });
43+
mkdirSync(DATA_DIR, { recursive: true });
44+
45+
const { buildSeededRepos } = (await import(
46+
join(REPO_ROOT, "tests", "fixtures", "build-seeded-repos.mjs")
47+
)) as { buildSeededRepos: (root: string) => { manifest: { defects: { kind: string }[] } } };
48+
const manifest = buildSeededRepos(FIXTURE_DIR).manifest;
49+
execFileSync("git", ["clone", "--bare", "--quiet", join(FIXTURE_DIR, "seeded-repo"), APP_REPO]);
50+
51+
// A reference clone and checkout, only so the answers can quote real lines.
52+
const referenceClone = join(E2E_ROOT, "reference.git");
53+
const referenceRoot = join(E2E_ROOT, "reference");
54+
execFileSync("git", ["clone", "--bare", "--quiet", APP_REPO, referenceClone]);
55+
execFileSync("git", [
56+
"-C",
57+
referenceClone,
58+
"worktree",
59+
"add",
60+
"--detach",
61+
"--quiet",
62+
join(referenceRoot, "app"),
63+
"feature/rename-prefs",
64+
]);
65+
66+
const { parseUnifiedDiff } = await import("@/lib/git/diff");
67+
const { importProtocol } = await import("@/lib/rulesets/import");
68+
const { diffText, mergeBase, resolveCommit } = await import("@/server/gitops/repo");
69+
const helpers = await import("../tests/helpers/ideal-answers");
70+
71+
const base = await mergeBase(referenceClone, "main", "feature/rename-prefs");
72+
const head = await resolveCommit(referenceClone, "feature/rename-prefs");
73+
const files = parseUnifiedDiff(await diffText(referenceClone, base, head));
74+
75+
helpers.writeAnswersDir(
76+
ANSWERS_DIR,
77+
helpers.answerSequence(
78+
helpers.buildIdealStageOutputs({
79+
files: files.map((file) => ({ repo: "primary" as const, slug: "app", file })),
80+
manifest: {
81+
...manifest,
82+
defects: manifest.defects.filter((defect) => defect.kind !== "cross-repo"),
83+
} as never,
84+
worktreeRoot: referenceRoot,
85+
rules: importProtocol(readFileSync(PROTOCOL_PATH, "utf8")).ruleset.rules,
86+
}),
87+
),
88+
);
89+
}

‎e2e/themes.spec.ts‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
/**
2+
* Every screen, in both themes, photographed and checked for the things a
3+
* screenshot cannot tell you.
4+
*
5+
* Runs after the journey, so the pages have real content in them: an empty
6+
* projects list photographs nothing worth looking at. The assertions are about
7+
* behaviour a picture would hide, and the pictures are for a person to judge.
8+
*/
9+
10+
import { mkdirSync } from "node:fs";
11+
import { join } from "node:path";
12+
import { fileURLToPath } from "node:url";
13+
import { expect, test } from "@playwright/test";
14+
15+
const EVIDENCE = join(
16+
fileURLToPath(new URL("..", import.meta.url)),
17+
"review",
18+
`${new Date().toISOString().slice(0, 10)}-e2e`,
19+
);
20+
21+
/**
22+
* Photographs the page by capturing the body rather than the whole page.
23+
*
24+
* A full-page capture failed intermittently here, and the reason is structural
25+
* rather than incidental: the rail polls for a running review every few
26+
* seconds, so the page is never idle and a capture can land mid-repaint. An
27+
* element capture does not take that path, and animations are frozen so two
28+
* photographs of the same screen are the same photograph.
29+
*/
30+
async function photograph(page: import("@playwright/test").Page, file: string): Promise<void> {
31+
await page.locator("body").screenshot({ path: file, animations: "disabled", caret: "hide" });
32+
}
33+
34+
const SCREENS = [
35+
{ path: "/projects", name: "projects", title: "Projects" },
36+
{ path: "/reviews", name: "reviews", title: "Reviews" },
37+
{ path: "/rulesets", name: "rulesets", title: "Rulesets" },
38+
{ path: "/settings", name: "settings", title: "Settings" },
39+
];
40+
41+
test.beforeAll(() => {
42+
mkdirSync(EVIDENCE, { recursive: true });
43+
});
44+
45+
for (const screen of SCREENS) {
46+
test(`${screen.name} renders and photographs`, async ({ page }, testInfo) => {
47+
await page.goto(screen.path);
48+
await expect(page.getByRole("heading", { name: screen.title, level: 1 })).toBeVisible();
49+
50+
// The rail marks where you are, for anyone not going by colour.
51+
const current = page.locator('nav a[aria-current="page"]');
52+
await expect(current).toHaveCount(1);
53+
54+
// Wide content scrolls inside its own container; the page never does.
55+
const overflow = await page.evaluate(
56+
() => document.documentElement.scrollWidth > document.documentElement.clientWidth,
57+
);
58+
expect(overflow, `${screen.path} scrolls horizontally`).toBe(false);
59+
60+
await photograph(page, join(EVIDENCE, `${testInfo.project.name}-${screen.name}.png`));
61+
});
62+
}
63+
64+
test("a completed review photographs with its report", async ({ page }, testInfo) => {
65+
await page.goto("/reviews");
66+
// The only review the journey left, whatever the project ended up called.
67+
await page.getByRole("main").getByRole("link").first().click();
68+
69+
await expect(page.getByRole("heading", { name: "Report" })).toBeVisible({ timeout: 30_000 });
70+
await photograph(page, join(EVIDENCE, `${testInfo.project.name}-review.png`));
71+
});
72+
73+
test("the theme actually follows the browser's preference", async ({ page }, testInfo) => {
74+
// Asserted rather than left to the screenshots, because a picture nobody
75+
// opens proves nothing. This caught a real bug: Tailwind's @theme does not
76+
// honour being nested in a media query, so the dark values were overwriting
77+
// the light ones unconditionally and the app only ever rendered dark.
78+
await page.goto("/projects");
79+
const background = await page
80+
.locator("body")
81+
.evaluate((element) => getComputedStyle(element).backgroundColor);
82+
83+
// Chromium reports these as lab(), where the first channel is lightness on
84+
// a 0 to 100 scale, and an rgb() answer would need averaging instead. The
85+
// first version of this test summed the lab channels and read 99.8 as
86+
// "dark", which was the test being wrong rather than the page.
87+
const lightness = lightnessOf(background);
88+
if (testInfo.project.name === "dark") expect(lightness).toBeLessThan(30);
89+
else expect(lightness).toBeGreaterThan(80);
90+
});
91+
92+
/** Lightness on a 0 to 100 scale, whichever colour syntax the browser used. */
93+
function lightnessOf(colour: string): number {
94+
const numbers = colour.match(/-?[\d.]+/g)?.map(Number) ?? [];
95+
if (colour.startsWith("lab(")) return numbers[0] ?? 0;
96+
if (colour.startsWith("oklch(")) return (numbers[0] ?? 0) * 100;
97+
const [red = 0, green = 0, blue = 0] = numbers;
98+
return ((red + green + blue) / 3 / 255) * 100;
99+
}
100+
101+
test("the first thing tabbed to is reachable and visibly focused", async ({ page }) => {
102+
// Focus has to be visible, not merely present: a keyboard user who cannot
103+
// see where they are is not being served by an outline that was removed.
104+
await page.goto("/projects");
105+
await page.keyboard.press("Tab");
106+
107+
const focused = page.locator(":focus");
108+
await expect(focused).toBeVisible();
109+
const outline = await focused.evaluate((element) => getComputedStyle(element).outlineStyle);
110+
expect(outline).not.toBe("none");
111+
});

0 commit comments

Comments
 (0)