perf: lazy-load PDF reader in DocumentViewer#750
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
📝 WalkthroughWalkthrough
ChangesPDF preview loading
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant DocumentViewer
participant next_dynamic
participant PdfPreviewLoading
participant PdfCanvasViewer
DocumentViewer->>next_dynamic: request PDF preview component
next_dynamic->>PdfPreviewLoading: render loading fallback
next_dynamic->>PdfCanvasViewer: load component without SSR
PdfCanvasViewer-->>DocumentViewer: render PDF preview
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/document-viewer-pdf-reader-lazy.test.ts (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a regex assertion instead of exact string matching for the dynamic import check.
The
toContaincall embeds a literal\nand two-space indentation, making the test brittle against any formatting change (e.g., Prettier line-width adjustments). A regex-basedtoMatchpreserves the intent without coupling to whitespace.♻️ Proposed refactor for resilient assertion
- expect(viewerSource).toContain('dynamic(\n () => import("`@/components/document-viewer/pdf-canvas-viewer`")'); + expect(viewerSource).toMatch( + /dynamic\(\s*\(\)\s*=>\s*import\(["']@\/components\/document-viewer\/pdf-canvas-viewer["']\)/, + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/document-viewer-pdf-reader-lazy.test.ts` at line 15, Update the dynamic import assertion in the PDF reader lazy-loading test to use toMatch with a regex that verifies dynamic imports "`@/components/document-viewer/pdf-canvas-viewer`" while allowing arbitrary whitespace and line breaks, instead of relying on the exact formatted string passed to toContain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/document-viewer-pdf-reader-lazy.test.ts`:
- Line 15: Update the dynamic import assertion in the PDF reader lazy-loading
test to use toMatch with a regex that verifies dynamic imports
"`@/components/document-viewer/pdf-canvas-viewer`" while allowing arbitrary
whitespace and line breaks, instead of relying on the exact formatted string
passed to toContain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 877dcf66-65a0-49da-93c8-4a6efe72ce78
📒 Files selected for processing (2)
src/components/DocumentViewer.tsxtests/document-viewer-pdf-reader-lazy.test.ts
Motivation
Description
next/dynamicforPdfCanvasViewerandNativePdfEmbed, deferringpdfjs-distload until the reader is actually needed.PdfPreviewLoading(usesaria-busyand polite live region) to keep the UX stable while the reader chunk loads.src/components/DocumentViewer.tsx,tests/document-viewer-pdf-reader-lazy.test.ts.Testing
npm ci(Node 24.x / npm 11.x) and observed no audit vulnerabilities. — succeeded../node_modules/.bin/vitest run tests/document-viewer-pdf-reader-lazy.test.ts— passed (1/1).npm run test— completed successfully in this environment.npm run verify:cheap(includes lint, typecheck, and unit tests) — completed successfully;typecheckandeslintwere included and passed as part of that gate.npm run build:analyzeand exact before/after gzip measurement could not complete because the build process in this environment failed to fetch Google-hosted font resources (fonts.gstatic.com), blocking exact production bundle-size measurement; this is an external-network/environment limitation, not a code regression.If you want, I can push this branch and open a PR for you once an
originremote is available (the push attempt here failed because no remote is configured), and I can re-runnpm run build:analyzein an environment that can reach the font CDN to capture the precise gzip delta before merge.Codex Task
Summary by CodeRabbit