Repository navigation
Two new CI verification gates: motion and merge-base pixel parity - #46
Merged
Merged
Conversation
A real bug survived on master through every branch and was found only by accident. Navigating into a principle played its view transition 26px too low and then snapped up on completion. Every screenshot gate we have was blind to it: the page was never in the wrong place, the *snapshot* was. `.principle-prose` is both a `[data-reveal]` element and the named `principle-page` transition group, and Reveal's settle pass was deferred by `requestAnimationFrame` — one frame later than the browser photographs `::view-transition-new`. c38ef92 fixed it by settling in a microtask. Nothing in the harness could have caught it, because everything in the harness photographs a page at rest. `verify-motion` watches the arrival instead. It clicks a real link — never `goto`, which skips the router entirely and with it the whole bug class — and samples, every animation frame for two seconds, the scroll offset and the bounding rect of the elements the transition is composed of. The primer that leaks into the snapshot is a `translateY(26px)` on the real element, and `getBoundingClientRect` sees transforms, so the frame that only exists in the pseudo-element is measurable through the DOM. The invariant: from `astro:after-swap` onwards, every tracked element's document-space position — `scrollY + rect.top`, which cancels the router's own scroll — is already its resting position. Visiting a position it later corrects away from by more than 2px is two motions, not one. Stating that against the resting value rather than "the first plateau" is deliberate: a bad frame can be a single sample, too short to ever form a plateau of its own, and a plateau-relative rule would score it as the settle it corrects into. The tolerance absorbs sub-pixel settle and layout rounding; 26px was the bug and 2px is the noise floor. `reducedMotion` stays at `no-preference`. Under `reduce`, Reveal marks every target revealed at construction and the gate would assert nothing — a green run proving only that it had been switched off. Proven both ways. Reverting c38ef92's one line makes all three principle cases and both newsletter step cases fail with a measured 26px correction at post-swap frame 0, naming the route; restoring it returns every case to a 0.00px correction. The newsletter index card at 420 is honestly green even when broken, because at that width the revealed elements are below the fold and nothing on screen is photographed wrong. Alongside it, two portability fixes the gates needed before CI could run any of them: - `playwright.mjs` resolves Playwright at run time instead of four scripts hard-coding one developer's npx cache path. `node_modules` first (CI installs it there, pinned), then `PLAYWRIGHT_MODULE` or that cache. Only a genuine ERR_MODULE_NOT_FOUND falls through, so a broken installation reports itself instead of masquerading as a missing one. - `verify-parity` accepts either ImageMagick 7's single `magick` entry point or the ImageMagick 6 per-command binaries that Debian, Ubuntu and therefore the CI runners package. `parity-summary.mjs` renders the two per-viewport parity reports into one PR comment and writes its verdict to a file rather than an exit code, so a crash in the summariser can never be read as "pixels moved".
Until now CI ran lint, format, the astro check ratchet and the build, and every visual claim on this repository was a human running the harness by hand. Two gates move into CI; the four existing jobs are untouched so an ordinary PR still gets its fast feedback from them. `motion` is required and cheap: install, build, serve dist on :4126, run `verify:motion:all` across both viewports. It is the gate for a bug class screenshots structurally cannot see, so it is the one that has to be mandatory. `visual` is the merge-base pixel sweep. No baselines are committed — the job checks out the PR's merge base, installs, builds and photographs it, then does the same for the head and compares the two with `verify:parity`, per viewport. Nothing to bless, nothing to go stale, and no way to "update the baseline" as a way of making a difference disappear. The tradeoff is that it pays for two `npm ci` runs and two builds, which is why it does not run when nothing under `src/`, `public/` or `package-lock.json` changed. Enforcement is by label, because the same measurement means different things on different PRs: - `dependencies`, which is what Dependabot applies: zero differing pixels required, job fails otherwise. A bump that moves rendering must not merge silently, and this is the only place that can be noticed. - `visual-change`: the job is skipped outright. The author is declaring that pixels are meant to move, and 15 minutes of proving they did is worth nothing. - Anything else: measured and posted as a PR comment — routes changed, differing pixel counts, peak channel delta — and never failing. The count locates a change, the delta sizes it: a large count at 1/255 is a sub-perceptual shift, a small count at 200/255 is something a reader sees. The comment is found by a marker and PATCHed in place on re-runs rather than appended, so a long-lived PR gets one report rather than a column of them. `pull_request` now also listens for `labeled`/`unlabeled`, since applying `visual-change` or `dependencies` has to be able to re-decide a run that already happened. Several details are load-bearing rather than incidental: - Playwright is installed at a version pinned in the workflow, Chromium only. In the visual job it is installed outside the checkout, because the job runs `npm ci` twice and anything inside `node_modules` would be wiped between the two captures. - `capture-shots.sh` tears its server down before returning. Reusing :4126 across two builds is the one reliable way to produce a confident, meaningless green: if the first server survives, the second sweep photographs the first build and parity passes because it compared a build to itself. - The comment step is best-effort and the verdict step is not, so a fork PR's read-only token costs a comment rather than the gate.
The merge base predates the gate that measures it: checking it out and running its own scripts/verify either fails outright — .github/scripts and the Playwright resolver do not exist there — or, worse, succeeds and compares two captures taken by two different procedures. Build each side in turn, keep both dists, then photograph both from the head's checkout. One camera, two subjects; only the built output differs.
Merge-base visual parity1440x1000 — 1 of 37 routes moved.
420x900 — 1 of 37 routes moved.
Differing pixels locate a change; the peak channel delta sizes it. A large count with a Enforced: this PR is labelled |
The first CI run of the visual job passed green while comparing the merge base to itself, which is the exact failure the script warned about in its own header comment. npx spawns http-server as a child. Killing $! killed the wrapper and left the real server holding :4126, so the second capture's http-server died of EADDRINUSE in the background — where set -e cannot see it, because it is a background job — and the sweep re-photographed the merge base. Parity then reported all 37 routes pixel-identical across both viewports, on a commit that deliberately changed a date on the privacy page. Two guards, since teardown can fail in ways nobody anticipated. The server is started under setsid and the whole process group is taken down, with the port confirmed free both before starting and after finishing. And before a single screenshot is taken, the bytes served at / are checked against dist/index.html on disk. The first guard fixes the bug that happened; the second makes the entire class impossible to reach silently, whatever else goes wrong with a server nobody is watching.
axsaucedo
added
dependencies
Both existed only to be measured. bench-cache.yml answered the caching question with real CI seconds — npm ci is 5s with setup-node's cache and 8s without, an actions/cache on node_modules saves ~5s more, and caching ~/.cache/ms-playwright takes the Chromium install from 19-21s to 12s. Set against a visual job whose two builds alone are 140s and a motion job that runs in 2m51s, none of that is worth another moving part, and a restored node_modules is a tree no package manager has verified. setup-node's npm cache stays because it is already there and free; nothing else is added. The privacy-page date change existed to make the visual job find something. It did: 559 differing pixels at 1440x1000 and 88 at 420x900, at a peak channel delta of 147/255, on exactly one of 37 routes, with the other 36 pixel-identical across two independent CI builds. Labelling the PR 'dependencies' then failed the job on the same measurement. Both paths are proven, so the probe goes.
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.
CI ran
lint,format:check,check:ratchetandbuild. None of the browser harness ran in CI and no pixel baselines are committed, so every visual claim so far has been a human running the harness by hand. This adds two gates.verify:motion— arrival happens in a single motionThe bug this exists for is
c38ef92: navigating into a principle played its view transition 26px too low and snapped up on completion. No screenshot gate could see it — the page was never in the wrong place, the snapshot was.scripts/verify/verify-motion.mjsclicks a real link (nevergoto) and samples scroll and bounding rects every animation frame for two seconds. Fromastro:after-swaponwards, each tracked element's document-space position must already be its resting position; a correction larger than 2px is a second motion.Reverting
c38ef92's one line makes the gate fail with a measured 26px correction at post-swap frame 0, naming the route; restoring it returns every case to 0.00px.visual— merge-base pixel parityNo baselines are committed: the job builds and photographs the merge base and the head itself and compares them, so nothing can go stale and nothing can be blessed by hand. Enforcement is by label —
dependenciesrequires zero pixels,visual-changeskips the job, anything else measures and comments without failing. Skipped entirely when nothing undersrc/,public/orpackage-lock.jsonchanged.Also here
scripts/verify/playwright.mjsresolves Playwright at run time instead of four scripts hard-coding one developer's npx cache path.verify-parityaccepts ImageMagick 6's per-command binaries as well as 7'smagick, which is what the runners package..github/workflows/bench-cache.ymlis temporary, present only to measure the caching question, and is removed before this merges.