fix(vcs): keep changes left out of a commit excluded (#1556)

A successful commit cleared every inclusion override in the Commit tool window, so files the user had unchecked fell back to included and could go into the next commit by accident. commitChanges now drops the overrides (checks and per-file block exclusions) only for the changes that went into the commit; files left out stay excluded.

Closes #1539

Co-authored-by: xtr-hub <[email protected]>

e2e/commitInclusion.spec.ts

@@ -0,0 +1,90 @@
+/**
+ * #1539: committing a subset of the changed files must not check the ones left out again.
+ * Git is stubbed at the IPC layer, like `commit.spec.ts`.
+ */
+import type { Page } from "@playwright/test";
+import { expect, ipcCalls, stubDesktop, test } from "./fixtures";
+
+const ROOT = "/dev/acme-api";
+const NAMES = Array.from({ length: 20 }, (_, i) => `src/f${i}.ts`);
+
+/** Twenty modified tracked files; `vcs_stage` + `vcs_commit` take the staged ones out of the status. */
+async function stubTwentyChanges(page: Page) {
+  await stubDesktop(page);
+  await page.evaluate(
+    async ({ root, names }) => {
+      const staged = new Set<string>();
+      const committed = new Set<string>();
+      const all = names.map((rel) => ({
+        path: `${root}/${rel}`,
+        relPath: rel,
+        staged: null,
+        unstaged: "modified",
+        renamedFrom: null,
+      }));
+      const h = window.__ipc.handlers;
+      h.vcs_roots = () => [{ root, operation: null, head: "main", headCommit: "1111111" }];
+      h.vcs_branch_info = () => ({
+        head: "main",
+        headCommit: "1111111",
+        upstream: null,
+        ahead: 0,
+        behind: 0,
+        operation: null,
+      });
+      h.vcs_status = () => all.filter((f) => !committed.has(f.path));
+      h.vcs_stage = (args) => {
+        for (const path of args.paths as string[]) staged.add(path);
+        return null;
+      };
+      h.vcs_unstage = () => null;
+      h.vcs_commit = () => {
+        for (const path of staged) committed.add(path);
+        staged.clear();
+        return "2222222";
+      };
+      const vcsModule = "/src/features/vcs/store.ts";
+      const { useVcs, refreshNow } = await import(/* @vite-ignore */ vcsModule);
+      useVcs.setState({ available: true, root, repo: root, branchesLoaded: false, stashesLoaded: false });
+      await refreshNow();
+    },
+    { root: ROOT, names: NAMES },
+  );
+}
+
+test("changes left out of a commit stay unchecked (#1539)", async ({ workbench: page }, testInfo) => {
+  await stubTwentyChanges(page);
+  await page.keyboard.press("Meta+k");
+  const panel = page.getByRole("region", { name: "Commit", exact: true });
+  const changes = panel.getByRole("tree", { name: "Changes" });
+
+  // Uncheck everything, then pick the 5 files to commit.
+  await changes.getByRole("checkbox", { name: "Include Changes in commit" }).click();
+  for (const name of NAMES.slice(0, 5)) {
+    await changes.getByRole("checkbox", { name: `Include ${name.split("/").pop()} in commit` }).click();
+  }
+  await expect(changes.getByRole("checkbox", { name: "Include f0.ts in commit" })).toHaveAttribute(
+    "aria-checked",
+    "true",
+  );
+
+  await panel.getByRole("textbox", { name: "Commit message" }).fill("Commit 5 of 20");
+  await panel.getByRole("button", { name: "Commit", exact: true }).click();
+  await expect(page.getByText(/5 files committed: Commit 5 of 20/)).toBeVisible();
+  expect((await ipcCalls(page, "vcs_stage")).flatMap((a) => a.paths as string[]).sort()).toEqual(
+    NAMES.slice(0, 5)
+      .map((rel) => `${ROOT}/${rel}`)
+      .sort(),
+  );
+
+  // The 15 files left out are still in the tree, unchecked: the next commit cannot take them by surprise.
+  const leftOut = changes.getByRole("checkbox", { name: "Include f19.ts in commit" });
+  await expect(leftOut).toBeVisible();
+  await page.screenshot({ path: testInfo.outputPath("commit-inclusion.png"), animations: "disabled" });
+  for (const name of NAMES.slice(5)) {
+    await expect(changes.getByRole("checkbox", { name: `Include ${name.split("/").pop()} in commit` })).toHaveAttribute(
+      "aria-checked",
+      "false",
+    );
+  }
+});

src/features/vcs/__tests__/changes.test.ts

@@ -3,12 +3,14 @@ import type { FileStatus } from "@/bindings";
 import {
   aggregateCheck,
   buildChangeTree,
+  checkOf,
   commitRepos,
   ignoreTargets,
   initialCheck,
   hunkIncluded,
   groupFileNodes,
   planCommit,
+  remainingOverrides,
   stagingSide,
   toggledHunk,
   type CheckState,
@@ -218,6 +220,28 @@ describe("commit inclusion", () => {
     expect(aggregateCheck([status[1]!], checks)).toMatchObject({ checked: false, indeterminate: true });
   });
 
+  it("keeps the changes left out of a commit excluded after it", () => {
+    const files = Array.from({ length: 20 }, (_, i) => fs(`src/f${i}.kt`, null, "modified"));
+    const leftOut = files.slice(5);
+    const checks = new Map<string, CheckState>(leftOut.map((f) => [f.path, "none"]));
+    const plan = planCommit(files, checks);
+    expect(plan.included).toHaveLength(5);
+
+    // The commit takes its 5 files out of the tree; the 15 left out keep their override.
+    const kept = remainingOverrides(checks, plan.included);
+    expect(kept.has(files[0]!.path)).toBe(false);
+    expect(leftOut.every((f) => checkOf(f, kept) === "none")).toBe(true);
+    expect(aggregateCheck(leftOut, kept)).toMatchObject({ checked: false, indeterminate: false });
+  });
+
+  it("drops only the committed paths from the per-file overrides", () => {
+    const overrides = new Map([
+      ["/r/a.ts", new Set(["h1"])],
+      ["/r/b.ts/", new Set(["h2"])],
+    ]);
+    expect([...remainingOverrides(overrides, [fs("a.ts", null, "modified")])]).toEqual([["/r/b.ts/", new Set(["h2"])]]);
+  });
+
   it("commits a file with excluded blocks partially, without staging it", () => {
     const checks = new Map<string, CheckState>([["/r/src/main/kotlin/App.kt", "hunks"]]);
     const plan = planCommit(status, checks);

src/features/vcs/__tests__/commitInclusion.test.ts

@@ -0,0 +1,73 @@
+import { beforeEach, describe, expect, it, vi } from "vitest";
+
+const unstage = vi.hoisted(() => vi.fn());
+const stage = vi.hoisted(() => vi.fn());
+
+vi.mock("@/platform/ipc", async (importOriginal) => {
+  const real = await importOriginal<typeof import("@/platform/ipc")>();
+  const ok = (data: unknown) => Promise.resolve({ status: "ok" as const, data });
+  return {
+    ...real,
+    isTauri: () => false,
+    call: async (p: Promise<{ status: "ok"; data: unknown }>) => (await p).data,
+    commands: {
+      ...real.commands,
+      vcsUnstage: unstage.mockImplementation(() => ok(null)),
+      vcsStage: stage.mockImplementation(() => ok(null)),
+      vcsCommit: () => ok("commit1"),
+    },
+  };
+});
+
+import type { FileStatus } from "@/bindings";
+import { commitChanges } from "../changeOps";
+import { useCommit } from "../commitStore";
+import { useVcs } from "../store";
+
+const files: FileStatus[] = Array.from({ length: 20 }, (_, i) => ({
+  path: `/p/src/f${i}.ts`,
+  relPath: `src/f${i}.ts`,
+  staged: null,
+  unstaged: "modified",
+  renamedFrom: null,
+}));
+
+const excluded = files.slice(5);
+
+describe("changes left out of a commit (#1539)", () => {
+  beforeEach(() => {
+    localStorage.clear();
+    unstage.mockClear();
+    stage.mockClear();
+    useVcs.setState({
+      available: false,
+      root: "/p",
+      repo: "/p",
+      roots: ["/p"],
+      status: files,
+      branch: null,
+      operations: {},
+    });
+    useCommit.setState({
+      message: "Commit 5 of 20",
+      amend: false,
+      checks: new Map(excluded.map((f) => [f.path, "none" as const])),
+      excludedHunks: new Map([[files[5]!.path, new Set(["h1"])]]),
+      checkProblems: [],
+    });
+  });
+
+  it("keeps the 15 excluded files excluded instead of checking them again", async () => {
+    expect(await commitChanges()).toBe("commit1");
+    // Only the 5 checked files were staged for the commit.
+    expect(stage.mock.calls[0]?.[0]).toEqual(files.slice(0, 5).map((f) => f.path));
+    expect(unstage).not.toHaveBeenCalled();
+
+    const { checks, excludedHunks } = useCommit.getState();
+    expect(checks.size).toBe(15);
+    expect(excluded.every((f) => checks.get(f.path) === "none")).toBe(true);
+    expect(files.slice(0, 5).some((f) => checks.has(f.path))).toBe(false);
+    // Block exclusions of files still in the tree are kept too.
+    expect(excludedHunks.get(files[5]!.path)).toEqual(new Set(["h1"]));
+  });
+});

src/features/vcs/changeOps.tsx

@@ -4,7 +4,7 @@ import type { FileStatus, PartialCommitFile, Stash } from "@/bindings";
 import { call, commands } from "@/platform/ipc";
 import { Checkbox, Dialog, Icon, Input, T, fileIconName } from "@/ui";
 import { notifications } from "@/workbench/contributions";
-import { commitRepos, groupOf, planCommit, renamedFromPath } from "./changes";
+import { commitRepos, groupOf, planCommit, remainingOverrides, renamedFromPath } from "./changes";
 import { useChangelists, withListDefaults } from "./changelists";
 import { runCommitChecks } from "./commitChecks";
 import { forgetAmendDraft, keepMessageAfterCommit, rememberMessage, useCommit } from "./commitStore";
@@ -114,7 +114,14 @@ export async function commitChanges(): Promise<string | undefined> {
   if (id !== undefined) {
     rememberMessage(message);
     forgetAmendDraft();
-    useCommit.setState({ amend: false, checks: new Map(), excludedHunks: new Map(), checkProblems: [] });
+    // What the user left out of this commit stays left out; only the committed changes start over.
+    const { checks: userChecks, excludedHunks } = useCommit.getState();
+    useCommit.setState({
+      amend: false,
+      checks: remainingOverrides(userChecks, plan.included),
+      excludedHunks: remainingOverrides(excludedHunks, plan.included),
+      checkProblems: [],
+    });
     keepMessageAfterCommit(message, workflowSettings().vcs.commit.clearInitialMessage);
   }
   return id;

src/features/vcs/changes.ts

@@ -381,6 +381,20 @@ export function planCommit(status: readonly FileStatus[], checks: ReadonlyMap<st
   return plan;
 }
 
+/**
+ * Inclusion overrides kept after a commit (same shape for `checks` and `excludedHunks`):
+ * the committed changes start over, since they either left the tree or only some of
+ * their blocks went in. The changes the user left out keep their override, so they stay
+ * excluded instead of falling back to "included" in the next commit.
+ */
+export function remainingOverrides<T>(
+  overrides: ReadonlyMap<string, T>,
+  committed: readonly FileStatus[],
+): Map<string, T> {
+  const done = new Set(committed.map((f) => stripSlash(f.path)));
+  return new Map([...overrides].filter(([path]) => !done.has(stripSlash(path))));
+}
+
 /**
  * Toggling the checkbox of one changed block (`key`, one of `all`) in the diff: the file's
  * new inclusion and excluded blocks. All blocks back in → `all`; none left → `none`.