Add "Uncommit changes" option to committed hunk context menu#13177
Add "Uncommit changes" option to committed hunk context menu#13177cbjeukendrup wants to merge 1 commit intogitbutlerapp:masterfrom
Conversation
cbjeukendrup
commented
Apr 3, 2026
cbjeukendrup
commented
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
There was a problem hiding this comment.
Pull request overview
Adds an “Uncommit changes” action to the hunk context menu when viewing committed diffs in the Desktop app, enabling uncommitting at the hunk level and keeping UI selection in sync after commit rewrite.
Changes:
- Pass
stackIdandcommitIdintoHunkContextMenufromUnifiedDiffView. - Add an “Uncommit changes” context menu item for committed hunks and implement the
uncommitChangescall + selection update.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/desktop/src/components/diff/UnifiedDiffView.svelte | Wires stackId/commitId into the hunk context menu so committed hunks can trigger uncommit. |
| apps/desktop/src/components/diff/HunkContextMenu.svelte | Adds “Uncommit changes” menu item and performs stackService.uncommitChanges, then updates lane selection. |
| const replacementCommit = replacedCommits.find(([before]) => before === commitId)?.[1]; | ||
| const selection = uiState.lane(stackId).selection.current; | ||
|
|
||
| if (replacementCommit && selection) { |
There was a problem hiding this comment.
When updating the lane selection after uncommitChanges, this will overwrite the current uiState.lane(stackId).selection even if the user is currently focused on a different commit in the same lane. Consider only updating the selection when selection.commitId === commitId (similar to how StackService.uncommit() only clears selection when the uncommitted commit matches the current selection), to avoid unexpected navigation jumps.
| if (replacementCommit && selection) { | |
| if (replacementCommit && selection && selection.commitId === commitId) { |
There was a problem hiding this comment.
I think this might not be a problem in practice, because I recall having seen similar code, and the existence of the HunkContextMenu instance already implies that its commit is the currently selected commit
There was a problem hiding this comment.
Actually this is not a bad idea to have.
Some extra context
We've been talking about decoupling selection from preview, since that has creating less-than nice UX around mutations. So, yes, the fact that you can access the HunkContextMenu at all currently implies that the commit in question is selected. But if that changes, we'd still mistakenly update the selection.
Let's err on double checking whether the stale commit was selected before we update the selection.
How to do this though
Let's add that check and update, only not at the component level.
In the StackService class (stackService.svelte.ts) we already have access to the UI State, and in some cases we already take care of updating the selection if necessary (e.g. updateBranchName).
That way, we don't have to deal with updating stale selection everywhere the mutation is called.
(We might still do so for some mutations but that is something to clean-up, rather than an intended pattern)
Please move this if-statement to the contents of uncommitChanges, so that the selection is updated only if the commit in question was already selected.
estib-vega
left a comment
estib-vega
left a comment
There was a problem hiding this comment.
Thank you for adding this ❤️. I have no idea why this wasn't already part of the context menu options. Just left a comment on updating the UI state stale selection
| const replacementCommit = replacedCommits.find(([before]) => before === commitId)?.[1]; | ||
| const selection = uiState.lane(stackId).selection.current; | ||
|
|
||
| if (replacementCommit && selection) { |
There was a problem hiding this comment.
Actually this is not a bad idea to have.
Some extra context
We've been talking about decoupling selection from preview, since that has creating less-than nice UX around mutations. So, yes, the fact that you can access the HunkContextMenu at all currently implies that the commit in question is selected. But if that changes, we'd still mistakenly update the selection.
Let's err on double checking whether the stale commit was selected before we update the selection.
How to do this though
Let's add that check and update, only not at the component level.
In the StackService class (stackService.svelte.ts) we already have access to the UI State, and in some cases we already take care of updating the selection if necessary (e.g. updateBranchName).
That way, we don't have to deal with updating stale selection everywhere the mutation is called.
(We might still do so for some mutations but that is something to clean-up, rather than an intended pattern)
Please move this if-statement to the contents of uncommitChanges, so that the selection is updated only if the commit in question was already selected.
