Repository navigation
List the contributor's own changes beside the applied patch's or pull request's - #679
Merged
Merged
Conversation
|
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 |
This was referenced Oct 9, 2026
ryanwelcher
added this pull request to stack #685
October 15, 2026 15:30
Slice 3 of #669. The section becomes "Changed files", in two groups: - "From the patch" / "From PR #N": the change's files, each marked "also edited" when the contributor has changed it since. A patch's files are fingerprinted when it is applied (src/change-fingerprint.cjs: SHA-256 of the content, line endings normalised) and compared with the disk. A pull request is its branch's base, so its files among the changes are edited. A patch applied before this has no fingerprints and is not marked. - "Your changes": every other file changed on the branch, "new" when created. A ticket with nothing applied shows only this group. `git:unsubmitted-work` returns each file's `added` and the `editedSinceChange` list; no new channel. It now reads the branch's record, so the test harness's `patchMain` gets an empty fake store. Refs #669
Two findings from the review of the #669 stack: - The contributor's file list stayed as it was after a discard, a ticket switch, or an apply, revert or pull request switch, until the window next regained focus. Each of those now clears the list and asks again. A journey discards a new file and checks it leaves "Your changes" without a refocus; the "list goes away" checks wait for no dialog to be open, since a modal makes an absence check pass on its own. - Moving a ticket onto trunk dropped the applied patch's text but kept its fingerprints, so a file trunk changed could be marked "also edited". The move now drops both. Refs #669
From the re-review of the #669 stack: every exit from an apply cleared "Your changes" and walked the tree again, including a patch that did not fit and a pull request checkout refused before it started. Nothing was written, so the list blinked out and back, moving the sidebar just as the refusal appeared. The list is now dropped only when the apply, revert or switch wrote; every exit still refreshes it. A journey holds the walk after a refused patch and checks the contributor's file is still listed. It fails without this. Refs #669
A switch that fails part-way keeps the list until the next walk answers, rather than clearing it. Said in the hook's comment, from the review of the previous commit. Refs #669
ryanwelcher
force-pushed
the
add/changed-files-yours
branch
from
October 15, 2026 15:30
eb7be4f to
8400845
Compare
Base automatically changed from
add/affected-files-pr
to
add/affected-files-sidebar
October 19, 2026 15:19
Collaborator
Author
ryanwelcher
added a commit
that referenced
this pull request
Oct 20, 2026
…n the editor (#677) ## Why After a patch is applied or a pull request is checked out, the app says how many files changed and nothing more. Unless a contributor already knows which files to look at, they can't find the change, and their own edits on top of it aren't listed anywhere. The only way to undo one of their edits was **Discard all changes**, which also took the patch away. This is #669 in one PR. It was a stack of five (#677, #678, #679, #682, #683); they are folded together here because each builds the same section, and a reviewer was already testing the top of the stack. ## What changes - The site's details gain a **Changed files** section, last in the sidebar after **Build watch**, so a long list grows without pushing the server and the watch out of reach. - It has up to two groups, each under its own heading and drawn as a tree of folders: - **From the patch** / **From PR #N**: the change's own files, each with a pencil when the contributor has edited it since. - **Your changes**: every other file changed on the branch, with a plus when the contributor created it. On a ticket with nothing applied, this is the only group. - Each folder starts open, and a chain of folders that hold only one another shares one row (`tests/phpunit/tests/wp`), as GitHub and VS Code draw them. A folder row's button, or the Left and Right arrows, close and open it. Each file shows its name with a page icon and its full path as a tooltip. Drawn with `__experimentalTreeGrid` from `@wordpress/components`, which gives the tree roles a screen reader needs and the arrow keys between rows. - One click on a file opens it in the first editor the app detects, inside the site's folder (`open -a App <site> <file>`, `Code.exe <site> <file>`). With no editor detected, the file is selected in Finder or Explorer instead. "Choose application…" after a file fails to open opens that file in the application picked. - **A revert button** on each file the contributor changed, at the end of its row. It asks first, and leaves the file as **Discard your changes** would: a file of theirs back to the base, a new file deleted, a pull request's file back to the pull request's version, a patch's file back to how the patch left it with the patch still applied. A patch file they did not touch, and either end of a rename, have no button. While an install, build, trunk update or dev server runs, the button is held and says why. - **Discard your changes** (renamed from "Discard all changes") now removes only the contributor's own work. With a patch applied, the tree goes back to the base and the patch is re-applied from its stored text, so the patch and **Revert this patch** stay. If the text was not kept, the patch goes too and the dialog says so first. **Discard this ticket to its base** still removes the patch. The ticket card's discard button from #696 calls the same discard, so every "discard your changes" control keeps the patch. - How the pencil is decided: a pull request is its branch's base, so any of its files with changes on top was edited. A patch is an uncommitted layer, so each file it wrote is fingerprinted when applied (SHA-256, line endings normalised, `src/change-fingerprint.cjs`) and compared with the disk. - The list refreshes when the window regains focus, which is when a contributor comes back from their editor, and whenever the app itself changes the tree. - New IPC `git:revert-file`, which accepts only paths Git reported as changed. `editor:open` and `dir:show` take an optional file inside the site, checked by its real path, so a symbolic link that leads out of the site is refused. Not in this PR: an editor preference, per-file diffs, marking deleted files, live updates while the app keeps focus, undo after a revert, reverting several files at once, and remembering which folders were closed across a relaunch. ## How to test this Platforms: macOS and Windows. The launch arguments, path checks and the revert's writes (`core.autocrlf`) differ per platform. The journeys cover the flow with stubbed editors and fixture repositories. Run from the repository root: ``` npm run test:e2e -- tests/e2e/journeys/patch-apply.spec.js tests/e2e/journeys/pr-checkout.spec.js tests/e2e/journeys/i18n.spec.js ``` They check opening a file in the editor or the file manager, "Choose application…", the groups and their marks, the list during a build, after a discard, after a refused apply and after a refocus, each kind of revert, and that a folder closes and opens and the revert button is reached with the arrow keys. What they can't reach is a real editor, a real `wordpress-develop` clone and a real pull request, so that part is by hand. **Starting state:** a site with a real `wordpress-develop` clone, VS Code installed, the dev server stopped, and a ticket linked whose Trac attachment applies cleanly, for example #23602 with `23602_pr14112_20261008.diff`. 1. Apply the attachment. **Changed files** appears at the bottom of the sidebar, under **From the patch**, as a tree of two folder rows: `src/wp-includes` over `class-wp.php`, and `tests/phpunit/tests/wp` over `queryPosts.php`. No pencil, no revert button. 2. Click the `src/wp-includes` folder. Its file is hidden and its chevron points right. Click it again. The file is back. 3. Click `class-wp.php`. VS Code opens the site folder with that file open. Click `queryPosts.php`. It opens in the same VS Code window. 4. In VS Code, change `class-wp.php` and `src/wp-login.php`, and create `src/new-helper.php`. Switch back to the app. `class-wp.php` has a pencil; under **Your changes**, `wp-login.php` has none and `new-helper.php` has a plus. All three have a revert button. 5. Revert `new-helper.php`. The dialog asks "Delete src/new-helper.php?". Confirm. The file is gone from disk and from the list. 6. Revert `class-wp.php`. The dialog says it goes back to how the patch left it. Confirm. The file has the patch's change and not yours, and the pencil is gone. 7. Change `class-wp.php` again and switch back. Click **Review & submit changes**, then **Discard your changes**. The dialog says the patch stays applied. Confirm. `class-wp.php` has the patch's change only, `wp-login.php` is back to trunk, and **Revert this patch** is still there. 8. Click **Revert this patch**. The section is gone. 9. Link a ticket with a GitHub pull request and check it out. **Changed files** lists its files under **From PR #N**, the same files its preview did. 10. On Windows, repeat steps 1, 3 and 6 with a site whose path has a space in it. **What must not have happened:** - A second VS Code window for the same site. - Anything opened outside the site folder. - A file listed in both groups. - Any patched file with a pencil right after applying, or right after step 6, before you changed anything. On Windows this would point at line endings. - The patch's change missing from `class-wp.php` after step 6 or step 7. - Any file other than the one reverted changed by a revert. - After checking out a different pull request, the list from the previous one. - A focus ring cut off at either edge of the sidebar, on a folder, a file or a revert button, after tabbing in or after cancelling a revert. ## Risks and limitations - Review of the combined branch: 7 [fix here] fixed, 3 [follow-up] deferred (see the review outcome). CodeRabbit's one finding, on patches applied before this PR, is fixed. - Rebased onto trunk after #696 and #697. The ticket card's discard button that #696 added now keeps an applied patch or pull request, like the note's link and the review modal's; before the rebase it would have removed the patch. - `__experimentalTreeGrid` is not a stable export yet; the window already uses another `__experimental` control the same way (`create-site-dialog.jsx`). - In the tree, the pencil and plus are the file button's description rather than focusable stops of their own, because only buttons are in the tree's reach of the arrow keys. A screen reader hears them; a sighted keyboard user sees the icon but can't open its tooltip. Follow-up. - Verified by hand with VS Code on macOS. PhpStorm, Zed, and Windows generally are untested with a file argument, and the revert and discard have not been run by hand on a real clone or on Windows. - A chain of folders shares one row, so deep paths indent less, but a long merged folder name, like a long file name, can wrap in a narrow sidebar. - A move onto trunk that updates the branch and then fails at the checkout keeps the patch's old text and fingerprints. A pencil can then mark a file trunk changed, and a discard or revert re-applies text written for the old trunk. Older than this PR; follow-up. - Neither a discard nor a revert is held while an apply or pull request checkout runs. Older than this PR (the discard link had the same gate); follow-up. - A pull request checked out before this ships has no list until it is checked out again. A patch applied before this ships marks every one of its files as edited, since the app did not record what it wrote: **Discard your changes** and each revert stay offered, reverting a file unmarks it, and a discard records the patch afresh. Covered by unit tests only; staging it by hand needs a site from an older build. A file the patch or the contributor deleted is still listed, and says it is missing when clicked. - The diff is about 3,000 lines across 32 files, well over the size AGENTS.md flags. It is one feature in five steps; the commits keep each step apart for reading one at a time, in this order: patches, pull requests, your changes, discard, per-file revert, then the tree. - In one of three local runs of the full journey suite, four journeys failed while the run took three times its usual length; the two runs either side passed all of them. I read it as load on the machine, not the branch. ## Related Part of #669. Replaces #678, #679, #682 and #683, which are folded in here. A pull request's list including files trunk changed, when the site's trunk is behind the pull request, is older than this PR and tracked in #701. --- <details> <summary>Design decisions and alternatives considered</summary> - **One PR, not a stack of five.** Every step builds one section, and a reviewer was testing the top of the stack while commenting on the bottom. - **Trees, one per group.** A reviewer asked for `TreeGrid` because full paths filled the sidebar's width. One tree per group keeps the headings. - **A chain of single folders on one row** (`tests/phpunit/tests/wp`), as GitHub and VS Code draw them. The first version gave every folder its own row; a reviewer pointed out it took five rows to show one file. - **Last in the sidebar**, not first: the list has no fixed height, and first it pushed the server and the build watch out of view. - **A revert leaves the file as Discard your changes would,** so the two never disagree about what "your changes" means. A patch file therefore goes back to the patch's version, not to trunk. - **Discard keeps the patch.** One button that always means "remove my edits, keep the patch or PR" matches what it already did for a PR. Removing the patch stays **Revert this patch**; re-applying the stored text is the only way back to "as the patch left it". - **Renames refused, not modelled.** One change spread over two paths cannot go back one path at a time. - **One click, no per-file menu.** There is no editor preference yet, so the click takes the first detected editor. A preference belongs in the Settings dialog from #638. - **The real path, not just the text.** A lexical check let a symbolic link inside the site point outside it. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> Earlier reviews of each folded PR are recorded on it (#678 at `8ddcb27`, #679 at `eb7be4f`, #682 at `85b47f9`, #683 at `ff23819`, before the rebase); every finding there was fixed or deferred, and the deferrals are in Risks. The reviews below cover this combined branch: reviews 1 to 3 on base `60078fe`, review 4 on trunk `819575a` after the rebase. - **Review 1:** completed. A separate agent context reviewed `60078fe..73b47df` (#677 to #679 plus the tree) against `.github/instructions/code-review.instructions.md`. `npm run lint` clean, `npm test` 2147 passed. 1 [fix here] · 1 [follow-up]. - Fixed in `eb93d07` (🟡): the file list was walked as soon as an apply, revert or pull request switch changed the tree, but the status it is grouped by was read again only after the install and the build, so for those minutes a patch's files were listed as the contributor's. The status is now read with the walk. The journey "while the build after an apply runs, the patch's files are the patch's (#669)" holds the build open; it fails without the fix. - Deferred (🔵): the marks and the rule for whether the section shows are decided in `site-details.jsx` rather than `changedFileGroups`. One branch each. - **Review 2:** completed. Separate agent context, the fix above. No findings; confirmed on every path into the install and build. Its comment note was taken in `52ecde4`. - **Review 3:** completed. Separate agent context, `8400845..52ecde4` (#682, #683 and the tree redrawn over them). 1 [fix here] · 2 [follow-up]. - Fixed in `288b7c8` (🟡): the tree remembers the last button focused in it and gives every other one no tab stop, so a file that left the list on a refocus took the whole tree out of the tab order. The tree is now keyed by its files. The journey "a file that leaves the list does not take its tree out of the tab order (#669)" fails without the fix. - Fixed in `288b7c8` (🔵, raised as a follow-up): the treegrid itself had no accessible name; it now has its group's. - Deferred (🔵): a sighted keyboard user cannot open a mark's tooltip (see Risks). - **Review 4 (rebase onto trunk):** completed. Separate agent context, `819575a...455d0e8`, on the conflict resolution with #696: the ticket card's discard button pointed at `discardYourChanges`, `discardLabel` at its question, and `nothingOfYours` in the shared reason. 1 [fix here] · 0 [follow-up]. Fixed in `a54b394` (🔵): a unit test still tied the label to the full-discard question, passing only because both say "Discard changes". Nothing from #696 or #697 lost; no `discardAllChanges` left. - **CodeRabbit** reviewed `a54b394`: 1 finding (🟡), fixed in `d55ba08`. A kept patch applied before fingerprints marked none of its files as edited, so **Discard your changes** was disabled and no file could be reverted. Its files still changed now count as possibly edited. - **Since then:** `c7f4ad9` merges a chain of single folders into one row, at @zaerl's request; `299bc2d` and `9a28812` give the revert button's focus ring room at the sidebar's edge. - **Review 5:** completed. Separate agent context, `a54b394..d55ba08`. 3 [fix here] · 1 [follow-up], all fixed: - Fixed in `1f93225` (🟡): a folder closed before a file left the list could stop being a row of its own and still hide everything under it, leaving an empty tree. Unit test. - Fixed in `1f93225` (🟡): the window dropped `renamedByChange` from the reply, so both ends of a rename were offered a revert the handler refuses. Now carried by `unsubmittedFromReply`, unit-tested. - Fixed in `0dea62b` (🔵): on a patch from before fingerprints, a reverted file still looked edited. Its first revert now records a full set. Unit test. - Fixed in `4a77c79` (🔵, raised as a follow-up): a first-level folder's focus ring was clipped at the sidebar's other edge. Every cell now has room for the ring; the journey checks a folder, a file and a revert button, and fails without it. - **On `4a77c79`:** `npm run lint` clean, `npm test` 2193 passed, full journey suite 163 passed hidden on macOS. </details> <details> <summary>Implementation notes</summary> - `resolveLaunch(editorPath, sitePath, { platform, file })` appends the file after the folder in the argument vector. Never a shell. - `existingSiteFile(sitePath, relPath, { platform, realPath })` runs realpath on both the site and the file, so a site behind a link (macOS's `/var`) still holds its files. - `src/renderer/affected-files.cjs` holds the decisions, unit-tested: `affectedFiles`, `changedFileGroups` (groups, labels, `canRevert`), `unsubmittedFromReply` (the reply as the list reads it), `affectedFileTree` (paths to tree rows, merging single-folder chains), `visibleAffectedRows` (what a closed folder hides), `revertFileQuestion`, `revertOutcome` and `fileOpenTarget`. - `src/git-write.cjs`: `restoreWorktreePaths(dir, source, paths)`, worktree only, literal pathspecs on stdin. `src/trunk-update.js`: `discardFile`. `src/patch-apply.js`: `applyPatchToDir({ paths })` keeps only those files' sections. - `src/main.js`: `git:revert-file`; `git:discard-to-base(sitePath, { keepChange })` re-applies the kept patch and fingerprints it again; `git:unsubmitted-work` adds `entries`, `editedSinceChange` and `renamedByChange`. - The tree's indent is read from each row's `aria-level` in CSS (`attr(aria-level type(<integer>))`), since the repository's lint bans inline styles. The revert button gets the tree's item props through `Tooltip.Trigger`'s render prop. - `forgetChangedFiles` in `src/renderer/index.jsx` bumps the dirty probe's generation, so an answer still in flight from before a change is dropped, then clears the list and asks again. </details> <details> <summary>Screenshots or recording</summary> To add. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
The section from the previous two PRs lists only the patch's or pull request's files. A contributor's own edits, and everything on a fresh ticket with nothing applied, weren't listed anywhere. This is slice 3 of #669.
What changes
The section becomes Changed files, in two groups:
How "also edited" is decided:
src/change-fingerprint.cjs) and compared with the disk. A patch applied before this ships has no fingerprints and gets no marks, rather than wrong ones.git:unsubmitted-workreturns each file's path and whether it was added, pluseditedSinceChange. No new IPC channel. The list refreshes when the window regains focus, which is when a contributor comes back from their editor, and whenever the app itself changes the tree (discard, ticket switch, apply, revert, pull request switch).Not in this PR: live updates while the app keeps focus (an edit made from a terminal shows at the next refocus), and per-file diffs.
How to test this
Platforms: any.
The journeys cover each case with stubbed editors and fixture repositories. Run from the repository root:
04963ef.5f556a7.pr-checkout.spec.jschecks also edited and Your changes on top of a pull request.By hand, on a real clone, since the journeys use small fixtures:
Starting state: a site with a real
wordpress-developclone and a linked ticket whose Trac attachment applies cleanly (for example #23602 with23602_pr14112_20261008.diff).src/wp-includes/class-wp.phpand create a new file insrc/. Switch back to the app.class-wp.phpshows also edited; the new file is under Your changes with new.What must not have happened:
Risks and limitations
Related
Part of #669. Stacked on #678.
Review outcome (required — see AGENTS.md)
.github/instructions/code-review.instructions.md. Head0ffe810, base7ee3ead. 2 [fix here] · 0 [follow-up], both in this PR, fixed in04963ef: the file list went stale after a discard, ticket switch, apply, revert or PR switch until the next refocus (🟡), and moving onto trunk kept the patch's fingerprints (🔵).04963ef, base8ddcb27. Both earlier findings resolved. 1 [fix here] · 1 [follow-up], both 🔵. Fixed in5f556a7: a refused apply cleared the list, so it blinked out and back. Deferred: a move onto trunk that fails at the checkout keeps the patch's text and fingerprints (see Risks).04963ef..5f556a7. 1 [fix here] · 1 [follow-up], both 🔵. A switch that fails part-way now keeps the list until the next walk answers, instead of clearing it; accepted and said in the hook's comment ineb7be4f. Deferred: no journey holds the walk across a successful apply, so deleting the clear there would pass the suite.5f556a7 → eb7be4fis a comment only.Implementation notes
changedFileGroupsinsrc/renderer/affected-files.cjsdecides the groups and their labels, unit-tested. A file is in one group only.forgetChangedFilesinsrc/renderer/index.jsxbumps the dirty probe's generation, so an answer still in flight from before a change is dropped, then clears the list and asks again.git:unsubmitted-worknow reads the store, so the IPC harness'spatchMain()includes the fake settings store.toHaveCount(0)) wait until no dialog is open and the sidebar's "Details" heading is visible. A modal hides the page from the accessibility tree, so the check would otherwise pass on its own.Screenshots or recording
To add.
🤖 Generated with Claude Code