From 94576eb8796f99301a2ba9d7b96fabd5f81c357a Mon Sep 17 00:00:00 2001 From: xtr-hub <3786329580@qq.com> Date: Thu, 5 Nov 2026 21:23:05 +0800 Subject: [PATCH] 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 <242808782+xtr-hub@users.noreply.github.com> --- e2e/commitInclusion.spec.ts | 90 +++++++++++++++++++ src/features/vcs/__tests__/changes.test.ts | 24 +++++ .../vcs/__tests__/commitInclusion.test.ts | 73 +++++++++++++++ src/features/vcs/changeOps.tsx | 11 ++- src/features/vcs/changes.ts | 14 +++ 5 files changed, 210 insertions(+), 2 deletions(-) create mode 100644 e2e/commitInclusion.spec.ts create mode 100644 src/features/vcs/__tests__/commitInclusion.test.ts diff --git a/e2e/commitInclusion.spec.ts b/e2e/commitInclusion.spec.ts new file mode 100644 index 000000000..b5a0bda45 --- /dev/null +++ b/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(); + const committed = new Set(); + 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", + ); + } +}); diff --git a/src/features/vcs/__tests__/changes.test.ts b/src/features/vcs/__tests__/changes.test.ts index 38ad0400b..679dd59ef 100644 --- a/src/features/vcs/__tests__/changes.test.ts +++ b/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(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([["/r/src/main/kotlin/App.kt", "hunks"]]); const plan = planCommit(status, checks); diff --git a/src/features/vcs/__tests__/commitInclusion.test.ts b/src/features/vcs/__tests__/commitInclusion.test.ts new file mode 100644 index 000000000..9b84e9777 --- /dev/null +++ b/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(); + 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"])); + }); +}); diff --git a/src/features/vcs/changeOps.tsx b/src/features/vcs/changeOps.tsx index 3a2ad7a01..54569f79e 100644 --- a/src/features/vcs/changeOps.tsx +++ b/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 { 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; diff --git a/src/features/vcs/changes.ts b/src/features/vcs/changes.ts index 9559885b5..43301a235 100644 --- a/src/features/vcs/changes.ts +++ b/src/features/vcs/changes.ts @@ -381,6 +381,20 @@ export function planCommit(status: readonly FileStatus[], checks: ReadonlyMap( + overrides: ReadonlyMap, + committed: readonly FileStatus[], +): Map { + 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`.