Synced from GitHub
test(web): PDF viewer behavior is covered by fast tests#25699
#25664 removes the flaky "navigates, searches, zooms, and selects mixed-height PDF pages" test, which left PdfViewer without tests. This restores that coverage with tests that stay fast under CPU contention.
What's covered
Unit tests (pdfLayout.test.ts, 14 tests, under 30 ms each):
- fit-width page width and scale, including a viewport narrower than its gutters
- zoom steps of 25% clamped to 25%..300%, starting from the fit scale
- page offsets for mixed page heights, at fit width and zoomed
- which page is current for a scroll position, at each page boundary, and that jumping to a page makes it current
- which pages stay mounted around the viewport
- page text joined from pdf.js items (line ends, marked content skipped)
- find highlights across text split into several spans, repeated matches, matches across a line break, blank queries
Browser test (PdfViewer.browser.test.tsx, one test): a generated two-page PDF (a short banner page whose title is drawn in two runs, and a letter page rotated 90°) checks:
- the first page canvas fills the viewport minus its gutters, with no horizontal scroll
- "quarterly outlook" reports 1/1 and highlights both spans, "Quarter" and "ly outlook"
- "budget" jumps to page 2 and the status shows "2 / 2 pages"
- the rotated page's text span runs top to bottom inside the page bounds, has
user-select: text, and selects as "Budget and risks" - a blank query reports 0/0
Every wait is on a milestone (the document's page count, the find status, the text layer span). None of them sleep.
Extraction
The layout, zoom, and find code in PdfViewer.tsx now lives in pure functions in pdfLayout.ts, moved without changing behavior. PdfViewer calls them where the inline code used to be.
Why the old test was slow, and the worker blob
Vite's dev transform serves the 1.3 MB pdf.worker.min.mjs as 6.8 MB with an inline source map. On one pinned core, fetching it took about 1.1 s, and booting the worker cost about 3 s of the old test. The new test mocks the worker's ?url import with a blob URL of the ?raw file. That's the same code without the dev-server cost.
Timings (local, taskset -c 0)
| Run | Browser test |
|---|---|
| unpinned | 1.0 s |
| pinned, alone | 1.3 s |
| pinned, two in parallel (two rounds) | 1.7, 2.4, 2.7, 2.7 s |
On the same setup, a version that also clicked Next/Previous/Zoom/Fit and used the transformed worker took 9.5 to 12.4 s. About 0.5 s of the current time is the viewer's own 250 ms find debounce, which runs twice.
Not covered
- clicking the page and zoom buttons (only the math behind them is tested)
- keeping the scroll position anchored on a page across a zoom change
- the 30 s load timeout, retry, and the 2,000-page and find-text size limits
🤖 Generated with Claude Code
Checks
23 successful · 16 skipped
Synced from GitHub
Approvability
Verdict: Approved at 0382ea0
Macroscope's review found this PR approvable — This PR adds focused unit and browser coverage for the PDF viewer while moving existing layout and search calculations into pure helpers. The production code’s behavior and defaults remain unchanged, with no deployment, schema, security, or static-analysis configuration impact.
Notes:
- All code in this push has already been reviewed. Approvability was decided on eligibility alone.
You can add or adjust custom eligibility rules. Learn more.
Added 2 commits
Synced from GitHub
No actionable comments were generated in the recent review.
External image Recent review info
-
External image Run configuration
- Source mode: Auto
- Repository: pingdotgg/t3code
- Branch: test/pdf-viewer-coverage
- File: .coderabbit.config.ts
- Commit: 0382ea0
- Layers: Repository TS config
- Review profile: CHILL
- Plan: Team
- Run ID:
f0a54620-a365-4a8d-972e-49c4599a9465
External image Commits
Reviewing files that changed from the base of the PR and between 1d272e48697543cbcfc30cf5691e7d368de2e179 and 0382ea0fcde282f5eb3cd8d505002df35baa0e4b.
External image Files selected for processing: 1
apps/web/src/components/files/pdfLayout.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
Walkthrough
The PDF viewer now uses shared helpers for page layout, navigation, zoom, and text search. New unit and browser tests cover layout calculations, search matching, rendering, and text selection.
Changes: PDF viewer behavior
Layer / File(s) Summary Layout, navigation, and zoom
apps/web/src/components/files/PdfViewer.tsx,apps/web/src/components/files/pdfLayout.ts,apps/web/src/components/files/pdfLayout.test.tsPage sizing, offsets, visibility, scroll positions, and zoom steps use shared helpers. Unit tests cover the calculations and their boundaries. Text search and rendering coverage
apps/web/src/components/files/pdfLayout.ts,apps/web/src/components/files/PdfViewer.tsx,apps/web/src/components/files/pdfLayout.test.ts,apps/web/src/components/files/PdfViewer.browser.test.tsxText extraction and match-run detection use shared helpers. Tests cover matches across runs and line breaks, PDF rendering, rotated-page search, text selection, and whitespace-only queries. Priority: External image Low
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Other
Suggested reviewers:
macroscopeapp
Merge Risk: External image Minimal · up to 0382e
No merge-blocking issue was identified; the PDF viewer tests can use the worker supplied by the installed dependency.
Pre-merge checks: Passed 4
Check name Status Explanation Title check Passed The title follows the conventional commit format and names the main change: PDF viewer tests were restored as fast tests. The changeset adds unit tests for pdfLayout helpers and a browser test for Pdf… Description check Passed The description follows the template's main structure. It has a problem statement (the removed flaky test left PdfViewer without coverage), a description of the change (unit tests, browser test, and e… Linked Issues check Passed Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request. Full details: Title check
Explanation
The title follows the conventional commit format and names the main change: PDF viewer tests were restored as fast tests. The changeset adds unit tests for pdfLayout helpers and a browser test for PdfViewer, and it moves layout and find logic into pdfLayout.ts. The title does not mention the logic extraction, but it does not need to cover every detail.
Full details: Description check
Explanation
The description follows the template's main structure. It has a problem statement (the removed flaky test left PdfViewer without coverage), a description of the change (unit tests, browser test, and extraction into pdfLayout.ts), and verification with local timings. It has no explicit 'Scope and approval' section. It does not link the triaged issue or maintainer approval, and the template asks for that. The 'Problem' heading has no clear reproduction or environment detail, though the flaky test and CPU contention are noted. It also lacks an agent or model attribution line, which the template asks for when an agent was used. The author did include a Claude Code attribution line.
Finishing Touches
📝 Generate docstrings
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
- Commit to this branch
- Create a new PR
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Synced from GitHub