Skip to content

perf: lazy load document PDF reader#763

Closed
BigSimmo wants to merge 4 commits into
mainfrom
codex/merge-perf-lazy-pdf-clean
Closed

perf: lazy load document PDF reader#763
BigSimmo wants to merge 4 commits into
mainfrom
codex/merge-perf-lazy-pdf-clean

Conversation

@BigSimmo

@BigSimmo BigSimmo commented Jul 17, 2026

Copy link
Copy Markdown
Owner

Merges the frontend document-viewer PDF chunking optimization from performance task: lazy-load PDF reader only when needed.

Summary by CodeRabbit

  • Performance Improvements

    • PDF previews now load on demand, improving initial document-viewer load times.
    • A loading indicator is displayed while the PDF viewer is being prepared.
  • Bug Fixes

    • Improved compatibility by ensuring PDF preview components load only in the browser.

@supabase

supabase Bot commented Jul 17, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PDF preview components in DocumentViewer now load client-side through next/dynamic, using an accessible loading fallback. A Vitest test verifies that static PDF viewer imports are absent and that the fallback remains configured.

Changes

PDF viewer lazy loading

Layer / File(s) Summary
Dynamic PDF loading
src/components/DocumentViewer.tsx
PdfCanvasViewer and NativePdfEmbed use client-only dynamic imports with a shared PdfPreviewLoading fallback.
Lazy-loading regression coverage
tests/document-viewer-pdf-reader-lazy.test.ts
Source-level assertions verify dynamic loading, the fallback, and the absence of static PDF viewer imports.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested labels: codex

Suggested reviewers: claude

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is too sparse and omits the required template sections for Summary, Verification, Risk and rollout, and Notes. Add the required template sections and fill in Summary, Verification, Risk and rollout, Clinical Governance Preflight, and Notes.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: lazy-loading the document PDF reader for performance.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/merge-perf-lazy-pdf-clean

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install timed out. The project may have too many dependencies for the sandbox.


Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/document-viewer-pdf-reader-lazy.test.ts (1)

12-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert both dynamic wrappers and ssr: false.

The test only checks for one generic dynamic import and one fallback string. It would still pass if either PDF component were removed or if ssr: false were dropped; assert each named wrapper and its options explicitly.

🤖 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` around lines 12 - 16, Update
the test assertions around the dynamic PDF viewer imports to verify both named
wrappers, NativePdfEmbed and PdfCanvasViewer, are present with their respective
dynamic import paths and explicitly configured with ssr: false. Keep the
existing loading fallback checks, but assert each component’s wrapper and
options independently so removing either wrapper or its SSR setting causes the
test to fail.
🤖 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`:
- Around line 12-16: Update the test assertions around the dynamic PDF viewer
imports to verify both named wrappers, NativePdfEmbed and PdfCanvasViewer, are
present with their respective dynamic import paths and explicitly configured
with ssr: false. Keep the existing loading fallback checks, but assert each
component’s wrapper and options independently so removing either wrapper or its
SSR setting causes the test to fail.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0d321b0d-fe40-4101-a199-47a3ba3df461

📥 Commits

Reviewing files that changed from the base of the PR and between 85ae254 and 10dbf64.

📒 Files selected for processing (2)
  • src/components/DocumentViewer.tsx
  • tests/document-viewer-pdf-reader-lazy.test.ts

@BigSimmo
BigSimmo enabled auto-merge July 17, 2026 20:02
@BigSimmo BigSimmo mentioned this pull request Jul 17, 2026
8 tasks
@BigSimmo BigSimmo closed this Jul 17, 2026
auto-merge was automatically disabled July 17, 2026 20:14

Pull request was closed

@BigSimmo
BigSimmo deleted the codex/merge-perf-lazy-pdf-clean branch July 18, 2026 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants