Conversation
Add in-document text search to the PDF viewer. Pressing Cmd+F opens a search bar that finds and highlights matches across all rendered pages. - Case-insensitive search through pdfjs text layer spans - Yellow highlight overlays on all matches, orange for current match - Navigate with Enter/Shift+Enter or Cmd+G/Cmd+Shift+G - Match counter showing "N of M" results - Auto-scrolls to bring current match into view - Escape or close button dismisses the search bar - Dark mode support for highlight colors Closes #99 https://claude.ai/code/session_01PtqSxhcyfUARLFvQPjMbqt
Playwright test that seeds a paper with a generated test PDF, opens it, triggers Cmd+F search, types a query, and captures screenshots at each step. Screenshots are saved to test-results/ and uploaded as CI artifacts. https://claude.ai/code/session_01PtqSxhcyfUARLFvQPjMbqt
Greptile SummaryThis PR adds comprehensive Find in PDF functionality (Cmd+F) with real-time text search, yellow/orange highlighting, and keyboard navigation. The implementation includes toolbar redesign, custom tooltips, remappable shortcuts, and fixes for PDF font warnings. Key additions:
Issues found:
The core search logic is solid with good MutationObserver-based invalidation, but needs the three fixes above plus the issues from previous review threads (DOM ref stability, debouncing, accessibility, performance limits). Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant PdfViewer
participant ShortcutStore
participant PdfSearchBar
participant TextLayers as PDF Text Layers
participant DOM
User->>PdfViewer: Press Cmd+F
PdfViewer->>ShortcutStore: buildKeyString(event)
ShortcutStore-->>PdfViewer: "Meta+f"
PdfViewer->>PdfViewer: setShowSearch(true)
PdfViewer->>PdfSearchBar: Mount component
PdfSearchBar->>DOM: Focus input & select text
User->>PdfSearchBar: Type "attention"
PdfSearchBar->>PdfSearchBar: Auto-commit (2+ chars)
PdfSearchBar->>TextLayers: findMatchesInTextLayers()
TextLayers-->>PdfSearchBar: SearchMatch[]
PdfSearchBar->>DOM: highlightMatches()
DOM-->>User: Yellow highlights appear
User->>PdfSearchBar: Press Enter
PdfSearchBar->>PdfSearchBar: goToNext()
PdfSearchBar->>DOM: Update current highlight (orange)
PdfSearchBar->>DOM: scrollIntoView()
User->>PdfViewer: Pinch zoom
PdfViewer->>PdfViewer: Update scale & rebuild text layers
DOM->>PdfSearchBar: MutationObserver fires
PdfSearchBar->>TextLayers: Re-run search
PdfSearchBar->>DOM: Re-create highlights
User->>User: Press Cmd+G (window-level)
User->>PdfViewer: Cmd+G event
PdfViewer->>PdfSearchBar: searchNavRef.goToNext()
PdfSearchBar->>DOM: Navigate & scroll
User->>PdfSearchBar: Press Escape
PdfSearchBar->>PdfViewer: onClose()
PdfViewer->>PdfSearchBar: Unmount
PdfSearchBar->>DOM: clearHighlights()
Last reviewed commit: 5fa649b |
There was a problem hiding this comment.
Pull request overview
Adds in-document “Find in PDF” functionality to the renderer’s PDF viewer, enabling Cmd+F search with match highlighting and navigation to satisfy issue #99.
Changes:
- Introduces a
PdfSearchBaroverlay that scans pdfjs text-layer spans, highlights matches, and supports keyboard navigation/scroll-to-current. - Hooks Cmd+F / Escape handling into
PdfViewerand renders the search UI within the PDF scroll container. - Adds global CSS highlight styles plus an e2e test (and helper) that generates a searchable PDF and verifies Cmd+F behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/renderer/styles/globals.css |
Adds styles for search highlights (including dark mode). |
src/renderer/components/pdf/PdfSearchBar.tsx |
Implements DOM-based text search, highlighting, and navigation UI. |
src/renderer/components/PdfViewer.tsx |
Wires Cmd+F/Escape to toggle the search bar and positions it in the viewer. |
e2e/pdf-search.spec.ts |
Adds Playwright coverage for opening search, querying, navigating, and closing. |
e2e/helpers/create-test-pdf.ts |
Creates a minimal PDF with embedded text for reliable search testing. |
Comments suppressed due to low confidence (3)
e2e/pdf-search.spec.ts:60
- This test writes screenshots to hard-coded paths under
test-results/. In Playwright, parallel execution, retries, or multiple shards can cause filename collisions and flaky artifacts. Prefertest.info().outputPath(...)(or only capture screenshots conditionally on failure) so each run gets isolated output locations.
// Take a screenshot of the PDF viewer before search
await window.screenshot({ path: 'test-results/pdf-viewer-before-search.png' });
// Open search with Cmd+F
await window.keyboard.press('Meta+f');
// Search bar should be visible
const searchInput = window.getByPlaceholder('Find in PDF...');
await expect(searchInput).toBeVisible({ timeout: 3000 });
// Take screenshot showing search bar open
await window.screenshot({ path: 'test-results/pdf-search-bar-open.png' });
src/renderer/components/pdf/PdfSearchBar.tsx:90
highlightMatchesdoes repeated layout reads/writes for every match (creating a Range, callinggetClientRects(), and computingpageWrapper.getBoundingClientRect()inside the per-match loop). With many matches this can become a noticeable performance bottleneck. Consider grouping matches by page wrapper, computingwrapperRectonce per page, and batching DOM insertion (e.g., via aDocumentFragment) to reduce layout thrash.
function highlightMatches(matches: SearchMatch[], currentIndex: number) {
// Group by page wrapper to batch DOM reads
for (let i = 0; i < matches.length; i++) {
const match = matches[i];
const span = match.span;
const pageWrapper = span.closest('[data-page-index]') as HTMLElement;
if (!pageWrapper) continue;
// We need to measure where the matched substring is within the span.
// Use a Range to get the bounding rect of the matched characters.
const textNode = span.firstChild;
if (!textNode || textNode.nodeType !== Node.TEXT_NODE) continue;
const range = document.createRange();
try {
range.setStart(textNode, match.startOffset);
range.setEnd(textNode, match.startOffset + match.length);
} catch {
continue;
}
const rects = range.getClientRects();
const wrapperRect = pageWrapper.getBoundingClientRect();
for (const rect of rects) {
if (rect.width === 0 || rect.height === 0) continue;
const highlight = document.createElement('div');
highlight.className =
i === currentIndex ? 'pdf-search-highlight pdf-search-highlight-current' : 'pdf-search-highlight';
highlight.style.position = 'absolute';
highlight.style.left = `${rect.left - wrapperRect.left}px`;
highlight.style.top = `${rect.top - wrapperRect.top}px`;
highlight.style.width = `${rect.width}px`;
highlight.style.height = `${rect.height}px`;
e2e/pdf-search.spec.ts:17
afterAllunconditionally callsfs.rmSync(pdfDir, ...), butpdfDiris assigned inbeforeAll. IfbeforeAllfails or is skipped,pdfDirwill beundefinedandrmSyncwill throw, masking the real failure. Guard the cleanup (e.g., checkpdfDiris truthy and exists) before deleting.
test.afterAll(() => {
fs.rmSync(pdfDir, { recursive: true, force: true });
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setMatches(found); | ||
| setCurrentIndex(0); | ||
| }, [query, containerRef]); | ||
|
|
There was a problem hiding this comment.
SearchMatch stores direct references to pdfjs text-layer elements. The text layer is rebuilt via container.innerHTML = '' during re-renders (e.g., zoom/scale change), so these span references can become detached and the search counter/highlight navigation will stop working or show stale results. Consider re-running findMatchesInTextLayers when the text layer is regenerated (e.g., pass scale/pdfVersion into PdfSearchBar and add them to the dependency list, or attach a MutationObserver on the container/textLayer to recompute matches when spans change).
| // Re-run search when the PDF text layer is regenerated (e.g., zoom/scale changes) | |
| useEffect(() => { | |
| const container = containerRef.current; | |
| if (!container || !query) { | |
| return; | |
| } | |
| const observer = new MutationObserver(mutations => { | |
| // Only react to changes in the direct children of the container, which | |
| // indicates the text layer was likely rebuilt (e.g., via innerHTML = ''). | |
| const hasChildListChange = mutations.some(mutation => mutation.type === 'childList'); | |
| if (!hasChildListChange) { | |
| return; | |
| } | |
| const updatedContainer = containerRef.current; | |
| if (!updatedContainer) { | |
| return; | |
| } | |
| const found = findMatchesInTextLayers(updatedContainer, query); | |
| setMatches(found); | |
| setCurrentIndex(0); | |
| }); | |
| observer.observe(container, { | |
| childList: true, | |
| }); | |
| return () => { | |
| observer.disconnect(); | |
| }; | |
| }, [query, containerRef]); |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| <div ref={containerRef} className="flex-1 overflow-auto bg-gray-100 dark:bg-gray-900"> | ||
| <div ref={containerRef} className="relative flex-1 overflow-auto bg-gray-100 dark:bg-gray-900"> | ||
| {showSearch && <PdfSearchBar containerRef={containerRef} onClose={closeSearch} />} |
There was a problem hiding this comment.
search state persists when switching papers - if a user has search open with matches and switches to a different paper, the old match count displays but references detached DOM nodes. add a key prop that changes when paperId changes to force remount
Create the test PDF inside electronApp.evaluate (main process) to eliminate cross-process file path issues. Add intermediate waits for "Loading PDF..." state and explicit error checking before waiting for text layer. Increase timeouts. Remove unused helper file. https://claude.ai/code/session_01PtqSxhcyfUARLFvQPjMbqt
| interface SearchMatch { | ||
| /** The text layer <span> containing this match */ | ||
| span: HTMLElement; | ||
| /** Character offset within the span's textContent where the match starts */ | ||
| startOffset: number; | ||
| /** Length of the matched text */ | ||
| length: number; | ||
| /** Page index (0-based) */ | ||
| pageIndex: number; | ||
| } |
There was a problem hiding this comment.
Storing HTMLElement references in matches breaks when PDF re-renders (zoom, scale changes). Text layers are rebuilt in PdfPage.tsx line 199, creating new DOM nodes. Old span refs become detached, causing span.closest() at line 62 to return null, making highlights disappear.
Solution: Re-run search when scale changes, or store match positions by text content + page index instead of DOM refs.
Electron main process is CJS — dynamic import() fails with "A dynamic import callback was not specified". Switch to require(). https://claude.ai/code/session_01PtqSxhcyfUARLFvQPjMbqt
The sandboxed evaluate context has neither require() nor import(). Create the PDF in the test's Node.js context and pass the path into evaluate for DB seeding only — matching the pattern used by other e2e tests. https://claude.ai/code/session_01PtqSxhcyfUARLFvQPjMbqt
| <input | ||
| ref={inputRef} | ||
| type="text" | ||
| value={query} | ||
| onChange={(e) => setQuery(e.target.value)} | ||
| placeholder="Find in PDF..." | ||
| className="w-48 text-mac-small bg-transparent border-none outline-none placeholder-gray-400" |
There was a problem hiding this comment.
Add aria-label attributes to improve screen reader accessibility. The input has a placeholder but needs an aria-label, and the navigation buttons rely only on title attributes.
| <input | |
| ref={inputRef} | |
| type="text" | |
| value={query} | |
| onChange={(e) => setQuery(e.target.value)} | |
| placeholder="Find in PDF..." | |
| className="w-48 text-mac-small bg-transparent border-none outline-none placeholder-gray-400" | |
| <input | |
| ref={inputRef} | |
| type="text" | |
| value={query} | |
| onChange={(e) => setQuery(e.target.value)} | |
| placeholder="Find in PDF..." | |
| aria-label="Search PDF text" | |
| className="w-48 text-mac-small bg-transparent border-none outline-none placeholder-gray-400" | |
| /> |
| // Find matches when query changes | ||
| useEffect(() => { | ||
| const container = containerRef.current; | ||
| if (!container) return; | ||
|
|
||
| if (!query) { | ||
| setMatches([]); | ||
| setCurrentIndex(0); | ||
| return; | ||
| } | ||
|
|
||
| const found = findMatchesInTextLayers(container, query); | ||
| setMatches(found); | ||
| setCurrentIndex(0); | ||
| }, [query, containerRef]); |
There was a problem hiding this comment.
Consider debouncing the search input to improve performance on large PDFs. Currently, every keystroke triggers a full search through all text layers, which could cause lag on PDFs with many pages.
You could use a debounce delay of 150-300ms to wait for the user to stop typing before running the search.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
The loading indicator is transient and may disappear before the assertion runs, especially in CI. Wait directly for the text layer to render instead. https://claude.ai/code/session_01PtqSxhcyfUARLFvQPjMbqt
electronApp.evaluate passes the Electron module as the first arg to the callback — the data arg comes second. Without _electron as the first parameter, _params received the Electron module object and _params.pdfPath was undefined, so the PDF viewer never loaded. Also bumps test timeout to 60s for PDF rendering in CI. https://claude.ai/code/session_01PtqSxhcyfUARLFvQPjMbqt
| const handleKeyDown = useCallback( | ||
| (event: React.KeyboardEvent) => { | ||
| if (event.key === 'Escape') { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| onClose(); | ||
| } else if (event.key === 'Enter' && event.shiftKey) { | ||
| event.preventDefault(); | ||
| goToPrev(); | ||
| } else if (event.key === 'Enter') { | ||
| event.preventDefault(); | ||
| goToNext(); | ||
| } else if (event.key === 'g' && event.metaKey && event.shiftKey) { | ||
| event.preventDefault(); | ||
| goToPrev(); | ||
| } else if (event.key === 'g' && event.metaKey) { | ||
| event.preventDefault(); | ||
| goToNext(); | ||
| } | ||
| }, | ||
| [onClose, goToNext, goToPrev], | ||
| ); |
There was a problem hiding this comment.
Cmd+G/Cmd+Shift+G shortcuts only work when search bar has focus. The handleKeyDown is attached to the div's onKeyDown (line 188), not as a window-level listener. If a user clicks on the PDF after searching, focus moves away and Cmd+G navigation stops working.
Move Cmd+G handling to a window event listener in a useEffect (like Cmd+F in PdfViewer line 417), or keep focus trapped in the search bar.
| function highlightMatches(matches: SearchMatch[], currentIndex: number) { | ||
| // Group by page wrapper to batch DOM reads | ||
| for (let i = 0; i < matches.length; i++) { | ||
| const match = matches[i]; | ||
| const span = match.span; | ||
| const pageWrapper = span.closest('[data-page-index]') as HTMLElement; | ||
| if (!pageWrapper) continue; | ||
|
|
||
| // We need to measure where the matched substring is within the span. | ||
| // Use a Range to get the bounding rect of the matched characters. | ||
| const textNode = span.firstChild; | ||
| if (!textNode || textNode.nodeType !== Node.TEXT_NODE) continue; | ||
|
|
||
| const range = document.createRange(); | ||
| try { | ||
| range.setStart(textNode, match.startOffset); | ||
| range.setEnd(textNode, match.startOffset + match.length); | ||
| } catch { | ||
| continue; | ||
| } | ||
|
|
||
| const rects = range.getClientRects(); | ||
| const wrapperRect = pageWrapper.getBoundingClientRect(); | ||
|
|
||
| for (const rect of rects) { | ||
| if (rect.width === 0 || rect.height === 0) continue; | ||
| const highlight = document.createElement('div'); | ||
| highlight.className = | ||
| i === currentIndex ? 'pdf-search-highlight pdf-search-highlight-current' : 'pdf-search-highlight'; | ||
| highlight.style.position = 'absolute'; | ||
| highlight.style.left = `${rect.left - wrapperRect.left}px`; | ||
| highlight.style.top = `${rect.top - wrapperRect.top}px`; | ||
| highlight.style.width = `${rect.width}px`; | ||
| highlight.style.height = `${rect.height}px`; | ||
| highlight.style.pointerEvents = 'none'; | ||
| if (i === currentIndex) { | ||
| highlight.setAttribute('data-current', 'true'); | ||
| } | ||
| pageWrapper.appendChild(highlight); | ||
| } | ||
| } |
There was a problem hiding this comment.
Rendering hundreds/thousands of highlight divs could cause performance issues for common words. If a user searches for "the" in a large document, this creates a DOM element for each match without limits.
Consider capping visible highlights (e.g., only render highlights on currently visible pages) or adding a maximum highlight count with a warning.
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'papershelf-pdf-')); | ||
| const pdfPath = path.join(tmpDir, 'test-paper.pdf'); | ||
| fs.writeFileSync(pdfPath, pdfBytes); |
There was a problem hiding this comment.
Temporary directory created with mkdtempSync is not cleaned up after the test completes. Over many test runs, this could accumulate files in the temp directory.
Add cleanup in test teardown or use a try...finally block.
- Fix search bar disappearing on zoom by moving it outside scrollable container - Delay auto-search for single-char queries (require Enter) - Unify header: title+zoom on row 1, authors+icons on row 2 - Add search icon with ShortcutHint (⌘F) to toolbar - Wire keyboard shortcuts through store so remapping works - Add grouped Cmd-hold hint pills for zoom and find next/prev - Add Tooltip component and hover tooltips for all toolbar buttons - Fix PDF font warnings by providing cMapUrl to pdf.js - Bump max zoom from 300% to 1000% - Add findInPdf to default shortcuts - Add shortcut store unit tests (20 tests) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1. Stale matches on zoom: MutationObserver re-runs search when text
layers are rebuilt after zoom commit
2. Search state persists across papers: key={paperId} remounts
PdfSearchBar when switching papers
3. Cmd+G only works with focus: window-level keyboard handler in
PdfViewer now handles Cmd+G/Cmd+Shift+G via searchNavRef exposed
through onNavigate callback
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| if (event.key === '=' || event.key === '+') { | ||
| zoomIn(); | ||
| return; | ||
| } | ||
| if (event.key === '-') { | ||
| zoomOut(); | ||
| return; | ||
| } | ||
| if (event.key === '0') { | ||
| zoomReset(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
missing event.preventDefault() calls for zoom shortcuts - browser's default Cmd+/Cmd- zoom will interfere with app zoom
| if (event.key === '=' || event.key === '+') { | |
| zoomIn(); | |
| return; | |
| } | |
| if (event.key === '-') { | |
| zoomOut(); | |
| return; | |
| } | |
| if (event.key === '0') { | |
| zoomReset(); | |
| return; | |
| } | |
| if (event.metaKey) { | |
| if (event.key === '=' || event.key === '+') { | |
| event.preventDefault(); | |
| zoomIn(); | |
| return; | |
| } | |
| if (event.key === '-') { | |
| event.preventDefault(); | |
| zoomOut(); | |
| return; | |
| } | |
| if (event.key === '0') { | |
| event.preventDefault(); | |
| zoomReset(); | |
| return; | |
| } | |
| } |
| case 'highlightSelection': | ||
| event.preventDefault(); | ||
| toggleHighlightMode(); | ||
| break; |
There was a problem hiding this comment.
highlight shortcut fires in readonly mode - add guard to prevent annotation attempts on papers not in library
| case 'highlightSelection': | |
| event.preventDefault(); | |
| toggleHighlightMode(); | |
| break; | |
| case 'highlightSelection': | |
| if (!readOnly) { | |
| event.preventDefault(); | |
| toggleHighlightMode(); | |
| } | |
| break; |
|
|
||
| <div ref={containerRef} className="flex-1 overflow-auto bg-gray-100 dark:bg-gray-900"> | ||
| <div className="relative flex-1"> | ||
| {showSearch && <PdfSearchBar key={paperId} containerRef={containerRef} onClose={closeSearch} onNavigate={handleSearchNavigate} />} |
There was a problem hiding this comment.
key only uses paperId, but for papers without IDs (search results), the component won't remount when switching papers. Use a fallback like arxivId or pdfUrl to ensure remounting works for all paper types.
Summary
Add in-document text search to the PDF viewer and polish the toolbar UX.
Find in PDF
Toolbar redesign
Keyboard shortcuts
Other improvements
Test plan
Closes #99
🤖 Generated with Claude Code