Repository navigation
fix(diff-viewer): keep trailing deleted lines reachable by scroll, scrollbar, and keyboard - #977
Merged
Merged
Conversation
…g the diff When a diff's final hunk only removes lines from the end of a file, scrolling the after (right) pane to its bottom left the before pane's viewport ending exactly where the deletion starts: the alignment transfer keeps shared rows level, and once the after pane was clamped at its own extent nothing could push the before pane any further. The removed lines were unreachable via wheel, scrollbar, keyboard, and mobile touch scrolling, which all drive the after side. scrollController.scrollBy now hands the delta the source pane cannot consume at an edge to the other pane (clamped to its own extent), and unwinds that handoff before moving the source again when direction reverses so neither pane jumps. Mid-file behaviour is unchanged. The same mechanism covers lines appended to the end of a file when scrolling the before pane, and deletions at the start of a file at the top edge. DiffViewer's open-file auto-scroll also anchors on the before pane when the first hunk has no after-side lines, so a pure trailing deletion lands in view immediately instead of just past the fold. Adds scrollController tests for the trailing-deletion fixture, the mirrored trailing-addition case, and the leading-deletion case. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…unk navigation on the pane with rows Follow-up to 5606eb1, which made trailing deleted lines reachable by wheel. Two gaps remained. The after pane's scrollbar sized and positioned its thumb against the pane's own extent and reported drags as deltas, so once the pane sat at its bottom a drag or track click produced a zero delta and the handoff never ran; the deleted tail stayed unreachable by scrollbar. scrollController now exposes getEffectiveExtent(side): the pane's own range plus the headroom the other pane can still travel past alignment at either edge (contentOffset for the top headroom, so markers can stay anchored to the pane's content). A new position-based scrollToEffective(side, y) applies the same handoff scrollBy performs incrementally. Scrollbar sizes its thumb against that extent, remaps marker percentages into the pane's sub-range, and reports thumb drags and track clicks through a new onScrollTo callback, so dragging to the bottom carries the other pane through the handoff and dragging back up unwinds it without the thumb lagging the pointer. Wheel over the scrollbar still goes through onScroll/scrollBy. The before pane is symmetric (trailing additions), and inside the pane's own range the mapping is identical to scrollTo. alignments and pane dimensions become $state.raw so the extents recompute when the diff or geometry changes, not just on scroll. Keyboard hunk navigation (n/p, first hunk) still anchored every jump on the after pane, which for a trailing pure deletion clamps at the after max and leaves the removed lines just past the before viewport. anchorSideForHunk (diffViewerHelpers) returns 'before' when a hunk has no after-side rows; diffKeyboard routes all hunk jumps through it and DiffViewer's open-file auto-scroll uses the same helper. The comment and search jumps are after-pane row jumps (spans resolve against afterLines), not hunk jumps, and are unchanged. Tests: cross-side handoff (before pane scrolled to its max, after pane wheeled up unwinds the before pane first), scrollTo/scrollToRow snapping away an outstanding handoff, getEffectiveExtent/scrollToEffective for the trailing deletion, trailing addition and leading deletion fixtures, and a unit test for anchorSideForHunk. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…es a trailing deletion Follow-up to 2335c02. n/p (j/k) derived the *current* hunk purely from the after pane's scroll anchor row. When the after pane is pinned at its max (a trailing pure deletion) or cannot scroll at all (file shorter than the viewport), no hunk whose after.start lies in the last ~2/3 of a viewport can ever reach the anchor, so it is never detected as current. Pressing n on such a hunk recomputed currentIndex as the hunk above and jumped straight back to it; a trailing deletion below it was unreachable by keyboard even though wheel and scrollbar now get there. setupDiffKeyboardNav now remembers its last hunk jump together with the after pane scrollY it landed on. While the host still reports that hunk focused (new optional getFocusedHunkIndex callback, which DiffViewer wires to the focusedHunkIndex it already receives via onHunkFocus) and the after pane has not scrolled since, that hunk is the current one and the viewport is known to sit at its start, so n advances to the next hunk and p goes directly to the previous one. Any scroll, a host-side focus clear (click, diff change), or an out-of-range index drops the record and falls back to the scroll-derived index, so mid-file behaviour is unchanged: p still snaps to the start of a hunk you have scrolled into before stepping back. The i (comment) shortcut uses the same resolution. The anchor-row math is factored into one helper instead of being duplicated across next/prev. DiffViewer's open-file auto-scroll drops the isDeletedFile override: git-diff builds every deleted-file hunk with after = (0, 0), both on the no-hunk path and from libgit2's +0,0 hunk, so anchorSideForHunk already answers 'before'. Tests (new diffKeyboard.test.ts, jsdom, driving a real scrollController the way DiffViewer does): the reviewer's case, a 40-line file in a 40-row pane with a modification at line 30 and lines 40-60 deleted. j from the top lands on line 30, j again reaches the deletion with rows 40-60 visible in the before pane, a further j is a no-op, k returns to line 30, i comments on the deletion, and clearing the host focus falls back to the scroll-derived hunk. A 200-line mid-file fixture covers stepping forward and back, re-deriving the current hunk after a wheel scroll (snap-to-start then previous), and k before the first hunk. The three trailing-deletion cases fail on the previous code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ne scrolls alone Follow-up to 2681ae1. The focused-hunk trust record (lastJump) was keyed on the after pane's scrollY only. With the after pane pinned at an edge (file fits the viewport, or a trailing deletion holds it at its max) a wheel scroll is handed off to the before pane and afterScrollY never changes, so the record stayed valid after the viewport had moved: k went straight to the previous hunk instead of snapping back to the one that had scrolled off, and i commented on an off-screen hunk. DiffNavConfig gains an optional getBeforeScrollY getter, wired from DiffViewer the same way getCurrentScrollY is. scrollToHunk records both panes' positions and findCurrentHunk drops the record when either has changed since the jump. Invalidating alone is not enough for k to snap back: the scroll-derived fallback compared every hunk's after.start against the after-pane anchor, which in this state sits at the top of the file and resolves to the first deletion. findCurrentHunkIndexFromScroll now locates each hunk on the pane that has its rows (anchorSideForHunk, already used for the jumps), so a pure deletion is found by the before pane's position, and goToPreviousHunk's snap-to-start check uses the same side. Mid-file both panes are level and the two anchors agree, so behaviour there is unchanged; the loop takes the last hunk reached rather than stopping at the first miss, since at an edge a pure deletion below an unreached after-side hunk can be the one actually in view. Test (diffKeyboard.test.ts): after content fits an 80-row viewport, before has deletions at rows 10 and 100. j, j lands on the second; scrollBy on the after pane moves only the before pane; k then snaps back to the start of the second deletion instead of jumping to the first. Fails on the previous code. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…doff test's j presses real Addresses the two test-quality findings from the review of a0cfbf6. a0cfbf6 moved the k snap-to-start tolerance onto the pane that has the hunk's rows, and described mid-file behaviour as unchanged. For a pure deletion mid-file it is not: the deletion collapses to one boundary on the after side, so scrolling the after pane a single row past it carries the before pane past the whole deletion (after 100 -> 101 maps to before 121 when rows 100-120 are deleted). Measured on the before side that is well over the 2-row tolerance, so k now snaps back to the deletion's start where the after-anchored code went to the previous hunk. That is the intended behaviour (the before pane visibly jumped 20 rows), so a new describe block pins it on a 220/200-line fixture: j, j lands both anchors on row 100, scrollBy('after', 1 line) leaves the after anchor at 101 and the before anchor at 121, k returns to (100, 'before') with the focused hunk unchanged, and a further k goes to the previous hunk. The handoff-invalidation test's "j, j" did not do what it claimed. With an 80-row viewport the initial before anchor row is 26, so a first deletion at row 10 was already current and the first j jumped straight to the second deletion; the second j was a no-op. The fixture's first deletion moves to row 40 (context 0-40, deletion 40-70, context 70-100, deletion 100-160; line counts unchanged) and the test now asserts the first j calls scrollToRow(40, 'before') and the second scrollToRow(100, 'before') before the k assertion. The k regression it guards is unaffected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 2d4ac33251
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…iles get no phantom scroll range Review finding on 2d4ac33 (PR #977): DiffViewer only called setDimensions for a pane that is mounted and has lines. Switching from a two-pane diff to a deleted file (no after pane) or a created file (no before pane) therefore left the absent side holding the previous file's geometry. getEffectiveExtent and scrollBy treat the other pane's remaining range as edge headroom, so the visible pane's scrollbar gained a phantom region the size of the old pane's scroll range, and wheel or thumb input past the end kept scrolling a pane that was not rendered. scrollController gains clearDimensions(side), which resets a side to the empty dimensions the controller starts with (no content, no viewport). With an empty target, transferScroll clamps to 0, so edge headroom is zero and a handoff has nowhere to go. DiffViewer's two dimension effects now clear the side whenever the pane is unmounted or has no lines, instead of leaving it untouched. Two-pane files are unaffected: both panes still measure and set their dimensions as before. The keyboard-nav getLineHeight/getViewportHeight getters always read the after pane; the stale value had been masking that for deleted files. They now read the pane that is rendered. Observable behaviour for a deleted file is unchanged (one hunk starting at row 0), but the anchor row is no longer computed from a zero viewport. Tests (scrollController.test.ts): starting from the trailing-deletion two-pane fixture with an outstanding handoff, switch to a 50-line deleted file (after cleared) and a 50-line created file (before cleared). The visible pane's effective extent equals its own range with no contentOffset, and scrollBy/scrollToEffective past the end pin the visible pane at its max and leave the absent pane at 0. Without the clear both fail with a maxScrollY of 2000/2400 instead of 700. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…TXTBSY and surface early probe failures Staged CI run 38676632489 (PR #977, a frontend-only change) failed cancellation_kills_bridge_and_native_child with only "probe did not spawn its native child: Elapsed(())". The test discards validate_staged_tool's result, so if validation fails immediately the sole symptom is probe_pids timing out after 5 s, and the reason is lost. The most likely cause is the ETXTBSY fork/exec race that 7cad570 fixed for the npm_view_version fixtures: on Linux, a test on another thread that forks while our write handle to a freshly written script is open inherits that fd until its own exec, and exec of a file anyone holds open for writing fails with "Text file busy". The validation tests write their fake node through the production write_shim and exec it straight away, with no guard. write_executable's probe-spawn loop is split out as wait_until_executable. validation.rs gains write_node_shim, which writes the shim through write_shim and then waits until it can be exec'd. Both node shims in the fixture (the staged probe in write_probe and the old runtime in rejected_upgrade) go through it. Each exits right away when run with no arguments, so probe-spawning them is safe. probe_pids now returns an Option and callers say what went wrong. The cancellation test races it against the validation JoinHandle, so an early return panics with the actual result (e.g. `could not start: ...`). The timeout tests include the validation error they already hold. Checked by deleting the node shim temporarily: the test then fails at once with "validation finished before spawning its native child: Ok(Err(Incomplete("... could not start: No such file or directory")))". write_fake_node_with_npm's npm is left unguarded: run with no arguments it would `cp -R` the template into `/`, so it cannot be probe-spawned, and those tests already fail loudly with the real error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
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.
Summary
When a diff's last hunk only deletes lines from the end of a file, the deleted lines couldn't be reached. Scrolling the after (right) pane to its bottom left the before pane's viewport ending exactly where the deletion starts. Wheel, scrollbar, touch, and keyboard navigation were all affected.
Scrolling (
scrollController)scrollBypasses any delta the source pane can't use at an edge on to the other pane. When the scroll direction reverses, it unwinds that handoff first, so neither pane jumps. The same mechanism covers lines appended at the end of a file (from the before pane) and deletions at the start of a file (at the top edge).getEffectiveExtent(side)and position-basedscrollToEffective(side, y), so absolute scrolls carry the handoff too.Scrollbar
onScrollTo. Dragging to the bottom now reaches the deleted tail, and dragging back up unwinds it without the thumb lagging the pointer.Keyboard hunk navigation (
diffKeyboard)anchorSideForHunkhelper: hunk jumps anchor on the before pane when the hunk has no after-side rows. Open-file auto-scroll uses the same helper.jon a hunk near the bottom of a pinned or non-scrollable after pane jumped back to that same hunk, so a trailing deletion could never be reached.k's snap-to-start check also locate each hunk on the pane that has its rows. A pure deletion is now found by the before pane's position.Tests
scrollController.test.ts: trailing deletion, trailing addition and leading deletion handoffs; cross-side unwinding;scrollToandscrollToRowsnapping; effective extent mapping.diffKeyboard.test.ts(jsdom, real scroll controller): reaching a trailing deletion withj,kback,icommenting on the deletion, falling back when focus clears or the before pane scrolls alone, and mid-filekon a pure deletion.anchorSideForHunkunit test.🤖 Generated with Claude Code