Repository navigation
Draw the apply card with the design system, and preview a patch in a dialog - #597
Conversation
…dialog Item 4 of #557: the card a pull request or a patch file is brought into the checkout from, as the prototype draws it, and the banner of a checked-out pull request. The card is a component of its own, apply-card.jsx, with what it decides in apply-card.cjs. On a project that takes patch files it has a tab for each way in. It can be folded, and opens while it has something to say: the steps of an apply, a patch that is applied, a failure. What a patch would change is shown in a dialog before it is applied, where it was a block in the card. A dialog is in front of the page, so a failed apply now drops its preview, and what went wrong is read on the card; the preview is set aside, and kept, while the app asks what should become of loose edits on trunk. The applied patch, the failure and its breakdown, and the checked-out pull request are the design system's notices. The ones that were alerts or status regions still are. The registry loses the card's heading and description, which the card now has translated, and pr-state.cjs goes with the last pill drawn from it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The first review of the apply card, applied. A dialog is in front of the whole window, and every site's view is in the window at once: a preview that arrived after the contributor had gone to another site opened over that one, with a button that applied to the site out of view. A preview is shown by its own site, and waits for it. An apply is refused while a command runs in the terminal, with a line in the terminal, which the dialog is in front of. Its button is held and says so. Saying no to the question about loose edits takes the preview with it, where it used to bring the dialog back. After an apply, focus goes to the card's header, since what opened the dialog has gone with the fields. Which preview shows, why its button is held and whether the card is open are decided in apply-card.cjs, with unit tests. "Near" and the line it introduces are one sentence a translation can reorder, a place in a breakdown is keyed by where it comes, and a word that was translated and never shown is gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The second review of the apply card, applied. The reason on the preview's held button named the terminal as where a command was running. Not everything that holds the terminal prints there: the build the watch runs before it starts prints in the watch's tab, and a ticket switch says its progress in the work-item card. The sentence says something else is running on the site, and that Ctrl+C in the terminal stops it, which is true of all of them. A dialog left without applying gives focus back to what opened it, whatever was pressed in it before. In the journeys: the two-site journey waits until the first site's view has taken the preview in before it reads that no dialog is shown; the helper that finds any way to a patch file no longer matches two buttons that are not one; an assertion that could not fail is gone. 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 configurationConfiguration used: Repository: WordPress/contributor-toolkit/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 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 |
## 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>
## Why #542 replaces the app's interface with the one in the [prototype](https://github.com/jameskoster/wordpress-contributor-toolkit-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/wpds` holds; 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. - **The shell** (#555): a sites list, the page area and a footer, in a larger window. #590 - **The page header and the details** (#556): the site's menu in the header, and what the checkout is in a sidebar that can be put away. #594 - **The site's page** (#557): the server and the build watch in the header and the details (#595); the ticket card, the apply card with a preview dialog, and the list of a site's tickets (#596, #597, #599); the create-site, rename and delete dialogs (#600, #601); confirmations as notices in the window's corner (#602); the page's notices, update card and setup checklist (#605); the terminal, logs, mail and the review dialogs painted with the design system's tokens (#606, #608); and lint rules that keep it so (#609, #610). - **The tray** (#558): along the bottom of the window, holding the terminal, the logs or the mail, with a count of unseen debug.log lines on the footer (#612, #613, #614, #615). - Along the way: #611 (the site menu's update item says why it is held), and three that tidy up before landing: #616 (a journey's race with the page's clock), #617 (comments the finished redesign made untrue) and #618 (the server's button by its present name in `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:** - The settings dialog (#559). Until it, nothing about how the window is arranged is remembered across restarts: the details, the tray's height, the apply card's fold. - Dark mode (#560). The terminal reads its colours once, when it is made, and will need to follow the theme. - The user guide and its screenshots (#561), which describe the old interface. They are published from release tags, so they are not wrong until a release ships this; #561 is next, ahead of #559 and #560, for that reason. - How a mail is drawn, and how the window handles a link. Both are unchanged and tracked separately. ## 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`: ``` npm run lint npm test npm run test:e2e ``` 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 at `548c020` (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 since `548c020` change 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: 1. Open the app with no sites. **Create new site**, a Core site, in a folder with a space in its path. The setup checklist runs its steps; the terminal's button at the foot of the window shows what each is doing. 2. Let the checklist's last step start the server, and finish WordPress's own setup in the browser. Back in the app the header says "Server running" and "Build watching". Stop the server and start it again: from **Server running** in the header, or with **Stop** and **Start** beside "Server" in the details. 3. Press the three buttons at the left of the footer in turn. Terminal: type `help`. Logs: the server's lines, the build watch's, and Debug.log. Email: where SMTP is listening. Drag the tray's top edge. 4. Link a ticket by its number. The card shows the ticket and its pull requests and patches. 5. Apply one of them. The preview dialog names the files; **Apply and rebuild** runs the steps on the card. 6. Edit a file under `src/`, then **Review & submit changes**. The diff is the edit and nothing else. 7. Create a second site, or open one from before, and move between the two from the sites list. Each has its own terminal, logs and mail. 8. **Update to latest trunk** from the site's menu. **What must not have happened:** work lost in a link, an apply, a switch or an update that did not say so; `node_modules` rebuilt 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 - **It is one commit on trunk**, since the repository allows only squash merges. The slices' pull requests are its record. `redesign/wpds` should not be branched from after this: it will have diverged from trunk. - **Reviewed in slices, not as a whole.** Twenty-two pull requests, all but a two-word change to `AGENTS.md` reviewed 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 (#617) and the three points that follow. - **Three questions and two failures still use the system's own dialogs**: the discards ask through its confirm box, and a path that could not be copied and a server started before there is a build are said in its alert. #557 meant to replace them and did so for deletions only. - **Some dialogs are repainted, not rebuilt.** The review dialog and its destinations, the mail dialog and the question an update asks about loose edits have the design system's colours and type on the old component library's parts. - **A real Gutenberg setup on Windows is not shown to pass on this branch.** See "How to test this". It passed on trunk before the redesign, in 35 minutes; here it was past 46 on its second attempt. Whether that is the runner or the branch is not known. - **The guide describes the old interface** until #561. Do not cut a release before it. - **The bundle roughly doubled** (#590): the sites list brings `DataViews` and what it needs. Launch time was not measured. - **Less is always in view.** A build's output, a new line in debug.log and a caught mail are in a tray that may be closed. A failure opens the terminal, or the logs if nothing else is open; unseen debug.log lines are counted on the footer; a mail that arrives shows nowhere until its tray is opened, which was decided and is not an oversight. - **Translation is about half done.** The shell, the cards, the tray and most dialogs go through the translator. Roughly 200 strings in components and hooks do not yet (the review dialog and its destinations, the setup checklist, the logs, most of what the hooks say), nor about 150 in the modules they take sentences from. Strings that are new are in English in the shipped translations until they are translated. - **Focus is lost in a few places**: when the app swaps what the tray holds from under it, when a notice with a focused button goes, when a row's own action removes the row. - **Some failures are still worded by the main process** ("Error invoking remote method…"): a failed rename, a failed setup. - **Things carried over, not made by this:** a start that throws leaves the server's button dead until a restart; erasing typed input in the terminal breaks once the line wraps; a line written to debug.log while it is being attached to can be shown twice. - **#571 and #603 will conflict with trunk after this**, in the mail's journey and helpers and in the dev server's hook. - Each slice's own list has more, and says which of its limits were chosen. ## Related Closes #557. Closes #558. Part of #542, which stays open for #559, #560 and #561. #555 and #556 are closed already. --- <details> <summary>Design decisions and alternatives considered</summary> - **An integration branch, and one landing.** A window drawn half by the old view and half by the new one could not be released, and the slices were too many to hold back one by one behind a flag. Fixes and changes with no visible effect went to trunk first and were merged across, so this diff is the redesign and little else. - **Landing before #559 to #561.** The plan was to land when all seven sub-issues were in the branch. The remaining three do not change what is there: two add to it and one documents it. Holding a finished window on a branch for them only lengthens the time trunk and the branch can drift apart. - **What the prototype lacks is kept and redrawn**, and what it has somewhere else moves to where it puts it. Its labels are adopted. Where a slice departs from it, the slice says so. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **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.** - **Review:** completed, per slice — #590, #594 to #602, #605, #606, #608 to #617 (no review was run for #618); everything marked to fix in a slice was fixed in that slice, and what was left is in its Risks. - **Review:** completed, for leftovers only — a separate agent context, read-only; `redesign/wpds` at `548c020` against 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 #617; the rest is in Risks above. - **CodeRabbit:** not run. Its check reads "Review skipped: automatic reviews are disabled" on this pull request and on the slices, so the slices' pre-PR reviews are the reviews of record, each for the head it names. - **Since review:** nothing was added to the branch outside a reviewed slice, apart from three merges of trunk (`da19ac6`, `3344172`, `23d46e7`). </details> <details> <summary>Implementation notes</summary> - `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. - The site's view, which was one component of five thousand lines, is spread over the four regions the layout has: the header, the cards, the details and the tray. Decisions are in `src/renderer/*.cjs` with unit tests; components draw them. - New for the suite: journeys per region under `tests/e2e/journeys/`, lint rules of the repository's own (`eslint-rules/`), and Stylelint over the window's stylesheet. - The main process changed in one place for the redesign's sake: the debug.log tail says which lines the file already held (#614). </details> <details> <summary>Screenshots or recording</summary> Not attached: I have no way to upload images from the command line. The prototype shows the design; the signed build shows the app. </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 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, nottrunk.What changes
apply-card.jsx, with what it decides inapply-card.cjsand 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:
Also:
Deliberately not in this 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:e2eruns them all;tests/e2e/journeys/apply-card.spec.jsis new:i18n.spec.jsgains the card and a patch file's preview in the pseudo-locale.patch-apply,pr-checkout,watch-during-changes,review-changes,store-persistence,gutenberg-siteandterminalwalk 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.
What must not have happened:
Risks and limitations
pr-checkout.cjs,applied-layer.cjs,apply-conflict.cjs,watch-activity.cjsandupdate-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.Related
Part of #557 (item 4 of its nine) and #542. Fifth slice on
redesign/wpds, after #590, #594, #595 and #596.Design decisions and alternatives considered
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.cjsis gone. Its last use was the old pill in the preview; the badge's rule lives with the work-item card's since #596.Review outcome (required — see AGENTS.md)
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
ceba525againstd5bbd47; second pass on the fix-upcab185f; 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:
apply-card.cjs, with unit tests.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:
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:
npx eslint . --max-warnings=0npm testnpm run test:electronnpm run test:e2e(journeys)npm run test:e2e:packagednpm run pack:dirbuild of the headThe 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.
apply-cardapply-cardapply-cardapply-cardapply-cardapply-cardapply-cardapply-cardapply-cardapply-card,patch-applyapply-card,patch-applyapply-cardapply-cardapply-cardapply-cardapply-cardpr-checkoutpr-checkoutpatch-applypatch-applygutenberg-siteterminali18ni18ni18ni18ni18nThe 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.
🤖 Generated with Claude Code