From bcddced148d21ed8c6f8b7ce75ce2d1926e0bb14 Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Thu, 26 Mar 2026 22:24:50 -0700 Subject: [PATCH] perf(review): debounce search highlights and O(1) active match swap (#407) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(review): debounce search highlights and O(1) active match swap Split the single search highlight effect into two paths: 1. Full rebuild (debounced 100ms) — fires on query/matches/file/style changes, collapsing rapid keystrokes into one DOM update instead of one per character typed. 2. Active swap (instant) — fires when stepping between matches (Enter/Shift+Enter), changing two elements' background color via querySelector instead of tearing down and rebuilding every in the shadow DOM. Also removes augmentedDiff from the effect deps (was triggering unnecessary rebuilds on context expansion) and exports clearSearchHighlights for the immediate-clear path on Escape. Closes #405 For provenance purposes, this commit was AI assisted. * perf(review): debounce search query and pre-index searchable lines The search input was laggy on large PRs because findReviewSearchMatches re-parsed every file's patch and scanned all lines synchronously on every keystroke, blocking React from re-rendering the input value. Two complementary fixes: 1. Pre-index: Build SearchableLine[] once when files change (useMemo), with pre-lowercased normalizedText, so matching skips repeated patch parsing and per-line toLowerCase() calls. 2. Debounced query: searchQuery updates instantly (input responsive), debouncedSearchQuery trails by 200ms and drives all expensive work. During the pending window, sidebar shows "Searching…", diff clears stale highlights, match count hides, and stepping is disabled. Also: refactors findReviewSearchMatches to delegate to the new findMatchesInIndex so there is a single matching implementation, removes the now-redundant 100ms DOM debounce in DiffViewer (query debounce handles it upstream), trims query in highlightQuery to prevent whitespace-only edits from breaking snippet highlighting, and restores augmentedDiff in the highlight effect deps. For provenance purposes, this commit was AI assisted. --- packages/review-editor/App.tsx | 9 ++- .../review-editor/components/DiffViewer.tsx | 32 +++++++--- .../review-editor/components/FileTree.tsx | 17 +++-- .../review-editor/hooks/useReviewSearch.ts | 29 +++++++-- packages/review-editor/utils/reviewSearch.ts | 63 ++++++++++--------- .../utils/reviewSearchHighlight.ts | 26 +++++++- 6 files changed, 127 insertions(+), 49 deletions(-) diff --git a/packages/review-editor/App.tsx b/packages/review-editor/App.tsx index de8f7381..43e6bdf1 100644 --- a/packages/review-editor/App.tsx +++ b/packages/review-editor/App.tsx @@ -147,6 +147,8 @@ const ReviewApp: React.FC = () => { const { searchQuery, + debouncedSearchQuery, + isSearchPending, isSearchOpen, activeSearchMatchId, activeSearchMatch, @@ -336,7 +338,7 @@ const ReviewApp: React.FC = () => { } // Enter/F3 to step through search matches - if ((e.key === 'Enter' || e.key === 'F3') && searchMatches.length > 0 && !isTypingTarget(e.target)) { + if ((e.key === 'Enter' || e.key === 'F3') && searchMatches.length > 0 && !isSearchPending && !isTypingTarget(e.target)) { e.preventDefault(); stepSearchMatch(e.shiftKey ? -1 : 1); return; @@ -362,7 +364,7 @@ const ReviewApp: React.FC = () => { window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [showExportModal, showDestinationMenu, searchQuery, searchMatches, openSearch, stepSearchMatch, clearSearch, files, gitContext?.diffOptions]); + }, [showExportModal, showDestinationMenu, searchQuery, searchMatches, isSearchPending, openSearch, stepSearchMatch, clearSearch, files, gitContext?.diffOptions]); // Get annotations for active file const activeFileAnnotations = useMemo(() => { @@ -1337,6 +1339,7 @@ const ReviewApp: React.FC = () => { currentBranch={gitContext?.currentBranch} stagedFiles={stagedFiles} searchQuery={searchQuery} + isSearchPending={isSearchPending} searchInputRef={searchInputRef} onSearchChange={handleSearchInputChange} onSearchClear={clearSearch} @@ -1389,7 +1392,7 @@ const ReviewApp: React.FC = () => { onStage={() => stageFile(activeFile.path)} canStage={canStageFiles} stageError={stageError} - searchQuery={searchQuery} + searchQuery={isSearchPending ? '' : debouncedSearchQuery} searchMatches={activeFileSearchMatches} activeSearchMatchId={activeSearchMatchId} activeSearchMatch={activeSearchMatch?.filePath === activeFile.path ? activeSearchMatch : null} diff --git a/packages/review-editor/components/DiffViewer.tsx b/packages/review-editor/components/DiffViewer.tsx index 9b25456a..3d10e0f2 100644 --- a/packages/review-editor/components/DiffViewer.tsx +++ b/packages/review-editor/components/DiffViewer.tsx @@ -16,8 +16,10 @@ import { SuggestionModal } from './SuggestionModal'; import { type ReviewSearchMatch } from '../utils/reviewSearch'; import { applySearchHighlights, + clearSearchHighlights, getSearchRoots, retryScrollToSearchMatch, + swapActiveSearchHighlight, } from '../utils/reviewSearchHighlight'; interface DiffViewerProps { @@ -204,17 +206,33 @@ export const DiffViewer: React.FC = ({ return () => clearTimeout(timeoutId); }, [selectedAnnotationId]); - // Apply search highlights to diff lines (including inside shadow DOM) + // Apply search highlights to diff lines (including inside shadow DOM). + // The query is already debounced upstream (useReviewSearch), so this runs synchronously. + // activeSearchMatchId is NOT in deps — the swap effect handles that with O(1) updates. useEffect(() => { if (!containerRef.current) return; - const frameId = requestAnimationFrame(() => { - const roots = getSearchRoots(containerRef.current!); - roots.forEach(root => applySearchHighlights(root, searchQuery, searchMatches, activeSearchMatchId)); - }); + const query = searchQuery; + const matches = searchMatches; - return () => cancelAnimationFrame(frameId); - }, [searchQuery, searchMatches, activeSearchMatchId, filePath, diffStyle, augmentedDiff]); + if (!query.trim() || matches.length === 0) { + const roots = getSearchRoots(containerRef.current); + roots.forEach(root => clearSearchHighlights(root)); + return; + } + + const roots = getSearchRoots(containerRef.current); + roots.forEach(root => + applySearchHighlights(root, query, matches, activeSearchMatchId) + ); + }, [searchQuery, searchMatches, filePath, diffStyle, augmentedDiff]); + + // Swap active search highlight instantly when stepping between matches. + // This avoids a full rebuild just to change two elements' background color. + useEffect(() => { + if (!containerRef.current) return; + swapActiveSearchHighlight(containerRef.current, activeSearchMatchId); + }, [activeSearchMatchId]); // Scroll to active search match (with retry for lazy-rendered content) useEffect(() => { diff --git a/packages/review-editor/components/FileTree.tsx b/packages/review-editor/components/FileTree.tsx index 7dc23ca7..277ff467 100644 --- a/packages/review-editor/components/FileTree.tsx +++ b/packages/review-editor/components/FileTree.tsx @@ -27,6 +27,7 @@ interface FileTreeProps { currentBranch?: string; stagedFiles?: Set; searchQuery?: string; + isSearchPending?: boolean; searchInputRef?: React.RefObject; onSearchChange?: (value: string) => void; onSearchClear?: () => void; @@ -58,6 +59,7 @@ export const FileTree: React.FC = ({ currentBranch, stagedFiles, searchQuery = '', + isSearchPending, searchInputRef, onSearchChange, onSearchClear, @@ -166,7 +168,7 @@ export const FileTree: React.FC = ({ e.preventDefault(); return; } - if (e.key === 'Enter' && searchMatches.length > 0) { + if (e.key === 'Enter' && searchMatches.length > 0 && !isSearchPending) { e.preventDefault(); onStepSearchMatch?.(e.shiftKey ? -1 : 1); } @@ -184,7 +186,7 @@ export const FileTree: React.FC = ({ /> {searchQuery && (
- {searchQuery.trim() && ( + {searchQuery.trim() && !isSearchPending && ( {searchMatches.length} @@ -332,7 +334,11 @@ export const FileTree: React.FC = ({ {/* File tree or search results */}
{searchQuery.trim() ? ( - searchGroups.length > 0 ? ( + isSearchPending ? ( +
+ Searching… +
+ ) : searchGroups.length > 0 ? ( searchGroups.map((group) => ( = ({ // --- Search result components --- function highlightQuery(text: string, query: string) { - if (!query.trim()) return text; - const regex = new RegExp(`(${query.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')})`, 'gi'); + const trimmed = query.trim(); + if (!trimmed) return text; + const regex = new RegExp(`(${trimmed.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')})`, 'gi'); const parts = text.split(regex); // split with a capturing group puts matches at odd indices (1, 3, 5...) return parts.map((part, i) => diff --git a/packages/review-editor/hooks/useReviewSearch.ts b/packages/review-editor/hooks/useReviewSearch.ts index 56f881b8..96921e59 100644 --- a/packages/review-editor/hooks/useReviewSearch.ts +++ b/packages/review-editor/hooks/useReviewSearch.ts @@ -1,7 +1,8 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { - findReviewSearchMatches, groupReviewSearchMatches, + buildSearchIndex, + findMatchesInIndex, type ReviewSearchMatch, type ReviewSearchableDiffFile, } from '../utils/reviewSearch'; @@ -33,14 +34,29 @@ export function useReviewSearch({ clearPendingSelection, }: UseReviewSearchOptions) { const [searchQuery, setSearchQuery] = useState(''); + const [debouncedSearchQuery, setDebouncedSearchQuery] = useState(''); const [isSearchOpen, setIsSearchOpen] = useState(false); const [activeSearchMatchId, setActiveSearchMatchId] = useState(null as string | null); const searchInputRef = useRef(null as HTMLInputElement | null); + useEffect(() => { + if (!searchQuery.trim()) { + setDebouncedSearchQuery(''); + return; + } + const timeoutId = setTimeout(() => setDebouncedSearchQuery(searchQuery), 200); + return () => clearTimeout(timeoutId); + }, [searchQuery]); + + const isSearchPending = searchQuery.trim() !== '' + && searchQuery.trim().toLowerCase() !== debouncedSearchQuery.trim().toLowerCase(); + + const searchIndex = useMemo(() => buildSearchIndex(files), [files]); + const searchMatches = useMemo(() => { - return findReviewSearchMatches(files, searchQuery); - }, [files, searchQuery]); + return findMatchesInIndex(searchIndex, debouncedSearchQuery); + }, [searchIndex, debouncedSearchQuery]); const searchGroups = useMemo(() => { return groupReviewSearchMatches(files, searchMatches); @@ -68,6 +84,7 @@ export function useReviewSearch({ const clearSearch = useCallback(() => { setSearchQuery(''); + setDebouncedSearchQuery(''); setActiveSearchMatchId(null); setIsSearchOpen(false); }, []); @@ -95,7 +112,7 @@ export function useReviewSearch({ }, [searchMatches, activeSearchMatchId]); useEffect(() => { - if (!searchQuery.trim() || searchMatches.length === 0) { + if (!debouncedSearchQuery.trim() || searchMatches.length === 0) { setActiveSearchMatchId(null); return; } @@ -106,7 +123,7 @@ export function useReviewSearch({ if (!stillExists) { setActiveSearchMatchId(searchMatches[0].id); } - }, [searchQuery, searchMatches, activeSearchMatchId]); + }, [debouncedSearchQuery, searchMatches, activeSearchMatchId]); useEffect(() => { if (!activeSearchMatch) return; @@ -120,6 +137,8 @@ export function useReviewSearch({ return { searchQuery, + debouncedSearchQuery, + isSearchPending, isSearchOpen, activeSearchMatchId, activeSearchMatch, diff --git a/packages/review-editor/utils/reviewSearch.ts b/packages/review-editor/utils/reviewSearch.ts index 9e1fffac..f10fe04d 100644 --- a/packages/review-editor/utils/reviewSearch.ts +++ b/packages/review-editor/utils/reviewSearch.ts @@ -42,12 +42,13 @@ export interface ReviewSearchFileGroup { matches: ReviewSearchMatch[]; } -interface SearchableLine { +export interface SearchableLine { filePath: string; side: ReviewSearchSide; lineNumber: number; altLineNumber?: number; text: string; + normalizedText: string; } function isDiffMetadataLine(line: string): boolean { @@ -84,6 +85,7 @@ function buildSearchableLinesForPatch(file: ReviewSearchableDiffFile): Searchabl lineNumber: newLine, altLineNumber: oldLine, text, + normalizedText: text.toLowerCase(), }); continue; } @@ -95,6 +97,7 @@ function buildSearchableLinesForPatch(file: ReviewSearchableDiffFile): Searchabl side: 'deletion', lineNumber: oldLine, text, + normalizedText: text.toLowerCase(), }); continue; } @@ -106,6 +109,7 @@ function buildSearchableLinesForPatch(file: ReviewSearchableDiffFile): Searchabl side: 'addition', lineNumber: newLine, text, + normalizedText: text.toLowerCase(), }); } } @@ -113,6 +117,10 @@ function buildSearchableLinesForPatch(file: ReviewSearchableDiffFile): Searchabl return searchableLines; } +export function buildSearchIndex(files: ReviewSearchableDiffFile[]): SearchableLine[] { + return files.flatMap(file => buildSearchableLinesForPatch(file)); +} + function buildSnippet(text: string, start: number, end: number): string { const snippetStart = Math.max(0, start - SNIPPET_CONTEXT); const snippetEnd = Math.min(text.length, end + SNIPPET_CONTEXT); @@ -121,47 +129,46 @@ function buildSnippet(text: string, start: number, end: number): string { return `${prefix}${text.slice(snippetStart, snippetEnd)}${suffix}`; } -export function findReviewSearchMatches(files: ReviewSearchableDiffFile[], query: string): ReviewSearchMatch[] { +export function findMatchesInIndex(index: SearchableLine[], query: string): ReviewSearchMatch[] { const trimmedQuery = query.trim(); if (!trimmedQuery) return []; const normalizedQuery = trimmedQuery.toLowerCase(); const matches: ReviewSearchMatch[] = []; - files.forEach((file) => { - const searchableLines = buildSearchableLinesForPatch(file); + index.forEach((line) => { + let fromIndex = 0; + let matchCountForLine = 0; - searchableLines.forEach((line) => { - const normalizedText = line.text.toLowerCase(); - let fromIndex = 0; - let matchCountForLine = 0; + while (fromIndex <= line.normalizedText.length) { + const matchStart = line.normalizedText.indexOf(normalizedQuery, fromIndex); + if (matchStart === -1) break; - while (fromIndex <= normalizedText.length) { - const matchStart = normalizedText.indexOf(normalizedQuery, fromIndex); - if (matchStart === -1) break; + const matchEnd = matchStart + normalizedQuery.length; + matches.push({ + id: `${line.filePath}:${line.side}:${line.lineNumber}:${matchStart}:${matchCountForLine}`, + filePath: line.filePath, + side: line.side, + lineNumber: line.lineNumber, + altLineNumber: line.altLineNumber, + text: line.text, + matchStart, + matchEnd, + snippet: buildSnippet(line.text, matchStart, matchEnd), + }); - const matchEnd = matchStart + normalizedQuery.length; - matches.push({ - id: `${line.filePath}:${line.side}:${line.lineNumber}:${matchStart}:${matchCountForLine}`, - filePath: line.filePath, - side: line.side, - lineNumber: line.lineNumber, - altLineNumber: line.altLineNumber, - text: line.text, - matchStart, - matchEnd, - snippet: buildSnippet(line.text, matchStart, matchEnd), - }); - - matchCountForLine += 1; - fromIndex = matchStart + Math.max(1, normalizedQuery.length); - } - }); + matchCountForLine += 1; + fromIndex = matchStart + Math.max(1, normalizedQuery.length); + } }); return matches; } +export function findReviewSearchMatches(files: ReviewSearchableDiffFile[], query: string): ReviewSearchMatch[] { + return findMatchesInIndex(buildSearchIndex(files), query); +} + export function groupReviewSearchMatches( files: ReviewSearchableDiffFile[], matches: ReviewSearchMatch[], diff --git a/packages/review-editor/utils/reviewSearchHighlight.ts b/packages/review-editor/utils/reviewSearchHighlight.ts index 8f25aba8..48db10e8 100644 --- a/packages/review-editor/utils/reviewSearchHighlight.ts +++ b/packages/review-editor/utils/reviewSearchHighlight.ts @@ -28,7 +28,7 @@ export function getSearchRoots(root: ParentNode): ParentNode[] { return roots; } -function clearSearchHighlights(root: ParentNode) { +export function clearSearchHighlights(root: ParentNode) { const marks = root.querySelectorAll('mark[data-review-search-match]'); marks.forEach((mark) => { const parent = mark.parentNode; @@ -66,6 +66,11 @@ function decorateSearchMatch(mark: HTMLElement, isActive: boolean) { mark.style.borderRadius = '3px'; mark.style.padding = '0 1px'; mark.style.boxShadow = isActive ? ACTIVE_MATCH_RING : PASSIVE_MATCH_RING; + if (isActive) { + mark.dataset.reviewSearchActive = ''; + } else { + delete mark.dataset.reviewSearchActive; + } } function lineKey(match: ReviewSearchMatch): string { @@ -150,6 +155,25 @@ export function applySearchHighlights( }); } +export function swapActiveSearchHighlight( + container: HTMLElement, + newActiveId: string | null, +): void { + const roots = getSearchRoots(container); + for (const root of roots) { + const prev = root.querySelector('mark[data-review-search-active]') as HTMLElement | null; + if (prev) { + decorateSearchMatch(prev, false); + } + if (newActiveId) { + const next = root.querySelector(`mark[data-review-search-match="${newActiveId}"]`) as HTMLElement | null; + if (next) { + decorateSearchMatch(next, true); + } + } + } +} + export function scrollToSearchMatch(root: ParentNode, match: ReviewSearchMatch): boolean { const lineEl = root.querySelector(getLineSelector(match)) as HTMLElement | null; if (!lineEl) return false;