Repository navigation
Redesign the interface with the WordPress Design System - #591
Merged
Merged
Conversation
…ow (#590) ## Why The app's interface was built from design-system components without a design review, and #542 replaces it with the one in the [prototype](https://github.com/jameskoster/wordpress-contributor-toolkit-prototype). #555 is the first piece: the shell everything else sits in. Today that is a dark sidebar of buttons beside one scrolling column in a 1000×700 window; the prototype's is a "My sites" list beside the open site's page, with a footer under it. **This PR targets `redesign/wpds`, not `trunk`.** The redesign is built on that branch and reaches trunk in one piece at the end, so trunk never holds a half-redesigned window. ## What changes - **The window is a sites list, a page and a footer.** The list is a `DataViews` list inside an `admin-ui` `Page`, as in the prototype. The open site's page is a `Page` whose header carries the site's name, its project and the button that hides the list. With no site there is no list: a centred "No sites" and one button. - **The site view inside the page is the old one, unchanged**, apart from its own name and project tag, which the header now shows. Rebuilding it is #556 and #557. Every site's view stays mounted and is hidden when it is not the open one, because a site's terminal, server and watch live in its view. The prototype mounts only the selected site; the app cannot. - **What the list says about a site is decided in `sites-list.cjs`**: its name, the line under it, what its dot reports, and which site a selection opens. - **The window opens at 1280×800**, or at the screen's work area where that is smaller, and has a minimum size. 1280 is the 320px list plus the 960px from which the page lays out in two columns. - **Toasts move to the bottom-right corner, above the footer.** The page now reaches the window's top-right corner, where the open site's actions are, and a toast that stays until dismissed sat on the site's menu. - **Two new dependencies:** `@wordpress/admin-ui` and `@wordpress/dataviews`. They and what they bring in are bundled by esbuild, so they are excluded from the package, like the renderer-only packages before them. **A bug this would have introduced, found and fixed here:** `DataViews` builds element ids from an item's id and points `aria-labelledby` at them, and that attribute is split on spaces. With the site's path as the id, a site in a folder whose path has a space, which is any Windows user with two names, lost its accessible name. A row's id is now the path encoded. `shell.spec.js` has a journey for it that was red before the fix. **Deliberately not in this PR**, because each needs per-site state that the list and the header cannot reach yet, or a region that does not exist yet: - the actions menu on each row and a "server running" dot (#556); - the header's process menus, review button and site menu (#556, #557); - the footer's tray toggles (#558) and Settings (#559); - dark mode (#560), and the documentation's screenshots and text (#561). ## How to test this Platforms: any. Use the Buildkite build for the current head of this branch. The journeys carry this. From the repository root, `npm run test:e2e` runs them all; `tests/e2e/journeys/shell.spec.js` is new and is the one that walks the shell: - pressing an entry opens that site, the header and the list's mark follow, and the site that was left is as it was left: what was typed into its field is still there; - pressing the open site's entry leaves it open; - a site in a folder with a space in its path is named in the list and opens; - a site with an unfinished update or an old trunk says so in its entry, after its name, before it is opened; - in the pseudo-locale, everything the list says but the site's own name and its project is translated (`i18n.spec.js`); - the list can be hidden and brought back, the button says which it is, and hidden the list is out of the accessibility tree and does not take focus; - a window with no site has no list, offers "Create site", and keeps the footer, whose feedback button explains the form before opening it; - the window opens at the designed size or the screen's, with its minimum. What they cannot reach is how it looks: **Starting state:** the app with two or more sites, one of them Gutenberg. 1. Look at the window as it opens. It is wider than before, the list is on the left with "My sites" and "Create new site" above it, and the open site's name and project are in the header of the page. 2. Click another site in the list. The page changes to that site, and its entry is the highlighted one. 3. Click the button at the left of the page's header. The list slides away and the page takes the whole window; click it again and the list comes back. 4. In the footer, click "Give feedback". A popover explains the form, above the button and inside the window. 5. Delete a site whose folder is open in another application, or otherwise make a deletion fail. The error appears at the bottom-right, above the footer, and does not cover the site's menu. **What must not have happened:** - Switching sites does not restart anything: a dev server or a build watch started on one site is still running when you come back to it. - The old dark sidebar is gone entirely, not sitting behind the new list. ## Risks and limitations - **The JavaScript bundle roughly doubles, from 3.2 MB to 6.7 MB unminified.** `DataViews` brings `@wordpress/data`, `@wordpress/date` with `moment-timezone`, and `@base-ui/react`. It is read from disk once at launch; I did not measure launch time. - **`DataViews`' own strings are not translated.** The app loads only its own catalogue. The list as configured here shows none of them, but anything a later slice turns on will be in English. - **The site view looks half-moved.** Its rename button sits without the name beside it, until the site's menu moves to the header in #556. - **Body text is now the design system's 13px/20px, in the old site view and in every dialog too.** Where one set no size or line height of its own, its text changed. - **An old rule still gives every `button` an 8px right margin.** The shell's own buttons are taken out of it; the rule itself stays until the old site view, whose rows of buttons lean on it, is rebuilt (#557). - **The row's dot means what it meant before**: an old trunk or an unfinished update. The prototype's means "server running", which the list cannot know yet. - **A site's terminal can show stretched text when the site was not the one open at launch.** This is on trunk already and is not from this change, but switching sites is now the main way around the app, so it shows more. - **The documentation still describes the old sidebar** ("Create a site", "Share feedback"). #561. - **The documentation's screenshots are not retaken.** The harness was updated and four of its shots were run to check it still drives the app (`empty-state`, `create-site-modal`, `site-menu`, `stale-site-notice`); the images were not committed. #561. - **Row ids can be prefixes of each other** (`/x/fix` and `/x/fix-2`), and `DataViews` matches some of its element ids by prefix. Nothing that reads those matches is on screen until rows have an actions menu, so the fix, a terminator on the id, belongs to #556. - **The window is sized from the primary display**, also when the system opens it on a second, smaller one. - **Only the left-to-right `DataViews` stylesheet is loaded**, as for the components already. - **Two `h1` headings in one window**: "My sites" and the open site's name, as in the prototype. - **The packaged smoke ran against an unsigned build**: `npm run pack:dir` stops at signing in a checkout without the provisioning profile, after the app is assembled. - **The manual steps above were not run by a person.** I looked at screenshots of the list, the hidden list, the empty window, the feedback popover and an error toast. - **Checked on macOS only.** Windows is covered by CI. - **Pre-PR review, two passes: everything marked to fix here was fixed, eight findings were left as follow-ups, and none was high.** The two that mattered: the new packages were shipping unbundled inside the app, and the screenshot harness could no longer select a site. Details are in the review outcome below. - **One unit test failed once and passed twice on the same commit**: `macOS signing removes its temporary key when Buildkite terminates the shell`, which waits five seconds for a shell to exit. Nothing in this PR touches signing. ## Related Part of #555 and #542. First PR on the `redesign/wpds` integration branch. --- <details> <summary>Design decisions and alternatives considered</summary> **`DataViews` for a list of a handful of sites.** It is heavy for what the list does today, and a dozen lines of markup would draw the same rows. It is what the prototype uses, #542 asks for the design system's own components, and the rows gain actions in #556, which is what `DataViews` is for. **The list hides; it does not collapse to a rail of initials.** The prototype's hides too, and the old 56px rail of single letters told two sites named alike apart by nothing. **"Deleting site…" replaces the project in the row, and the entry's name ends with "(Deleting)".** The prototype has no such state. The old entry was a disabled button; a `DataViews` row cannot be disabled, so `siteToOpen` refuses a site that is being deleted, and the name says why pressing it does nothing. **The footer is there with no site too.** The prototype's first screen has none. Feedback is about the app, and a newcomer who cannot create their first site is the person most likely to want to say so. **The three notices with no place in the prototype stay at the top of the page area**: a setup in flight, the Playground web server where a build ships one, and a ticket that arrived from a link when there is no site to put it in. **`#root` is not made a stacking context**, which the prototype does. The toast region has to rise over a dialog, which is drawn on `body`, so that what was done inside one is still confirmed. **The stylesheet is a file, `shell.css`**, imported by the entry point and bundled into `index.css`. It is excluded from the package by name, like the `.jsx` sources it is bundled with. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> Run as the repository's review procedure, with the judgement pass in a fresh context that took no part in the change. First pass on `eeef3cc` against `e1b17dd`; second pass on the fix-up `81fbeb4`; the head, `63723c4`, applies what the second pass asked for. **First pass: sixteen findings, two of medium weight, none high.** It compared the old `App` with the new one prop by prop and found every `SiteRow` prop, the selection effect and the hidden views carried over, and one real loss, the page's maximum width. Fixed: - **The new packages shipped unbundled in `app.asar`** (medium), about 18 MB. They are excluded like the renderer-only packages before them, and the re-packaged archive holds none of them. - **The screenshot harness clicked a site's name as text** (medium), which the list's own button now covers. It selects by the entry's button; four shots were run to check. - **The page had lost its maximum width.** Restored, at the prototype's 1080px. - **A silent `catch`** around reading the screen in `createWindow`. Removed: every caller runs after the app is ready. The unit tests' Electron had no `screen` and had been passing because of that catch; it has one now, with a test that the window is given what it reports. - **A hundred-and-first site would have dropped out of the list**, paginated at 100 with no pagination control. The list is given every row and a constant view. - **The hide button said its state twice**, as a pressed state and as a name that changes. It says it once, as expanded or not. - **A site's warning was read before its name.** The name is first, the rest follows in brackets. - **An old `button { margin-right: 8px }` reached every new button**, so a row's click area stopped short of its edge. The shell's own buttons are taken out of it. - **In the journeys**: an absence read without the round trip `TESTING.md` asks for, "Deleting site…" read from the page and not from its row, two helpers only one journey uses sitting in `ui.cjs`, and a class selector where text would do. **Second pass: four findings, all low, two to fix here.** Fixed: three of the list's strings were not translatable (what a dot reports, the brackets around it, "Deleting"), in an area whose other strings are; they are wrapped, and `i18n.spec.js` gains a journey for the list. And nothing asserted what the hide button's `aria-controls` points at; `shell.spec.js` now does. Left as follow-ups, all in Risks above or here: row ids that can be prefixes of each other; the window sized from the primary display; the left-to-right `DataViews` stylesheet only; the design system's body type reaching the old view and dialogs; two `h1` headings; `ui.cjs` exporting two more part-of-the-screen locators than the six its header counts, which predates this change; `i18n.spec.js` finding the feedback popover by its class, which it needs a container for; and two small unit-test gaps (`rowId` being prefix-free, which goes with the first follow-up, and `sitesListRows` given `deleting: null`, which the app never does). **Checks, on macOS, on the head of this branch:** | Check | Result | | --- | --- | | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1757 pass, 2 skipped | | `npm run test:electron` | 1758 pass, 1 skipped, twice; one failure in a first run, described in Risks | | `npm run test:e2e` (journeys) | 62 of 62 | | `npm run test:e2e:packaged` | 15 of 15, on an unsigned `npm run pack:dir` build of `81fbeb4`; the head differs from it in the renderer's strings and in tests | | `app.asar` of that build | none of the eight excluded packages, and no `shell.css` | **The tests' claims were audited by breaking each one** in the source and running the test that should notice. 32 breaks, all 32 turn a test red on the line that states the claim. | Break | Caught by | | --- | --- | | Pressing another entry opens nothing | `shell` | | Pressing the open site closes it | `shell` | | Only the open site is mounted | `shell` | | The header names the first site whatever is open | `shell` | | The header has no project badge | `shell` | | A row's id is the raw path | `shell` | | What a site has to report is not in its name | `shell` | | The list is oldest first | `shell` | | A hidden list is still in the accessibility tree | `shell` | | A hidden list still takes focus | `shell` | | The list cannot be brought back | `shell` | | A window with no site still shows the list | `shell` | | A window with no site has no footer | `shell` | | The first-site button opens nothing | `shell` | | The feedback link opens another address | `shell` | | The window opens at its old size | `shell` | | The window has no minimum size | `shell` | | A site being deleted does not say so in its name | `engine` | | A site being deleted keeps its project line | `engine` | | The row does not wear its project | `engine` | | Toasts are back in the top-right corner | `engine` | | A second site can be started while one is being created | `create-site` | | The footer button is not translatable | `i18n` | | The empty window is not translatable | `i18n` | | What a site has to report comes before its name | `shell` | | The hide button always says the list is open | `shell` | | The window ignores the screen | `ipc-wiring` (unit) | | What a site has to report is not translatable | `i18n` | | The brackets around it are not translatable | `i18n` | | The list's title is not translatable | `i18n` | | The hide button names an element that is not there | `shell` | | The hide button names nothing | `shell` | </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Brings in the terminal fix (#592): a site's terminal is opened the first time the site is shown. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Brings in the site-status hook (#593): what the main process says about a site leaves SiteRow for useSiteStatus, with a journey for the sites list following it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…594) ## Why #556 is the second slice of the redesign (#542): the open site's header and its details, as the [prototype](https://github.com/jameskoster/wordpress-contributor-toolkit-prototype) draws them. After #590 the page had a header with only the site's name in it, and the site's view still opened with the old lines: a status pill, a pencil, two dates, the path, a link that opened a menu of applications, and a second menu at the far right. **This PR targets `redesign/wpds`, not `trunk`.** ## What changes - **The site's menu is in the page's header**, and holds everything done to a site as a whole: rename, copy the path, show it in the file manager, update to the latest trunk, open the folder in an application, delete. Renaming was a pencil beside the name and opening the folder was a menu of its own beside the path; both are here, the applications under "Open in". What the menu offers is decided in `site-menu.cjs`. - **The header belongs to the window, and what is in it belongs to the open site.** The window leaves a slot in the header and the open site's view fills it, so the buttons sit where the design puts them and still call that site's own functions. Only the open site fills it. - **The site's view is two columns: its cards, and its details beside them.** The details are what the old first lines said, as labelled facts: whether setup is done, when the site was made, the date of its trunk and its age when that is old, where it is, with a button that copies the path, and what it is a checkout of. They stay in view while the cards scroll. What they say is decided in `site-details.cjs`. - **The details can be put away**, with a button in the header. That is the window's choice and not one site's: it stays as it was left when another site is opened. Where the page is too narrow for two columns, the details go under the cards. Two things this needed: - **The app's own element is a stacking context again**, as in the prototype. The design system's menus and tooltips are drawn on `body` with no z-index, and the page's header, which has one, covered them: the menu opened under the page. #590 had removed this so that toasts could rise over dialogs; the toasts are now drawn on `body` themselves, which keeps that. - **The terminal scrolls sideways.** At the default window size, beside the details, its column is narrower than its eighty columns of text. It moves to the tray in #558. **Deliberately not in this PR:** - the header's process menus and its review button, and the sidebar's Server and Latest build sections (#557); - the staleness notice moving into the sidebar, as the prototype has it: see Risks; - "Open in terminal", PHP version and debugging in the details: the app has no such features or settings yet (#559); - the sites list's row actions and its "server running" dot, which follow in their own PR. ## How to test this Platforms: any. Use the Buildkite build for the current head of this branch. The journeys carry this. From the repository root, `npm run test:e2e` runs them all; `tests/e2e/journeys/site-header.spec.js` is new: - the menu looks for applications as it opens and offers each by name under "Open in", while its own list stays as it was; copying the path, opening the folder in an application or in one to be chosen, and showing it in the file manager all act on the site that is open, and on the other site once that one is opened; - a copy from the menu says so once, in a toast, and leaves the details' own button alone; a copy from that button is said to a screen reader once, and its going back is not; - an application that will not open the folder says why on the page, in view even when the page is scrolled to its end, with the way to choose another; - the details stay in view while the cards scroll; they say the open site's folder, project and setup, and the age of an old trunk; their button copies the path and says so; put away, they are out of the accessibility tree and do not take focus, the button says they are closed, and they stay put away when another site is opened. Renaming, updating and deleting through the menu are walked by `rename-site.spec.js`, `trunk-update.spec.js` and `engine.spec.js`, which now reach them there. `i18n.spec.js` gains the details and the menu in the pseudo-locale. What they cannot reach is how it looks, and the real applications: **Starting state:** the app with two sites, one with an editor installed on the machine. 1. Click the button with three dots at the right of the page's header. The menu opens over the page, not under it. 2. Point at "Open in". A second menu has "Other application…" first and your editors by name under it. Click your editor: the site's folder opens in it. 3. Click the button to the left of the three dots. The details slide away and the cards take the width; click it again and they come back. 4. Make the window narrow, or close the sites list and make it narrow. Below a certain width the details sit under the cards. **What must not have happened:** - The menu and the details button are the open site's: after opening another site, "Copy path" copies that site's path. - A toast, such as "Copied the path", still shows over an open dialog. ## Risks and limitations - **The staleness notice stays in the main column**, where the prototype puts it in the details under "Trunk as of". A put-away sidebar would hide the app's one prompt to update, and the next-action cue points at that banner. The details say the age in words. Worth deciding with the design before #557. - **The details' open state is not remembered across restarts.** The prototype keeps it; the app's only store is electron-store, and a preference for it belongs with the settings (#559). - **The button reads "Hide details" and "Show details"**, where the prototype says "sidebar": the window has two things that could be called that. - **Deleting still asks through the system's confirm box**, and a failed copy through its alert, as before. The prototype's dialogs for these come later. - **The main column is 560px wide at the default window size with both the list and the details open.** The old cards were laid out for more; they wrap, and the terminal scrolls sideways. - **The documentation still shows the old controls.** #561. Two shots of the harness were run to check it still drives the app. - **The manual steps above were not run by a person.** I looked at screenshots of the header, the open menu, the details, the details put away and a toast. - **Checked on macOS only.** Windows is covered by CI. - **Pre-PR review, two passes: everything marked to fix here was fixed, and one follow-up is left**, the guide pages that describe the old controls (#561). Two of the findings were of medium weight: a refused open said out of sight, and rows of the menu moving under the pointer. ## Related Part of #556 and #542. Second slice on `redesign/wpds`, after #590. --- <details> <summary>Design decisions and alternatives considered</summary> **A slot in the header, filled by the site's view.** The alternatives were to lift each site's state up to the window, or to give every site a page and header of its own. The first is a large move whose shape depends on slices not built yet; the second puts a header in every hidden view. A slot moves the markup and nothing else, and it is what the design system's own `Page` does for its sidebar toggle. **The applications are in a menu of their own, under "Open in".** The prototype has one "Open in" item naming the editor chosen in the settings. There are no settings yet, so the app offers what detection found, each by name, and always "Other application…", as the old menu did. They are not in the site's menu's own list because detection answers after the menu is on screen: the rows it adds would push "Delete site" down under a pointer aimed at something else. For the same reason "Other application…" is first in its menu, and what arrives is added under it. **"Setup" is a row of the details.** The prototype has no such row. The old view said "Initialized" or "Uninitialized" in a pill before anything else, and a site whose setup did not finish should still say so somewhere. **The details go under the cards by the page's width, not the window's.** The prototype switches at a window width of 960px. Here the sites list takes a share of the window that the page never sees, and it can be closed, so the rule is a container query on the page. **`useId` for the element the button names.** A site's path is not safe in an id, as #590 found; the id only has to tie a button to the element it shows and hides. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> Run as the repository's review procedure, with the judgement pass in a fresh context that took no part in the change. First pass on `6cb7dd5` against `3344172`; second pass on the fix-up `f1420bb`; `538bc8a` applies what the second pass asked for, and the head, `3eb1ee3`, corrects a journey after CI. **First pass: seven findings to fix here, one follow-up; one medium, none high.** It read the removed block line by line and found nothing lost outright: the setup state and the path are now visible only while the details are open, and the path can still be copied from the menu. It read the slot/fill, the design system's menu and base-ui's focus handling in `node_modules` and found the fill rendered by exactly one site at any moment, context and events reaching it, and the rename dialog taking focus cleanly from the closing menu. Fixed: - **A refused open was said off-screen** (medium). The notice is at the top of the cards and the header's menu can be used from anywhere down the page. It is brought into view when it is said. - **Rows of the menu moved under the pointer**, with "Delete site" below them, as detection answered. The applications are in a menu of their own, so the site's menu's own list never changes. - **The details did not stay in view while the cards scrolled**: the sticky rule had no room to act in. One line. - **A copy from the menu was announced twice** to a screen reader. The menu's copy leaves the details' button alone. - **Two strings chosen by branches in a component** moved to `site-details.cjs`, with unit tests. - **The translation journey could scan the menu before the application was in it.** It waits for it by name. - **The journey's header** now says what its stand-ins leave out of the main process. **Second pass: five findings to fix here, one medium.** - **"Other application…" still moved, inside the new menu** (medium), down as applications arrived and up as the "looking" row went. It is first now, with what arrives and goes below it, and a unit test walks the lists before and after an answer. - **A row lost its identity, and the focus on it, when the list changed**: its key came from its position. It comes from what the row is. - **A refusal still on the page was scrolled to again every time its site was opened.** The effect now brings each refusal into view once. No journey holds this: see below. - **The details' Copy button was itself a live region**, so its change back to "Copy" was announced too. A status region beside it holds "Copied" only while it is true. - **A comment about keys said what the code did not do.** **After CI: one journey step removed.** `538bc8a` added a step claiming that a refusal still on the page does not take the page back to itself when its site is opened again. It was red on Windows, on both attempts, and it was the step that was wrong: opening a site moves the page in any case, because the next-action cue centres the next step each time a site becomes the open one (#252). On macOS that scroll is animated and the step read the page before it had moved. The step is gone, the code's comment says whose the page's position is on opening a site, and the journey was re-run in a window the size of the Windows runner's screen. Left as a follow-up: `docs/guide/trunk-updates.md`, `editors.md` and `managing-sites.md` describe "More", "Open directory in" and the copy icon. The screenshot harness still works: its `site-menu` shot was run. **Checks, on macOS, on the head of this branch:** | Check | Result | | --- | --- | | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1774 pass, 2 skipped | | `npm run test:electron` | 1775 pass, 1 skipped | | `npm run test:e2e` (journeys) | 68 of 68 on the head | | `npm run test:e2e:packaged` | 15 of 15, on an unsigned `npm run pack:dir` build of `f1420bb`; the head differs from it in the renderer and in tests, and CI's packaged smoke passed on `538bc8a` on both platforms | **The tests' claims were audited by breaking each one** in the source and running the test that should notice. 25 breaks, all 25 turn a test red on the line that states the claim. | Break | Caught by | | --- | --- | | Every site draws its actions in the header, not only the open one | `site-header` | | The menu does not look for applications as it opens | `site-header` | | Copying from the menu says nothing | `site-header` | | "Other application" opens the first one found | `site-header` | | Showing in the file manager does nothing | `site-header` | | A refused open is not said on the page | `site-header` | | The details' button does not say it copied | `site-header` | | An old trunk does not say how old in the details | `site-header` | | Hidden details still take focus | `site-header` | | Hidden details are still in the accessibility tree | `site-header` | | The details come back when another site is opened | `site-header` | | The details' button names an element that is not there | `site-header` | | Menus are drawn under the page | `site-header` | | Renaming from the menu opens nothing | `rename-site` | | A site being deleted can be deleted again from the menu | `engine` | | Updating from the menu does nothing | `trunk-update` | | The details' heading is not translatable | `i18n` | | A menu item is not translatable | `i18n` | | A refused open is said out of sight | `site-header` | | A copy from the menu also lights the details' button | `site-header` | | The details scroll away with the cards | `site-header` | | The applications are in the menu's own list | `site-header` | | The way to choose another application is last, under what arrives | `site-header`, and `site-menu` (unit) | | A copy from the details is not said to a screen reader | `site-header` | | The copy's going back is said to a screen reader too | `site-header` | Nothing asserts that a toast paints over an open dialog, or that the terminal can be scrolled sideways: both are about what is painted, which the journeys do not read. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
#595) ## Why #557 rebuilds what is inside the site's page, as the [prototype](https://github.com/jameskoster/wordpress-contributor-toolkit-prototype) draws it. It lists nine things; this is the first two, the Server section and the process menus. Until now a site's server, its build watch and the way to its changes were three large buttons at the top of the cards, with a line of links under them, and they scrolled away with the cards. **This PR targets `redesign/wpds`, not `trunk`.** ## What changes - **The page's header has a menu for each of the site's two processes**, the development server and the build watch. Each says what its process is doing, with a dot, and holds the one thing to do about it. While the server has an address its menu also goes to the site and to its admin. Beside the two menus is the button that opens the review. - **The site's details have a section for each process.** The server's says where the site, its admin and its database are once it has an address, and what to log in with, the password shown only when asked for. The build watch's says what it is doing. Each section has a button that starts and stops its process, named as the header menu's item is. - **Both places say the same.** What they say is decided in one module, `site-processes.cjs`. - **The row of three buttons is gone**, with the line of links under it. - **A server that is starting cannot be pressed.** The button and the menu's item say it is starting and are held. Before, the button could be pressed and did nothing. - **The details tell three ends apart**: a watch that ended by itself, a build that failed before the watch could start, and a build that was stopped. The hook now records which process an end was of, and a build stopped by the watch's button or by Ctrl+C in the terminal is a stop, whatever the kill left it with. Before, a stopped build was logged as "failed with code null". Two things the taller details and the fuller header needed: - **The details stay in view only while they fit.** With the two sections they are taller than the page at the default window size, and a column held in place would keep its own end out of reach until the page's end. Taller than what is in view, they scroll with the cards, as in the prototype. - **The header's actions give way by the page's width.** Short of room, the two processes are their dots and the review button is its first word; each keeps its name. At a 1024px window the site's name was otherwise squeezed to nothing. **Deliberately not in this PR**: the other seven items of #557, each in its own PR; and the sites list's row actions and its "server running" dot. ## How to test this Platforms: any. Use the Buildkite build for the current head of this branch. The journeys carry this. From the repository root, `npm run test:e2e` runs them all: - `dev-server.spec.js`: the server's button in the details starts one server, is held while the server starts, as is the header menu's item, and offers to stop it once it has an address; a third, new journey starts and stops the same server from the header's menu, finds nothing but the start in a stopped server's menu and the site and its admin in a running one's, reads the section's three addresses, follows one from each place and finds it opened by the main process with the window still on the app, and reads the credentials, the password only after asking. - `build-watch.spec.js`: the header and the details say the same of the watch as it starts, is stopped from the header's menu, and ends by itself with an error; a build that fails before the watch is said as the build's failure; and a third, new journey stops that build by the watch's button and by Ctrl+C, and finds neither said as a failure. - `i18n.spec.js`: a fourth journey reads the two processes in the pseudo-locale, stopped, starting and running, in the header, in their menus and in the details. - `site-header.spec.js`: details that fit stay in view and details that do not scroll with the cards; a site still in its setup is offered no server, watch or review; and a header short of room, at two widths, keeps the site's name and every action under its own name. - Every journey that starts a server, a watch or the review now does it through these controls: the mail, the logs, the update with the watch running, the review. What they cannot reach is how it looks, and a real server: **Starting state:** a Core site that has finished setup, with its dev server stopped. 1. In the page's header, click "Server stopped", then "Start development server". The menu's button reads "Server starting…" with an amber dot, and in the details the Server section says how long it has been starting. 2. When it is up, the dot is green, the browser opens the site, and the Server section shows "View site", "wp-admin" and "Database", with the username under them. Click the eye beside the dots: the password shows. 3. Click "wp-admin". It opens in the browser; the app's window does not change. Click "Server running" in the header: "View site" and "wp-admin" are there too, above "Stop development server". 4. In the details, click "Start" beside "Build watch". The header's second menu reads "Build watching". 5. Make the window narrower. First the two menus become their dots, then "Review & submit changes" becomes "Review". The site's name is always there. **What must not have happened:** - A second press while the server starts does nothing: there is one server. - Stopping the server leaves the build watch running. - On a site with no build yet, such as a Gutenberg checkout before its first build, "Start build watch" and then "Stop build watch" while it still says "Build building" leaves a grey dot and "Build watch offline", not a red dot and a failed build. ## Risks and limitations - **The server's address is no longer shown as text**, only behind "View site". The prototype has it so. - **The build watch's tooltip is gone**, the one that said it compiles `src/` edits on save and runs apart from the server. The section's sentence says the first half while the watch runs. - **The next-action cue no longer scrolls to "Start dev server" or to the review button**: they are in the header, which is always in view. It still puts its glow on them and says the step for a screen reader. - **At the default window size the details are taller than the page once a site is set up**, so they scroll with the cards from the start. They stay in view only on a taller window, or while a site is in its setup. - **With the processes shown as dots, two grey dots look alike.** Each has its name as a tooltip and for a screen reader. - **How the window handles a link is unchanged**, and is tracked separately. The new links are opened as the old ones were. - **A paused watch offers "Start"**, as it always has, now beside a sentence saying it comes back by itself. Starting it during an apply is an old gap, left for its own change. - **A start that throws still leaves the button dead until the app is restarted**, and **a boot that never answers still cannot be cancelled**: the button is now held where before it could be pressed and did nothing. Neither is new. - **The guide pages still describe the three buttons.** #561. - **The Logs tab and the details can name the same end differently.** The tab reads "Build watcher (exited 1)" for a build that failed before any watcher existed, and "(stopped)" for a watch that ended with no code, which the header and the details now draw as failed. The tab's label is older than this and is not changed here. - **The time inside "Dev server is starting… (12s)" is not translatable**: the sentence is, the unit is not. - **A menu of the design system, opened again while it is still closing, can miss the Escape that follows.** Found by a journey at machine speed, where it took about one Escape in two; a second closes it, and it did not happen in 60 runs once the closing menu had gone, which takes less time than a person needs. The journey waits for the closed menu to be gone. - **About 1,160 lines added, 546 of them tests.** It is one pull request because the header's menus and the details' sections draw from one module, and the three buttons could not go until both were there. - **The manual steps above were not run by a person.** I looked at screenshots of the header and details with the server stopped and starting, and of the header at three widths. - **Checked on macOS only**, with the journeys this touches also run in a window resized to the Windows runner's 1024×728. Windows itself is CI's. - **Pre-PR review, two passes: everything marked to fix here was fixed, eleven findings, one of medium weight; six follow-ups are left**, all in this list. The medium one was a build stopped by hand shown as a build that failed. ## Related Part of #557 (items 1 and 2 of its nine) and #542. Third slice on `redesign/wpds`, after #590 and #594. --- <details> <summary>Design decisions and alternatives considered</summary> **"Build watch", not "Latest build".** The prototype's section reports the last compile: whether it succeeded, how long ago, how long it took, which files. The app does not record any of that; it knows the watch's state and what the watcher printed. So the section is named for what it is and says the state in a sentence. Recording compiles is a feature of its own. **One module for both places.** The header and the details show the same two processes. Two sets of conditions would drift, and the first sign would be a header saying "running" over a section saying "offline". **A starting server is held, not pressable.** The prototype draws it so. The guard that made a second press do nothing (#488) stays in the hook, but no press reaches it any more, so the journey's claim is the held button and the held menu item, and one server counted. **The header's server menu has the site and its admin in it.** The prototype's menu has only start and stop. The details can be put away, and under about 1088px of window they sit at the foot of the page; the two links most often wanted should not go with them. The database stays in the details. **The processes and the review are not offered during setup.** The old row of buttons was not drawn then either: the checklist's last step is what starts the server the first time. **The details let go by measuring, not by a rule in the stylesheet.** Whether they fit depends on the window's height and on what they are showing, which changes as a server starts. The prototype measures too. **The header gives way by the page's width, with container queries.** The page's width is the window's less the sites list, which can be closed, so a rule on the window's width would be wrong half the time. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> Run as the repository's review procedure, with the judgement pass in a fresh context that took no part in the change. First pass on `6c6cc59` against `f066f94`; second pass on the fix-up `ac56e03`; the head, `e8dee93`, applies what the second pass asked for. **First pass: seven findings to fix here, five follow-ups; none high.** It confirmed that the disabled states, the next-action cue, the setup checklist's own start step and the way out of an update that is waiting for the watch all carried over from the three buttons. Fixed: - **A watch that ended by itself with code 0, or killed by a signal, showed as a plain grey "stopped".** The old dot was red for every end nobody asked for. Every such end is drawn as failed, with the code when there is one. - **A build that failed before the watch could start was blamed on the watch**, which had never started. The hook records which of the two ended. - **What the server's section shows was decided in `index.jsx`**, out of the unit tests' reach. It is in `site-processes.cjs`. - **The journey's double click no longer reached the guard it named** (#488): the second click landed on a held button. The journey now claims what holds: the button and the menu's item are held while the server starts. - **"Dev server is starting…" was not translatable**, and the translation journey scanned none of the new parts. The string is wrapped and a fourth journey scans the two processes in three states. - **The check that the window stays on the app could pass after a link had navigated**: it read the address too early. It asks the main process, after a round trip. - **The header's process menus were reached from three journey files**, each with its own locator. One helper in `ui.cjs`. One follow-up was done here instead: the site's links were only in the details, which can be put away. "View site" and "wp-admin" are in the header's server menu. **Second pass: four findings to fix here, one medium; three follow-ups.** It traced every write of the watch's state and found that the record of which process ended cannot go stale, and that the server's address, "running" and "starting" are set and cleared together, so the section cannot be drawn from inputs that disagree. Fixed: - **A build stopped by hand was shown as a build that failed** (medium): a red dot and "the build … failed". A killed process ends with no code, and the first fix-up had made every unasked-for end a failure. The build's end is left alone when a stop has already said the watch is idle, and Ctrl+C says so before it kills. The new journey is red without either half. - **Which links the header's menu is given was decided in `index.jsx`.** It is in `site-processes.cjs`, with a unit test. - **The dev server journey's header still described a menu with one thing in it**, and **a count left over from the double click could not fail.** Rewritten and removed. One follow-up was done here: the translation journey now waits for the server and the watch to have been asked for before it speaks for them. Left as follow-ups, all in Risks above: a paused watch offering "Start"; a start that throws leaving the button dead; a boot that cannot be cancelled; how the window handles a link; the Logs tab's label; and the time's unit. **Checks, on macOS, on the head of this branch:** | Check | Result | | --- | --- | | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1793 pass, 2 skipped | | `npm run test:electron` | 1794 pass, 1 skipped | | `npm run test:e2e` (journeys) | 73 of 73 | | `npm run test:e2e:packaged` | 15 of 15, on an unsigned `npm run pack:dir` build of the head | | `dev-server`, `build-watch`, `site-header`, `i18n` in a 1024×728 window, the Windows runner's | pass | **The tests' claims were audited by breaking each one** in the source and running the test that should notice. 30 breaks, each run on the head, and all 30 turn a journey red on a line that states the claim. | Break | Caught by | | --- | --- | | The header's server menu does nothing | `dev-server` | | A link of the server's section is not opened by the main process | `dev-server` | | The password is shown without being asked for | `dev-server` | | A server that is starting can be pressed | `dev-server` | | The header says the server is stopped whatever it is doing | `dev-server` | | The server's section has no links | `dev-server` | | The header's build menu does nothing | `build-watch` | | The details do not say what the watch watches | `build-watch` | | A watch that ended does not say with what code | `build-watch` | | A build that failed before the watch is said as a watch that ended | `build-watch` | | A build stopped by the watch's button is said to have failed | `build-watch` | | Ctrl+C on that build is not taken for a stop | `build-watch` | | The header's server menu has no links | `dev-server` | | The header's server menu has the database too | `dev-server` | | A link in the header's menu opens nothing | `dev-server` | | A stopped server's menu offers a link | `dev-server` | | A link takes the window with it | `dev-server` | | The starting sentence is not translatable | `i18n` | | A process menu's item is not translatable | `i18n` | | A link's name is not translatable | `i18n` | | The credentials' heading is not translatable | `i18n` | | The header's name for a process is not translatable | `i18n` | | The sentence for a watch that ended is not translatable | `i18n` | | Details taller than the page stay held in place | `site-header` | | Details that fit scroll away | `site-header` | | The processes keep their words however narrow the header | `site-header` | | The review button keeps all its words however narrow | `site-header` | | The header's review button does nothing | `review-changes` | | An update of trunk does not hold the server's button | `trunk-update-watch` | | A site in its setup is offered the server and the review | `site-header` | </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
## Why #557 rebuilds what is inside the site's page, as the [prototype](https://github.com/jameskoster/wordpress-contributor-toolkit-prototype) draws it. This is item 3 of its nine, the ticket card: where a site links the Trac ticket or the GitHub issue it is working on, and reads what already exists on it. It was about 300 lines of inline markup in the middle of the site's view, in the old look. **This PR targets `redesign/wpds`, not `trunk`.** ## What changes - **The card is drawn with the design system**, as the prototype has it: a card with a title, "Trac ticket" or "GitHub issue"; with nothing linked, a field, its button and the way to find a first ticket; with one linked, the ticket's number and summary as the way to it, what Trac said of it, and under that its pull requests and, on Trac, its attachments, each as a list hanging from its heading. - **It is a component of its own**, `ticket-card.jsx`, and what it says is decided in `ticket-card.cjs`: its words for a ticket and for an issue, what Trac said as a badge and a line of facts, each row, and what each list says besides its rows: reading, empty, or failed and with what in its place. It has unit tests of its own. - **Its strings are translatable.** A ticket and an issue get whole sentences each; before, one sentence had the noun put into it. - **A button that is held still says why**, in the design system's button: `ReasonedUiButton`, beside the old one, which the rest of the page still uses. - **The two "Refresh" buttons are named for their lists**: each is its word and then its section's heading, as both are on screen. Each shows the one word. - **The notice that trunk has moved is still said to a screen reader.** The old block was a status region. The design system's notice says what is in it, and with a held button in it that reading fails and says nothing, so it is told its sentence. - **One sentence named a button that was not there.** After Trac's window was closed the card said to click "Show Trac attachments", which is gone once a read has answered. It says "Refresh". **Deliberately not in this PR:** - the pull request's banner, the note about uncommitted changes, and what a link or a switch says back: they are made by the site's view, placed in the card, and keep their old look until items 4 and 9; - the "Apply a patch or PR" card under it (item 4) and the list of a site's other tickets (item 5). ## How to test this Platforms: any. Use the Buildkite build for the current head of this branch. The journeys carry this. From the repository root, `npm run test:e2e` runs them all; `tests/e2e/journeys/ticket-card.spec.js` is new: - with nothing linked the card asks for a ticket and its button waits for one to be typed; the way to find a first ticket, the ticket's number, its component, a pull request's title and an attachment's name each open their page through the main process; - linking reads the pull requests and the ticket once each with nothing more pressed; the card then says the ticket's summary, status and facts, each pull request's number, state and date, and each attachment's uploader, age and size; - a list that could not be read says why and keeps the rows it last had, or says there is nothing to fall back on; an empty one says it is empty; Trac's window closed early is said as that, and not as "no attachments"; - a site opened with a ticket already linked does not go to Trac until asked, and offers to in both places; asked once, it fills in both; - while a build runs, Unlink, the field and Link are held, say why as their description, can still be reached, do nothing when pressed, and come back as the same buttons. `ticket-rebase.spec.js` now also holds that the notice about trunk having moved is said, by recording what the page's live regions say. `i18n.spec.js` gains the card in the pseudo-locale, with nothing linked, with a ticket and its lists, and with a failed and an empty list. `pr-checkout`, `ticket-branches`, `gutenberg-site` and `open-pull-request` now find the linked ticket by its number in the card. What they cannot reach is how it looks, and the real Trac: **Starting state:** a Core site that has finished setup, with no ticket linked. 1. In "Trac ticket", type `62281` and click "Link ticket". Trac's window opens for its human-check; pass it. The card shows `#62281` with the ticket's summary, a badge with its status and a line of facts, then "Linked pull requests" and "Trac attachments", each row with a tick to the rule down its side. 2. Click the ticket's number. The ticket opens in the browser; the app's window does not change. 3. Click "Refresh" beside "Linked pull requests" with the network off. A yellow notice says GitHub could not be reached and, under that, when the list on screen was read; the rows stay. **What must not have happened:** - Opening the app with a ticket already linked does not open Trac's window by itself. - "Unlink" during a build or an update does nothing, and says why on hover. ## Risks and limitations - **The explicit "Open in Trac" button is gone.** The ticket's number and summary are the way to it, with "Open in Trac" as the tooltip, as the prototype has it. A newcomer has to recognise a link. - **"Unlink" is no longer red.** It is a plain button in the card's header, as in the prototype. It parks the ticket's work and loses nothing, which red overstated. - **The linking prompt is shorter.** It no longer says the ticket is stored with the site, survives restarts and can be changed or removed. The prototype's sentence is "Link the ticket you're working on." - **Three things inside the card keep the old look**: the checked-out pull request's banner, the note about uncommitted changes, and what a link or a switch says back. They are the site's view's and are redrawn with items 4 and 9. - **"Apply…" and "Show Trac attachments" are held during a build, an install, an update or an apply with no stated reason**, as before; held buttons of the design system can now be reached by keyboard, so the missing reason is met more often. Several "Apply…" buttons share one name, as before. Both go with item 4, which redraws the apply. - **Announcements can overwrite one another.** Right after a link, "Linking ticket", "Checking GitHub" and "Reading the ticket on Trac" are put in the one live region in quick succession; and a refresh that fails twice with the same sentence is said once. The first is how the design system's loading buttons speak. - **The sentence the stale-ticket notice speaks is its two sentences joined with a space**, and those two, with every reason a button is held for, are not translatable yet: they come from modules this PR does not touch. The translation journey does not reach that state. - **For a moment after a build ends, "Unlink" can be pressed and is turned away** with "A command is already running": the card's buttons come back before the terminal does. It is older than this card, and longer on a slow disk; CI's Windows runner found it. Left for its own fix on `trunk`. - **Section headings are as large as the card's title**, as in the prototype. - **The guide pages still show the old card.** #561. - **About 1,475 lines added, 680 of them tests**, against about 360 removed. It is one pull request because the card is one block of the site's view: it could not be half moved. - **The manual steps above were not run by a person**: they need Trac's human-check. I looked at screenshots of the card with nothing linked, with a ticket and its lists, with a failed read of each list, and in a 1024px window. - **Checked on macOS only**, with the journeys this touches also run in a window resized to the Windows runner's 1024×728. Windows itself is CI's. - **Pre-PR review, two passes: everything marked to fix here was fixed, nine findings, two of medium weight; nine follow-ups are left**, all in this list or in the review outcome. The medium two were a notice that was no longer said to a screen reader, and a journey whose stand-ins could be late. ## Related Part of #557 (item 3 of its nine) and #542. Fourth slice on `redesign/wpds`, after #590, #594 and #595. --- <details> <summary>Design decisions and alternatives considered</summary> **The ways out are buttons drawn as links.** The prototype has anchors. They were buttons in the old card, and the main process opens the page; an anchor would also be something the window itself could follow. **The heading is the way to the ticket.** The old card had the number in a pill and an "Open in Trac" button beside it. The prototype makes the number and the summary the link; "Open in Trac", or "Open on GitHub", is its tooltip. **"Milestone:" is labelled.** The prototype's line reads "Defect (bug) · General · 7.2 · Opened 4 weeks ago". A real ticket's milestone is as often "Awaiting Review" or "Future Release", which beside a status badge reads as a second status. **"Apply…" keeps its dots.** The prototype says "Apply" and opens a dialog. Here the button reads the patch into the card below, which item 4 redraws; the dots say that it does not apply yet. **A closed pull request has no colour, a merged one is blue.** The old pill used GitHub's own three colours. The design system's badge has no purple, and its red means that something failed, which a closed pull request has not (#227). **"Read details from Trac" stays**, where the prototype has no such button: it always has the details. Reading them opens Trac's window and may ask for a human, so the app does it once after a ticket is linked and afterwards only when asked. **`pr-state.cjs` stays**, and the card's badge takes from it which of the three a state is. The apply's preview still draws its old pill from there, until item 4. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> Run as the repository's review procedure, with the judgement pass in a fresh context that took no part in the change. First pass on `fde8836` against `5388e5d`; second pass on the fix-up `9b7a6ef`; `86bd6b5` applies what the second pass asked for, and the head, `4b0f828`, corrects a journey after CI. **First pass: five findings to fix here, eight follow-ups; two medium, none high.** It compared the removed block with the new card line by line and found every gate unchanged: when "Apply…" is hidden and held, when each "Refresh" and "Show Trac attachments" show, what holds Unlink, Link and the field, the Enter key, the next-action cue, and the spacing of what is handed into the card. Fixed: - **The notice that trunk has moved was never said** (medium). The design system's notice says its content by rendering it to a string, the held button in it cannot be rendered that way, and the failure is swallowed. The notice is told its sentence, and `ticket-rebase.spec.js` holds that it is said, red without the fix. - **The journey that reopens a site with a ticket linked installed its stand-ins after the window existed** (medium), so the first read of the pull requests could go to GitHub and a read of Trac made early would not be counted. A session can now run a journey's code before its window is waited for, and the journey checks that its stand-in answered that read. - **"A press on a held Unlink does nothing" could not fail**: the journey read the branch before a press that got through could have moved it. It now reads what the card would say at once if the press had got through. - **Two controls were named in two journey files** and are in the shared helpers; `TESTING.md` no longer counts them. - **Translation**: two sentences that quote a button now say so to translators; a failed read is two lines and not two sentences joined with a space, and so is the way to a first ticket; each Refresh is named from the two strings on screen, so its name cannot disagree with what it shows. Three follow-ups were done here too: the ticket field's helper matches a whole label; the registry's unused browse label is gone; a held button's reason is no longer also read as a sentence of its own. And the component is labelled "Component:", which following the prototype had dropped. **Second pass: four findings to fix here, three follow-ups; all low.** It read the design system's button, tooltip and notice and confirmed the names, the description and the spoken sentence reach the page as intended, and that the fourth journey's premise holds: a press that got through raises an alert in the card before anything is asked of the main process. Fixed: - **A count in that journey could not tell the two cases apart**, for the reason above: removed, with the wrap that fed it. - **The rebase journey's record of what was said kept only the last of two messages put in a region in one turn.** It keeps what each change put there. - **Two comments named one of the two places the registry's label is read.** - A comment line ran long. Two follow-ups were done here: a session checks its throwaway profile before a journey's code runs against the app, and the attachments' failure is told to a screen reader as written, since read out of its markup an "&" in an error would be said as "&". Left as follow-ups, all in Risks above except the last two: no stated reason on "Apply…" and "Show Trac attachments"; several "Apply…" with one name; announcements overwriting one another; the stale notice's spoken sentence joined in the component, to move to its module when that module is translated; heading sizes; the shorter prompt; the guide pages; a few small decisions still in the component (when a row's button is held, when "Refresh" shows); and two notices that still say what is in them the default way, which works for what they hold. **After CI: one journey corrected.** The journey that holds the card's buttons during a build was red on Windows, on both attempts, at the Unlink it presses once the build has ended. When a build ends the card's buttons come back at once and the terminal a moment later, after the site's status has been read again; a press in between is turned away with "A command is already running". On macOS that read is over before the journey presses, and on the Windows runner it is not. The journey now waits for the terminal's hint to be a link again. The failure was reproduced here by slowing the status read by a second and a half, and the corrected journey passes with it slowed. The gap itself is the app's and older than this card: see Risks. **Checks, on macOS, on `86bd6b5`; the head differs from it in that one journey, which was run again on the head, also in the small window:** | Check | Result | | --- | --- | | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1811 pass, 2 skipped | | `npm run test:electron` | 1812 pass, 1 skipped | | `npm run test:e2e` (journeys) | 78 of 78 | | `npm run test:e2e:packaged` | 15 of 15, on an unsigned `npm run pack:dir` build of the head | | Eight journey files that reach the card or restart the app, in a 1024×728 window, the Windows runner's | 28 of 28 | **The tests' claims were audited by breaking each one** in the source and running the test that should notice. 36 breaks, and all 36 turn a journey red on a line that states the claim. 34 were run on the head; two, marked below, on the commit before it, whose lines the head does not touch. One claim is not shown red: that the reopened site's first read of the pull requests was answered by the stand-in. Making the stand-in late enough to show it would send that read to GitHub. | Break | Caught by | | --- | --- | | The way to find a first ticket goes nowhere | `ticket-card` | | An empty field can be linked | `ticket-card` | | The ticket's number goes nowhere | `ticket-card` | | The ticket does not say where it goes | `ticket-card`, `gutenberg-site` | | What Trac said of the ticket is not shown | `ticket-card` | | When the ticket was opened has no date behind it | `ticket-card` | | The component is not a way to its tickets | `ticket-card` | | "Read details" stays after the ticket has been read | `ticket-card` | | A pull request does not say its state | `ticket-card` | | A pull request does not say when it last moved | `ticket-card` | | A pull request's title goes nowhere | `ticket-card` | | An attachment does not say who uploaded it | `ticket-card` | | An attachment's name goes nowhere | `ticket-card` | | A patch cannot be read from its row | `ticket-card` | | A list that could not be read says nothing | `ticket-card` | | A list that could not be read loses its rows | `ticket-card` | | A failed read does not say there is nothing to fall back on | `ticket-card` | | A failed read says why and not what is shown instead (on `9b7a6ef`) | `ticket-card` | | An empty list says nothing | `ticket-card` | | A read Trac did not answer is said as "no attachments" | `ticket-card` | | A failed read of the attachments says nothing | `ticket-card` | | An unread ticket offers no way to read its attachments | `ticket-card` | | A Refresh button is named by its word alone (on `9b7a6ef`) | `ticket-card` | | A held button does not say why | `ticket-card` | | A held button cannot be reached | `ticket-card` | | A press on a held Unlink gets through | `ticket-card` | | The field is not held during a build | `ticket-card` | | The stale-ticket notice is not told what to say | `ticket-rebase` | | The checked-out pull request's banner is under the ticket's number | `pr-checkout` | | A checked-out pull request can still be applied from its row | `pr-checkout` | | A Gutenberg site's card is worded for Trac | `gutenberg-site` | | A Gutenberg site's card has Trac's attachments | `gutenberg-site` | | A section's heading is not translatable | `i18n` | | A fact's label is not translatable | `i18n` | | A state's word is not translatable | `i18n` | | A failure's sentence is not translatable | `i18n` | The audit also changed the code once: a prop on the held button turned out to do nothing the design system's button does not already do, and was removed. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…dialog (#597) ## Why #557 rebuilds what is inside the site's page, as the [prototype](https://github.com/jameskoster/wordpress-contributor-toolkit-prototype) draws it. This is item 4 of its nine, apply patch and PR checkout: the card a pull request or a patch file is brought into the checkout from, the preview of what one would change, and the banner of a pull request that is checked out. It was about 250 lines of inline markup in the site's view, in the old look. **This PR targets `redesign/wpds`, not `trunk`.** ## What changes - **The apply card is drawn with the design system**, as the prototype has it: a card that can be folded, with a tab for each way in on a project that takes patch files, "Pull request" and "Diff", and the pull request's field alone on one that does not. - **What a patch would change is shown in a dialog** before it is applied, where it was a block in the card: its files, whose work it would land on, anything that changes what applying it means, and the button that applies it. - **The applied patch, a failure with its breakdown, and the checked-out pull request are the design system's notices.** The ones that were alerts or status regions still are. - **It is a component of its own**, `apply-card.jsx`, with what it decides in `apply-card.cjs` and unit tests beside it. Its own strings are translatable. The dialog is in front of the page, and that changed four things about the flow: - **A failed apply drops its preview.** It used to stay on the card beside the error. A dialog would come back in front of the error, so the error is read on the card and the patch is chosen again to retry. - **The preview steps aside while the app asks what should become of loose edits on trunk**, and is kept for the answer that goes on with the checkout. Saying "Cancel" to that question drops it. - **A preview is shown by its own site.** Every site's view is in the window at once, and one read while the contributor had gone to another site would have opened over that one. It waits for its site. - **The dialog's button is held, with the reason, while a command runs in the terminal.** An apply is refused then with a line in the terminal, which the dialog is in front of. Also: - **The card stays open while it has something to say** (the steps of an apply, a patch that is applied, a failure) whatever was chosen, and goes back to that choice afterwards. It remembers which tab it was left on. - **The screenshot harness waits for the ticket's number**, where it waited for a heading that #596 removed. That was missed there. **Deliberately not in this PR:** - translating what the card passes through from older modules: what a pull request's preview and banner say, what an applied patch says, why a patch did not fit, and the steps of an apply; - the list of a site's other tickets under the card (item 5), and the note about uncommitted changes (item 9). ## How to test this Platforms: any. Use the Buildkite build for the current head of this branch. The journeys carry this. From the repository root, `npm run test:e2e` runs them all; `tests/e2e/journeys/apply-card.spec.js` is new: - a patch that was chosen is shown in a dialog named for it, with what it changes, and nothing is written; leaving the dialog by its button or by Escape applies nothing and gives the card back on the tab it was left on; applied from the dialog, the patch is on disk, and focus goes to the card's header; - a patch that does not fit says before anything is written whose work it would land on; refused, its preview goes, the folded card opens to say what went wrong, the ticket's rows can be read again, and once the failure is dismissed the card is folded again; - a preview read for one site is not shown over another, and is there when its own site is opened again; - a patch cannot be applied while a command runs in the terminal, and the dialog's button says why; the command over, the same dialog can apply; - saying "Cancel" to the question about loose edits leaves the checkout and the edit as they were and does not bring the preview back. `i18n.spec.js` gains the card and a patch file's preview in the pseudo-locale. `patch-apply`, `pr-checkout`, `watch-during-changes`, `review-changes`, `store-persistence`, `gutenberg-site` and `terminal` walk the same flows as before, now through the tabs and the dialog. What they cannot reach is how it looks, a real pull request from GitHub, and an install that takes minutes: **Starting state:** a Core site that has finished setup, with a ticket linked. 1. In "Apply a patch or PR", type a pull request's number and click "Apply PR". A dialog opens, titled "Apply PR #…", with the files it changes. Click "Cancel": the dialog closes and nothing has changed. 2. Click the "Diff" tab, then "Choose a .diff or .patch file…", and choose a patch. The dialog is titled for the file. Click "Apply and rebuild": the dialog closes and the card shows the steps, then a green notice with "Revert this patch". 3. Click the card's header. It does not fold while the notice is there. Click "Revert this patch", and once it is gone click the header again: it folds. **What must not have happened:** - A patch that does not apply leaves no dialog in front of the error. - Nothing is written to the checkout before "Apply and rebuild" is pressed. ## Risks and limitations - **A failed apply cannot be tried again from the same preview.** The patch is chosen again; a pull request that was typed is typed again, since the field is cleared when it is read. - **Much of what the card says is not translatable yet**: the sentences it passes through from `pr-checkout.cjs`, `applied-layer.cjs`, `apply-conflict.cjs`, `watch-activity.cjs` and `update-plan.cjs`. Two notices join two of those sentences with a space. The translation journey covers the card with nothing applied and a patch file's preview. - **In a breakdown, why a change did not fit is on its own line**, lowercase and without a full stop: it was written to follow "near … ·". - **While the question about loose edits is up, the spoken "next step" still says to apply or cancel**, though neither button is on screen. And the next-action ring is not drawn round the dialog, which is in front of everything already. - **Linking a ticket while that question is up clears it, and the preview comes back.** Only "Cancel" drops it. - **A preview that arrives while another dialog is open on the same site**, the review or the create-site dialog, opens on top of it. - **After a pull request is checked out the card is gone, and focus with it.** The focus fix holds for a patch file, and for a pull request while its apply runs. - **During an update, an install or a build the dialog's button is held with no stated reason**, as the old one was. The reason is given when something holds the terminal. - **The card's heading is inside its header's button**, as the design system's collapsible card has it. The card is a region named by that heading; moving by headings may not find it. - **Whether the card is folded, and its tab, are not remembered across restarts.** That belongs with the settings (#559). - **The guide pages still show the old card.** #561. - **About 1,400 lines added, 560 of them tests**, against about 410 removed. One pull request, because the card, its preview and the pull request's banner are one block of the site's view and one flow. - **The manual steps above were not run by a person.** I looked at screenshots of the card with nothing applied, on each tab, of the dialog for a patch file and for a pull request, of the steps, of an applied patch, of a failure with its breakdown, and of a checked-out pull request. - **Checked on macOS only**, with the journeys this touches also run in a window resized to the Windows runner's 1024×728. Windows itself is CI's. - **Pre-PR review, two passes: everything marked to fix here was fixed, five findings, two of medium weight; the follow-ups left are in this list.** The medium two were a preview that could open over another site, and an apply button that could do nothing with its reason out of sight. ## Related Part of #557 (item 4 of its nine) and #542. Fifth slice on `redesign/wpds`, after #590, #594, #595 and #596. --- <details> <summary>Design decisions and alternatives considered</summary> **The patch file is chosen with a button, not the prototype's file input.** A page is not told where a file is, and the app reads the patch in the main process. The button opens the system's own dialog, as before, under the "Diff" tab. **"Apply PR" and "Apply and rebuild" keep their words on Gutenberg**, where the prototype says "Check out PR" and "Check out and rebuild". The app's sentences about a pull request ("PR #7 is applied", "Revert this PR") come from modules this PR passes through; changing two buttons alone would leave the two vocabularies side by side. **The description is the prototype's**, "Test changes in this checkout. Your own changes are preserved." The old sentence, that a pull request is checked out with its author's commits and a patch file is applied as a removable layer, is the line of help under each tab. **A failure drops the preview rather than showing the failure in the dialog.** A failure can carry a breakdown by file and place, and up to four ways on. That is the card's to hold, and it was already drawn there. **The steps stay on the card.** The prototype's dialog shows a spinner on its button until the apply is done. An apply here can install and build, for minutes, and the card's list says which step it is on while the page stays usable. **Notices are told to say nothing by themselves.** The design system's notice reads its own content out, and #596 found that reading can fail silently. The ones here that were alerts or status regions keep those roles, which announce them; the two that said nothing before, the applied patch and the checked-out pull request, still say nothing. **`pr-state.cjs` is gone.** Its last use was the old pill in the preview; the badge's rule lives with the work-item card's since #596. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> Run as the repository's review procedure, with the judgement pass in a fresh context that took no part in the change. First pass on `ceba525` against `d5bbd47`; second pass on the fix-up `cab185f`; the head, `df655d2`, applies what the second pass asked for. **First pass: four findings to fix here, six follow-ups; two medium, none high.** It compared the removed block with the new components line by line and found every line of the applied patch's and the checked-out pull request's banners, the failure's buttons and their gates, and the alert and status roles carried over. It traced every reader of the preview after a failed apply and found none that needed it, and no path on which the dialog goes without an error or a question being shown. Fixed: - **A preview could open over another site** (medium). Every site's view renders its own dialog, and a dialog is drawn over the whole window: one read while the contributor had gone elsewhere opened there, with a button that applied to the site out of view. A preview is shown by its own site. - **"Apply and rebuild" could do nothing, with the reason behind the dialog** (medium). The refusal while something holds the terminal is older than the dialog, and was a line on the same page. The button is held and says why. - **Two decisions this change made were in the components**: when the dialog shows and when the card is open. They are in `apply-card.cjs`, with unit tests. - **Translation**: "Near" and the line it introduces are one sentence a translation can reorder; "Diff" says to translators that it is a noun; a string that was extracted and never shown is gone. Follow-ups done here too: focus goes to the card's header after an apply, where it fell to the top of the window; a place in a breakdown is keyed by where it comes; a weak assertion was moved and a helper widened; and "Cancel" on the question about loose edits drops the preview, where it would have brought the dialog back. **Second pass: one finding to fix here, five follow-ups; all low.** It read base-ui's handling of where focus goes and confirmed it always lands on the card's header and never on a field, that the reason and the terminal's lock are written together, and that the three new journeys read their absences after the page has rendered. Fixed: - **The reason named the terminal as where a command was running**, and not everything that holds the terminal prints there. It says something else is running on the site, and how to stop it. Follow-ups done here too: a dialog left without applying gives focus back to its opener whatever was pressed in it; the two-site journey waits for a positive sign before it reads that no dialog is shown; the helper that finds a way to a patch file no longer matches two unrelated buttons; an assertion that could not fail is gone. Left as follow-ups, all in Risks above: the breakdown's wording; the spoken next step and the ring during the question; linking a ticket during the question; a preview over another dialog of the same site; focus after a pull request's checkout; no stated reason during an update, install or build; and retrying from the same preview. **Checks, on macOS, on the head of this branch:** | Check | Result | | --- | --- | | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1827 pass, 2 skipped | | `npm run test:electron` | 1828 pass, 1 skipped | | `npm run test:e2e` (journeys) | 84 of 84 | | `npm run test:e2e:packaged` | 15 of 15, on an unsigned `npm run pack:dir` build of the head | | Nine journey files that reach the card, in a 1024×728 window, the Windows runner's | 34 of 34 | **The tests' claims were audited by breaking each one** in the source and running the test that should notice. 27 breaks, each run on the head, and all 27 turn a journey red on a line that states the claim. | Break | Caught by | | --- | --- | | A pull request can be asked for with nothing typed | `apply-card` | | The preview is not named for its patch | `apply-card` | | The preview does not say how much the patch changes | `apply-card` | | The preview does not list the files | `apply-card` | | Leaving the preview does not drop it | `apply-card` | | The card forgets the tab it was left on | `apply-card` | | The card cannot be folded | `apply-card` | | A folded card stays folded over a failure | `apply-card` | | A failed patch keeps its preview | `apply-card` | | The preview does not say whose work the patch lands on | `apply-card`, `patch-apply` | | A failed patch does not say the checkout was left alone | `apply-card`, `patch-apply` | | A failure cannot be dismissed | `apply-card` | | A preview opens over whichever site is on screen | `apply-card` | | The dialog's button is not held while something holds the terminal | `apply-card` | | Saying no to the question leaves the preview | `apply-card` | | Focus is left on nothing when the dialog closes on an apply | `apply-card` | | The preview is in front of the question about loose edits | `pr-checkout` | | Reverting a pull request does nothing | `pr-checkout` | | The failure is not an alert | `patch-apply` | | Reverting a patch does nothing | `patch-apply` | | A Gutenberg site is offered a patch file | `gutenberg-site` | | The pull request field is not held during a build | `terminal` | | The card's title is not translatable | `i18n` | | A tab's help is not translatable | `i18n` | | The preview's button is not translatable | `i18n` | | The preview's way out is not translatable | `i18n` | | The preview's sentence is not translatable | `i18n` | The audit also changed the code once: the first version of the focus fix did nothing, which the journey written for it showed, and it was redone through the dialog's own option for it. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
## Why Part of the redesign in #542, item 5 of #557: the list of a site's tickets. It was the last card in the site's view still drawn by hand, with fixed colours and English written into the markup. The prototype has no such list anywhere: its site holds one ticket. So nothing is taken from it here and nothing is taken away. The list stays where it is, does what it did, and is redrawn. ## What changes The list is a component of its own, `TicketListCard`, drawn with the design system: a card that is a named region, with one row for each ticket. - **A row** says the ticket's number and when it was last worked on, and has the same two actions as before: go back to it, and delete its work. - **Going back** is **Switch** while a ticket is linked and **Continue working** while none is. The sentence "You also have work on #N — switch" is gone; the card's heading, "Other tickets on this site", says it once for all rows. - **Deleting** asks the same question first, and the row being deleted shows it is busy. - **A held button says why**, as before, through the same button the ticket card uses. - **Each button is named for its row**: the words it shows and then the ticket's number, "Switch #60001". Before, every row's delete button had one name. - **The card's words are translatable**, in whole sentences for a ticket and for an issue. They are in `ticket-branch-list.cjs`, where the rows were already decided. Not in this pull request: - The confirmation is still the system's own dialog. A dialog of the design system comes with item 7 of #557, the rename and delete dialogs. - The delete button is not red. The design system's button has no destructive tone, and the other cards draw theirs the same way. - What the ticket card says back after a switch (the refusal, the progress line, the question about loose edits) is not redrawn here. ## How to test this Platforms: any. The change is in the renderer only. The journeys walk the list on both platforms. From the repository's root: ```bash npm run build:once && npx playwright test --project=journeys ticket-list ticket-branches i18n ``` `ticket-list.spec.js` is new, five journeys: what the card lists and what its buttons are called, in both states; the buttons held with their reason while the site builds; saying no to the question; the row that is being deleted; and the same card on a Gutenberg site. `i18n.spec.js` gains a scan of the card in the pseudo-locale. Each claim was checked by breaking it: 31 breaks, all red. What a journey cannot judge is how it looks. By hand, on the current head, on either platform: **Starting state:** a Core site that is set up, with no ticket linked. 1. In **Trac ticket**, link `60001`, then **Unlink**, then link `60002`. - Below **Apply a patch or PR** there is a card headed **Other tickets on this site**, with one row: **#60001**, "Edited just now", **Switch** and **Delete this ticket's work**. 2. Narrow the window until the row's buttons no longer fit beside the number. - They move under it. Nothing is cut off and nothing scrolls sideways. 3. Type `npm run build` in the terminal and, while it runs, put the pointer on **Switch**. - The button is greyed and a tooltip says "Wait for the build to finish." 4. When the build has ended, click **Unlink**. - The card is headed **Your tickets on this site** and has two rows, #60002 first. Each offers **Continue working**. 5. Click **Delete this ticket's work** on #60001 and answer **Cancel**. - The row is still there. 6. Click it again and answer **OK**. - The row goes. #60002 is still listed. **What must not have happened:** - Step 5 deleted nothing: `ticket/60001` is still a branch of the site. - Step 6 deleted only what was asked for: `ticket/60002` is still a branch, and the site is on trunk. - A site with no parked tickets shows no card at all, not an empty one. ## Risks and limitations - **Review:** two passes by a separate context. 3 to fix here on the first, all fixed; none on the second. Details below. - **Windows failed once on this pull request, and the cause was this change.** The journeys know a switch has ended by the linked ticket's number appearing, and looked for it by its text anywhere on the page. The redrawn row shows a parked ticket's number as text of its own, so `ticket-rebase.spec.js` was answered by the row it had just pressed and asked for a trunk update while the switch still held the terminal. macOS was fast enough to hide it. The helper now looks for the number in the card's heading, a journey pins that a row does not answer it, and `ticket-rebase` waits for the terminal to be free. Reproduced, and checked fixed, by slowing the site's status read by a second: the five journey files that use the list pass that way, 20 of 20. Otherwise checked on macOS only. - **The manual steps above were not run by a person.** I looked at screenshots of both states and of a narrow window. - **The reason on a held button is not translated yet.** It comes from `ticket-actions.cjs`, which the ticket card shares, and it still puts the noun into the sentence. The pseudo-locale scan does not see it, because nothing is held while it scans. - **Focus falls to the page when a row's own action removes the row**: after a delete, and after a switch makes the row's ticket the linked one. It did before this change too. - **A reasoned button's `id` now goes to its tooltip's trigger.** This touches the button the ticket card and the apply card use. Neither passes an `id`, so nothing changes for them; their journeys pass. - **The guide still describes the old row**, and its screenshot shows it: `docs/guide/ticket-branches.md`. That is #561. - **"Edited on" a date** still writes the date the way the computer does, not the way the app's language does. ## Related Part of #557. Part of #542. Follows #597. --- <details> <summary>Design decisions and alternatives considered</summary> - **Short words on the buttons, the number in the name.** "Continue working on #60001" on every row repeats the number the row already shows. The button shows what it does, and its name is that and then the row's number, by `aria-labelledby`, so that the name begins with what is seen in any language. The ticket card names its two **Refresh** buttons the same way. - **Two words for going back, as before.** With nothing linked a row is where work starts again; with a ticket linked it is a change of ticket. The app made that difference before and it is kept. - **A flat list with a rule between rows**, not the tree the ticket card draws for pull requests: those hang off a ticket, these are peers. - **The ticket's number is not a link.** A way out to Trac from each row would be new, and this pull request adds nothing the list did not do. - **`provider` in place of `noun`.** The module was handed "ticket" or "issue" and put it into a sentence. It now picks a whole sentence, which is what a translator needs. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 3 [fix here] · 3 [follow-up] on the first pass, all 3 fixed. 0 · 0 on the second. - **Review:** completed — a separate agent context, against `.github/instructions/code-review.instructions.md`; first pass on the staged change over base `74f3c231272d226a410d6d0d0309938c24a48f5d` (it became `6bd89d1`), reading the diff and running lint and the list's unit tests, not the journeys. Fixed: 1. A row button's `id` was put on the button while its tooltip kept the trigger under an id of its own. It behaved when I ran it (hover, focus, Escape, the same as **Unlink**), and leaned on the library's insides to. The `id` is the trigger's now. 2. The journey step "a held button does nothing when pressed" read the branch a round trip later, which a switch that had started would also have passed. It now reads what the page would say of a press that got through, and goes red when one does. 3. "Switch" and "Deleting" had nothing to tell a translator what they are. - Also taken: the journeys' helper asks the module for a row button's words. - Follow-ups, none introduced here: the untranslated reason, the guide, and the lost focus, all in Risks above. - **Second pass:** completed — a separate agent context; head `4a4637cbea2329fcc9bcef927a591d160b032d02`, base `74f3c231272d226a410d6d0d0309938c24a48f5d`; no findings, three notes on wording. - **Since review:** `4a4637c` → `e091bb4`: three wordings from the second pass, in two comments and one translators' note. `e091bb4` → `96a120e2875254a7a4516c137b297cfdaeba59fb`: journeys only, the fix for the Windows failure described in Risks (`tests/e2e/helpers/ui.cjs`, `ticket-rebase.spec.js`, one assertion in `ticket-list.spec.js`). No source under `src/` changed since the second pass. Not reviewed again; lint and all journeys were run on it, and the new assertion goes red with the old helper. Checks on `96a120e2875254a7a4516c137b297cfdaeba59fb`, macOS (the unit suites and the packaged smoke on `e091bb4`; only journeys changed since): | Check | Result | |---|---| | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1830 pass, 2 skipped | | `npm run test:electron` | 1831 pass, 1 skipped | | `npm run test:e2e` | 90 of 90 | | `npm run test:e2e:packaged` | 15 of 15, on an unsigned build | | Eight journey files in a 1024×728 window | 36 of 36 | </details> <details> <summary>Implementation notes</summary> - `src/renderer/components/ticket-list.jsx` (new): `TicketListCard` and its row. It draws what it is given. - `src/renderer/ticket-branch-list.cjs`: `ticketListCard` returns the card's words (`heading`, `action`, `remove`, `removing`) or null for no card; `deleteWorkQuestion`; a row carries its `number`; "Edited 2 days ago" through `_n`. - `src/renderer/index.jsx`: the inline rows are gone. - `src/renderer/components/reasoned-button.jsx`: `id` to the tooltip's trigger. - `tests/e2e/helpers/ui.cjs`: `ticketListCard`, `deleteWorkButton`; `switchBackButton`, `continueWorkingButton` and `ticketRow` find a row by role inside the card, and no longer by the shape of the markup. - The list's journeys answer for GitHub and Trac themselves. Reading a ticket's attachments opens a window of its own, and a journey that speaks to the app's window by position would find that one. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload an image from here. What the screenshots I took show: - **A ticket linked:** a card headed "Other tickets on this site"; each row has the number in bold with "Edited just now" under it in grey, and to the right an outlined **Switch** and a plain **Delete this ticket's work**; a thin rule between rows. - **None linked:** the same card headed "Your tickets on this site", three rows, each with an outlined **Continue working**. - **A narrow window:** each row's two buttons sit under its number. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…has it (#600) ## Why Part of the redesign in #542, item 6 of #557: the create-site dialog. It was the older component library's modal, with its colours written into the markup. ## What changes The dialog is the design system's, laid out as the prototype has it: - **Title and close:** "Create site", with a close button. Escape and a press outside close it too. The **Cancel** button is gone, as in the prototype. - **Project:** a segmented choice of two, labelled "Project", where there was a radio list labelled "Contribute to". Under it is what the chosen project is, which each radio option used to say of itself, and "A site's project cannot be changed later." - **Location:** a field of its own, with the folder that was chosen under it. - **A missing answer** is said as an alert, so it is announced when it appears. What it does is what it did: - It refuses a missing name or folder, one at a time, and starts nothing while one is missing. - It opens empty, on the name, with Core chosen, however the last one was left. - It sends the setup what was chosen and closes; the setup goes on on the site's page. - Opened while a setup runs (from the notice on a site the old engine made), it is inert and stays until the setup has ended. Its decisions are in `create-site.cjs`, with unit tests: the projects it offers, what it says of the chosen one, and which answer is missing. Not in this pull request, by decision: - **Setup progress inside the dialog.** The prototype keeps the dialog open with a spinner through download, install and compile, and has no setup checklist. Here the dialog still closes and the site's page runs the checklist, with its retry buttons and its log. Moving the setup into the dialog is its own change. - **A default name.** The prototype creates an unnamed site as "My WordPress site". An empty name is still refused. - **A default location.** The prototype fills the folder from its settings, which are sub-issue #559. ## How to test this Platforms: any. The change is in the renderer only. The journey walks the dialog on both platforms. From the repository's root: ```bash npm run build:once && npx playwright test --project=journeys create-site.spec legacy-site.spec i18n.spec ``` `create-site.spec.js` covers what the dialog refuses, that it starts clean each time (also when opened again while it fades), what it sends, and how it stands while a setup runs and when one fails. Each claim was checked by breaking it: 28 breaks, all red. What a journey cannot reach is the system's folder dialog and a real setup. By hand, on the current head, on either platform: **Starting state:** the app with at least one site, so that the sites list is showing. 1. Click **Create new site**. - A small dialog titled **Create site** opens, with the cursor in **Site name**, **WordPress Core** chosen, and "No folder selected yet." under **Location**. 2. Click **Gutenberg**. - The lines under the choice change to describe the block editor, and still end with "A site's project cannot be changed later." 3. Click **Create site** with the name empty. - A red notice says "Please provide a site name." Nothing else happens. 4. Type a name and press Enter. - The notice now says "Please choose where to create the site." 5. Click the file button under **Location** and choose a folder in the system's dialog. - The folder's path appears under the field and the notice goes. 6. Press Escape, then click **Create new site** again. - The dialog is empty again, on WordPress Core. 7. Fill it in and click **Create site**. - The dialog closes and the new site's page shows the setup running, as before this change. **What must not have happened:** - Steps 3 and 4 started nothing: no new entry in the sites list, no folder created. - Step 6 kept nothing: not the name, the project, or the folder. - The site created in step 7 is in a new subfolder of the folder chosen, named after the site, and is a checkout of the project chosen. ## Risks and limitations - **Review:** two passes by a separate context. 2 to fix here on the first, both in the journey, both fixed; none on the second. Details below. - **A lint rule is switched off on one import.** The segmented control is exported by `@wordpress/components` only under an `__experimental` name, which `@wordpress/no-unsafe-wp-apis` forbids. The design system has no other segmented control and documents this one under that name, and the prototype uses it. - **Checked on macOS only**, in a 1024×728 window and with the site's status read slowed by a second as well as at the default size. Windows is CI's. - **The manual steps above were not run by a person.** I looked at screenshots of the dialog empty, filled and with a complaint. - **The field for the folder is the browser's file input**, as before and as in the prototype. Its button and "No file chosen" are the system's words, in the system's language, and it never shows a selection: the folder is said under it. - **A dialog opened during a setup cannot be closed until the setup ends.** That is how it was, and the journey marks it as a characterisation. It has no close button then, where the old one showed one that did nothing. - **The two ways into the dialog treat a leftover message differently**, as before: the sites list's button clears it, the button in the old-engine notice does not. A message is now an alert, so one left over is announced when the dialog opens from that notice. Where a failed setup is said belongs with the toasts, item 8 of #557. - **Choosing a folder fails silently if the system's dialog throws.** Carried over unchanged. - **Three labels are new strings** ("Project", "Location", and the sentence about the project being for good), so they are in English in the shipped translations until they are translated. - **The guide still describes the old dialog**, and its screenshot shows it: `docs/guide/creating-a-site.md`, `docs/guide/getting-started.md`. That is #561. ## Related Part of #557. Part of #542. Follows #599. --- <details> <summary>Design decisions and alternatives considered</summary> - **Form only.** Asked and decided before building: the dialog keeps closing on Create. The prototype's spinner covers a setup that takes minutes here and can fail at each of its steps, which the site's page shows with a retry for each. - **The project's description under the choice.** A segment has room for a name and nothing else. The line each radio option carried is said under the control, for the project that is chosen, and the sentence that the choice is for good with it. They are two sentences one under the other, and not side by side with a space typed between them. - **No Cancel button**, as the prototype: the close button, Escape and a press outside do it. - **The form is the dialog's content, and the button is in its footer**, tied to the form by its id, so that Enter in the name asks for the site as the button does. - **The dialog is always in the tree and told whether it is open**, so that it can fade and give the focus back. Its three answers are kept by a component inside the popup, which is not there while the dialog is closed. - **No focus is asked for.** The design system puts it on the first thing that is not the close button, which is the name. The audit showed that asking was redundant. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 2 [fix here] · 2 [follow-up] on the first pass, both fixed. 0 defects on the second. - **Review:** completed — a separate agent context, against `.github/instructions/code-review.instructions.md`; head `d2f33e7`, base `1319e26e51c5d5e94e02182b521dbbb24ef429ec`; it read the diff and the libraries' source and ran lint and the unit tests, not the journeys. Fixed: 1. "Escape does not close it while a setup runs" asked whether the dialog could be seen, which a dialog that is closing still can for the length of its fade. It asks whether it is open. 2. "A press on the held button asks for nothing" counted the setups asked for, which an empty form would not have added to. It reads what the form would say of a press that got through. - Also taken: the two help sentences one under the other, the folder field's rules scoped to the dialog, a comment about how long the popup is there. - Follow-ups, neither introduced here: the leftover message at the notice's door, and the guide. Both in Risks above. - **Second pass:** completed — a separate agent context; head `e06a2a7f2ba062518424e3ed98d706d30c11bc28`, same base; no defects. It confirmed from the libraries' source that the dialog's open state and its fade are separate attributes, that the held button is held by the design system and not by the browser, and that the page's rules win over the design system's. It asked whether a dialog opened again while it fades keeps its answers: it does not, and the journey now pins that. - **Since review:** `e06a2a7` → `6d8ce06957d6068a2980af2850bd3478ff50d9d5`: that journey step, and one sample string in a unit test. No source under `src/` changed. Not reviewed again; every suite was run on it. Checks on `6d8ce06957d6068a2980af2850bd3478ff50d9d5`, macOS: | Check | Result | |---|---| | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1836 pass, 2 skipped | | `npm run test:electron` | 1837 pass, 1 skipped | | `npm run test:e2e` | 90 of 90 | | `npm run test:e2e:packaged` | 15 of 15, on an unsigned build | | Four journey files in a 1024×728 window | 14 of 14 | | Four journey files with the status read slowed | 15 of 15 | </details> <details> <summary>Implementation notes</summary> - `src/renderer/components/create-site-dialog.jsx`: was `create-site-modal.jsx`. `CreateSiteDialog`, its form and its folder field. - `src/renderer/create-site.cjs` (new): `projectChoices`, `projectHelp`, `createSiteProblem`. - `src/renderer/index.jsx`: the dialog is rendered always and given `open`. - `src/renderer/shell.css`: the folder field, and the type of the older library's help text inside the dialog. - `tests/e2e/real-setup/create-site.spec.js` is the journey run by hand against a real clone; only the field's label changed in it, and it was not run. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload an image from here. What the screenshots I took show: - **Empty:** a small centred dialog, "Create site" and a close button at the top; **Site name** with the placeholder "My WordPress site" and the focus ring; **Project** with two equal segments, WordPress Core selected, and two grey lines under it; **Location** with the file button, "No folder selected yet." and a grey description; a blue **Create site** button at the bottom right. - **Filled:** Gutenberg selected, its description under the control, and the chosen folder's path, wrapped, under the file button. - **With a complaint:** a red notice with an icon, "Please provide a site name.", above the button. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…ion in the app's own dialog (#601) ## Why Part of the redesign in #542, item 7 of #557: the rename and delete dialogs. Renaming a site was the older component library's modal. Deleting a site, and deleting a ticket's work, asked through the system's own confirm box, which says one sentence, cannot say what goes, and has two buttons called OK and Cancel. ## What changes **The rename dialog** is the design system's, as the prototype has it: a small dialog with the name and one button. - The button says **Rename** (it said Save). There is no Cancel: the close button, Escape and a press outside leave it. - It still opens on the name the site has, selected; refuses an empty name; and cannot be left while the name is being written. - A name that could not be written is said in the dialog, as an alert, with the focus back on the name. Before, it was a system alert box, after which the dialog closed as if the rename had worked. **Deleting asks in a dialog of the app's own**, as the prototype has it for a site: - It is titled with what would go: "Delete *site name*?", or "Delete all work on ticket #60001?". - It says what that means and that it cannot be undone. - It opens on **Cancel**. The other button is red and says what it does: **Delete site**, **Delete this ticket's work**. - A press outside is not an answer. Cancel and Escape are a no. The same dialog asks before a ticket's work is deleted, which #599 left to this pull request. On a Gutenberg site it says "issue". The questions' words are in `site-dialogs.cjs` and `ticket-branch-list.cjs`, with unit tests. Not in this pull request: - **The three discards** (loose edits on trunk, "discard all changes", and the discard before a trunk update) still ask through the system's box. Each is asked from, or over, a modal of the older library, and two libraries' dialogs on top of one another is a risk better taken when those modals are redrawn. - **A default for an empty name.** The prototype keeps the old name when the field is emptied. An empty name is still refused, as decided for the create-site dialog. ## How to test this Platforms: any. The change is in the renderer only. The journeys walk all three on both platforms. From the repository's root: ```bash npm run build:once && npx playwright test --project=journeys rename-site.spec delete-site.spec ticket-list.spec ticket-branches.spec i18n.spec ``` `delete-site.spec.js` is new: the question names the site, nothing but a yes deletes it, and a yes removes the folder. `rename-site.spec.js` gains a journey for a rename that is pending and one that fails. `ticket-list.spec.js` asks its question through the new dialog. Each claim was checked by breaking it: 37 breaks, all red. By hand, on the current head, on either platform: **Starting state:** a Core site that is set up, with one ticket parked (link a ticket, then **Unlink**). 1. Open the site's menu (⋮, top right) and choose **Rename…**. - A small dialog titled **Rename site** opens with the site's name selected in the field. 2. Empty the field and click **Rename**. - A red notice says "Site name cannot be empty." and the cursor is in the field. 3. Type a new name and press Enter. - The dialog closes; the heading and the sites list show the new name. 4. Open the menu and choose **Delete site**. - A dialog asks "Delete *the new name*?", says the site and all its files will be permanently deleted, and has **Cancel** focused beside a red **Delete site**. 5. Click outside the dialog, then press Escape. - The click does nothing. Escape closes the dialog. The site is still there. 6. In **Your tickets on this site**, click **Delete this ticket's work**. - A dialog asks "Delete all work on ticket #…?" with a red **Delete this ticket's work**. 7. Click **Cancel**, then ask again and click the red button. - Cancel leaves the row. The red button removes it. **What must not have happened:** - Step 5 deleted nothing: the site's folder is still on disk. - Step 7's Cancel deleted nothing: the ticket's branch is still in the site. - Step 7's yes deleted that ticket's work only, and the site is still on trunk. ## Risks and limitations - **Review:** two passes by a separate context. No defects on either. 3 follow-ups on the first, one taken here; 1 on the second, taken here. Details below. - **Checked on macOS only**, at the default size, in a 1024×728 window, and with the site's status read slowed by a second. Windows is CI's. - **The manual steps above were not run by a person.** I looked at screenshots of the three dialogs. - **A question can now stay open while the app goes on.** The system's box froze the window; this one does not. Nothing that holds the delete buttons can start behind an open question, since the question is modal, so a yes is not checked again. - **A failed rename shows the error as the main process words it** ("Error invoking remote method…"), untranslated. It is the same text the alert box showed. A sentence of the app's own means changing the handler in the main process. - **A deletion that fails is said where it was before**: for a site, in the notice at the top of the window; for a ticket's work, in the ticket card. Not in the question, which has closed by then. - **The guide quotes the old question** word for word, and still describes the old rename: `docs/guide/ticket-branches.md`, `docs/guide/managing-sites.md`. That is #561. - **`session.acceptConfirms()`** is still used by three journey files, all for discards. ## Related Part of #557. Part of #542. Follows #600. Does what #599 left for the ticket list's question. --- <details> <summary>Design decisions and alternatives considered</summary> - **One dialog for both questions.** `ConfirmDialog` takes what is asked and what a yes does. It is the design system's alert dialog with its "irreversible" intent, which is what makes the button red, puts the focus on Cancel and turns a press outside away. - **A yes closes the dialog at once.** The alert dialog can stay open with a spinner until the work is done. A site's deletion already says how it stands in the sites list, and a ticket's in its row, and both say a failure where they always did. The dialog does not take that over. - **The title says what goes, by name.** The system's box said "this site". The dialog can be read without looking behind it. - **The rename dialog stays held while it fades after a rename that worked**, and is let go when its popup has gone. Otherwise its button and close button come back for a fifth of a second. - **A complaint puts the focus on the field and is the field's description**, so that it is read with the field as well as when it appears. - **The note about the system's alerts and confirms** moved from `onRename`, which no longer has one, to `confirmAnd`, which is where the remaining ones are. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 0 [fix here] · 3 [follow-up] on the first pass. 0 defects · 1 [follow-up] on the second. - **Review:** completed — a separate agent context, against `.github/instructions/code-review.instructions.md`; head `389e081`, base `abb144269b9b108f0cd17e739b620c9199b1e89c`; it read the diff and the libraries' source and ran lint and the unit tests, not the journeys. No defects. Follow-ups: 1. "A no deletes nothing" looked for the folder, or the branch, which is still there at the start of a deletion too. **Taken:** the journeys also read what a deletion says as it begins. 2. The guide quotes the old question. Left for #561. 3. A failed rename shows the main process's wording. Left; in Risks. - Notes taken: the dialog coming back to life while it fades after a rename, and the focus after a failed one. - **Second pass:** completed — a separate agent context, on the fix-up before it was committed (staged over `389e081`), same base. No defects. It checked that the held state cannot be left stuck (no way to reopen the dialog while it fades: the backdrop takes the pointer and the focus is given back after), that the focus is not taken at a wrong moment, and that the slowed fade in the journey applies to the popup. One follow-up, **taken:** the complaint is now the field's description. - **Since review:** the second pass's fix-up plus that description, two comments and one assertion read once instead of retried, committed together as `db9e863a9a0e71218bcdb713d782b0be61445efa`. Not reviewed again; every suite was run on it, and the new assertion goes red when its line is removed. Checks on `db9e863a9a0e71218bcdb713d782b0be61445efa`, macOS: | Check | Result | |---|---| | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1840 pass, 2 skipped | | `npm run test:electron` | 1841 pass, 1 skipped | | `npm run test:e2e` | 93 of 93 | | `npm run test:e2e:packaged` | 15 of 15, on an unsigned build | | Seven journey files in a 1024×728 window | 26 of 26 | | Seven journey files with the status read slowed | 26 of 26 | </details> <details> <summary>Implementation notes</summary> - `src/renderer/components/rename-site-dialog.jsx`: was `rename-site-modal.jsx`. - `src/renderer/components/confirm-dialog.jsx` (new): `ConfirmDialog`. - `src/renderer/site-dialogs.cjs` (new): `renameProblem`, `deleteSiteQuestion`. - `src/renderer/ticket-branch-list.cjs`: `deleteWorkQuestion` gives a title, a description and the button's words, for a ticket and for an issue. - `src/renderer/index.jsx`: `asking` and `askFirst` in the site's view; `onRename` rejects. - `tests/e2e/helpers/ui.cjs`: `confirmDialog`, `confirmYesButton`, `confirmNoButton`. - The rename journey slows the dialog's fade to three seconds for one step, to look at the dialog on its way out. It costs that test three seconds. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload an image from here. What the screenshots I took show: - **Rename:** a small centred dialog, "Rename site" and a close button, the field with the name, a blue **Rename** button at the bottom right; with an empty name, a red notice "Site name cannot be empty." between the field and the button. - **Delete site:** a wider dialog with no close button, "Delete first-name?" as its title, two lines under it, and at the bottom right **Cancel** with the focus ring and a red **Delete site**. - **Delete a ticket's work:** the same, titled "Delete all work on ticket #60001?", with a red **Delete this ticket's work**. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…ay a failed setup there (#602) ## Why Part of the redesign in #542, item 8 of #557: the toasts. The window's confirmations were the older component library's snackbars, restyled by hand in the page's own stylesheet, and only one of their sentences could be translated. One thing they could not say at all: why a site failed to set up. That was shown only if the create-site dialog happened to be open when the setup failed, and lost otherwise (#600 left it for here). ## What changes **The confirmations are the design system's notices**, stacked in the window's bottom corner as the prototype has them: green for what worked, red for what did not, each with a **Dismiss** button. What they do is what the snackbars did: - One for something that worked is said politely and goes by itself after six seconds. - One for something that did not is said at once and does not time out. - A repeat of the newest is not shown or said twice, and three are kept at most. What is different: - **Every notice has a Dismiss button.** A snackbar was dismissed by pressing it anywhere. Dismissing one leaves the focus on the stack, as the snackbars' list had it. - **The stack is a region named "Notifications"**, which a screen reader can go to. - **A site that could not be set up is said there**, as an error. The create-site dialog's message is its own again: which answer is missing. Its two ways in no longer differ over a message left behind, since none is. - **The sentences can be translated.** The ones a value or a branch chooses are worded in `confirmations.cjs`, with unit tests: what the apply flow did, a saved patch, an opened pull request, a deletion or a setup that failed. - **Tooltips of the design system are drawn over everything.** They were under the stack, so the name of a notice's button was hidden by the notice above it. Not in this pull request, by decision: **no new confirmations.** The prototype raises a toast for many things the app does not confirm (a rename, a deletion, a server starting). The app says what it said before, plus the failed setup. ## How to test this Platforms: any. The change is in the renderer only. The journeys walk the stack on both platforms. From the repository's root: ```bash npm run build:once && npx playwright test --project=journeys toasts.spec create-site.spec i18n.spec ``` `toasts.spec.js` is new: a confirmation is shown and said once, goes at its time and not before, can be dismissed; an error is said at once, stays, and is joined by a later success without going with it; a notice's tooltip is over the notice above it. `create-site.spec.js` covers the failed setup, with a dialog open and with none. Each claim was checked by breaking it: 23 breaks, all red. By hand, on the current head, on either platform: **Starting state:** a site that is set up. 1. Open the site's menu (⋮, top right) and choose **Copy path**. - A green notice "Copied the path" appears in the bottom right corner, above the footer. It goes after about six seconds. 2. Choose **Copy path** again and click the notice's **✕**. - It goes at once. 3. Turn the network off, click **Create new site**, fill the dialog in and click **Create site**. - The dialog closes. When the clone fails, a red notice says "The site could not be created." and why. It is still there a minute later. 4. While it is showing, choose **Copy path** again and rest the pointer on the green notice's **✕**. - The green notice is under the red one. A "Dismiss" tooltip shows over the red notice, not behind it. 5. Click the red notice's **✕**. - It goes. **What must not have happened:** - Step 3 left nothing in the sites list for the site that failed, and no dialog open. - The red notice did not go by itself in step 4, when the green one did. - A notice never stopped a click on what is beside it: the stack takes a press only where a notice is. ## Risks and limitations - **Review:** two passes by a separate context. 3 to fix here on each, all fixed. The first pass caught a mistake of mine: I had set the lifetime to ten seconds, believing that was the snackbars'. It was six, and is six. Details below. - **Checked on macOS only**, at the default size and in a 1024×728 window. Windows is CI's. - **With the site's status read slowed by a second, two journeys fail that this change does not touch**: `site-header.spec.js` (a viewport check) and `trunk-update.spec.js` (an update asked for straight after a ticket is linked). Both fail the same way on `redesign/wpds` without this change; I checked. They pass at normal speed and have been passing on the Windows runner. The eight other files that read a toast pass slowed, 25 of 25. - **The manual steps above were not run by a person.** I looked at screenshots of one notice, of two, and of the tooltip. - **A failed setup's notice shows the error as the main process words it** ("Error invoking remote method 'wordpress:setup': Error: …"). The dialog showed the same text. It is now in a notice that stays in front of a newcomer. - **Three newer notices push out the oldest, an error included.** As before. A reducer that drops successes first would be a change of behaviour. - **A notice raised while a dialog is open cannot be reached by keyboard** until the dialog closes: the dialog keeps the focus. It is still said, and can be dismissed with the pointer, which leaves the dialog open; I ran that with the rename dialog. - **The "Notifications" region is always there**, empty when nothing is said. - **The five sentences worded where they are raised** ("Updated to the latest trunk", "This site is ready to work on", and three more) go through the translator, but no test would notice if one stopped: the unit tests reach the module's, and the pseudo-locale journey reads "Copied the path". - **The guide's other pages** still describe the redesign's earlier screens (#561). One sentence is corrected here because this change made it false. ## Related Part of #557. Part of #542. Follows #601. Settles what #600 left about a failed setup. --- <details> <summary>Design decisions and alternatives considered</summary> - **Six seconds, not the prototype's 3.2.** The prototype's is short for a sentence, and the app's has not changed. - **A Dismiss button on every notice.** The prototype's notices have none, and its errors time out like the rest. An error that stays needs a way to go, and a success could always be sent away early. - **The dialog no longer shows why a setup failed.** Two places saying it, one of them only sometimes, is what lost it. With the notice saying it always, the dialog's line is its own and needs no owner outside it. - **No shadow in the prototype; one here.** A notice over a page of cards reads as one more card without it. It is the design system's elevation token. - **The journeys find the stack by its label, not its role.** A confirmation can be raised while a dialog is open, for something done inside it, and an open dialog hides everything else from the roles. - **Every lookup of a toast's text goes through the stack.** A notice is said through a live region elsewhere in the document, which holds the same text; a search of the whole page finds both. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 3 [fix here] · 3 [follow-up] on the first pass, all 3 fixed. 3 [fix here] · 2 [follow-up] on the second, all 3 fixed. - **Review:** completed — a separate agent context, against `.github/instructions/code-review.instructions.md`; head `3297f74`, base `6c7e6792026c533f8e91de7105c61ef5505d6e2b`; it read the diff and the libraries' source and ran lint and the unit tests, not the journeys. Fixed: 1. The lifetime was ten seconds, said to be the snackbars'. Theirs was six (`NOTICE_TIMEOUT` in `@wordpress/components` 41). It is six. 2. Dismissing a notice left the focus at the top of the document. It is put on the stack. 3. The guide said a failed setup's reason goes to the log and not to a dialog. - Follow-ups taken: the design system's tooltips under the stack; two steps of the journey. Not reproduced when run: dismissing a notice closing an open dialog. - **Second pass:** completed — a separate agent context; head `953f4a32e08a9b3a274a7a9979a4382d7f87b755`, same base. No finding in the app's code. Fixed, all in `toasts.spec.js`: 1. The check that a confirmation is still there depended on how fast the machine was. The clock is stopped under it. 2. The check that a repeat is not shown twice could be read before a second one had appeared. It is told by what is left after one is dismissed, and by what was said by the end. 3. The tooltip's place could be read before the tooltip had been moved to its button. It is read once it reaches the notice above. - Also taken: the guide's sentence said the notice stays until dismissed, which three newer ones can undo. Left: the main process's wording in the notice; in Risks. - **Since review:** `953f4a3` → `ba41d4e633b8c7f8e001a5e3ef9930907302f67e`: the journey and one sentence of the guide. No source under `src/` changed. Not reviewed again; every suite was run on it, and eight breaks go red against the rewritten journey. Checks on `ba41d4e633b8c7f8e001a5e3ef9930907302f67e`, macOS: | Check | Result | |---|---| | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1846 pass, 2 skipped | | `npm run test:electron` | 1847 pass, 1 skipped | | `npm run test:e2e` | 95 of 95 | | `npm run test:e2e:packaged` | 15 of 15, on an unsigned build | | Ten journey files in a 1024×728 window | 33 of 33 | | Eight journey files with the status read slowed | 25 of 25 | </details> <details> <summary>Implementation notes</summary> - `src/renderer/components/toast-stack.jsx` (new): `ToastStack`. - `src/renderer/confirmations.cjs`: the reducer is as it was. New: `toastView`, `TOAST_LIFETIME_MS`, `setupFailureMessage`, `applyDoneMessage`, `patchSavedMessage`, `savedAndResetMessage`. The module now needs `@wordpress/i18n`. - `src/renderer/index.jsx`: the stack in the same portal on `body`; the failed setup's notice; `createSiteError` gone. - `src/renderer/index.html`: the snackbars' rules are gone. `src/renderer/shell.css`: the stack, and `--wp-ui-tooltip-z-index` on `:root`, which is where the design system reads it from. - `tests/e2e/helpers/ui.cjs`: `toasts`, `toast`. Eight journey files used `getByTestId( 'snackbar' )` or a search of the whole page. - The toasts journey uses Playwright's clock. Stopped, it also stops the app's menus from being pressed, so it is stopped only between a notice appearing and its time being up. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload an image from here. What the screenshots I took show: - **Two notices** in the bottom right corner, above the footer, 360 pixels wide: a red one with a warning icon and four lines of bold text, and under it a green one with a tick, "Copied the path". Each has a ✕ at its right. - **The tooltip:** a small dark "Dismiss" label above the green notice's ✕, drawn over the lower edge of the red notice. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…e design system (#605) ## Why Part of the redesign in #542, item 9 of #557: replace all hardcoded colours with tokens. What is left is about 190 hardcoded colours and 200 inline style blocks, too much to review in one go, so it comes as a short series. This is the first part. What the site's page says above its cards was drawn by hand: each box with its own colours, padding and font size written into the markup. None of it could follow a theme, and each box was a little different from the next. ## What changes Everything above the cards is drawn by the design system, so that none of its CSS is written here. Tokens put into the same inline styles would have replaced the colours and kept the rest; components take all of it. - **The notices are the design system's notice.** A site the old engine made, a merge left half done, an update that did not finish, code that is old, an update that is done, a ticket handed over by a link, a folder that would not open. And what the ticket card says back: a refusal, a switch under way, edits carried or saved, the question about loose edits. Each keeps its words, its role, its buttons and what they do. - **An update of trunk under way is a card** with the list of steps the apply card has. That list is one component for both now, `StepList`. - **The setup checklist is a card with a list of steps**, each with a badge for how it stands and the button that does it. The prototype has no checklist; it is kept and redrawn. In `index.jsx` this takes the hardcoded colours from 100 to 23 and the inline style blocks from 106 to 34. The 23 are the terminal, the logs and the mail. What looks different: - A notice's title and what follows it are two lines, with the design system's icon. They were one run of text. - The checklist's rows are not tinted by state. A badge and a mark say it. - "Retry install & build" and the question's "Discard…" are not red. The design system's button has no destructive tone, and the other redrawn cards draw theirs the same way. Not in this pull request: - **The words.** They are as they were, and still not translatable where they were not. - **The rest of the series:** the terminal, logs and mail panels and the page's leftover styles; the review dialog and its destinations, and the discard and email dialogs; and a lint rule that keeps hardcoded colours out of the renderer. ## How to test this Platforms: any. The change is in the renderer only. The journeys walk these on both platforms. From the repository's root: ```bash npm run build:once && npx playwright test --project=journeys site-page.spec legacy-site.spec merge-in-progress.spec trunk-update.spec trunk-update-watch.spec ``` `site-page.spec.js` is new: the checklist's steps, how each stands, its button and the way out; and the notice for old code, with its button and the next-action cue on it. The update journeys now read the update card as a region with a list and a current step, and dismiss the summary. Each claim was checked by breaking it: 21 breaks, all red. What a journey cannot judge is how it looks. By hand, on the current head, on either platform: **Starting state:** the app with no site. 1. Create a site and leave it alone. - The page shows a card headed **Initial setup checklist**. A blue notice at its top says "Setting this site up for you — step N of 3" and has a **Stop setup** button. - Each step is a row: a mark, its name, a badge (Completed, In progress, Locked), a line about it, and its button. The step under way has a spinner, and its button can be read and cannot be pressed. 2. When the setup has ended, click **Start dev server and finish the wizard**. - The checklist goes and the site's cards appear. 3. Open the site's menu (⋮) and choose **Update to latest trunk**. - A card headed **Updating to latest trunk** says "step N of 3" and lists the steps, the current one with a spinner. - When it ends, a green notice says "Up to date with trunk as of today." Its **✕** sends it away. 4. Edit a file of the site by hand, then link a ticket in **Trac ticket**. - A yellow notice under the field asks what should happen to the edit, with three buttons and **Cancel**. **What must not have happened:** - Step 1's buttons were never pressable while their step ran, and the setup went on to the build by itself. - Step 4 moved nothing before a button was pressed: the edit is still in the file, and no branch was made. - No notice's buttons sit off to one side or leave a gap at its end. ## Risks and limitations - **Review:** two passes by a separate context. 5 to fix here on the first, all fixed; 1 on the second, fixed. No behaviour was found lost. Details below. - **Checked on macOS only**, at the default size, in a 1024×728 window, and with the site's status read slowed by a second. Windows is CI's. - **The manual steps above were not run by a person.** I looked at screenshots of the checklist, the question about loose edits, and the old-engine notice. - **A real setup was not run.** The checklist under way (the blue notice, a step in progress, a failed step) is drawn from the same states as before and has no journey; `tests/e2e/real-setup/create-site.spec.js` runs one by hand and was not run. - **Nothing is announced that was not before, and nothing less.** Every notice is told to say nothing of its own: the ones that were alerts or status regions still are, by their role, and the static ones were silent. - **The focus is lost when a notice with a focused button goes** (Dismiss, Hide, Not now, Retry, Update), as it was before. - **The next-action ring has a corner a little rounder than the notice or card it is on.** Its rule is in the page's own stylesheet, which is the next part of this series. - **The guide calls "Skip initialization wizard" a link**; it is a button now. That is #561. ## Related Part of #557. Part of #542. Follows #602. --- <details> <summary>Design decisions and alternatives considered</summary> - **Components, not tokens in the old styles.** The request was for as little hard-coded CSS as possible. A notice of the design system needs no CSS here at all; a box with token values would still carry its padding, radius, font size and layout in the markup. - **The checklist loses its tints.** Five states each had a background, a border, a text colour and an indicator colour: twenty values. A badge has the state's colour by its intent, and the mark is an icon. What is left in CSS is the row and the mark's size. - **The checklist's buttons are not given the design system's loading state.** It hides a button's words behind a spinner and says "Loading" to a screen reader. An install takes minutes, and the site it belongs to may not be the one on screen. The buttons are held while their step runs, as before; the step's mark spins and its badge says "In progress". - **One step list.** The update card and the apply card show the same thing, a plan with a current step, and were drawn twice. - **A class on the page's notices, for now.** The page's stylesheet gives every `button` a right margin, which the redrawn areas are each cleared of. That rule goes with the next part, and the list of exceptions with it. - **A failed discard's reason is a line of text, not a notice.** It follows a sentence inside the ticket card, where a notice inside a sentence would be too much. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 5 [fix here] · 1 [follow-up] on the first pass, all 5 fixed. 1 [fix here] · 2 [follow-up] on the second, fixed, with one follow-up taken. - **Review:** completed — a separate agent context, against `.github/instructions/code-review.instructions.md`; head `495db32`, base `4bee60f003b363548d4a3a697c33d6d120a6145e`; it compared each removed block with what replaced it (conditions, handlers, refs, cue props, roles, text) and found none lost; it ran lint and the unit tests, not the journeys. Fixed: 1. A journey looked for "Dependencies unchanged" on the whole page, which the terminal says too. 2. A failed discard's reason ran on from the sentence before it. 3. The checklist's buttons said a bare "Loading" and hid their words while a step ran. 4. The page's notices' buttons had the page's older margin. 5. The screenshots' crop of "Update incomplete" took the column. - Follow-up left: the guide's word for the skip button. - **Second pass:** completed — a separate agent context; head `3be3413`, same base. No defect in the fix-up. Fixed: the question about loose edits is also drawn in the review dialog, where its buttons still had the margin. Taken: the update journey's look for the summary is by the summary's own sentence. Left as it is: the legacy journey's count of alerts can be answered as the page settles and would not catch one that came later; its comment says so now. - **Since review:** `3be3413` → `8c1512be3d5d2374b600a0549271fdf0b202d2d2`: that class on one notice, one redundant prop removed, and two lines of journeys. Not reviewed again; every suite was run on it. Checks on `8c1512be3d5d2374b600a0549271fdf0b202d2d2`, macOS: | Check | Result | |---|---| | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1847 pass, 2 skipped | | `npm run test:electron` | 1848 pass, 1 skipped | | `npm run test:e2e` | 97 of 97 | | `npm run test:e2e:packaged` | 15 of 15, on an unsigned build | | Ten journey files in a 1024×728 window | 31 of 31 | | Nine journey files with the status read slowed | 28 of 28 | </details> <details> <summary>Implementation notes</summary> - `src/renderer/index.jsx`: the boxes are `Notice`, `Stack` and `Text`; `UPDATE_STEP_MARKS` and `checklistVisuals` are gone; `pageNotice(id)` gives a page-level notice its class and, where it has one, its cue. - `src/renderer/components/step-list.jsx` (new): `StepList`, from the apply card's `Progress`. - `src/renderer/components/trunk-update-card.jsx`, `setup-checklist.jsx` (new). - `src/renderer/setup-steps.cjs`: `setupStepBadge`, with a unit test. - `src/renderer/shell.css`: `.apply-step*` is `.step-list*`; `.setup-step*`; `.error-text`. Every value is a token with its fallback. - `scripts/screenshots/shots.cjs`: two crops follow the new markup. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload an image from here. What the screenshots I took show: - **The checklist:** a card headed "Initial setup checklist"; each step a bordered row with a green tick, the step's name in bold with a green "Completed" badge beside it, a grey line under it, and an outlined button at the right ("Dependencies installed", "Build complete"); the last step ringed as the next thing to do, with a blue "Ready" badge and a solid blue button. - **The question about loose edits:** a yellow notice with an icon under the ticket field, the question, then three outlined buttons and a plain "Cancel". - **A site the old engine made:** a red notice with an icon at the top of the page, a bold first line, three lines under it and a blue "Create site" button; under the ticket field, a second red notice with the refusal. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…d empty the page's own stylesheet (#606) ## Why Part of the redesign in #542, item 9 of #557: replace all hardcoded colours with tokens. This is the second part of the series; #605 was the first. The terminal, the logs and the mail under a site's cards were black panes and grey text written into the markup, and the page still had a stylesheet of its own in `index.html`, with a rule that gave every button in the window a margin, which each redrawn area then had to be cleared of. ## What changes **The terminal and the log panes are light**, as the prototype's tray has them: the design system's weak surface with dark monospace text. That was decided before building. The design system has no token for a dark surface in a light theme, so staying dark would have meant keeping colours of our own. - **The terminal draws itself and takes its colours and font as values**, so they are read off the tokens where it stands when it is made. Which token each colour is, is `terminal-theme.cjs`'s, with unit tests. - The sixteen colours a command can ask for by number are the design system's nearest: red is what an error is said in, green a success, yellow a warning. - The bright eight are the plain eight. The design system's stronger colours are for text on a tinted notice and are all but black here. - A selection is the strong brand surface, with the text colour the design system has for it. - Text a command prints on a background of its own is moved until it can be read (the terminal's minimum-contrast option, at 4.5). - **A log line is coloured by its kind** in the stylesheet: an error, a warning, a deprecation or notice, each in the design system's colour for it. - **The three panels are regions named by their headings.** Their buttons are the design system's; a command named under the terminal is its link, made of a button. - **The window's own cards** (the Playground web server, a setup in flight) are the design system's card. **The page's own stylesheet is gone from `index.html`.** - What was still used moved to `shell.css`, with tokens: the next-action ring and the review dialog's two columns. - What was not used is deleted: five rules for markup that no longer exists. - **The margin it gave every button** is the older library's dialogs' alone now. The list of redrawn areas cleared of it, sixteen selectors long, is gone with it. `index.jsx` has no colour and no inline style left. It began this item with 100 and 106. Not in this pull request: - **The words**, which are as they were. - **The panels' layout.** They move to the tray in #558; here they keep their place and their shape. - **The rest of the series:** the review dialog and its destinations, the diff panes, the discard and email dialogs; and a lint rule that keeps hardcoded colours out. ## How to test this Platforms: any. The change is in the renderer only. The journeys walk the three panels on both platforms. From the repository's root: ```bash npm run build:once && npx playwright test --project=journeys terminal.spec logs.spec mail.spec ``` They now also say: each panel is a region named by its heading, in their order; the terminal's surface and text are the tokens' colours, read where the terminal paints them; red printed in bold and in plain is the error colour; text on a background of its own can be read; a log pane is the same surface; a warning and a notice in `debug.log` have their colours; a command link is in the terminal's type. Each claim was checked by breaking it: 29 breaks, all red. What a journey cannot judge is how it looks. By hand, on the current head, on either platform: **Starting state:** a site that is set up, with the dev server running. 1. Scroll to **Terminal**. - A pale grey pane with dark monospace text, and no dark surface anywhere on the page. 2. Type `npm run build` and press Enter. - The build's output is readable throughout. Anything it prints in colour (warnings in yellow, errors in red) can be told from the text around it. 3. Drag across a few lines of the output. - The selection is a solid blue with light text on it. 4. Under the terminal, with nothing running, click **npm install** in the hint. - It is in monospace, and it types `npm install` at the prompt without running it. 5. Scroll to **Logs** and open **debug.log** after loading a page of the site that logs a PHP warning. - The pane is the same pale surface; the warning's line is in the warning colour, a stack trace under it in grey. 6. Send a mail from the site (a password reset) and look at **Mail**. - A framed list with one row: when, from whom, and the subject in bold. The row highlights under the pointer and opens the mail. 7. Open **Review & submit changes**. - The dialog looks as it did: its buttons are spaced as before. **What must not have happened:** - No text in the terminal is unreadable: nothing white on the pale surface, nothing dark on a dark background. - The terminal's columns line up: the help's two columns are straight, and no line is cut in half. - Buttons elsewhere on the page have not moved together or apart: the log tabs, the feedback popover, the header. ## Risks and limitations - **Review:** two passes by a separate context. 5 to fix here on the first, 3 on the second, all fixed. Two of the first pass's were real readability problems of the light terminal; details below. - **Checked on macOS only**, at the default size, in a 1024×728 window, and with the site's status read slowed by a second. Windows is CI's. The terminal's font is the tokens' list (Menlo, Consolas, monaco), which on Windows is Consolas, as before. - **The manual steps above were not run by a person.** I looked at screenshots of the three panels, of coloured output with a selection, and of the review dialog. Real `npm` output was not looked at: the journeys print stand-in lines. - **The terminal reads its colours once, when it is made.** It will not follow a change of theme until it is made again. That matters with dark mode, #560. - **Two things a command can ask for are less readable than on the dark terminal**, both from how the terminal itself works: - Reversed text with no colours given (`ESC[7m`) is mid-grey on dark, about 4.8:1 where it was 15:1. The contrast option stops raising it once it reaches its ratio. - Dim text is drawn at half strength after the contrast check, so dim grey is about 2:1. - Both are rare here: a command's output is piped and nothing forces colour. - **Removing the page's button margin moved old-library buttons outside dialogs 8px closer**: the log tabs, the feedback popover's, the links in the changes note. I looked at the log tabs and the note; they read better for it. - **The next-action ring is the design system's warning stroke**, a paler amber than the one it had, at the cards' radius. - **A dark-on-light terminal is a change of habit** for anyone used to the black one. It is the prototype's, and it follows the theme. ## Related Part of #557. Part of #542. Follows #605. --- <details> <summary>Design decisions and alternatives considered</summary> - **Light, not dark with a few colours of our own.** Asked and decided before building. Dark panes would have kept a set of hardcoded values and would not follow a theme. - **The terminal's colours are resolved, not passed as `var(…)`.** The terminal parses colours itself and paints them into its own stylesheet and cells. Each token is given to a hidden element and read back, and written through a canvas as `#rrggbb`, which is the one form every part of the terminal takes. - **Tokens are written as `var(--wpds-…)` in the module**, which is how the repository's lint rule finds a token, and how the build can give one its fallback. - **Bright is plain.** The alternative, the design system's stronger colours, made a bold red error indistinguishable from the text around it. - **The selection has its own text colour.** Without it the text on a selection is readable only because of the contrast option, which is a dependence nobody would see. - **Regions with headings**, like the cards above them. It also makes the journeys' way to a panel the same for all of them, where the old markup was found by its shape. - **The margin stays for `.components-modal__frame .components-button`.** The older library's dialogs are redrawn in the next part; until then their buttons are spaced as they were. The design system's buttons inside them are not touched by it. - **`hidden` in place of an inline `display`** on a site's view that is not on screen. Nothing sets a `display` on that element that would undo it. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> 5 [fix here] · 2 [follow-up] on the first pass, all 5 fixed and both follow-ups taken. 3 [fix here] · 2 [follow-up] on the second, all 3 fixed. - **Review:** completed — a separate agent context, against `.github/instructions/code-review.instructions.md`; on the change before it was committed (staged over `daefeb35a613478f38452221c0b197983cf044c7`; it became `3eaa3d3`); it read the diff, the terminal library's source and the removed stylesheet rule by rule, and ran lint and the unit tests, not the journeys. Fixed: 1. A selection in the terminal could not be seen. 2. Bold coloured output was all but black, and text on a coloured background could vanish. 3. The window's own cards had lost the room under them. 4. A command link under the terminal had lost its monospace. 5. A new journey file cost an app launch on every pull request for what three existing journeys could say. - Follow-ups taken: a rule for the review dialog's title that had never matched; a check in a hook the unit tests cannot reach, moved to the module. - **Second pass:** completed — a separate agent context; head `30779e9`, same base. Fixed: 1. My reason for the selection's colour was wrong (this terminal paints it solid), and its text was readable only through the contrast option. It has its own text colour. 2. The mail journey says the panels' order, which the folded journey had said. 3. A margin doubled under the last of the window's notices. - Left, and in Risks: reversed text and dim text. - **Since review:** `30779e9` → `2df2d559812fb776d7ee569a1d49a864df524325`: those three. Not reviewed again; every suite was run on it. Checks on `2df2d559812fb776d7ee569a1d49a864df524325`, macOS: | Check | Result | |---|---| | `npx eslint . --max-warnings=0` | clean | | `npm test` | 1861 pass, 2 skipped | | `npm run test:electron` | 1862 pass, 1 skipped | | `npm run test:e2e` | 97 of 97 | | `npm run test:e2e:packaged` | 15 of 15, on an unsigned build | | Nine journey files in a 1024×728 window | 24 of 24 | | Nine journey files with the status read slowed | 24 of 24 | </details> <details> <summary>Implementation notes</summary> - `src/renderer/terminal-theme.cjs` (new): `TERMINAL_COLOR_TOKENS`, `terminalTheme`, `terminalFont`, `tokenName`, `TERMINAL_READABILITY`. - `src/renderer/hooks/use-site-terminal.jsx`: `readTerminalLook`; `TERMINAL_FONT` is gone. - `src/renderer/index.jsx`: the three panels and the window's cards; `LOG_PANE_STYLE` and `pageNotice` are gone. - `src/renderer/components/log-text.jsx`, `terminal-command-link.jsx`: classes and the design system's link. - `src/renderer/index.html`: no `<style>`. `src/renderer/shell.css`: the panes, the log lines, the mail list, the ring, the review dialog's columns. - `src/renderer/components/review-dialog.jsx`: a prop the older library's modal ignores is removed. - The terminal draws a row again as more is printed, so the journey's reads of a printed word are asked until they are so, both colours off the same row. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload an image from here. What the screenshots I took show: - **The terminal:** a pale grey pane with dark monospace text, the help's two columns aligned, three grey lines of hints under it with the two commands as blue links. - **Coloured output:** red, green, yellow, blue, magenta and cyan words, each readable, bold ones in the same colours; words on red, green, yellow, blue and dark backgrounds in light text; a selection across one line in solid blue with light text. - **Logs and mail:** three tabs over a pale pane; a framed mail list saying "No emails yet." with an outlined **Clear emails** button above it. - **The review dialog:** as before. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Brings in the journey engine's forced kill (#607): an app still running five seconds after it was asked to quit is ended, and a launch that gives no window says what its main process knew. The launch here asks for the profile and runs a journey's beforeWindow before it waits for the window, so the evidence is taken around that wait, in this branch's order. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…kens (#608) ## Why Item 9 of #557 is to replace every hardcoded colour with the design system's tokens, with as little hand-written CSS as possible. Parts 1 and 2 did the site page and its panels. What was left was inside the three dialogs still built on the older library: "Review & submit changes" with its diff and its three destinations, the "Update to latest trunk?" question, and the mail dialog. Between them they held 95 inline `style` attributes and 87 colour values, including a dark diff pane and a dark raw-mail pane that no longer matched the light terminal and logs. ## What changes The three dialogs keep their frames, buttons and inputs from `@wordpress/components`. Everything drawn by hand inside them is now the design system's. - **The diff** is on the strong surface inside a hairline, and each kind of line (added, removed, hunk, file name, the app's own header) is a class painted with tokens, as the prototype has them. **The raw mail** is on the surface the logs and terminal are on. - **Boxes that warned, failed or informed** are `Notice`s: the old-trunk warning, the unreadable diff, nothing to send, someone else's work in the checkout, the branch based on today's trunk, the update question's failure, and test mode. - **Lines that say how something went** are `Text` in one of three shared classes, `success-text`, `warning-text` and `problem-text`. These also replace the apply card's and the details sidebar's own copies, and `.error-text` from part 1 now uses the same red (its token gave a near-black `#470000`). - **Destinations** are stacks and text inside a card per group. The update question's two answers, its file list and the mail's panes are classes in `shell.css`. - The rule that gave every button in an older dialog a right margin is gone; the layouts use gaps. After this, no inline `style` and no colour value is left in any component under `src/renderer`. **Deliberately not in this PR:** - Turning these dialogs into the design system's `Dialog`, or the review into the prototype's three-column drawer with a file list. That is a redesign of the screen, not colour work, and is not listed in #557. - The three discards that still ask through `window.confirm`. - The lint rule that forbids a hardcoded colour in the renderer. That is part 4. - Mail HTML rendering is unchanged and tracked separately. ## How to test this Platforms: any. The journeys below drive each dialog on macOS and Windows and assert the colours against the tokens, read from the window itself. From the repository root, after `npm ci` and `npm run build:once`: ``` npx playwright test --project=journeys review-changes.spec trunk-update.spec mail.spec open-pull-request.spec ``` Each new assertion is red without the style it claims; I broke the rules one at a time to check (10 mutations, 10 red): - `review-changes.spec.js`: an added and a removed line are in the success and error colours on their washes; a failed save is in the error colour and a saved file's line in the success colour. - `trunk-update.spec.js`: the chosen answer is ringed in the brand colour and the other is not, the ring moves with the choice, the chosen answer's detail is in the full text colour, and the answer that loses work stays red, chosen or not. - `mail.spec.js`: the raw mail is on the weak surface. **Worth a look by hand, since no journey judges taste** (any platform, the current head): 1. On a site with an edit, press **Review & submit changes**. The diff is light, with green added and red removed lines on a tint; the destinations are two cards. 2. From the site's menu choose **Update to latest trunk** with an edit in the tree. The chosen answer is ringed in blue; choosing **Discard them** moves the ring and keeps that answer red. 3. Start the dev server on a site that has sent mail, open a mail, and press **Raw**. The pane is light. **What must not have happened:** no wording, role or accessible name changed (the existing journeys find everything by role and text and pass unchanged, all 99), and nothing outside these dialogs moved except the two colour classes named above. **What cannot be tested by hand here:** the GitHub device code and a real pull request result need a GitHub sign-in; the journeys stand in for GitHub and cover both states. ## Risks and limitations - Review: 2 passes, 6 findings fixed here, 2 accepted as they are. Details below. - **Contrast is lower than the dark pane's, though every pair passes 4.5:1.** Added lines are 4.86:1 and hunk lines 4.50:1, where the dark pane had about 9.7:1. These are the prototype's tokens for a diff. - **Visible changes beyond colour:** destination cards have the design system's 24px padding (was 14/16px); titles are the design system's medium heading; the GitHub device code is 20px (was 24px); test mode is a neutral notice and not a dashed box; the mail dialog's two tabs sit flush, which is the library's default; a stand-alone link in a destination is clickable on its words only, not across the column. - The error under the site page's changes note turns from near-black to red, with `.error-text`. - Accepted, not fixed: no journey checks the 12px size of the four stand-alone links. - The strings in these components are still not translatable. That was so before, and is not changed here. ## Related Part of #557, after #605 and #606. --- <details> <summary>Design decisions and alternatives considered</summary> - **Frames and controls stay the older library's.** A dialog half in each library's buttons would look worse than either, and the frame is what a later redesign of the review replaces. - **`Notice` with nothing spoken.** The design system's notice speaks its text when it mounts. Each one here either carries `role="alert"`, as the box it replaces did, or was silent before and stays so; none is announced twice. - **`Text` renders a span.** Every one is a flex or grid item, or is given a block element, so no line runs into the next. - **Shared classes for text colour** and not one per card: three classes replace six rules that said the same thing. - **The pressed answer's ring is drawn inside its border**, so choosing does not move the other answer by a pixel, as the old 2px border did. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **Pass 1: 4 [fix here] · 2 [follow-up]. Pass 2 (on the fix-up): 2 [fix here], both low. 6 fixed, 2 accepted.** - **Review:** completed — a separate agent context, read-only; head `50129cd` (rebased since, as `369cf6e`) / base `9570450`; compared each component with its old version, read `Notice`'s implementation, computed contrast for every new pair; 4 [fix here], 2 [follow-up]. - Fixed: the chosen answer's detail text was 4.34:1 on its tint (now the full text colour, 14.6:1); four stand-alone links had grown from 12px to 13px; the mail journey's text-colour check passed without the rule it was for. - Fixed though marked follow-up: the test helper `tokenColour` could not notice a misspelled token (it now refuses one the window does not have); `.error-text` painted the same error in a different red. - Accepted: the mail dialog's tabs sitting flush, the library's default. - **Review:** completed — a second separate agent context, read-only; head `86555dd` / base `23d46e7`; 2 [fix here], both low. - Fixed: nothing pinned the losing answer's detail staying red once chosen, which holds on source order alone; a journey now asserts it. - Accepted: no journey checks the links' size. - **Since review:** `86555dd → 1460d81` adds that one assertion; `trunk-update.spec` re-run, and red with the two rules swapped. </details> <details> <summary>Implementation notes</summary> - The branch was rebased once, before it was pushed, onto `redesign/wpds` after trunk's journey-engine fix (#607) was merged into it. - `ui.tokenColour` and `ui.paintOf` are new shared helpers in `tests/e2e/helpers/ui.cjs`; `logs.spec.js` and `terminal.spec.js` use the first in place of their own copies. - Local checks on macOS: lint clean, `npm test` and `npm run test:electron` pass (1863 tests), `npm run test:e2e` 99 pass, the affected journeys pass in a 1024×728 window and with the status read slowed by a second, `npm run test:e2e:packaged` 15 pass. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload images from the command line. The three steps under "Worth a look by hand" show every changed surface, and Buildkite's build for this branch can be used for them. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…r's components (#609) ## Why Parts 1 to 3 of item 9 in #557 left no colour value and no inline `style` in any component under `src/renderer`: the window is painted from classes in `src/renderer/shell.css` with the design system's tokens. Nothing stopped one coming back. A colour typed into a component is one the theme cannot reach, and `TESTING.md` rules out the only other check, a test that reads a component's source. ## What changes Two lint rules of this repository's own, switched on for `src/renderer/**/*.{js,jsx,cjs}`: - **`toolkit/no-hardcoded-colors`** reports a colour written out in a string or in the fixed part of a template: a hex value of 3, 4, 6 or 8 digits, or a function that takes a colour's channels (`rgb()`, `hsl()`, `oklch()`, `color()` and the like). A token wrapped in `var()` passes, and so does `color-mix()` of tokens. - **`toolkit/no-inline-styles`** reports a `style` attribute on a JSX element. They live in `eslint-rules/`, registered as a local plugin in `eslint.config.mjs`. No new dependency: the rules are plain modules, tested with ESLint's own `RuleTester`. **Not in this PR:** - **The stylesheet.** `shell.css` is not checked by these rules, and it is where all the styling now is. That is the next pull request: Stylelint with the design system's own rules. - A colour's name (`red`, `white`), a colour put together from pieces, a `style` passed in a spread or set from an effect. Each rule's header says what it does not read. None of these exists in the renderer today. - `src/main.js`, whose patch window still carries a few inline colours. It is not the renderer. ## How to test this Platforms: any. No user-visible surface; this is lint and its tests. From the repository root, after `npm ci`: ``` npm run lint node --test tests/unit/eslint-rules.test.cjs ``` Both pass. To see the rules bite, add `style={{ borderColor: "#dcdcde" }}` to any element in `src/renderer/components/destination.jsx` and run `npx eslint src/renderer`: it reports the inline style and the colour. I did that, and the same with an `rgb()` in `src/renderer/terminal-theme.cjs`. `tests/unit/eslint-rules.test.cjs` runs each rule over code that must pass and code that must not. I removed each guard in the two patterns in turn (the lookbehinds, the lookahead, the case flag, the string-only check, reading a template as the program gets it, `color()` in the list) and the suite went red every time. **What must not have happened:** the rules must not fire on anything in the tree as it is (`npm run lint` is clean with no disable comment added), and `eslint-rules/` must not be packaged into the app (`build.files` in `package.json` does not include it). ## Risks and limitations - Review: 2 passes, 9 findings fixed here, 1 kept as it is. Details below. - **A reference of three or four digits written out in a string reads as a colour**: `'Fixed in #607'` is reported, because `#607` is a grey and `#000` must not pass. Nothing in the tree does this; the renderer builds such strings from the number (`#%d`, `#${ n }`), and the message says so. A fragment made only of hex letters, `'#add'`, is reported too unless it follows a word or a slash. - Kept: these could have been three selectors given to the core rule `no-restricted-syntax`, with no rule files. They are rules of their own because the colour rule's message names what it found and the patterns are tested by themselves. ## Related Part of #557, after #605, #606 and #608. --- <details> <summary>Review outcome (required — see AGENTS.md)</summary> **Pass 1: 5 [fix here] · 1 [follow-up]. Pass 2 (on the fix-up): 4 [fix here]. 9 fixed, 1 kept.** - **Review:** completed — a separate agent context, read-only; head `e8903a2` / base `5b7e1fb`; ran the test file on Node and on Electron's Node, eslint over the renderer, and probed both rules with throwaway code; 5 [fix here], 1 [follow-up]. - Fixed: a test that passed without the guard it named; the header claiming a written-out reference could not be flagged; a fragment after a slash being flagged; four wordings that claimed more than the code does (the `TESTING.md` sentence, both messages, the rule's description). - Fixed though marked follow-up: tests for uppercase hex, hex after `:` and `(`, a template that cannot be read, a namespaced attribute. - Kept: custom rules over `no-restricted-syntax`, with the reason now in each header. - **Review:** completed — a second separate agent context, read-only; head `022aebf` / base `5b7e1fb`; removed each guard in memory and re-ran the suite; 4 [fix here], all fixed. - Three guards could be removed with the suite still green; two now have a test, and the third decided nothing and is gone. - Two comments named the wrong case as pinning the string-only check. - `color()` and `device-cmyk()` were missing from the functions the rule reads. - One of the two reasons given for a custom style rule did not hold. - **Since review:** `022aebf → b9bc2e2` is those four fixes only; the test file, `npm test`, `npm run test:electron` and `npm run lint` re-run on it. </details> <details> <summary>Implementation notes</summary> - The preset from `@wordpress/eslint-plugin` does not configure `no-restricted-syntax`, so nothing is overridden either way. - `RuleTester` is given `node:test`'s `describe` and `it`, so a failing case fails `npm test`; checked with a deliberately wrong case. - The journeys and the packaged smoke were not run: nothing under `src/` or `tests/e2e/` changes. </details> Nothing on screen changes, so there are no screenshots. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
#610) ## Why #609 keeps a colour and an inline style out of the renderer's components. That left the place where all the styling now is, `src/renderer/shell.css`, checked by nothing: a colour typed there would pass. This closes item 9 of #557 for the stylesheet as well. ## What changes **Stylelint over the window's stylesheet**, with six rules and no preset: - The design system's own three, shipped in `@wordpress/theme`: a token that is used has to exist, a token is not given a value in the stylesheet, and a token has no fallback value written beside it. - Three of Stylelint's core rules: no hex colour, no colour by name, no function given a colour's channels (`rgb()`, `hsl()`, `oklch()`, `color()` and the like, in any case). `npm run lint` and the lint workflow run it after ESLint. `tests/unit/stylelint-config.test.cjs` runs the configuration over styles that must pass and styles that must not. **The stylesheet's tokens are written bare.** All 227 hand-written fallbacks (`var(--wpds-…, #707070)`) are gone. The design system forbids them because they drift, and six of ours had: a 24px gap beside a 32px token, `#cc1818` beside a stroke that is `#e0afa7`, and four more. None was ever painting, since the tokens' stylesheet is in the same bundle, so nothing changes on screen from those. **New dev dependency:** `stylelint` ^17.16.0. It is the peer the design system's plugins are written for. Nothing of it reaches the packaged app. **Not in this PR:** a preset (Stylelint's standard one would bring a few hundred formatting findings to a file written before it, the same reasoning as for Prettier in `eslint.config.mjs`), and the build plugin that injects generated fallbacks, which the app does not need. ## How to test this Platforms: any. From the repository root, after `npm ci`: ``` npm run lint node --test tests/unit/stylelint-config.test.cjs ``` Both pass. To see it bite, add `.x { color: #cc1818; }` to `src/renderer/shell.css` and run `npm run lint`: Stylelint reports `color-no-hex`. I planted ten kinds of style this way (hex, a name, `rgb()`, `RGB()`, `color()`, an unknown token, a fallback, a token given a value, and controls that must pass); each came out as it should. The config's test is red when any one of its rules is weakened: I switched off or loosened eight settings in turn and it failed each time. **Two things to look at by hand** (any platform, the current head): 1. Do anything that raises a confirmation in the bottom corner, such as saving a patch. **It has no drop shadow now**; it keeps its border and tint. See Risks. 2. Apply a patch that conflicts. The conflicting lines are in the same monospace as the terminal and the diff. **What must not have happened:** no colour, gap or size anywhere else changed. I checked that mechanically: the old stylesheet with its fallbacks stripped differs from the new one only in the header comment, the shadow and that one font line, and all 55 tokens the file uses are defined in the tokens stylesheet the bundle loads. All 99 journeys pass, including the ones that assert colours against tokens. ## Risks and limitations - Review: 2 passes, 6 findings fixed here, 1 answered in a comment. Details below. - **The confirmation's shadow is gone.** It was written with `--wpds-elevation-md`, a token the design system has withdrawn while its elevation model is being defined, so the shadow was only ever the hand-written fallback. The prototype's confirmations have none. If one is wanted back, it has to be a hand-written value with the rule disabled on that line. - **The conflict lines' font** changes from `ui-monospace, SFMono-Regular, Menlo` to the design system's mono stack, like every other code pane. - **What the rules do not hold the stylesheet to:** a gap, a radius or a font that is not a token passes; so do the system's own colours (`Canvas` and the like), which nothing here uses. - **One journey failure I could not reproduce.** In one of three full local runs, `toasts.spec.js:52` failed; it passed in the other two and 7 times out of 7 with its file run alone. I did not capture its error. The journeys take keyboard focus on a machine in use, which has failed this kind of test before, so I believe that is what it was, and CI will say otherwise if not. - `npm ci --omit=dev` would now install Stylelint's tree, because it is an optional peer of `@wordpress/theme`. Nothing in the repository installs that way. ## Related Part of #557, after #609. --- <details> <summary>Review outcome (required — see AGENTS.md)</summary> **Pass 1: 4 [fix here] · 1 [follow-up]. Pass 2 (on the fix-up): 1 [fix here] · 1 [follow-up]. 6 fixed, 1 answered in a comment.** - **Review:** completed — a separate agent context, read-only; head `72c3814` (rebased since) / base `5b7e1fb`; diffed the stylesheet with its fallbacks stripped, checked every token against the bundled tokens file, probed the rules with styles on stdin, read the lockfile diff and what electron-builder packs; 4 [fix here], 1 [follow-up]. - Fixed: the function rule matched lower case only, so `RGB()` passed; a `TESTING.md` sentence said a gap and a font had to be tokens; the config's header leaned on files another pull request added; an unused `lint:css` script (the follow-up). - Answered in the config's comment, with no rule added: the system's own colours pass. - One statement in that review was wrong and is not acted on: that the shadow never painted. On the base the declaration carried a fallback, and the fallback painted. - **Review:** completed — a second separate agent context, read-only; head `d13ad2c` / base `1e9133c`; 1 [fix here], 1 [follow-up], both fixed. - The stylesheet's header still said a gap, a radius and a font are tokens. - Nothing tested the configuration, which is now `tests/unit/stylelint-config.test.cjs`. - **Since review:** `d13ad2c → 3c1edff` is those two fixes, the conflict lines' font and four wordings the second review noted; lint, `npm test`, `npm run test:electron` and the journeys re-run on it. </details> <details> <summary>Implementation notes</summary> - Lockfile: 82 packages added, none removed or re-resolved, all from the npm registry and none with an install script. Eight existing entries change from `dev` to `devOptional`, because `stylelint` and `postcss` are optional peers of `@wordpress/theme`. - The packaged app does not contain Stylelint: the asar listing has no entry for it, and the packaged smoke passes. - The lint job is still called `eslint`, so a required check keeps its name. - `.stylelintignore` keeps the built `src/renderer/index.css` out, which is every library's styles in one file and is not in Git. - Local checks on macOS: `npm run lint` clean, `npm test` and `npm run test:electron` pass (1920 tests), `npm run test:e2e` 99 pass in two runs of three (see Risks), `npm run test:e2e:packaged` 15 pass. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload images from the command line. The two steps under "Two things to look at by hand" show both visible changes. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…l or build runs, and say why (#611) ## Why The site menu's **Update to latest trunk** item was always enabled, but `startTrunkUpdate` returns at once while an update, an install or a build is running. Choosing the item then did nothing and said nothing: no line in the terminal, no notice, no change on the page. The review standard names this shape: a control that swallows a click is a control that does nothing. ## What changes The root cause is that the guard and the control did not share an answer: the hook knew the update could not start, and the menu was never told. - `updateHeldReason` in `src/renderer/site-menu.cjs` is now the one answer to "can an update start": `''` when it can, otherwise a sentence naming what is in the way ("Wait for the trunk update to finish.", "Wait for the installation to finish.", "Wait for the build to finish."). The update leads, since its own install and build are running while it is. - `useTrunkUpdate` asks it for its own guard and returns it as `updateHeld`, so the guard and the controls cannot disagree. - `siteMenuItems` takes it and returns the item with `disabled` and a `description`; the menu draws the description under the item's name with `Menu.ItemDescription`, which the design system wires as the item's `aria-describedby`. A held item is still reachable with the keyboard. - The stale-site notice's **Update to latest trunk** button was already disabled during an install or build, with no sentence. It is now a `ReasonedUiButton` held with the same reason, so there is no second inline answer. - `docs/guide/trunk-updates.md` described the old behaviour ("clicking it in that state simply does nothing") and now describes this one. Deliberately not in it: the case where the terminal is held by something that is not an install or a build (a typed `npm run lint`, say). The item stays enabled there and the update is refused in `beginTrunkUpdate` with a line in the terminal, as before. ## How to test this Platforms: any. Nothing here touches paths, spawning or line endings. **Covered by a journey.** `a site with no build is built before it is watched…` in `tests/e2e/journeys/build-watch.spec.js` walks the reported path: a Gutenberg site with no build, **Start build watch** so its build is running, then the **Site actions** menu. It asserts the item is disabled, that its accessible description and the text on it are the reason for a build, and that after the build ends the item is enabled with no description. Run it from the repository root, after `npm ci` and `npm run build:once`: ``` npx playwright test --project=journeys -g "a site with no build is built before it is watched" ``` It is red without the change: with the menu not told the update is held, it fails on `toBeDisabled` ("unexpected value "enabled""). Checked by removing `updateHeld` from the menu's props, rebuilding and running that one test. **What the journey cannot reach** is a real install or build, since it stands in for the script runner. On a real site, with the build of this branch's current head (Buildkite, either platform): **Starting state:** a WordPress Core site whose setup has finished, with nothing running. 1. In the site's terminal, type `npm run build` and press Enter. 2. While it runs, open the **⋮ Site actions** menu. **Expected:** **Update to latest trunk** is greyed out and cannot be chosen, and under its name it says "Wait for the build to finish.". Every other item is as it was. 3. Close the menu, wait for the build to end, open the menu again. **Expected:** the item is enabled and has no line under it. 4. Choose it. **Expected:** the update starts (the three-step card appears), or "Already up to date" is confirmed. 5. While that update runs, open the menu again. **Expected:** the item is held and says "Wait for the trunk update to finish.", through the install and build steps too, not "Wait for the build to finish.". **What must not have happened:** - The item must not be held when nothing is running, and must not stay held after the build or the update has ended. - No other item of the menu is disabled or gains a second line. - With a command other than an install or build running in the terminal (type `npm run lint`), the item is still enabled, and choosing it on a clean tree prints "A command is already running. Press Ctrl+C to stop it." in the terminal, as before. **Not tested by hand:** the stale notice's button with its tooltip needs a site whose trunk is more than 14 days old and an install or build running on it; its reason comes from the same function and the same component the ticket actions use, and no journey reaches that pair of states. I ran the journeys and not a real `npm run build`. ## Risks and limitations - Review outcome: 0 `[fix here]` · 3 `[follow-up]`, one of them folded in. Details in the collapsed block. - **The bottom tray is not on `redesign/wpds` yet.** The brief for this change described the terminal-held refusal as opening the tray on the Terminal (#558). On this base, `beginTrunkUpdate` only prints the line; `app-footer.jsx` still says "once there are trays". This PR leaves that path exactly as it found it, so it composes with the tray work whichever lands first. - **Left out, found in review:** with the terminal held by a typed command and edits in the tree, the update's question about the edits is asked before the terminal is checked, so choosing to discard throws the edits away for an update that is then refused. This predates the PR and is traced in code, not reproduced. It deserves its own fix and journey rather than a widening of this one. - The three sentences also exist, not yet wrapped for translation, in `ticket-actions.cjs` and `watch-activity.cjs`. Here they are wrapped, because the menu is a translated area. Under a translated locale the same sentence will be translated on the menu and English on a ticket action until that area is wrapped. - The setup of a new site runs an install and a build, so the item is held during it, with the install's or the build's sentence. Before, it was enabled there and did nothing. ## Related No issue. The always-enabled item came with the site menu (#556). --- <details> <summary>Design decisions and alternatives considered</summary> - **A description on the item, not a tooltip.** `ReasonedUiButton` pairs a tooltip with a hidden description because a button has nowhere else to put a sentence. A menu item does: `Menu.ItemDescription` is the design system's own slot for it, it is visible without hovering, and it is wired to `aria-describedby` by the menu itself. - **The reason lives in `site-menu.cjs`**, beside the items it is said on, and that module is already translated. `update-plan.cjs` was the other candidate; it is not translated and is not where a sentence on a control belongs. - **The hook's guard stays**, asking the same function. With both controls held it is no longer what a contributor meets, but a state change between a render and a click should still not start a second chain. - **Assertions added to an existing journey** rather than a new test: the fixture and the running build were already there, and a new test costs an app launch on both platforms for every pull request. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **0 `[fix here]` · 3 `[follow-up]`** — one folded in, two left as follow-ups. 1. Architecture · 🔵 · `[follow-up]` — the stale notice's button answered "can an update start" inline (`disabled={installing || building}`) and gave no reason. **Folded in:** it is held with `updateHeld` through `ReasonedUiButton`. 2. Architecture · 🔵 · `[follow-up]` — the same sentences are unwrapped in `ticket-actions.cjs` and `watch-activity.cjs`. **Deferred:** those modules are not translated yet; wrapping them belongs to that area's i18n pass. Named in Risks. 3. Architecture · 🔵 · `[follow-up]` — discard-then-refused with the terminal held by a typed command (see Risks). **Deferred:** predates this change, outside its scope, and needs its own reproduction. Checked and found sound by the reviewer: the guard and the menu read one value from one render; no new stuck state under the #507 rule (while the update waits on the watch the item was already refused by the guard, and **Stop build watch** still ends the wait); `Menu.ItemDescription` is used within its contract and the item's name is still its label, so the exact-name locator matches; the unit tests and the journey's `toBeDisabled` fail on the old code; every other journey and `scripts/screenshots/shots.cjs` open the menu while idle. Deterministic layer, on the committed head: `npm run lint` clean, `npm test` 1923 pass, 0 fail, 2 skipped, `npm run test:e2e` 99 passed (macOS only; Windows is CI's). - **Review:** completed — a separate agent context (Claude, fresh context, given only the diff and the review standard); reviewed the staged diff on base `bda9083` before it was committed; outcome above. - **Since review:** staged diff → `771c88e` checked by the author: the notice's button moved to `ReasonedUiButton` (finding 1), two comments reworded to say "the two controls", and the docs sentence tightened as the reviewer's style note asked. Lint, the fast suite and the journeys were rerun on `771c88e`. That delta was not re-reviewed by a separate context. - **CodeRabbit:** not requested yet. </details> <details> <summary>Screenshots or recording</summary> The held item, while a build runs (macOS, from the journey): the item greyed out, with "Wait for the build to finish." under "Update to latest trunk", and the other five items unchanged. I have the capture locally and could not attach it from the command line; the journey above reproduces it. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…to it (#612) ## Why First part of #558. In the redesign the terminal, the logs and the mail leave the foot of each site's page for a tray along the bottom of the window, as the prototype has them. This pull request builds the tray and moves the terminal into it; the logs and the mail follow in their own pull requests and stay on the page until then. ## What changes **The tray.** Between the page and the footer, closed when the window opens. The footer has a button for it, "Toggle Terminal", pressed while the tray is open; the tray's own Close button puts the focus back on that button. Its top edge is dragged with a pointer or moved with the arrow keys (Home is its smallest, End its largest), from 200px up to half the window. It takes its room from the page, so nothing on the page is under it, and a confirmation in the corner stands clear of it. **The terminal.** The tray is the window's and what is in it is a site's: every site's view fills the tray's slot with its own terminal, hidden unless the site is the open one. A terminal keeps what was printed in it through a close and a look at another site. It is fitted to the tray, as many columns and rows as there is room for, where it was a fixed 80 by 12. **When the tray opens by itself.** On the page the terminal was always in view, and the app leaned on that. So the tray opens on the terminal in the cases whose only word is there: - an install or a build that fails with its output in the terminal (setup, an update, an applied patch); - an action refused because a command is running, which is a line printed there; - an update that cannot fetch, and an apply whose hand-off to the build watch fails. It does not open for a run the contributor stopped, for a failure whose output is in the logs (the build that runs ahead of the watch), or on another site than the one that failed; that site's terminal comes up when the site is opened. The copy that said "the Terminal below" now says "the Terminal". **Not in this pull request:** - The logs and the mail in the tray. - Remembering the tray's height or whether it was open across restarts. The prototype remembers the height; the window's other arrangements (the sites list, the details) are not remembered either. - A slide animation as the tray opens. - The user guide, which still describes a terminal panel on the page: #561 retakes the screenshots and the text. ## How to test this Platforms: any. The journeys below drive every part of this on macOS and Windows. From the repository root, after `npm ci` and `npm run build:once`: ``` npx playwright test --project=journeys tray.spec terminal.spec ``` `tests/e2e/journeys/tray.spec.js` is new, nine journeys: - closed until the footer opens it; shows the open site's own terminal; each terminal survives a close and a look at another site; the page and a confirmation are not under it; - the edge moves by keyboard and by pointer, stays inside its limits, a drag that loses the pointer is over, and the terminal is fitted (rows and columns) at every size; - a failed build, an update that cannot fetch and a refusal each bring the terminal up; a stopped build, a failure whose output is in the logs, a failure on another site and a site deleted mid-build do not. I removed or broke 26 behaviours one at a time; 25 turned a journey red, and the one that did not was a redundant clamp, which is gone. `tests/unit/tray.test.cjs` and `tests/unit/terminal-hints.test.cjs` hold the decisions (limits, keys, the terminal's grid, which run's end shows the terminal). **Worth a look by hand** (any platform, the current head): open a site, press the icon at the left of the footer, drag the tray's top edge, type `help`, switch site and back, close it. Then choose **Update to latest trunk** on a site with no remote, or let a build fail: the tray opens on the reason. **What must not have happened:** a site's terminal losing its output when the tray is closed or another site is opened; the tray opening for something the contributor did on purpose (a stop); anything on the page ending up under the tray. **What cannot be tested by hand here:** "Stop setup" during a real first setup, which needs a clone and an install that take minutes. The stop is decided in one place for every run, and a journey covers it through Ctrl+C. ## Risks and limitations - Review: 2 passes, 13 findings fixed here, 4 left as follow-ups, 1 design question. Details below. - **The footer's button is an icon with a tooltip, as in the prototype, and the copy now says "the Terminal" without saying where it is.** A failure opens the tray, so nobody is left looking for the output of one. The sentences that are not failures ("Run npm run build in the Terminal", "the Terminal shows what it is doing") point at a tray the contributor has to find. The second reviewer suggested a visible "Terminal" label beside the icon; I have left that as a design decision. - **A build's output is no longer always in view.** During a long setup or update the page shows the steps and the terminal stays closed unless it is opened. - Erasing typed input breaks once the line wraps. It did before; a terminal that follows the window's width makes it easier to reach in a narrow window. - The hint sentences under the terminal are still not translatable, as before. ## Related Part of #558. Mail HTML rendering is unchanged and tracked separately. --- <details> <summary>Design decisions and alternatives considered</summary> - **In the page's flow, not laid over it.** The prototype lays the tray over the foot of the page and pads the page by its height. Here it is a row of the window's column, so the page is simply shorter: nothing to pad, and the page's own scrolling and its sticky details need no knowledge of the tray. - **Never unmounted.** A terminal is drawn in one element for as long as it lives, and the prototype's tray, which mounts only what is selected, would cost every other site its terminal. The tray and every site's panel stay in the document, hidden. - **The terminal is made when the site's view mounts and drawn when first shown.** What a site prints before then is kept in the terminal's buffer. Its element arrives through the tray's slot a render later, so it is state and not a ref. - **One reveal function per site**, which waits if the site is not the open one, rather than opening the tray on whatever site is in front. - **A run's caller says where its output goes** (`outputInTerminal`), and the runner knows whether it was asked to stop. Opening the tray only where the page points at the terminal was considered and is the same thing with more places to keep in step. - **A splitter, not a button**, for the edge: it can be focused, moved with the arrow keys, and says how tall the tray is. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **Pass 1: 8 [fix here] · 3 [follow-up]. Pass 2 (on the fix-up): 3 [fix here] · 3 [follow-up]. 13 fixed (7 and 6), 4 left as follow-ups (the guide, deferred to #561, and pass 1's three), and 1 design question.** - **Review:** completed — a separate agent context, read-only; head `480ee0e` / base `bda9083`; lint and the tray's unit tests run, the journeys read and not run; 8 [fix here], 3 [follow-up]. - Fixed: the app said "the Terminal below" with the tray closed, and refusals were printed only there (the tray now opens for a failure or a refusal; decided with the maintainer); the tray's floor left the terminal no rows; no test checked the terminal's columns; one layout check read positions once; Home and End were the reverse of a splitter's, and the edge did not name its pane; a drag could get stuck; confirmations covered the open tray. - Deferred to #561: the user guide. - Follow-ups: wrapped input, the untranslated hints, a terminal that cannot move to a new element (no path does). - **Review:** completed — a second separate agent context, read-only; head `4e0cdff` / base `bda9083`; every caller of the two runners and of the kill traced; 3 [fix here], 3 [follow-up]. - Fixed: "Stop setup" opened the tray on a stop that was asked for; an update that fails before it runs anything said so only in the terminal; a site deleted mid-build opened the tray on the next site. - Fixed though marked follow-up: the journey measured a column off an element xterm keeps for itself; an assertion at the smallest height that could not fail; the decision moved into `terminal-hints.cjs` with a unit test, and the watcher says its output is not in the terminal. - Left as a design decision: a visible label on the footer's button. - **Since review:** `4e0cdff → d89b1a8` is those fixes, and `edd378a` merges `redesign/wpds` (#611), with one conflicting line in the site view resolved to keep both changes; lint, `npm test`, `npm run test:electron`, all 108 journeys, the small-window and slowed runs of the tray's journeys and the packaged smoke re-run on it. Then `f595b31`, after both journey jobs failed on `edd378a` in the new edge journey, for reasons of the journey's own: on Windows it measured the terminal before the site's status had brought the two hints under it, and on macOS its own measure of a column came out a hair wider than the terminal's on the runner's font. The journey now waits for the hints and counts columns from the room left over; no app code changed. </details> <details> <summary>Implementation notes</summary> - New: `src/renderer/tray.cjs`, `src/renderer/components/bottom-tray.jsx`, `tests/unit/tray.test.cjs`, `tests/e2e/journeys/tray.spec.js`. `runFailedInTerminal` is in `src/renderer/terminal-hints.cjs`. - The tray's height reaches the stylesheet as a property the component sets from an effect (`--app-tray-height` on the tray, `--app-tray-inset` on the document for the confirmations), since the lint rules forbid an inline `style`. - A dozen journeys now open the Terminal tray first; `ui.openTray`, `ui.tray` and `ui.trayToggle` are in `tests/e2e/helpers/ui.cjs`. - Local checks on macOS: `npm run lint` clean, `npm test` and `npm run test:electron` pass (1930 tests), `npm run test:e2e` 108 pass, `npm run test:e2e:packaged` 15 pass. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload images from the command line. The steps under "Worth a look by hand" show everything that changed. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…a server that fails (#613) ## Why Second part of #558. #612 built the tray along the bottom of the window and moved the terminal into it. This pull request moves the logs there, as the prototype has them; the mail follows in its own pull request and stays on the page until then. ## What changes **A second button in the footer, "Toggle Logs".** The tray holds one thing at a time, the terminal or the logs, and each button puts its own in the other's place or closes the tray. **The three logs are tabs in the tray**, drawn with the design system's tabs: Server, Build watch, Debug.log. The selected log fills the tray and follows its height; debug.log's path and its three buttons (Show in folder, Copy, Clear) are along the bottom of its tab. The Logs section is gone from the page. Each site keeps its own logs and its own selected tab, as each keeps its own terminal. **The tabs follow the app's choice of tab (a fix).** The old tab panel kept its own idea of which tab was open, so the app's "show the watch's tab" never changed what was on screen. The tabs are controlled now: starting the build watch on a site with no build shows the watch's tab, as the code always meant. **When the tray opens by itself.** On the page the logs were always in view, and two failures have their only words there. So the tray opens on the Logs: - on the Build watch tab, when the build watch ends by itself or the build that runs ahead of it fails; - on the Server tab, when the development server stops by itself or cannot start. The logs never take the terminal's place: someone may be typing there. With the terminal open the tab is still selected, and the page says what happened and where its last lines are, which for the server is new: "The development server stopped by itself. Its last lines are in the Logs." and "The development server could not start. Its last lines are in the Logs.", under the server's details until it is started again. The terminal may still take the logs' place for a failure or a refusal printed there. A watch or a server stopped by its own button brings nothing up, and a failure on another site than the open one waits until that site is opened. **Labels follow the prototype:** "Build watcher" is "Build watch" and "debug.log" is "Debug.log" on the tabs. Two sentences that quoted the header's label ("Wait for the header to say Build watching") no longer quote anything: the label is hidden in a narrow window. **Not in this pull request:** - A count of unseen debug.log lines on the footer's button while the tray is closed. The count is on the Debug.log tab, as before, so a closed tray hides it. It comes as the next pull request. - The mail in the tray. - The user guide, which still shows the logs on the page: #561 retakes the screenshots and the text. ## How to test this Platforms: any. The journeys below drive this on macOS and Windows. From the repository root, after `npm ci` and `npm run build:once`: ``` npx playwright test --project=journeys tray.spec logs.spec build-watch.spec dev-server.spec ``` - `tray.spec.js`: the tray holds one thing at a time; a watch that fails brings the logs up, one stopped by its button brings nothing, and neither takes the terminal's place; a refusal by the watch is said in the terminal; a site whose deletion fails brings nothing up later. - `logs.spec.js`: the Logs tray shows debug.log's lines, the pane grows with the tray, lines that arrive while the tray is away are there when it comes back, and the pane is at its end after a reopen. - `build-watch.spec.js` and `dev-server.spec.js`: the watch's tab is selected when the watch ends; a server that crashes with the tray closed opens the Logs on the Server tab and the page says so; a start that fails under the terminal leaves the terminal, and the page says so; the sentence goes when the server is started again. I broke 20 behaviours one at a time; 19 turned a journey red, and the one that did not was a guard nothing needed, which is gone. `tests/unit/tray.test.cjs` and `tests/unit/site-processes.test.cjs` hold the decisions (which request takes the tray, what the server's details say). **Worth a look by hand** (any platform, the current head; the journeys stand in for the server and the watch, so this is the part they cannot reach): on a site that is set up, press **Start development server**, open the Logs from the footer, and look at the three tabs while the site is used: the server's lines under Server, the watch's under Build watch, and under Debug.log whatever `wp-content/debug.log` gets, with **Show in folder**, **Copy** and **Clear** along the bottom. Drag the tray's top edge: the log follows it. Open another site and come back: the tab that was selected still is. **What must not have happened:** a log losing lines while the tray was closed, was showing the terminal, or another site was open; the logs taking the place of a terminal someone is typing in; the tray opening for a watch or a server the contributor stopped. **What cannot be tested by hand here:** a real server that fails to start or dies, which I could not stage. Main tells the window of a failed start twice, with an answer and with the server's exit, so which of the two new sentences a real failed start gets depends on which arrives first; both say where the lines are. The journeys cover each message on its own. ## Risks and limitations - Review: 2 passes, 12 findings fixed here, 1 answered only in part (the next but one point), 4 left as follow-ups. Details below. - **The logs are no longer always in view.** A new line in debug.log shows nowhere while the tray is closed, until the next pull request puts its count on the footer's button. - **With the terminal open, a failed watch or server does not open the logs.** That is the rule chosen for this tray; the page's sentence is what says it. - A kill that outlasts a failed deletion's five-second wait can still bring the logs up afterwards, for an ending the deletion caused. The path where the kill lands in time is covered. - After a failed deletion the page says the build watch "ended by itself", and when a start is refused because a command holds the terminal the Server tab says the build watch "stopped before build/ was complete". Both sentences were there before and are untrue in those two cases. - The logs' strings are still not translatable, as before. - The packaged smoke failed once, at "the packaged app boots and paints its first window", on the run straight after packaging, and passed 15 of 15 on the next run with nothing changed. I could not reproduce it. ## Related Part of #558. Follows #612. Mail HTML rendering is unchanged and tracked separately. --- <details> <summary>Design decisions and alternatives considered</summary> - **The logs never replace the terminal; the terminal may replace the logs.** Three rules were put to the maintainer: this one, whatever fails takes the tray, and nothing ever swaps an open tray. Under the second a watch that dies takes the tray from someone typing a command; under the third a refusal printed in the terminal while the logs are open looks like a button that did nothing. The rule is one function, `trayAfterReveal` in `src/renderer/tray.cjs`, and a request made while the site is not the open one is kept by the same rule. - **The page says a server failed.** With the logs kept out of the terminal's way, a failed start under an open terminal had no words on screen at all. The server's entry gets a failed state with a sentence, as the watch's already had. - **Controlled tabs.** The app chooses the tab when something happens (a watch starting, a server failing), so the selection is the site's state and not the tab component's. - **All three logs stay in the document's memory, one is drawn.** The design system's tabs unmount the panels that are not selected; the lines are the site's state, so nothing is lost, and each pane goes to its end when it is drawn again. - **Debug.log's buttons stay with its tab**, along the bottom, and not in the tray's title bar: they act on one log, and the prototype's tray has no place for them at all. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **Pass 1: 6 [fix here] · 3 [follow-up]. Pass 2 (on the fix-up): 6 [fix here] · 2 [follow-up], counted from its list; its own headline said 5 and 3. 12 fixed (7 and 5), 1 answered only in part, 4 left as follow-ups.** - **Review:** completed — a separate agent context, read-only; head `3afcd88` / base `2ef8177`; lint and the unit tests run, the journeys read and not run; 6 [fix here], 3 [follow-up]. - Fixed: the screenshot script looked for a tab by its old name; the server's failures were said only in a tab of a closed tray (the tray now opens for them); the logs took the terminal's place while someone might be typing there (they no longer do; the rule was the maintainer's choice of three); a site whose deletion fails kept a pending request that fired later; two sentences quoted a header label that a narrow window hides; the real-setup journey could no longer tell an idle watch from one that had exited; and, marked follow-up, a "nothing opened" check that could pass by timing. - Follow-ups: the count of unseen debug.log lines while the tray is closed (the next pull request); the untranslated strings. - **Review:** completed — a second separate agent context, read-only; head `1c0d06f` / base `2ef8177`; lint and the tray's and the watch's unit tests run, every request to show the tray traced to what ends up on screen; 6 [fix here], 2 [follow-up]. - Fixed: a server failure under an open terminal was silent (the page says it now); a request kept for a site that is not open was last-write-wins, against the tray's own rule; two journey assertions that could not fail; "Wait for it to finish" said of a watch that never finishes. - Answered in part: a kill that outlasts a failed deletion's wait still brings the logs up, against what a comment claimed. The comment now says what is and is not covered and the gap is in Risks; closing it means main telling the window which endings a deletion caused, which is more than this change should carry. - Follow-ups: the Server tab's cancelled line when the cause is a refusal; the watch's status after a failed deletion. Both predate this change. - **Since review:** `1c0d06f → 637d29a` is those fixes, not reviewed again. The fix for the silent failure is the one the reviewer proposed, and a journey pins that the sentence goes when the server is started again (red without the line that clears it). The run with a slowed status read then showed one journey of the header's racing the site's status, one time in five: it read where the page was while the app was still bringing the site's next step into view. It now waits for the status and has the app jump instead of glide; ten slowed runs pass. Lint, `npm test`, `npm run test:electron`, all 111 journeys, the small-window and slowed runs of the affected journeys and the packaged smoke re-run on `637d29a`. </details> <details> <summary>Implementation notes</summary> - New: `src/renderer/components/logs-panel.jsx`. `trayAfterReveal` is in `src/renderer/tray.cjs`; the server's failed state is `serverProcess`'s in `src/renderer/site-processes.cjs`, fed by `serverFailure` in `use-dev-server.jsx`, which is cleared when a start is asked for. - `useSiteLogs` takes `shown` and hands out callback refs for the three panes, since a pane is in the document only while its tab is selected and the tray's slot fills a render after the site's view mounts. - The build that runs ahead of the watch and the watch itself say their output is not in the terminal (`outputInTerminal: false`), so their failure opens the logs and not the terminal. - Journeys that read the watch's state off a tab's label now read it off the header's process menu, which is on screen whatever the tray shows. - Local checks on macOS: `npm run lint` clean, `npm test` and `npm run test:electron` pass (1937 tests), `npm run test:e2e` 111 pass, `npm run test:e2e:packaged` 15 pass. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload images from the command line. The steps under "Worth a look by hand" show everything that changed. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
## Why Part of #558, after #613 moved the logs into the tray. The count of debug.log lines nobody has looked at is on the Debug.log tab, and the tab is on screen only while the tray shows the logs. With the tray closed, or showing the terminal, a PHP error arrived with nothing to show for it. On the page the count was always in view. ## What changes **The footer's Logs button counts them.** A number on the button's corner, for the site that is open, and the words that say what it counts ("3 unseen lines in Debug.log") as the button's description. The button's name is still "Toggle Logs". Past 99 the number reads "99+" and the words still say how many. **What counts as seen.** A line was unseen while another tab was selected. It is now unseen unless debug.log's pane is in front of someone: the Logs are what the tray shows, for the open site, with Debug.log the tab selected. A tab left selected in a closed tray is not being read. The tab's own count follows the same rule. **What the file already held is not news.** Starting the server shows what debug.log held, with the app's line under it that says where the earlier run ends, and that was counted: every start of the server on a site with history would have lit the button before anything had happened. The main process now says which lines the file held before the server started, and those are shown and not counted. Two things follow from doing that properly: - The file is watched from before the server starts, where it was watched once the server was up. A line WordPress logs while it boots is this run's, and is counted. - A debug.log that first appears while the server runs, the common case, is news from its first line, and the "earlier run" line is no longer put under it. That line was untrue there before this change too. **A tab's panel has its room from the moment its tab is selected** (its own commit). The design system's tabs keep the panel that is leaving in the document until the next frame is drawn, and for that long it shared the room with the panel coming in: a log that opened 42.5px short and then grew, and on the apply card fields two tabs tall and then one. This is what the macOS runner measured in #613's logs journey, three attempts in four. Every panel but the selected one is now out of the layout. **Not in this pull request:** - Unseen lines of a site that is not the open one. They are counted, and the button says so when the site is opened; nothing in the sites list says it before. - The words on hover. The tooltip says "Toggle Logs"; the number is drawn and its words are for a screen reader. - The user guide, whose page on the logs describes the old count: #561. ## 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 logs.spec apply-card.spec ``` - `logs.spec.js`, "debug.log shows what the file holds, counts what arrived unseen on its tab and on the footer…": what the file held at the start is told to the window and counted as nothing; lines written afterwards are counted on the tab and on the footer, one line and then two with the tray closed and under the terminal; another site's page shows none of it; coming back to the tab clears both. - "a debug.log that first appears while the server runs is news from its first line": counted, and no "earlier run" line under it. - "what WordPress logs while the server is starting is news…": the stand-in server is held mid-start while a line is written. Red with the file watched only once the server is up. - Both files check that one panel is taking room straight after a tab is selected, and again when the tab just left is gone back to within the same frame. I broke 17 behaviours one at a time and each turned a journey red. `tests/unit/debug-log.test.cjs` holds the decisions (when the pane is in front of someone, what a chunk adds to the count, what the button says) and `tests/unit/preload-listeners.test.cjs` the bridge's part. **Worth a look by hand** (any platform, the current head; I have not done this, the journeys stand in for the server): on a site that is set up, press **Start development server** with the tray closed. The Logs button shows no number, even if `debug.log` has lines from before. Make WordPress log something, for example by adding `error_log( 'seen?' );` on a new line after the opening `<?php` of `src/wp-login.php`, waiting for the build watch, and opening the site's login page. The button shows **1**. Open the Logs and select **Debug.log**: the number goes. **What must not have happened:** a number on the button straight after a server start; a line losing its place in the count because the tray was closed, the terminal was showing, or another site was open when it arrived; the log pane or the apply card's fields jumping in height as a tab is selected. **What cannot be tested by hand here:** what a real WordPress logs while it boots. The journey holds a stand-in server mid-start and writes the line itself. ## Risks and limitations - Review: 2 passes, 7 findings fixed here, 1 decided by the maintainer and done, 1 follow-up applied here, 4 left as follow-ups. Details below. - **The Debug.log tab's count changes too**, not only the footer: it no longer counts what the file held at the start, and it counts lines that arrive while the tab is selected in a tray nobody is looking at. - **The file is watched before the server starts.** A start that fails now empties the pane and shows the file again, where it used to leave the pane as it was. - **The "earlier run" line's fix is here and not on trunk first**, because the flag that makes it possible is introduced here. Trunk keeps the old line until the redesign lands. - A line that arrives in the instant the tray is being closed or switched by the app itself (a failure bringing the terminal up) can go uncounted. One task wide; the same instant for a tab the app selects is closed. - A line written while the file is being attached to can be shown twice, as before, and is now counted once. A file replaced in place is sent again in full and counted again. Both are in the tail's reads, on trunk as well, and flagged separately. - A third button in the footer, the mail's, would sit under a "99+". The mail's pull request places it. - `toasts.spec.js:52` failed once in a full local run with "Cannot fast-forward to the past" and passed on the next. It is the second time I have seen it and it is not from this change; flagged separately. ## Related Part of #558. Follows #613. Mail HTML rendering is unchanged and tracked separately. --- <details> <summary>Design decisions and alternatives considered</summary> - **A description, not a longer name.** The button is found by its name, by people and by the journeys, and a name that changes with every line is a button that keeps being announced. The count is state, so it is the description. - **A slot in the footer that the open site fills**, as the tray's contents and the header's actions are. The alternative, the count lifted into the window's state, redraws every site's view for every line. - **The main process says what is backlog.** The renderer could have looked for the app's marker line in the text, which ties the count to a sentence. Main knows which read is the one made as the tail starts. - **The backlog is not counted at all**, the maintainer's choice of two. Counting it, as the tab did, makes the button mean "debug.log is not empty" on most starts. - **`inert` and not the tabs' own mark for a leaving panel.** A panel gone back to before the next frame still carries that mark, and was hidden for the frame when I used it. `inert` is on every panel but the selected one and on nothing else that is a tab's panel here: the old tab panel in the mail dialog never has it, and the app's own `inert` is on ancestors. - **One rule for every set of tabs**, by role, and not one per place they are used: the mail's tabs in the tray are next. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **Pass 1: 3 [fix here] · 3 [follow-up]. Pass 2 (on the fix-up): 4 [fix here] · 3 [follow-up]. All 7 [fix here] fixed. Of the follow-ups: 1 was the maintainer's decision and is done, 1 is applied here, 4 are left.** - **Review:** completed — a separate agent context, read-only; the change as staged on base `6be1ee1`, before it could be committed (signing was unavailable; the commits are `324b00e` and `381be54`); lint and the unit tests of the module run, the journeys read and not run; 3 [fix here], 3 [follow-up]. - Fixed: the journey's tab check chose tabs by name across every site's copy in the tray; one "stays empty" check ran before the line it was about could have arrived; a line arriving as the app itself selects another tab went uncounted. - Decided by the maintainer and done: whether what the file held at the start counts (it does not). - Applied here though marked follow-up: the apply card's tabs have the same frame of two panels (one rule for both). - Deferred to #561: the user guide. - **Review:** completed — a second separate agent context, read-only; the whole working change on base `6be1ee1`; lint and two unit files run, every path into the tail's attach and every way the pane leaves the screen traced; 4 [fix here], 3 [follow-up]. - Fixed: the tail started after the server, so what WordPress logged while booting was marked as the earlier run's and not counted; the "earlier run" line was sent under a file that appeared during the run; the new journey waited for that line and could miss it when the file was seen empty; the count's rule was in the hook and is now in `debug-log.cjs` with a unit test. - Follow-ups: the tail's reads have no end, so a line can be sent twice; a file replaced in place is sent again; a one-task window when the app itself takes the logs off screen. - Nothing found: the wider CSS selector (every element with the role, and everything that can be `inert`), the bridge's new argument, the journey helper. - **Since review:** the second pass's fixes are not reviewed again. One journey was added after it, for a line logged while the server starts, because the stand-in server logged nothing and moving the tail back did not turn anything red. Lint, `npm test`, `npm run test:electron`, all the journeys, the affected journeys at three window heights and with a slowed status read, and the packaged smoke were run on the tree that became `381be54`. </details> <details> <summary>Implementation notes</summary> - `src/renderer/debug-log.cjs`: `debugLogOnScreen`, `unseenIn`, `unseenLinesNote`. `src/renderer/hooks/use-site-logs.jsx` keeps whether the pane is on screen in a ref the tail's callback reads; it is cleared at once when another tab is selected and set by a layout effect. - `src/main.js` `startWpDebugTail`: `send( data, backlog )`; `attachFileWatcher( fromBefore )` is true only for the attempt made as the tail starts. `src/preload.js` hands the callback `{ backlog }` as a second argument. - `src/renderer/hooks/use-dev-server.jsx`: `startDebugTail()` before `startServer`, and `stopDebugTail()` on the one way out that does not go through `stopDevServer`. - `src/renderer/components/app-footer.jsx`: `LogsToggleNote` and the slot, which is the element `aria-describedby` names, so an empty slot is an empty description and nothing dangles. - `tests/e2e/helpers/ui.cjs` `panelsTakingRoom`: selects tabs and counts panels inside the page in one turn, since a frame is what must not come between. - The logs journeys keep what the window is told of debug.log (`debugTold`), because a pane that is not on screen cannot say a line has arrived. - Local checks on macOS: `npm run lint` clean, `npm test` and `npm run test:electron` pass (1943 tests), `npm run test:e2e` 113 pass, `npm run test:e2e:packaged` 15 pass. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload images from the command line. The steps under "Worth a look by hand" show everything that changed. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
## Why Third and last part of #558. The terminal (#612) and the logs (#613) are in the tray along the bottom of the window; the mail a site's WordPress sent was still a section at the foot of each site's page. This moves it into the tray, as the prototype has it. ## What changes **A third button in the footer, "Toggle Email".** The tray holds one thing at a time: the terminal, the logs or the mail. **The mail in the tray.** The open site's list, newest first, filling the tray, and under it where the mail server is listening. A row opens its mail in the dialog, as before. The Mail section is gone from the page. Each site keeps its own list, as each keeps its own terminal and logs. **"Clear emails" is in the tray's heading**, beside the button that closes it, and cannot be pressed while the list is empty. That is the prototype's, and it takes one thing away: the list is empty until the dev server has first run, whatever the app holds from before, so mail kept from an earlier session can be cleared only once the server has been started. Before, the button could always be pressed. **The logs take nothing's place when the app brings them up.** They gave way to the terminal alone, where someone may be typing; with the mail open someone may be about to open one. They come up by themselves only in a tray that is closed or already showing them, and the page says where a failure's lines are, as it does under the terminal. The terminal still takes the tray whatever is in it. **The count on the Logs button** starts at the footer's top edge and is one line tall, so that a wide one is clear of the envelope beside it, and is drawn over the buttons. **Labels follow the prototype:** the tray is "Email", and an empty list says "No mail yet. WordPress transactional email will appear here once the site sends any." **Not in this pull request:** - A sign that mail has arrived while the tray is closed or showing something else. On the page the list was always in view; now such a mail shows nowhere until the tray is opened. Left out on purpose, by the maintainer's decision. - The dialog a mail opens in. Mail HTML rendering is unchanged and tracked separately. - The user guide and the README, which still describe a Mail panel on the page: #561. ## 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 mail.spec tray.spec site-header.spec ``` - `mail.spec.js`: the mail is in the Email tray, named and headed for it, and with the tray put away no row is on screen; a row opens the mail that was clicked; mail that arrives joins the list for its own site only; "Clear emails" is disabled with nothing to clear, empties the list and what the app keeps, and is disabled again. - `tray.spec.js`: each of the three footer buttons puts its own in the tray in the others' place; with two sites the heading has one "Clear emails"; a build that fails with its output in the logs leaves an open Email tray where it is, and the page says where the lines are. - `site-header.spec.js`: two journeys used the Mail section as the end of the page and now give the page its height with the tray. I broke seven behaviours of the mail tray one at a time and each turned a journey red, and the details' two rules (always let go, never let go) at three window sizes. `tests/unit/site-mail.test.cjs` and `tests/unit/tray.test.cjs` hold the decisions. **Worth a look by hand** (any platform, the current head; I have not done this, the journeys stand in for the server): on a site that is set up, press **Start development server**, then press the envelope in the footer. The tray says where SMTP is listening. Make WordPress send a mail, for example **Lost your password?** on the site's login page: the mail is in the list. Press its row: the dialog opens. Close it and press **Clear emails**: the list is empty and the button cannot be pressed. **What must not have happened:** a site's mail in another site's tray; the list losing mail when the tray is closed, shows something else, or another site is opened; the logs coming up over an open mail list; anything on the page ending up under the tray. **What cannot be tested by hand here:** I had no real server to send a mail through. The journeys say what the main process says when one is caught, on the channels it says it on. ## Risks and limitations - Review: 2 passes; 10 findings fixed here, 2 settled by the maintainer's decisions (below), 4 left as follow-ups. Details below. - **Mail that arrives unseen shows nowhere** until the tray is opened. See "Not in this pull request". - **Mail kept from an earlier session cannot be cleared until the server has run once.** See "What changes". - Focus drops to the page when the app itself puts the terminal in place of the mail while the focus is in the mail's tray. The logs and the terminal already behave this way when one replaces the other. - A row's hover is faint on the list's ground, the same weak surface the logs are on. - A clear that fails says nothing, as before. - `toasts.spec.js:52` failed once in a full local run with "Cannot fast-forward to the past" and passes on its own; it is the third time I have seen it, it is not from this change, and it is flagged separately. ## Related Part of #558, and the last of its three. Follows #614. Mail HTML rendering is unchanged and tracked separately. --- <details> <summary>Design decisions and alternatives considered</summary> - **What the prototype lacks is kept and redrawn.** Its tray has a title, "Clear emails" and a placeholder text: no list, no line for the mail server, no dialog. The app has all three and keeps them, in the tray. - **A slot in the tray's heading**, filled by the open site while the mail is what the tray shows, for what is done to the whole of what the tray holds. The tray's heading is the window's, and the list is a site's. - **The dialog is left exactly as it is**, old components and all, so that this change and the one that redraws how a mail is shown do not meet in the same file. - **The logs give way to everything, not only to the terminal.** One rule for "the app asks for the logs" that does not have to know what each tray is for: they never take a tray from under someone. - **The count stays the design system's badge**, one line tall by losing its vertical padding, and not a smaller drawing of my own. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **Pass 1: 7 [fix here] · 5 [follow-up]. Pass 2 (on the fix-up): 3 [fix here] · 1 [follow-up]. 10 fixed here (7 and 3, one of pass 1's being a follow-up applied here), 2 settled by the maintainer's decisions, 4 left as follow-ups.** - **Review:** completed — a separate agent context, read-only; head `b9b7daa` / base `351d34b`; lint and the two unit files run, the journeys read and not run; 7 [fix here], 5 [follow-up]. - Fixed: the check that the mail is nowhere but the tray ran before the list had loaded and could not fail; the details journey guarded on a margin of my own and not on the app's boundary, and read it before the page was at its top; the count on the Logs button was drawn under the next button when that was pressed, and its comment was wrong; the step that leaves an open mail list where it is did not read that the page says where the lines are; one journey needed a page longer than the room above a tray at its first height; the check that the tray covers nothing read the last card and not what ends the page. - Applied here though marked follow-up: who a mail is from now gives way before its subject in a narrow window. - Decided by the maintainer: "Clear emails" stays disabled on an empty list, as the prototype has it (one [fix here], recorded above as what it takes away); no sign for mail that arrives unseen (one [follow-up], not wanted). - Follow-ups left: focus when the app swaps the tray from under it; the row's faint hover; a failed clear that says nothing. - **Review:** completed — a second separate agent context, read-only; `b9b7daa` plus the fix-up as staged, before it could be committed (it is `09ab94d`), base `351d34b`; lint and the two unit files run, each fix-up traced, every journey that depends on the page's length listed; 3 [fix here], 1 [follow-up]. - Fixed: a rule and its comment left from the heading the mail had on the page; a comment that said who a mail is from gives way first, which the rules did not do (they do now); a comment in the details journey that did not say what its loop does. - Follow-up left: a comment in the mail dialog's file still says the list is under "Mail". The dialog is not touched here. - Nothing found: the seven fix-ups, the journeys' absences, the other journeys against a shorter page, the screenshot script. - **Since review:** `b9b7daa → 09ab94d` is the two passes' fixes; the second pass's three are not reviewed again, and are a deleted rule, one flex value and comments. Lint, `npm test`, `npm run test:electron`, all 113 journeys, the affected journeys at three smaller window heights and with a slowed status read, and the packaged smoke were run on the tree that became `09ab94d`, the journey suite before the second pass's three fixes and the affected journeys after them. </details> <details> <summary>Implementation notes</summary> - New: `src/renderer/site-mail.cjs` (`smtpNote`, `noMailNote`, `mailRow`), `src/renderer/components/mail-panel.jsx` (`MailPanel`, `MailTrayActions`), `tests/unit/site-mail.test.cjs`. - `src/renderer/components/bottom-tray.jsx`: `SiteTrayActionsFill` and its slot in the heading. `src/renderer/tray.cjs`: the third tray and `trayAfterReveal`'s rule. - A mail's row is a native button in a list item, where it was a `div` with a role and key handlers. Who a mail is from now gives way before its subject does in a narrow window. - `scripts/screenshots/shots.cjs`: the mail's shot is taken of the tray. I ran that one shot to check the script and did not commit the image; #561 retakes them all. - The details journey reads both of its halves part of the way down the page, where a column that is held is not yet pushed by the end of the cards, and guards on the app's own boundary: the details fit until they are taller than the page, which is 24px less than nothing to spare under them. - Local checks on macOS: `npm run lint` clean, `npm test` and `npm run test:electron` pass (1948 tests), `npm run test:e2e` 113 pass, `npm run test:e2e:packaged` 15 pass. </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload images from the command line. The steps under "Worth a look by hand" show everything that changed. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
zaerl
marked this pull request as ready for review
October 5, 2026 10:27
This was referenced Oct 5, 2026
…616) ## Why `tests/e2e/journeys/toasts.spec.js`, "a confirmation is shown in the corner and said once, goes by itself when its time is up, and can be dismissed before", failed three times in the full local runs of the last few pull requests, with: ``` Error: clock.pauseAt: Error: Cannot fast-forward to the past ``` It never failed with its file run alone, and it passed on CI, so it read as a machine being busy. It is a race in the journey, and it would have come to trunk with the redesign (#591). ## What changes **The root cause.** The journey read the page's time in one round trip and stopped the clock at that time in another: `page.clock.pauseAt( await page.evaluate( () => Date.now() ) )`. Playwright's page clock catches up with real time whenever the page reads the time, and at least every tenth of a second by its own timer, but not as it is stopped. So if anything made it catch up between the two calls, the time asked for was behind the clock, and a stop behind the clock is refused. **The fix.** The clock is stopped one second ahead of the time read, and what is then fast-forwarded is one second less. The confirmation is looked at at the same two ages as before, short of its lifetime by the slack and at its lifetime, so the journey proves what it proved. No app code changes. ## How to test this Platforms: any. There is no surface to drive by hand; the change is to one journey. From the repository root, after `npm ci` and `npm run build:once`: ``` npx playwright test --project=journeys toasts.spec ``` - **The cause, shown:** with a 300ms real wait put between the read and the stop, the old line failed three times out of three with the error above, and the new one passes. - **The flake, gone:** the spec passed 20 times in a row, and the whole journey suite twice (113 each). - **What the journey still proves:** with a confirmation's lifetime made 2.5 seconds longer in the app the journey is red at "gone at its time", and with it made 2.5 seconds shorter it is red at "still there". **What must not have happened:** the journey passing because it no longer looks at the right moments. The two mutations above are that check. ## Risks and limitations - Review: 1 pass, 0 to fix, 1 follow-up taken here (a comment reworded). Details below. - The stop can still be refused if more than a second passes between the two calls. That is two round trips to the page; the journey already allows a slow machine two seconds elsewhere. - There is no test for the race itself: it cannot be made to happen from a journey without the wait I put in by hand. ## Related Found while working on #558; fixed before #591 lands. The journey is on `redesign/wpds` only. --- <details> <summary>Design decisions and alternatives considered</summary> - **Ask again on a refusal.** It works, but only because the refused stop has already stopped the clock, which is how the clock happens to be written and not something it promises. - **Stop the clock from inside the page, in the same turn as the read**, through the object Playwright puts there. Exact, and it leans on a name that is not part of Playwright's API. - **A margin taken off the slack.** Simpler still, and it makes the journey less tolerant of a slow machine. Taking the margin off what is fast-forwarded costs nothing. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **0 [fix here] · 1 [follow-up], taken here.** - **Review:** completed — a separate agent context, read-only; the change as staged on base `548c020`, before it could be committed (it is `f762591`); ESLint on the file run, the journeys not run; the diagnosis checked against the clock's code in `playwright-core` 1.63.0, the confirmation's timer traced to `toast-stack.jsx`, every other use of the page clock in `tests/` checked for the same race. Nothing found in the diagnosis, the arithmetic or elsewhere; the comment above the new constant said less than the cause and claimed a guarantee, and is reworded. - **Since review:** the comment, and nothing else. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
…eld nothing reads (#617) ## Why The redesign was built as about twenty slices on `redesign/wpds`, each reviewed on its own. Before the branch is merged to trunk (#591) I had a separate reviewer sweep it for what falls between slices: what one left "until a later slice" and the later slice did not clean up. It found no dead CSS, no unused dependency and no repository doc that names the branch. It found thirteen comments the finished redesign made untrue, and one field nothing reads. ## What changes No behaviour changes. **Comments corrected**, in ten files: - `src/renderer/index.jsx`: the note that the system's alert and confirm are "replaced … as its part of the window is redrawn (#557)" now says what is left of them (three discards that ask through `window.confirm`, two failures said in an alert) and that moving them is a change of its own; two notes that said "yet" of things that are settled. - `src/renderer/hooks/use-dev-server.jsx`: "the one button" that starts the server is the header's menu, the details' button and the setup checklist's; a failed server is said on the page as well as in the logs; the button's old names. - `src/project-type.cjs`: where a project's tag is drawn, and that nothing upper-cases it any more. - `tests/e2e/journeys/build-watch.spec.js`: its header called it a bug that the watch's button cannot select the watch's tab. It can, since the logs' tabs follow the app's choice (#613), and the same file asserts it. - `tests/e2e/journeys/tray.spec.js`, `logs.spec.js`: the tray's two buttons are three; the logs are in the tray. - `TESTING.md` and `scripts/screenshots/shots.cjs`: the server's button by its present name; the instructions for one screenshot no longer describe the old three buttons. **One field removed.** `trunkAgeInfo` in `src/renderer/update-plan.cjs` returned `label`, the sentence "trunk as of …" that the old header drew. Nothing has read it since the header was replaced (#594). Its unit tests now pin the date it is built from. **Deliberately not here:** - The three questions and two alerts that still use the system's dialogs. Replacing them changes what a contributor is shown. - The roughly 350 strings that are not yet translatable, and the dialogs still drawn with the old component library. Both are recorded in #591. - One stale comment in `src/renderer/components/email-modal.jsx`, a file another open pull request is changing. - `AGENTS.md`, which names the server's button by its old words twice, in the note about the Electron pin. I left the project's instructions alone. ## How to test this Platforms: any. There is nothing on screen to drive: the change is comments, one unread field and its tests. From the repository root, after `npm ci`: ``` npm run lint npm test ``` That the field is unread: `git grep -n "trunkAgeInfo" src` lists its two callers, `src/renderer/index.jsx` and `src/renderer/sites-list.cjs`, and neither they nor what they hand the result to (the details, the review dialog) read `label`. **What must not have happened:** a code line changed along with a comment. The diff outside `update-plan.cjs` and its test is comments, one string shown to a person taking a screenshot, and prose. ## Risks and limitations - Review: 1 pass, 5 findings fixed here. Details below. - A comment is only as true as the day it was checked. Each reworded one was checked against the code on this branch by the reviewer, by name, and the list is below. ## Related Part of #542. Before #591. --- <details> <summary>Review outcome (required — see AGENTS.md)</summary> **5 [fix here] · 0 [follow-up]. All 5 fixed.** - **Review:** completed — a separate agent context, read-only; the change as it stood uncommitted on base `548c020` (it is `197d21a` with the fixes); the diff read line by line for anything that is not a comment, every consumer of `trunkAgeInfo` followed through the props it is handed on in, each reworded comment checked against the code it describes. - Fixed: a new assertion on the date's day of the month failed in any time zone behind UTC, where midnight UTC is the day before (removed; the suite pins no time zone); the comment about the server's controls left out the setup checklist's; three more comments in the same file still described one button, by its old names; a comment in the same block said a failed server is said "nowhere on the page", which stopped being true in #613; "the Logs panel" once more in a file already touched. - Nothing found: no code line changed outside the field; the field has no reader; `TESTING.md`'s paragraph is still one line. - **Since review:** the five fixes. `npm run lint` and `npm test` re-run after them, the unit file for the field in three time zones (`America/Los_Angeles`, `Pacific/Kiritimati`, `UTC`). Before them, on the same code but for comments and that one assertion: `npm run test:electron`, all 113 journeys and the packaged smoke. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
## Why `AGENTS.md`, in its note about the Electron pin, says the Playground server crashes "about a second after "Start dev server"" and that the packaged smoke "never clicks "Start dev server"". Since the redesign (#595) the control in the page's header and in the details reads "Start development server". Someone following the note to check an Electron bump would look for a button that is not there. #617 corrected the same words in `TESTING.md` and left this file for a decision, since it is the project's instructions. ## What changes Two words, twice, in one line of `AGENTS.md`: "Start dev server" becomes "Start development server". Nothing else. The setup checklist's own button still reads "Start dev server and finish the wizard"; that is a different control, and the note is about starting a server on a site that is set up. ## How to test this Platforms: any. Nothing runs differently: it is prose. To see that the name is the control's: ``` git grep -n "Start development server" src/renderer/site-processes.cjs tests/e2e/helpers/ui.cjs ``` ## Risks and limitations - Review: not run by a separate reviewer. See below. - #571 changes one line of `AGENTS.md` too; if it is the same line, one of the two will need the other's words when trunk is merged across. ## Related Part of #542. Follows #617, before #591. --- <details> <summary>Review outcome (required — see AGENTS.md)</summary> - **Review:** not run — the change is two words repeated in one line of prose, at the maintainer's request. I checked that the line is still one line, that the name is the one `src/renderer/site-processes.cjs` gives the control, and that no other tracked doc still uses the old name (`git grep "Start dev server" AGENTS.md TESTING.md CONTRIBUTING.md .github`). - **Since review:** none. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This was referenced Oct 5, 2026
zaerl
added a commit
that referenced
this pull request
Oct 5, 2026
#620) ## Why First part of #561. The redesign is on trunk (#591) and the guide's screenshots still show the old window. Twelve of the guide's images are taken by the harness with nobody at the keyboard; this retakes those, and fixes what made a retake depend on the day and on the network. ## What changes **Twelve screenshots retaken** (`npm run shots`, the fixture tier): the empty window, the create-site dialog, a Core and a Gutenberg site, the site's menu, the notice for old code, the apply card, the question a link raises, the terminal, the logs and the mail in the tray, and an incomplete update. **The harness, so that a retake is the same picture whenever it is taken:** - **The fixture's dates are counted back from the day of the shots.** They were written down as 16 September, which was fresh for a fortnight; after that every site in every picture wore "This site's WordPress code is 19 days old". The code's date is at midnight UTC, since the app counts its age in whole days. The mail and the lines in debug.log are dated the day before. - **No automatic shot reaches the network or starts a process.** A ticket's pull requests are answered by the fixture: left to GitHub, the site's menu was photographed over a card still "Checking GitHub…". The dev server and the script it runs first are answered for every shot; the mail's shot used to start a real server in a folder that holds no WordPress, and showed whatever port the mail server had picked. - **Motion is reduced**, so a tab's underline is not caught half way to its tab. **Beside the images:** the alt text of three of them said what the old pictures showed, and the few sentences next to the create-site dialog's named the old button and labels. `docs/public/screenshots/pr-applied.png`, which no page uses, is removed. **Not in this pull request:** - The other sixteen images: nine that need a person at a real site, and six nobody's script takes. They are being moved to the automatic tier, in the pull requests that follow. - The guide's prose, apart from the lines above. It follows the screenshots, in four pull requests by topic. ## How to test this Platforms: macOS to reproduce the committed images; the harness runs on any. Nothing here ships in the app: the change is a documentation tool and its output. From the repository root, after `npm ci`: ``` npm run shots git status --short docs/public/screenshots ``` 1. The run prints twelve lines, each `✓ <name>.png`, and exits 0. It opens the app's window four times and closes it; do not type while it runs. 2. On the machine and the UTC day the committed images were taken, `git status` shows nothing changed. On a later day it shows the images that carry a date (below); on another machine it may show them all, since fonts are drawn differently. 3. Open the twelve PNGs. Each shows a finished state: no spinner, no menu half open, nothing cut by the edge of a crop. Every path is under `/tmp/wpct-docs-fixture/`. **What must not have happened:** a real server started, or anything asked of GitHub or Trac, by a shot; a picture with a path under a home folder, or with a notice that the code is old on a site that is meant to be fresh. **What cannot be promised:** the same bytes on another machine or another day. The images are identical between two runs on one machine within one UTC day, which I checked by taking them twice. A date is drawn in the whole-window shots (when a site was made and its code's date), in the site's menu shot (the pull requests' dates), in the mail and in the logs; system fonts and the window's frame differ between systems; and on Windows the fixture's folder is under the system's temporary folder, so the path in the logs' shot differs. ## Risks and limitations - Review: 2 passes, 7 findings fixed here, and 5 follow-ups all taken here. Details below. - **A run that crosses midnight UTC** can draw dates that do not agree with each other: the fixture counts its days once, as the run starts. - **The dev server never comes up in a shot.** Its start is accepted and it gives no address, so a whole-window shot taken after pressing Start would show it starting for ever. None of the twelve does that; the two that press Start are crops of the tray. A shot of a running server needs the address said to the window as well, which belongs with the shots that follow. - The guide's text now describes the old window beside new pictures, until the text follows. ## Related Part of #561, which is part of #542. --- <details> <summary>Design decisions and alternatives considered</summary> - **Days counted back, not dates bumped.** The file's own comment said to bump the dates "when that happens", expecting years; the notice appears after fourteen days. Counted back, "126 days old" is the same on every retake, where written down it changed every day. - **One set of stand-ins for the whole tier**, installed once per launch, and not a few lines in each shot that needs them. A shot that forgets is then one that cannot start a server, where before it was one that did. - **The mail server is said to have started, on a port of the fixture's.** Cropping its line out of the picture was the other way; the line is part of what the tray shows. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **Pass 1: 4 [fix here] · 3 [follow-up]. Pass 2 (on the fix-up): 3 [fix here] · 2 [follow-up]. All 7 fixed, and all 5 follow-ups taken here.** - **Review:** completed — a separate agent context, read-only; head `44cc5ff` / base `887afe1`; ESLint on the scripts run, the twelve images opened and looked at one by one, the shots not run; 4 [fix here], 3 [follow-up]. - Fixed: the old site read "125 days old" before nine in the morning UTC and "126" after, from the hour I had put on its date; the stand-in for a ticket's pull requests left out two fields of the real answer; a comment said when the card asks and was wrong; the alt text of three images. - Follow-ups, taken here: the mail's shot started a real server; it showed a port that is different every run; the mail was dated before its site was made. - **Review:** completed — a second separate agent context, read-only; head `a9c285b` / base `887afe1`; every path through the two replaced handlers traced for each shot, what loads the mail list and what can write the port, the date arithmetic at every hour, the three alt texts against their pictures; 3 [fix here], 2 [follow-up]. - Fixed: the stand-in's comment said the server is "up"; a comment in the logs' shot contradicted itself; one alt text counted one line where the picture has two. - Follow-ups, taken here: the sentences beside that image still named the old button and labels; the fixture's debug.log lines were dated weeks before their site. - Nothing found: no shot needs a real server or script; the mail's port can only be the fixture's; nothing references the removed image. - **Since review:** `a9c285b → 2ce5ad9` is the second pass's fixes, not reviewed again: two comments, one alt text, five lines of prose, and the dates in the fixture's log, with that one image retaken. `npm run lint` and `npm test` pass; the twelve shots were taken again. </details> <details> <summary>Implementation notes</summary> - `scripts/screenshots/fixtures.cjs`: `daysAgo`, `FRESH`, `STALE`; `standInForTheOutside` (replaces `git:list-ticket-patches`, `npm:run-script` and `playground:start` in the main process); `sayMailServerStarted`. - `scripts/screenshots/capture.cjs`: reduced motion and the stand-ins, once per launch of the fixture tier. The live tier gets neither. - `scripts/screenshots/shots.cjs`: the site's menu waits for the pull requests' answer; the logs' tray is made taller so the file's first line is not cut; the mail waits for where the server is said to listen. </details> <details> <summary>Screenshots or recording</summary> The twelve images are the change: `docs/public/screenshots/` in the diff, which GitHub shows 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
…d over itself (#637) ## Why Two findings from a code review of the debug.log tail, both read from the code; both are confirmed here, by a script against Node's `fs` and by tests that are red on `trunk`. A line WordPress writes just as the tail is reading shows up in the Debug.log pane twice. The tail stats the file, opens a stream from an offset with no end, and attaches the watcher; the stream reads to wherever the file ends by the time it gets round to it, and the `change` event for the same write reads from the stat's size again. The footer's badge counts the line once, so the pane and the badge disagree. Saving `debug.log` from an editor refills the pane with every line it already showed, and the footer counts all of them as unseen. An editor's save writes a new file and renames it over the path, the watcher on the old file reports `rename`, and the tail forgot its offset and replayed the file as if it had just appeared. ## What changes **Every range the tail plans ends at the size it was planned from.** `planInitialRead` and `planTailRead` in `src/log-tail.js` return `{ start, end }` with `end` inclusive, as `fs.createReadStream` takes it. Each read ends where the next one begins, so a byte is sent exactly once, by the read whose size saw it. `startWpDebugTail` already passes the range object straight to the stream; it needed no change for this half. **A file still at the path after a `rename` is the file the tail was reading, until its size says otherwise.** `attachFileWatcher` now plans with `planTailRead` when it has an offset, by the same rule a `change` uses: a larger file is read from the old offset, an unchanged one not at all, a smaller one from its start, as after a Clear. The offset is forgotten only in `watchForFile`, when the file is actually gone, so what appears at the path later is still read whole, as a `grunt clean` and rebuild expects. This is the decision the second finding asked for, and it is the one the module's own comment already made for a same-length rewrite: replaying the file on every such event duplicates it into the pane, which is worse than missing a replacement that happens to be no smaller. Nothing on screen changes shape. The renderer's `backlog` flag and its counting are untouched. ## How to test this Platforms: any for the pane; the `rename` path is where the platforms differ in which events `fs.watch` sends, so a pass on Windows is worth more than a second one on macOS. Build: the current head, or the Buildkite artifact for it once it exists. **The journey carries the editor's save.** From the repository root: ```bash npx playwright test --project=journeys -g "first appears while the server runs" ``` It is red on `trunk` at the new step: the footer says 3 unseen lines where 1 is right, because the whole file was replayed and counted. I checked by putting `trunk`'s two source files back under the new tests. The duplicate from the first finding is a race between a stat and a stream's first read, which a person cannot stage by hand; `node --test tests/unit/log-tail.test.cjs` reproduces it on a real file (also red on `trunk`), and the two new tests in `tests/unit/ipc-wiring.test.cjs` reach the `rename` decision directly, with the file present and with it gone. **What the journey cannot reach**, for a pass by hand against a real site with a built `wordpress-develop` and the dev server started: **Starting state:** a site whose dev server is running, with the Logs tray open on the **Debug.log** tab and at least one line in the pane. `build/wp-content/debug.log` under the site is the file; the pane names its path. 1. Open that file in an editor that saves atomically (VS Code, TextEdit), add a line at the end, save, and close the tray with the footer's **Logs** button first so the count is visible. Expected: the footer's badge says 1, and opening the tray shows the added line once under the lines that were already there. 2. From a terminal in the site's directory (either shell), append another line: `echo "PHP Notice: by hand" >> build/wp-content/debug.log`. Expected: it arrives in the pane without anything being clicked. This is the tail on the new file. 3. Click **Clear**, then append again. Expected: the pane empties and the appended line arrives, as before this change. **What must not have happened:** no line in the pane appears twice; the badge after the save counts the added line, not the file's line count; the marker line about an earlier run appears only once, under the backlog the start replayed, and not after the save. ## Risks and limitations - A byte WordPress writes between the tail's `statSync` and its `fs.watch` attaching, two synchronous calls apart, now waits for the next `change` event instead of arriving at once and then again. WordPress logs in bursts, so the next line brings it. - A different and larger file put at the path while the tail is attached, such as an old `debug.log` copied over it, is read from the old offset and its head is not shown. The same trade-off `planTailRead` already makes for a same-length rewrite on a `change`; a smaller one is replayed from its start, as a Clear is. - The `rename` path's events were checked by script on macOS (one `rename`, file present, old watcher dead) and not by hand on Windows, where `fs.watch` watches the directory and may send a `change` as well; every ordering I could construct reads each byte once, the wiring tests pin the decision without a real watcher, and the journey runs on Windows in CI. - A file removed and recreated so fast that the `rename` arrives after the new file exists is taken for the old one: with the first write longer than the old file, its first bytes up to the old size are skipped. The events are asynchronous and a few milliseconds behind, so the window exists; nothing reproduces it in practice, and the inode would not tell the cases apart either, since an editor's save is a new inode too. - The task described this function as also changed by the badge pull request on `redesign/wpds`; that branch merged into `trunk` as #591 on 2026-10-05 with #614 in it, so this change edits the badge version directly and there is nothing left to merge it into. ## Related Follow-up to #217 (the debug.log tail and its read planner), #558 (the logs tray) and #614 (the unseen count, which is what the second finding miscounts). --- <details> <summary>Design decisions and alternatives considered</summary> **Why `end` rather than reordering the stat and the watcher.** Attaching `fs.watch` before the `statSync` would also close the gap in the first risk above, but a stat that fails after the watcher is attached leaves a watcher to close on a failure path, and the duplicate was the user-facing bug. The bound is one field, and the plan is what the tests read. **Why the decision lives in `attachFileWatcher` and not in a third planner.** The two planners already cover both answers; the only new fact is whether the tail had a file before, which is `lastSize > 0`, and the place that knows the file is gone is `watchForFile`. A `planReattachRead(lastSize, size)` would be a one-line ternary with its own tests and nothing the wiring tests do not already pin. **Why the file found after a `rename` is not always treated as new.** It is new in the sense of being another inode, but the app cannot tell an editor's save from a `grunt clean` that way; the size can, as far as a size can, and the pane's own comment already chose missing a same-length rewrite over duplicating the file. The case this gets wrong, a larger unrelated file placed at the path, is in Risks. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **1 [fix here] · 0 [follow-up] — fixed.** Pre-PR review per `.github/instructions/code-review.instructions.md`; CodeRabbit not yet requested. - **Review:** completed — fresh agent context (Claude Fable 5.1, a subagent given the diff and the standard, separate from the session that wrote the code); reviewed head `d18513a673e034beecc8e769d10ea4f12723dbf8` / base `0cf005f86417f786397a792b290f6748b21adbcb` (`origin/trunk`); `npm run lint` and `npm test` green on that head (1961 tests, 0 failures), the two touched unit files re-run by the reviewer, the journey read and not run. Finding, tests 🟡 [fix here]: the wiring test for a file removed and brought back could not go red, because a 5-byte file after a 20-byte one is read from its start by either rule. Fixed in `ce9fb0f` by bringing the file back at 25 bytes; confirmed red with the reset removed and green with it. Two style notes taken: the comment in `attachFileWatcher` no longer says the offset is 0 *only* when there was no file (a Clear zeroes it too), and the remove-and-recreate window is now in Risks. Verified and not findings: no range the planners produce can make `fs.createReadStream` throw; two in-flight streams read disjoint ranges; every path through `watchForFile` is right; Windows may deliver two `rename` events for a replace and the second reads nothing; the journey's new matches are scoped to the tray and run after the exact byte stream was asserted. - **Since review:** `d18513a` → `ce9fb0f3d1da8ad4623777209c21394f725c9d2d` checked: one commit, the test fix above plus the comment, lint and the three `wp-debug:start` wiring tests green; no other file touched. </details> <details> <summary>Implementation notes</summary> - `src/log-tail.js`: `end: size - 1` on every range; header comment says why. A one-byte file is the edge the new unit test pins (`{ start: 0, end: 0 }`). - `src/main.js`, `startWpDebugTail`: `attachFileWatcher` chooses `planTailRead` when `state.lastSize > 0`, `planInitialRead` otherwise; `watchForFile` zeroes `lastSize` when the file is absent; `reattachAfterLoss` no longer zeroes it. The `fromBefore` flag and the marker are as they were, so a file found after a `rename` is told as news without the marker. - `tests/unit/log-tail.test.cjs`: ranges updated, a range-end test, and a real-file test that opens a stream from a plan, appends before it reads, and asserts the stream carried only what the plan saw. - `tests/unit/ipc-wiring.test.cjs`: `fakeTailedFile` stands in for `existsSync`, `statSync`, `createReadStream` and `watch` on the log path only, falling through to the real `fs` elsewhere, so a watcher's event is delivered with the file in the state the test says. One test for a `rename` with the file present (continued from the offset, old watcher closed, one new watcher, no marker, nothing read for an unchanged size), one for a `rename` with the file gone (folder watched, file read whole when it comes back). - `tests/e2e/journeys/logs.spec.js`: the "first appears" test gains an editor's save over the file and an append after it, asserting the exact text the window was told and the count on the footer. - Confirmation scripts, not committed: a stream opened from `{ start: 0 }` on a 14-byte file, appended to before it read, delivered 21 bytes, and a re-read from 14 delivered the same 7 again; `fs.watch` on a file renamed over on macOS delivered one `rename` with the path present at its new size and nothing for a later append. </details> <details> <summary>Screenshots or recording</summary> Nothing on screen changed shape: the same pane shows fewer lines in two cases where it showed too many. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
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)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
#542 replaces the app's interface with the one in the prototype, drawn with the WordPress Design System. It was built in slices on
redesign/wpds, so that trunk, which releases are cut from, never held a half-redesigned window. The window is whole now: the shell, the page header and details, every card and dialog, and the tray. This pull request is how the branch reaches trunk.It lands before the last three sub-issues, on purpose. The settings dialog (#559) and dark mode (#560) add to a finished window and do not need a branch to hide in, and the guide (#561) is easier to rewrite against what is on trunk. They follow as pull requests against trunk.
What changes
Everything
redesign/wpdsholds; this pull request adds nothing of its own. Each slice was reviewed in its own pull request against that branch, and says there how to test it and what it leaves out.AGENTS.md).What a contributor will notice first: the terminal, the logs and the mail are no longer on the page but behind three buttons at the foot of the window; the terminal is dark text on light; the server and the build watch are started from the header; a patch is previewed in a dialog before it is applied; and what worked is said in the window's corner.
Not in this pull request:
How to test this
Platforms: macOS and Windows, both. Buildkite builds signed artifacts for this pull request; use the build for its current head, and check that it is: a push to the branch makes earlier builds stale.
The suite, from the repository root, after
npm ci:113 journeys walk every slice on macOS and Windows on each push. What they do not do is run a real site: a clone of
wordpress-develop, an install, a build and a server are stood in for. The one test that does,real-setup.yml, is run by hand and was run on this branch at548c020(https://github.com/WordPress/contributor-toolkit/actions/runs/37298667056): a Core site on macOS and on Windows and a Gutenberg site on macOS were created, installed, built and served, and passed. The Gutenberg site on Windows did not finish. Its first attempt was cancelled after 74 minutes inside the journey, a minute short of the limit that would have failed it with evidence, so it says nothing about where it was; a second attempt was still running when this was written and its result is not in this description. The commits since548c020change a journey, comments and prose.A pass by hand, on a real machine (any platform; Apple Silicon for the server, which no test on a pull request starts for real). It follows the order a contributor meets things in:
help. Logs: the server's lines, the build watch's, and Debug.log. Email: where SMTP is listening. Drag the tray's top edge.src/, then Review & submit changes. The diff is the edit and nothing else.What must not have happened: work lost in a link, an apply, a switch or an update that did not say so;
node_modulesrebuilt where nothing changed; a failure said only in a tray that stayed closed; one site's output in another's tray; anything on the page under the tray.What cannot be tested by hand, and what was not: none of the slices' manual steps were run by a person. I looked at screenshots and drove the app through the journeys; everything local was on macOS, with Windows left to CI. This pass is the first one by hand.
Risks and limitations
redesign/wpdsshould not be branched from after this: it will have diverged from trunk.AGENTS.mdreviewed by a separate reviewer context before they were opened, most in two passes; see below. The branch was then swept once for what falls between slices, which found comments and one unread field (Correct the comments the finished redesign made untrue, and drop a field nothing reads #617) and the three points that follow.DataViewsand what it needs. Launch time was not measured.Related
Closes #557. Closes #558. Part of #542, which stays open for #559, #560 and #561. #555 and #556 are closed already.
Design decisions and alternatives considered
Review outcome (required — see AGENTS.md)
No review of the whole diff was run. The record is the slices': twenty-two pull requests into
redesign/wpds, each but one reviewed before it was opened by a separate, read-only reviewer context following.github/instructions/code-review.instructions.md, most in two passes (the change, then its fix-up), with the counts and the findings in each pull request's own review outcome. The one is #618, two words in a line of prose.redesign/wpdsat548c020against trunk; not a review of correctness, which the slices had: a search for dead CSS and code, comments that waited for a slice that has landed, what is left of the old component library and of the system's dialogs, strings not yet translatable, and repository docs that name the branch. Nothing found in the stylesheet, the dependencies or the docs; thirteen comments and one unread field, fixed in Correct the comments the finished redesign made untrue, and drop a field nothing reads #617; the rest is in Risks above.da19ac6,3344172,23d46e7).Implementation notes
git diff origin/trunk...origin/redesign/wpds: 148 files, about 14,800 lines added and 3,000 removed; about 6,300 of the added lines are tests.src/renderer/*.cjswith unit tests; components draw them.tests/e2e/journeys/, lint rules of the repository's own (eslint-rules/), and Stylelint over the window's stylesheet.Screenshots or recording
Not attached: I have no way to upload images from the command line. The prototype shows the design; the signed build shows the app.
🤖 Generated with Claude Code