Skip to content

Add a settings dialog, with where new sites go and who the contributor is - #638

Merged
zaerl merged 3 commits into
trunkfrom
feat/settings-dialog
Oct 6, 2026
Merged

zaerl merged 3 commits into
trunkfrom
feat/settings-dialog

Conversation

@zaerl

@zaerl zaerl commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Why

First part of #559. The app has had two app-wide settings for a while, the contributor's WordPress.org username and the event they are at, asked once in the mentor handoff and kept in the store's preferences; there was no place to see or change them, and nowhere to put the settings the prototype has (#542). This adds the dialog and its first two tabs, with one new setting, so that the next ones (language, the PHP version and debug flags, what starts when a site opens and what happens when you quit, the theme of #560) each arrive as one small change to it.

What changes

A Settings dialog, opened from a cog in the footer beside "Give feedback", and from the menu: Settings… in the application menu on macOS (⌘,) and under File on Windows and Linux (Ctrl+,). The menu brings the main window forward first, as a deep link does; with no window open it opens one and nothing more, since the page is not yet listening.

General: New sites go here. The folder new sites are made in. With one set, the create-site dialog starts on it, and another can still be chosen for one site without changing the setting. A folder that is not there is refused when it is chosen, and Forget this folder clears it.

Account: How you contribute. The WordPress.org username and the event, through the same answers the mentor handoff uses, so the two cannot disagree; a pasted profile link is kept as the username it names, as it is there. And GitHub: which account the app opens pull requests with, and a way to sign out of it. Signing in stays where it is needed, in the review dialog.

The model. src/settings.cjs says what each setting is and what it accepts, pure, with the disk's answer handed to it. Main writes one setting at a time and answers with all of them as they then stand, so the window shows what was kept and not what it sent; the disk is asked only about a full path for a setting there is, and off the main process.

Beside it:

  • The folder field the create-site dialog asked with is a component of its own, since the settings ask with it too.
  • The refusals of the username and event parsers are translated. The dialog is the first screen on which they are seen in the pseudo-locale, so the journey found them.
  • A guide page, Settings, under Reference, and a sentence on the create-site page.

Not in this pull request:

  • Every other setting of the prototype's dialog: the language (next), the PHP version and the two debug flags, starting the server or the build watch when a site opens, what happens to running servers on quit, the theme (Add dark mode #560), a default editor and terminal, the admin login. Nothing here promises any of them.
  • Signing in to GitHub from the dialog. The device flow lives in the review dialog and is used there; the tab shows the account and signs out.
  • Dark mode: the dialog is painted with the design system's tokens, like the rest of the window, and will follow Add dark mode #560.

How to test this

Platforms: any. The journeys drive this on macOS and Windows. From the repository root, after npm ci and npm run build:once:

npx playwright test --project=journeys settings.spec i18n.spec create-site.spec
  • settings.spec.js: the cog opens the dialog on General; a folder chosen is shown, kept in the store and offered to be forgotten; a folder that is not there is refused with main's words and the kept one stays; the create-site dialog starts on the folder and still lets another be chosen for one site without changing the setting; the menu's Settings… opens the same dialog; forgetting the folder puts both dialogs back. On Account: a username that is not one is refused with the handoff's words; a profile link is kept as the username, the event with it, and Enter saves as the button does; an emptied event is forgotten; the GitHub account is named and signing out asks main and says so; reopened, the dialog is on General and shows what was kept.
  • i18n.spec.js: both tabs and a refusal in the pseudo-locale, and the menu item's label.
  • create-site.spec.js: the dialog with no folder set, as before.

I broke four of the settings journey's claims one at a time (the create-site dialog not given the folder; main not asking the disk; the menu's request not reaching the dialog; the username not shown as main kept it) and each turned it red. tests/unit/settings.test.cjs, settings-view.test.cjs, menu.test.cjs and the settings:* and menu tests in ipc-wiring.test.cjs hold the decisions, and the menu's window test is red on the first version of the handler.

Worth a look by hand (any platform, the current head): press the cog, or ⌘, / Ctrl+,, with the window minimised or behind a patch window: the window comes forward with the dialog open. Choose a folder, press Create new site: the Location is that folder. On Account, type your username and press Save; open Review & submit changes on a site with an edit and look at Hand it to a mentor: it offers to save the patch as that username.

What must not have happened: a folder chosen for one site changing the setting; the handoff and the dialog disagreeing about the username; the setting written when main refused the folder; the dialog opening in a patch window.

What I could not test: Windows and Linux, where the item is under File with Ctrl+,; the journeys run on Windows in CI and click the item by its id, which does not say where it is. And the small-window and slowed-status variants of the journeys: the dialog has no layout assertion to be sensitive to them.

Risks and limitations

  • Review: 2 passes, 11 findings fixed here, 3 left as follow-ups. Details below.
  • A remembered folder deleted after it was chosen is recreated silently by the setup, which ensureDirs the folder it is given. That was so before; the setting makes it easier to reach. Follow-up: have the setup refuse a folder that is not there.
  • Signing out here while a review dialog is open leaves that dialog saying "Signed in as …" until it is reopened, when it re-reads the account.
  • An unsaved Account draft is lost on a tab switch, since the design system's tabs unmount the panel that is not shown.
  • The window never learns of a setting changed in the store by something other than itself; nothing else writes one.

Related

Part of #559, which is part of #542.


Design decisions and alternatives considered
  • preferences in the one store, not the renderer's localStorage the prototype uses. Half of the settings to come are consumed in the main process (the PHP version at server start, the quit behaviour, the locale before a window exists), and the two that exist already live there.
  • One pure module for what a setting is, and one handler for one setting at a time. A value is checked at the IPC boundary as the username is, because the window is not the only thing that writes the store; the answer is the whole, so a canonical form chosen by main is what the window shows.
  • Global, not per-site. The prototype's "New sites" tab suggests values copied into a site at creation; here a setting is read when it is used. A per-site override can go in siteMeta later if one is wanted.
  • The username and the event through the handoff's own answers, not a second pair of fields writing the same keys: the handoff asks for them in the moment they are needed, the dialog is where they are changed afterwards, and both hold the one state.
  • GitHub shown, not signed in to, here. The device flow and the wait for its outcome live in the review dialog, where the account is used. A second copy for the settings would be a second flow to keep right.
  • The menu item written out beside the role's defaults. Giving a role a submenu replaces its items, so the app menu and the File menu are written item by item as Electron has them, with the one addition; the menu test pins the list.
  • The theme row left to Add dark mode #560. The design system makes dark cheap at the token level, but the older components still in the window ship light styles only, and a theme row that half works is what color-scheme.test.cjs exists to prevent.
Review outcome (required — see AGENTS.md)

Pass 1: 9 [fix here] · 2 [follow-up]. Pass 2 (on the fix-up): 2 [fix here] · 1 [follow-up]. All 11 fixed, 3 left as follow-ups.

  • Review: completed — a separate agent context, read-only; head a58fb4e / base 0cf005f; the store discipline of the two handlers read against Overlapping writes to a site's metadata can drop a ticket branch's patch base #172, the validation at the boundary, the menu against Electron's default roles, every string checked for render-time wrapping, the pot generation run, the journey read against the standard's rules; ESLint and the unit files run, Playwright not; 9 [fix here], 2 [follow-up].
    • Fixed: the menu opened the dialog in whichever window Electron listed first and never brought the main window forward; the folder was stat'd synchronously on the main process; the i18n test's wait could pass before the Account tab had rendered; the folder field said "Not set" before the settings had been read; the folder was trimmed before it was stored; the guide promised settings that are not there; the General tab's heading pointed at settings that are not there; two claims in the model's header; the create-site page did not say where its folder now starts.
    • Follow-ups left: a remembered folder deleted since is recreated by the setup; signing out while a review dialog is open.
  • Review: completed — a second separate agent context, read-only; head fc9b8b3 / base 0cf005f; the fix-up's yield checked against the store rule, the window choice, the untrimmed path through the create flow, the .or() wait; ESLint and the unit files run; 2 [fix here], 1 [follow-up].
    • Fixed: the handler stat'd any string for any key before the pure check could refuse it; the corrected window choice had no test.
    • Follow-up left: the menu's send is outside a try, where the deep link's is inside one; no realistic way to reach a throw was found.
  • Since review: fc9b8b3 → 0bee55b is those two fixes, not reviewed again. On 0bee55b: npm run lint, npm test (1975), the three journeys above and the packaged smoke pass. On fc9b8b3: npm run test:electron (1973) and the docs build pass; on a58fb4e: all 117 journeys.
Implementation notes
  • New: src/settings.cjs, src/renderer/settings-view.cjs, src/renderer/hooks/use-settings.jsx, src/renderer/components/settings-dialog.jsx, src/renderer/components/folder-field.jsx, docs/guide/settings.md, tests/unit/settings.test.cjs, tests/unit/settings-view.test.cjs, tests/e2e/journeys/settings.spec.js.
  • src/main.js: settings:get, settings:set, isDirectory, openSettingsFromMenu. src/preload.js: getSettings, setSetting, subscribeSettingsOpen. src/menu.js: the item and the two menus written out.
  • src/wporg-handle.cjs, src/patch-provenance.cjs: refusals through __() at the moment they are given.
  • tests/unit/ipc-wiring.test.cjs: settings:* wired, and the menu's window choice through the harness's recorded menu and windows.
Screenshots or recording

Not attached: I have no way to upload images from the command line. The journey above drives everything that changed, and the guide page says what each tab holds.

🤖 Generated with Claude Code

zaerl and others added 3 commits October 6, 2026 10:49
…r is

The first of the settings (#559): a dialog on tabs, opened from a cog
in the footer beside "Give feedback" and from the menu's "Settings…",
in the application menu on macOS and under File elsewhere, with each
platform's shortcut.

The settings are app-wide, under the store's `preferences` beside the
contributor's WordPress.org username and event, and for the same reason:
a setting is about the person or the machine, not a checkout. What each
is and what it accepts is one pure module, `src/settings.cjs`; main
writes one at a time and answers with all of them as they then stand, so
the window shows what was kept and not what it sent.

General holds the folder new sites go in. With one set, the create-site
dialog starts on it, and another can still be chosen for one site; a
folder that is not there is refused. Account holds the username and the
event through the answers the mentor handoff already uses, so the two
cannot disagree, and the GitHub account the app acts for, which can be
signed out of here. The refusals the username and event parsers give are
now translated: the dialog is the first screen on which they are seen in
the pseudo-locale.

The folder field the create-site dialog asked with is a component of its
own, since the settings ask with it too.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…og gave it

From the review. The menu's "Settings…" opened the dialog in whichever
window came first, a patch window included, and never brought the main
window forward; it now does what a deep link does. The folder is asked of
the disk before the pure check and without holding the main process, and
is stored as the system's dialog gave it: trimmed, a name that ends in a
space would have been another folder's. Until the settings are read the
folder field says so, where it said none was set. The General tab's
heading names what is under it, the guide promises no setting that is
not there, and the create-site page says where its folder now starts.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…the menu reaches

From the second review. The handler asked the disk about any string for
any key before the pure check could refuse it; now only a full path for
a setting there is. The menu's choice of window, which the first review
corrected, has the wiring test it lacked: the main window gets the
request and is brought forward, a patch window listed first does not,
and with the main window closed one is opened and sent nothing.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 6, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository: WordPress/contributor-toolkit/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 5ac58801-dd88-419a-8a4c-b5a0e3001e12

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@zaerl
zaerl merged commit fe02e6b into trunk Oct 6, 2026
10 checks passed
@zaerl
zaerl deleted the feat/settings-dialog branch October 6, 2026 09:28
zaerl added a commit that referenced this pull request Oct 6, 2026
## Why

Second part of #559, on the dialog of #638. The app is translatable
(#540) and picks its language from the operating system's; a contributor
on a machine set to a language they do not read, or at a shared laptop,
had no way to choose. This adds the language to the settings.

## What changes

**Language, on the General tab.** Your system's language, which is the
default, or English, or any language the build ships a catalog for, each
named in its own language ("Deutsch", "Português (Brasil)") so that
someone looking for theirs finds it whatever the app is showing. The
list is what `src/languages/` holds, so it grows as releases ship
catalogs; today that is English.

**Where it is read.** Main puts the chosen language in front of the OS's
languages and loads the first one with a catalog, as it walked the OS's
before. In front, not in their place: a chosen language whose catalog a
later release dropped falls back to the OS's language, not to English.
The `--lang` switch still replaces the whole list; it is how the
journeys pick a locale, and a flag typed at launch is a decision.

**A change applies after a relaunch**, since main applies the catalog as
it starts, for its own strings and the window's. The dialog says so once
what is set is no longer what the window started in, and offers
**Relaunch now**: a quit and a start, so the quit sweep ends every child
the app started, as it does on any quit. The new instance gets this
one's arguments less a `wpct://` address a cold start was given, which
is not a second request for its ticket, and a `--lang`, which would
outrank the language just chosen; on Linux it is started from the
AppImage, since the mounted copy is gone once this one quits.

**The first read of the store is now before the window exists.** One
that cannot be read is logged and counts as no choice, so the window
still opens; every later read then fails as it did before.

**Beside it:** the catalogs' README and CONTRIBUTING.md say where the
language comes from now; a journey can launch without `--lang`, as the
app launches for a contributor.

**Not in this pull request:**

- Switching the language without a relaunch. Main applies the catalog
once, at startup, for its own strings; the window would need a second
path, and the menu and the native dialogs a third.
- The slugs no operating system reports (`pirate`, `art-xemoji`), which
are not offered, as they could not be picked from the OS list either.
- The rest of #559: the PHP version and the debug flags, what starts
when a site opens and what happens on quit, the theme (#560).

## How to test this

Platforms: any. The journeys drive this on macOS and Windows. From the
repository root, after `npm ci` and `npm run build:once`:

```
npx playwright test --project=journeys settings.spec i18n.spec
```

- `settings.spec.js`, the language journey: the pseudo-locale set in the
store, with no `--lang`, is the language the app starts in; the control
shows it though the build has no catalog for it; English chosen is kept
at once and the dialog offers the relaunch, which asks main; the
system's language chosen is kept as none and the offer stays; the
pseudo-locale chosen again is refused, since the build has no catalog
for it; started again with English kept, the app is in English and the
control says so with no offer.
- `i18n.spec.js`: the dialog with the control, in the pseudo-locale.

I broke two of the journey's claims one at a time (main not reading the
chosen language; the relaunch button asking nothing of main) and each
turned it red. `tests/unit/i18n.test.cjs` holds the list,
`settings.test.cjs` what is accepted, `settings-view.test.cjs` the
control's entries and when the relaunch is offered, and
`ipc-wiring.test.cjs` the order the languages are tried in, the store
that cannot be read, and what the relaunch is given.

**Worth a look by hand** (any platform, the current head): Settings →
General → Language: with no catalog shipped the list is your system's
language and English. Choose English, press **Relaunch now**: the app
quits and comes back, the control says English and offers nothing.
Choose your system's language, press the cog's relaunch again: as
before. With `npm run i18n:download -- --locales=de` run first (it needs
the network), "Deutsch" is in the list, and chosen and relaunched, the
app is in German where it is translated.

**What must not have happened:** a running server or build left behind
by the relaunch (the quit sweep ends them, as on any quit); a `wpct://`
ticket opened again after a relaunch of an app that was started from a
link (Windows and Linux); the app failing to open a window when
`settings.json` is not JSON.

**What I could not test:** Linux, where the relaunch is started from the
AppImage; the unit test pins what Electron is told, not that the image
starts. And a real catalog in the dialog: the repository ships none, and
I did not run the download.

## Risks and limitations

- Review: 2 passes, 4 findings fixed here, 1 follow-up taken here.
Details below.
- **The language names are not translated**, by design: each is in its
own language.
- **A store that cannot be read** lets the window open but every later
read fails as it did before this change; the app does not say which file
is at fault.
- The relaunch is a quit: nothing asks first, as nothing asks on ⌘Q.

## Related

Part of #559, which is part of #542. Follows
#638. Translations:
#540.

---

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

- **A relaunch, not a live switch.** Main applies the locale once, at
startup, for the menu, the native dialogs and the sentences it sends,
and the window reads its catalog before its first render; three paths to
re-run for a setting changed a few times in the app's life. The dialog
says what the relaunch costs.
- **The chosen language in front of the OS's, not in their place.** A
release drops a catalog that fell under the cut-off; the contributor who
chose it should get their OS's language, as they did before choosing.
- **The control's entries with their tags as values**, drawn by the
dialog, and the list also handed over for the names. Handed the list
alone, the design system's select takes each entry as its own value and
tells them apart by identity, which a value kept in the store cannot
match.
- **Entries made from the language the window started in**, not the one
set, so that a choice does not change them: an entry taken away under
the control while it is choosing is reported as a second choice, of
none. A language the window started in that the build has no catalog for
is listed, so the control shows what is set, and refused if chosen
again.
- **The quit sweep over `app.exit`.** The relaunch goes through
`app.quit()` so every child the app started is ended first.

</details>

<details>
<summary>Review outcome (required — see AGENTS.md)</summary>

**Pass 1: 3 [fix here] · 0 [follow-up] · 4 style notes. Pass 2 (on the
fix-up): 1 [fix here] · 1 [follow-up] · 1 style note. All 4 fixed, the
follow-up taken here, every style note applied.**

- **Review:** completed — a separate agent context, read-only; head
`82aac18` / base `fe02e6b`; the store read traced from `whenReady`, the
relaunch against Electron's quit events and the single-instance lock,
the language names checked on ICU for lowercase tags and Valencian, the
select's value handling read in the design system's source, the journey
read against the standard; ESLint, the unit files and the pot extraction
run; 3 [fix here].
- Fixed: a store that could not be read would have stopped the window
opening, since the locale is resolved before it; a chosen language
replaced the OS list, so one whose catalog was dropped gave English
rather than the OS's language; the relaunch reused the launch's
arguments, a `wpct://` address and a `--lang` included.
- Style notes, applied: a stale comment in the store module; a silent
catch on the relaunch; the system entry's mapping as a tested function;
the catalogs' README counting entries not offered; a journey comment.
- **Review:** completed — a second separate agent context, read-only;
head `7eab0b6` / base `fe02e6b`; the fallback order followed through
`resolveCatalog` for each case, the relaunch arguments against
Electron's documented default and a dev launch, the three changed wiring
tests confirmed red on the first commit; ESLint and the unit files run;
1 [fix here], 1 [follow-up].
- Fixed: the README still described the chosen language as used instead
of the OS's rather than before them.
- Follow-up, taken here: on Linux the relaunch would start from the
AppImage's mount, which is gone once the app quits; the new instance is
started from the image.
  - Style note, applied: one comment line left unwrapped.
- **Since review:** `7eab0b6 → b68f7c2` is that finding, the follow-up
and the rewrap, not reviewed again. On the head: `npm run lint`, `npm
test`, `npm run test:electron`, the settings, i18n and create-site
journeys and the packaged smoke pass; all 118 journeys passed on
`82aac18`.

</details>

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

- `src/i18n.cjs`: `languageChoices(names)`. `src/settings.cjs`:
`locale`. `src/main.js`: `localeReply` reads the store, `languages()`,
`i18n:languages`, `app:relaunch`, `relaunchArgs`. `src/preload.js`:
`listLanguages`, `relaunch`.
- `src/renderer/settings-view.cjs`: `languageItems`, `languageValue`,
`languageChanged`, `SYSTEM_LANGUAGE`.
`src/renderer/components/settings-dialog.jsx`: `LanguageControl`.
`src/renderer/hooks/use-settings.jsx`: `loaded`.
- `tests/e2e/helpers/app.cjs`: `lang: false` drops the `--lang` switch.

</details>

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

Not attached: I have no way to upload images from the command line. The
language journey drives everything that changed.

</details>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
zaerl added a commit that referenced this pull request Oct 6, 2026
…he next server start (#640)

## Why

Third part of #559, on the dialog of #638. Every site's development
server ran on the PHP the bundled Playground chooses and with
WordPress's debug constants fixed on; a contributor whose ticket is
about PHP 7.4, or about what a site does with `WP_DEBUG` off, had no way
to see it. This adds the PHP version and the two debug constants that
can be turned off to the settings, and shows what a server will start
with in the open site's details.

## What changes

**A Sites tab: Development server.** The PHP version, one of the seven
the bundled Playground has (7.4 to 8.5), 8.3 unless another is chosen;
and two switches, **Report notices and deprecations (WP_DEBUG)** and
**Use unminified scripts (SCRIPT_DEBUG)**. The rest of the constants a
server is booted with (the log, the display, the disabled fatal handler
and updater) stay as they were and are not anyone's to change.

**Applied at the next start.** Main reads the three at every server
start and hands them to the runner in its config; a server that is
running keeps what it started with until it is stopped and started, and
the tab says so. A version a release has taken out of the bundle is
passed over for 8.3, with a line in the app log, and the control shows
8.3 as chosen with a note saying why.

**The details say what a server will start with**, as the prototype has
them: "WordPress Core · PHP 8.3" on the Checkout row, and a Debugging
row with the constants that are on, or "Off".

**What WP_DEBUG off does, said for what it is.** It takes away notices
and deprecations. Warnings and errors still reach `debug.log` and the
browser, and `error_log()` calls still reach `debug.log`, because
Playground's own PHP keeps logging and display on whatever WP_DEBUG
says. The switch, the guide and the constants' module say so.

**The guide's pictures are retaken.** The details changed here, and the
footer's cog from #638 had never been retaken. The three pictures of a
site's page are taken in a window 900 high, so the details' Build watch
section is whole.

**Not in this pull request:**

- The PHP version a *running* server is on, in the Server section, where
the details show the version the next start is given. Follow-up.
- Per-site values: these are global, like the other settings. A per-site
override can go in `siteMeta` if one is ever wanted.
- The remaining constants (`WP_DEBUG_LOG`, `WP_DEBUG_DISPLAY`,
`WP_DISABLE_FATAL_ERROR_HANDLER`, `AUTOMATIC_UPDATER_DISABLED`), on
purpose: `src/wp-debug-constants.js` says why each is on.
- The rest of #559: what starts when a site opens and what happens on
quit; the theme (#560).

## How to test this

Platforms: any for the dialog and the details; macOS for the real
server, below. The journeys drive the dialog and the details on macOS
and Windows. From the repository root, after `npm ci` and `npm run
build:once`:

```
npx playwright test --project=journeys settings.spec site-header.spec i18n.spec
```

- `settings.spec.js`, the Sites journey: the fallbacks (8.3, both
constants on); 8.4 chosen is kept, WP_DEBUG off is kept and SCRIPT_DEBUG
left as it was; the open site's details then say "WordPress Core · PHP
8.4" and "SCRIPT_DEBUG"; restarted, the tab shows what was kept.
- `site-header.spec.js`: the details name the project with the PHP a
server starts on and which constants are on.
- `i18n.spec.js`: the tab in the pseudo-locale.

I broke two claims one at a time (main not reading the settings at a
start; the switch not writing) and each was caught, the first by
`ipc-wiring.test.cjs`, the second by the journey.
`tests/unit/runner-wiring.test.cjs` proves the runner hands Playground
the version and turns the two constants off in the blueprint;
`wp-debug-constants.test.cjs` that nothing else in the set moves;
`settings.test.cjs` and `settings-view.test.cjs` what is accepted and
what the control shows.

**The real server, run by me on this Mac (Apple Silicon, the current
head), as TESTING.md asks for a change to `src/server-runner.js`.** The
app was driven through Playwright with a throwaway profile listing a
built `wordpress-develop` checkout; **Start development server** was
pressed; the served WordPress was logged in to with the app's own
credentials and asked, on Site Health's Info tab, which PHP it runs on:

- PHP version set to **8.4**: the server came up, Site Health reported
**PHP 8.4.25**, wp-admin served, Stop ended it.
- PHP version left at the fallback, **8.3**: Site Health reported **PHP
8.3.33**.

**Worth a look by hand** (any platform, the current head): Settings →
Sites, choose 7.4, close: the details say "PHP 7.4". Start the server:
wp-admin → Tools → Site Health → Info → Server says PHP 7.4. Turn
WP_DEBUG off, stop and start the server: a page with a deprecated call
shows nothing for it, and a page that calls a missing function still
shows the fatal.

**What must not have happened:** a running server changing PHP or
constants under the contributor; a change applied to a site other than
the next start; `WP_DEBUG_LOG` or the fatal handler's constant moving
with the switches; the details saying a version the server will not
start on.

**What I could not test:** Windows, and a real server with WP_DEBUG off
(I checked what the code sends, not a page's output).

## Risks and limitations

- Review: 2 passes, 8 findings fixed here, 1 follow-up taken here (the
real boot), 2 left. Details below.
- **The details show the PHP of the next start, not of a server already
running.** Change the version while a server runs and "PHP 8.4" sits
above "Server running" on 8.3. Follow-up: the Server section should name
the running server's PHP.
- **Main requires `@php-wasm/universal`** for the list of versions. It
is not a declared dependency; it is `@wp-playground/cli`'s, at whatever
version that pins, and the comment says so. Declaring it pinned is a
`package.json` decision I have left to the maintainer.
- **The Electron unit suite failed one test once** in a run of 2004 and
passed on three runs after; I could not see which test it was the first
time.
- A `WP_DEBUG` switch that is off still leaves `WP_DEBUG_LOG` on, by
design, so the debug.log tab keeps showing warnings and errors.

## Related

Part of #559, which is part of #542. Follows
#639.

---

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

- **Global, applied at the next start, and read by main at each start.**
The prototype's "New sites" tab suggests values copied into a site at
creation. Here the version and the constants are what the server is
booted with, so reading them at each start is simpler and also reaches
existing sites.
- **An explicit fallback, always passed.** The CLI's own default is 8.3
today; passing it ourselves keeps the behaviour stable across a
Playground bump, and a stored version the bump took out falls back to it
rather than to whatever the CLI then chooses.
- **The list from the bundle, not written down.** `SupportedPHPVersions`
from `@php-wasm/universal` is what the runner can serve; a list of our
own would drift.
- **Only two constants have a switch.** The others exist to make a fatal
show as itself, the log exist for the tab and the updater stay out of a
checkout; a switch for each would be one more thing to get wrong at a
Contributor Day.
- **One decision for which version is shown and used**,
`phpVersionChoice` in `settings-view.cjs`: the dialog, the details and
the start handler all agree on the fallback.

</details>

<details>
<summary>Review outcome (required — see AGENTS.md)</summary>

**Pass 1: 3 [fix here] · 3 [follow-up]. Pass 2 (on the fix-up and the
details): 5 [fix here] · 1 [follow-up]. All 8 fixed, the real-boot
follow-up taken here, 2 left.**

- **Review:** completed — a separate agent context, read-only; head
`9131ed7` / base `77751a1`; the runner's options checked against the
CLI's `runCLI` signature and its persistent `start` command, WordPress's
`wp_debug_mode()` and Playground's php.ini and mu-plugin read for what
WP_DEBUG off does, the design system's switch and toggle props
confirmed; ESLint and the unit files run; 3 [fix here], 3 [follow-up].
- Fixed: WP_DEBUG off was said to empty the log and the browser of
errors, where it takes away notices and deprecations; which versions the
control offers was decided in the component; the real boot TESTING.md
asks for was not recorded.
- Follow-ups: the undeclared require (a comment, left to the maintainer
to declare); a stored version the bundle no longer has was shown as
chosen but not used (taken here); the journey pins 8.4 (left).
- **Review:** completed — a second separate agent context, read-only;
head `ca84255` / base `77751a1`; the four rewordings checked against
WordPress and Playground again, the details' new rows and their
journeys, the retaken pictures opened against their alt texts; ESLint
and the unit files run; 5 [fix here], 1 [follow-up].
- Fixed: the details showed the version set where the dialog and the
start used the fallback; `error_log()` was said to reach the browser;
three pictures cut the details' Build watch section off under the
footer; the require had no comment on whose dependency it rides; a
failed read of the versions left an enabled, empty control.
- Follow-up left: the Server section naming the PHP a running server is
on.
- **Since review:** `ca84255 → ba66103` is those five fixes and two
style notes, not reviewed again. The commit after it changes one
journey: `site-header.spec.js`'s stickiness test failed on both runners,
since the Debugging row left the details too tall to fit over the tray
at its smallest on their screens; its seeded site now has no creation
date and no trunk date, two rows fewer, and the journey was run at
windows 700, 680 and 640 high and is red at 640 with the app's rule
broken. On the head: `npm run lint`, `npm test` (2002), `npm run
test:electron` (2003), all 119 journeys, the packaged smoke and the docs
build pass. The twelve pictures were taken four times before the last
change and two of them moved by 12 and 30 pixels on a corner between
takes, the known noise; the three retaken after it were taken once.

</details>

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

- `src/settings.cjs`: `phpVersion`, `wpDebug`, `scriptDebug`,
`acceptSwitch`. `src/wp-debug-constants.js`: `debugConstants(debug)`.
`src/server-runner.js`: `php` and the constants from the config.
- `src/main.js`: `phpVersions()`, `playground:php-versions`,
`playground:start` reads the store at each start. `src/preload.js`:
`listPhpVersions`.
- `src/renderer/settings-view.cjs`: `phpVersionChoice`.
`src/renderer/hooks/use-settings.jsx`: `php`.
`src/renderer/components/settings-dialog.jsx`: `SitesTab`.
`src/renderer/site-details.cjs`: `phpVersion` and `debug` in the rows.
- `scripts/screenshots/shots.cjs`: the three site views at 1200×900.

</details>

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

The retaken images are in the diff: `docs/public/screenshots/` shows the
details before and after.

</details>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
zaerl added a commit that referenced this pull request Oct 6, 2026
…what a quit stopped (#641)

## Why

Fourth part of #559, on the dialog of #638. Every site started cold: its
development server and its build watch waited for a press, every time
the site was opened and every time the app was. A contributor who works
on one site all afternoon, and a mentor who opens the app to find the
sites they left running, each had a press to make first.

## What changes

**Opening and quitting, on the General tab.**

- **Start the server when I open a site** and **Start the build watch
when I open a site**, both off unless turned on. On, opening a site that
is set up starts its server, its watch or both, as a press of Start
would; on WordPress Core the server's start brings the watch with it.
Nothing starts for a site whose setup, update or deletion is under way,
and nothing already running or starting is started again.
- **When I quit, running servers and build watches**: **Stop them**,
which is what the app has always done, or **Stop them, and start them
again next time**. Under the second, main writes down at quit which
sites had a server or their project's watch running, and the next launch
starts them again, whichever site it opens on, then forgets the list.
The prototype's third answer, leaving them running past the quit, is
deliberately not offered: the quit sweep exists to end every child the
app started (#83).

**How a site decides.** Opening a site is an edge, and so is the list a
quit left: each is consumed once, when the site is ready for it, in a
pure module (`src/renderer/auto-start.cjs`) the row asks. The row keeps
the gates and the calls: the server's start first, awaited, then the
watch where that start did not bring it up. A watch the server's start
was refused, because the terminal is held, is not asked for twice.

**How the quit remembers.** `before-quit` runs synchronously and is not
awaited, so the write goes through the store's synchronous accessor,
which the first read at startup has made; a store that cannot be written
is logged and does not stop the sweep. A site with both its server and
its watch running is listed under both.

**Not in this pull request:**

- Leaving servers running past the quit. See above.
- Starting a site that is not set up: the setup checklist is what starts
things the first time.
- The theme (#560), the last of #559's rows.

## How to test this

Platforms: any. The journeys drive this on macOS and Windows. From the
repository root, after `npm ci` and `npm run build:once`:

```
npx playwright test --project=journeys auto-start.spec settings.spec i18n.spec site-header.spec
```

- `auto-start.spec.js`: with the server set to start, the site the
window opens on asks for its server once, and on Core the watch with it;
opening another site asks for that site's; coming back to one whose
server runs asks for nothing and does not stop it. With the watch set to
start, the watch is asked for and no server; turned off, a relaunch
starts nothing. With a list a quit left, the served site's server and
the watched site's watch are asked for, whichever site the window opens
on, the served site's watch once; the list is forgotten as it is read; a
relaunch with nothing left starts nothing, and a list left under
'restart' is not followed under 'stop'. The quit setting is chosen and
kept.
- `settings.spec.js`, `i18n.spec.js`: the dialog, in English and in the
pseudo-locale.
- `site-header.spec.js`: the details' stickiness journey now sizes its
window itself.

Nothing is run: the handlers that start the server and a script are
stood in, and the journeys read what they were asked. The quit's own
write is `ipc-wiring.test.cjs`'s subject: under 'restart' it writes the
sites with a server and those running their project's watch, under
'stop' nothing, and a store that cannot be written is logged and the
sweep still reaches every child. I broke the open-site gate (every row
started) and the quit's write, and each was caught.

**Worth a look by hand** (any platform, the current head, a site that is
set up): Settings → General, turn on **Start the server when I open a
site**, close, open another site and come back: the server starts, and
the Logs tray shows its lines. Set **When I quit** to **Stop them, and
start them again next time**, leave the server running, quit, open the
app: the server starts again on that site, whichever site the window
opens on. Quit again with it running and set the quit back to **Stop
them** before you open the app: nothing starts.

**What must not have happened:** a server started for a site whose setup
or update is under way; a server stopped by the setting (it only
starts); a second watch started on a site whose server's start started
one; a server or watch running past the quit; the list followed twice
after a relaunch that was not a quit.

**What I could not test:** the real quit path end to end, since the
journeys stand in for the server; the wiring test drives the handler
with recorded children.

## Risks and limitations

- Review: 2 passes, 12 findings fixed here, none left. Details below.
- **A start asked for cannot be taken back:** a site deactivated, or an
update pressed, in the moment between the plan and the server's first
answer still gets its start. The window is one request to main.
- **Two servers starting at once** (on a relaunch with two sites listed)
can both find Playground's first port free and one fall back to another
port; pressing Start on two sites quickly has the same race today.
- **On Gutenberg a relaunch with both listed starts the watch**, which
removes `build/` under the just-started server for its rebuild, as both
settings on do.
- The stickiness journey's second half held only on screens that leave
the tray at its largest too little room; on a tall screen it did not, so
the journey sizes its window to the smallest the suite runs on.

## Related

Part of #559, which is part of #542. Follows
#640. The quit
sweep: #83.

---

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

- **Edges, consumed once, in a pure module.** The first version decided
inline in the component, which the suite cannot load, and predicted
whether the server's start would bring the watch from a copy of "built"
that the start re-reads from disk. Now the server's start is awaited and
the watch's state read after it.
- **The quit writes through a synchronous accessor.** `before-quit` is
not awaited by Electron; the store is an ESM import made at startup for
the locale, so it is there by the quit, and the sweep must not wait on
it.
- **Two answers to the quit question, not three.** Leaving children past
the quit undoes #83.
- **The list is read once and forgotten**, and only while the setting
still says 'restart': a launch that ends badly does not start everything
twice, and a list left under 'restart' is not followed under 'stop'.

</details>

<details>
<summary>Review outcome (required — see AGENTS.md)</summary>

**Pass 1: 5 [fix here] · 2 [follow-up]. Pass 2 (on the fix-up): 3 [fix
here] · 2 [follow-up]. All 12 applied here.**

- **Review:** completed — a separate agent context, read-only; head
`e5a0ba4` / base `c34aa45`; the quit path read against the store rule
and the sweep, the effect's edges and gates, Playground's port choice
for two servers at once, the journeys against the standard; ESLint, the
unit files and the pot extraction run, Playwright not; 5 [fix here], 2
[follow-up].
- Fixed: an error writing the list at quit would have skipped the sweep;
a site with both its server and its watch was remembered under the
servers only, and on Gutenberg with a build the watch was not started
again; the start decision lived in the component, with a prediction from
a stale copy of "built"; a #557 comment sat over the wrong code; the
journeys slept.
- Follow-ups, taken here: the stickiness journey's comment and its wait
for the resize; a dead check in the effect.
- **Review:** completed — a second separate agent context, read-only;
head `c75017f` / base `c34aa45`; the awaited server start traced through
the hook's two paths for whether the watch's state is set before it
resolves, the throwing-store test checked to be red on the first commit;
ESLint and the unit files run; 3 [fix here], 2 [follow-up].
- Fixed: a round trip to main was not a sound sign that nothing was
started, since a start asks main only after awaits of its own, so the
journeys now read the controls after the settings are in the page and
the effects have run; the resume journey did not seed the list a quit
now writes; the effect's catch was silent.
- Follow-ups, taken here: a watch the server's start was refused was
asked for a second time; the throwing-store test did not read the log.
- **Since review:** `c75017f → 5117052` is those five, not reviewed
again. On the head: `npm run lint`, `npm test` (2018), `npm run
test:electron` (2019), all 123 journeys, the packaged smoke and the docs
build pass.

</details>

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

- `src/settings.cjs`: `autoStartServer`, `autoStartWatch`,
`quitBehavior`. `src/resume-sites.cjs`: `sitesToResume`, `readResume`.
`src/settings-store.js`: `peekStore`. `src/main.js`: `scriptByRunId`,
`rememberRunningSites` in `before-quit`, `sites:resume`.
`src/preload.js`: `takeResumeList`.
- `src/renderer/auto-start.cjs`: `autoStartPlan`.
`src/renderer/settings-view.cjs`: `quitItems`, `resumeFor`.
`src/renderer/index.jsx`: the row's effect; App reads the list once.
`src/renderer/components/settings-dialog.jsx`: `OpeningAndQuitting`.
- Tests: `tests/unit/auto-start.test.cjs`, `resume-sites.test.cjs`,
`settings.test.cjs`, `settings-view.test.cjs`, `ipc-wiring.test.cjs`;
`tests/e2e/journeys/auto-start.spec.js`.

</details>

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

Not attached: I have no way to upload images from the command line. The
journeys drive everything that changed, and the guide page says what
each control does.

</details>

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
@zaerl zaerl added this to the v2.0.0 milestone Oct 7, 2026
ryanwelcher added a commit that referenced this pull request Oct 13, 2026
## Why

41 commits have reached trunk since `v1.2.1`, and milestone
`v2.0.0-beta.1` has 77 closed issues and none open. The main changes are
the redesign on the WordPress Design System (#591), a settings dialog
with language, theme, PHP version and debug flags (#638 to #651), and
translation of the whole interface (#634 to #664). This is a beta: it
ships with `--prerelease`, so `releases/latest` stays on `v1.2.1`.

`electron-builder` names the artifacts after `package.json`, so the
version has to move before the tag and artifacts exist.

## What changes

- **The version.** The three root version fields in `package.json` and
`package-lock.json` move from `1.2.1` to `2.0.0-beta.1`. No dependency
versions change.
- **The first shipped translations.** `npm run i18n:download` added a
catalog for each of the seven locales at 80% or more on
translate.wordpress.org. None was removed, and no right-to-left locale
was held back.

| Locale | Translated | Strings |
| --- | --- | --- |
| cs | 94% | 1001 |
| en-ca | 100% | 1062 |
| en-gb | 100% | 1062 |
| gl | 100% | 1062 |
| hu | 100% | 1062 |
| nl | 100% | 1062 |
| pl | 96% | 1028 |

## How to test this

**Platforms:** any for steps 1 to 3. The artifact checks need macOS for
`Info.plist` and Windows for the `.exe`.

**Starting state:** this branch checked out with `node_modules`
installed. Run every command from the repository root.

1. `npm run lint` is clean and `npm test` passes.
2. `node -e "console.log(require('./package.json').version)"` prints
`2.0.0-beta.1`.
3. `python3 -c "import
json;d=json.load(open('package-lock.json'));print(d['version'],
d['packages']['']['version'])"` prints `2.0.0-beta.1 2.0.0-beta.1`.
4. On the Buildkite build for the current head, the artifacts are named
`wordpress-contributor-toolkit-2.0.0-beta.1-*`. The `app.asar` root
holds exactly `node_modules`, `package.json` and `src`, with npm 11
under `node_modules/npm` and no `.codesigning`. On macOS,
`CFBundleShortVersionString` reads `2.0.0-beta.1`.
5. In the installed app, set the language to Dutch in the settings and
relaunch. The interface is in Dutch.

**What must not have happened:** no dependency version may change. `git
diff trunk -- . ':!src/languages'` shows exactly three changed lines,
all of them the root version.

## Risks and limitations

`npm test` run from an interactive terminal fails two integration tests
on macOS. That is a test bug, fixed separately in #680. It does not
affect CI or the app.

## Related

Milestone `v2.0.0-beta.1`. Release notes follow on the release.

---

<details>
<summary>Review outcome (required — see AGENTS.md)</summary>

0 [fix here] · 0 [follow-up]. Automatic checks only, as in #533: this
change has no logic to review.

- **Review:** completed, deterministic layer, in session; head `9853082`
/ base `5d5f701` (trunk). `npm run lint` passes, `npm test` passes (2098
tests, 0 failures, 2 skipped), both files are valid JSON, and the diff
outside `src/languages/` is the three root version fields. Outcome:
nothing to fix.
- **Since review:** none.

</details>

🤖 Generated with [Claude Code](https://claude.com/claude-code)
ryanwelcher added a commit that referenced this pull request Oct 20, 2026
…n the editor (#677)

## Why

After a patch is applied or a pull request is checked out, the app says
how many files changed and nothing more. Unless a contributor already
knows which files to look at, they can't find the change, and their own
edits on top of it aren't listed anywhere. The only way to undo one of
their edits was **Discard all changes**, which also took the patch away.
This is #669 in one PR. It was a stack of five (#677, #678, #679, #682,
#683); they are folded together here because each builds the same
section, and a reviewer was already testing the top of the stack.

## What changes

- The site's details gain a **Changed files** section, last in the
sidebar after **Build watch**, so a long list grows without pushing the
server and the watch out of reach.
- It has up to two groups, each under its own heading and drawn as a
tree of folders:
- **From the patch** / **From PR #N**: the change's own files, each with
a pencil when the contributor has edited it since.
- **Your changes**: every other file changed on the branch, with a plus
when the contributor created it. On a ticket with nothing applied, this
is the only group.
- Each folder starts open, and a chain of folders that hold only one
another shares one row (`tests/phpunit/tests/wp`), as GitHub and VS Code
draw them. A folder row's button, or the Left and Right arrows, close
and open it. Each file shows its name with a page icon and its full path
as a tooltip. Drawn with `__experimentalTreeGrid` from
`@wordpress/components`, which gives the tree roles a screen reader
needs and the arrow keys between rows.
- One click on a file opens it in the first editor the app detects,
inside the site's folder (`open -a App <site> <file>`, `Code.exe <site>
<file>`). With no editor detected, the file is selected in Finder or
Explorer instead. "Choose application…" after a file fails to open opens
that file in the application picked.
- **A revert button** on each file the contributor changed, at the end
of its row. It asks first, and leaves the file as **Discard your
changes** would: a file of theirs back to the base, a new file deleted,
a pull request's file back to the pull request's version, a patch's file
back to how the patch left it with the patch still applied. A patch file
they did not touch, and either end of a rename, have no button. While an
install, build, trunk update or dev server runs, the button is held and
says why.
- **Discard your changes** (renamed from "Discard all changes") now
removes only the contributor's own work. With a patch applied, the tree
goes back to the base and the patch is re-applied from its stored text,
so the patch and **Revert this patch** stay. If the text was not kept,
the patch goes too and the dialog says so first. **Discard this ticket
to its base** still removes the patch. The ticket card's discard button
from #696 calls the same discard, so every "discard your changes"
control keeps the patch.
- How the pencil is decided: a pull request is its branch's base, so any
of its files with changes on top was edited. A patch is an uncommitted
layer, so each file it wrote is fingerprinted when applied (SHA-256,
line endings normalised, `src/change-fingerprint.cjs`) and compared with
the disk.
- The list refreshes when the window regains focus, which is when a
contributor comes back from their editor, and whenever the app itself
changes the tree.
- New IPC `git:revert-file`, which accepts only paths Git reported as
changed. `editor:open` and `dir:show` take an optional file inside the
site, checked by its real path, so a symbolic link that leads out of the
site is refused.

Not in this PR: an editor preference, per-file diffs, marking deleted
files, live updates while the app keeps focus, undo after a revert,
reverting several files at once, and remembering which folders were
closed across a relaunch.

## How to test this

Platforms: macOS and Windows. The launch arguments, path checks and the
revert's writes (`core.autocrlf`) differ per platform.

The journeys cover the flow with stubbed editors and fixture
repositories. Run from the repository root:

```
npm run test:e2e -- tests/e2e/journeys/patch-apply.spec.js tests/e2e/journeys/pr-checkout.spec.js tests/e2e/journeys/i18n.spec.js
```

They check opening a file in the editor or the file manager, "Choose
application…", the groups and their marks, the list during a build,
after a discard, after a refused apply and after a refocus, each kind of
revert, and that a folder closes and opens and the revert button is
reached with the arrow keys. What they can't reach is a real editor, a
real `wordpress-develop` clone and a real pull request, so that part is
by hand.

**Starting state:** a site with a real `wordpress-develop` clone, VS
Code installed, the dev server stopped, and a ticket linked whose Trac
attachment applies cleanly, for example #23602 with
`23602_pr14112_20261008.diff`.

1. Apply the attachment. **Changed files** appears at the bottom of the
sidebar, under **From the patch**, as a tree of two folder rows:
`src/wp-includes` over `class-wp.php`, and `tests/phpunit/tests/wp` over
`queryPosts.php`. No pencil, no revert button.
2. Click the `src/wp-includes` folder. Its file is hidden and its
chevron points right. Click it again. The file is back.
3. Click `class-wp.php`. VS Code opens the site folder with that file
open. Click `queryPosts.php`. It opens in the same VS Code window.
4. In VS Code, change `class-wp.php` and `src/wp-login.php`, and create
`src/new-helper.php`. Switch back to the app. `class-wp.php` has a
pencil; under **Your changes**, `wp-login.php` has none and
`new-helper.php` has a plus. All three have a revert button.
5. Revert `new-helper.php`. The dialog asks "Delete
src/new-helper.php?". Confirm. The file is gone from disk and from the
list.
6. Revert `class-wp.php`. The dialog says it goes back to how the patch
left it. Confirm. The file has the patch's change and not yours, and the
pencil is gone.
7. Change `class-wp.php` again and switch back. Click **Review & submit
changes**, then **Discard your changes**. The dialog says the patch
stays applied. Confirm. `class-wp.php` has the patch's change only,
`wp-login.php` is back to trunk, and **Revert this patch** is still
there.
8. Click **Revert this patch**. The section is gone.
9. Link a ticket with a GitHub pull request and check it out. **Changed
files** lists its files under **From PR #N**, the same files its preview
did.
10. On Windows, repeat steps 1, 3 and 6 with a site whose path has a
space in it.

**What must not have happened:**

- A second VS Code window for the same site.
- Anything opened outside the site folder.
- A file listed in both groups.
- Any patched file with a pencil right after applying, or right after
step 6, before you changed anything. On Windows this would point at line
endings.
- The patch's change missing from `class-wp.php` after step 6 or step 7.
- Any file other than the one reverted changed by a revert.
- After checking out a different pull request, the list from the
previous one.
- A focus ring cut off at either edge of the sidebar, on a folder, a
file or a revert button, after tabbing in or after cancelling a revert.

## Risks and limitations

- Review of the combined branch: 7 [fix here] fixed, 3 [follow-up]
deferred (see the review outcome). CodeRabbit's one finding, on patches
applied before this PR, is fixed.
- Rebased onto trunk after #696 and #697. The ticket card's discard
button that #696 added now keeps an applied patch or pull request, like
the note's link and the review modal's; before the rebase it would have
removed the patch.
- `__experimentalTreeGrid` is not a stable export yet; the window
already uses another `__experimental` control the same way
(`create-site-dialog.jsx`).
- In the tree, the pencil and plus are the file button's description
rather than focusable stops of their own, because only buttons are in
the tree's reach of the arrow keys. A screen reader hears them; a
sighted keyboard user sees the icon but can't open its tooltip.
Follow-up.
- Verified by hand with VS Code on macOS. PhpStorm, Zed, and Windows
generally are untested with a file argument, and the revert and discard
have not been run by hand on a real clone or on Windows.
- A chain of folders shares one row, so deep paths indent less, but a
long merged folder name, like a long file name, can wrap in a narrow
sidebar.
- A move onto trunk that updates the branch and then fails at the
checkout keeps the patch's old text and fingerprints. A pencil can then
mark a file trunk changed, and a discard or revert re-applies text
written for the old trunk. Older than this PR; follow-up.
- Neither a discard nor a revert is held while an apply or pull request
checkout runs. Older than this PR (the discard link had the same gate);
follow-up.
- A pull request checked out before this ships has no list until it is
checked out again. A patch applied before this ships marks every one of
its files as edited, since the app did not record what it wrote:
**Discard your changes** and each revert stay offered, reverting a file
unmarks it, and a discard records the patch afresh. Covered by unit
tests only; staging it by hand needs a site from an older build. A file
the patch or the contributor deleted is still listed, and says it is
missing when clicked.
- The diff is about 3,000 lines across 32 files, well over the size
AGENTS.md flags. It is one feature in five steps; the commits keep each
step apart for reading one at a time, in this order: patches, pull
requests, your changes, discard, per-file revert, then the tree.
- In one of three local runs of the full journey suite, four journeys
failed while the run took three times its usual length; the two runs
either side passed all of them. I read it as load on the machine, not
the branch.

## Related

Part of #669. Replaces #678, #679, #682 and #683, which are folded in
here.

A pull request's list including files trunk changed, when the site's
trunk is behind the pull request, is older than this PR and tracked in
#701.

---

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

- **One PR, not a stack of five.** Every step builds one section, and a
reviewer was testing the top of the stack while commenting on the
bottom.
- **Trees, one per group.** A reviewer asked for `TreeGrid` because full
paths filled the sidebar's width. One tree per group keeps the headings.
- **A chain of single folders on one row** (`tests/phpunit/tests/wp`),
as GitHub and VS Code draw them. The first version gave every folder its
own row; a reviewer pointed out it took five rows to show one file.
- **Last in the sidebar**, not first: the list has no fixed height, and
first it pushed the server and the build watch out of view.
- **A revert leaves the file as Discard your changes would,** so the two
never disagree about what "your changes" means. A patch file therefore
goes back to the patch's version, not to trunk.
- **Discard keeps the patch.** One button that always means "remove my
edits, keep the patch or PR" matches what it already did for a PR.
Removing the patch stays **Revert this patch**; re-applying the stored
text is the only way back to "as the patch left it".
- **Renames refused, not modelled.** One change spread over two paths
cannot go back one path at a time.
- **One click, no per-file menu.** There is no editor preference yet, so
the click takes the first detected editor. A preference belongs in the
Settings dialog from #638.
- **The real path, not just the text.** A lexical check let a symbolic
link inside the site point outside it.

</details>

<details>
<summary>Review outcome (required — see AGENTS.md)</summary>

Earlier reviews of each folded PR are recorded on it (#678 at `8ddcb27`,
#679 at `eb7be4f`, #682 at `85b47f9`, #683 at `ff23819`, before the
rebase); every finding there was fixed or deferred, and the deferrals
are in Risks. The reviews below cover this combined branch: reviews 1 to
3 on base `60078fe`, review 4 on trunk `819575a` after the rebase.

- **Review 1:** completed. A separate agent context reviewed
`60078fe..73b47df` (#677 to #679 plus the tree) against
`.github/instructions/code-review.instructions.md`. `npm run lint`
clean, `npm test` 2147 passed. 1 [fix here] · 1 [follow-up].
- Fixed in `eb93d07` (🟡): the file list was walked as soon as an apply,
revert or pull request switch changed the tree, but the status it is
grouped by was read again only after the install and the build, so for
those minutes a patch's files were listed as the contributor's. The
status is now read with the walk. The journey "while the build after an
apply runs, the patch's files are the patch's (#669)" holds the build
open; it fails without the fix.
- Deferred (🔵): the marks and the rule for whether the section shows are
decided in `site-details.jsx` rather than `changedFileGroups`. One
branch each.
- **Review 2:** completed. Separate agent context, the fix above. No
findings; confirmed on every path into the install and build. Its
comment note was taken in `52ecde4`.
- **Review 3:** completed. Separate agent context, `8400845..52ecde4`
(#682, #683 and the tree redrawn over them). 1 [fix here] · 2
[follow-up].
- Fixed in `288b7c8` (🟡): the tree remembers the last button focused in
it and gives every other one no tab stop, so a file that left the list
on a refocus took the whole tree out of the tab order. The tree is now
keyed by its files. The journey "a file that leaves the list does not
take its tree out of the tab order (#669)" fails without the fix.
- Fixed in `288b7c8` (🔵, raised as a follow-up): the treegrid itself had
no accessible name; it now has its group's.
- Deferred (🔵): a sighted keyboard user cannot open a mark's tooltip
(see Risks).
- **Review 4 (rebase onto trunk):** completed. Separate agent context,
`819575a...455d0e8`, on the conflict resolution with #696: the ticket
card's discard button pointed at `discardYourChanges`, `discardLabel` at
its question, and `nothingOfYours` in the shared reason. 1 [fix here] ·
0 [follow-up]. Fixed in `a54b394` (🔵): a unit test still tied the label
to the full-discard question, passing only because both say "Discard
changes". Nothing from #696 or #697 lost; no `discardAllChanges` left.
- **CodeRabbit** reviewed `a54b394`: 1 finding (🟡), fixed in `d55ba08`.
A kept patch applied before fingerprints marked none of its files as
edited, so **Discard your changes** was disabled and no file could be
reverted. Its files still changed now count as possibly edited.
- **Since then:** `c7f4ad9` merges a chain of single folders into one
row, at @zaerl's request; `299bc2d` and `9a28812` give the revert
button's focus ring room at the sidebar's edge.
- **Review 5:** completed. Separate agent context, `a54b394..d55ba08`. 3
[fix here] · 1 [follow-up], all fixed:
- Fixed in `1f93225` (🟡): a folder closed before a file left the list
could stop being a row of its own and still hide everything under it,
leaving an empty tree. Unit test.
- Fixed in `1f93225` (🟡): the window dropped `renamedByChange` from the
reply, so both ends of a rename were offered a revert the handler
refuses. Now carried by `unsubmittedFromReply`, unit-tested.
- Fixed in `0dea62b` (🔵): on a patch from before fingerprints, a
reverted file still looked edited. Its first revert now records a full
set. Unit test.
- Fixed in `4a77c79` (🔵, raised as a follow-up): a first-level folder's
focus ring was clipped at the sidebar's other edge. Every cell now has
room for the ring; the journey checks a folder, a file and a revert
button, and fails without it.
- **On `4a77c79`:** `npm run lint` clean, `npm test` 2193 passed, full
journey suite 163 passed hidden on macOS.

</details>

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

- `resolveLaunch(editorPath, sitePath, { platform, file })` appends the
file after the folder in the argument vector. Never a shell.
- `existingSiteFile(sitePath, relPath, { platform, realPath })` runs
realpath on both the site and the file, so a site behind a link (macOS's
`/var`) still holds its files.
- `src/renderer/affected-files.cjs` holds the decisions, unit-tested:
`affectedFiles`, `changedFileGroups` (groups, labels, `canRevert`),
`unsubmittedFromReply` (the reply as the list reads it),
`affectedFileTree` (paths to tree rows, merging single-folder chains),
`visibleAffectedRows` (what a closed folder hides),
`revertFileQuestion`, `revertOutcome` and `fileOpenTarget`.
- `src/git-write.cjs`: `restoreWorktreePaths(dir, source, paths)`,
worktree only, literal pathspecs on stdin. `src/trunk-update.js`:
`discardFile`. `src/patch-apply.js`: `applyPatchToDir({ paths })` keeps
only those files' sections.
- `src/main.js`: `git:revert-file`; `git:discard-to-base(sitePath, {
keepChange })` re-applies the kept patch and fingerprints it again;
`git:unsubmitted-work` adds `entries`, `editedSinceChange` and
`renamedByChange`.
- The tree's indent is read from each row's `aria-level` in CSS
(`attr(aria-level type(<integer>))`), since the repository's lint bans
inline styles. The revert button gets the tree's item props through
`Tooltip.Trigger`'s render prop.
- `forgetChangedFiles` in `src/renderer/index.jsx` bumps the dirty
probe's generation, so an answer still in flight from before a change is
dropped, then clears the list and asks again.

</details>

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

To add.

</details>

🤖 Generated with [Claude Code](https://claude.com/claude-code)
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.

1 participant