Skip to content

Write the user guide and point the README at it - #232

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

Write the user guide and point the README at it#232
juanmaguitar merged 1 commit into
trunkfrom
docs/guide

Conversation

@juanmaguitar

@juanmaguitar juanmaguitar commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

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.
Two things the code corrected while writing

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.

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.

Self-review — 8 findings, all fixed; three of them were false statements about the app

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.

This was referenced Aug 10, 2026
Merged
Merged
@juanmaguitar
juanmaguitar force-pushed the docs/screenshots branch 2 times, most recently from aec819b to 17c75c8 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 added a commit that referenced this pull request Aug 10, 2026
@juanmaguitar
Add a screenshot harness that photographs the app for the docs site (#…
e2cd912 
…231)

## 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.

<details>
<summary>Why these specific choices</summary>

**`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.
</details>

## 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.

---

<details>
<summary><b>Self-review</b> — 2 findings fixed, 1 follow-up taken
anyway, security guard verified</summary>

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.
</details>
@juanmaguitar
Write the user guide and point the README at it
3867e49 
Eighteen pages covering every screen the app has: 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, and troubleshooting.

Each page was written against the renderer rather than from memory, so it names
controls by the words actually on screen. Two things the code corrected along
the way: the Logs panel has two tabs, not three (npm output goes to the
Terminal), and the Trac destination never posts anything — it saves the diff
and opens the ticket's attach page, and you upload it yourself.

The README keeps what a contributor to this repository needs and hands the
rest over: the platform-by-platform install, the Gatekeeper notes, the
ten-step walkthrough and the trunk-update section move into the guide rather
than being copied there. AGENTS.md asks for one home per fact, and a README
that restates the guide is the drift this rule exists to prevent.

The eleven fixture-tier screenshots come from `npm run shots`. Screens needing
a live server are not illustrated yet; the live tier exists for them.
@juanmaguitar
juanmaguitar changed the base branch from docs/screenshots to trunk August 10, 2026 13:21
@juanmaguitar
juanmaguitar merged commit e0be741 into trunk Aug 10, 2026
5 checks passed
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