docs(ui): design the Compass UI visual-regression gate (RIG-2154) - #932
Open
rigel-mintaka wants to merge 6 commits into
Open
docs(ui): design the Compass UI visual-regression gate (RIG-2154)#932rigel-mintaka wants to merge 6 commits into
rigel-mintaka wants to merge 6 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
rigel-mintaka
changed the base branch from
main
to
compass-managed/rig-3107-ledger-renumber
September 6, 2026 08:37
|
Compass engineering docs preview: https://compass-ux-2154-visual-regre.compass-eng-docs.pages.dev Deployed from Changed pages: |
Freeze the automated visual-regression gate for the Compass UI: built-in Playwright `toHaveScreenshot` with baselines committed in-repo and regenerated only by a pinned nix Chromium in CI, never from a developer machine. - `visual-gate` moon task hard-gates the PR from first landing - `regen-visual-baselines` `workflow_dispatch` is the only baseline writer - `maxDiffPixelRatio` 0.001 + `threshold` 0.2, per-shot floors 10-25px - 11 captures: 7 fullPage, 3 element, 1 clip - @playwright/test 1.62.1 Ledger row DL-341 lands with the record in this commit. The Open Questions section carries Matt's five load-bearing forks; per skill://design they block the merge-freeze, not the submit. Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
force-pushed
the
compass-ux/2154-visual-regression-gate
branch
from
September 6, 2026 08:49
2b8fc4b to
e1be9dd
Compare
rigel-mintaka
changed the base branch from
compass-managed/rig-3107-ledger-renumber
to
main
September 6, 2026 08:49
rigel-mintaka
changed the title
…ions (RIG-2154) Review found the record's central threshold mechanism inverted against Playwright 1.62.1's actual comparator. The record prescribed a config-level `maxDiffPixelRatio` of 0.001 plus per-shot `maxDiffPixels` of 10-25px described as an "absolute floor". Playwright resolves the two pixel-count knobs with `Math.min`, not max (`playwright-core/lib/coreBundle.js:7556-7562`), and a config-level ratio is never stripped from a per-call merge — `maxDiffPixelRatio` is absent from `NonConfigProperties` (`playwright/lib/matchers/expect.js:12391-12398`, merged at `:12419-12423`). So a per-shot `maxDiffPixels` could only ever tighten tolerance. Measured against the committed baselines, `state-dot.png` (9x10, area 90) resolved to an effective tolerance of 0px — byte-exact, precisely the outcome the section existed to prevent — while `bridge-colheads` and `right-sidebar` were tightened rather than widened. The widening knob is now a per-shot `maxDiffPixelRatio`, which does replace the config default, computed as `max(base, floor / area)`: `state-dot` 0.1111, `bridge-card` 0.00117, and explicitly no override for `bridge-colheads` or `right-sidebar` (their area-scaled budgets already exceed the intended slack). Global Constraints, T1, T2 and OQ-1 are restated to match; OQ-1 stays open. Also: - Re-resolve every `.github/workflows/ci.yml` citation. The file has grown to 2434 lines and the offsets were stale by +2 early and +48 in the regen region, so cited ranges landed in the wrong job. Each citation now carries a durable job-name/quoted-string anchor alongside the number. - Narrow DL-341 to what is actually ruled. The row froze tolerance values, the hard-gate rollout, and capture coverage while OQ-1/OQ-4/OQ-5 still await Matt's ruling; it now states the mechanism decisions only and names those three as open. Co-authored-by: Matt Wilkinson <matt@rigel.build>
… (RIG-2154) - The peer-job rationale named `dogfood-e2e`, which is not a job in ci.yml. The dogfood suite runs inside the `e2e` job, split out for a privileged container rather than closure weight; only a cache-key prefix carries that string. Replaced with `microvm` (`ci.yml:624`), a genuine closure-weight peer job. The argument it supports is unchanged. - T2's test cycle told the executor to run a local `--update-snapshots` and parenthesized the baselines as "NOT committed", but that command silently rewrites all 11 tracked baseline PNGs and nothing ignores `__screens__/` (it must stay tracked — the baselines are the oracle). Added an explicit `jj restore apps/ui/e2e/__screens__` after the verification run, and stated in Global Constraints that the dirtying is by design, so the CI-only rule is enforced by review rather than tooling. - The `snapshotPathTemplate` justification understated the default it avoids: the default relocates every baseline into a `<specfile>-snapshots/` subdirectory as well as appending project/suffix segments, so the chosen template prevents a directory move, not just a rename. - T5's ordering parenthetical implied T3 lands split across two steps while the Tasks checklist treats it as atomic. Restated to match. Co-authored-by: Matt Wilkinson <matt@rigel.build>
Second review pass confirmed the threshold mechanism fix is materially
correct — verified against real Playwright 1.62.1 — but found the fix
reintroduced its own defect one pixel deep, plus three anchor and
consistency errors.
- The derived overrides were written as truncated decimals. The comparator
tests `count > area * ratio` with no rounding, so `0.1111` on state-dot
yields a budget of 9.999px and tolerates 9 pixels, one short of the
intended 10; `0.00117` on bridge-card yields 24.988px and tolerates 24, not
25. Both are now prescribed as exact fractions (`10/90`, `25/21357`), which
give budgets of exactly 10.0 and 25.0. Any decimal written for readability
is the rounded-up form. The Threshold section now states the no-rounding
constraint so it is self-enforcing, and T2 prescribes the call-site form as
a derived expression rather than a transcribed literal.
- The Threshold section's opening still said all four close-ups need a
per-shot widening, contradicting its own conclusion — and the explicit
"an executor must not add one" — thirty lines later. bridge-colheads and
right-sidebar already carry 35px and 260px of area-scaled budget. The
opening now names only the two shots that take an override.
- The durable anchors added in the previous commit named a `moon-battery`
job, which does not exist: the job is `moon` (ci.yml:236) and `Moon
battery` is a step within it (ci.yml:378). All nine anchors corrected; the
line numbers were already right and are unchanged. The "no business
building" quote at ci.yml:1128 is reattributed to `gtk4-e2e` (ci.yml:1122),
whose own rationale it is.
- T3's failure-artifact step specified a fixed artifact name against a
four-leg matrix job, which is a hard error under upload-artifact v4+ when a
second leg fails, and dropped the `matrix.run == 'true'` conjunct every
step in that job carries. Now `visual-gate-diffs-${{ matrix.group }}` gated
on `if: failure() && matrix.run == 'true'`, with a note that
actions/upload-artifact is new to the file and needs a fresh SHA pin.
Co-authored-by: Matt Wilkinson <matt@rigel.build>
…l (RIG-2154) Third review pass confirmed the previous round's four fixes correct against a calibrated real-comparator harness, and found that round's own fix had made the call-site prescription unexecutable. - The derived-expression form `Math.max(BASE, 10/90)` referenced a symbol defined nowhere: `BASE` appeared three times, all uses, no binding, and `playwright.config.ts` exports only the config object, so nothing was importable. An executor transcribing it got a ReferenceError — the previous text was wrong-but-executable, this was neither. The wrapper was also unreachable in principle: a per-shot `maxDiffPixelRatio` replaces the config base rather than combining with it, so the base is not in scope at the call site. Both sites now prescribe the resolved literal fractions (`10/90`, `25/21357`), with `max(base, floor / area)` kept in prose as the derivation the reader performs. - The claim that bridge-colheads and right-sidebar need no override was stated unconditionally, and as an absolute "an executor must not add one", but the 35px/260px budgets behind it hold only at base 0.001 — which OQ-1 has not settled. At 0.0005 bridge-colheads falls to 17.53px, below the 25px of intended slack, so the record's own rule yields an override there. Both statements are now conditional on the ruling, right-sidebar is noted safe at every candidate base (130px even at 0.0005), and the base-specific figures carry an `@0.001` tag. The Global Constraints bullet and T2's tail restated the same flat claim and are scoped to match. No base is chosen — OQ-1 remains open. - The comparator citation for the unrounded `count > budget` test pointed at the budget computation two lines above it; corrected to `:7557,7564`. The `Math.min` citation it was borrowed from is unchanged. - Noted that the exact-fraction form needs a per-area check: float division can land `area * (floor / area)` a hair under the floor, in which case the rounded-up decimal is required. Both current cells are clean. Co-authored-by: Matt Wilkinson <matt@rigel.build>
Fourth review pass found the conditional rewrite reached four of six base-dependent sites and left two stating the override set as a flat fact. - The Threshold opening said "the 2 smallest shots need a per-shot widening" unqualified while qualifying only the second half of the same sentence, which by contrast implied the count is base-independent. It is not: the count is 3 at base 0.0005, 2 at 0.001 and 1 at 0.002. Now scoped to the recommended base with the three counts stated. - T1 described "the two per-shot overrides" as a fixed pair. Reworded to the overrides the ruled base ratio yields, noting two at the recommended 0.001. Both are completions of the previous commit's own edit rather than new claims; no arithmetic changed and OQ-1 remains unruled. Co-authored-by: Matt Wilkinson <matt@rigel.build>
rigel-mintaka
marked this pull request as ready for review
September 6, 2026 10:45
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Freeze the automated visual-regression gate for the Compass UI: built-in
Playwright
toHaveScreenshotwith baselines committed in-repo andregenerated only by a pinned nix Chromium in CI, never from a developer
machine.
visual-gatemoon task hard-gates the PR from first landingregen-visual-baselinesworkflow_dispatchis the only baseline writermaxDiffPixelRatio0.001 +threshold0.2, per-shot floors 10-25pxLedger row DL-341 lands with the record in this commit.
The Open Questions section carries Matt's five load-bearing forks; per
skill://design they block the merge-freeze, not the submit.
Co-authored-by: Matt Wilkinson matt@rigel.build