Skip to content

First stab at supporting Gutenberg projects - #7

Closed
adamziel wants to merge 1 commit into
create-prfrom
gutenberg-contribution
Closed

adamziel wants to merge 1 commit into
create-prfrom
gutenberg-contribution

Conversation

@adamziel

@adamziel adamziel commented Nov 8, 2025

Copy link
Copy Markdown
Contributor

wip

juanmaguitar added a commit that referenced this pull request Aug 7, 2026
- git:apply-patch rejects a sitePath that is not a registered site before
  writing anything, the same gate sites:set-ticket uses (#8).
- Persisting the revert record is now part of the apply transaction: if the
  store write fails the apply is undone, and if that also fails the result
  says the patch is applied-but-untracked rather than reporting a clean
  failure (#7).
- git:preview-patch surfaces a worktree-inspection failure instead of
  reporting "no collisions" when it could not look (#9).
- git:discard-changes clears the applied-patch record with the reset, so a
  later trunk-update network failure cannot leave a revert banner for a
  patch that is already gone (#6).
- git:apply-patch is now a WIRED handler with real tests (guard, reverse
  lookup, delegation, done event, metadata) via fakeSettingsStore, instead
  of a NOT_REACHABLE hole (#13).

Not gating git:preview-patch: it is read-only, and the registered-site
check would add a store dependency to a handler that has none.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar added a commit that referenced this pull request Aug 7, 2026
The #7 transactional branch — undo the apply when the revert record cannot
be saved, and report applied-but-untracked when that undo also fails — had
no coverage. Drive store.set to throw and assert both outcomes.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@juanmaguitar

Copy link
Copy Markdown
Collaborator

Closing as superseded. This branch predates the current architecture by ~100 commits — it targets the old single-project layout and the pre-modularisation main.js/index.jsx, so it isn't revivable as code. The intent behind it is sound, though, and is now captured fresh against today's trunk in #251. Thanks for the early sketch — it's what confirmed the idea was worth doing properly.

@juanmaguitar
juanmaguitar deleted the gutenberg-contribution branch August 21, 2026 09:12
zaerl added a commit that referenced this pull request Oct 2, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants