-
-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add Find in PDF (Cmd+F) text search #102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dakl
wants to merge
9
commits into
main
Choose a base branch
from
claude/review-open-issues-Vz2GM
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
1cad6d2
feat: add Find in PDF (Cmd+F) text search
claude 7c6ec5c
test: add e2e test for PDF search with screenshots
claude 9c9c4e6
fix: rewrite pdf-search e2e test for CI reliability
claude e6da335
fix: use require() instead of import() in e2e evaluate
claude 9c111c3
fix: move PDF creation out of electronApp.evaluate
claude 763d77b
fix: remove flaky "Loading PDF..." assertion in e2e test
claude 6ec7105
fix: add missing _electron param in evaluate callback
claude 8d830ed
polish: find-in-PDF UX, toolbar layout, shortcuts, tooltips
dakl 5fa649b
fix: address 3 Greptile review bugs in PDF search
dakl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| import fs from 'fs'; | ||
| import os from 'os'; | ||
| import path from 'path'; | ||
| import { PDFDocument, rgb, StandardFonts } from 'pdf-lib'; | ||
| import { expect, test } from './fixtures'; | ||
|
|
||
| test('Cmd+F opens search bar in PDF viewer', { timeout: 60_000 }, async ({ electronApp, window }) => { | ||
| // Create the test PDF in the test's Node.js context (where imports work) | ||
| const doc = await PDFDocument.create(); | ||
| const font = await doc.embedFont(StandardFonts.Helvetica); | ||
| const page = doc.addPage([612, 792]); | ||
| const lines = [ | ||
| { text: 'Attention Is All You Need', size: 18 }, | ||
| { text: '', size: 11 }, | ||
| { text: 'Abstract', size: 14 }, | ||
| { text: '', size: 11 }, | ||
| { text: 'The dominant sequence transduction models are based on complex recurrent', size: 11 }, | ||
| { text: 'or convolutional neural networks that include an encoder and a decoder.', size: 11 }, | ||
| { text: 'We propose a new simple network architecture, the Transformer, based', size: 11 }, | ||
| { text: 'solely on attention mechanisms, dispensing with recurrence entirely.', size: 11 }, | ||
| ]; | ||
| let y = 720; | ||
| for (const line of lines) { | ||
| page.drawText(line.text, { x: 72, y, size: line.size, font, color: rgb(0, 0, 0) }); | ||
| y -= line.size + 6; | ||
| } | ||
| const pdfBytes = await doc.save(); | ||
|
|
||
| const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'papershelf-pdf-')); | ||
| const pdfPath = path.join(tmpDir, 'test-paper.pdf'); | ||
| fs.writeFileSync(pdfPath, pdfBytes); | ||
|
|
||
| // Seed the paper in the main process DB (only DB access needs evaluate) | ||
| // Note: electronApp.evaluate passes the Electron module as the first arg; | ||
| // the data argument comes second (see take-screenshots.ts for the pattern) | ||
| await electronApp.evaluate( | ||
| async (_electron, _params: { pdfPath: string }) => { | ||
| const db = (global as Record<string, any>).__papershelf_db; | ||
| db.insertPaper({ | ||
| arxivId: '1706.03762', | ||
| title: 'Attention Is All You Need', | ||
| authors: ['Ashish Vaswani', 'Noam Shazeer'], | ||
| abstract: 'The dominant sequence transduction models are based on complex recurrent neural networks.', | ||
| publishedDate: '2017-06-12T00:00:00Z', | ||
| updatedDate: '2017-06-12T00:00:00Z', | ||
| categories: ['cs.CL', 'cs.AI'], | ||
| arxivUrl: 'https://arxiv.org/abs/1706.03762', | ||
| pdfUrl: 'https://arxiv.org/pdf/1706.03762', | ||
| pdfPath: _params.pdfPath, | ||
| fullText: null, | ||
| }); | ||
| }, | ||
| { pdfPath }, | ||
| ); | ||
|
|
||
| // Navigate to library and click the paper | ||
| await window.getByRole('button', { name: 'My Library' }).click(); | ||
| await expect(window.getByText('Attention Is All You Need').first()).toBeVisible({ timeout: 5000 }); | ||
| await window.getByText('Attention Is All You Need').first().click(); | ||
|
|
||
| // Wait for PDF to fully render — the loading indicator is transient and may | ||
| // disappear before we can assert on it, so just wait for the text layer | ||
| await window.locator('.textLayer span').first().waitFor({ timeout: 30000 }); | ||
|
|
||
| // 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' }); | ||
|
|
||
| // Type a search query | ||
| await searchInput.fill('attention'); | ||
|
|
||
| // Wait for match counter to appear | ||
| await expect(window.getByText(/\d+ of \d+/)).toBeVisible({ timeout: 5000 }); | ||
|
|
||
| // Take screenshot showing search results with highlights | ||
| await window.screenshot({ path: 'test-results/pdf-search-results.png' }); | ||
|
|
||
| // Navigate to next match with Enter | ||
| await searchInput.press('Enter'); | ||
|
Check failure on line 88 in e2e/pdf-search.spec.ts
|
||
| await window.screenshot({ path: 'test-results/pdf-search-next-match.png' }); | ||
|
|
||
| // Close search with Escape | ||
| await searchInput.press('Escape'); | ||
| await expect(searchInput).not.toBeVisible(); | ||
|
|
||
| // Take screenshot after closing search (highlights should be gone) | ||
| await window.screenshot({ path: 'test-results/pdf-search-closed.png' }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,175 @@ | ||
| import { beforeEach, describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| vi.stubGlobal('localStorage', { | ||
| getItem: vi.fn(() => null), | ||
| setItem: vi.fn(), | ||
| removeItem: vi.fn(), | ||
| clear: vi.fn(), | ||
| length: 0, | ||
| key: vi.fn(() => null), | ||
| }); | ||
|
|
||
| // Mock electronAPI for store methods that call it | ||
| vi.stubGlobal('window', { | ||
| ...globalThis.window, | ||
| electronAPI: { | ||
| getShortcutOverrides: vi.fn(() => Promise.resolve({})), | ||
| saveShortcutOverrides: vi.fn(), | ||
| }, | ||
| }); | ||
|
|
||
| import { buildKeyString, formatKeys, getDefaultKeys, useShortcutStore } from '../stores/shortcutStore'; | ||
|
|
||
| function fakeKeyEvent(overrides: Partial<KeyboardEvent>): KeyboardEvent { | ||
| return { | ||
| key: '', | ||
| metaKey: false, | ||
| ctrlKey: false, | ||
| altKey: false, | ||
| shiftKey: false, | ||
| ...overrides, | ||
| } as KeyboardEvent; | ||
| } | ||
|
|
||
| beforeEach(() => { | ||
| useShortcutStore.getState().resetAll(); | ||
| }); | ||
|
|
||
| // --- buildKeyString --- | ||
|
|
||
| describe('buildKeyString', () => { | ||
| it('returns null for lone modifier presses', () => { | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'Meta', metaKey: true }))).toBeNull(); | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'Shift', shiftKey: true }))).toBeNull(); | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'Alt', altKey: true }))).toBeNull(); | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'Control', ctrlKey: true }))).toBeNull(); | ||
| }); | ||
|
|
||
| it('returns null when no modifier is held', () => { | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'f' }))).toBeNull(); | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'Enter' }))).toBeNull(); | ||
| }); | ||
|
|
||
| it('builds Meta+key strings', () => { | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'f', metaKey: true }))).toBe('Meta+f'); | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'b', metaKey: true }))).toBe('Meta+b'); | ||
| }); | ||
|
|
||
| it('lowercases single-char keys', () => { | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'F', metaKey: true }))).toBe('Meta+f'); | ||
| }); | ||
|
|
||
| it('preserves multi-char key names', () => { | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'Enter', metaKey: true }))).toBe('Meta+Enter'); | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'Escape', ctrlKey: true }))).toBe('Control+Escape'); | ||
| }); | ||
|
|
||
| it('combines multiple modifiers in order', () => { | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'g', metaKey: true, shiftKey: true }))).toBe('Meta+Shift+g'); | ||
| expect(buildKeyString(fakeKeyEvent({ key: 'z', metaKey: true, altKey: true }))).toBe('Meta+Alt+z'); | ||
| }); | ||
| }); | ||
|
|
||
| // --- formatKeys --- | ||
|
|
||
| describe('formatKeys', () => { | ||
| it('formats Meta as ⌘', () => { | ||
| expect(formatKeys('Meta+f')).toBe('⌘F'); | ||
| }); | ||
|
|
||
| it('formats Shift as ⇧', () => { | ||
| expect(formatKeys('Meta+Shift+g')).toBe('⌘⇧G'); | ||
| }); | ||
|
|
||
| it('formats Alt as ⌥', () => { | ||
| expect(formatKeys('Meta+Alt+z')).toBe('⌘⌥Z'); | ||
| }); | ||
|
|
||
| it('formats Control as ⌃', () => { | ||
| expect(formatKeys('Control+c')).toBe('⌃C'); | ||
| }); | ||
| }); | ||
|
|
||
| // --- Default shortcuts --- | ||
|
|
||
| describe('default shortcuts', () => { | ||
| it('includes findInPdf shortcut', () => { | ||
| const shortcut = useShortcutStore.getState().getShortcut('findInPdf'); | ||
| expect(shortcut).toBeDefined(); | ||
| expect(shortcut?.keys).toBe('Meta+f'); | ||
| expect(shortcut?.label).toBe('Find in PDF'); | ||
| }); | ||
|
|
||
| it('includes highlightSelection shortcut', () => { | ||
| const shortcut = useShortcutStore.getState().getShortcut('highlightSelection'); | ||
| expect(shortcut).toBeDefined(); | ||
| expect(shortcut?.keys).toBe('Meta+e'); | ||
| }); | ||
|
|
||
| it('returns undefined for unknown shortcut IDs', () => { | ||
| expect(useShortcutStore.getState().getShortcut('nonexistent')).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('getDefaultKeys returns the default for known IDs', () => { | ||
| expect(getDefaultKeys('findInPdf')).toBe('Meta+f'); | ||
| expect(getDefaultKeys('toggleSidebar')).toBe('Meta+b'); | ||
| }); | ||
|
|
||
| it('getDefaultKeys returns undefined for unknown IDs', () => { | ||
| expect(getDefaultKeys('nonexistent')).toBeUndefined(); | ||
| }); | ||
| }); | ||
|
|
||
| // --- Shortcut remapping --- | ||
|
|
||
| describe('shortcut remapping', () => { | ||
| it('remaps a shortcut and finds it by new keys', () => { | ||
| const store = useShortcutStore.getState(); | ||
| const result = store.setShortcutKeys('findInPdf', 'Meta+Shift+f'); | ||
| expect(result.success).toBe(true); | ||
|
|
||
| const updated = useShortcutStore.getState().getShortcut('findInPdf'); | ||
| expect(updated?.keys).toBe('Meta+Shift+f'); | ||
| }); | ||
|
|
||
| it('detects conflicts when remapping to an existing key', () => { | ||
| const store = useShortcutStore.getState(); | ||
| // Try to remap findInPdf to Meta+b which is toggleSidebar | ||
| const result = store.setShortcutKeys('findInPdf', 'Meta+b'); | ||
| expect(result.success).toBe(false); | ||
| expect(result.conflict).toBe('Toggle Sidebar'); | ||
| }); | ||
|
|
||
| it('resets a shortcut to default', () => { | ||
| const store = useShortcutStore.getState(); | ||
| store.setShortcutKeys('findInPdf', 'Meta+Shift+f'); | ||
| expect(useShortcutStore.getState().getShortcut('findInPdf')?.keys).toBe('Meta+Shift+f'); | ||
|
|
||
| useShortcutStore.getState().resetShortcut('findInPdf'); | ||
| expect(useShortcutStore.getState().getShortcut('findInPdf')?.keys).toBe('Meta+f'); | ||
| }); | ||
|
|
||
| it('resetAll restores all defaults', () => { | ||
| const store = useShortcutStore.getState(); | ||
| store.setShortcutKeys('findInPdf', 'Meta+Shift+f'); | ||
| store.setShortcutKeys('toggleSidebar', 'Meta+Shift+b'); | ||
|
|
||
| useShortcutStore.getState().resetAll(); | ||
|
|
||
| expect(useShortcutStore.getState().getShortcut('findInPdf')?.keys).toBe('Meta+f'); | ||
| expect(useShortcutStore.getState().getShortcut('toggleSidebar')?.keys).toBe('Meta+b'); | ||
| }); | ||
|
|
||
| it('remapped shortcut is found via key lookup (simulating keyboard dispatch)', () => { | ||
| useShortcutStore.getState().setShortcutKeys('findInPdf', 'Meta+Shift+f'); | ||
|
|
||
| const keyString = buildKeyString(fakeKeyEvent({ key: 'f', metaKey: true, shiftKey: true })); | ||
| const match = useShortcutStore.getState().shortcuts.find((s) => s.keys === keyString); | ||
| expect(match?.id).toBe('findInPdf'); | ||
|
|
||
| // Old key should no longer match findInPdf | ||
| const oldKeyString = buildKeyString(fakeKeyEvent({ key: 'f', metaKey: true })); | ||
| const oldMatch = useShortcutStore.getState().shortcuts.find((s) => s.keys === oldKeyString); | ||
| expect(oldMatch?.id).not.toBe('findInPdf'); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temporary directory created with
mkdtempSyncis 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...finallyblock.