Skip to content

Add "Uncommit changes" option to committed hunk context menu #13177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cbjeukendrup wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add "Uncommit changes" option to committed hunk context menu #13177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions apps/desktop/src/components/diff/HunkContextMenu.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import { PROJECTS_SERVICE } from "$lib/project/projectsService";
import { SETTINGS } from "$lib/settings/userSettings";
import { STACK_SERVICE } from "$lib/stacks/stackService.svelte";
import { UI_STATE } from "$lib/state/uiState.svelte";
import { inject } from "@gitbutler/core/context";
import { ContextMenu, ContextMenuItem, ContextMenuSection, TestId } from "@gitbutler/ui";
import type { TreeChange } from "$lib/hunks/change";
Expand All @@ -29,6 +30,8 @@
trigger: HTMLElement | undefined;
projectId: string;
change: TreeChange;
stackId?: string;
commitId?: string;
discardable: boolean;
selectable: boolean;
selectAllHunkLines: (hunk: DiffHunk) => void;
Expand All @@ -40,6 +43,8 @@
trigger,
projectId,
change,
stackId,
commitId,
discardable,
selectable,
selectAllHunkLines,
Expand All @@ -48,6 +53,7 @@
}: Props = $props();

const stackService = inject(STACK_SERVICE);
const uiState = inject(UI_STATE);
const ircApiService = inject(IRC_API_SERVICE);
const projectService = inject(PROJECTS_SERVICE);
const urlService = inject(URL_SERVICE);
Expand Down Expand Up @@ -106,6 +112,40 @@
});
}

async function uncommitHunk(item: HunkContextItem) {
if (!(stackId && commitId)) return;

const previousPathBytes =
change.status.type === "Rename" ? change.status.subject.previousPathBytes : null;

unselectAllHunkLines(item.hunk);

const isWholeFileChange =
change.status.type === "Addition" || change.status.type === "Deletion";

const { replacedCommits } = await stackService.uncommitChanges({
projectId,
stackId,
commitId,
changes: [
{
previousPathBytes,
pathBytes: change.pathBytes,
hunkHeaders: isWholeFileChange ? [] : [item.hunk],
},
],
});

const replacementCommit = replacedCommits.find(([before]) => before === commitId)?.[1];
const selection = uiState.lane(stackId).selection.current;

if (replacementCommit && selection) {
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
if (replacementCommit && selection) {
if (replacementCommit && selection && selection.commitId === commitId) {

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@cbjeukendrup cbjeukendrup Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@estib-vega estib-vega Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

uiState.lane(stackId).selection.set({ ...selection, commitId: replacementCommit });
}

contextMenu?.close();
}

export function open(e: MouseEvent | HTMLElement | undefined, item: HunkContextItem) {
contextMenu?.open(e, item);
}
Expand Down Expand Up @@ -148,6 +188,17 @@
{/if}
</ContextMenuSection>
{/if}
{#if stackId && commitId}
<ContextMenuSection>
<ContextMenuItem
label="Uncommit changes"
icon="commit-undo"
onclick={async () => {
await uncommitHunk(item);
}}
/>
</ContextMenuSection>
{/if}
<ContextMenuSection>
<ContextMenuItem
testId={TestId.HunkContextMenu_OpenInEditor}
Expand Down
2 changes: 2 additions & 0 deletions apps/desktop/src/components/diff/UnifiedDiffView.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,8 @@
trigger={viewport}
{projectId}
{change}
{stackId}
{commitId}
discardable={isUncommittedChange}
{selectable}
{selectAllHunkLines}
Expand Down
Loading