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..01e5518 100644 --- a/packages/web/src/components/chapter/hunk-highlight-overlay.tsx +++ b/packages/web/src/components/chapter/hunk-highlight-overlay.tsx @@ -67,6 +67,46 @@ 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; + } + return root.querySelector(`[data-column-number][data-line-index="${lineIndex}"]`); +} + +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) 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 +128,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..337a4de 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,116 @@ 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); + if (tryScroll()) disconnectAll(); + } 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); + if (tryScroll()) disconnectAll(); + }, 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 +163,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) { + cancelPending(); + if (!entries.some((e) => e.file.path === filePath)) return; + + 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],