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; diff --git a/packages/diff-viewer/src/lib/components/DiffViewer.svelte b/packages/diff-viewer/src/lib/components/DiffViewer.svelte index 7adf0d772..03b73e261 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, @@ -302,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)); @@ -473,14 +476,16 @@ // 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'; - 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 + // (pure deletions, including whole deleted files, only exist on the before side). + const side = anchorSideForHunk(firstHunk); + scrollController.scrollToRow(firstHunk[side].start, side); }); }); - // 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); @@ -494,6 +499,8 @@ viewportWidth: beforePane.clientWidth, contentWidth, }); + } else { + scrollController.clearDimensions('before'); } }); @@ -510,6 +517,8 @@ viewportWidth: afterPane.clientWidth, contentWidth, }); + } else { + scrollController.clearDimensions('after'); } }); @@ -531,6 +540,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. @@ -1219,8 +1234,7 @@ handleWheel('after', e); } - function handleBeforeScrollbarScroll(deltaY: number) { - scrollController.scrollBy('before', deltaY); + function syncOverlaysAfterScrollbar() { redrawConnectorsImpl(); updateToolbarPosition(); updateCommentEditorPosition(); @@ -1228,13 +1242,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) { @@ -2169,8 +2194,10 @@ scrollToRow: (row, side) => scrollController.scrollToRow(row, side), scrollBy: (deltaY) => scrollController.scrollBy('after', deltaY), getCurrentScrollY: () => scrollController.afterScrollY, - getLineHeight: () => scrollController.getDimensions('after').lineHeight, - getViewportHeight: () => scrollController.getDimensions('after').viewportHeight, + getBeforeScrollY: () => scrollController.beforeScrollY, + // 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; @@ -2180,6 +2207,7 @@ onHunkFocus: (hunkIndex) => { focusedHunkIndex = hunkIndex; }, + getFocusedHunkIndex: () => focusedHunkIndex, }); document.addEventListener('copy', handleCopy); @@ -2309,11 +2337,13 @@ {:else}
@@ -2406,11 +2436,13 @@
@@ -2574,11 +2606,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}
0 && sourceScrollY >= maxScrollY(sourceDims)) return overscroll; + if (overscroll < 0 && sourceScrollY <= 0) return overscroll; + return 0; + } + + /** + * How far the other pane can travel past its aligned position once this + * pane is pinned at the top or bottom: the rows that exist on the other + * side alone before the first (or after the last) shared row. + */ + function edgeHeadroom(side: 'before' | 'after'): { top: number; bottom: number } { + const sourceDims = side === 'before' ? beforeDims : afterDims; + const targetDims = side === 'before' ? afterDims : beforeDims; + const top = transferScroll(0, side, sourceDims, targetDims); + const alignedAtMax = transferScroll(maxScrollY(sourceDims), side, sourceDims, targetDims); + return { top, bottom: maxScrollY(targetDims) - alignedAtMax }; + } + return { // Expose reactive state get beforeScrollY() { @@ -232,20 +294,51 @@ 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. + * + * 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; } }, @@ -269,6 +362,54 @@ export function createScrollController() { afterScrollX = clampScrollX(afterScrollX + deltaX, afterDims); }, + /** + * The extent a pane's scrollbar should run over (see ScrollExtent). The + * position includes any outstanding edge handoff, so a scrollbar bound to + * it tracks scrollBy and scrollToEffective without jumping. + */ + getEffectiveExtent(side: 'before' | 'after'): ScrollExtent { + const sourceDims = side === 'before' ? beforeDims : afterDims; + const targetDims = side === 'before' ? afterDims : beforeDims; + const sourceScrollY = side === 'before' ? beforeScrollY : afterScrollY; + const targetScrollY = side === 'before' ? afterScrollY : beforeScrollY; + + const { top, bottom } = edgeHeadroom(side); + const overscroll = edgeOverscroll(sourceScrollY, side, sourceDims, targetScrollY, targetDims); + const maxScroll = top + maxScrollY(sourceDims) + bottom; + return { + scrollY: Math.max(0, Math.min(maxScroll, top + sourceScrollY + overscroll)), + maxScrollY: maxScroll, + contentHeight: Math.max(sourceDims.contentHeight, sourceDims.viewportHeight) + top + bottom, + contentOffset: top, + }; + }, + + /** + * Set the position within a pane's effective extent (see getEffectiveExtent). + * Inside the pane's own range this is scrollTo; in the headroom beyond + * either edge the pane stays pinned and the remainder moves the other pane, + * the same handoff scrollBy performs incrementally. + */ + scrollToEffective(side: 'before' | 'after', effectiveY: number) { + const sourceDims = side === 'before' ? beforeDims : afterDims; + const targetDims = side === 'before' ? afterDims : beforeDims; + + const { top, bottom } = edgeHeadroom(side); + const clamped = Math.max(0, Math.min(top + maxScrollY(sourceDims) + bottom, effectiveY)); + const newSource = clampScroll(clamped - top, sourceDims); + const residual = clamped - top - newSource; + const aligned = transferScroll(newSource, side, sourceDims, targetDims); + const newTarget = clampScroll(aligned + residual, targetDims); + + if (side === 'before') { + beforeScrollY = newSource; + afterScrollY = newTarget; + } else { + afterScrollY = newSource; + beforeScrollY = newTarget; + } + }, + /** * Set scroll position directly. 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 new file mode 100644 index 000000000..f8f59bbd2 --- /dev/null +++ b/packages/diff-viewer/src/lib/state/scrollController.test.ts @@ -0,0 +1,329 @@ +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 plain values. + beforeEach(() => { + const identity = (value: unknown) => value; + vi.stubGlobal('$state', Object.assign(identity, { raw: identity })); + }); + + 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('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', () => { + 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)); + }); + + 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 }, + { 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); + }); + + 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); + }); + }); + + 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); + }); + }); +}); 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..29111b3ac --- /dev/null +++ b/packages/diff-viewer/src/lib/utils/diffKeyboard.test.ts @@ -0,0 +1,417 @@ +// @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, + getBeforeScrollY: () => controller.beforeScrollY, + 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('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, 40], [0, 40], false), + hunk([40, 70], [40, 40]), + hunk([70, 100], [40, 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'); + 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; + 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 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; + 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 b256b699c..169e42def 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 @@ -22,11 +23,23 @@ 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; /** 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; @@ -45,50 +58,107 @@ const DEFAULT_CONFIG: DiffNavConfig = { onHunkFocus: () => {}, }; +interface NavState { + /** 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 pane's viewport). + */ +function anchorRow(config: DiffNavConfig, side: 'before' | 'after' = 'after'): number { + const anchorY = scrollYFor(config, side) + 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); - - // 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 <= anchorRow) { + const hunk = alignments[i].alignment; + const side = anchorSideForHunk(hunk); + if (hunk[side].start <= anchorRow(config, side)) { currentIndex = i; - } else { - break; } } return currentIndex; } +/** + * 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 neither pane has 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; + 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; + } + 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 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(), + beforeScrollY: scrollYFor(config, 'before'), + }; + 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) { - const nextHunk = alignments[nextIndex].alignment; - config.scrollToRow(nextHunk.after.start, 'after'); - config.onHunkFocus?.(nextIndex); + scrollToHunk(config, state, nextIndex); return true; } @@ -98,8 +168,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; @@ -110,46 +180,34 @@ 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) { - config.scrollToRow(currentHunk.after.start, 'after'); - config.onHunkFocus?.(currentIndex); + const side = anchorSideForHunk(currentHunk); + if (!atStart && anchorRow(config, side) > currentHunk[side].start + 2) { + scrollToHunk(config, state, currentIndex); return true; } // Otherwise go to previous hunk if (currentIndex > 0) { - const prevHunk = alignments[currentIndex - 1].alignment; - config.scrollToRow(prevHunk.after.start, 'after'); - 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) { - config.scrollToRow(alignments[0].alignment.after.start, 'after'); - config.onHunkFocus?.(0); - return true; - } - - return false; + scrollToHunk(config, state, 0); + return true; } /** @@ -158,6 +216,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; @@ -199,13 +258,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; } @@ -225,7 +284,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; } } 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,