Repository navigation
Revert one changed file from the site's details, and mark edits with icons - #683
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 |
deb2a0d to
52b9862
Compare
…icons Each file under "Your changes", and each patch or pull request file the contributor edited, gets a revert button. The file ends as "Discard your changes" would leave it: a file of the contributor's goes back to the base or is deleted, a pull request's goes back to the pull request, and a patch's goes back to the base with its part of the patch put back. An untouched patch file, or one whose patch text was not kept, has none. "also edited" and "new" become a pencil and a plus, named for screen readers and explained in a tooltip. Part of #669.
- Add revertFile to the packaged smoke test's preload keys. - Refuse either end of a file the patch renames, and give neither a button: it goes back only with the whole patch. - Decide a patch file by the same "edited" answer the list marks it by, so a file the contributor deleted or edited back to the base reverts. - Give the held revert button its reason, as the discard link does, and make the marks focusable so a keyboard reaches their tooltips. - Move the toast and the reply's wording into affected-files.cjs. - Clear a revert's error when the list it was shown under goes. - Say when a failed revert could not put the file back, and do not report a failure for a revert whose record write failed. - Test the rollback, a file the patch created or deleted, renames, and the #172 guard. Part of #669.
The new revert-file tests built their repository without core.autocrlf, so on Windows the app's CRLF view wrote the files back with CRLF and the LF assertions failed. They now use the shape the app's own clone has, as the comment on adoptedRepo says byte-for-byte tests should. The patch preview's path is now looked for in the dialog only: the file list in the site's details names the same file once it is edited, which made the locator match twice whenever the list refreshed first.
52b9862 to
e5bf043
Compare
|
Folded into #677: this PR's three commits ( |
…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)
Why
"Changed files" lists what a contributor changed, but the only way to undo one file was Discard your changes, which takes everything. And the also edited and new text marks read as clutter beside the paths. This is slice 5 of #669.
What changes
git:revert-file. It accepts only paths Git itself reported as changed (or a patch file the list marks as edited), and refuses the rest with a sentence. The file is snapshotted first and put back if its part of the patch will not re-apply.restoreWorktreePaths(git restore --source=<base> --worktree), reached through adiscardFileflow insrc/trunk-update.jsnext todiscardToBase.applyPatchToDirgains apathsoption.Not in this PR: undo after a revert, reverting several files at once, and reverting either end of a rename on its own.
How to test this
Platforms: macOS and Windows. The revert writes files through Git's worktree view, which differs on Windows (
core.autocrlf).The journeys cover the flows with fixture repositories. From the repository root:
patch-apply.spec.js: a new file, a file of your own, and an edited patch file.pr-checkout.spec.js, which now reverts an edited PR file.By hand, on a real clone:
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). Dev server stopped.src/wp-includes/class-wp.php, editsrc/wp-login.php, and createsrc/new-helper.php. Switch back to the app.class-wp.phphas a pencil; under Your changes,wp-login.phphas none andnew-helper.phphas a plus. All three have a revert button.src/new-helper.php. The dialog asks "Delete src/new-helper.php?". Confirm: the file is gone from disk and from the list.src/wp-login.php. It is back to trunk's content and leaves the list.src/wp-includes/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, the pencil is gone, and Revert this patch is still offered.What must not have happened:
class-wp.phpafter step 6.Risks and limitations
wordpress-developclone or on Windows; the journeys use fixtures.Related
Part of #669. Stacked on #682.
Design decisions and alternatives considered
Review outcome (required — see AGENTS.md)
9 [fix here] · 1 [follow-up]. Fixed: 9 of 9.
.github/instructions/code-review.instructions.md. Head9c3bfcf, base85b47f9.npm run lintclean;npm test2165 pass / 0 fail / 2 skipped; journeys 160 passed.revertFilemissing from the packaged smoke test's preload keys. Fixed.index.jsx. Fixed:revertOutcomeand the question'sdonetext inaffected-files.cjs, with tests.9c3bfcf → ff23819is the fixes above, not re-reviewed. Checked onff23819:npm run lintclean;npm test2172 pass / 0 fail / 2 skipped; full journey suite 160 passed. The packaged smoke test was not run locally; its key list was checked againstsrc/preload.js.Implementation notes
src/git-write.cjs:restoreWorktreePaths(dir, source, paths), worktree only, literal pathspecs on stdin, Windows view prefixed.src/trunk-update.js:discardFile(dir, baseOid, relPath, { added }).src/patch-apply.js:applyPatchToDir({ paths })keeps only those files' sections.src/main.js:git:revert-file;renamedByPatch;git:unsubmitted-workaddsrenamedByChange.src/renderer/affected-files.cjs:canRevertper row,revertFileQuestion,revertOutcome.src/renderer/components/site-details.jsx: the marks andRevertButton.Screenshots or recording
To add.
🤖 Generated with Claude Code