Skip to content

Add an opt-in visual diff of every frontend page - #740

Open
barborico wants to merge 4 commits into
mainfrom
brynna/visual_diff_tool
Open

barborico wants to merge 4 commits into
mainfrom
brynna/visual_diff_tool

Conversation

@barborico

@barborico barborico commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds npm run visual-diff, a local command that renders every route in src/App.tsx on the current branch and on its merge base with origin/main. It then lists the pages whose screenshots changed, the pages that are new, and console, network, or render errors that only the branch has. It exits non-zero only on new errors, and it adds no CI workflow and stores no baseline images.

Urgency: low - one week
Expected review effort: <15 min, assuming focus on the tool's shape and documentation rather than reading the script logic itself (IMO not worth scrutiny)

Motivation

Frontend dependency PRs such as the MUI 9 / MUI X 9 upgrade (#706) pass tsc and vitest, but nothing checks the rendered pages, and clicking through about 25 pages by hand on every upgrade isn't practical. Rendering both sides on the same machine in one run avoids stored baselines and the font differences between operating systems that make checked-in screenshots brittle.

Validation

Confidence: medium

Run on macOS (Node 22, Playwright 1.64 with Chromium):

  • With no frontend changes it reported no diffs on 10 consecutive runs. Before the 100-pixel noise tolerance was added, about 1 in 10 pages flagged 4–49 pixels of glyph antialiasing noise per run.
  • A deliberate mt: 4 → mt: 6 on the main Container flagged every page with a readable 16px-shift diff, and the run exited 0.
  • A temporary new /about route was reported as a new page, and a temporary console.error in Home was reported as a branch-only error with exit code 1.
  • A Vite dev server on port 3000 kept serving throughout a run. git status, the checkout's build/, and .claude/.api-port were unchanged afterwards.
  • Ctrl-C during the base install, during the build, and during rendering exited 130 each time, with no leftover worktree, temp directory, uvicorn, Chromium, or npm process.
  • --base HEAD~3 (whose merge base has a different uv.lock) built its own virtualenv and worked.
  • First run against a new merge base took about 35s. A cached run took about 23s. Both had warm npm and uv caches and Chromium already downloaded.
  • Ran against the MUI 9 upgrade (Upgrade to MUI 9 and MUI X 9 #706) with this branch cherry-picked onto it. It surfaced real changes: shrunken link icons and shorter accordion rows on Home (a codemod turned fontSize="small" into sx={{fontSize: 'small'}}), EmptyListEntry's "None" turning from grey to black, nav chevrons moving to the drawer edge, and Chip labels shifting by about 1px. Its one false "new error" (a pre-existing warning whose React component stack changed) led to the second commit, which compares warnings without their stacks.
  • Try it on Linux

Risks

None for the app itself: the change adds a dev-only tool and the @playwright/test devDependency. The tool's main limitation is that the seed data and the spec's route table need a case added whenever a page is added. A spec test fails the run when src/App.tsx has an uncovered <Route>.

Guidance for Reviewers

Design choices worth a look:

  • Noise tolerance. Two launches of headless Chromium on the same page still antialias a few glyphs differently, even with GPU off, sRGB forced, hinting and subpixel positioning off, and transitions disabled. Pages are allowed up to 100 differing pixels; the largest noise seen was 49. A one-character text change could fall under that. Retrying the branch's screenshot was the alternative, but it doesn't help when the base sample is the outlier.
  • Development-mode, unminified builds on both sides, so React and MUI warnings survive and their component names match across builds.
  • Isolation. Each side's backend runs from a scratch directory with a minimal environment, in a virtualenv built from its own uv.lock without dev dependencies. Neither the checkout's .env nor plugins installed in its .venv can leak in. The first attempt failed exactly this way: an example plugin installed in .venv required Google credentials.
  • One spec, one seed, both from the branch. The seed sticks to long-stable columns, so a base older than a model change to those columns would fail to seed.

Detailed Description of Changes

  • scripts/visual-diff/run.js is the orchestrator. It computes the merge base, adds a temporary git worktree, runs npm ci --ignore-scripts and builds the base on a cache miss (cache key is the commit), builds the branch from the working tree into a temp directory, seeds and starts both backends, serves each build with a small static server that proxies /api, runs Playwright twice, and prints the summary. Cache lives under ${XDG_CACHE_HOME:-~/.cache}/access-visual-diff.
  • scripts/visual-diff/seed.py and fixtures.json hold the fixed data set. Timestamps are offsets from midnight UTC of the run, and the browser clock is frozen relative to the same point, so relative dates match.
  • scripts/visual-diff/pages.spec.ts visits each route, records errors, waits for spinners and skeletons to clear, grows the viewport to <main>'s scroll height (the app scrolls inside <main>, so fullPage alone stops at one screenful), and takes the screenshot. A route that renders the not-found page on the base writes no expectation, so it reports as new.
  • Docs are in scripts/visual-diff/README.md, including what to update as pages and models change. The testing guide and the top-level README each have a short pointer to it. The frontend guide and CLAUDE.md's common mistakes tell authors to add a page case and seed data with every new route.
  • Already surfaced on main: request detail pages fire /api/audit/... with empty ids (404s), and several list pages nest <a>/<td>/<button> invalidly. These appear on both sides, so they aren't reported as new.

🤖 Generated with Claude Code

barborico and others added 2 commits November 4, 2026 17:34
Dependency upgrades like MUI 9 pass tsc and vitest without anything
checking the rendered pages. `npm run visual-diff` renders every route
in src/App.tsx on the current branch and on its merge base with
origin/main, then lists the pages whose screenshots changed, the pages
that are new, and the console, network, and render errors only the
branch has. It exits 1 only on new errors, since a screenshot diff may
be intended. It is a local tool, not a CI check.

Both sides render on the developer's machine in one run, so no baseline
images are stored and OS font differences don't matter. The base is
checked out into a temporary worktree (installed with --ignore-scripts
so husky leaves the shared core.hooksPath alone), and its install and
build are cached by commit. Each side gets its own SQLite database,
backend, and static server on free ports, seeded identically from
fixed ids and timestamps with the browser clock frozen to match, so
nothing touches Okta or a running dev server. The current branch's
Playwright spec renders both sides, so a route the branch adds reports
as new instead of failing. Ctrl-C stops every child and removes the
worktree.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The MUI 9 upgrade (#706) renamed handlers and dropped
default props on components above an existing button-in-button warning
on the home page, so the warning's component stack differed between the
two sides and the visual diff reported it as new.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@barborico barborico mentioned this pull request Nov 5, 2026
3 tasks done
Comment thread .claude/access-dev-testing.md Resolved
Comment thread scripts/visual-diff/pages.spec.ts Resolved
…rrent

The tool's details now live in scripts/visual-diff/README.md, with a
short lead-in in the testing guide saying when to reach for it. The
README adds what to update when a route, a seeded model column, or a
loading indicator changes, and the frontend guide and CLAUDE.md's common
mistakes point new routes at the spec's page table, so the table and the
seed don't drift from the app between dependency upgrades.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@barborico
barborico marked this pull request as ready for review November 5, 2026 02:54
Jeff-Rowell
Jeff-Rowell previously approved these changes Nov 5, 2026

@Jeff-Rowell Jeff-Rowell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! IMO I'd drop the "side by side" screen shot viewer or make the side by side images larger. I found I just toggled between "Actual" and "Expected" or used the slider more.

Comment thread .claude/access-dev-frontend.md Resolved
The frontend steering doc now lists the same dependencies as the tool's
README and the testing doc, so all three agree on when to run it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants