From 49cf2396f3cc259fc08dd5b4a3d8358ca6da7be7 Mon Sep 17 00:00:00 2001 From: Ash Shah <494shah@gmail.com> Date: Sun, 9 Aug 2026 17:03:14 -0700 Subject: [PATCH 1/2] fix: resolve an untracked selection into Unstaged once the lists are merged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Section presence describes lists ON SCREEN, not file states — the rule the SectionPresence doc already spells out. The component derived the three fields inline and they disagreed about it: `unstaged` came from the merged display list, `untracked` came from the raw file state. So selecting an untracked file and then turning on "Merge Untracked into Unstaged" reported presence in BOTH. resolveSection kept the recorded "untracked", the pane resolved to a section with no rows in that mode, nothing highlighted, and the Unstaged header offered "Stage all" instead of "Stage". The rule has now been violated once per field, so it moves out of the component into `renderedPresence` in workingSection.ts, beside the type it builds and the resolver that consumes it, with the merged case under test. `untracked` was also answering two different questions through one field. It now answers only "is the Untracked SECTION showing this row", which is false while merging. Whether the FILE is untracked — which selects `git diff --no-index` and disables hunk staging — reads from the file list directly and is unaffected by the mode, which is what it always needed to be. The Untracked header that also consults it renders only when NOT merging, so the two readings coincide there. Reported by Codex review on the v0.4.0 promotion (#30). npm run check 530 files 0 errors; npm test 393 passed; cargo test 232 + 7. Co-Authored-By: Claude Opus 5 --- src/lib/components/WorkingCopyView.svelte | 33 +++++++++++------- src/lib/workingSection.test.ts | 42 ++++++++++++++++++++++- src/lib/workingSection.ts | 32 +++++++++++++++++ 3 files changed, 94 insertions(+), 13 deletions(-) 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. * From 3d8f4094e0dbe8fc9e9fa9c20706e2761b6710e1 Mon Sep 17 00:00:00 2001 From: Ash Shah <494shah@gmail.com> Date: Sun, 9 Aug 2026 17:12:28 -0700 Subject: [PATCH 2/2] fix: re-check the repository after a ref dialog, not only before it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `refActionsBlocked()` covers the instant of the click. Every ref action that asks first then waits on a prompt with no timeout, and `gitActions` targets `appState.repo` — not whatever was on screen when the dialog opened. Switch repositories mid-prompt and confirming runs the OLD repo's branch or tag name against the NEW repository; where both hold that name, "Delete" force-deletes the wrong one. The same time-of-check/time-of-use shape as the stale-diff guard on discard, which is the precedent for fixing it the same way. Codex flagged branch delete. Six more sites have the identical gap, three of them destructive, so all seven are guarded rather than the one reported: confirmBranchDelete -> deleteBranch (reported) prompt -> renameBranch prompt -> createTag prompt -> createBranch (from ref) confirm -> deleteRemoteBranch confirm -> deleteTag prompt -> createBranch (detached HEAD) `sameRepoAfterPrompt()` captures the repo before the await and returns a predicate checked after it, so each call site keeps its own control flow instead of being wrapped in a callback. Refusing states plainly that nothing was done, rather than failing silently or acting on a guess. Reported by Codex review on #32. npm run check 530 files 0 errors; npm test 393 passed; cargo test 232 + 7. Co-Authored-By: Claude Opus 5 --- src/lib/components/Sidebar.svelte | 38 +++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 7 deletions(-) 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); }); }, }, ];