Conversation
Design for a nested hover affordance (hunk + change-block rings with floating stage/discard actions) in the working-copy diff, hunk- and line-level discard, and a replacement for the line-selection visual that is effectively invisible in the NERV theme. Interaction model validated against a live prototype before writing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eight TDD tasks: pure block/ordinal helpers, git-core discard ops, Tauri/api/gitActions plumbing, hover rings, floating toolbar, range selection, split view, and keyboard parity. Also corrects the spec's ring mechanism: box-shadow does not merge across rules but custom properties do, so the block ring is pure CSS rather than a JS-positioned overlay. Only the toolbar needs measuring. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Shift+Arrow steps by BLOCK in split view, where row-stepping was a no-op after selection snapping (new `splitBlockRanges`, which `splitRangeSnappedToBlocks` now reuses). - The hunk-scope toolbar anchors to the hovered row, so hunks taller than the pane no longer hide it. - The Discard confirm states the changed-line count instead of "this hunk", which understated Whole-file mode's single whole-file hunk. - Roving tabindex: one tab stop per diff instead of one per row, with unshifted arrows moving between rows and Tab reaching the toolbar. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Implementation disproved the spec's claim that a paired split row could select "as a unit". A paired row is a display artifact, so selecting one emits non-contiguous ordinals and build_partial_hunk — which emits in hunk source order — silently reorders the file. Verified against real git. Selection now snaps to whole change blocks, enforced in the frontend and refused outright by git-core as a second line of defence. Also folds in two plan corrections made during execution: hover must clear on the outer wrapper rather than the tbody, and the min-height/min-width discrepancy in Task 5. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hreads
TempFolder::new() named its directory git-it-init-test-{pid}-{nanos}. The
four initialize_repository tests run on parallel threads in one process, so
the pid is shared and two threads could read the same nanosecond; fs::create_dir
then failed with AlreadyExists and the unwrap() panicked. A different one of
the four failed on each run, which is what made it look random.
Replace the wall-clock stamp with a process-wide AtomicU32 counter, matching
the TempRepo fixtures in ops.rs and ops_worktree.rs. pid + counter cannot
collide by construction, so create_dir is kept over create_dir_all: a
collision should now be impossible, and would be a real bug worth panicking on.
Verified with five consecutive full cargo test runs, all green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
selectedIsStaged asked "does this file have any staged content", so a partially-staged (MM) file always showed its STAGED diff even when the user clicked its Unstaged row. Every action gated on that flag was therefore unavailable on the unstaged half: not just the new hunk/line Discard, but hunk/line Stage too — both sides of the workflow the feature exists for. Selection now records the section the row was clicked in, the same discriminator onRowContext already used for its menu. resolveSection() picks the section to display, falling back to wherever the file went if it has left the clicked one (staging all of it, say) rather than stranding an empty diff. Row highlight follows the resolved section, so an MM file highlights only the row being diffed instead of both. Discarding unstaged lines of a partially-staged file reverts them to the staged version rather than to HEAD, so the confirm dialog now says that instead of claiming the change is permanently lost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
presence.unstaged checked unstagedFiles, which excludes untracked, while the rendered Unstaged list is unstagedDisplay, which includes them when "Merge Untracked into Unstaged" is on. A merged untracked row therefore reported section "unstaged" but resolved to "untracked" — a section with no rows in that mode — so the selected row never highlighted, clicking it again could not deselect, and the Unstaged header fell back to "Stage all". Sections are rendered lists, not file states, so presence now describes what is on screen. Two comments that asserted the opposite are corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Hunk-scope actions anchored to hov.ri, so every context row the pointer crossed re-anchored the toolbar and it chased the cursor down the hunk. That anchor existed to stop the toolbar hiding on hunks taller than the pane; clamping (d8b84c6) now covers that, so the anchor can be stable. Hunk actions now sit on the hunk's own @@ header bar, where their scope is written. Block and selection actions still anchor to their ring, so the toolbar's position says which of the two you are targeting. The header row gains data-h/data-i as the anchor, which also made it match the arrow-key row query; that selector is narrowed to .diff-row so ArrowDown cannot land on a non-focusable header and dead-end. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous commit claimed pid + counter "cannot collide by construction" and kept `create_dir` over `create_dir_all` on that basis. That reasoning only holds among LIVE processes. The counter restarts at zero every run, so uniqueness rests entirely on the pid never repeating — and a pid is unique only while its process is alive. A run killed before `Drop` (Ctrl-C on cargo test, or a panic=abort) leaves its directories behind; once the OS recycles that pid, a fresh process counting from zero reproduces the exact same path and `create_dir(...).unwrap()` panics with AlreadyExists. Not hypothetical: this machine currently holds 36 orphaned `gte-*` fixture directories spanning 18 distinct pids. Remove any leftover first, then `create_dir_all` — the same two lines the TempRepo fixtures in ops.rs, ops_worktree.rs, ops_merge.rs, ops_rewrite.rs and graph.rs already use. Those fixtures were right and this one was the outlier; the extra line is the point, not redundancy. Reported by Codex review on #25. cargo test -p git-core: 183 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tale diff Both from the Codex review on #26. Investigating the first turned up a worse bug underneath it. ## Hunk placement at -U0 `git apply` positions a hunk by the coordinate of the image it produces — new_start forward, old_start under --reverse — then offset-searches for the preimage around it. That search is why a wrong coordinate normally goes unnoticed, and it is exactly what hid this: at context >= 1 every case still landed correctly. At -U0 a pure insertion (forward) or a pure deletion (reverse) has an EMPTY preimage. Nothing to search for, so git applies at precisely the line named and reports success from the wrong place. Two defects met there: - A hunk lifted out of a multi-hunk diff keeps BOTH coordinates from the full diff, so the side not facing the apply target is offset by the hunks left behind. `build_partial_hunk` made it worse by writing old_start into both slots. New `reanchor` keeps the trustworthy side and derives the other; it is now shared with the whole-hunk ops, which had the same fault by way of replaying git's live header. - Without --unidiff-zero git enforces "a hunk with no trailing context must match at EOF", which every context-free hunk trips. Passed only when context == 0; at any real depth those checks stay on. Removing either half alone reintroduces failures (13 and 5 respectively), so both are load-bearing. Worst case found: at -U0, staging a hunk silently appended the line at end-of-file and returned Ok. Restoring a deleted line via discard was mis-placed the same way — on the path with no reflog. ## Stale diff under the confirmation dialog A hunk index is just an integer; nothing in it says which hunk it meant. The discard confirmation has no timeout, and the fswatch refresh never invalidates the captured index, so an external edit could re-split the file and the op would reverse-apply a hunk the user never saw. The displayed diff now travels with the request and git-core compares it to the live one BEFORE selecting any hunk, so a mismatch cannot touch the working tree. Whole-file rather than per-hunk: distinguishing a harmless edit would mean trusting the same index arithmetic that is in question. Left alone deliberately: stage_hunk/unstage_hunk freshness (same window, but they mutate the index, which is recoverable). 27 tests over four shapes x six ops. cargo test -p git-core 220 passed; npm run check 530 files 0 errors; npm test 386 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
When the last line of a file with no trailing newline changes, git emits a
`-`/`+` pair where BOTH sides carry `\ No newline at end of file`. The marker
claims the line before it ends the image it belongs to. `build_partial_hunk`
kept that marker whenever the preceding line had been emitted — including when
a demoted-to-context line was emitted after it, which makes the claim false.
`git apply` accepts the contradiction and resolves it by concatenating. With
base "a\nt" and worktree "a\nT2", discarding just the deletion produced
"a\ntT2" — the two lines merged onto one, Ok(()) returned, on the path with no
reflog and no backup bundle. Pre-existing: reproduces against HEAD.
The two directions are not symmetric, so they get different answers.
Reversing (discard/unstage) keeps the other half as context on the side that
still ends there, so dropping the stale marker expresses the intent exactly —
verified against real git, the file becomes "a\nt\nT2".
Going forward (stage) would need the demoted line newline-terminated on the new
side and not on the old. One context line cannot say that, so no patch
expresses it and it is now refused, in the same spirit as require_contiguous.
Whole-hunk ops replay git's own hunk verbatim and are unaffected, so staging
the whole hunk remains available.
Deciding the marker's fate needs to know what follows it, so the emitter now
tracks what the previous line was emitted AS rather than merely that it was,
and build_partial_hunk returns Result<Option<String>, String> to carry the
refusal. Callers keep their own "no lines selected" messages.
partial_hunk_no_newline_marker asserted the buggy output — its comment narrated
the mechanism ("last_emitted=true -> marker kept"). That exact patch shape was
confirmed against real git to stage "oldnew" instead of "old\nnew", so the
first half now expects the refusal.
cargo test -p git-core 224 passed; npm run check 530 files 0 errors;
npm test 386 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A window-level handler cancelled every native context menu in the app. The handful of row types with their own menu already call preventDefault themselves, so all it did elsewhere was take functionality away: right-clicking a commit-message box, a review textarea, diff code, or rendered Markdown gave nothing at all — no Copy, Paste, Look Up, or spelling. On macOS that reads as broken. The intent was presumably to hide WebKit's developer menu, but `devtools` is not in the Tauri feature list, so the inspector only exists on debug builds and never ships. Nothing needed guarding in release. Rather than invent a second notion of "editable or selectable", read the computed `user-select`: the stylesheet right below already curates that set, opting a no-select body back in for form fields, diff code, rendered markdown, commit bodies and `.sha`. It inherits, so descendants of `.md`/`.selectable` are covered without enumerating them, and DiffView's gutter — which sets its own `user-select: none` so gutter-drag staging never selects text — keeps suppressing its menu for free. Both the prefixed and unprefixed properties are read: minimumSystemVersion is 12.3, where unprefixed support is not a given. Verified by running the handler against the live stylesheet: chrome, nested toolbar spans and the gutter suppress; input, textarea, .md and its descendants, .diff-cell, .sha, .body-msg and deep .selectable children get the native menu. What that menu RENDERS is a WKWebView decision no test environment reproduces, so it still wants a look in a bundled build. npm run check 524 files 0 errors; npm test 330 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fix: make the init-repo test fixture collision-proof under parallel threads
`initialize_repository` guarded against an existing repository two ways, and a bare one slipped between them: it has no `.git` child, and `rev-parse --is-inside-work-tree` answers `false` inside it. The destination is non-empty, so the user got the "folder is not empty" prompt rather than "already a Git repository" — and on confirming, `git init` ran with the bare repo as its working directory. Verified against real git: it prints "Initialized empty Git repository in .../bare.git/.git/" and creates a nested repository inside the bare one. The bare repo's own HEAD, refs and core.bare survive untouched — `diff -rq` against a backup shows only the added `.git` — so this is a wrong state rather than data loss, but it is exactly what the sibling guard exists to prevent. Probe the destination itself with `rev-parse --resolve-git-dir`. It answers for the path GIVEN and does not walk up to a parent, so a plain folder that merely sits inside a repository is still a valid destination — that case belongs to is_inside_worktree, and a test now pins it. The `.git` check is kept alongside rather than replaced, so nothing that was refused before is accepted now. Reported by Codex review on #26; the code is not part of that PR — it came from 15e51c3 (2026-07-17) and git_ops.rs is untouched there — so it is fixed here. cargo test full workspace green (git-core 185); npm run check 524 files 0 errors; npm test 330 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`chmod +x` plus an edited line is one diff with one file header, and git puts `old mode`/`new mode` in that header. Discard reverse-applies the header to the working tree, and `git apply` honours a mode pair — so "Discard 1 line" also took the executable bit off. Not in the confirmation, not in the line count, and not undoable. Verified against real git: reverse-applying the header with the mode pair took f.sh from 755 to 644 while reverting the text; dropping the two mode lines left 755 and reverted the text just the same. Codex flagged `discard_lines`. `discard_hunk` reverse-applies the same header and loses the bit identically, so both destructive ops now build a content-only header. Stage and unstage keep the mode deliberately: there it belongs to the same index entry the caller is moving, and the result is recoverable either way. cargo test -p git-core: 222 passed (the one failure in a full run is the `git_ops.rs` init-repo flake, fixed on `next` by #25 and merged in next commit). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A discard carries the patch the user was judging so git-core can refuse one picked against a diff that has since moved. That guard worked, but the retry did not: `applyWorkingChanges` compares WorkingFile metadata and skips the update when it matches, so an external edit to an ALREADY-modified file leaves the status list identical, never bumps `workingChangesRev`, and the shown patch stays stale. `runWorktree` does not refresh on failure, so every retry sent the same stale patch and got the same refusal — until the user happened to reselect the file or change the context depth. Add `invalidateWorkingDiff()`, which bumps the revision the diff effect keys on, and call it when a discard fails. Refusal becomes something the next click can get past instead of a dead end. Deliberately on any discard failure rather than only the stale one: after a failed destructive op the shown state is unverified either way, and matching on message text would be brittle. Success still refreshes exactly once through `runWorktree` — untouched, so the stage/unstage row animation is unaffected. npm run check 530 files 0 errors; npm test 386 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Picks up #25's init-repo fixture fix so this branch's CI stops rolling the `git_ops::tests::initialize_repository_*` dice.
… prefixes `git add -N` makes a path tracked-and-unstaged, so the UI offers line-level discard on it. Its diff header says `new file mode` / `--- /dev/null`, but a partial selection keeps the unselected additions as CONTEXT — an old side the header denies — and git refused the whole apply with "new file X depends on old contents". Whole-hunk discard worked, so it looked arbitrary. The deleted-file mirror needs no repair: reverse-apply DROPS unselected `-` lines rather than demoting them, so the new side stays empty and `+++ /dev/null` stays true. The repair is therefore conditioned on the emitted hunk having an old side, which also leaves a selection covering every addition alone so it still deletes the file the way a whole-hunk discard does. Adversarial review caught the first attempt being far worse than the bug. It rewrote the `---` side by replacing the first `b/` ANYWHERE in the `+++` path. `diff()` did not pin the prefixes, so under `diff.mnemonicPrefix` (prefixes become `i/`/`w/`) `w/lib/util.js` became `w/lia/util.js` — git read that as a rename and applied it: the file the user asked to discard one line from was emptied, a committed file they never touched was rewritten, and it returned Ok(()). Reproduced against real git. Two changes, either of which would have prevented it, because this path has no reflog: - `diff()` pins `--src-prefix=a/ --dst-prefix=b/`. The output is not merely displayed, it is fed back to `git apply`; the user's config governs what they read in a terminal, not what this reconstructs. Also closes a pre-existing hazard: under `diff.noprefix` a patch loses a component to apply's `-p1` and lands on the wrong file, which affects stage/unstage too. - The rewrite only ever swaps a LEADING `b/`, and when there is no such prefix the header is left completely untouched — the shape that shipped before this repair, which git refuses. Half-repairing was never verified, so it is not a state worth entering. `discard_lines_is_unaffected_by_diff_prefix_config` covers both configs and asserts the neighbour file is untouched; confirmed to fail without the pin. cargo test -p git-core 225 passed; npm run check 530 files 0 errors; npm test 386 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`clippy::if_same_then_else` fired on the two arms that both drop a header line: the mode pair, and `new file mode` when the new-file header is being repaired. Name the two conditions and test them together. No behaviour change. Missed locally because the documented gate is `npm run check` + `npm test` + `cargo test`, while CI also runs `cargo clippy -p git-core -p git-it -D warnings`. cargo test -p git-core -p git-it green; clippy clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#26 gained four commits while this sat stacked on it — the mode-change and intent-to-add discard fixes, the diff-prefix pin, and a merge of `next`. Two conflicts, both in ops_worktree.rs: - `discard_lines`: this branch made `build_partial_hunk` return a Result (`?`), #26 routed the header through `discard_header`. Kept both. - The tests block: both sides appended tests at the same point and shared one trailing brace. Kept every test from both, closing the first block explicitly. cargo test -p git-core 229 passed — all four no-newline-EOF tests and all five of #26's discard tests green together; clippy clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
feat: add diff hunk/block hover affordance and hunk/line discard
…tion fix: stop a partial selection at a no-newline EOF from merging two lines
…logs `tryPresentPendingAutomaticUpdate` gated on `dialogs.state` alone, so a check completing while Settings, Manage Repository, amend/rebase, branch-colour or a GitHub action was open still passed the guard — and the update dialog, which sits at a higher z-index, mounted straight over the workflow in progress. `overlays.ts` already exists as the one place that knows every overlay singleton, precisely so callers stop chasing individual stores and drifting as new ones are added. Ask it instead. The pending update is not dropped: the existing retry timer re-checks until nothing is covering the screen. `dialogs` stays imported — still used by the two confirm() calls below. No import cycle: nothing overlays.ts pulls in imports the updater. Reported by Codex review on #27. npm run check 524 files 0 errors; npm test 330 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two findings from Codex review on #29. **Git directory as parent.** The same blind spot this PR fixed for the destination, one level up. When the destination does not exist yet the nesting probe falls back to the PARENT, and `--is-inside-work-tree` answers false inside a Git directory just as it does inside a bare repo. Verified: selecting `some-repo/.git` produced `some-repo/.git/proj`, a whole repository inside another repository's metadata, and returned initialized: true. A bare repo as parent did the same. `is_git_dir(&parent)` now rejects both before any filesystem mutation, with a message that names the actual problem. **Node floor.** Vite 8 requires `^20.19.0 || >=22.12.0` (recorded in package-lock.json) but package.json declared no `engines` and the build-from-source instructions named no version, so `npm install` on Node 18 or early 20 warns and then fails later as a confusing build error. Declared in `engines` and stated in the README next to the commands it governs. CI already builds on 24, so this documents the existing requirement rather than changing it. Unrelated to the rest of this PR, but fixing it here keeps the review on one branch. cargo test 186 + 7 passed; clippy clean; npm run check 524 files 0 errors; npm test 330 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fix: let the native context menu through where text is selectable
…rseded update Two findings from Codex review on #29. Both are pre-existing on `next` and in files this PR does not otherwise touch; fixed here to keep the review on one branch. **P1 — sidebar acting on the wrong repository.** A switch deliberately keeps the previous repo's refs on screen until the reload lands, because clearing them flashed the sidebar empty mid-switch. But `appState.repo` already points at the new repo, so a command fired from a stale row runs against it. With a name both repos have — `main`, `develop` — "Delete branch" deletes the wrong repo's branch, and there is no reflog prompt in front of that. Clearing the state would reintroduce the flicker the retention exists to prevent, so the rows stay and the actions go inert: checkout, delete (also the Linked Worktrees panel, which routes through the same handler), and the ref context menu all refuse while `repoLoading`, which `reloadGraph` clears in a `finally` guarded against a superseding switch. The context menu bails before it is built, so its items are unreachable rather than merely guarded. Selection and scrolling stay live — they read nothing and write nothing. Also gated the detached-HEAD menu, which the report did not mention: "Create branch here" would use the OLD repo's HEAD sha, and two clones of one project share commit ids, so that can quietly succeed in the wrong repository instead of erroring on an unknown sha. **P2 — superseded automatic update still prompting.** A manual check replaces the backend's pending-update slot, but the UI queue was only cleared on an exact version match. A manual check finding nothing, or a different version, left the old one queued; closing the overlay then presented a version the backend no longer had — Download failed with "no pending update", or fetched something other than what the dialog named. The queue is now dropped when a manual check starts. Two regression tests, both watched failing first. npm run check 530 files 0 errors; npm test 388 passed; cargo test 232 + 7; clippy clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…r itself `is_git_dir` resolves an exact path, so the parent guard saw `repo/.git` and a bare repo but walked straight past `repo/.git/hooks` and `bare.git/objects` — which a folder picker reaches just as easily. `--is-inside-work-tree` is false down there too, so nothing else caught it either, and the flow created a repository inside another repository's object store. Reproduced: it returned initialized: true with a path of `host/.git/hooks/proj`. `--is-inside-git-dir` is documented as true anywhere below the repository directory, which is the question a candidate parent actually has to answer: path resolve-git-dir is-inside-git-dir normal (worktree) false false normal/.git true true normal/.git/hooks false true bare.git true true bare.git/objects false true plain (no repo) false error The working-tree row is why this stays a separate probe from the nesting one: a normal working directory must remain a valid parent. The error row is handled the way `is_inside_worktree` already does — a non-zero exit counts as false. `is_git_dir` still guards the DESTINATION, where the exact-path question is the right one. The parent test now covers both tops and both descendants. cargo test 232 + 7; clippy clean; npm run check 530 files 0 errors; npm test 388 passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fix: reject an existing bare repository as an init destination
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90f66147ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`resolveSection` returns null once the path is in no list — its last change was committed or discarded, so the file is no longer part of the working copy. But `diffKey` tested only `selectedFile`, so it stayed non-empty and the effect fetched an unstaged diff for a file with nothing to diff. No row could highlight, since the row test already requires a matching section, while the pane rendered an empty diff instead of "Select a file to view its diff." Gate both on `section !== null` rather than clearing the selection: no state written from a derived or an effect, and it keeps the "follow the file wherever it went" behaviour the section resolution exists for — if the path comes back, the selection resumes instead of having been silently dropped. Reported by Codex review on the v0.4.0 promotion (#30). Verified by reading plus `npm run check`; there is no WorkingCopyView test harness, and the path is behind `isTauri()` so the browser preview cannot reach it either. npm run check 530 files 0 errors; npm test 388 passed; cargo test 232 + 7. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tion fix: show the empty state when the selected file leaves the working copy
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02af5d3e74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…merged 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 <noreply@anthropic.com>
`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 <noreply@anthropic.com>
fix: resolve an untracked selection into Unstaged once the lists are merged
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 554c1c941d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+931
to
+934
| if (!confirmed) return false; | ||
| return discarded( | ||
| await runWorktree(`Discard hunk in ${path}`, () => | ||
| api.discardHunk(appState.repo, path, hunkIndex, expectedDiff, appState.effectiveDiffContext), |
There was a problem hiding this comment.
When the repository changes while this confirmation is open—for example through the still-available macOS File → Open Repository menu—the callback reads appState.repo only after the await and applies the old path, hunk index, and displayed patch to the new repository. The snapshot guard does not bind the request to a repository, so switching between clones/worktrees with the same path and diff lets the check pass and irreversibly discards changes in the wrong repository; capture the repository before opening the dialog and abort if it changes. The same issue is present in discardLines below.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Confirmed and fixed in #33 — next is protected by a ruleset requiring a PR, so it lands there and this promotion picks it up.
You are right, including the part that matters most: expected_diff pins the DIFF, not the repository. I built that guard for exactly this class of staleness and it does not cover this one — two clones or worktrees holding the same path with the same diff pass the check, and the wrong copy is irreversibly discarded.
This is the third appearance of the shape — guard, unbounded await, act — after the diff snapshot itself and the sidebar ref actions. So rather than patch discardHunk/discardLines, I audited every awaited dialog in the file. Nine sites, all now bound:
| site | |
|---|---|
runDestructive |
covers every caller — reset, rebase, amend, reword… |
runDestructiveRebase |
ditto |
discardHunk, discardLines |
reported |
discard (files), clean (untracked) |
destructive |
force push --force-with-lease |
|
| create pull request | |
runRemote credential retry |
the retry re-invokes the caller's closure, so a switch during sign-in would push to a remote the user never chose |
That last one was not on my list until an audit script flagged it; both destructive wrappers hold their dialog internally, so guarding them covers every caller at once.
Refusing rather than retargeting, which was your suggestion and I agree with it: the user authorised an action against what they were looking at, and silently applying it elsewhere is a different action.
Verification is a script that walks every await dialogs. and checks a capture precedes it and a guarded use follows — 9 sites, 0 unguarded. npm run check 530 files / 0 errors · npm test 393 · cargo test 232 + 7. These paths need a running app and a mid-dialog repo switch to exercise, so they are verified by reading and the type gate rather than by a test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotes
nexttomain. Merging this cuts stable v0.4.0 —compute-version.mjsresolvesv0.3.0+ afeat:commit to a minor bump:44 commits, 25 files, +4772/−315, from five merged PRs (#25 #26 #27 #28 #29). No
website/changes, so this does not redeploy the site — it publishes a signed + notarized stable build and advances the stable updater channel.Headline — diff hunk/block affordance and discard (#26)
Nested hover rings over hunks and change-blocks, a floating Stage/Unstage/Discard toolbar, contiguous range selection with keyboard parity, and section-aware working-copy selection so a partially-staged (
MM) file behaves correctly on both halves. Discard is new and unstaged-only; it reverse-applies against the index, so discarding unstaged lines reverts them to the staged version and staged work survives.Correctness fixes worth calling out
Several of these are silent-corruption or wrong-target bugs found during review rather than cosmetic polish:
-U0hunk placement.git applyoffset-searches for the preimage, so a wrong@@coordinate is invisible at context ≥ 1 — but at-U0a pure insertion (forward) or pure deletion (reverse) has an empty preimage and lands exactly where the header says. Staging a hunk silently appended the line at end-of-file and returnedOk. Fixed by re-anchoring extracted hunks and passing--unidiff-zeroonly at-U0.chmod +xplus an edit putold mode/new modein the header, so "Discard 1 line" also removed the executable bit.diff()now pins--src-prefix/--dst-prefix. Underdiff.noprefixa reconstructed patch loses a path component togit apply's-p1and lands on the wrong file — this affected stage/unstage too, not just the new discard paths.appState.repoalready points at the new one — so "Delete branchmain" could delete the wrong repository's branch. Ref actions are now inert whilerepoLoading. The detached-HEAD "Create branch here" was gated too: two clones of a project share commit ids, so a stale sha can silently succeed in the wrong repo rather than erroring..gitdirectory, or anything below either is now refused as a destination or parent — previouslygit initwould nest a repository inside another's metadata.^20.19.0 || >=22.12.0, Vite 8's requirement).Verification
cargo test -p git-core -p git-it232 + 7 ·cargo clippy -p git-core -p git-it -- -D warningsclean ·npm run check530 files / 0 errors ·npm test388 passed. Every constituent PR was CI-green and carries a Codex 👍.Known gaps — worth a look before or shortly after release
None are known-broken; all three are unverifiable in any test environment, so they shipped on evidence-by-reading rather than evidence-by-running:
devtoolsis not in the Tauri feature list.🤖 Generated with Claude Code