diff --git a/package.json b/package.json index 79019211d..d436459a1 100644 --- a/package.json +++ b/package.json @@ -82,6 +82,7 @@ "devDependencies": { "@eslint/js": "^9.36.0", "@playwright/test": "^1.56.0", + "@storybook/addon-docs": "^10.0.0", "@storybook/addon-links": "^10.0.0", "@storybook/react-vite": "^10.0.0", "@storybook/test-runner": "^0.24.0", @@ -120,6 +121,7 @@ "eslint": "^9.36.0", "eslint-plugin-react": "^7.37.5", "eslint-plugin-react-hooks": "^5.2.0", + "eslint-plugin-storybook": "10.0.0", "eslint-plugin-tailwindcss": "4.0.0-beta.0", "jest": "^30.1.3", "mermaid": "^11.12.0", @@ -146,9 +148,7 @@ "typescript-eslint": "^8.45.0", "vite": "^7.1.11", "vite-plugin-svgr": "^4.5.0", - "vite-plugin-top-level-await": "^1.6.0", - "eslint-plugin-storybook": "10.0.0", - "@storybook/addon-docs": "^10.0.0" + "vite-plugin-top-level-await": "^1.6.0" }, "files": [ "dist/**/*.js", diff --git a/src/components/Messages/MarkdownComponents.tsx b/src/components/Messages/MarkdownComponents.tsx index 8b46c49d6..fb2b0303f 100644 --- a/src/components/Messages/MarkdownComponents.tsx +++ b/src/components/Messages/MarkdownComponents.tsx @@ -1,5 +1,5 @@ import type { ReactNode } from "react"; -import React, { useState, useEffect } from "react"; +import React, { useMemo } from "react"; import { Mermaid } from "./Mermaid"; import { getShikiHighlighter, @@ -7,6 +7,7 @@ import { SHIKI_THEME, } from "@/utils/highlighting/shikiHighlighter"; import { CopyButton } from "@/components/ui/CopyButton"; +import { useIntersectionHighlight } from "@/hooks/useIntersectionHighlight"; interface CodeProps { node?: unknown; @@ -58,21 +59,20 @@ function extractShikiLines(html: string): string[] { } /** - * CodeBlock component with async Shiki highlighting - * Displays code with line numbers in a CSS grid + * CodeBlock component with lazy async Shiki highlighting + * Displays code with line numbers in a CSS grid. + * Highlighting is deferred until the block enters the viewport. */ const CodeBlock: React.FC = ({ code, language }) => { - const [highlightedLines, setHighlightedLines] = useState(null); - // Split code into lines, removing trailing empty line - const plainLines = code - .split("\n") - .filter((line, idx, arr) => idx < arr.length - 1 || line !== ""); - - useEffect(() => { - let cancelled = false; + const plainLines = useMemo( + () => code.split("\n").filter((line, idx, arr) => idx < arr.length - 1 || line !== ""), + [code] + ); - async function highlight() { + // Lazy highlight when code block becomes visible + const { result: highlightedLines, ref } = useIntersectionHighlight( + async () => { try { const highlighter = await getShikiHighlighter(); const shikiLang = mapToShikiLang(language); @@ -89,30 +89,24 @@ const CodeBlock: React.FC = ({ code, language }) => { theme: SHIKI_THEME, }); - if (!cancelled) { - const lines = extractShikiLines(html); - // Remove trailing empty line if present - const filteredLines = lines.filter( - (line, idx, arr) => idx < arr.length - 1 || line.trim() !== "" - ); - setHighlightedLines(filteredLines.length > 0 ? filteredLines : null); - } + const lines = extractShikiLines(html); + // Remove trailing empty line if present + const filteredLines = lines.filter( + (line, idx, arr) => idx < arr.length - 1 || line.trim() !== "" + ); + return filteredLines.length > 0 ? filteredLines : null; } catch (error) { console.warn(`Failed to highlight code block (${language}):`, error); - if (!cancelled) setHighlightedLines(null); + return null; } - } - - void highlight(); - return () => { - cancelled = true; - }; - }, [code, language]); + }, + [code, language] + ); const lines = highlightedLines ?? plainLines; return ( -
+
{lines.map((content, idx) => ( diff --git a/src/components/shared/DiffRenderer.test.tsx b/src/components/shared/DiffRenderer.test.tsx index b1c155c05..649c44f59 100644 --- a/src/components/shared/DiffRenderer.test.tsx +++ b/src/components/shared/DiffRenderer.test.tsx @@ -1,10 +1,29 @@ /** * Tests for DiffRenderer components - * - * These are integration tests that verify the review note feature works end-to-end. - * We test the line extraction and formatting logic that ReviewNoteInput uses internally. */ + +// ============================================================================ +// Component Rendering Tests +// ============================================================================ +// TODO: Add React Testing Library tests for component rendering +// These tests would verify: +// - Plain diff renders immediately +// - Line numbers display correctly with custom oldStart/newStart +// - Line selection works before highlighting completes +// - Height consistency between plain and highlighted modes +// +// Note: RTL tests need proper DOM setup which is complex in bun test environment. +// Consider adding these as integration tests using Playwright or Cypress instead. +// +// For now, the critical functionality is covered by the review note logic tests below, +// and the component works correctly in manual testing. +// ============================================================================ + +// ============================================================================ +// Review Note Logic Tests +// ============================================================================ + describe("SelectableDiffRenderer review notes", () => { it("should extract correct line content for review notes", () => { // Simulate the internal review note building logic diff --git a/src/components/shared/DiffRenderer.tsx b/src/components/shared/DiffRenderer.tsx index ebd375f57..50d085872 100644 --- a/src/components/shared/DiffRenderer.tsx +++ b/src/components/shared/DiffRenderer.tsx @@ -4,7 +4,7 @@ * ReviewPanel uses SelectableDiffRenderer for interactive line selection. */ -import React, { useEffect, useState } from "react"; +import React, { useMemo } from "react"; import { cn } from "@/lib/utils"; import { getLanguageFromPath } from "@/utils/git/languageDetector"; import { Tooltip, TooltipWrapper } from "../Tooltip"; @@ -14,6 +14,7 @@ import { highlightSearchMatches, type SearchHighlightConfig, } from "@/utils/highlighting/highlightSearchTerms"; +import { useIntersectionHighlight } from "@/hooks/useIntersectionHighlight"; // Shared type for diff line types export type DiffLineType = "add" | "remove" | "context" | "header"; @@ -108,21 +109,18 @@ interface DiffRendererProps { } /** - * Hook to pre-process and highlight diff content in chunks - * Runs once when content/language changes (NOT search - that's applied post-process) + * Hook to lazily pre-process and highlight diff content in chunks + * Defers highlighting until the diff enters the viewport. + * Search decorations are applied post-highlight. */ function useHighlightedDiff( content: string, language: string, oldStart: number, newStart: number -): HighlightedChunk[] | null { - const [chunks, setChunks] = useState(null); - - useEffect(() => { - let cancelled = false; - - async function highlight() { +): { chunks: HighlightedChunk[] | null; ref: React.RefObject } { + const { result: chunks, ref } = useIntersectionHighlight( + async () => { // Split into lines const lines = content.split("\n").filter((line) => line.length > 0); @@ -134,19 +132,12 @@ function useHighlightedDiff( diffChunks.map((chunk) => highlightDiffChunk(chunk, language)) ); - if (!cancelled) { - setChunks(highlighted); - } - } - - void highlight(); - - return () => { - cancelled = true; - }; - }, [content, language, oldStart, newStart]); + return highlighted; + }, + [content, language, oldStart, newStart] + ); - return chunks; + return { chunks, ref }; } /** @@ -173,27 +164,71 @@ export const DiffRenderer: React.FC = ({ [filePath] ); - const highlightedChunks = useHighlightedDiff(content, language, oldStart, newStart); + const { chunks: highlightedChunks, ref } = useHighlightedDiff( + content, + language, + oldStart, + newStart + ); - // Show loading state while highlighting - if (!highlightedChunks) { - return ( -
-
Processing...
-
- ); - } + // Normalize data structure for rendering (highlighted or plain) + const normalizedLines = useMemo(() => { + if (highlightedChunks) { + // Use highlighted chunks + return highlightedChunks.flatMap((chunk) => + chunk.lines.map((line) => ({ + key: line.originalIndex, + type: chunk.type, + lineNumber: line.lineNumber, + indicator: chunk.type === "add" ? "+" : chunk.type === "remove" ? "-" : " ", + content: line.html, + isHighlighted: true, + })) + ); + } else { + // Fallback to plain lines with proper line numbering + const lines = content.split("\n").filter((line) => line.length > 0); + const result = []; + let oldLine = oldStart; + let newLine = newStart; + + for (let idx = 0; idx < lines.length; idx++) { + const line = lines[idx]; + const firstChar = line.charAt(0); + const type = (firstChar === "+" + ? "add" + : firstChar === "-" + ? "remove" + : firstChar === "@" + ? "header" + : "context") as DiffLineType; + + // For unified diffs, show new line number for additions/context, old for removals + const lineNumber = type === "remove" ? oldLine : newLine; + + result.push({ + key: idx, + type, + lineNumber, + indicator: firstChar, + content: line.substring(1), + isHighlighted: false, + }); + + // Update line counters (headers don't affect line numbers) + if (type !== "header") { + if (type !== "add") oldLine++; + if (type !== "remove") newLine++; + } + } + + return result; + } + }, [highlightedChunks, content, oldStart, newStart]); return (
= ({ gridTemplateColumns: "minmax(min-content, 1fr)", }} > - {highlightedChunks.flatMap((chunk) => - chunk.lines.map((line) => { - const indicator = chunk.type === "add" ? "+" : chunk.type === "remove" ? "-" : " "; - return ( -
( +
+
+ -
+ {showLineNumbers && ( + - - {indicator} - - {showLineNumbers && ( - - {line.lineNumber} - - )} - - - -
-
- ); - }) - )} + {line.lineNumber} + + )} + + {line.isHighlighted ? ( + + ) : ( + line.content + )} + +
+
+ ))}
); }; @@ -395,33 +429,84 @@ export const SelectableDiffRenderer = React.memo( [filePath] ); - const highlightedChunks = useHighlightedDiff(content, language, oldStart, newStart); + const { chunks: highlightedChunks, ref } = useHighlightedDiff( + content, + language, + oldStart, + newStart + ); - // Build lineData from highlighted chunks (memoized to prevent repeated parsing) - // Note: content field is NOT included - must be extracted from lines array when needed + // Build lineData for both highlighted and plain modes + // This ensures line selection works before highlighting completes const lineData = React.useMemo(() => { - if (!highlightedChunks) return []; + if (highlightedChunks) { + // Highlighted mode - use chunk data + const data: Array<{ + index: number; + type: DiffLineType; + lineNum: number; + html: string; + isHighlighted: boolean; + }> = []; + + highlightedChunks.forEach((chunk) => { + chunk.lines.forEach((line) => { + data.push({ + index: line.originalIndex, + type: chunk.type, + lineNum: line.lineNumber, + html: line.html, + isHighlighted: true, + }); + }); + }); - const data: Array<{ - index: number; - type: DiffLineType; - lineNum: number; - html: string; - }> = []; + return data; + } else { + // Plain mode - compute from raw lines with proper line numbering + const rawLines = content.split("\n").filter((line) => line.length > 0); + const data: Array<{ + index: number; + type: DiffLineType; + lineNum: number; + html: string; + isHighlighted: boolean; + }> = []; + let oldLine = oldStart; + let newLine = newStart; + + for (let idx = 0; idx < rawLines.length; idx++) { + const line = rawLines[idx]; + const firstChar = line.charAt(0); + const type = (firstChar === "+" + ? "add" + : firstChar === "-" + ? "remove" + : firstChar === "@" + ? "header" + : "context") as DiffLineType; + + // For unified diffs, show new line number for additions/context, old for removals + const lineNum = type === "remove" ? oldLine : newLine; - highlightedChunks.forEach((chunk) => { - chunk.lines.forEach((line) => { data.push({ - index: line.originalIndex, - type: chunk.type, - lineNum: line.lineNumber, - html: line.html, + index: idx, + type, + lineNum, + html: line.substring(1), + isHighlighted: false, }); - }); - }); - return data; - }, [highlightedChunks]); + // Update line counters (headers don't affect line numbers) + if (type !== "header") { + if (type !== "add") oldLine++; + if (type !== "remove") newLine++; + } + } + + return data; + } + }, [highlightedChunks, content, oldStart, newStart]); // Memoize highlighted line data to avoid re-parsing HTML on every render // Only recalculate when lineData or searchConfig changes @@ -476,28 +561,12 @@ export const SelectableDiffRenderer = React.memo( return index >= start && index <= end; }; - // Show loading state while highlighting - if (!highlightedChunks || highlightedLineData.length === 0) { - return ( -
-
Processing...
-
- ); - } - - // Extract lines for rendering (done once, outside map) - const lines = content.split("\n").filter((line) => line.length > 0); + // Extract raw lines for ReviewNoteInput (done once, outside map) + const lines = useMemo(() => content.split("\n").filter((line) => line.length > 0), [content]); return (
( )} - + {lineInfo.isHighlighted ? ( + + ) : ( + lineInfo.html + )}
diff --git a/src/hooks/useIntersectionHighlight.ts b/src/hooks/useIntersectionHighlight.ts new file mode 100644 index 000000000..c453db09f --- /dev/null +++ b/src/hooks/useIntersectionHighlight.ts @@ -0,0 +1,74 @@ +import { useEffect, useRef, useState } from "react"; + +/** + * Hook for lazy-loading expensive async operations using IntersectionObserver + * + * Defers execution of `highlightFn` until the element enters the viewport, + * improving performance by avoiding work for off-screen content. + * + * @example + * const { result, ref } = useIntersectionHighlight( + * async () => await expensiveOperation(), + * [dependency] + * ); + * return
{result ?? fallback}
; + */ +export function useIntersectionHighlight( + highlightFn: () => Promise, + deps: React.DependencyList +): { result: T | null; ref: React.RefObject } { + const [result, setResult] = useState(null); + const ref = useRef(null); + const hasHighlightedRef = useRef(false); + + useEffect(() => { + const element = ref.current; + if (!element) return; + + // Reset if dependencies change + hasHighlightedRef.current = false; + setResult(null); + + // Check if IntersectionObserver is supported (fallback for older browsers) + if (typeof IntersectionObserver === "undefined") { + void highlightFn().then((value) => { + setResult(value); + hasHighlightedRef.current = true; + }); + return; + } + + const observer = new IntersectionObserver( + ([entry]) => { + if (entry.isIntersecting && !hasHighlightedRef.current) { + hasHighlightedRef.current = true; + + void highlightFn() + .then((value) => { + setResult(value); + }) + .catch((error) => { + console.warn("Intersection highlight failed:", error); + }); + + // Disconnect after first highlight - no need to observe anymore + observer.disconnect(); + } + }, + { + // Start loading slightly before element enters viewport for smooth UX + rootMargin: "200px", + } + ); + + observer.observe(element); + + return () => { + observer.disconnect(); + }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, deps); + + return { result, ref }; +} +