diff --git a/src/lib/components/Sidebar.svelte b/src/lib/components/Sidebar.svelte index 22b55fe..7890d9e 100644 --- a/src/lib/components/Sidebar.svelte +++ b/src/lib/components/Sidebar.svelte @@ -83,6 +83,23 @@ return true; } + // `refActionsBlocked()` only covers the instant of the click. Every ref action that asks + // first then waits on a prompt with no timeout, during which the repository can change — + // and `gitActions` targets `appState.repo`, not whatever was on screen when the dialog + // opened. Confirming afterwards would run the OLD repo's branch or tag name against the + // NEW repository; where both hold that name, "Delete" force-deletes the wrong one. + // + // Capture the repo before the await and re-check after it. Returns a predicate rather than + // taking a callback so each call site keeps its own control flow readable. + function sameRepoAfterPrompt(): () => boolean { + const opened = appState.repo; + return () => { + if (appState.repo === opened) return true; + appState.status = "Repository changed while that dialog was open — nothing was done."; + return false; + }; + } + function onRefCheckout(r: RefEntry, kind: "local" | "remote" | "tag") { if (refActionsBlocked()) return; if (kind === "local") { @@ -108,8 +125,9 @@ const upstream = remoteExists ? tracking : null; const remoteGone = tracking !== null && !remoteExists; const worktree = worktreeFor(name) ?? null; + const sameRepo = sameRepoAfterPrompt(); const res = await dialogs.confirmBranchDelete({ branch: name, upstream, remoteGone, worktree }); - if (!res.confirmed) return; + if (!res.confirmed || !sameRepo()) return; let remote: string | undefined; let remoteBranch: string | undefined; if (res.deleteRemote && upstream) { @@ -175,15 +193,17 @@ items.push({ label: "Rename…", action: async () => { + const sameRepo = sameRepoAfterPrompt(); const n = await dialogs.prompt({ title: "Rename branch", label: "New name", value: r.name }); - if (n && n !== r.name) gitActions.renameBranch(r.name, n); + if (n && n !== r.name && sameRepo()) gitActions.renameBranch(r.name, n); }, }); items.push({ label: "Create tag here…", action: async () => { + const sameRepo = sameRepoAfterPrompt(); const n = await dialogs.prompt({ title: "New tag", label: "Tag name", placeholder: "v1.0.0" }); - if (n) gitActions.createTag(n, r.sha); + if (n && sameRepo()) gitActions.createTag(n, r.sha); }, }); items.push({ separator: true }); @@ -193,8 +213,9 @@ items.push({ label: "Create local branch…", action: async () => { + const sameRepo = sameRepoAfterPrompt(); const n = await dialogs.prompt({ title: `New branch from ${r.name}`, label: "Branch name" }); - if (n) gitActions.createBranch(n, r.sha); + if (n && sameRepo()) gitActions.createBranch(n, r.sha); }, }); items.push({ separator: true }); @@ -205,13 +226,14 @@ const slash = r.name.indexOf("/"); const remote = r.name.slice(0, slash); const branch = r.name.slice(slash + 1); + const sameRepo = sameRepoAfterPrompt(); const ok = await dialogs.confirm({ title: "Delete remote branch", message: `Delete "${r.name}" on the remote? This removes it for everyone with access to ${remote}.`, confirmLabel: "Delete", danger: true, }); - if (ok) gitActions.deleteRemoteBranch(remote, branch); + if (ok && sameRepo()) gitActions.deleteRemoteBranch(remote, branch); }, }); } else { @@ -221,13 +243,14 @@ label: "Delete tag", danger: true, action: async () => { + const sameRepo = sameRepoAfterPrompt(); const ok = await dialogs.confirm({ title: "Delete tag", message: `Delete tag "${r.name}"?`, confirmLabel: "Delete", danger: true, }); - if (ok) gitActions.deleteTag(r.name); + if (ok && sameRepo()) gitActions.deleteTag(r.name); }, }); } @@ -264,8 +287,9 @@ { label: "Create branch here…", action: async () => { + const sameRepo = sameRepoAfterPrompt(); const n = await dialogs.prompt({ title: `New branch at ${sha.slice(0, 7)}`, label: "Branch name" }); - if (n) gitActions.createBranch(n, sha).then((ok) => { if (ok) gitActions.checkout(n); }); + if (n && sameRepo()) gitActions.createBranch(n, sha).then((ok) => { if (ok) gitActions.checkout(n); }); }, }, ]; diff --git a/src/lib/components/WorkingCopyView.svelte b/src/lib/components/WorkingCopyView.svelte index 9ac6c0f..c76143a 100644 --- a/src/lib/components/WorkingCopyView.svelte +++ b/src/lib/components/WorkingCopyView.svelte @@ -8,7 +8,7 @@ import CommitComposer from "./CommitComposer.svelte"; import FileTree from "./FileTree.svelte"; import { buildFileTree } from "../fileTree"; - import { resolveSection, type WorkingSection } from "../workingSection"; + import { renderedPresence, resolveSection, type WorkingSection } from "../workingSection"; import type { WorkingFile } from "../types"; import { crossfade, fade } from "svelte/transition"; import { flip } from "svelte/animate"; @@ -62,15 +62,20 @@ // Which RENDERED lists currently hold the selected path — sections are lists on screen, // not file states. A partially-staged file is in two of them at once, which is precisely - // why the clicked section has to be recorded. Note `unstagedDisplay`, not `unstagedFiles`: - // with unifyUnstaged on, untracked rows are rendered in the Unstaged list and report - // "unstaged", so presence has to agree or the selection resolves to a section that has - // no rows in that mode. - const presence = $derived({ - staged: selectedFile !== null && stagedFiles.some((f) => f.path === selectedFile), - unstaged: selectedFile !== null && unstagedDisplay.some((f) => f.path === selectedFile), - untracked: selectedFile !== null && untrackedFiles.some((f) => f.path === selectedFile), - }); + // why the clicked section has to be recorded. The merged-list handling lives in + // `renderedPresence` rather than here: computing the three fields inline is how they came + // to disagree about what "rendered" means. + const presence = $derived( + renderedPresence( + selectedFile, + { + staged: stagedFiles.map((f) => f.path), + unstaged: unstagedFiles.map((f) => f.path), + untracked: untrackedFiles.map((f) => f.path), + }, + appState.unifyUnstaged, + ), + ); // The section the pane actually shows: the one the user clicked, or — if the file has // since left it (they staged all of it, say) — wherever it went. See resolveSection(). @@ -87,8 +92,12 @@ // Untracked files have no index entry, so the regular diff is empty — they need the // `git diff --no-index` path (and hunk/line staging doesn't apply to them). This is a // property of the FILE, not of the section: with unifyUnstaged on, an untracked file - // is rendered in the Unstaged list but still needs --no-index. - const selectedIsUntracked = $derived(presence.untracked); + // is rendered in the Unstaged list but still needs --no-index. Read from the file list, + // NOT from `presence.untracked`, which answers the narrower "is the Untracked section + // showing this row" and is false while merging. + const selectedIsUntracked = $derived( + selectedFile !== null && untrackedFiles.some((f) => f.path === selectedFile), + ); // Whether the selected file is displayed in the Unstaged section — including an // untracked file that unifyUnstaged has merged into that list, which `presence.unstaged` diff --git a/src/lib/workingSection.test.ts b/src/lib/workingSection.test.ts index a8a75f5..f4a2275 100644 --- a/src/lib/workingSection.test.ts +++ b/src/lib/workingSection.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect } from "vitest"; -import { resolveSection, type SectionPresence, type WorkingSection } from "./workingSection"; +import { + renderedPresence, + resolveSection, + type SectionPresence, + type WorkingSection, +} from "./workingSection"; const present = (p: Partial = {}): SectionPresence => ({ staged: false, @@ -72,3 +77,38 @@ describe("resolveSection", () => { } }); }); + +describe("renderedPresence", () => { + const files = { staged: ["s.txt"], unstaged: ["u.txt"], untracked: ["n.txt"] }; + + it("reports nothing for no selection", () => { + expect(renderedPresence(null, files, false)).toEqual(present()); + }); + + it("maps each file to its own section when the lists are separate", () => { + expect(renderedPresence("s.txt", files, false)).toEqual(present({ staged: true })); + expect(renderedPresence("u.txt", files, false)).toEqual(present({ unstaged: true })); + expect(renderedPresence("n.txt", files, false)).toEqual(present({ untracked: true })); + }); + + it("sees a partially-staged file in both lists at once", () => { + const both = { staged: ["p.txt"], unstaged: ["p.txt"], untracked: [] }; + expect(renderedPresence("p.txt", both, false)).toEqual(present({ staged: true, unstaged: true })); + }); + + // The bug: with unifyUnstaged on there IS no Untracked list on screen — those rows are + // rendered in Unstaged and report "unstaged". Reporting untracked presence anyway let + // resolveSection keep a recorded "untracked" and resolve to a section with no rows, so + // nothing highlighted and the Unstaged header offered "Stage all" instead of "Stage". + it("moves untracked presence into Unstaged when the lists are merged", () => { + expect(renderedPresence("n.txt", files, true)).toEqual(present({ unstaged: true })); + }); + + it("resolves a recorded untracked selection to Unstaged once merging is enabled", () => { + const before = renderedPresence("n.txt", files, false); + expect(resolveSection("untracked", before)).toBe("untracked"); + + const after = renderedPresence("n.txt", files, true); + expect(resolveSection("untracked", after)).toBe("unstaged"); + }); +}); diff --git a/src/lib/workingSection.ts b/src/lib/workingSection.ts index 01c009f..142af37 100644 --- a/src/lib/workingSection.ts +++ b/src/lib/workingSection.ts @@ -27,6 +27,38 @@ export interface SectionPresence { untracked: boolean; } +/** + * Build a `SectionPresence` from the working-copy file lists. + * + * The rule above — presence describes lists ON SCREEN, not file states — is easy to state and + * easy to get wrong: it has been violated once per field. Deriving it inline in the component + * meant `unstaged` was computed from the merged display list while `untracked` was computed + * from the raw file state, so with merging on a path reported presence in BOTH, `resolveSection` + * kept the recorded `"untracked"`, and the pane resolved to a section with no rows — nothing + * highlighted and the Unstaged header offered "Stage all" instead of "Stage". Encoding it here + * keeps the two fields from drifting apart again, and makes the merged case testable. + * + * `untracked` is asked twice for different reasons; do not conflate them. This answers "is the + * Untracked SECTION showing this row", which is false while merging. Whether the FILE is + * untracked — which decides `git diff --no-index` and disables hunk staging — is a separate + * question the caller must answer from the file list regardless of mode. + */ +export function renderedPresence( + path: string | null, + files: { staged: string[]; unstaged: string[]; untracked: string[] }, + unifyUnstaged: boolean, +): SectionPresence { + if (path === null) return { staged: false, unstaged: false, untracked: false }; + const holds = (list: string[]) => list.includes(path); + return { + staged: holds(files.staged), + // With merging on, the Unstaged list renders the untracked rows too. + unstaged: holds(files.unstaged) || (unifyUnstaged && holds(files.untracked)), + // ...and the Untracked section is not rendered at all. + untracked: !unifyUnstaged && holds(files.untracked), + }; +} + /** * The section the diff pane should actually show for the selected file. *