Validation only (do not merge): trunk merge for #277 - #393
Closed
mokagio wants to merge 6 commits into
Closed
Conversation
The renderer is styled by 317 inline style objects, so every panel picks its own spacing, border grey and radius and no two agree. The visible result is that controls float: "Start dev server" and "Review & submit changes" sat in bare whitespace between the header and the Trac card, owned by nothing. Introduces src/renderer/styles/tokens.css as the one place those decisions are made, four primitives in src/renderer/ui/ that consume it, and converts the site header as the first region to prove the approach end to end. The header's three stacked rows for path, copy and "Open directory in" become one meta block, and the two buttons move into a bounded action row under a hairline, so they read as this site's actions. The dev-server button loses its hand-picked 15px/12px-radius treatment for the stock WPDS shape, which in turn makes .next-action-cue read the control radius: a 10px glow around a 2px button looked like a rounded box floating behind a square one. The cue's own colour becomes --wpct-cue, pixel-identical to the #f0b849 it replaces. It is deliberately not the warning amber: the cue says "do this next" rather than "something is wrong", and it is often drawn around a warning notice, where a glow in that notice's own border colour would disappear into it. The content column's cap was already there as an inline maxWidth: 1040; it now reads --wpct-content-max-width and is tightened to 880px. status-tone.cjs maps a status to a tone and nothing else. It deliberately does not restate the checklist's words, because setupStepLabel already owns them and makes a distinction the map cannot see (#257). Two notices in the header keep their inline styling: every notice box in the window is converted together in a later pass, and doing this one now would leave two notice styles on screen until the rest caught up.
Contrast and layout, both introduced by the token pass itself, plus a lookup that could return a non-tone. `.wpct-meta code` styled every `code` element in a meta line. That was written for the path chip, but it also caught the `admin` / `password` in the dev-server credentials line, which are words in a sentence rather than a value to copy. It becomes an explicit `.wpct-chip` class on the path alone, and its text moves from --wpct-text-muted to --wpct-text: #6c6f72 on the chip's #f0f0f1 is 4.44:1, under the 4.5:1 AA threshold at 12px, where trunk's #3c434a was 8.81:1. The same credentials line was also a sentence inside a flex row, so each run of text became its own flex item — five items with a gap between every one, including before the full stop, each free to wrap separately. MetaText gains a `flow` prop for prose. statusTone read through Object.prototype: 'constructor' resolved to the Object constructor and '__proto__' to the prototype itself, both destructuring to an undefined tone and rendering a badge classed `wpct-badge--undefined`. Only already-lowercase keys could reach it, since the lookup lowercases first, but the fallback this function documents has to hold for every string. Both new test cases fail on the old lookup and pass on Object.hasOwn. A fourth finding — StatusBadge rendering an empty pill for a status whose label is deliberately blank — is left for the phase that first calls it that way.
The cap was the one part of this change nothing could show. Every screenshot is taken at 1200px, where a 280px sidebar and 32px of padding a side leave a content area of about 856px — narrower than the 880px cap. At that size the cap has no effect, so removing it entirely would change no image in docs/, while the app ran headings and meta lines to the edge of a maximised window. The harness had one window size for every shot, which is why. A shot may now carry its own `window`, and `site-view-wide` uses 1600px, where the content area is 1256px and the cap visibly bites. The bounds are set before every shot rather than only for the ones that ask, so a widened window cannot leak into the next image. test/content-column.test.cjs pins both halves: the rule and its token, that the scrolling container still applies the class, that no inline maxWidth is reintroduced to override it, and that the wide shot still exists and is still wide enough to prove anything. Each assertion was checked against a mutation that should break it — a widened token, a deleted rule, a renamed shot, a removed window override. The screenshot on its own is a regression net only if someone looks at it. This is the half that fails without being looked at.
The harness is about to be built on by six more PRs, so an API that is easy to
misuse costs more here than in a leaf change.
A shot's `window` was substituted for the default rather than merged over it.
`setBounds` takes a partial rectangle, so a shot declaring only `{ width: 1600 }`
— the natural thing to write when only the width matters — would have kept
whatever height the shot before it left, and `--only=<slug>` would have produced
a different image than a full run. That is the same "a shot inherits another's
size" bug the call was added to prevent, reappearing as a height.
`window` was also a fifth entry key documented only inside the one shot using
it, and silently ignored in the live tier. It is now in the contract at the top
of shots.cjs, and a live-tier shot that declares one fails loudly instead.
The wide screenshot was asserted with existsSync alone, which could never fail
again once the file was committed: an image captured before the cap regressed,
or re-captured at the default width, would both pass. It now reads the PNG's
IHDR width and compares it to the width the shot declares.
The two CSS fixes from the first review shipped without regression tests, in a
repo that has the pattern for exactly this. test/meta-text.test.cjs computes the
chip's contrast ratio from the tokens rather than hard-coding it, and pins the
prose layout — including that .wpct-meta--flow still comes after .wpct-meta,
since equal specificity means source order is what makes it win.
Each assertion was checked against a mutation that should break it: the chip
colour reverted, the descendant selector reintroduced, `flow` dropped from the
credentials line, the flow rule moved above the rule it overrides, the shot's
declared width changed out from under the committed image, and the cap token
deleted. The last one previously died with a TypeError instead of a message.
Also corrects the content-area figure in two comments from 856px to the measured
855px — the sidebar's right border is a pixel.
Three PNGs changed on every run with no code change at all. Two causes, both fixed here. CSS animations. The "Checking GitHub…" spinner was photographed at whatever angle it happened to be at, so `page.screenshot` now passes `animations: 'disabled'`, which freezes animations and transitions and rewinds them to their first frame. The linked-pull-request lookup. Selecting a site with a linked ticket fires a network call, and the panel spins until it answers, so whether a shot caught the spinner or the result was a coin flip. `selectSite` now waits for the spinner to go. That fixes a real docs bug as well as the churn: the same run produced site-view.png mid-check and trac-ticket-panel.png already resolved, which is two different answers to the same question in one set of images. The wait is bounded and swallowed, because a site with no ticket never shows the spinner at all, and a harness that hangs on a slow GitHub is worse than one that photographs a spinner. The three re-captured images are the fix taking effect. This is not UI polish, and it is here rather than on its own branch because the alternative is worse. This branch already edits both harness files and eleven of the screenshots, including these three. A separate PR touching the same files would collide with this one on rebase, and a binary conflict has no resolution except to pick a side and re-capture — which is the exact failure this commit exists to stop. It matters now because the phases after this one stack on this branch. A full run is still occasionally not reproducible on site-menu.png and stale-site-notice.png, while each shot is reproducible on its own under `--only`. That points at network and cache state shared across shots in one app instance, and ending it needs the fixture to stop reaching GitHub at all. Left for its own issue.
Five conflicts needed a decision rather than a pick. `src/renderer/index.jsx` — trunk moved the Adminer control out of the action row and into the URL line as `DB inspect (Adminer)`, while this branch still had it as a button in the row it was tokenising. Keeping both would have shipped two ways into Adminer, so the button goes and the row closes as `</ActionRow>` rather than trunk's `</div>`, since the opener here is the tokenised one. `scripts/screenshots/capture.cjs` — the two sides added different functions at the same point, `setWindow` here and `expandHome` on trunk. Both are called, so both stay. `scripts/screenshots/shots.cjs` — trunk moved `setup-wizard` and `site-view` to the live tier (#298) because seeded state cannot produce what they photograph. That move stands. `site-view-wide` stays a fixture shot: what it depends on is the window width, not the Trac facts that forced the others across, and other fixture shots still drive `selectSite` the same way. The three test files move to `tests/unit/` with the rest, and their paths out to `src/`, `scripts/` and the repo root gain the level that move costs them. The screenshots take this branch's versions. Both sides changed all seven, and these are the ones showing the header this branch exists to change — but three of them are live-tier now, so only their author can recapture them. --- Generated with the help of Claude Code, https://code.claude.com Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Validation-only merge of trunk into #277 to exercise CI and surface integration costs; it is not intended for merge.
Changes:
- Adds renderer design tokens and reusable UI primitives.
- Converts the site header and action area to the new styling layer.
- Stabilizes screenshot capture and adds regression tests.
Reviewed changes
Copilot reviewed 13 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
docs/public/screenshots/apply-patch-panel.png |
Refreshes panel screenshot. |
docs/public/screenshots/create-site-modal.png |
Refreshes modal screenshot. |
docs/public/screenshots/debug-log.png |
Refreshes log screenshot. |
docs/public/screenshots/mail-panel.png |
Refreshes mail screenshot. |
docs/public/screenshots/setup-wizard.png |
Refreshes wizard screenshot. |
docs/public/screenshots/site-menu.png |
Refreshes site-menu screenshot. |
docs/public/screenshots/site-view-wide.png |
Adds wide-layout reference. |
docs/public/screenshots/site-view.png |
Refreshes site view. |
docs/public/screenshots/stale-site-notice.png |
Refreshes stale-site view. |
docs/public/screenshots/terminal.png |
Refreshes terminal screenshot. |
docs/public/screenshots/trac-ticket-panel.png |
Refreshes Trac panel screenshot. |
scripts/screenshots/capture.cjs |
Adds deterministic capture and per-shot sizing. |
scripts/screenshots/shots.cjs |
Updates screenshot tiers and definitions. |
src/renderer/index.html |
Tokenizes next-action cue styling. |
src/renderer/index.jsx |
Migrates header and actions to primitives. |
src/renderer/status-tone.cjs |
Maps statuses to semantic tones. |
src/renderer/styles/tokens.css |
Defines tokens and component styling. |
src/renderer/ui/ActionRow.jsx |
Adds grouped-action primitive. |
src/renderer/ui/MetaText.jsx |
Adds supporting-text primitive. |
src/renderer/ui/Section.jsx |
Adds semantic section primitive. |
src/renderer/ui/StatusBadge.jsx |
Adds reusable status badge. |
tests/unit/content-column.test.cjs |
Tests width cap and wide screenshot. |
tests/unit/meta-text.test.cjs |
Tests flow and contrast rules. |
tests/unit/status-tone.test.cjs |
Tests status mapping behavior. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+36
to
+40
| // Setup checklist step states, from computeSetupStepState in setup-steps.cjs. | ||
| complete: { label: '', tone: 'success' }, | ||
| current: { label: '', tone: 'info' }, | ||
| pending: { label: '', tone: 'neutral' }, | ||
| locked: { label: '', tone: 'neutral' } |
| .wpct-badge--neutral { | ||
| background: var(--wpct-surface-subtle); | ||
| border-color: var(--wpct-border); | ||
| color: var(--wpct-text-muted); |
Comment on lines
+87
to
+91
| async function setWindow(app, bounds) { | ||
| await app.evaluate(({ BrowserWindow }, size) => { | ||
| const win = BrowserWindow.getAllWindows()[0]; | ||
| win.setBounds({ x: 40, y: 40, ...size }); | ||
| }, bounds); |
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
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.
Why
Every open PR needs
trunkmerged before the Buildkite pipeline is unarchived (AINFRA-2941), so a signed build off a stale branch cannot re-embed the App Store Connect key that #390 stopped leaking.#277 is 53 commits behind and conflicts in 13 files, which reads like the worst of the set. It is not: git auto-merged almost all of it, and the real decisions come down to five.
What changes
Nothing of its own — this is #277's head with
trunkmerged in. The five decisions are in the merge commit message; three are mechanical and two are @ryanwelcher's to confirm:src/renderer/index.jsx— one hunk, not the rewrite the diff stat implies. Trunk moved the Adminer control out of the action row into the URL line asDB inspect (Adminer). This branch still had it as a button in the row it was converting to tokens. Keeping both would ship two routes into Adminer, so the button goes and the row closes as</ActionRow>.scripts/screenshots/shots.cjs— needs your call. Trunk movedsetup-wizardandsite-viewto the live tier in #298, because seeded state cannot produce what they photograph. I kept that and keptsite-view-wideas a fixture shot, reading its dependency as the 1600px window rather than the Trac facts that forced the others across — other fixture shots still driveselectSitethe same way. If that reading is wrong, this is the line to change.The seven screenshots — needs your call. Both sides changed all seven. I took this branch's, since they show the header the branch exists to change. But
setup-wizard,site-viewandtrac-ticket-panelare live-tier now, so only you can recapture them against a real site; the other four arenpm run shots.How to test this
Platforms: macOS and Windows — three test files changed directory depth, so the Windows leg matters.
Starting state: this branch's GitHub Actions runs against the current head commit.
npm run lintexits 0,npm run build:onceexits 0.What must not have happened: the three relocated tests must not be silently skipped. They moved from
test/totests/unit/, and git moves a file without rewriting what is inside it, so a stale relative path throwsMODULE_NOT_FOUNDrather than failing visibly.tests/unit/content-column.test.cjsis the one to watch — it readsshots.cjsand assertssite-view-wideis still in the list, so it fails if the tier decision above is wrong.Risks and limitations
The screenshot harness itself was not run. CI does not exercise it, and the fixture tier needs a real Electron launch, so whether
site-view-widestill captures correctly after trunk's tier changes is unproven — it is checked here only through the shot-list assertion.This cannot exercise macOS signing or notarization; that lives in the archived Buildkite pipeline. It does cover the packaged smoke job's
.codesigningcanary from #390.Opened as a normal PR rather than a draft on purpose:
e2e.ymlskips drafts, so a draft would come back green having run neither the journeys nor the packaged jobs.Related
Part of AINFRA-2941. Validates the trunk merge for #277. Same exercise as #391 did for #358.