From 87d9e69c1f455525f580af8fe5c21b891117787a Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 4 May 2026 14:48:51 -0700 Subject: [PATCH 1/3] feat(web): scroll to exact line when focusing a key change Add scrollToLine to FileDiffList that auto-expands collapsed files, waits for shadow DOM rendering, and scrolls the target diff line into view. Update hunk highlight overlay to measure line rects across number + content columns for accurate positioning. --- .../__tests__/hunk-highlight-overlay.test.ts | 232 ++++++++++++++++++ .../chapter/hunk-highlight-overlay.tsx | 51 +++- .../src/components/files/file-diff-list.tsx | 133 +++++++++- .../web/src/routes/chapter-detail-page.tsx | 6 +- 4 files changed, 410 insertions(+), 12 deletions(-) create mode 100644 packages/web/src/components/chapter/__tests__/hunk-highlight-overlay.test.ts diff --git a/packages/web/src/components/chapter/__tests__/hunk-highlight-overlay.test.ts b/packages/web/src/components/chapter/__tests__/hunk-highlight-overlay.test.ts new file mode 100644 index 0000000..c9fcad5 --- /dev/null +++ b/packages/web/src/components/chapter/__tests__/hunk-highlight-overlay.test.ts @@ -0,0 +1,232 @@ +// @vitest-environment happy-dom + +import { describe, expect, it } from "vitest"; +import { DIFF_SIDE } from "@/lib/diff-types"; +import { + findKeyChangeIdAtPoint, + getHighlightLineRect, + isPointInReviewStateBadge, + shouldIgnoreOverlayClick, +} from "../hunk-highlight-overlay"; +import { findRenderedDiffLine } from "../rendered-line-target"; + +describe("findRenderedDiffLine", () => { + it("finds split-view lines within the matching side column", () => { + const host = document.createElement("div"); + const shadowRoot = host.attachShadow({ mode: "open" }); + const additions = document.createElement("code"); + additions.setAttribute("data-code", ""); + additions.setAttribute("data-additions", ""); + const line = document.createElement("div"); + line.setAttribute("data-line", "12"); + additions.appendChild(line); + shadowRoot.appendChild(additions); + + expect(findRenderedDiffLine(shadowRoot, DIFF_SIDE.ADDITIONS, 12)).toBe(line); + }); + + it("finds unified-view lines by diff side instead of raw line number alone", () => { + const host = document.createElement("div"); + const shadowRoot = host.attachShadow({ mode: "open" }); + const unified = document.createElement("code"); + unified.setAttribute("data-code", ""); + unified.setAttribute("data-unified", ""); + + const deletionLine = document.createElement("div"); + deletionLine.setAttribute("data-line", "42"); + deletionLine.setAttribute("data-line-type", "change-deletion"); + unified.appendChild(deletionLine); + + const additionLine = document.createElement("div"); + additionLine.setAttribute("data-line", "42"); + additionLine.setAttribute("data-line-type", "change-addition"); + unified.appendChild(additionLine); + + shadowRoot.appendChild(unified); + + expect(findRenderedDiffLine(shadowRoot, DIFF_SIDE.ADDITIONS, 42)).toBe(additionLine); + expect(findRenderedDiffLine(shadowRoot, DIFF_SIDE.DELETIONS, 42)).toBe(deletionLine); + }); + + it("finds unified-view context lines for either side", () => { + const host = document.createElement("div"); + const shadowRoot = host.attachShadow({ mode: "open" }); + const unified = document.createElement("code"); + unified.setAttribute("data-unified", ""); + + const contextLine = document.createElement("div"); + contextLine.setAttribute("data-line", "50"); + contextLine.setAttribute("data-line-type", "context"); + unified.appendChild(contextLine); + + shadowRoot.appendChild(unified); + + expect(findRenderedDiffLine(shadowRoot, DIFF_SIDE.ADDITIONS, 50)).toBe(contextLine); + expect(findRenderedDiffLine(shadowRoot, DIFF_SIDE.DELETIONS, 50)).toBe(contextLine); + }); +}); + +describe("getHighlightLineRect", () => { + it("uses the line-number gutter and code content bounds for Pierre rows", () => { + const row = document.createElement("div"); + row.setAttribute("data-line", "12"); + const number = document.createElement("span"); + number.setAttribute("data-column-number", ""); + const content = document.createElement("span"); + content.setAttribute("data-column-content", ""); + row.append(number, content); + + row.getBoundingClientRect = () => DOMRect.fromRect({ x: 10, y: 20, width: 190, height: 20 }); + number.getBoundingClientRect = () => DOMRect.fromRect({ x: 10, y: 20, width: 48, height: 20 }); + content.getBoundingClientRect = () => + DOMRect.fromRect({ x: 58, y: 20, width: 142, height: 20 }); + + const rect = getHighlightLineRect(content); + + expect(rect.left).toBe(10); + expect(rect.right).toBe(200); + expect(rect.width).toBe(190); + }); + + it("matches Pierre's separate gutter and content cells by line index", () => { + const side = document.createElement("div"); + side.setAttribute("data-additions", ""); + const gutter = document.createElement("div"); + gutter.setAttribute("data-gutter", ""); + const content = document.createElement("div"); + content.setAttribute("data-content", ""); + const number = document.createElement("div"); + number.setAttribute("data-column-number", "56"); + number.setAttribute("data-line-index", "4"); + const line = document.createElement("div"); + line.setAttribute("data-line", "56"); + line.setAttribute("data-line-index", "4"); + gutter.append(number); + content.append(line); + side.append(gutter, content); + + number.getBoundingClientRect = () => DOMRect.fromRect({ x: 240, y: 40, width: 52, height: 20 }); + line.getBoundingClientRect = () => DOMRect.fromRect({ x: 292, y: 40, width: 220, height: 20 }); + + const rect = getHighlightLineRect(line); + + expect(rect.left).toBe(240); + expect(rect.right).toBe(512); + expect(rect.width).toBe(272); + }); +}); + +describe("findKeyChangeIdAtPoint", () => { + it("returns the matching key change when the click lands inside a box", () => { + expect( + findKeyChangeIdAtPoint(40, 30, [ + { + top: 20, + left: 10, + width: 80, + height: 40, + firstLineHeight: 20, + keyChangeId: "kc-1", + filePath: "src/foo.ts", + side: DIFF_SIDE.ADDITIONS, + startLine: 20, + endLine: 21, + isChecked: false, + }, + ]), + ).toBe("kc-1"); + }); + + it("returns null when the click lands outside every box", () => { + expect( + findKeyChangeIdAtPoint(200, 200, [ + { + top: 20, + left: 10, + width: 80, + height: 40, + firstLineHeight: 20, + keyChangeId: "kc-1", + filePath: "src/foo.ts", + side: DIFF_SIDE.ADDITIONS, + startLine: 20, + endLine: 21, + isChecked: false, + }, + ]), + ).toBeNull(); + }); +}); + +describe("isPointInReviewStateBadge", () => { + it("uses the rendered badge bounds instead of reconstructing them with magic offsets", () => { + expect( + isPointInReviewStateBadge( + 88, + 11, + { + top: 11, + left: 82, + width: 16, + height: 16, + }, + { + top: 8, + left: 10, + }, + ), + ).toBe(true); + }); + + it("returns false when the point lands outside the badge area", () => { + expect( + isPointInReviewStateBadge( + 40, + 30, + { + top: 11, + left: 82, + width: 16, + height: 16, + }, + { + top: 8, + left: 10, + }, + ), + ).toBe(false); + }); +}); + +describe("shouldIgnoreOverlayClick", () => { + it("ignores clicks when text is actively selected", () => { + expect( + shouldIgnoreOverlayClick([], { + isCollapsed: false, + toString: () => "selected code", + }), + ).toBe(true); + }); + + it("ignores clicks on interactive elements", () => { + const button = document.createElement("button"); + expect(shouldIgnoreOverlayClick([button], null)).toBe(true); + }); + + it("ignores clicks inside inline comment annotation content", () => { + const annotation = document.createElement("div"); + annotation.setAttribute("data-line-annotation", ""); + expect(shouldIgnoreOverlayClick([annotation], null)).toBe(true); + }); + + it("allows ordinary diff line clicks when no selection is active", () => { + const line = document.createElement("div"); + line.setAttribute("data-line", "42"); + expect( + shouldIgnoreOverlayClick([line], { + isCollapsed: true, + toString: () => "", + }), + ).toBe(false); + }); +}); diff --git a/packages/web/src/components/chapter/hunk-highlight-overlay.tsx b/packages/web/src/components/chapter/hunk-highlight-overlay.tsx index 9123bac..85579c8 100644 --- a/packages/web/src/components/chapter/hunk-highlight-overlay.tsx +++ b/packages/web/src/components/chapter/hunk-highlight-overlay.tsx @@ -67,6 +67,49 @@ function findLastVisibleLine( return null; } +function findLineRow(lineEl: HTMLElement): HTMLElement { + if (lineEl.hasAttribute("data-line")) return lineEl; + return lineEl.closest("[data-line]") ?? lineEl; +} + +function findLineNumberElement(row: HTMLElement): HTMLElement | null { + const nested = row.querySelector("[data-column-number]"); + if (nested) return nested; + + const lineIndex = row.getAttribute("data-line-index"); + if (!lineIndex) return null; + + const scope = row.closest("[data-additions], [data-deletions], [data-unified]"); + const root = scope ?? row.getRootNode(); + if (!(root instanceof Document || root instanceof ShadowRoot || root instanceof HTMLElement)) { + return null; + } + for (const candidate of root.querySelectorAll("[data-column-number]")) { + if (candidate.getAttribute("data-line-index") === lineIndex) return candidate; + } + return null; +} + +export function getHighlightLineRect(lineEl: HTMLElement): DOMRect { + const row = findLineRow(lineEl); + const rowRect = row.getBoundingClientRect(); + const numberRect = findLineNumberElement(row)?.getBoundingClientRect(); + const contentRect = + row.querySelector("[data-column-content]")?.getBoundingClientRect() ?? rowRect; + + if (!numberRect || !contentRect) return rowRect; + + const left = Math.min(numberRect.left, contentRect.left); + const right = Math.max(numberRect.right, contentRect.right); + + return DOMRect.fromRect({ + x: left, + y: rowRect.top, + width: right - left, + height: rowRect.height, + }); +} + /** * `getBoundingClientRect()` returns the container's border-box, but * `position: absolute` measures from the padding edge — subtract @@ -88,10 +131,12 @@ function measureLineRange( const lastEl = findLastVisibleLine(shadowRoot, lineRef.side, lineRef.startLine, lineRef.endLine); if (!firstEl || !lastEl) return null; - const firstRect = firstEl.getBoundingClientRect(); - const lastRect = lastEl.getBoundingClientRect(); + const firstRow = findLineRow(firstEl); + const lastRow = findLineRow(lastEl); + const firstRect = getHighlightLineRect(firstRow); + const lastRect = getHighlightLineRect(lastRow); let bottom = lastRect.bottom; - let trailingAnnotation = lastEl.nextElementSibling; + let trailingAnnotation = lastRow.nextElementSibling; while ( trailingAnnotation instanceof HTMLElement && diff --git a/packages/web/src/components/files/file-diff-list.tsx b/packages/web/src/components/files/file-diff-list.tsx index 97f49e0..a8a33b7 100644 --- a/packages/web/src/components/files/file-diff-list.tsx +++ b/packages/web/src/components/files/file-diff-list.tsx @@ -1,12 +1,15 @@ import { FileCode } from "lucide-react"; -import { forwardRef, useCallback, useImperativeHandle, useState } from "react"; +import { forwardRef, useCallback, useEffect, useImperativeHandle, useRef, useState } from "react"; import { FileHeader } from "@/components/chapter/file-header"; import { PierreDiffViewer } from "@/components/chapter/pierre-diff-viewer"; -import type { AnnotatedLineRef, LineRef } from "@/lib/diff-types"; +import { findRenderedDiffLine } from "@/components/chapter/rendered-line-target"; +import type { AnnotatedLineRef, DiffSide, LineRef } from "@/lib/diff-types"; import type { FileDiffEntry } from "@/lib/parse-diff"; export interface FileDiffListHandle { scrollToFile: (filePath: string) => void; + scrollToLine: (filePath: string, side: DiffSide, line: number) => void; + cancelScrollToLine: () => void; } export interface CollapseState { @@ -41,15 +44,115 @@ interface FileDiffListProps { } const FILE_TOP_PADDING = 16; +const SCROLL_TO_LINE_POLL_MS = 100; +const SCROLL_TO_LINE_TIMEOUT_MS = 3000; export const FileDiffList = forwardRef(function FileDiffList( { entries, emptyMessage, viewedPathSet, onToggleViewed, collapseState, chapterOverlay }, ref, ) { - useImperativeHandle( - ref, - () => ({ + const scrollRequestRef = useRef(0); + const pendingDisconnectsRef = useRef void>>(new Set()); + + useEffect(() => { + const pending = pendingDisconnectsRef.current; + return () => { + scrollRequestRef.current += 1; + for (const disconnect of pending) disconnect(); + pending.clear(); + }; + }, []); + + useImperativeHandle(ref, () => { + const cancelPending = () => { + scrollRequestRef.current += 1; + const pending = pendingDisconnectsRef.current; + for (const disconnect of pending) disconnect(); + pending.clear(); + }; + + const runWithContainer = ( + fileContainer: HTMLElement, + side: DiffSide, + line: number, + isLatestRequest: () => boolean, + ) => { + const tryScroll = () => { + if (!isLatestRequest()) return true; + + const diffsContainer = fileContainer.querySelector("diffs-container"); + const shadowRoot = diffsContainer?.shadowRoot; + if (!shadowRoot) return false; + + const lineEl = findRenderedDiffLine(shadowRoot, side, line); + if (!lineEl) return false; + if (lineEl.offsetParent === null) return false; + + lineEl.scrollIntoView({ behavior: "smooth", block: "center" }); + return true; + }; + + if (tryScroll()) return; + + let shadowObserver: MutationObserver | null = null; + let shadowRootRetryTimer: ReturnType | null = null; + let timeoutHandle: ReturnType | null = null; + + const disconnectAll = () => { + observer.disconnect(); + shadowObserver?.disconnect(); + if (shadowRootRetryTimer) clearInterval(shadowRootRetryTimer); + if (timeoutHandle) clearTimeout(timeoutHandle); + pendingDisconnectsRef.current.delete(disconnectAll); + }; + + const attachShadowObserver = (shadowRoot: ShadowRoot) => { + shadowObserver?.disconnect(); + shadowObserver = new MutationObserver(() => { + if (!isLatestRequest() || tryScroll()) disconnectAll(); + }); + shadowObserver.observe(shadowRoot, { + childList: true, + subtree: true, + }); + }; + + const observer = new MutationObserver(() => { + if (!isLatestRequest() || tryScroll()) disconnectAll(); + }); + observer.observe(fileContainer, { + childList: true, + subtree: true, + attributes: true, + attributeFilter: ["hidden"], + }); + pendingDisconnectsRef.current.add(disconnectAll); + + const existingShadowRoot = fileContainer.querySelector("diffs-container")?.shadowRoot; + if (existingShadowRoot) { + attachShadowObserver(existingShadowRoot); + } else { + shadowRootRetryTimer = setInterval(() => { + if (!isLatestRequest()) { + disconnectAll(); + return; + } + const shadowRoot = fileContainer.querySelector("diffs-container")?.shadowRoot; + if (!shadowRoot) return; + if (shadowRootRetryTimer) clearInterval(shadowRootRetryTimer); + shadowRootRetryTimer = null; + attachShadowObserver(shadowRoot); + tryScroll(); + }, SCROLL_TO_LINE_POLL_MS); + } + + timeoutHandle = setTimeout(disconnectAll, SCROLL_TO_LINE_TIMEOUT_MS); + }; + + return { + cancelScrollToLine: cancelPending, scrollToFile(filePath: string) { + cancelPending(); const el = document.getElementById(`file-${filePath}`); if (!el) return; const stickyOffset = parseFloat( @@ -59,9 +162,23 @@ export const FileDiffList = forwardRef(fu el.getBoundingClientRect().top + window.scrollY - stickyOffset - FILE_TOP_PADDING; window.scrollTo({ top }); }, - }), - [], - ); + scrollToLine(filePath: string, side: DiffSide, line: number) { + if (!entries.some((e) => e.file.path === filePath)) return; + + scrollRequestRef.current += 1; + const requestToken = scrollRequestRef.current; + const isLatestRequest = () => scrollRequestRef.current === requestToken; + + if (collapseState.collapsedFiles.has(filePath)) { + collapseState.toggleFileCollapsed(filePath); + } + + const fileContainer = document.getElementById(`file-${filePath}`); + if (!fileContainer) return; + runWithContainer(fileContainer, side, line, isLatestRequest); + }, + }; + }, [entries, collapseState]); if (entries.length === 0) { return ( diff --git a/packages/web/src/routes/chapter-detail-page.tsx b/packages/web/src/routes/chapter-detail-page.tsx index a25d5f4..4b445b6 100644 --- a/packages/web/src/routes/chapter-detail-page.tsx +++ b/packages/web/src/routes/chapter-detail-page.tsx @@ -144,9 +144,13 @@ function ChapterDetailContent({ chapter, chapterIndex, patch }: ChapterDetailCon const handleFocusKeyChange = useCallback( (keyChangeId: string | null, scrollTarget?: LineRef | null) => { setFocusedKeyChangeId(keyChangeId); + if (!keyChangeId) { + diffListRef.current?.cancelScrollToLine(); + return; + } const target = scrollTarget ?? findScrollTarget(chapter, keyChangeId); if (target) { - diffListRef.current?.scrollToFile(target.filePath); + diffListRef.current?.scrollToLine(target.filePath, target.side, target.startLine); } }, [chapter], From 2ab1bc8b393d1e86a8a3c0c21f0a51a577e51913 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 4 May 2026 19:01:28 -0700 Subject: [PATCH 2/3] fix(web): address hunk highlight review feedback --- .../web/src/components/chapter/hunk-highlight-overlay.tsx | 5 +---- packages/web/src/components/files/file-diff-list.tsx | 5 +++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/web/src/components/chapter/hunk-highlight-overlay.tsx b/packages/web/src/components/chapter/hunk-highlight-overlay.tsx index 85579c8..5df14ac 100644 --- a/packages/web/src/components/chapter/hunk-highlight-overlay.tsx +++ b/packages/web/src/components/chapter/hunk-highlight-overlay.tsx @@ -84,10 +84,7 @@ function findLineNumberElement(row: HTMLElement): HTMLElement | null { if (!(root instanceof Document || root instanceof ShadowRoot || root instanceof HTMLElement)) { return null; } - for (const candidate of root.querySelectorAll("[data-column-number]")) { - if (candidate.getAttribute("data-line-index") === lineIndex) return candidate; - } - return null; + return root.querySelector(`[data-column-number][data-line-index="${lineIndex}"]`); } export function getHighlightLineRect(lineEl: HTMLElement): DOMRect { diff --git a/packages/web/src/components/files/file-diff-list.tsx b/packages/web/src/components/files/file-diff-list.tsx index a8a33b7..337a4de 100644 --- a/packages/web/src/components/files/file-diff-list.tsx +++ b/packages/web/src/components/files/file-diff-list.tsx @@ -131,6 +131,7 @@ export const FileDiffList = forwardRef(fu const existingShadowRoot = fileContainer.querySelector("diffs-container")?.shadowRoot; if (existingShadowRoot) { attachShadowObserver(existingShadowRoot); + if (tryScroll()) disconnectAll(); } else { shadowRootRetryTimer = setInterval(() => { if (!isLatestRequest()) { @@ -142,7 +143,7 @@ export const FileDiffList = forwardRef(fu if (shadowRootRetryTimer) clearInterval(shadowRootRetryTimer); shadowRootRetryTimer = null; attachShadowObserver(shadowRoot); - tryScroll(); + if (tryScroll()) disconnectAll(); }, SCROLL_TO_LINE_POLL_MS); } @@ -163,9 +164,9 @@ export const FileDiffList = forwardRef(fu window.scrollTo({ top }); }, scrollToLine(filePath: string, side: DiffSide, line: number) { + cancelPending(); if (!entries.some((e) => e.file.path === filePath)) return; - scrollRequestRef.current += 1; const requestToken = scrollRequestRef.current; const isLatestRequest = () => scrollRequestRef.current === requestToken; From c0889002d8f45a8a7bc6dcf6df6ef170d878d220 Mon Sep 17 00:00:00 2001 From: Dean Stratakos <29683763+dastratakos@users.noreply.github.com> Date: Mon, 4 May 2026 19:18:21 -0700 Subject: [PATCH 3/3] fix(web): remove dead highlight guard --- packages/web/src/components/chapter/hunk-highlight-overlay.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/web/src/components/chapter/hunk-highlight-overlay.tsx b/packages/web/src/components/chapter/hunk-highlight-overlay.tsx index 5df14ac..01e5518 100644 --- a/packages/web/src/components/chapter/hunk-highlight-overlay.tsx +++ b/packages/web/src/components/chapter/hunk-highlight-overlay.tsx @@ -94,7 +94,7 @@ export function getHighlightLineRect(lineEl: HTMLElement): DOMRect { const contentRect = row.querySelector("[data-column-content]")?.getBoundingClientRect() ?? rowRect; - if (!numberRect || !contentRect) return rowRect; + if (!numberRect) return rowRect; const left = Math.min(numberRect.left, contentRect.left); const right = Math.max(numberRect.right, contentRect.right);