Skip to content

Add a screenshot harness that photographs the app for the docs site - #231

Merged
juanmaguitar merged 1 commit into
trunkfrom
docs/screenshots
Aug 10, 2026
Merged

Add a screenshot harness that photographs the app for the docs site#231
juanmaguitar merged 1 commit into
trunkfrom
docs/screenshots

Conversation

@juanmaguitar

@juanmaguitar juanmaguitar commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Why

A user guide needs pictures of the screens it describes, and they have to be retakeable by
whoever changes the UI — a screenshot nobody can regenerate is a screenshot that goes stale and
starts lying. This adds the harness. The images themselves land with the guide in #232.

Stacked on #230; review that one first.

What changes

  • npm run shots launches the repo's own Electron binary through playwright-core's Electron
    driver, drives each documented screen by the words on it, and writes a fixed-size PNG into
    docs/public/screenshots/. Eleven screens are declared today.
  • A new TOOLKIT_USER_DATA_DIR hook in src/main.js (10 lines) points userData at a
    throwaway directory holding a seeded settings.json, so the harness never reads or writes the
    contributor's real site registry.
  • test/user-data-override.test.cjs pins both sides of that hook's guard.
  • Screens a seeded registry cannot reach — a running dev server, a real diff, the GitHub
    device-code screen — are declared as a live tier rather than faked:
    npm run shots -- --tier=live prompts for each and waits.
Why these specific choices

playwright-core, not playwright — same Electron driver, no postinstall script, no browser
download, so no new entry in allowScripts. If #70 lands @playwright/test, this direct
dependency can go and the harness can require the driver from there; the note is in the file.

Under scripts/, not e2e/ — it is a CLI, not a test. #70 makes e2e/ the Playwright
testDir, and nothing here is run by npm run test:e2e. It also means no new ESLint exemption:
scripts/**/*.cjs already allows console.

Username-free fixture paths (/tmp/wpct-docs-fixture/...) — safe-log.js redacts logs, but
nothing redacts a screenshot, and every path the app renders ends up in a published image.

Captured locally, not in CI — capturing needs the app running against a seeded site, which is
minutes of flaky work on a runner for images that change rarely.

How to test this

Starting state: a machine that has used the app before, so you have real sites registered.
npm ci done on this branch.

  1. Launch the app normally (npm start) and note your real sites in the sidebar. Quit.
  2. npm run shots — the app opens and closes a few times; expect eleven ✓ <name>.png lines and
    the files under docs/public/screenshots/.
  3. Open each PNG. Every path visible must start /tmp/wpct-docs-fixture/ — your home directory
    must appear nowhere    .
  4. npm start again — your real sites are still there, exactly as in step 1.
  5. npm run shots -- --only=terminal captures just that one; --only=nope fails and lists the
    valid slugs.
  6. npm test — green, including the three new guard tests.

Must not have happened: your real settings.json must be untouched (step 4 is the check), and
no packaged build may honour the variable — the third guard test is what pins that.

Platforms: driven on macOS. The fixture root falls back to os.tmpdir() on Windows, which is
the part most worth a second pair of eyes; the harness is a developer tool, so a Windows failure
costs a retake, not a shipped bug.

Risks

src/main.js gains an environment-driven app.setPath call. It is guarded by !app.isPackaged,
so an installed build ignores it — that guard is the whole security story here and the reason the
test asserts the packaged case explicitly.

Nothing automated catches a stale screenshot. CONTRIBUTING now says a UI-changing PR re-runs
npm run shots; that is a convention, not a gate.

Related

Stack: #230 → this → #232 (guide content).
Related to #70: it defines e2e/ as the Playwright test directory, which is why this lives in
scripts/. No change needed there.

Self-review — 2 findings fixed, 1 follow-up taken anyway, security guard verified

Ran .github/instructions/code-review.instructions.md with the judgement pass in a fresh context.
Deterministic layer was clean.

2 [fix here] · 1 [follow-up]. All three fixed:

Dimension Was Now
architecture 🔵 app.setPath throws when the directory does not exist (Electron typings, electron.d.ts:1781). It sat at module scope, before initLogging() and before any window, so a stale TOOLKIT_USER_DATA_DIR in a shell profile — or a temp dir the OS reaped — killed npm start with a stack on stdout and nothing in the log file wrapped; a bad value is reported on stderr and the real userData is kept. A fourth test covers it, and it fails without the fix (verified by reverting the guard)
cross-platform 🔵 spawnSync('npm', …, { shell: true on Windows }) — a bare host npm and a Windows shell, both shapes the standard calls out deleted. "shots": "npm run build:once && node …" does the same job with no spawn, no shell, no existsSync
architecture 🔵 the live tier called launchApp({}), and launchApp spreads process.env — so an exported TOOLKIT_USER_DATA_DIR (easiest to acquire while debugging the fixture tier) would silently point the "your real sites" tier at fixture state, producing committed screenshots that are wrong in a way only a careful look at the sidebar reveals passes TOOLKIT_USER_DATA_DIR: undefined explicitly. Taken despite being [follow-up]: one line, and the failure mode is a wrong published image

The security guard was checked hard and stands. Nothing required above the hook resolves
userData at require time — settings-store.js defers its import('electron-store') to the first
getStore(), and logging.js only reads app.getPath('logs') inside initLogging(). sessionData
follows the override too, since it resolves lazily from userData pre-ready. A packaged build is
genuinely dead to the variable.

And the test was checked against the "green while proving nothing" list. It requires the real
src/main.js under a stubbed electron and asserts on calls the real guard makes; deleting either
condition fails a case. Module._load, process.env and the require-cache entry all restore in a
finally, and node --test gives each file its own process, so there is no cross-file leak.

Also verified: playwright-core has no scripts field and no hasInstallScript in the
lockfile, so it owes no allowScripts entry and downloads no browsers. cleanFixtureSites can only
remove the one module-level fixture root; userData dirs come from mkdtempSync; an unexpected
variant still writes only under those roots.

This was referenced Aug 10, 2026
Merged
Merged
@juanmaguitar
juanmaguitar force-pushed the docs/site branch 2 times, most recently from 133fea3 to fd9810e Compare August 10, 2026 12:52
juanmaguitar added a commit that referenced this pull request Aug 10, 2026
@juanmaguitar
Add the VitePress documentation site and its Pages deploy workflow (#230
b5d3d7f 
)

## Why

The app's user documentation is the README, and it has outgrown it:
install steps, a ten-step
walkthrough and the trunk-update rules all compete for the same page,
and there is nowhere to put
a screenshot. This PR gives the guide a home and a deploy pipeline. It
carries no guide content
beyond the landing page — that arrives in #232, and the harness that
photographs the app in #231.

## What changes

- `docs/` becomes a VitePress site with **its own npm package**, so a
docs-only CI job never runs
the root `postinstall` (electron-builder + esbuild) and the app's
dependency tree stays free of
  a static-site generator.
- `.github/workflows/docs.yml` builds and deploys to GitHub Pages on
pushes to `trunk` that touch
`docs/`. Pull requests get a **build-only** job: a dead link fails
before merge, and the job
  holding the OIDC token never runs on PR code.
- The site's `base` is derived from the repository name at run time. A
project Pages site is served
under `/<repo-name>/`, and this repo has already been renamed once —
deriving it means another
  rename cannot break every asset URL.
- `srcExclude` keeps `docs/testing.md` (from #70) out of the published
site: it documents how to
run the suites, which is contributor material, and this site is for
users of the app.

## How to test this

**Starting state:** this branch checked out, `npm ci` done.

1. `npm run docs:build` — expect `build complete`. Now add a link to a
page that does not exist in
`docs/index.md` and run it again: it must **fail** with `dead link(s)
found`. Undo.
2. `npm run docs:preview`, open the printed URL — the home page renders
with the WordPress
Contributor Toolkit hero and the sidebar. Clean URLs work; this is
exactly what Pages serves.
3. `npm run docs:dev` — pages are served at their `.html` paths in dev
(`/index.html`). With the
dev server open, run `npm run docs:build` in another terminal: the dev
server must **not** spew
   reload lines (it ignores its own output directory).
4. `npm run lint` and `npm test` — both green, unchanged from `trunk`.

**Must not have happened:** no `deploy` job may run on this pull request
— check the Actions tab
and confirm only `build site` ran.

**Not testable by hand here:** the deploy itself. It needs the workflow
on `trunk`, and Pages is
already set to "GitHub Actions" as its source.

## Risks

Merging this alone deploys a site whose sidebar names the full guide, so
**those links 404 until
the content PR lands**. Merge the stack in order rather than leaving
this on `trunk` by itself.

The deploy job holds `pages: write` and `id-token: write`. Its actions
are pinned to commit SHAs
rather than tags, following the reasoning already written down in
`download-stats.yml`.

## Related

Stack: this → #231 (screenshot harness) → #232 (guide content).
Touches #70 only through `srcExclude`; that PR needs no change.

---

<details>
<summary><b>Self-review</b> — 5 findings, all fixed</summary>

Ran `.github/instructions/code-review.instructions.md` with the
judgement pass in a fresh context.
Deterministic layer was clean (ESLint and the unit suite on macOS and
Windows).

**5 [fix here] · 1 [follow-up]. All five fixed in this branch:**

| Dimension | Was | Now |
|---|---|---|
| security 🟡 | `checkout` kept the job's `GITHUB_TOKEN` in `.git/config`
while the job runs PR-authored code (`npm ci` with lifecycle scripts,
and a VitePress build that evaluates `config.mjs`) |
`persist-credentials: false`, matching `lint.yml` |
| security 🔵 | `if: github.event_name != 'pull_request'` let a
`workflow_dispatch` on any ref publish to the live site, which is not
what the comment claimed | `if: github.ref == 'refs/heads/trunk'` |
| cross-platform 🔵 | `node-version: 22` hardcoded while `.nvmrc` says
24.18.0 — reintroducing the Node drift of #37/#46 for the one command
CONTRIBUTING calls "what CI runs" | `node-version-file: .nvmrc` |
| architecture 🔵 | one concurrency group spanning build and deploy, with
`cancel-in-progress: true`, so a second push could cancel an in-flight
`deploy-pages` | split: builds cancel, deploys do not |
| architecture 🔵 | CONTRIBUTING documented `docs:*` without saying the
nested package needs its own install, so the commands fail on a clean
clone | `npm ci --prefix docs` documented |

**Deferred [follow-up]:** `electron-builder` has no `files` filter, so
tracked `docs/` sources ship
inside `app.asar` — and #232 adds screenshots on top. Real,
pre-existing, and overlaps #23; an
`!docs{,/**/*}` entry closes it. Not done here because it changes what
every release artifact
contains, which deserves its own PR and its own testing.

**Checked and clean:** all four action pins resolve to the tags their
comments claim (verified
against the GitHub API); `pull_request_target` is correctly not used and
no fork PR can reach
`pages: write` / `id-token: write`; the lockfile is 175 packages, all
from registry.npmjs.org, with
install scripts only on esbuild and fsevents; the `import/no-unresolved`
exemption is a genuine
false positive and hides nothing; no `src/` code, IPC surface or spawn
path is touched.
</details>
@juanmaguitar
Add a screenshot harness that photographs the app for the docs site
eef4df1 
The guide needs pictures of every screen, and they have to be retakeable by
whoever changes the UI. `npm run shots` launches the repo's own Electron binary
through playwright-core's Electron driver, drives each documented screen by the
words on it, and writes a fixed-size PNG into docs/public/screenshots/.

It never touches the contributor's real sites. A new TOOLKIT_USER_DATA_DIR hook
in src/main.js points userData at a throwaway directory holding a seeded
settings.json. The hook is guarded to dev runs — a packaged app ignores the
variable, so no installed build can be redirected to an attacker-chosen store
path through the environment — and test/user-data-override.test.cjs pins both
sides of that guard.

Fixture site paths are deliberately username-free (/tmp/wpct-docs-fixture/...):
safe-log.js redacts logs, but nothing redacts a screenshot, and every path the
app renders ends up in a published image.

Screens a seeded registry cannot reach — a running dev server, a real diff, the
GitHub device-code screen — are declared as a live tier instead of faked:
`npm run shots -- --tier=live` prompts for each one and waits.

playwright-core rather than playwright: same Electron driver, no postinstall
script, no browser download. If #70 lands @playwright/test, this direct
dependency can go and the harness can require the driver from there.

The harness lives under scripts/ rather than e2e/ because it is a CLI, not a
test: #70 makes e2e/ the Playwright testDir, and nothing here is run by
`npm run test:e2e`.
@juanmaguitar
juanmaguitar changed the base branch from docs/site to trunk August 10, 2026 13:17
@juanmaguitar
juanmaguitar merged commit e2cd912 into trunk Aug 10, 2026
5 of 8 checks passed
juanmaguitar added a commit that referenced this pull request Aug 10, 2026
@juanmaguitar
Write the user guide and point the README at it (#232)
e0be741 
## Why

With the site (#230) and the harness (#231) in place, this is the
documentation itself: eighteen
pages covering every screen the app has, and a README that hands the
user-facing material over
instead of duplicating it.

Stacked on #231; review the two below it first.

## What changes

- **Eighteen guide pages**: creating a site, the setup wizard, running
the site, trunk updates,
editors, Trac tickets, applying patches and pull requests, the three
submit destinations, the
terminal, logs and `debug.log`, the mail catcher, Adminer, managing
sites, troubleshooting.
- **Eleven screenshots** from `npm run shots`, ~550 KB total.
- **README shrinks to what a contributor to this repository needs.** The
platform-by-platform
install, the Gatekeeper notes, the ten-step walkthrough and the whole
"Keeping a site up to date
with trunk" section **move** into the guide — they are not copied.
AGENTS.md asks for one home
per fact, and a README restating the guide is exactly the drift that
rule exists to prevent.

<details>
<summary>Two things the code corrected while writing</summary>

Each page was written against `src/renderer/index.jsx` rather than from
memory, so it names
controls by the words actually on screen. Two claims did not survive
that check:

- **The Logs panel has two tabs, not three.** `logTabs` defines Server
and `debug.log`; npm output
  goes to the Terminal. The page says so.
- **The Trac destination never posts anything.** It saves the plain diff
and opens the ticket's
attach page; you upload it yourself. The page says that plainly rather
than implying the app
  files it for you.
</details>

## How to test this

**Starting state:** this branch checked out, `npm ci` done.

1. `npm run docs:build` — expect `build complete` and **no dead links**.
This is the real check on
   eighteen pages of cross-references.
2. `npm run docs:preview` and open the printed URL. Walk the sidebar top
to bottom: every entry
   resolves, and each page's screenshots load.
3. Open the app beside the guide and pick three pages at random — every
control named in bold must
   exist on screen with exactly that wording.
4. Read the README end to end: it must not restate anything the guide
covers, and its
"Documentation" pointer must resolve to
`https://wordpress.github.io/contributor-toolkit/`.

**Must not have happened:** no user-facing instruction may exist in both
README and the guide.
That duplication is the specific failure this PR is shaped to avoid.

**Not testable by hand here:** the live Pages URL — it only exists once
this reaches `trunk`.

## Risks

The guide describes UI that changes. Nothing automated catches a page
that has drifted from the
app; the convention added in #231 (re-run `npm run shots`, re-read the
page) is what holds it.

Screens needing a running server are **not illustrated yet** — the live
tier exists for them and
those images can land later without touching the prose.

The README's badge URLs still use the old repository name. They
redirect, and renaming them is
unrelated churn; deliberately left for a separate change.

## Related

Stack: #230 → #231 → this.

---

<details>
<summary><b>Self-review</b> — 8 findings, all fixed; three of them were
false statements about the app</summary>

Ran `.github/instructions/code-review.instructions.md` with the
judgement pass in a fresh context,
pointed specifically at verifying every claim against `src/`. That was
the right call: the review
found three places where the documentation simply said something untrue.

**8 [fix here] · 1 [follow-up]. All fixed:**

| Severity | Was | Now |
|---|---|---|
| 🔴 | The ten-step walkthrough said **Install dependencies** and **Build
WordPress**. The renderer says `Install npm dependencies` and `Run full
build`, and "Build WordPress" exists nowhere in the app — while the
screenshot directly below showed the real labels | both corrected |
| 🟡 | `terminal.md` recommended `npm run watch`.
`dev-server-command.cjs` documents why the app refuses it: the
Gruntfile's `watch` wrapper runs a full production build first, "30+
minutes on a Windows VM" — the exact failure this project exists to
remove | replaced with a warning callout explaining the trap, and a
pointer to the watcher the dev server already starts |
| 🟡 | `creating-a-site.md` sent a failing contributor to look in "the
dialog". The modal closes before `setupWordPress` is awaited, so
`createSiteError` is set into state nothing renders | says the row
disappears and the reason is in **Help → Open App Log** |
| 🟡 | **Show in File Explorer** — the Windows label is `Show in
Explorer`, and `editors.md` already had it right, so the guide
contradicted itself | corrected |
| 🟡 | The README still duplicated the guide's "what you get" bullets and
the "No Git, no Node" paragraph — and the two copies had already
drifted, which is the exact failure AGENTS.md's single-source rule
exists to prevent | README keeps one positioning sentence; the expanded
version lives only in the guide |
| 🔵 | The update step was called **Fetch latest trunk** (a label that is
never rendered; the screen says `Fetch and reset to trunk`), and said
uncommitted changes "are saved" — unconditionally, when **Discard them**
sits right beside that button and loses the work | both corrected, and
the destructive choice is now named |
| 🔵 | Said a hint stays clickable while a command runs;
`TerminalCommandLink` returns plain text when disabled | corrected |
| 🔵 | **Copy patch** — the button says `Copy` | corrected. Also noted
that the ☰ **Update to latest trunk** entry, unlike the notice button,
is not disabled while the server runs and silently does nothing |

**Deferred [follow-up]:** none outstanding — the two stale "Ideas and
future work" entries the guide
now contradicts (the SMTP catcher and the Trac/GitHub integration, both
shipped) were removed here
rather than deferred, since leaving them is the same drift the rest of
this PR is fixing.

**Verified correct, and worth listing because they were the claims most
likely to be wrong:** the
Trac destination does not post to Trac (`saveForTrac` saves, then opens
the attach page); Forget vs
Delete match `site-registry.js`; the terminal's allowed set matches
`TERMINAL_ALLOWED_SCRIPTS`; the
Logs tabs are exactly Server and debug.log; the debug-constants table
matches
`wp-debug-constants.js` line for line including the REST/AJAX caveat;
512 KB matches
`MAX_LOG_CHARACTERS`; the mail catcher binds `127.0.0.1` on an
OS-assigned port; the 14-day amber
threshold, the red incomplete state and the local-only staleness
judgement match `update-plan.cjs`;
the provenance header and filename match `patch-provenance.cjs`; the 90
s Trac human-check and 120 s
server-start timeouts match their sources.

**Content-loss check:** every section removed from the README has a
covering page in the guide —
nothing was deleted rather than moved.
</details>
@juanmaguitar juanmaguitar mentioned this pull request Aug 10, 2026
Merged
juanmaguitar added a commit that referenced this pull request Aug 10, 2026
@juanmaguitar @claude
Document the site-holds-many-tickets model, and correct the guide pag…
ba9e7eb 
…es that predate it (#245)

> [!WARNING]
> **Do not merge this before the ticket-branches stack (#168 #185 #198
#205 #218 #238 #241 #242 #243).** It documents features that are not on
trunk yet. Merged first, the published guide describes an app nobody can
download.

## Why

The user guide describes a site as one working tree and one implicit
patch, because that is what the app was when the guide was written. The
ticket-branches stack makes a ticket a branch on the site, which leaves
several pages not merely incomplete but wrong: what a patch contains,
what **Unlink** costs you, what a trunk update does to work in progress,
whether the applied-patch banner describes the ticket you are looking
at.

The stack also adds surface nothing documents at all — the tickets card,
the question asked about edits made before a ticket was picked, the note
about unsubmitted work, the switch progress line, coloured pull request
states.

## What changes

Docs only. The diff is `docs/` — guide pages,
`docs/.vitepress/config.mjs` for the sidebar, and screenshots. No
`src/`, no `scripts/`.

**New page: `guide/ticket-branches.md`, "Working on several tickets".**
The model (a site is the expensive substrate, a ticket is a cheap
branch), starting a second ticket, the tickets card in both its states,
what a switch says while it runs and what to do when one fails part-way,
deleting one ticket's work, the four-choice question about loose trunk
edits, the unsubmitted-work note, and what a ticket's patch does and
does not contain. Added to the sidebar under "Contributing changes",
after "Working on a Trac ticket".

**Corrections to pages that describe the old model:**

| Page | What was wrong |
| --- | --- |
| `trac-tickets.md` | Unlink described as only forgetting an
association; no mention that a site holds many tickets, that linking
asks about loose edits, or that PR states are now coloured pills with a
third value |
| `submitting-changes.md` | "everything this site has that its copy of
trunk does not" — the diff is now the ticket's own work, measured from
its branch point. Also gained what a patch can and cannot carry:
deletions now travel, binaries and unreadable files are named above the
diff rather than dropped |
| `trunk-updates.md` | Said nothing about being on a ticket — that the
update parks and returns, and that the branch point deliberately does
not move |
| `applying-patches.md` | Applied-patch state is per ticket now; and
"use Update to latest trunk to reset the checkout" does not hold on a
ticket branch, where the update puts you back exactly where you were |
| `creating-a-site.md` | Implied a site per piece of work |
| `managing-sites.md` | **Delete this site** takes every ticket's work
with it; and its dirty-tree dialog is about loose edits only, never
parked ticket work |
| `troubleshooting.md` | New entry for the refused-switch message, which
is what someone will search for |
| `getting-started.md`, `setup-wizard.md`, `submit-*.md`,
`running-the-site.md` | The button is **Review & submit changes**; the
guide still quoted **Submit changes** everywhere |

**Deliberately not in this PR:** new entries in
`scripts/screenshots/shots.cjs` for the panels the stack adds. A shot
definition belongs with the code that draws the panel, and one added
here would fail on trunk where the panel does not exist — see Risks.

## How to test this

**Platforms:** any. This is a static site; nothing here touches the app.

**Starting state:** this branch checked out.

1. `npm run docs:build` → completes. This is the real check:
`ignoreDeadLinks: false`, so a broken link or a missing image fails it.
2. `npm run docs:dev` and open `/guide/ticket-branches` → the page
renders, the six screenshots load, and "Working on several tickets" is
in the sidebar under **Contributing changes**, between "Working on a
Trac ticket" and "Applying patches and PRs".
3. Follow the internal links out of that page —
`#what-a-patch-contains`, `#edits-you-made-before-picking-a-ticket`,
`#deleting-a-ticket-s-work`,
`trunk-updates#updating-while-you-are-on-a-ticket`,
`submitting-changes#what-a-patch-can-and-cannot-carry` → each lands on
the heading it names, not at the top of the page.
4. `npm run lint` → clean.

**And the part a build cannot check** — drive the stack (the Buildkite
artifact for `juanmaguitar/pr-state-colours` exercises all nine PRs)
with the new page open, and check the prose against the app:

5. On a site with two tickets, read "Starting a second ticket" and
follow it literally. → Unlink, then type the second number. The page
should not describe a control that is not on screen.
6. Read the switch progress sentences in the page against the ones the
panel actually shows.
7. Compare the six screenshots against the panels as they render for
you.

**What must not have happened:**

- **No `src/` or `scripts/` file in the diff.** `git diff --stat
origin/trunk...HEAD` must list only `docs/`. If a screenshot-harness
change slipped in, it will fail on trunk.
- **No invented UI string.** Every label quoted in these pages was read
out of the stack's source or seen in the app. A plausible-sounding
button that does not exist is worse than no documentation.
- **No page left describing the old model.** The table above is the
list; if another page still says a site carries one ticket, it was
missed.

## Risks and limitations

**This PR must merge after the stack.** It is the only real risk here
and it is not defended by anything automated: nothing in CI knows the
difference. The Pages deploy runs on trunk, so merging early publishes
it immediately.

- **`scripts/screenshots/shots.cjs` has no entries for the new panels**,
so the six new images are not reproducible with `npm run shots` as
things stand. They were taken by driving the stack through the same
Playwright harness in a throwaway worktree, with fixture and shot
definitions that were not committed anywhere. Follow-up, and it belongs
on a branch that has the panels: the fixtures need real repositories
with ticket branches, where the current ones are empty directories.
**Follow-up issue not yet filed** — happy to file it.
- **`site-view.png` is re-shot** and is the only existing image
replaced. Its button still read **Submit changes**, which was already
stale on trunk after #235 renamed it — so that image and the text
corrections around it are true of trunk today, stack or no stack.
- **The `MERGED` pill is documented but not screenshotted.** A
repository-wide search returns no merged pull requests on
`wordpress-develop`; the state is real in the API and unit-tested, and
#243 says the same.
- **The screenshots name a real ticket and its real pull requests**
(#29798), pulled live from GitHub when the images were taken. They will
age the way any screenshot of live data ages.
- Review outcome: **5 [fix here] · 1 [follow-up] — all 6 addressed.**
Every one was a factual claim about the app, not prose. Details in the
collapsed block.

## Related

Documents #168, #185, #198, #205, #218, #238, #241, #242, #243 — the
ticket-branches stack, tip branch `juanmaguitar/pr-state-colours`. Part
of the contribution-flow tracker #110. Follows the docs site (#230), its
screenshot harness (#231) and the guide itself (#232).

---

<details>
<summary>Design decisions and alternatives considered</summary>

**A new page rather than growing `trac-tickets.md`.** That page is about
one ticket: linking it, and reading the work that already exists on it.
Several tickets is a different subject with its own vocabulary —
parking, switching, deleting a branch — and folding it in would have
doubled the length of a page a first-time contributor reads on their
first ticket, to describe something they do not do yet.
`trac-tickets.md` gains pointers instead.

**Corrections before additions.** A page that is wrong costs more than a
page that is missing: someone acts on it. So the pass over the existing
guide came first, and the new page was written to fill what was left.

**Screenshots taken by hand rather than by adding shot definitions.**
The harness lives on trunk and the panels live on the stack, so a shot
definition committed here would fail every `npm run shots` on trunk
until the stack lands. The images were taken by assembling the two in a
scratch worktree — stack tip, plus trunk's `scripts/screenshots/` and
the `TOOLKIT_USER_DATA_DIR` hook, plus fixtures that build real
repositories with ticket branches — and only the resulting PNGs were
copied here. That worktree is gone; nothing of it is in this diff.

**Quoting the app rather than paraphrasing it.** Where a sentence in the
app is the thing being explained — the question about loose edits, the
switch stages, the confirm before a delete — the page quotes it
verbatim, so someone reading with the app open can match what they see.
Every quote was read out of the stack's source.

</details>

<details>
<summary>Review outcome (5 [fix here] · 1 [follow-up] — all
addressed)</summary>

Run per `.github/instructions/code-review.instructions.md`.
Deterministic layer inline and clean — `npm run lint` clean, `npm test`
638/638, `npm run docs:build` clean. The judgement pass went to a
subagent with the diff, the instructions file and the stack's source,
and nothing from the session that wrote the pages; it was asked to check
every quoted string and every behavioural claim against the code rather
than to read the prose.

Every finding was an accuracy finding — the guide asserting something
the app does not do. All five were verified against the source before
being fixed.

| # | What was wrong |
| --- | --- |
| 1 🔴 | The new page's central how-to. "Type the number into the **Trac
ticket** panel" — the panel holds no input while a ticket is linked; the
field is in the unlinked branch only. The route is **Unlink**, then link
the new number. Unfollowable as written. |
| 2 🟡 | The mid-switch advice described a recovery that does not exist:
the marker is written only when a checkout throws inside a running app,
so a force-quit leaves nothing behind, and `midSwitchBlock` refuses the
retry along with everything else. The one action it allows is
**Unlink**, which is now what the page and `troubleshooting.md` say. |
| 3 🟡 | `applying-patches.md`'s "use **Update to latest trunk** to reset
the checkout" escape hatch. On a ticket the update parks, resets trunk
and checks the branch back out — applied patch included — so it leaves
you where you started. |
| 4 🟡 | "Nothing is rebuilt" was true and misleading: a switch does not
rebuild, so a running dev server keeps serving the previous ticket's
assets. The page now says to run `npm run build` after a switch, which
is what the site view itself says. |
| 5 🔵 | "If the note says two changes, the patch has two files." The
note counts everything the patch speaks about, binaries included, and
those are named above the diff rather than carried — so one text file
and one image is two in the note and one in the diff. It also
contradicted this PR's own `submitting-changes.md`. |
| 6 🔵 | The by-hand replay recipe skipped the trunk update that gives
the new branch a newer base, and offered **Delete this ticket's work**
for the ticket you are on, which the card never lists. |

**`[follow-up]`, both taken here rather than deferred** because they
were two sentences each: `managing-sites.md` and `trunk-updates.md`
described the same dirty-tree dialog differently, and
`troubleshooting.md` had no entry for the refused-switch message.

Style notes, also applied: the tickets card is additionally gated on the
setup checklist being finished or skipped, and the loose-edits question
can be raised from the **Attach to Trac** card's link field too.

The pass also confirmed what was most likely to be wrong and was not:
every other quoted label and message is verbatim, all six screenshots
match their alt text, `#deleting-a-ticket-s-work` resolves under
VitePress's slugifier, and the new page is in the sidebar.

</details>

<details>
<summary>Implementation notes</summary>

**How the screenshots were taken.** `git worktree add` from the stack
tip, then `git checkout <trunk> -- scripts/screenshots docs/…`,
`playwright-core` installed, and the `TOOLKIT_USER_DATA_DIR` hook from
`src/main.js` re-applied — the stack predates all of it, so `npm run
shots` does not exist on that branch. Two throwaway fixture variants
were added, both building real repositories through the app's own
`src/ticket-branches.js` (`startTicketBranch`, `parkCurrentWork`,
`switchToBranch`) so the state is the state the app makes: one site on
`ticket/29798` with parked work and a second branch, one parked back on
trunk with a loose edit. Same 1200×800 window and
`--force-device-scale-factor=1` as `capture.cjs`, so the new images sit
beside the existing ones without a size jump. The worktree has been
removed.

**The new slugs**, none of which `shots.cjs` knows about:
`ticket-list-card`, `ticket-list-unlinked`, `trunk-work-question`,
`carried-work-notice`, `linked-pull-requests`, `site-with-tickets`.

**`linked-pull-requests.png` is one image doing three jobs** — the
coloured state pills (#243), the unsubmitted-changes note (#241), and
the reorganised ticket card with the tickets list no longer inside it
(#242).

**`docs/` is its own npm package**, so `npm run docs:build` needs `npm
--prefix docs ci` first on a fresh checkout.

</details>

<details>
<summary>Screenshots or recording</summary>

Six new images, and one replaced.

**The tickets card, linked** — `ticket-list-card.png`:

![Other tickets on this
site](https://raw.githubusercontent.com/WordPress/contributor-toolkit/juanmaguitar/docs-ticket-branches/docs/public/screenshots/ticket-list-card.png)

**The tickets card, unlinked** — `ticket-list-unlinked.png`:

![Your tickets on this
site](https://raw.githubusercontent.com/WordPress/contributor-toolkit/juanmaguitar/docs-ticket-branches/docs/public/screenshots/ticket-list-unlinked.png)

**The question about loose trunk edits** — `trunk-work-question.png`:

![You have 1 uncommitted change on this site, not on any ticket yet.
What should happen to
them?](https://raw.githubusercontent.com/WordPress/contributor-toolkit/juanmaguitar/docs-ticket-branches/docs/public/screenshots/trunk-work-question.png)

**After choosing to carry them** — `carried-work-notice.png`:

![Your 1 uncommitted change came along into #62281, and will go into its
patch.](https://raw.githubusercontent.com/WordPress/contributor-toolkit/juanmaguitar/docs-ticket-branches/docs/public/screenshots/carried-work-notice.png)

**The ticket card with coloured pull request states and the
unsubmitted-work note** — `linked-pull-requests.png`:

![A red CLOSED pill and a green OPEN pill on the two linked pull
requests](https://raw.githubusercontent.com/WordPress/contributor-toolkit/juanmaguitar/docs-ticket-branches/docs/public/screenshots/linked-pull-requests.png)

**The three cards in their new order** — `site-with-tickets.png`:

![Trac ticket, then Other tickets on this site, then Apply a patch or
PR](https://raw.githubusercontent.com/WordPress/contributor-toolkit/juanmaguitar/docs-ticket-branches/docs/public/screenshots/site-with-tickets.png)

**Replaced:** `site-view.png`, whose action bar read **Submit changes**.
No stack behaviour is visible in it — it is a straight re-take of a
stale image.

</details>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@juanmaguitar