From 5606eb1915e30289b2233bbc942cfead72f42a12 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 14 Oct 2026 20:24:46 +1100 Subject: [PATCH 1/7] fix(diff-viewer): keep trailing deleted lines reachable when scrolling 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 Signed-off-by: Matt Toohey --- .../src/lib/components/DiffViewer.svelte | 7 +- .../src/lib/state/scrollController.svelte.ts | 67 +++++++-- .../src/lib/state/scrollController.test.ts | 142 ++++++++++++++++++ 3 files changed, 204 insertions(+), 12 deletions(-) create mode 100644 packages/diff-viewer/src/lib/state/scrollController.test.ts diff --git a/packages/diff-viewer/src/lib/components/DiffViewer.svelte b/packages/diff-viewer/src/lib/components/DiffViewer.svelte index 7adf0d772..958d1f1b2 100644 --- a/packages/diff-viewer/src/lib/components/DiffViewer.svelte +++ b/packages/diff-viewer/src/lib/components/DiffViewer.svelte @@ -473,8 +473,11 @@ // Wait for next frame to ensure dimensions are set requestAnimationFrame(() => { const firstHunk = changedAlignments[0].alignment; - // Scroll to first change in the after pane (or before pane for deleted files) - const side = isDeletedFile ? 'before' : 'after'; + // Scroll to the first change in the after pane, unless it only exists on + // the before side (deleted file or pure deletion) – anchoring on the after + // pane there would leave the removed lines just past the before viewport. + const side = + isDeletedFile || firstHunk.after.end === firstHunk.after.start ? 'before' : 'after'; const startRow = side === 'before' ? firstHunk.before.start : firstHunk.after.start; scrollController.scrollToRow(startRow, side); }); diff --git a/packages/diff-viewer/src/lib/state/scrollController.svelte.ts b/packages/diff-viewer/src/lib/state/scrollController.svelte.ts index 4932fe653..9880ccfa3 100644 --- a/packages/diff-viewer/src/lib/state/scrollController.svelte.ts +++ b/packages/diff-viewer/src/lib/state/scrollController.svelte.ts @@ -108,12 +108,18 @@ export function createScrollController() { let beforeScrollX = $state(0); let afterScrollX = $state(0); + /** + * Maximum vertical scroll position for a pane. + */ + function maxScrollY(dims: PaneDimensions): number { + return Math.max(0, dims.contentHeight - dims.viewportHeight); + } + /** * Clamp a vertical scroll position to valid bounds. */ function clampScroll(scrollY: number, dims: PaneDimensions): number { - const maxScroll = Math.max(0, dims.contentHeight - dims.viewportHeight); - return Math.max(0, Math.min(maxScroll, scrollY)); + return Math.max(0, Math.min(maxScrollY(dims), scrollY)); } /** @@ -181,6 +187,26 @@ export function createScrollController() { return clampScroll(targetY, targetDims); } + /** + * How far the target pane sits past its alignment-derived position because + * scrolling was handed off to it while the source pane was pinned at an edge + * (see scrollBy). Only counts while the source is still at that edge; any + * other discrepancy is alignment rounding and keeps snapping as before. + */ + function edgeOverscroll( + sourceScrollY: number, + sourceSide: 'before' | 'after', + sourceDims: PaneDimensions, + targetScrollY: number, + targetDims: PaneDimensions + ): number { + const aligned = transferScroll(sourceScrollY, sourceSide, sourceDims, targetDims); + const overscroll = targetScrollY - aligned; + if (overscroll > 0 && sourceScrollY >= maxScrollY(sourceDims)) return overscroll; + if (overscroll < 0 && sourceScrollY <= 0) return overscroll; + return 0; + } + return { // Expose reactive state get beforeScrollY() { @@ -234,18 +260,39 @@ export function createScrollController() { /** * Scroll by a delta amount. Updates both panes in sync. + * + * The alignment transfer keeps shared rows level, so content that exists on + * only one side past the last shared row (e.g. lines deleted from the end + * of a file) sits beyond the other pane's scroll range. Once the source + * pane is pinned at an edge, the delta it cannot consume is handed off to + * the other pane so that content stays reachable; reversing direction + * unwinds the handoff first so neither pane jumps. */ scrollBy(side: 'before' | 'after', deltaY: number) { + const sourceDims = side === 'before' ? beforeDims : afterDims; + const targetDims = side === 'before' ? afterDims : beforeDims; + const sourceScrollY = side === 'before' ? beforeScrollY : afterScrollY; + const targetScrollY = side === 'before' ? afterScrollY : beforeScrollY; + + let overscroll = edgeOverscroll(sourceScrollY, side, sourceDims, targetScrollY, targetDims); + let remaining = deltaY; + if (overscroll !== 0 && Math.sign(remaining) === -Math.sign(overscroll)) { + const unwind = Math.sign(remaining) * Math.min(Math.abs(remaining), Math.abs(overscroll)); + overscroll += unwind; + remaining -= unwind; + } + + const newSource = clampScroll(sourceScrollY + remaining, sourceDims); + const residual = sourceScrollY + remaining - newSource; + const aligned = transferScroll(newSource, side, sourceDims, targetDims); + const newTarget = clampScroll(aligned + overscroll + residual, targetDims); + if (side === 'before') { - // Update before, compute after - const newBefore = clampScroll(beforeScrollY + deltaY, beforeDims); - beforeScrollY = newBefore; - afterScrollY = transferScroll(newBefore, 'before', beforeDims, afterDims); + beforeScrollY = newSource; + afterScrollY = newTarget; } else { - // Update after, compute before - const newAfter = clampScroll(afterScrollY + deltaY, afterDims); - afterScrollY = newAfter; - beforeScrollY = transferScroll(newAfter, 'after', afterDims, beforeDims); + afterScrollY = newSource; + beforeScrollY = newTarget; } }, diff --git a/packages/diff-viewer/src/lib/state/scrollController.test.ts b/packages/diff-viewer/src/lib/state/scrollController.test.ts new file mode 100644 index 000000000..142a19e33 --- /dev/null +++ b/packages/diff-viewer/src/lib/state/scrollController.test.ts @@ -0,0 +1,142 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { Alignment } from '../types'; +import { createScrollController, type PaneDimensions } from './scrollController.svelte'; + +const LINE_HEIGHT = 20; +/** 15 rows tall; the 1/3 scroll anchor lands exactly on a row boundary. */ +const VIEWPORT_HEIGHT = 300; + +function dims(lineCount: number): PaneDimensions { + return { + viewportHeight: VIEWPORT_HEIGHT, + contentHeight: lineCount * LINE_HEIGHT, + lineHeight: LINE_HEIGHT, + }; +} + +function maxScroll(lineCount: number): number { + return lineCount * LINE_HEIGHT - VIEWPORT_HEIGHT; +} + +/** Last row index (exclusive) that fits in a pane's viewport at the given offset. */ +function lastVisibleRowEnd(scrollY: number): number { + return Math.floor((scrollY + VIEWPORT_HEIGHT) / LINE_HEIGHT); +} + +function setup(beforeLines: number, afterLines: number, alignments: Alignment[]) { + const controller = createScrollController(); + controller.setDimensions('before', dims(beforeLines)); + controller.setDimensions('after', dims(afterLines)); + controller.setAlignments(alignments, 'file.ts'); + return controller; +} + +describe('scrollController edge handoff', () => { + // Runes are not compiled under vitest; the controller only needs a plain value. + beforeEach(() => { + vi.stubGlobal('$state', (value: unknown) => value); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + describe('final hunk deletes the last 20 of 100 lines', () => { + const BEFORE_LINES = 100; + const AFTER_LINES = 80; + const alignments: Alignment[] = [ + { before: { start: 0, end: 80 }, after: { start: 0, end: 80 }, changed: false }, + { before: { start: 80, end: 100 }, after: { start: 80, end: 80 }, changed: true }, + ]; + + it('leaves the deleted tail just past the before viewport when the after pane bottoms out', () => { + const controller = setup(BEFORE_LINES, AFTER_LINES, alignments); + + controller.scrollBy('after', maxScroll(AFTER_LINES)); + + expect(controller.afterScrollY).toBe(maxScroll(AFTER_LINES)); + // Shared rows stay level, so the before pane ends exactly where the deletion starts. + expect(lastVisibleRowEnd(controller.beforeScrollY)).toBe(80); + }); + + it('hands further after-pane scrolling to the before pane so every deleted line is reachable', () => { + const controller = setup(BEFORE_LINES, AFTER_LINES, alignments); + controller.scrollBy('after', maxScroll(AFTER_LINES)); + + controller.scrollBy('after', 20 * LINE_HEIGHT); + + expect(controller.afterScrollY).toBe(maxScroll(AFTER_LINES)); + expect(controller.beforeScrollY).toBe(maxScroll(BEFORE_LINES)); + expect(lastVisibleRowEnd(controller.beforeScrollY)).toBe(BEFORE_LINES); + }); + + it('unwinds the handoff before the after pane moves back up', () => { + const controller = setup(BEFORE_LINES, AFTER_LINES, alignments); + controller.scrollBy('after', maxScroll(AFTER_LINES)); + controller.scrollBy('after', 20 * LINE_HEIGHT); + + controller.scrollBy('after', -5 * LINE_HEIGHT); + expect(controller.afterScrollY).toBe(maxScroll(AFTER_LINES)); + expect(controller.beforeScrollY).toBe(maxScroll(BEFORE_LINES) - 5 * LINE_HEIGHT); + + // 15 rows unwind the remaining handoff, the other 20 move both panes together. + controller.scrollBy('after', -35 * LINE_HEIGHT); + expect(controller.afterScrollY).toBe(maxScroll(AFTER_LINES) - 20 * LINE_HEIGHT); + expect(controller.beforeScrollY).toBe(controller.afterScrollY); + }); + + it('keeps the panes alignment-synced away from the edges', () => { + const controller = setup(BEFORE_LINES, AFTER_LINES, alignments); + + controller.scrollBy('after', 20 * LINE_HEIGHT); + expect(controller.beforeScrollY).toBe(20 * LINE_HEIGHT); + + controller.scrollBy('after', -5 * LINE_HEIGHT); + expect(controller.beforeScrollY).toBe(15 * LINE_HEIGHT); + }); + }); + + it('mirrors the handoff for lines appended to the end of a file', () => { + const controller = setup(80, 100, [ + { before: { start: 0, end: 80 }, after: { start: 0, end: 80 }, changed: false }, + { before: { start: 80, end: 80 }, after: { start: 80, end: 100 }, changed: true }, + ]); + + controller.scrollBy('before', 5000); + + expect(controller.beforeScrollY).toBe(maxScroll(80)); + expect(controller.afterScrollY).toBe(maxScroll(100)); + }); + + describe('first hunk deletes the first 20 of 100 lines', () => { + const alignments: Alignment[] = [ + { before: { start: 0, end: 20 }, after: { start: 0, end: 0 }, changed: true }, + { before: { start: 20, end: 100 }, after: { start: 0, end: 80 }, changed: false }, + ]; + + it('hands off at the top so the deleted head is reachable after explicit navigation', () => { + const controller = setup(100, 80, alignments); + + // Navigation lands on aligned positions: after row 0 is level with before row 20. + controller.scrollToRow(0, 'after'); + expect(controller.afterScrollY).toBe(0); + expect(controller.beforeScrollY).toBe(20 * LINE_HEIGHT); + + controller.scrollBy('after', -20 * LINE_HEIGHT); + expect(controller.afterScrollY).toBe(0); + expect(controller.beforeScrollY).toBe(0); + }); + + it('brings the before pane into alignment before leaving the top, and the round trip returns there', () => { + const controller = setup(100, 80, alignments); + + controller.scrollBy('after', 25 * LINE_HEIGHT); + expect(controller.afterScrollY).toBe(5 * LINE_HEIGHT); + expect(controller.beforeScrollY).toBe(25 * LINE_HEIGHT); + + controller.scrollBy('after', -25 * LINE_HEIGHT); + expect(controller.afterScrollY).toBe(0); + expect(controller.beforeScrollY).toBe(0); + }); + }); +}); From 2335c0291302e0c75dccce828d3df2ce81913778 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 14 Oct 2026 20:42:55 +1100 Subject: [PATCH 2/7] fix(diff-viewer): drive scrollbars over the edge handoff and anchor hunk navigation on the pane with rows Follow-up to 5606eb19, 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 Signed-off-by: Matt Toohey --- .../src/lib/components/DiffViewer.svelte | 66 ++++++--- .../src/lib/components/Scrollbar.svelte | 45 ++++-- .../src/lib/state/scrollController.svelte.ts | 95 ++++++++++++- .../src/lib/state/scrollController.test.ts | 134 +++++++++++++++++- .../diff-viewer/src/lib/utils/diffKeyboard.ts | 19 ++- .../diffViewerHelpers.anchorSide.test.ts | 23 +++ .../src/lib/utils/diffViewerHelpers.ts | 9 ++ packages/diff-viewer/src/lib/utils/index.ts | 1 + 8 files changed, 348 insertions(+), 44 deletions(-) create mode 100644 packages/diff-viewer/src/lib/utils/diffViewerHelpers.anchorSide.test.ts diff --git a/packages/diff-viewer/src/lib/components/DiffViewer.svelte b/packages/diff-viewer/src/lib/components/DiffViewer.svelte index 958d1f1b2..12b41abe0 100644 --- a/packages/diff-viewer/src/lib/components/DiffViewer.svelte +++ b/packages/diff-viewer/src/lib/components/DiffViewer.svelte @@ -57,6 +57,7 @@ import type { SearchState, FileSearchResult } from '../state/searchState.svelte'; import type { SearchMatch, MatchLocation } from '../utils/diffSearch'; import { + anchorSideForHunk, buildAfterMarkers, buildBeforeMarkers, buildLineToAlignmentMap, @@ -473,13 +474,10 @@ // Wait for next frame to ensure dimensions are set requestAnimationFrame(() => { const firstHunk = changedAlignments[0].alignment; - // Scroll to the first change in the after pane, unless it only exists on - // the before side (deleted file or pure deletion) – anchoring on the after - // pane there would leave the removed lines just past the before viewport. - const side = - isDeletedFile || firstHunk.after.end === firstHunk.after.start ? 'before' : 'after'; - const startRow = side === 'before' ? firstHunk.before.start : firstHunk.after.start; - scrollController.scrollToRow(startRow, side); + // Scroll to the first change, anchored on the pane that has its rows + // (deleted files and pure deletions only exist on the before side). + const side = isDeletedFile ? 'before' : anchorSideForHunk(firstHunk); + scrollController.scrollToRow(firstHunk[side].start, side); }); }); @@ -534,6 +532,12 @@ let beforeContentHeight = $state(0); let afterContentHeight = $state(0); + // Scrollbars run over each pane's effective extent (own range plus the edge + // handoff headroom) so rows that exist on one side alone stay reachable by + // thumb drag and track click, not just by wheel. + let beforeScrollExtent = $derived(scrollController.getEffectiveExtent('before')); + let afterScrollExtent = $derived(scrollController.getEffectiveExtent('after')); + // Reactive line/viewport metrics that drive the rendered window (Phase 2 // virtualization). Kept in sync wherever dimensions are measured below so the // window deriveds recompute on resize and font changes, not just on scroll. @@ -1222,8 +1226,7 @@ handleWheel('after', e); } - function handleBeforeScrollbarScroll(deltaY: number) { - scrollController.scrollBy('before', deltaY); + function syncOverlaysAfterScrollbar() { redrawConnectorsImpl(); updateToolbarPosition(); updateCommentEditorPosition(); @@ -1231,13 +1234,24 @@ updateLineCommentEditorPosition(); } + function handleBeforeScrollbarScroll(deltaY: number) { + scrollController.scrollBy('before', deltaY); + syncOverlaysAfterScrollbar(); + } + + function handleBeforeScrollbarScrollTo(scrollY: number) { + scrollController.scrollToEffective('before', scrollY); + syncOverlaysAfterScrollbar(); + } + function handleAfterScrollbarScroll(deltaY: number) { scrollController.scrollBy('after', deltaY); - redrawConnectorsImpl(); - updateToolbarPosition(); - updateCommentEditorPosition(); - updateLineSelectionToolbar(); - updateLineCommentEditorPosition(); + syncOverlaysAfterScrollbar(); + } + + function handleAfterScrollbarScrollTo(scrollY: number) { + scrollController.scrollToEffective('after', scrollY); + syncOverlaysAfterScrollbar(); } function handleHorizontalScrollbarScroll(deltaX: number) { @@ -2312,11 +2326,13 @@ {:else}
@@ -2409,11 +2425,13 @@
@@ -2577,11 +2595,13 @@ {/if}
void; + /** Callback when the user drags the thumb or clicks the track (absolute target) */ + onScrollTo: (scrollY: number) => void; /** Markers to show (changes, comments) */ markers?: Marker[]; } - let { scrollY, contentHeight, viewportHeight, side, onScroll, markers = [] }: Props = $props(); + let { + scrollY, + contentHeight, + viewportHeight, + markerRange, + side, + onScroll, + onScrollTo, + markers = [], + }: Props = $props(); // ========================================================================== // State @@ -62,6 +79,18 @@ maxScroll > 0 ? (scrollY / maxScroll) * (100 - thumbHeightPercent) : 0 ); + /** Markers mapped from their own range onto the full track */ + let trackMarkers = $derived.by(() => { + if (!markerRange || contentHeight <= 0) return markers; + const scale = markerRange.height / contentHeight; + const offset = (markerRange.top / contentHeight) * 100; + return markers.map((marker) => ({ + ...marker, + top: offset + marker.top * scale, + height: marker.height * scale, + })); + }); + /** Whether scrollbar should be visible (content overflows) */ let canScroll = $derived(contentHeight > viewportHeight); @@ -119,9 +148,7 @@ const deltaY = e.clientY - dragStartY; const scrollDelta = (deltaY / availableTrack) * maxScroll; - const newScrollY = Math.max(0, Math.min(maxScroll, dragStartScrollY + scrollDelta)); - - onScroll(newScrollY - scrollY); + onScrollTo(Math.max(0, Math.min(maxScroll, dragStartScrollY + scrollDelta))); } function handleDragEnd() { @@ -152,9 +179,7 @@ if (availableTrack <= 0) return; const targetScrollY = (targetThumbTop / availableTrack) * maxScroll; - const clampedScrollY = Math.max(0, Math.min(maxScroll, targetScrollY)); - - onScroll(clampedScrollY - scrollY); + onScrollTo(Math.max(0, Math.min(maxScroll, targetScrollY))); } // ========================================================================== @@ -194,7 +219,7 @@
- {#each markers as marker} + {#each trackMarkers as marker}
{ - // Runes are not compiled under vitest; the controller only needs a plain value. + // Runes are not compiled under vitest; the controller only needs plain values. beforeEach(() => { - vi.stubGlobal('$state', (value: unknown) => value); + const identity = (value: unknown) => value; + vi.stubGlobal('$state', Object.assign(identity, { raw: identity })); }); afterEach(() => { @@ -94,6 +95,101 @@ describe('scrollController edge handoff', () => { controller.scrollBy('after', -5 * LINE_HEIGHT); expect(controller.beforeScrollY).toBe(15 * LINE_HEIGHT); }); + + it('unwinds a handoff reached from the before pane before the after pane moves', () => { + const controller = setup(BEFORE_LINES, AFTER_LINES, alignments); + + // Scrolling the before pane to its end clamps the after pane at its own max, + // leaving the same 20-row handoff that wheeling the after pane would create. + controller.scrollBy('before', 5000); + expect(controller.beforeScrollY).toBe(maxScroll(BEFORE_LINES)); + expect(controller.afterScrollY).toBe(maxScroll(AFTER_LINES)); + + controller.scrollBy('after', -5 * LINE_HEIGHT); + expect(controller.afterScrollY).toBe(maxScroll(AFTER_LINES)); + expect(controller.beforeScrollY).toBe(maxScroll(BEFORE_LINES) - 5 * LINE_HEIGHT); + + // 15 more rows finish unwinding, the last 5 move both panes together. + controller.scrollBy('after', -20 * LINE_HEIGHT); + expect(controller.afterScrollY).toBe(maxScroll(AFTER_LINES) - 5 * LINE_HEIGHT); + expect(controller.beforeScrollY).toBe(controller.afterScrollY); + }); + + it('snaps away an outstanding handoff on programmatic navigation', () => { + const controller = setup(BEFORE_LINES, AFTER_LINES, alignments); + controller.scrollBy('after', maxScroll(AFTER_LINES) + 20 * LINE_HEIGHT); + expect(controller.beforeScrollY).toBe(maxScroll(BEFORE_LINES)); + + controller.scrollTo('after', maxScroll(AFTER_LINES)); + expect(controller.afterScrollY).toBe(maxScroll(AFTER_LINES)); + expect(controller.beforeScrollY).toBe(maxScroll(AFTER_LINES)); + + controller.scrollBy('after', 20 * LINE_HEIGHT); + controller.scrollToRow(60, 'after'); + expect(controller.afterScrollY).toBe(60 * LINE_HEIGHT - VIEWPORT_HEIGHT / 3); + expect(controller.beforeScrollY).toBe(controller.afterScrollY); + }); + + describe('effective extent (scrollbar range)', () => { + it('extends the after pane by the deleted tail and leaves the before pane alone', () => { + const controller = setup(BEFORE_LINES, AFTER_LINES, alignments); + + expect(controller.getEffectiveExtent('after')).toEqual({ + scrollY: 0, + maxScrollY: maxScroll(BEFORE_LINES), + contentHeight: BEFORE_LINES * LINE_HEIGHT, + contentOffset: 0, + }); + expect(controller.getEffectiveExtent('before')).toEqual({ + scrollY: 0, + maxScrollY: maxScroll(BEFORE_LINES), + contentHeight: BEFORE_LINES * LINE_HEIGHT, + contentOffset: 0, + }); + }); + + it('keeps the position moving through a wheel handoff', () => { + const controller = setup(BEFORE_LINES, AFTER_LINES, alignments); + + controller.scrollBy('after', maxScroll(AFTER_LINES)); + expect(controller.getEffectiveExtent('after').scrollY).toBe(maxScroll(AFTER_LINES)); + + controller.scrollBy('after', 20 * LINE_HEIGHT); + expect(controller.getEffectiveExtent('after').scrollY).toBe(maxScroll(BEFORE_LINES)); + }); + + it('scrollToEffective carries the before pane through the tail with the after pane pinned', () => { + const controller = setup(BEFORE_LINES, AFTER_LINES, alignments); + + controller.scrollToEffective('after', maxScroll(BEFORE_LINES)); + expect(controller.afterScrollY).toBe(maxScroll(AFTER_LINES)); + expect(controller.beforeScrollY).toBe(maxScroll(BEFORE_LINES)); + + // Dragging back up unwinds the handoff; the position reports exactly what was requested. + const partway = maxScroll(AFTER_LINES) + 5 * LINE_HEIGHT; + controller.scrollToEffective('after', partway); + expect(controller.afterScrollY).toBe(maxScroll(AFTER_LINES)); + expect(controller.beforeScrollY).toBe(partway); + expect(controller.getEffectiveExtent('after').scrollY).toBe(partway); + + // Inside the pane's own range it behaves like scrollTo. + controller.scrollToEffective('after', 40 * LINE_HEIGHT); + expect(controller.afterScrollY).toBe(40 * LINE_HEIGHT); + expect(controller.beforeScrollY).toBe(40 * LINE_HEIGHT); + }); + + it('clamps scrollToEffective to the extent', () => { + const controller = setup(BEFORE_LINES, AFTER_LINES, alignments); + + controller.scrollToEffective('after', 99999); + expect(controller.afterScrollY).toBe(maxScroll(AFTER_LINES)); + expect(controller.beforeScrollY).toBe(maxScroll(BEFORE_LINES)); + + controller.scrollToEffective('after', -50); + expect(controller.afterScrollY).toBe(0); + expect(controller.beforeScrollY).toBe(0); + }); + }); }); it('mirrors the handoff for lines appended to the end of a file', () => { @@ -108,6 +204,20 @@ describe('scrollController edge handoff', () => { expect(controller.afterScrollY).toBe(maxScroll(100)); }); + it('mirrors the effective extent for lines appended to the end of a file', () => { + const controller = setup(80, 100, [ + { before: { start: 0, end: 80 }, after: { start: 0, end: 80 }, changed: false }, + { before: { start: 80, end: 80 }, after: { start: 80, end: 100 }, changed: true }, + ]); + + expect(controller.getEffectiveExtent('before').maxScrollY).toBe(maxScroll(100)); + expect(controller.getEffectiveExtent('after').maxScrollY).toBe(maxScroll(100)); + + controller.scrollToEffective('before', maxScroll(100)); + expect(controller.beforeScrollY).toBe(maxScroll(80)); + expect(controller.afterScrollY).toBe(maxScroll(100)); + }); + describe('first hunk deletes the first 20 of 100 lines', () => { const alignments: Alignment[] = [ { before: { start: 0, end: 20 }, after: { start: 0, end: 0 }, changed: true }, @@ -138,5 +248,25 @@ describe('scrollController edge handoff', () => { expect(controller.afterScrollY).toBe(0); expect(controller.beforeScrollY).toBe(0); }); + + it('exposes the deleted head as top headroom of the after pane', () => { + const controller = setup(100, 80, alignments); + + controller.scrollToRow(0, 'after'); + const extent = controller.getEffectiveExtent('after'); + expect(extent.contentOffset).toBe(20 * LINE_HEIGHT); + expect(extent.maxScrollY).toBe(maxScroll(100)); + // Aligned at after row 0 the thumb sits below the headroom, not at the very top. + expect(extent.scrollY).toBe(20 * LINE_HEIGHT); + + controller.scrollToEffective('after', 0); + expect(controller.afterScrollY).toBe(0); + expect(controller.beforeScrollY).toBe(0); + expect(controller.getEffectiveExtent('after').scrollY).toBe(0); + + controller.scrollToEffective('after', 30 * LINE_HEIGHT); + expect(controller.afterScrollY).toBe(10 * LINE_HEIGHT); + expect(controller.beforeScrollY).toBe(30 * LINE_HEIGHT); + }); }); }); diff --git a/packages/diff-viewer/src/lib/utils/diffKeyboard.ts b/packages/diff-viewer/src/lib/utils/diffKeyboard.ts index b256b699c..bacfcd256 100644 --- a/packages/diff-viewer/src/lib/utils/diffKeyboard.ts +++ b/packages/diff-viewer/src/lib/utils/diffKeyboard.ts @@ -15,6 +15,7 @@ */ import type { Alignment } from '../types'; +import { anchorSideForHunk } from './diffViewerHelpers'; export interface DiffNavConfig { scrollAmount: number; // pixels per keypress for smooth scroll @@ -75,6 +76,14 @@ function findCurrentHunkIndex(config: DiffNavConfig): number { return currentIndex; } +/** + * Scroll a hunk into view, anchored on the pane that actually has its rows. + */ +function scrollToHunk(config: DiffNavConfig, hunk: Alignment): void { + const side = anchorSideForHunk(hunk); + config.scrollToRow(hunk[side].start, side); +} + /** * Navigate to the next diff hunk. */ @@ -86,8 +95,7 @@ function goToNextHunk(config: DiffNavConfig): boolean { const nextIndex = currentIndex + 1; if (nextIndex < alignments.length) { - const nextHunk = alignments[nextIndex].alignment; - config.scrollToRow(nextHunk.after.start, 'after'); + scrollToHunk(config, alignments[nextIndex].alignment); config.onHunkFocus?.(nextIndex); return true; } @@ -128,15 +136,14 @@ function goToPreviousHunk(config: DiffNavConfig): boolean { // If we're more than 2 lines into the current hunk, go to its start if (anchorRow > currentHunk.after.start + 2) { - config.scrollToRow(currentHunk.after.start, 'after'); + scrollToHunk(config, currentHunk); config.onHunkFocus?.(currentIndex); return true; } // Otherwise go to previous hunk if (currentIndex > 0) { - const prevHunk = alignments[currentIndex - 1].alignment; - config.scrollToRow(prevHunk.after.start, 'after'); + scrollToHunk(config, alignments[currentIndex - 1].alignment); config.onHunkFocus?.(currentIndex - 1); return true; } @@ -144,7 +151,7 @@ function goToPreviousHunk(config: DiffNavConfig): boolean { // If at or before first hunk, go to first hunk if (alignments.length > 0) { - config.scrollToRow(alignments[0].alignment.after.start, 'after'); + scrollToHunk(config, alignments[0].alignment); config.onHunkFocus?.(0); return true; } diff --git a/packages/diff-viewer/src/lib/utils/diffViewerHelpers.anchorSide.test.ts b/packages/diff-viewer/src/lib/utils/diffViewerHelpers.anchorSide.test.ts new file mode 100644 index 000000000..6a5e90f01 --- /dev/null +++ b/packages/diff-viewer/src/lib/utils/diffViewerHelpers.anchorSide.test.ts @@ -0,0 +1,23 @@ +import { describe, expect, it } from 'vitest'; +import type { Alignment } from '../types'; +import { anchorSideForHunk } from './diffViewerHelpers'; + +function hunk(before: [number, number], after: [number, number]): Alignment { + return { + before: { start: before[0], end: before[1] }, + after: { start: after[0], end: after[1] }, + changed: true, + }; +} + +describe('anchorSideForHunk', () => { + it('anchors a pure deletion on the before pane, where its rows exist', () => { + expect(anchorSideForHunk(hunk([80, 100], [80, 80]))).toBe('before'); + expect(anchorSideForHunk(hunk([0, 20], [0, 0]))).toBe('before'); + }); + + it('anchors modifications and additions on the after pane', () => { + expect(anchorSideForHunk(hunk([10, 12], [10, 15]))).toBe('after'); + expect(anchorSideForHunk(hunk([10, 10], [10, 15]))).toBe('after'); + }); +}); diff --git a/packages/diff-viewer/src/lib/utils/diffViewerHelpers.ts b/packages/diff-viewer/src/lib/utils/diffViewerHelpers.ts index 0c6bf9d35..48aea7bfc 100644 --- a/packages/diff-viewer/src/lib/utils/diffViewerHelpers.ts +++ b/packages/diff-viewer/src/lib/utils/diffViewerHelpers.ts @@ -70,6 +70,15 @@ export function normalizeLineSelection( return { pane: lineSelection.pane, start, end }; } +/** + * Which pane to anchor on when scrolling a hunk into view. A hunk with no + * after-side rows (a pure deletion) only exists in the before pane; anchoring + * on the after pane there would leave the removed lines just past its viewport. + */ +export function anchorSideForHunk(alignment: Alignment): PaneSide { + return alignment.after.end === alignment.after.start ? 'before' : 'after'; +} + export function isLineInChangedAlignment( side: PaneSide, lineIndex: number, diff --git a/packages/diff-viewer/src/lib/utils/index.ts b/packages/diff-viewer/src/lib/utils/index.ts index 1bd53f631..0b49edefa 100644 --- a/packages/diff-viewer/src/lib/utils/index.ts +++ b/packages/diff-viewer/src/lib/utils/index.ts @@ -23,6 +23,7 @@ export { } from './diffModalHelpers'; export { + anchorSideForHunk, buildLineToAlignmentMap, buildBeforeMarkers, buildAfterMarkers, From 2681ae168d9ad6f59a2585a74ab3c3aa1535107c Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 14 Oct 2026 20:56:57 +1100 Subject: [PATCH 3/7] fix(diff-viewer): trust the focused hunk so keyboard navigation reaches a trailing deletion Follow-up to 2335c029. 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 Signed-off-by: Matt Toohey --- .../src/lib/components/DiffViewer.svelte | 5 +- .../src/lib/utils/diffKeyboard.test.ts | 285 ++++++++++++++++++ .../diff-viewer/src/lib/utils/diffKeyboard.ts | 110 ++++--- 3 files changed, 359 insertions(+), 41 deletions(-) create mode 100644 packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts diff --git a/packages/diff-viewer/src/lib/components/DiffViewer.svelte b/packages/diff-viewer/src/lib/components/DiffViewer.svelte index 12b41abe0..7943a5ea1 100644 --- a/packages/diff-viewer/src/lib/components/DiffViewer.svelte +++ b/packages/diff-viewer/src/lib/components/DiffViewer.svelte @@ -475,8 +475,8 @@ requestAnimationFrame(() => { const firstHunk = changedAlignments[0].alignment; // Scroll to the first change, anchored on the pane that has its rows - // (deleted files and pure deletions only exist on the before side). - const side = isDeletedFile ? 'before' : anchorSideForHunk(firstHunk); + // (pure deletions, including whole deleted files, only exist on the before side). + const side = anchorSideForHunk(firstHunk); scrollController.scrollToRow(firstHunk[side].start, side); }); }); @@ -2197,6 +2197,7 @@ onHunkFocus: (hunkIndex) => { focusedHunkIndex = hunkIndex; }, + getFocusedHunkIndex: () => focusedHunkIndex, }); document.addEventListener('copy', handleCopy); diff --git a/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts b/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts new file mode 100644 index 000000000..601b722f3 --- /dev/null +++ b/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts @@ -0,0 +1,285 @@ +// @vitest-environment jsdom +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { createScrollController, type PaneDimensions } from '../state/scrollController.svelte'; +import type { Alignment } from '../types'; +import { setupDiffKeyboardNav } from './diffKeyboard'; + +const LINE_HEIGHT = 20; + +function dims(lineCount: number, viewportRows: number): PaneDimensions { + return { + viewportHeight: viewportRows * LINE_HEIGHT, + contentHeight: lineCount * LINE_HEIGHT, + lineHeight: LINE_HEIGHT, + }; +} + +function hunk(before: [number, number], after: [number, number], changed = true): Alignment { + return { + before: { start: before[0], end: before[1] }, + after: { start: after[0], end: after[1] }, + changed, + }; +} + +/** Rows visible in a pane at the given scroll offset: [first, lastExclusive). */ +function visibleRows(scrollY: number, viewportRows: number): [number, number] { + const first = Math.floor(scrollY / LINE_HEIGHT); + return [first, first + viewportRows]; +} + +function press(key: string) { + window.dispatchEvent(new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true })); +} + +/** + * Wire keyboard navigation to a real scroll controller the way DiffViewer does, + * including the host's focusedHunkIndex round-trip through onHunkFocus. + */ +function setup(opts: { + beforeLines: number; + afterLines: number; + viewportRows: number; + alignments: Alignment[]; +}) { + const controller = createScrollController(); + controller.setDimensions('before', dims(opts.beforeLines, opts.viewportRows)); + controller.setDimensions('after', dims(opts.afterLines, opts.viewportRows)); + controller.setAlignments(opts.alignments, 'file.ts'); + + const changed = opts.alignments + .map((alignment, index) => ({ alignment, index })) + .filter(({ alignment }) => alignment.changed); + + const host = { focusedHunkIndex: null as number | null }; + const scrollToRow = vi.fn((row: number, side: 'before' | 'after') => + controller.scrollToRow(row, side) + ); + const startCommentOnHunk = vi.fn(); + + const cleanup = setupDiffKeyboardNav({ + getChangedAlignments: () => changed, + scrollToRow, + scrollBy: (deltaY) => controller.scrollBy('after', deltaY), + getCurrentScrollY: () => controller.afterScrollY, + getLineHeight: () => controller.getDimensions('after').lineHeight, + getViewportHeight: () => controller.getDimensions('after').viewportHeight, + startCommentOnHunk, + onHunkFocus: (index) => { + host.focusedHunkIndex = index; + }, + getFocusedHunkIndex: () => host.focusedHunkIndex, + }); + + return { controller, host, scrollToRow, startCommentOnHunk, cleanup }; +} + +describe('diffKeyboard hunk navigation', () => { + let cleanup: (() => void) | null = null; + + // Runes are not compiled under vitest; the controller only needs plain values. + beforeEach(() => { + const identity = (value: unknown) => value; + vi.stubGlobal('$state', Object.assign(identity, { raw: identity })); + }); + + afterEach(() => { + cleanup?.(); + cleanup = null; + vi.unstubAllGlobals(); + }); + + describe('40-line file in a 40-row pane: modification at line 30, lines 40-60 deleted', () => { + const VIEWPORT_ROWS = 40; + const BEFORE_LINES = 60; + const AFTER_LINES = 40; + const alignments: Alignment[] = [ + hunk([0, 30], [0, 30], false), + hunk([30, 31], [30, 31]), + hunk([31, 40], [31, 40], false), + hunk([40, 60], [40, 40]), + ]; + + function setupTrailingDeletion() { + const result = setup({ + beforeLines: BEFORE_LINES, + afterLines: AFTER_LINES, + viewportRows: VIEWPORT_ROWS, + alignments, + }); + cleanup = result.cleanup; + return result; + } + + it('j from the top lands on the modification, which the after pane cannot scroll to', () => { + const { controller, host, scrollToRow } = setupTrailingDeletion(); + + press('j'); + + expect(scrollToRow).toHaveBeenLastCalledWith(30, 'after'); + expect(host.focusedHunkIndex).toBe(0); + // The whole after file fits, so the pane is pinned at 0 and the anchor + // row never reaches line 30. + expect(controller.afterScrollY).toBe(0); + }); + + it('j again advances to the trailing deletion with the deleted rows in the before pane', () => { + const { controller, host, scrollToRow } = setupTrailingDeletion(); + press('j'); + + press('j'); + + expect(scrollToRow).toHaveBeenLastCalledWith(40, 'before'); + expect(host.focusedHunkIndex).toBe(1); + const [first, last] = visibleRows(controller.beforeScrollY, VIEWPORT_ROWS); + expect(first).toBeLessThanOrEqual(40); + expect(last).toBe(BEFORE_LINES); + // The after pane has nothing below alignment to show; it stays put. + expect(controller.afterScrollY).toBe(0); + }); + + it('j on the trailing deletion has nowhere to go and leaves the view alone', () => { + const { controller, host, scrollToRow } = setupTrailingDeletion(); + press('j'); + press('j'); + const beforeScrollY = controller.beforeScrollY; + scrollToRow.mockClear(); + + press('j'); + + expect(scrollToRow).not.toHaveBeenCalled(); + expect(host.focusedHunkIndex).toBe(1); + expect(controller.beforeScrollY).toBe(beforeScrollY); + }); + + it('k from the trailing deletion returns to the modification', () => { + const { controller, host, scrollToRow } = setupTrailingDeletion(); + press('j'); + press('j'); + + press('k'); + + expect(scrollToRow).toHaveBeenLastCalledWith(30, 'after'); + expect(host.focusedHunkIndex).toBe(0); + expect(controller.beforeScrollY).toBe(0); + expect(controller.afterScrollY).toBe(0); + }); + + it('falls back to the scroll position once the host clears the focused hunk', () => { + const { host, scrollToRow } = setupTrailingDeletion(); + press('j'); + host.focusedHunkIndex = null; // e.g. the user clicked elsewhere + + press('j'); + + // With the after pane pinned at 0 the scroll anchor sits above line 30, + // so without a trusted focus the first hunk is the next one again. + expect(scrollToRow).toHaveBeenLastCalledWith(30, 'after'); + expect(host.focusedHunkIndex).toBe(0); + }); + + it('i comments on the focused trailing deletion', () => { + const { startCommentOnHunk } = setupTrailingDeletion(); + press('j'); + press('j'); + + press('i'); + + expect(startCommentOnHunk).toHaveBeenCalledWith(1); + }); + }); + + describe('mid-file hunks in a scrollable 200-line file', () => { + const VIEWPORT_ROWS = 15; + const LINES = 200; + const alignments: Alignment[] = [ + hunk([0, 50], [0, 50], false), + hunk([50, 51], [50, 51]), + hunk([51, 100], [51, 100], false), + hunk([100, 101], [100, 101]), + hunk([101, 150], [101, 150], false), + hunk([150, 151], [150, 151]), + hunk([151, 200], [151, 200], false), + ]; + + /** Scroll offset that puts `row` at the 1/3 anchor, as scrollToRow does. */ + function anchoredScrollY(row: number): number { + return row * LINE_HEIGHT - (VIEWPORT_ROWS * LINE_HEIGHT) / 3; + } + + function setupMidFile() { + const result = setup({ + beforeLines: LINES, + afterLines: LINES, + viewportRows: VIEWPORT_ROWS, + alignments, + }); + cleanup = result.cleanup; + return result; + } + + it('j steps through each hunk in order and k steps back', () => { + const { controller, host } = setupMidFile(); + + press('j'); + expect(controller.afterScrollY).toBe(anchoredScrollY(50)); + expect(host.focusedHunkIndex).toBe(0); + + press('j'); + expect(controller.afterScrollY).toBe(anchoredScrollY(100)); + expect(host.focusedHunkIndex).toBe(1); + + press('j'); + expect(controller.afterScrollY).toBe(anchoredScrollY(150)); + expect(host.focusedHunkIndex).toBe(2); + + press('k'); + expect(controller.afterScrollY).toBe(anchoredScrollY(100)); + expect(host.focusedHunkIndex).toBe(1); + + // Both panes stay level mid-file. + expect(controller.beforeScrollY).toBe(controller.afterScrollY); + }); + + it('re-derives the current hunk from the scroll position after the user scrolls', () => { + const { controller, host, scrollToRow } = setupMidFile(); + press('j'); + press('j'); + expect(host.focusedHunkIndex).toBe(1); + + // Wheel 10 rows down past the second hunk; focus is stale now. + controller.scrollBy('after', 10 * LINE_HEIGHT); + + // k first snaps back to the start of the hunk we are inside of... + press('k'); + expect(scrollToRow).toHaveBeenLastCalledWith(100, 'after'); + expect(controller.afterScrollY).toBe(anchoredScrollY(100)); + + // ...and k again goes to the previous hunk. + press('k'); + expect(scrollToRow).toHaveBeenLastCalledWith(50, 'after'); + expect(host.focusedHunkIndex).toBe(0); + }); + + it('j after a manual scroll advances from the hunk under the anchor', () => { + const { controller, scrollToRow } = setupMidFile(); + press('j'); + press('j'); + controller.scrollBy('after', 10 * LINE_HEIGHT); + + press('j'); + + expect(scrollToRow).toHaveBeenLastCalledWith(150, 'after'); + expect(controller.afterScrollY).toBe(anchoredScrollY(150)); + }); + + it('k before any hunk jumps to the first one', () => { + const { controller, host } = setupMidFile(); + + press('k'); + + expect(controller.afterScrollY).toBe(anchoredScrollY(50)); + expect(host.focusedHunkIndex).toBe(0); + }); + }); +}); diff --git a/packages/diff-viewer/src/lib/utils/diffKeyboard.ts b/packages/diff-viewer/src/lib/utils/diffKeyboard.ts index bacfcd256..4c3c8218f 100644 --- a/packages/diff-viewer/src/lib/utils/diffKeyboard.ts +++ b/packages/diff-viewer/src/lib/utils/diffKeyboard.ts @@ -28,6 +28,12 @@ export interface DiffNavConfig { startCommentOnHunk: (hunkIndex: number) => void; /** Called when keyboard navigation focuses a hunk */ onHunkFocus?: (hunkIndex: number | null) => void; + /** + * Hunk the host currently shows as focused (fed back from onHunkFocus), or + * null once the user clicked away or the diff changed. Lets navigation trust + * its last jump when the scroll position alone cannot identify the hunk. + */ + getFocusedHunkIndex?: () => number | null; /** Search callbacks */ onOpenSearch?: () => void; onNextSearchResult?: () => void; @@ -46,27 +52,35 @@ const DEFAULT_CONFIG: DiffNavConfig = { onHunkFocus: () => {}, }; +interface NavState { + /** Last keyboard hunk jump and the after-pane scroll position it landed on. */ + lastJump: { index: number; scrollY: number } | null; +} + +/** + * Row at the scroll anchor (1/3 down the after pane's viewport). + */ +function anchorRow(config: DiffNavConfig): number { + const anchorY = config.getCurrentScrollY() + config.getViewportHeight() / 3; + return Math.floor(anchorY / config.getLineHeight()); +} + /** * Find the index of the current hunk based on scroll position. * Returns the index of the hunk that's currently visible (or just passed). */ -function findCurrentHunkIndex(config: DiffNavConfig): number { +function findCurrentHunkIndexFromScroll(config: DiffNavConfig): number { const alignments = config.getChangedAlignments(); if (alignments.length === 0) return -1; - const scrollY = config.getCurrentScrollY(); - const lineHeight = config.getLineHeight(); - const viewportHeight = config.getViewportHeight(); - // Consider a hunk "current" if its start is within the top third of the viewport - const anchorY = scrollY + viewportHeight / 3; - const anchorRow = Math.floor(anchorY / lineHeight); + const anchor = anchorRow(config); // Find the last hunk whose start is at or before the anchor let currentIndex = -1; for (let i = 0; i < alignments.length; i++) { const hunkStart = alignments[i].alignment.after.start; - if (hunkStart <= anchorRow) { + if (hunkStart <= anchor) { currentIndex = i; } else { break; @@ -77,26 +91,55 @@ function findCurrentHunkIndex(config: DiffNavConfig): number { } /** - * Scroll a hunk into view, anchored on the pane that actually has its rows. + * Resolve the hunk keyboard navigation should treat as current. + * + * The scroll-derived answer breaks down whenever the after pane cannot move far + * enough to bring a hunk up to the anchor: pinned at its max by a trailing + * deletion, or a file shorter than the viewport. Any hunk in that last stretch + * is never "reached", so n/p would keep re-selecting the hunk above it. The + * last keyboard jump is authoritative there, as long as the host still reports + * it focused and the after pane has not scrolled since; that jump also means + * the viewport sits at the hunk's start. + */ +function findCurrentHunk( + config: DiffNavConfig, + state: NavState +): { index: number; atStart: boolean } { + const { lastJump } = state; + if (lastJump) { + const focused = config.getFocusedHunkIndex ? config.getFocusedHunkIndex() : lastJump.index; + const inRange = lastJump.index < config.getChangedAlignments().length; + if (focused === lastJump.index && inRange && config.getCurrentScrollY() === lastJump.scrollY) { + return { index: lastJump.index, atStart: true }; + } + state.lastJump = null; + } + return { index: findCurrentHunkIndexFromScroll(config), atStart: false }; +} + +/** + * Scroll a hunk into view, anchored on the pane that actually has its rows, + * and remember where that left the after pane. */ -function scrollToHunk(config: DiffNavConfig, hunk: Alignment): void { +function scrollToHunk(config: DiffNavConfig, state: NavState, index: number): void { + const hunk = config.getChangedAlignments()[index].alignment; const side = anchorSideForHunk(hunk); config.scrollToRow(hunk[side].start, side); + state.lastJump = { index, scrollY: config.getCurrentScrollY() }; + config.onHunkFocus?.(index); } /** * Navigate to the next diff hunk. */ -function goToNextHunk(config: DiffNavConfig): boolean { +function goToNextHunk(config: DiffNavConfig, state: NavState): boolean { const alignments = config.getChangedAlignments(); if (alignments.length === 0) return false; - const currentIndex = findCurrentHunkIndex(config); - const nextIndex = currentIndex + 1; + const nextIndex = findCurrentHunk(config, state).index + 1; if (nextIndex < alignments.length) { - scrollToHunk(config, alignments[nextIndex].alignment); - config.onHunkFocus?.(nextIndex); + scrollToHunk(config, state, nextIndex); return true; } @@ -106,8 +149,8 @@ function goToNextHunk(config: DiffNavConfig): boolean { /** * Start a comment on the current hunk. */ -function commentOnCurrentHunk(config: DiffNavConfig): boolean { - const currentIndex = findCurrentHunkIndex(config); +function commentOnCurrentHunk(config: DiffNavConfig, state: NavState): boolean { + const currentIndex = findCurrentHunk(config, state).index; if (currentIndex >= 0) { config.startCommentOnHunk(currentIndex); return true; @@ -118,45 +161,33 @@ function commentOnCurrentHunk(config: DiffNavConfig): boolean { /** * Navigate to the previous diff hunk. */ -function goToPreviousHunk(config: DiffNavConfig): boolean { +function goToPreviousHunk(config: DiffNavConfig, state: NavState): boolean { const alignments = config.getChangedAlignments(); if (alignments.length === 0) return false; - const currentIndex = findCurrentHunkIndex(config); + const { index: currentIndex, atStart } = findCurrentHunk(config, state); // If we're past the first hunk, go to current or previous // We need to check if we're at the very start of the current hunk if (currentIndex >= 0) { const currentHunk = alignments[currentIndex].alignment; - const scrollY = config.getCurrentScrollY(); - const lineHeight = config.getLineHeight(); - const viewportHeight = config.getViewportHeight(); - const anchorY = scrollY + viewportHeight / 3; - const anchorRow = Math.floor(anchorY / lineHeight); // If we're more than 2 lines into the current hunk, go to its start - if (anchorRow > currentHunk.after.start + 2) { - scrollToHunk(config, currentHunk); - config.onHunkFocus?.(currentIndex); + if (!atStart && anchorRow(config) > currentHunk.after.start + 2) { + scrollToHunk(config, state, currentIndex); return true; } // Otherwise go to previous hunk if (currentIndex > 0) { - scrollToHunk(config, alignments[currentIndex - 1].alignment); - config.onHunkFocus?.(currentIndex - 1); + scrollToHunk(config, state, currentIndex - 1); return true; } } // If at or before first hunk, go to first hunk - if (alignments.length > 0) { - scrollToHunk(config, alignments[0].alignment); - config.onHunkFocus?.(0); - return true; - } - - return false; + scrollToHunk(config, state, 0); + return true; } /** @@ -165,6 +196,7 @@ function goToPreviousHunk(config: DiffNavConfig): boolean { */ export function setupDiffKeyboardNav(config: Partial = {}): () => void { const cfg = { ...DEFAULT_CONFIG, ...config }; + const state: NavState = { lastJump: null }; function handleKeydown(event: KeyboardEvent): void { const target = event.target as HTMLElement; @@ -206,13 +238,13 @@ export function setupDiffKeyboardNav(config: Partial = {}): () => // J or ArrowDown — next hunk (no modifiers) if ((key === 'j' || key === 'arrowdown') && !ctrl && !meta && !shift && !alt) { - if (goToNextHunk(cfg)) event.preventDefault(); + if (goToNextHunk(cfg, state)) event.preventDefault(); return; } // K or ArrowUp — previous hunk (no modifiers) if ((key === 'k' || key === 'arrowup') && !ctrl && !meta && !shift && !alt) { - if (goToPreviousHunk(cfg)) event.preventDefault(); + if (goToPreviousHunk(cfg, state)) event.preventDefault(); return; } @@ -232,7 +264,7 @@ export function setupDiffKeyboardNav(config: Partial = {}): () => // I — add comment on current hunk (no modifiers) if (key === 'i' && !ctrl && !meta && !shift && !alt) { - if (commentOnCurrentHunk(cfg)) event.preventDefault(); + if (commentOnCurrentHunk(cfg, state)) event.preventDefault(); return; } } From a0cfbf621321d9e2bbf74bf85e79013248dac4b7 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 14 Oct 2026 21:07:17 +1100 Subject: [PATCH 4/7] fix(diff-viewer): invalidate the trusted hunk jump when the before pane scrolls alone Follow-up to 2681ae16. 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 Signed-off-by: Matt Toohey --- .../src/lib/components/DiffViewer.svelte | 1 + .../src/lib/utils/diffKeyboard.test.ts | 45 ++++++++++++++ .../diff-viewer/src/lib/utils/diffKeyboard.ts | 58 +++++++++++++------ 3 files changed, 85 insertions(+), 19 deletions(-) diff --git a/packages/diff-viewer/src/lib/components/DiffViewer.svelte b/packages/diff-viewer/src/lib/components/DiffViewer.svelte index 7943a5ea1..40d13e287 100644 --- a/packages/diff-viewer/src/lib/components/DiffViewer.svelte +++ b/packages/diff-viewer/src/lib/components/DiffViewer.svelte @@ -2186,6 +2186,7 @@ scrollToRow: (row, side) => scrollController.scrollToRow(row, side), scrollBy: (deltaY) => scrollController.scrollBy('after', deltaY), getCurrentScrollY: () => scrollController.afterScrollY, + getBeforeScrollY: () => scrollController.beforeScrollY, getLineHeight: () => scrollController.getDimensions('after').lineHeight, getViewportHeight: () => scrollController.getDimensions('after').viewportHeight, startCommentOnHunk: (hunkIndex) => { diff --git a/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts b/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts index 601b722f3..ac5ad8fb1 100644 --- a/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts +++ b/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts @@ -62,6 +62,7 @@ function setup(opts: { scrollToRow, scrollBy: (deltaY) => controller.scrollBy('after', deltaY), getCurrentScrollY: () => controller.afterScrollY, + getBeforeScrollY: () => controller.beforeScrollY, getLineHeight: () => controller.getDimensions('after').lineHeight, getViewportHeight: () => controller.getDimensions('after').viewportHeight, startCommentOnHunk, @@ -189,6 +190,50 @@ describe('diffKeyboard hunk navigation', () => { }); }); + describe('after content fits the viewport: before has deletions at rows 10 and 100', () => { + const VIEWPORT_ROWS = 80; + const BEFORE_LINES = 160; + const AFTER_LINES = 70; + const alignments: Alignment[] = [ + hunk([0, 10], [0, 10], false), + hunk([10, 40], [10, 10]), + hunk([40, 100], [10, 70], false), + hunk([100, 160], [70, 70]), + ]; + + it('k after a handoff scroll that moved only the before pane snaps back to the second deletion', () => { + const result = setup({ + beforeLines: BEFORE_LINES, + afterLines: AFTER_LINES, + viewportRows: VIEWPORT_ROWS, + alignments, + }); + cleanup = result.cleanup; + const { controller, host, scrollToRow } = result; + press('j'); + press('j'); + expect(scrollToRow).toHaveBeenLastCalledWith(100, 'before'); + expect(host.focusedHunkIndex).toBe(1); + const landedBeforeScrollY = controller.beforeScrollY; + expect(landedBeforeScrollY).toBeGreaterThan(0); + expect(controller.afterScrollY).toBe(0); + + // Wheel the after pane down: it is pinned at 0 (the file fits), so the + // edge handoff moves only the before pane and afterScrollY stays put. + controller.scrollBy('after', 5 * LINE_HEIGHT); + expect(controller.afterScrollY).toBe(0); + expect(controller.beforeScrollY).toBe(landedBeforeScrollY + 5 * LINE_HEIGHT); + + press('k'); + + // The viewport moved, so the jump is stale: k snaps back to the start of + // the deletion we are inside of instead of jumping to the first one. + expect(scrollToRow).toHaveBeenLastCalledWith(100, 'before'); + expect(host.focusedHunkIndex).toBe(1); + expect(controller.beforeScrollY).toBe(landedBeforeScrollY); + }); + }); + describe('mid-file hunks in a scrollable 200-line file', () => { const VIEWPORT_ROWS = 15; const LINES = 200; diff --git a/packages/diff-viewer/src/lib/utils/diffKeyboard.ts b/packages/diff-viewer/src/lib/utils/diffKeyboard.ts index 4c3c8218f..169e42def 100644 --- a/packages/diff-viewer/src/lib/utils/diffKeyboard.ts +++ b/packages/diff-viewer/src/lib/utils/diffKeyboard.ts @@ -23,6 +23,12 @@ export interface DiffNavConfig { scrollToRow: (row: number, side: 'before' | 'after') => void; scrollBy: (deltaY: number) => void; getCurrentScrollY: () => number; + /** + * Before-pane scroll position. With the after pane pinned at an edge, a + * scroll can move only the before pane (the edge handoff), so this is needed + * to tell that the viewport has moved and to locate pure deletions. + */ + getBeforeScrollY?: () => number; getLineHeight: () => number; getViewportHeight: () => number; startCommentOnHunk: (hunkIndex: number) => void; @@ -53,15 +59,21 @@ const DEFAULT_CONFIG: DiffNavConfig = { }; interface NavState { - /** Last keyboard hunk jump and the after-pane scroll position it landed on. */ - lastJump: { index: number; scrollY: number } | null; + /** Last keyboard hunk jump and the scroll position of each pane it landed on. */ + lastJump: { index: number; scrollY: number; beforeScrollY: number } | null; +} + +/** Scroll position of a pane; the before pane falls back to the after pane's. */ +function scrollYFor(config: DiffNavConfig, side: 'before' | 'after'): number { + if (side === 'before' && config.getBeforeScrollY) return config.getBeforeScrollY(); + return config.getCurrentScrollY(); } /** - * Row at the scroll anchor (1/3 down the after pane's viewport). + * Row at the scroll anchor (1/3 down the pane's viewport). */ -function anchorRow(config: DiffNavConfig): number { - const anchorY = config.getCurrentScrollY() + config.getViewportHeight() / 3; +function anchorRow(config: DiffNavConfig, side: 'before' | 'after' = 'after'): number { + const anchorY = scrollYFor(config, side) + config.getViewportHeight() / 3; return Math.floor(anchorY / config.getLineHeight()); } @@ -73,17 +85,17 @@ function findCurrentHunkIndexFromScroll(config: DiffNavConfig): number { const alignments = config.getChangedAlignments(); if (alignments.length === 0) return -1; - // Consider a hunk "current" if its start is within the top third of the viewport - const anchor = anchorRow(config); - - // Find the last hunk whose start is at or before the anchor + // Consider a hunk "current" if its start is within the top third of the + // viewport. Each hunk is located on the pane that has its rows, so a pure + // deletion is found by the before pane even when the after pane is pinned at + // an edge and the edge handoff has moved only the before pane. Mid-file both + // panes are level and the two anchors agree; take the last hunk reached. let currentIndex = -1; for (let i = 0; i < alignments.length; i++) { - const hunkStart = alignments[i].alignment.after.start; - if (hunkStart <= anchor) { + const hunk = alignments[i].alignment; + const side = anchorSideForHunk(hunk); + if (hunk[side].start <= anchorRow(config, side)) { currentIndex = i; - } else { - break; } } @@ -98,8 +110,8 @@ function findCurrentHunkIndexFromScroll(config: DiffNavConfig): number { * deletion, or a file shorter than the viewport. Any hunk in that last stretch * is never "reached", so n/p would keep re-selecting the hunk above it. The * last keyboard jump is authoritative there, as long as the host still reports - * it focused and the after pane has not scrolled since; that jump also means - * the viewport sits at the hunk's start. + * it focused and neither pane has scrolled since; that jump also means the + * viewport sits at the hunk's start. */ function findCurrentHunk( config: DiffNavConfig, @@ -109,7 +121,10 @@ function findCurrentHunk( if (lastJump) { const focused = config.getFocusedHunkIndex ? config.getFocusedHunkIndex() : lastJump.index; const inRange = lastJump.index < config.getChangedAlignments().length; - if (focused === lastJump.index && inRange && config.getCurrentScrollY() === lastJump.scrollY) { + const unscrolled = + config.getCurrentScrollY() === lastJump.scrollY && + scrollYFor(config, 'before') === lastJump.beforeScrollY; + if (focused === lastJump.index && inRange && unscrolled) { return { index: lastJump.index, atStart: true }; } state.lastJump = null; @@ -119,13 +134,17 @@ function findCurrentHunk( /** * Scroll a hunk into view, anchored on the pane that actually has its rows, - * and remember where that left the after pane. + * and remember where that left both panes. */ function scrollToHunk(config: DiffNavConfig, state: NavState, index: number): void { const hunk = config.getChangedAlignments()[index].alignment; const side = anchorSideForHunk(hunk); config.scrollToRow(hunk[side].start, side); - state.lastJump = { index, scrollY: config.getCurrentScrollY() }; + state.lastJump = { + index, + scrollY: config.getCurrentScrollY(), + beforeScrollY: scrollYFor(config, 'before'), + }; config.onHunkFocus?.(index); } @@ -173,7 +192,8 @@ function goToPreviousHunk(config: DiffNavConfig, state: NavState): boolean { const currentHunk = alignments[currentIndex].alignment; // If we're more than 2 lines into the current hunk, go to its start - if (!atStart && anchorRow(config) > currentHunk.after.start + 2) { + const side = anchorSideForHunk(currentHunk); + if (!atStart && anchorRow(config, side) > currentHunk[side].start + 2) { scrollToHunk(config, state, currentIndex); return true; } From 2d4ac33251964e7f896849e2d16954da7c562b5b Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 14 Oct 2026 21:35:45 +1100 Subject: [PATCH 5/7] test(diff-viewer): pin mid-file k on a pure deletion and make the handoff test's j presses real Addresses the two test-quality findings from the review of a0cfbf62. a0cfbf62 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 Signed-off-by: Matt Toohey --- .../src/lib/utils/diffKeyboard.test.ts | 95 ++++++++++++++++++- 1 file changed, 91 insertions(+), 4 deletions(-) diff --git a/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts b/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts index ac5ad8fb1..29111b3ac 100644 --- a/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts +++ b/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts @@ -190,14 +190,17 @@ describe('diffKeyboard hunk navigation', () => { }); }); - describe('after content fits the viewport: before has deletions at rows 10 and 100', () => { + describe('after content fits the viewport: before has deletions at rows 40 and 100', () => { const VIEWPORT_ROWS = 80; const BEFORE_LINES = 160; const AFTER_LINES = 70; + // The first deletion sits below the initial before-pane anchor row + // (VIEWPORT_ROWS / 3 = 26) so that it is not already "current" from the + // top and the first j is a real jump to it rather than past it. const alignments: Alignment[] = [ - hunk([0, 10], [0, 10], false), - hunk([10, 40], [10, 10]), - hunk([40, 100], [10, 70], false), + hunk([0, 40], [0, 40], false), + hunk([40, 70], [40, 40]), + hunk([70, 100], [40, 70], false), hunk([100, 160], [70, 70]), ]; @@ -210,8 +213,14 @@ describe('diffKeyboard hunk navigation', () => { }); cleanup = result.cleanup; const { controller, host, scrollToRow } = result; + press('j'); + expect(scrollToRow).toHaveBeenCalledTimes(1); + expect(scrollToRow).toHaveBeenLastCalledWith(40, 'before'); + expect(host.focusedHunkIndex).toBe(0); + press('j'); + expect(scrollToRow).toHaveBeenCalledTimes(2); expect(scrollToRow).toHaveBeenLastCalledWith(100, 'before'); expect(host.focusedHunkIndex).toBe(1); const landedBeforeScrollY = controller.beforeScrollY; @@ -234,6 +243,84 @@ describe('diffKeyboard hunk navigation', () => { }); }); + describe('mid-file pure deletion: 220/200-line file, before rows 100-120 deleted at after row 100', () => { + const VIEWPORT_ROWS = 15; + const BEFORE_LINES = 220; + const AFTER_LINES = 200; + const alignments: Alignment[] = [ + hunk([0, 50], [0, 50], false), + hunk([50, 51], [50, 51]), + hunk([51, 100], [51, 100], false), + hunk([100, 120], [100, 100]), + hunk([120, 220], [100, 200], false), + ]; + + /** Scroll offset that puts `row` at the 1/3 anchor, as scrollToRow does. */ + function anchoredScrollY(row: number): number { + return row * LINE_HEIGHT - (VIEWPORT_ROWS * LINE_HEIGHT) / 3; + } + + function setupMidFileDeletion() { + const result = setup({ + beforeLines: BEFORE_LINES, + afterLines: AFTER_LINES, + viewportRows: VIEWPORT_ROWS, + alignments, + }); + cleanup = result.cleanup; + return result; + } + + // Deliberate semantic choice: the snap-to-start tolerance for k is measured + // on the pane that has the hunk's rows. A pure deletion collapses to a + // single boundary on the after side, so scrolling the after pane one row + // past it carries the before pane past the whole deletion. k then snaps + // back to the deletion's start (the before pane visibly jumped 20 rows) + // instead of treating a one-row move as "already at the start" and going + // to the previous hunk, which is what anchoring on the after pane did. + it('k after scrolling the after pane one row past the deletion snaps back to its start on the before pane', () => { + const { controller, host, scrollToRow } = setupMidFileDeletion(); + press('j'); + press('j'); + expect(scrollToRow).toHaveBeenLastCalledWith(100, 'before'); + expect(host.focusedHunkIndex).toBe(1); + // Landed with both anchors on row 100: the deletion start on the before + // side, its collapsed boundary on the after side. + expect(controller.beforeScrollY).toBe(anchoredScrollY(100)); + expect(controller.afterScrollY).toBe(anchoredScrollY(100)); + + controller.scrollBy('after', LINE_HEIGHT); + + // One after row (100 -> 101) maps to before row 121: the alignment + // transfer skips the deleted rows 100-120 entirely. + expect(controller.afterScrollY).toBe(anchoredScrollY(101)); + expect(controller.beforeScrollY).toBe(anchoredScrollY(121)); + + press('k'); + + expect(scrollToRow).toHaveBeenLastCalledWith(100, 'before'); + expect(host.focusedHunkIndex).toBe(1); + expect(controller.beforeScrollY).toBe(anchoredScrollY(100)); + expect(controller.afterScrollY).toBe(anchoredScrollY(100)); + }); + + it('k again from the start of the deletion goes to the previous hunk', () => { + const { controller, host, scrollToRow } = setupMidFileDeletion(); + press('j'); + press('j'); + controller.scrollBy('after', LINE_HEIGHT); + press('k'); + expect(host.focusedHunkIndex).toBe(1); + + press('k'); + + expect(scrollToRow).toHaveBeenLastCalledWith(50, 'after'); + expect(host.focusedHunkIndex).toBe(0); + expect(controller.afterScrollY).toBe(anchoredScrollY(50)); + expect(controller.beforeScrollY).toBe(anchoredScrollY(50)); + }); + }); + describe('mid-file hunks in a scrollable 200-line file', () => { const VIEWPORT_ROWS = 15; const LINES = 200; From 9a0d424664d70468ae45bcb05f1297f129a68a7a Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 15 Oct 2026 14:28:29 +1100 Subject: [PATCH 6/7] fix(diff-viewer): clear the absent pane's dimensions so single-pane files get no phantom scroll range Review finding on 2d4ac332 (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 Signed-off-by: Matt Toohey --- .../src/lib/components/DiffViewer.svelte | 15 ++++- .../src/lib/state/scrollController.svelte.ts | 27 +++++---- .../src/lib/state/scrollController.test.ts | 57 +++++++++++++++++++ 3 files changed, 86 insertions(+), 13 deletions(-) diff --git a/packages/diff-viewer/src/lib/components/DiffViewer.svelte b/packages/diff-viewer/src/lib/components/DiffViewer.svelte index 40d13e287..03b73e261 100644 --- a/packages/diff-viewer/src/lib/components/DiffViewer.svelte +++ b/packages/diff-viewer/src/lib/components/DiffViewer.svelte @@ -303,6 +303,8 @@ let isNewFile = $derived(diff !== null && !isEmptyDiff && diff.before === null); let isDeletedFile = $derived(diff !== null && !isEmptyDiff && diff.after === null); let isTwoPaneMode = $derived(!isNewFile && !isDeletedFile); + /** The pane that exists for every file: only a deleted file has no after pane. */ + const renderedPaneSide = (): 'before' | 'after' => (isDeletedFile ? 'before' : 'after'); let isBinary = $derived(diff !== null && isBinaryDiff(diff)); let isImage = $derived(diff !== null && isImageDiff(diff)); @@ -481,7 +483,9 @@ }); }); - // Update dimensions when panes are available or content changes + // Update dimensions when panes are available or content changes. A pane that + // is not rendered (created/deleted files) or has no lines is cleared so the + // other pane's effective extent does not inherit a previous file's geometry. $effect(() => { if (beforePane && beforeLines.length > 0) { const lineHeight = measureLineHeight(beforePane); @@ -495,6 +499,8 @@ viewportWidth: beforePane.clientWidth, contentWidth, }); + } else { + scrollController.clearDimensions('before'); } }); @@ -511,6 +517,8 @@ viewportWidth: afterPane.clientWidth, contentWidth, }); + } else { + scrollController.clearDimensions('after'); } }); @@ -2187,8 +2195,9 @@ scrollBy: (deltaY) => scrollController.scrollBy('after', deltaY), getCurrentScrollY: () => scrollController.afterScrollY, getBeforeScrollY: () => scrollController.beforeScrollY, - getLineHeight: () => scrollController.getDimensions('after').lineHeight, - getViewportHeight: () => scrollController.getDimensions('after').viewportHeight, + // Both panes share a viewport height; read the one that is rendered. + getLineHeight: () => scrollController.getDimensions(renderedPaneSide()).lineHeight, + getViewportHeight: () => scrollController.getDimensions(renderedPaneSide()).viewportHeight, startCommentOnHunk: (hunkIndex) => { if (!commentingEnabled) return; commentingOnRange = hunkIndex; diff --git a/packages/diff-viewer/src/lib/state/scrollController.svelte.ts b/packages/diff-viewer/src/lib/state/scrollController.svelte.ts index 10f6e8afa..38bc387ea 100644 --- a/packages/diff-viewer/src/lib/state/scrollController.svelte.ts +++ b/packages/diff-viewer/src/lib/state/scrollController.svelte.ts @@ -107,6 +107,11 @@ function transferRow(row: number, alignment: Alignment, side: 'before' | 'after' return Math.min(target.start + targetOffset, target.end - 1); } +/** Dimensions of a pane that is not rendered: nothing to scroll, no headroom to hand off. */ +function emptyDimensions(): PaneDimensions { + return { viewportHeight: 0, contentHeight: 0, lineHeight: 20 }; +} + /** * Create a scroll controller for synchronized diff panes. */ @@ -114,16 +119,8 @@ export function createScrollController() { // Internal state. Reactive (raw, replaced wholesale) so derived extents // recompute when the diff or pane geometry changes, not just on scroll. let alignments: Alignment[] = $state.raw([]); - let beforeDims: PaneDimensions = $state.raw({ - viewportHeight: 0, - contentHeight: 0, - lineHeight: 20, - }); - let afterDims: PaneDimensions = $state.raw({ - viewportHeight: 0, - contentHeight: 0, - lineHeight: 20, - }); + let beforeDims: PaneDimensions = $state.raw(emptyDimensions()); + let afterDims: PaneDimensions = $state.raw(emptyDimensions()); // Track current file to detect file changes vs content refreshes let currentFilePath: string | null = null; @@ -297,6 +294,16 @@ export function createScrollController() { } }, + /** + * Forget a pane's dimensions when it is not rendered (created and deleted + * files show a single pane). Stale dimensions from a previous file would + * otherwise give the visible pane phantom edge headroom and let scrollBy + * hand input off to a pane that does not exist. + */ + clearDimensions(side: 'before' | 'after') { + this.setDimensions(side, emptyDimensions()); + }, + /** * Scroll by a delta amount. Updates both panes in sync. * diff --git a/packages/diff-viewer/src/lib/state/scrollController.test.ts b/packages/diff-viewer/src/lib/state/scrollController.test.ts index e163aedf9..f8f59bbd2 100644 --- a/packages/diff-viewer/src/lib/state/scrollController.test.ts +++ b/packages/diff-viewer/src/lib/state/scrollController.test.ts @@ -269,4 +269,61 @@ describe('scrollController edge handoff', () => { expect(controller.beforeScrollY).toBe(30 * LINE_HEIGHT); }); }); + + describe('switching from a two-pane diff to a single-pane file', () => { + // git-diff emits a deleted file as one hunk with an empty after span. + const deletedFile: Alignment[] = [ + { before: { start: 0, end: 50 }, after: { start: 0, end: 0 }, changed: true }, + ]; + const createdFile: Alignment[] = [ + { before: { start: 0, end: 0 }, after: { start: 0, end: 50 }, changed: true }, + ]; + + function twoPaneThenCleared(absent: 'before' | 'after') { + const controller = setup(100, 80, [ + { before: { start: 0, end: 80 }, after: { start: 0, end: 80 }, changed: false }, + { before: { start: 80, end: 100 }, after: { start: 80, end: 80 }, changed: true }, + ]); + // Leave an outstanding handoff behind so the previous file's state is as messy as it gets. + controller.scrollBy('after', maxScroll(80) + 20 * LINE_HEIGHT); + + const present = absent === 'before' ? 'after' : 'before'; + controller.setDimensions(present, dims(50)); + controller.clearDimensions(absent); + controller.setAlignments(absent === 'after' ? deletedFile : createdFile, 'other.ts'); + return controller; + } + + it('gives a deleted file no phantom headroom from the previous after pane', () => { + const controller = twoPaneThenCleared('after'); + + const extent = controller.getEffectiveExtent('before'); + expect(extent.contentOffset).toBe(0); + expect(extent.maxScrollY).toBe(maxScroll(50)); + expect(extent.contentHeight).toBe(50 * LINE_HEIGHT); + + // Past the end nothing is handed to the pane that is not rendered. + controller.scrollBy('before', maxScroll(50) + 20 * LINE_HEIGHT); + expect(controller.beforeScrollY).toBe(maxScroll(50)); + expect(controller.afterScrollY).toBe(0); + expect(controller.getEffectiveExtent('before').scrollY).toBe(maxScroll(50)); + + controller.scrollToEffective('before', maxScroll(50) + 20 * LINE_HEIGHT); + expect(controller.beforeScrollY).toBe(maxScroll(50)); + expect(controller.afterScrollY).toBe(0); + }); + + it('gives a created file no phantom headroom from the previous before pane', () => { + const controller = twoPaneThenCleared('before'); + + const extent = controller.getEffectiveExtent('after'); + expect(extent.contentOffset).toBe(0); + expect(extent.maxScrollY).toBe(maxScroll(50)); + expect(extent.contentHeight).toBe(50 * LINE_HEIGHT); + + controller.scrollBy('after', maxScroll(50) + 20 * LINE_HEIGHT); + expect(controller.afterScrollY).toBe(maxScroll(50)); + expect(controller.beforeScrollY).toBe(0); + }); + }); }); From 9c82b0d8cf995feaa7b6745826d6ae772c9e1aa8 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 15 Oct 2026 15:29:01 +1100 Subject: [PATCH 7/7] test(managed-acp): guard the validation fixture's node shim against ETXTBSY 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 7cad5706 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 Signed-off-by: Matt Toohey --- .../staged/src-tauri/src/managed_acp_tools.rs | 18 +++++--- .../src/managed_acp_tools/tests/validation.rs | 45 ++++++++++++------- 2 files changed, 42 insertions(+), 21 deletions(-) diff --git a/apps/staged/src-tauri/src/managed_acp_tools.rs b/apps/staged/src-tauri/src/managed_acp_tools.rs index 3aaeb9d37..73b2221ee 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools.rs @@ -1523,16 +1523,22 @@ mod tests { } /// Writes `contents` to `path` as a 0755 script and returns only once it - /// can actually be exec'd. On Linux, a test on another thread that forks - /// while our write handle is open inherits it until its own exec, and - /// exec of a file anyone still holds open for writing fails with - /// `ETXTBSY` — which `npm_view_version` would faithfully report as - /// "unknown" and fail the test. Probe-spawn (and immediately kill) the - /// script until that window has passed. + /// can actually be exec'd (see [`wait_until_executable`]). fn write_executable(path: &Path, contents: &str) { std::fs::write(path, contents).unwrap(); use std::os::unix::fs::PermissionsExt; std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o755)).unwrap(); + wait_until_executable(path); + } + + /// Returns only once the freshly written script at `path` can actually be + /// exec'd. On Linux, a test on another thread that forks while our write + /// handle is open inherits it until its own exec, and exec of a file + /// anyone still holds open for writing fails with `ETXTBSY` — which the + /// code under test would faithfully report and fail the test. Probe-spawn + /// (and immediately kill) the script until that window has passed, so the + /// script must be harmless to run with no arguments. + fn wait_until_executable(path: &Path) { for _ in 0..1000 { match std::process::Command::new(path) .stdin(std::process::Stdio::null()) diff --git a/apps/staged/src-tauri/src/managed_acp_tools/tests/validation.rs b/apps/staged/src-tauri/src/managed_acp_tools/tests/validation.rs index 19f3c9c2d..f2683a381 100644 --- a/apps/staged/src-tauri/src/managed_acp_tools/tests/validation.rs +++ b/apps/staged/src-tauri/src/managed_acp_tools/tests/validation.rs @@ -3,6 +3,15 @@ use super::*; const TIMEOUT: Duration = super::super::validation::VALIDATION_TIMEOUT; const MISSING_NATIVE: &str = "echo 'native binary not found for this platform' >&2\nexit 1\n"; +/// Installs `contents` as the fake `node` under `bin_dir`, through the +/// production shim writer, and waits out the `ETXTBSY` window before the +/// validation probe execs it. A busy exec would otherwise fail validation +/// with `could not start: Text file busy`. +fn write_node_shim(bin_dir: &Path, contents: &str) { + write_shim(bin_dir, "node", contents).unwrap(); + wait_until_executable(&bin_dir.join("node")); +} + fn write_probe(node: &Path, template: &Path, tool: &ManagedTool, script: &str) { write_fixture_install(template, tool, "1.2.4"); std::fs::write(npm_entrypoint(template, tool.package), script).unwrap(); @@ -14,15 +23,14 @@ fn write_probe(node: &Path, template: &Path, tool: &ManagedTool, script: &str) { "codex-acp" => "[ \"$2\" = cli ] && [ \"$3\" = -V ]", _ => panic!("missing probe fixture for {}", tool.id), }; - write_shim( + // With no arguments this exits 91 at once, so it is safe to probe-spawn. + write_node_shim( &node.join("bin"), - "node", &format!( "#!/bin/sh\ncase \"$1\" in /*) ;; *) exit 91 ;; esac\n[ \"$1\" -ef \"$PWD/node_modules/{}/dist/index.js\" ] || exit 91\n{args_check} && [ $# = 3 ] || exit 92\nexec /bin/sh \"$@\"\n", tool.package ), - ) - .unwrap(); + ); } #[tokio::test] @@ -113,12 +121,7 @@ async fn rejected_upgrade( let live = tool_install_dir(&root, tool.id); let entrypoint = npm_entrypoint(&live, tool.package); std::fs::write(&entrypoint, "echo '1.2.3'\n").unwrap(); - write_shim( - &old_node.join("bin"), - "node", - "#!/bin/sh\nexec /bin/sh \"$@\"\n", - ) - .unwrap(); + write_node_shim(&old_node.join("bin"), "#!/bin/sh\nexec /bin/sh \"$@\"\n"); let shim = shim_bin_dir(&root).join(tool.binary); let shim_before = std::fs::read(&shim).unwrap(); let state_before = std::fs::read(state_path(&root)).unwrap(); @@ -245,7 +248,10 @@ async fn personal_overrides_cannot_mask_missing_native_binary() { const HANGING_NATIVE: &str = "/bin/sleep 60 &\necho \"$$ $!\" > ../../validation-pids\necho 'waiting for native CLI' >&2\n"; -async fn probe_pids(root: &Path) -> Vec { +/// The bridge and native-child PIDs the hanging probe records, or `None` if +/// it has not recorded them within 5 seconds. Callers attach whatever they +/// know about why validation never got that far. +async fn probe_pids(root: &Path) -> Option> { tokio::time::timeout(Duration::from_secs(5), async { loop { if let Ok(contents) = tokio::fs::read_to_string(root.join("validation-pids")).await { @@ -261,7 +267,7 @@ async fn probe_pids(root: &Path) -> Vec { } }) .await - .expect("probe did not spawn its native child") + .ok() } async fn assert_probe_tree_stopped(pids: &[i32]) { @@ -300,7 +306,9 @@ async fn timeout_preserves_install_and_kills_tree(bridge_exit: &str) { Duration::from_secs(1), ) .await; - let pids = probe_pids(&dir.path().join("packages")).await; + let pids = probe_pids(&dir.path().join("packages")) + .await + .unwrap_or_else(|| panic!("probe did not spawn its native child: {error}")); assert_probe_tree_stopped(&pids).await; assert!(error.contains("timed out after 1 seconds"), "{error}"); assert!(error.contains("waiting for native CLI"), "{error}"); @@ -326,10 +334,17 @@ async fn cancellation_kills_bridge_and_native_child() { let node = root.join("node"); let staging = staging_install_dir(&root, tool.id); write_probe(&node, &staging, &tool, &format!("{HANGING_NATIVE}wait\n")); - let probe = tokio::spawn(async move { + let mut probe = tokio::spawn(async move { super::super::validation::validate_staged_tool(&node, &staging, &tool, TIMEOUT).await }); - let pids = probe_pids(&root).await; + // Validation should hang until aborted. If it returns first (say, the + // node shim failed to start), report why instead of a bare timeout. + let pids = tokio::select! { + pids = probe_pids(&root) => pids.expect("probe did not spawn its native child"), + result = &mut probe => { + panic!("validation finished before spawning its native child: {result:?}") + } + }; probe.abort(); assert!(probe.await.unwrap_err().is_cancelled()); assert_probe_tree_stopped(&pids).await;