feat: add diff hunk/block hover affordance and hunk/line discard - #26
feat: add diff hunk/block hover affordance and hunk/line discard#26ashproto wants to merge 25 commits into
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>
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>
💡 Codex Reviewgit-it/crates/git-core/src/ops_worktree.rs Lines 519 to 521 in 36547f7 If the file changes in an external editor while the discard confirmation is open, an inserted or removed earlier hunk can reindex the diff before this command runs. The caller still submits the old numeric index, but git-it/crates/git-core/src/ops_worktree.rs Line 536 in 36547f7 When the user reduces diff context to 0 and discards lines from a later hunk after an earlier hunk has changed the line count, this calls git-it/src/routes/+page.svelte Lines 295 to 298 in 36547f7 This window-level handler prevents every native context menu, including those on commit-message inputs, review textareas, diffs, and rendered Markdown, while the repository only supplies custom menus for a few row types. Consequently, users who right-click editable or selectable content lose mouse access to copy, paste, spelling suggestions, and related platform actions. Limit suppression to elements that provide a custom menu or to non-editable chrome. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…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>
|
Addressed in P1 — validate the selected patch before destructive apply ✅ fixedConfirmed, and the exposure window is wider than described: it is the entire confirmation dialog, which has no timeout. The displayed diff now travels with the request; git-core compares it to the live one before Left alone on purpose: P2 — preserve both hunk coordinates ✅ fixed, though not for the stated reasonThe header defect is real, but the mechanism in the report does not hold up. I tested Chasing that turned up something worse. At
Ablating either half alone reintroduces failures (13 and 5), so both are load-bearing. Worst case found: at 27 tests across four shapes × six ops. P2 — keep context menus available in editable content ⏸️ real, but not this PRAgreed on the substance, but Separately, a review pass found a pre-existing silent corruption in |
💡 Codex Reviewgit-it/crates/git-core/src/git_ops.rs Lines 94 to 96 in eccbee2 When the destination is an existing bare repository, it has no ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Addressed in #29, not here — The finding is correct. Both guards do miss a bare repository:
One correction to the characterisation, since "mutates a directory that should have been rejected" reads worse than what happens. Running the exact command with a bare repo as cwd: It nests a repository inside the bare one, but the bare repo survives intact — a wrong state, not data loss. Still exactly what the sibling guard exists to prevent, so it is worth fixing. Fixed by probing the destination with |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eccbee2c1d
ℹ️ 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".
`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.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a42da328b
ℹ️ 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".
| let h = hunks.get(hunk_index).ok_or("hunk index out of range")?; | ||
| let set: std::collections::HashSet<usize> = selected.iter().copied().collect(); | ||
| let partial = build_partial_hunk(h, &set, true).ok_or("no lines selected to discard")?; | ||
| git_apply(repo, &format!("{}{}", content_only_header(&header), partial), true, false, context == 0) |
There was a problem hiding this comment.
Normalize new-file headers before partial discard
When a path has been marked intent-to-add with git add -N, working_changes classifies it as tracked and unstaged, so the UI exposes line-level discard. build_partial_hunk(..., true) turns unselected additions into context, but this call prepends the original new file mode and --- /dev/null header; Git then rejects the partial patch with new file f depends on old contents. Consequently, “Discard 1 line” consistently fails for intent-to-add files even though whole-hunk discard works; construct an ordinary content-diff header when a partial new-file patch retains context.
Useful? React with 👍 / 👎.
Adds a Fork/SourceTree-style hover affordance to the working-copy diff, adds hunk- and line-level Discard (which the app had at no granularity below whole-file), and fixes the line-selection visual that was effectively invisible in the NERV theme.
The three problems this solves
1. Selection was invisible in NERV.
.diff-row.selectedwasbox-shadow: inset 2px 0 0 var(--accent)plus a 15% accent wash. In Classic that's blue over near-white and reads fine; in NERV the accent is orange#F2542Dand a deleted row's background is alreadyrgba(255,68,56,0.15), so 15% orange over 15% red on a#12171Cpanel was close to no change at all — andnerv.cssrecolors--accentper scheme, so crimson and phosphor made it worse. Fixed by deleting the rule: selection now reuses the hover ring. That works because hover is fully suppressed while a selection is locked, so the two can never both be on screen and can safely share one visual.2. No way to discard a hunk or a line range. Added end to end.
git_applygains acachedflag; dropping--cachedturns the existing reverse-apply into a worktree discard. Because the unstaged diff's old side is the index, discarding reverts to your staged state rather than HEAD — staged work on the same file survives, which is what makes Discard safe to sit beside Stage.3. No hover affordance. Nested rings — hunk outline (a CSS
outlineon a per-hunk<tbody>) and an inner block ring — with a floating Stage/Discard/Unstage toolbar. Double-click locks a contiguous selection, Shift+click extends, plain click clears. Unified and Split.Two bugs found during review, both fixed here
The toolbar was unclickable.
onmouseleavesat on the<tbody>while the toolbar lives outside the table, so moving the pointer toward it unmounted it before the click landed (b4abb4c).Split-view selection silently reordered files. A "paired row" is a display artifact —
toSplitRowsputs the Nth deletion beside the Nth addition because they fit on a line together, not because they correspond. Selecting one pair emitted non-contiguous ordinals ([2,5]), andbuild_partial_hunkemits in hunk source order, so the restored line landed above the lines the user kept. Reproduced against real git: discarding a pair producedccc XXX YYYinstead ofXXX YYY ccc, accepted silently, with no reflog to recover from.Fixed in two layers: split selection now snaps to whole blocks (
e11b9ef), andgit-corerefuses a gapped ordinal set outright (ad86a8d) so no future caller can reintroduce it. The invariant: only contiguous ordinal sets are correct — unified ranges and whole blocks always are; partial coverage of a mixed block never is.Also included
selectedIsStagedmeant "has any staged content", so anMMfile always showed its staged diff even when you clicked its Unstaged row — hiding hunk/line Stage and Discard on the very half those actions exist for. Selection now records which list the row was clicked in (41ec3b5,0981c36).@@header bar; block/selection actions anchor to the ring, so the toolbar's position tells you what it will hit.Testing
npm run check530 files / 0 errors, 0 warnings ·npm test386 passed (343 baseline + 43 new) ·cargo test -p git-core190 passedNew pure modules with vitest coverage:
src/lib/diff/blocks.ts,src/lib/diff/splitRows.ts,src/lib/workingSection.ts. Two coverage holes were closed by mutation testing — bothisChangeRowclauses could previously be deleted with the whole suite staying green.Reviewed per-task throughout, plus a 22-agent whole-branch review across five dimensions with adversarial refutation of every finding.
Manually verified in a running build (28 items), including the four that no automated test can reach: the
<tbody>outline does render in WKWebView; the ring is legible in all six NERV schemes plus Classic; discard reverts the correct lines with staged work surviving (checked againstgit show :file); and a whole-block split discard leaves the file identical to HEAD — no reordering.Known / not verified
color-mix(--diff-ring 34%)at 1px) and is the only indicator of hunk scope for a destructive action — may deserve strengthening.--diff-ringexists as a token sonerv.csscan retune a scheme without touching component markup.🤖 Generated with Claude Code