From 325eb51941243b637640f809e8dfdada30578ee2 Mon Sep 17 00:00:00 2001 From: FND Date: Wed, 16 Sep 2026 13:16:35 +0200 Subject: [PATCH 1/2] fix(review): full-range gutter annotations and next/prev hunk jump Gutter + now honors the live multi-line selection (single-line fallback preserved); N/P jump between changed hunks on both diff views. Closes #991, closes #1543. --- .github/workflows/test.yml | 1 + .../AllFilesCodeView.lifecycle.test.tsx | 67 +++++++++++ .../components/AllFilesCodeView.tsx | 97 ++++++++++++++- .../DiffViewer.compactTouchSelection.test.tsx | 65 +++++++++++ .../review-editor/components/DiffViewer.tsx | 104 ++++++++++++++++- packages/review-editor/shortcuts.ts | 14 +++ .../review-editor/utils/diffSelection.test.ts | 102 ++++++++++++++++ packages/review-editor/utils/diffSelection.ts | 27 +++++ .../utils/hunkNavigation.test.ts | 84 +++++++++++++ .../review-editor/utils/hunkNavigation.ts | 110 ++++++++++++++++++ 10 files changed, 665 insertions(+), 6 deletions(-) create mode 100644 packages/review-editor/utils/diffSelection.test.ts create mode 100644 packages/review-editor/utils/hunkNavigation.test.ts create mode 100644 packages/review-editor/utils/hunkNavigation.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ed5f0fa5e..b99c00d79 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -132,6 +132,7 @@ jobs: packages/review-editor/components/TokenHoverCard.test.tsx packages/review-editor/components/TokenHoverAnnouncementDialog.test.tsx packages/review-editor/utils/stitchTokenIdentifier.test.ts + packages/review-editor/utils/diffSelection.test.ts packages/review-editor/hooks/useAutoViewed.test.tsx packages/review-editor/hooks/useCallFlowAnalysis.test.tsx packages/review-editor/hooks/useCallFlowInstall.test.tsx diff --git a/packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx b/packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx index f7b48b6ae..85ca24f33 100644 --- a/packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx +++ b/packages/review-editor/components/AllFilesCodeView.lifecycle.test.tsx @@ -324,6 +324,73 @@ describe('AllFilesCodeView compact-touch line selection', () => { expect(toolbarSelections).toEqual([range]); }); + + test.skipIf(!hasDom)('multi-line selection + gutter click on the middle line publishes the full range (#991)', async () => { + await mount(false); + const { options, item } = getSelectionCallbacks(); + + const line4 = document.createElement('div'); + line4.setAttribute('data-line', '4'); + line4.setAttribute('data-additions', ''); + const line8 = document.createElement('div'); + line8.setAttribute('data-line', '8'); + line8.setAttribute('data-additions', ''); + host!.appendChild(line4); + host!.appendChild(line8); + + const originalGetSelection = window.getSelection; + window.getSelection = () => ({ + isCollapsed: false, + toString: () => 'line 4 through 8', + anchorNode: line4, + focusNode: line8, + removeAllRanges: () => {}, + } as unknown as Selection); + + try { + const scrollContainer = (host!.querySelector('.overflow-y-auto') as HTMLElement) ?? host!; + scrollContainer.dispatchEvent(new Event('pointerdown', { bubbles: true })); + + const middleLineRange: SelectedLineRange = { start: 6, end: 6, side: 'additions' }; + await act(async () => { + options.onGutterUtilityClick?.(middleLineRange, { item }); + }); + + expect(toolbarSelections).toEqual([{ start: 4, end: 8, side: 'additions' }]); + } finally { + window.getSelection = originalGetSelection; + line4.remove(); + line8.remove(); + } + }); + + test.skipIf(!hasDom)('no-selection gutter click preserves the single-line fallback (#991)', async () => { + await mount(false); + const { options, item } = getSelectionCallbacks(); + + const originalGetSelection = window.getSelection; + window.getSelection = () => ({ + isCollapsed: true, + toString: () => '', + anchorNode: null, + focusNode: null, + removeAllRanges: () => {}, + } as unknown as Selection); + + try { + const scrollContainer = (host!.querySelector('.overflow-y-auto') as HTMLElement) ?? host!; + scrollContainer.dispatchEvent(new Event('pointerdown', { bubbles: true })); + + const singleLineRange: SelectedLineRange = { start: 6, end: 6, side: 'additions' }; + await act(async () => { + options.onGutterUtilityClick?.(singleLineRange, { item }); + }); + + expect(toolbarSelections).toEqual([singleLineRange]); + } finally { + window.getSelection = originalGetSelection; + } + }); }); describe('AllFilesCodeView readOnly (portable guide host)', () => { diff --git a/packages/review-editor/components/AllFilesCodeView.tsx b/packages/review-editor/components/AllFilesCodeView.tsx index 1d6fd28a6..bdfec851f 100644 --- a/packages/review-editor/components/AllFilesCodeView.tsx +++ b/packages/review-editor/components/AllFilesCodeView.tsx @@ -27,13 +27,21 @@ import { useIsWorkerPoolReadyOrDisabled, useWorkerPoolThemeSync } from '../worke import type { DiffFile, AnnotationScrollTarget } from '../types'; import { buildFileTree, getVisualFileOrder } from '../utils/buildFileTree'; import { buildCodeNavRequest } from '../utils/buildCodeNavRequest'; -import { getDiffSelection, getLineNumberFromNode, getSideFromNode } from '../utils/diffSelection'; +import { getDiffSelection, getLineNumberFromNode, getSideFromNode, snapshotDiffSelection, type DiffSelectionSnapshot } from '../utils/diffSelection'; import { isContentConsistentWithPatch } from '../utils/patchConsistency'; import { hashString } from '../utils/hashString'; import { resolveLineSelectionBehavior, type LineSelectionSource, } from '../utils/lineSelectionBehavior'; +import { + findHunkLineElement, + getElementScrollTop, + getHunkTargetLine, + resolveTargetHunkIndex, + scrollToHunkElement, + type HunkLike, +} from '../utils/hunkNavigation'; import { isContentlessBinaryPatch, isOversizedReviewStubPatch } from '@plannotator/shared/diff-paths'; import { OversizedFileNotice } from './OversizedFileNotice'; import { ToolbarHost, type ToolbarHostHandle } from './ToolbarHost'; @@ -637,6 +645,7 @@ export const AllFilesCodeView: React.FC = ({ useWorkerPoolThemeSync(pierreTheme.syntaxTheme); const viewerRef = useRef | null>(null); const scrollRef = useRef(null); + const selectionSnapshotRef = useRef(null); // State mirror of the scroll container so the leading-content portal can // mount once CodeView has rendered it (a plain ref can't trigger that). const [scrollEl, setScrollEl] = useState(null); @@ -1570,9 +1579,34 @@ export const AllFilesCodeView: React.FC = ({ useEffect(() => { const root = scrollRef.current; if (!root) return; + let clearTimer: ReturnType | null = null; + const onPointerDown = () => { + if (clearTimer) { + clearTimeout(clearTimer); + clearTimer = null; + } + selectionSnapshotRef.current = snapshotDiffSelection(root); + }; + const onCancel = () => { + selectionSnapshotRef.current = null; + }; + const onPointerUp = () => { + clearTimer = setTimeout(() => { + selectionSnapshotRef.current = null; + }, 200); + }; + root.addEventListener('pointerdown', onPointerDown, true); + root.addEventListener('pointercancel', onCancel, true); + root.addEventListener('pointerup', onPointerUp, true); const handler = () => handleContentTextSelection(); root.addEventListener('mouseup', handler, true); - return () => root.removeEventListener('mouseup', handler, true); + return () => { + if (clearTimer) clearTimeout(clearTimer); + root.removeEventListener('pointerdown', onPointerDown, true); + root.removeEventListener('pointercancel', onCancel, true); + root.removeEventListener('pointerup', onPointerUp, true); + root.removeEventListener('mouseup', handler, true); + }; // eslint-disable-next-line react-hooks/exhaustive-deps }, [fileSetKey]); @@ -1970,7 +2004,16 @@ export const AllFilesCodeView: React.FC = ({ const handleGutterUtilityClick = useStableCallback( (range: SelectedLineRange, item: CodeViewItem) => { - handleLineSelectionInteraction('gutter-comment-action', range, item); + const snapshot = selectionSnapshotRef.current; + selectionSnapshotRef.current = null; + let effectiveRange = range; + if (snapshot) { + const snapshotItemId = snapshot.host ? nodeToItemIdRef.current.get(snapshot.host) : undefined; + if (!snapshot.host || !snapshotItemId || snapshotItemId === item.id) { + effectiveRange = { start: snapshot.start, end: snapshot.end, side: snapshot.side }; + } + } + handleLineSelectionInteraction('gutter-comment-action', effectiveRange, item); }, ); @@ -2241,6 +2284,41 @@ export const AllFilesCodeView: React.FC = ({ return () => cancelAnimationFrame(raf); // eslint-disable-next-line react-hooks/exhaustive-deps }, [scrollTargetAnnotation, filePathToItemId]); + const jumpHunk = useStableCallback((direction: 'next' | 'prev') => { + const container = scrollRef.current; + if (!container) return; + + const hunkTops: Array<{ top: number; el: HTMLElement; itemId: string }> = []; + for (const item of identity.items) { + if (item.type !== 'diff' || !item.fileDiff?.hunks || isItemCollapsed(item.id)) continue; + for (const hunk of item.fileDiff.hunks as HunkLike[]) { + const target = getHunkTargetLine(hunk); + const el = findHunkLineElement(container, target); + if (el) { + hunkTops.push({ + top: getElementScrollTop(container, el), + el, + itemId: item.id, + }); + } + } + } + + if (hunkTops.length === 0) return; + + hunkTops.sort((a, b) => a.top - b.top); + + const targetIdx = resolveTargetHunkIndex( + hunkTops.map((h) => h.top), + container.scrollTop, + direction, + ); + + if (targetIdx != null) { + scrollToHunkElement(hunkTops[targetIdx].el); + } + }); + useEffect(() => { if (!isActive || readOnly) return; @@ -2316,6 +2394,18 @@ export const AllFilesCodeView: React.FC = ({ onStage?.(currentPath); return; } + // n / p — jump to next / previous changed hunk. + if (e.key === 'n' || e.key === 'N') { + e.preventDefault(); + jumpHunk('next'); + return; + } + if (e.key === 'p' || e.key === 'P') { + e.preventDefault(); + jumpHunk('prev'); + return; + } + if (e.key !== '[' && e.key !== ']') return; e.preventDefault(); @@ -2346,6 +2436,7 @@ export const AllFilesCodeView: React.FC = ({ canStageFiles, canStagePath, onStage, + jumpHunk, ]); // --- Custom header render slot (the full Plannotator FileHeader) ----------- diff --git a/packages/review-editor/components/DiffViewer.compactTouchSelection.test.tsx b/packages/review-editor/components/DiffViewer.compactTouchSelection.test.tsx index 66cd808c0..60efa5064 100644 --- a/packages/review-editor/components/DiffViewer.compactTouchSelection.test.tsx +++ b/packages/review-editor/components/DiffViewer.compactTouchSelection.test.tsx @@ -250,4 +250,69 @@ describe.if(hasDom)('DiffViewer compact-touch line selection (DOM)', () => { // the controlled-repaint handler. expect('onLineSelectionChange' in pierre().options).toBe(false); }); + + test('multi-line selection + gutter click on the middle line publishes the full range (#991)', async () => { + await mount(false); + + const line2 = document.createElement('div'); + line2.setAttribute('data-line', '2'); + line2.setAttribute('data-additions', ''); + const line4 = document.createElement('div'); + line4.setAttribute('data-line', '4'); + line4.setAttribute('data-additions', ''); + host!.appendChild(line2); + host!.appendChild(line4); + + const originalGetSelection = window.getSelection; + window.getSelection = () => ({ + isCollapsed: false, + toString: () => 'line 2\nline 3\nline 4', + anchorNode: line2, + focusNode: line4, + removeAllRanges: () => {}, + } as unknown as Selection); + + try { + const diffContainer = host!.querySelector('.p-4') as HTMLElement; + diffContainer.dispatchEvent(new Event('pointerdown', { bubbles: true })); + + const middleLineRange: SelectedLineRange = { start: 3, end: 3, side: 'additions' }; + await act(async () => { + pierre().options.onGutterUtilityClick?.(middleLineRange); + }); + + expect(toolbarSelections).toEqual([{ start: 2, end: 4, side: 'additions' }]); + } finally { + window.getSelection = originalGetSelection; + line2.remove(); + line4.remove(); + } + }); + + test('no-selection gutter click preserves the single-line fallback (#991)', async () => { + await mount(false); + + const originalGetSelection = window.getSelection; + window.getSelection = () => ({ + isCollapsed: true, + toString: () => '', + anchorNode: null, + focusNode: null, + removeAllRanges: () => {}, + } as unknown as Selection); + + try { + const diffContainer = host!.querySelector('.p-4') as HTMLElement; + diffContainer.dispatchEvent(new Event('pointerdown', { bubbles: true })); + + const singleLineRange: SelectedLineRange = { start: 3, end: 3, side: 'additions' }; + await act(async () => { + pierre().options.onGutterUtilityClick?.(singleLineRange); + }); + + expect(toolbarSelections).toEqual([singleLineRange]); + } finally { + window.getSelection = originalGetSelection; + } + }); }); diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index ab3eef688..dfcb2fda3 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -20,7 +20,7 @@ import { isContentlessBinaryPatch, isOversizedReviewStubPatch } from '@plannotat import { isFileScopedAnnotation, lineRangeForAnnotation } from '../utils/annotationScope'; import { lineAnnotationMetadata } from '../utils/annotationDisplay'; import type { AnnotationScrollTarget } from '../types'; -import { getLineNumberFromNode, getSideFromNode, getDiffSelection } from '../utils/diffSelection'; +import { getLineNumberFromNode, getSideFromNode, getDiffSelection, snapshotDiffSelection, type DiffSelectionSnapshot } from '../utils/diffSelection'; import { isContentConsistentWithPatch } from '../utils/patchConsistency'; import { hashString } from '../utils/hashString'; import { InlineAnnotation } from './InlineAnnotation'; @@ -38,6 +38,14 @@ import { resolveLineSelectionBehavior, type LineSelectionSource, } from '../utils/lineSelectionBehavior'; +import { + findHunkLineElement, + getElementScrollTop, + getHunkTargetLine, + resolveTargetHunkIndex, + scrollToHunkElement, + type HunkLike, +} from '../utils/hunkNavigation'; interface PierreDiffContentProps { filePath: string; @@ -302,6 +310,7 @@ export const DiffViewer: React.FC = ({ useOverlayViewport(); const splitSurfaceRef = useRef(null); const diffContentRef = useRef(null); + const selectionSnapshotRef = useRef(null); const [fileCommentAnchor, setFileCommentAnchor] = useState(null); // Resizable split pane — only applies when Pierre renders a two-column grid @@ -688,12 +697,36 @@ export const DiffViewer: React.FC = ({ }, [onLineSelection]); const handleGutterUtilityClick = useCallback((range: SelectedLineRange) => { - handleLineSelectionInteraction('gutter-comment-action', range); + const snapshot = selectionSnapshotRef.current; + selectionSnapshotRef.current = null; + const effectiveRange: SelectedLineRange = snapshot + ? { start: snapshot.start, end: snapshot.end, side: snapshot.side } + : range; + handleLineSelectionInteraction('gutter-comment-action', effectiveRange); }, [handleLineSelectionInteraction]); useEffect(() => { const root = diffContentRef.current; if (!root) return; + let clearTimer: ReturnType | null = null; + const onPointerDown = () => { + if (clearTimer) { + clearTimeout(clearTimer); + clearTimer = null; + } + selectionSnapshotRef.current = snapshotDiffSelection(root); + }; + const onCancel = () => { + selectionSnapshotRef.current = null; + }; + const onPointerUp = () => { + clearTimer = setTimeout(() => { + selectionSnapshotRef.current = null; + }, 200); + }; + root.addEventListener('pointerdown', onPointerDown, true); + root.addEventListener('pointercancel', onCancel, true); + root.addEventListener('pointerup', onPointerUp, true); const handler = () => { requestAnimationFrame(() => { const selection = getDiffSelection(root); @@ -712,12 +745,77 @@ export const DiffViewer: React.FC = ({ }); }; root.addEventListener('mouseup', handler, true); - return () => root.removeEventListener('mouseup', handler, true); + return () => { + if (clearTimer) clearTimeout(clearTimer); + root.removeEventListener('pointerdown', onPointerDown, true); + root.removeEventListener('pointercancel', onCancel, true); + root.removeEventListener('pointerup', onPointerUp, true); + root.removeEventListener('mouseup', handler, true); + }; }, []); const handlePierreLineSelectionEnd = useCallback((range: SelectedLineRange | null) => { handleLineSelectionInteraction('range-gesture', range); }, [handleLineSelectionInteraction]); + const jumpHunk = useCallback((direction: 'next' | 'prev') => { + const container = containerRef.current; + const hunks = (augmentedDiff?.hunks ?? fileDiff?.hunks ?? []) as HunkLike[]; + if (!container || hunks.length === 0) return; + + const hunkTops: Array<{ index: number; top: number; el: HTMLElement }> = []; + for (let i = 0; i < hunks.length; i++) { + const target = getHunkTargetLine(hunks[i]); + const el = findHunkLineElement(container, target); + if (el) { + hunkTops.push({ + index: i, + top: getElementScrollTop(container, el), + el, + }); + } + } + + if (hunkTops.length === 0) return; + + const targetIdx = resolveTargetHunkIndex( + hunkTops.map((h) => h.top), + container.scrollTop, + direction, + ); + + if (targetIdx != null) { + scrollToHunkElement(hunkTops[targetIdx].el); + } + }, [augmentedDiff, fileDiff]); + + useEffect(() => { + if (collapsed || isFocused === false) return; + const handler = (e: KeyboardEvent) => { + const el = (e.composedPath?.()[0] ?? e.target) as HTMLElement | null; + if ( + el && + (el.tagName === 'INPUT' || + el.tagName === 'TEXTAREA' || + el.tagName === 'SELECT' || + el.isContentEditable) + ) + return; + if (e.metaKey || e.ctrlKey || e.shiftKey || e.altKey) return; + if (e.key === 'n' || e.key === 'N') { + const hunks = (augmentedDiff?.hunks ?? fileDiff?.hunks ?? []) as HunkLike[]; + if (hunks.length === 0) return; + e.preventDefault(); + jumpHunk('next'); + } else if (e.key === 'p' || e.key === 'P') { + const hunks = (augmentedDiff?.hunks ?? fileDiff?.hunks ?? []) as HunkLike[]; + if (hunks.length === 0) return; + e.preventDefault(); + jumpHunk('prev'); + } + }; + window.addEventListener('keydown', handler); + return () => window.removeEventListener('keydown', handler); + }, [collapsed, isFocused, jumpHunk, augmentedDiff, fileDiff]); // Token interaction handlers (code area clicks) const handleTokenClick = useCallback((props: DiffTokenEventBaseProps, event: MouseEvent) => { diff --git a/packages/review-editor/shortcuts.ts b/packages/review-editor/shortcuts.ts index 56b16cf81..6c09a4d0f 100644 --- a/packages/review-editor/shortcuts.ts +++ b/packages/review-editor/shortcuts.ts @@ -104,6 +104,20 @@ export const reviewEditorShortcuts = defineShortcutScope({ hint: 'Available when staging is supported (not in PR review mode).', displayOrder: 20, }, + nextHunk: { + description: 'Jump to next changed hunk', + bindings: ['N'], + section: 'File Actions', + hint: 'Jumps to the next changed hunk in the active diff.', + displayOrder: 30, + }, + prevHunk: { + description: 'Jump to previous changed hunk', + bindings: ['P'], + section: 'File Actions', + hint: 'Jumps to the previous changed hunk in the active diff.', + displayOrder: 40, + }, }, }); diff --git a/packages/review-editor/utils/diffSelection.test.ts b/packages/review-editor/utils/diffSelection.test.ts new file mode 100644 index 000000000..89de372ad --- /dev/null +++ b/packages/review-editor/utils/diffSelection.test.ts @@ -0,0 +1,102 @@ +import { describe, expect, test } from 'bun:test'; +import { + getLineNumberFromNode, + getSideFromNode, + snapshotDiffSelection, +} from './diffSelection'; + +const hasDom = typeof document !== 'undefined'; + +describe.skipIf(!hasDom)('diffSelection', () => { + test('resolves line number from node or ancestor data-line attribute', () => { + const parent = document.createElement('div'); + parent.setAttribute('data-line', '42'); + const child = document.createElement('span'); + parent.appendChild(child); + const textNode = document.createTextNode('code content'); + child.appendChild(textNode); + + expect(getLineNumberFromNode(textNode)).toBe(42); + expect(getLineNumberFromNode(child)).toBe(42); + expect(getLineNumberFromNode(parent)).toBe(42); + expect(getLineNumberFromNode(document.createElement('div'))).toBeNull(); + }); + + test('resolves diff side from node or ancestor attribute, defaulting to additions', () => { + const additionsEl = document.createElement('div'); + additionsEl.setAttribute('data-additions', ''); + const delEl = document.createElement('div'); + delEl.setAttribute('data-deletions', ''); + const neutralEl = document.createElement('div'); + + expect(getSideFromNode(additionsEl)).toBe('additions'); + expect(getSideFromNode(delEl)).toBe('deletions'); + expect(getSideFromNode(neutralEl)).toBe('additions'); + }); + + test('guards against collapsed selection by returning null', () => { + const el = document.createElement('div'); + el.setAttribute('data-line', '10'); + const selection = { + isCollapsed: true, + toString: () => 'selected', + anchorNode: el, + focusNode: el, + } as unknown as Selection; + + expect(snapshotDiffSelection(null, selection)).toBeNull(); + }); + + test('guards against whitespace-only selection by returning null', () => { + const el = document.createElement('div'); + el.setAttribute('data-line', '10'); + const selection = { + isCollapsed: false, + toString: () => ' \n \t ', + anchorNode: el, + focusNode: el, + } as unknown as Selection; + + expect(snapshotDiffSelection(null, selection)).toBeNull(); + }); + + test('guards against selection missing anchor or focus lines by returning null', () => { + const lineEl = document.createElement('div'); + lineEl.setAttribute('data-line', '10'); + const outsideEl = document.createElement('div'); + + const selectionNoAnchor = { + isCollapsed: false, + toString: () => 'code', + anchorNode: outsideEl, + focusNode: lineEl, + } as unknown as Selection; + + expect(snapshotDiffSelection(null, selectionNoAnchor)).toBeNull(); + }); + + test('snapshots a multi-line selection with min/max bounds and side', () => { + const line2 = document.createElement('div'); + line2.setAttribute('data-line', '2'); + line2.setAttribute('data-additions', ''); + const line6 = document.createElement('div'); + line6.setAttribute('data-line', '6'); + line6.setAttribute('data-additions', ''); + + // Focus on line 2, anchor on line 6 (backward selection drag) + const selection = { + isCollapsed: false, + toString: () => 'lines 2 through 6', + anchorNode: line6, + focusNode: line2, + } as unknown as Selection; + + const snapshot = snapshotDiffSelection(null, selection); + expect(snapshot).toEqual({ + start: 2, + end: 6, + side: 'additions', + host: null, + }); + }); +}); diff --git a/packages/review-editor/utils/diffSelection.ts b/packages/review-editor/utils/diffSelection.ts index 00a3012e3..1c806f014 100644 --- a/packages/review-editor/utils/diffSelection.ts +++ b/packages/review-editor/utils/diffSelection.ts @@ -38,3 +38,30 @@ export function getDiffSelection(root: HTMLElement | null): Selection | null { } return window.getSelection(); } + +export interface DiffSelectionSnapshot { + start: number; + end: number; + side: 'deletions' | 'additions'; + host?: HTMLElement | null; +} + +export function snapshotDiffSelection( + root: HTMLElement | null, + customSelection?: Selection | null, +): DiffSelectionSnapshot | null { + const selection = customSelection ?? getDiffSelection(root); + if (!selection || selection.isCollapsed || !selection.toString().trim()) return null; + const anchorLine = getLineNumberFromNode(selection.anchorNode); + const focusLine = getLineNumberFromNode(selection.focusNode); + if (anchorLine == null || focusLine == null) return null; + const side = getSideFromNode(selection.anchorNode); + const rootNode = selection.anchorNode?.getRootNode(); + const host = rootNode instanceof ShadowRoot && rootNode.host instanceof HTMLElement ? rootNode.host : null; + return { + start: Math.min(anchorLine, focusLine), + end: Math.max(anchorLine, focusLine), + side, + host, + }; +} diff --git a/packages/review-editor/utils/hunkNavigation.test.ts b/packages/review-editor/utils/hunkNavigation.test.ts new file mode 100644 index 000000000..bac384971 --- /dev/null +++ b/packages/review-editor/utils/hunkNavigation.test.ts @@ -0,0 +1,84 @@ +import { describe, expect, test } from 'bun:test'; +import { + getHunkTargetLine, + resolveTargetHunkIndex, +} from './hunkNavigation'; + +describe('hunkNavigation: resolveTargetHunkIndex', () => { + const HUNKS = [100, 350, 800, 1500]; + + test('guards against staying on current hunk due to subpixel/scroll settling (next jump from hunk top)', () => { + // When exactly at or slightly past hunk 0 (100 or 105), next must jump to hunk 1 (350), not re-target 0 + expect(resolveTargetHunkIndex(HUNKS, 100, 'next')).toBe(1); + expect(resolveTargetHunkIndex(HUNKS, 105, 'next')).toBe(1); + expect(resolveTargetHunkIndex(HUNKS, 95, 'next')).toBe(0); + // Pin that epsilon is active (>=1px) while the 95->0 assertion pins it bounded (<5px) + expect(resolveTargetHunkIndex(HUNKS, 99, 'next')).toBe(1); + }); + + test('guards against out-of-bounds index or looping past the last hunk (next jump)', () => { + // When at or past the final hunk (1500), next must return null rather than wrap or exceed bounds + expect(resolveTargetHunkIndex(HUNKS, 1500, 'next')).toBeNull(); + expect(resolveTargetHunkIndex(HUNKS, 1600, 'next')).toBeNull(); + }); + + test('guards against jumping to the wrong target when positioned between hunks (next jump)', () => { + // Positioned midway between hunk 1 (350) and hunk 2 (800) + expect(resolveTargetHunkIndex(HUNKS, 500, 'next')).toBe(2); + }); + + test('guards against staying on current hunk due to subpixel rounding (prev jump from hunk top)', () => { + // When at hunk 2 (800), prev must jump to hunk 1 (350), not re-target 2 + expect(resolveTargetHunkIndex(HUNKS, 800, 'prev')).toBe(1); + // 805 is 5px inside hunk 2, so prev goes to hunk 2's top at index 2 + expect(resolveTargetHunkIndex(HUNKS, 805, 'prev')).toBe(2); + }); + + test('guards against negative index when at or before the first hunk (prev jump)', () => { + // When at or above hunk 0 (100), prev must return null rather than negative index + expect(resolveTargetHunkIndex(HUNKS, 100, 'prev')).toBeNull(); + expect(resolveTargetHunkIndex(HUNKS, 50, 'prev')).toBeNull(); + expect(resolveTargetHunkIndex(HUNKS, 0, 'prev')).toBeNull(); + }); + + test('guards against wrong target when positioned between hunks (prev jump)', () => { + // Positioned midway between hunk 1 (350) and hunk 2 (800) + expect(resolveTargetHunkIndex(HUNKS, 600, 'prev')).toBe(1); + // Positioned midway between hunk 0 (100) and hunk 1 (350) + expect(resolveTargetHunkIndex(HUNKS, 250, 'prev')).toBe(0); + }); + + test('guards against crash or infinite loop when hunk list is empty', () => { + expect(resolveTargetHunkIndex([], 0, 'next')).toBeNull(); + expect(resolveTargetHunkIndex([], 100, 'prev')).toBeNull(); + }); +}); + +describe('hunkNavigation: getHunkTargetLine', () => { + test('resolves additionStart for hunks with additions', () => { + const hunk = { + additionStart: 25, + additionCount: 10, + additionLines: 4, + deletionStart: 20, + deletionCount: 2, + }; + expect(getHunkTargetLine(hunk)).toEqual({ lineNumber: 25, side: 'additions' }); + }); + + test('resolves deletionStart for pure deletion hunks', () => { + const pureDeletionHunk = { + additionStart: 0, + additionCount: 0, + additionLines: 0, + deletionStart: 45, + deletionCount: 5, + deletionLines: 5, + }; + expect(getHunkTargetLine(pureDeletionHunk)).toEqual({ lineNumber: 45, side: 'deletions' }); + }); + + test('falls back gracefully when starts are zero or missing', () => { + expect(getHunkTargetLine({})).toEqual({ lineNumber: 1 }); + }); +}); diff --git a/packages/review-editor/utils/hunkNavigation.ts b/packages/review-editor/utils/hunkNavigation.ts new file mode 100644 index 000000000..552d19247 --- /dev/null +++ b/packages/review-editor/utils/hunkNavigation.ts @@ -0,0 +1,110 @@ +import { getSearchRoots } from './reviewSearchHighlight'; + +export interface HunkLineTarget { + lineNumber: number; + side?: 'additions' | 'deletions'; +} + +export interface HunkLike { + additionStart?: number; + deletionStart?: number; + additionCount?: number; + deletionCount?: number; + additionLines?: number; + deletionLines?: number; +} + +/** + * Resolves the primary line number and side to query for a given diff hunk. + * Prioritizes additionStart for hunks containing additions or context; + * falls back to deletionStart for pure deletions. + */ +export function getHunkTargetLine(hunk: HunkLike): HunkLineTarget { + if ((hunk.additionLines ?? 0) > 0 || (hunk.additionCount ?? 0) > 0) { + if (hunk.additionStart && hunk.additionStart > 0) { + return { lineNumber: hunk.additionStart, side: 'additions' }; + } + } + if (hunk.deletionStart && hunk.deletionStart > 0) { + return { lineNumber: hunk.deletionStart, side: 'deletions' }; + } + return { lineNumber: hunk.additionStart || hunk.deletionStart || 1 }; +} + +/** + * Pure index resolution: given sorted vertical hunk start positions, current viewport + * scrollTop, and jump direction, returns the 0-based index of the target hunk. + * + * An epsilon threshold (default 2px) covers subpixel settling only (~2px): + * positions within 2px of a hunk top count as ON it, anything further counts + * as genuinely before/inside. + * + * Returns null if no valid target exists (before first hunk on prev, past last hunk on next, + * or empty list). + */ +export function resolveTargetHunkIndex( + hunkStarts: readonly number[], + scrollTop: number, + direction: 'next' | 'prev', + threshold = 2, +): number | null { + if (hunkStarts.length === 0) return null; + + if (direction === 'next') { + for (let i = 0; i < hunkStarts.length; i++) { + if (hunkStarts[i] > scrollTop + threshold) { + return i; + } + } + return null; + } + + // direction === 'prev' + for (let i = hunkStarts.length - 1; i >= 0; i--) { + if (hunkStarts[i] < scrollTop - threshold) { + return i; + } + } + return null; +} + +/** + * Queries shadow roots inside the container for the element matching a hunk target line. + */ +export function findHunkLineElement( + container: HTMLElement, + target: HunkLineTarget, +): HTMLElement | null { + const roots = getSearchRoots(container); + const { lineNumber, side } = target; + for (const root of roots) { + if (side) { + const sided = (root as ParentNode).querySelector?.( + `[data-line="${lineNumber}"][data-${side}]`, + ); + if (sided instanceof HTMLElement) return sided; + } + const anyLine = (root as ParentNode).querySelector?.(`[data-line="${lineNumber}"]`); + if (anyLine instanceof HTMLElement) return anyLine; + } + return null; +} + +/** + * Returns the vertical position of an element in the scroll container's scrollTop coordinate space. + */ +export function getElementScrollTop( + scrollContainer: HTMLElement, + target: HTMLElement, +): number { + const containerRect = scrollContainer.getBoundingClientRect(); + const targetRect = target.getBoundingClientRect(); + return scrollContainer.scrollTop + (targetRect.top - containerRect.top); +} + +/** + * Smoothly scrolls the target hunk element into view at the top of the viewport. + */ +export function scrollToHunkElement(target: HTMLElement): void { + target.scrollIntoView({ behavior: 'smooth', block: 'start', inline: 'nearest' }); +} From afb36a29fad25bb75357684bc8a96334f1a0b587 Mon Sep 17 00:00:00 2001 From: FND Date: Wed, 16 Sep 2026 13:23:53 +0200 Subject: [PATCH 2/2] chore(guides-show): regenerate pinned viewer manifest AllFilesCodeView is bundled into the portable guide viewer; the hunk-navigation change alters the bundle hash. --- packages/core/guide-viewer-manifest.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/guide-viewer-manifest.ts b/packages/core/guide-viewer-manifest.ts index 4445ebc92..d2a93dfbb 100644 --- a/packages/core/guide-viewer-manifest.ts +++ b/packages/core/guide-viewer-manifest.ts @@ -5,9 +5,9 @@ import type { GuideViewerAssets } from "./guide-format"; export const GUIDE_VIEWER_MANIFEST: Omit = { - js: "viewer.KTNT-M2b.js", + js: "viewer.C8eISG76.js", css: "viewer.NkTIi4sR.css", - jsIntegrity: "sha384-UGxkmDjeL0LMAKSAnleY0ewq4d4vtotFlHvHYWaK6UGIWmV30DyT5wSKQEz2NHdV", + jsIntegrity: "sha384-u11b9grzN+4gFAWEBBA6KaZBMGHNQYMiHZVMd3M315Pdq/ILkSi7bqzK3jMEh5eX", cssIntegrity: "sha384-2tINtoWgdpcbwUZudhmxJiiW7Tu+29vXj6P12fLLkRtc2sWERGoHl71K35L+UR9f", langs: { "astro": "chunks/astro.Ts5EKq2l.js",