test: commit reviewed visual-regression baselines - #2753
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Fresh-context review record (coordinator, 2026-09-06; read-only Opus 5 reviewer; route: the standing D-12 rule for Codex-session PRs). VERDICT: FIX-FIRST, parked pending one CI fact. HIGH-1 — the 22 baselines were captured with local Disposition: if the labelled run is green, the coordinator fixes HIGH-2, MEDIUM-3 and MEDIUM-4 in-PR and merges; if red, the PR stays open for baselines regenerated from the CI |
Chris0Jeky
added
the
testing
|
Parked (coordinator, 2026-09-06). The |
…egenerates them The 22 baselines on this branch were captured on Windows. The hosted lane runs Chromium on ubuntu-latest, where font rasterisation differs, so every image failed there; the run stopped at the config's CI maxFailures of 5 with 5 failed and 2 passed, which means the diff artifact only ever covered 5 of the 22. Removing the directory puts the reusable Visual Regression job back on its bootstrap path: with no baselines present it runs --update-snapshots and uploads the generated set as the visual-regression-baselines artifact. That Linux set is then reviewed and committed on top of this commit, so the images this branch finally lands are captured by the same platform that verifies them. This commit is a deliberate intermediate state: with no baselines the suite cannot detect a regression, so it must not be merged on its own. Refs #1363
Re-capturing the baselines on Linux (2026-09-06)Diagnosis of the parked state. The 22 baselines on this branch were captured on Windows; the hosted job runs Chromium on Remedy in flight. The PR is in draft while this is in flight and must not be merged at |
…ux-captured The first Linux bootstrap of this branch's baselines came back with transient success toasts burned into four of the 22 images: one on board-empty and on starter-pack-modal-import, three on card-modal-edit, and an eight-toast stack across board-populated's header and board-actions row. Those toasts are raised by the specs' own seeding — one per created board, column and card — and the toast store removes each after its own duration, so how many are still on screen when Playwright captures is a function of how fast the runner got there. Committing them would have made those four baselines flake on runner speed rather than on any visual change. hideDynamicContent already hid the session-timeout alert via [role="alert"][aria-live="assertive"], but ToastContainer and PaperToastContainer only set those attributes on *error* toasts, so success toasts were never covered. Both skins tag every toast with data-toast-id, so that is the selector used here. The stack is position: fixed and pointer-events: none, so hiding it shifts no page layout, and because the rule lives in the injected stylesheet rather than a one-shot DOM edit it also covers toasts raised after the helper runs. Also documents two things this branch learned the hard way: - Baselines must come from the hosted Linux lane. Chromium font rasterisation differs enough between platforms that the Windows-captured set this branch started with failed on every image. The bootstrap-and-review procedure is now written down in the policy. - The visual-regression-diffs artifact is not a repair source: maxFailures is 5 under CI, so it only ever holds the first five failures. Refs #1363
22 baselines, generated by the hosted Visual Regression job's own bootstrap path at a391a95 (run 34052627422, artifact visual-regression-baselines) and reviewed image by image before landing. These replace the Windows-captured set this branch started with, which could never have passed: the job runs Chromium on ubuntu-latest, and font rasterisation differs enough that every image failed there. Reviewed for layout and for per-run value leakage. Session identity, presence and profile values are masked on settings-profile and board-toolbar; the calendar renders April 2026 from the installed clock rather than the wall clock; capture-modal-typed is correctly the typed *tab* in its default empty state, not a composer with text in it. No timestamps, user ids, board ids or other run-varying values survive in any image. The toast leak that the first bootstrap exposed is gone: board-empty, board-populated, card-modal-edit and starter-pack-modal-import are now captured without the seeding toasts that had covered the board header, the board-actions row and the card modal's date control. Refs #1363
Re-captured on Linux, reviewed, and committed (
|
Green, and the one red leg was investigated rather than waved through
The webkit failure
I did not treat it as flaky on sight. The branch's own history decides it:
None of those touches Then the conclusive check: re-running the failed job against the identical tree ( So it is nondeterminism in the webkit lane, not a defect this PR introduces. Filed as #2763 with the full evidence, deliberately not fixed here — a blanket Playwright retry would hide real webkit regressions, and the label gate makes the true rate unmeasured. Worth knowing: the job only runs on Marking ready for review. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…umented Review round 1 on #2753. The fresh-context pass found that the Linux-capture section added in a391a95 contradicted four passages it had not touched, which between them still instructed both of the things that section forbids. That is a wrong executable instruction, not a style point: an agent following "Adding New Visual Tests" steps 4-6 verbatim on Windows would commit 22 Windows-rasterised PNGs, and the next visual-labelled run would fail on all of them with maxFailures: 5 truncating the report so the cause was not even fully visible. Reconciled, without weakening anything: - "Generating Initial Baselines" and "Updating Baselines" merge into one section that names the hosted lane as the only committable source and keeps npm run test:visual:update for local iteration, which is what it is good for. - The "regenerate CI-compatible baselines" recipe no longer says to lift *-actual.png out of visual-regression-diffs and commit it. That recipe is actively harmful at more than five failures: the artifact is capped, so the images you do not replace stay stale and the next run fails on those instead. Reading the diffs to confirm intent is still right, and still recommended. - "Adding New Visual Tests" step 4 now says to iterate locally and then discard those images in favour of a hosted set. - The local-vs-CI font note said baselines "may differ". They do not may — it fails on essentially every image, which is what parked this PR. - Last Updated bumped; it had stood at 2026-04-09 through substantive edits. Also corrects a stale claim in playwright.visual.config.ts, pre-existing and identical on main: the header said the snapshot path template "includes platform for OS-specific baselines" three lines above a template that has no {platform} in it. Omitting it is correct and deliberate under the single-canonical-platform policy, so the comment now says that instead. Verification: npx tsc --noEmit clean, eslint clean on both changed TS files, check-docs-governance and check-doc-links green. Refs #1363
Review round 1 — one fresh-context adversarial pass, completeVerdict: no merge blockers. The reviewer verified the mechanism against source rather than taking the commit messages on trust. One finding was a wrong executable instruction that I had introduced myself, fixed in Confirmed sound (checked against source, not asserted)
Fixed here — the policy contradicted itself (MEDIUM-1)The Linux-capture section I added in Settled from the image review rather than left open (MEDIUM-2)The reviewer flagged, as an unverified check, that Tracked, not fixed → #2764Variable-length test usernames eroding the pixel-diff budget (real, but inside Also refuted, recorded so it is not re-raised
Base moved under this PR
|
Review round 1 on #2765. No blockers; these are accuracy fixes to a correction, which is exactly the document that has to be right. Two of them are places where I over-read my own evidence: - I wrote that the false red "does not self-heal - three re-runs failed". But .claude/rules/ci-control.md already records that `gh run rerun` reuses the OLD merge ref, so those three failures are the documented consequence of that behaviour, not independent evidence that the condition persists. The re-runs now say what they actually show, and the sentence credits what really cleared it: pushing a merge of origin/main, after which Plan, Required Gate and Planner Self-Test are all green at ab3d76c. - I stated as fact that a moved base "causes it for every open PR at once". That is an extrapolation from n=1, and n=1 was forced: #2753 was the only other open PR at 19:31Z, so there was no second PR to check. The mechanism does imply it, but the text now says the generalisation is inferred from the code path rather than measured, and names what would confirm it. And three consistency fixes: - The changelog entry I wrote earlier today still published the retracted "zero false reds" figure, untouched by the correction on the row. A future session reading the changelog for what the last session established would have taken away the exact claim this PR retracts. It now carries the retraction inline. - The superseded annotation's lead-in still read "the observation window is complete and this is now actionable" in bold, unstruck. The row's four annotations are not in date order - a 2026-09-03 packet note follows the two 2026-09-06 ones - so "last annotation wins" is not a rule this row obeys and ordering alone was a weak disambiguator for a skimmer. Struck and pointed at the correction. - The 72/66/5/1 tally is now explicitly scoped to before 19:34Z, so it reads as a snapshot rather than a closed count that contradicts the correction below it. Tracked, not fixed here: docs/releases/V0_3_0_READINESS.md still records #2506 as closing this planner shape, which this evening's reproduction contradicts. That is a different document and belongs on #2562. No checkbox changed state; verified the SC-4 row is still `- [ ]` and that the diff against main adds no `- [x]` line. Verification: check-docs-governance and check-doc-links both green. Refs #2327, #2562
1 participant
Summary\n- add the reviewed 22-image Chromium visual baseline set under the configured snapshot path\n- stabilize visual captures by hiding per-run shell identity/presence and the session timeout warning\n- correct settings profile masking and document exact-head baseline bootstrap/review/negative-control rules\n\n## Verification\n-
px playwright test --config playwright.visual.config.ts --workers=1 --update-snapshots --reporter=line (22 passed)\n-
px playwright test --config playwright.visual.config.ts --workers=1 --reporter=line (22 passed)\n- deliberate temporary 10px fixture mutation: 1 failed, 7,200 differing pixels, 3 diff artifacts, snapshot restored\n-
pm run typecheck\n-
pm run build\n-
px eslint tests/visual/visual-test-helpers.ts tests/visual/settings-view.visual.spec.ts tests/visual/archive-view.visual.spec.ts\n-
ode scripts/check-docs-governance.mjs\n-
ode scripts/check-golden-principles.mjs\n- git diff --check\n\n## Notes\n- resolved the supplied stale locator to the live ests/visual/*.visual.spec.ts suite and configured snapshot template\n- screenshots were reviewed for layout and per-run value leakage; dynamic identity and profile values are excluded/masked\n- hosted checks and independent review remain pending\n\nRefs #1363