user can now change pdfs in the pdf viewer by InfinityBowman · Pull Request #88 · InfinityBowman/corates

InfinityBowman · 2025-12-18T20:19:15Z

Summary by CodeRabbit

New Features
- Multi-PDF selection and switching support in checklist and reconciliation views
- Optional PDF deletion controls for local checklists
- Improved PDF loading to maintain visibility during transitions
Style
- Streamlined PDF selector interface

_{✏️ Tip: You can customize this high-level summary in your review settings.}

cloudflare-workers-and-pages · 2025-12-18T20:19:24Z

Deploying with Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Preview URL	Updated (UTC)
✅ Deployment successful! View logs	corates	`9577364`	Commit Preview URL	Dec 18 2025, 08:29 PM

coderabbitai · 2025-12-18T20:19:32Z

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)

packages/landing/public/bimi.svg is excluded by !**/*.svg

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This PR refactors the PDF handling system across checklist and reconciliation UI components to support multi-PDF selection and enhanced PDF loading behavior. Changes include new props for PDF deletion control, multi-PDF switching, and introduction of docId-based DOM recreation in usePdfJs to properly handle PDF transitions while retaining prior PDF data during loading.

Changes

Cohort / File(s)	Summary
Build Configuration `.gitignore`	Updated ignore pattern from `docs/plans/` to `docs/plans` to match docs/plans directory and any path prefix starting with docs/plans.
Checklist Component Wrappers `packages/web/src/components/checklist-ui/ChecklistWithPdf.jsx`, `packages/web/src/components/checklist-ui/LocalChecklistView.jsx`	Added four new props to ChecklistWithPdf (allowDelete, pdfs, selectedPdfId, onPdfSelect) for multi-PDF support and delete button control. LocalChecklistView passes allowDelete={true} to enable deletion in local checklist context.
Checklist State Management `packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx`	Removed PDF deletion flow (handlePdfClear, deletePdf calls) and PdfSelector component. Simplified PDF state management by removing data clearing on PDF switch and adding explicit selectedPdfId tracking. Updated ChecklistWithPdf props and removed onPdfClear.
Reconciliation Components `packages/web/src/components/checklist-ui/compare/ReconciliationWithPdf.jsx`, `packages/web/src/components/checklist-ui/compare/ReconciliationWrapper.jsx`	ReconciliationWithPdf now forwards pdfs, selectedPdfId, and onPdfSelect props to PdfViewer. ReconciliationWrapper introduces selectedPdfId state, memoized studyPdfs/defaultPdf/currentPdf helpers, auto-selects default PDF on load, adds handlePdfSelect handler, and modifies loading flow to skip unnecessary reloads while preserving prior PDF data.
PDF UI Components `packages/web/src/components/checklist-ui/pdf/PdfSelector.jsx`, `packages/web/src/components/checklist-ui/pdf/PdfToolbar.jsx`, `packages/web/src/components/checklist-ui/pdf/PdfViewer.jsx`	PdfSelector simplified with streamlined menu items (value/label format) and unified selection handler. PdfToolbar adds allowDelete, pdfs, selectedPdfId, onPdfSelect props; integrates PdfSelector for multi-PDF display and conditionally shows delete button. PdfViewer updates rendering from Index to For with docId-based keys for DOM recreation and forwards new props to PdfToolbar.
PDF Loading Logic `packages/web/src/components/checklist-ui/pdf/usePdfJs.js`	Introduced docId state and loadedPdfName tracking for DOM recreation on PDF switch. Modified load logic to skip reload if same file already loaded, clear canvases and increment docId on new PDF load. Uses queueMicrotask for deferred rendering with captured scale. Exposes docId in return for consumers to react to PDF changes.

Sequence Diagram

sequenceDiagram
    participant User
    participant ReconciliationWrapper as ReconciliationWrapper<br/>(State Mgmt)
    participant PdfViewer as PdfViewer<br/>(Component)
    participant PdfToolbar as PdfToolbar<br/>(UI)
    participant usePdfJs as usePdfJs<br/>(Load Logic)
    participant Cache as PDF Cache

    User->>ReconciliationWrapper: Select PDF (onPdfSelect)
    activate ReconciliationWrapper
    ReconciliationWrapper->>ReconciliationWrapper: Update selectedPdfId<br/>Derive currentPdf from<br/>studyPdfs list
    ReconciliationWrapper->>PdfViewer: Pass pdfs, selectedPdfId,<br/>onPdfSelect props
    deactivate ReconciliationWrapper

    activate PdfViewer
    PdfViewer->>PdfToolbar: Forward multi-PDF props
    deactivate PdfViewer

    activate PdfToolbar
    PdfToolbar->>User: Render PdfSelector<br/>with PDF options
    activate User
    User->>PdfToolbar: Confirm selection
    deactivate User
    PdfToolbar->>PdfViewer: Trigger onPdfSelect callback
    deactivate PdfToolbar

    activate usePdfJs
    usePdfJs->>usePdfJs: Check loadedPdfName<br/>vs. new PDF name
    alt Same file already loaded
        usePdfJs->>User: Return (skip reload)
    else New PDF file
        usePdfJs->>usePdfJs: Clear canvases<br/>Increment docId
        usePdfJs->>Cache: getCachedPdf(fileName)
        activate Cache
        Cache-->>usePdfJs: pdfData (or null)
        deactivate Cache
        alt Cache hit
            usePdfJs->>usePdfJs: Load from cache
        else Cache miss
            usePdfJs->>Cache: downloadPdf(fileName)
            Cache-->>usePdfJs: pdfData
        end
        usePdfJs->>usePdfJs: Update loadedPdfName<br/>Queue deferred render<br/>with docId key
        usePdfJs->>PdfViewer: Trigger DOM recreation<br/>(docId-based keys)
        activate PdfViewer
        PdfViewer->>User: Render new PDF pages
        deactivate PdfViewer
    end
    deactivate usePdfJs

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

ReconciliationWrapper: Introduces new state management (selectedPdfId, memoized helpers) and reworks PDF loading flow with multi-step fallback logic—requires careful review of selection/loading interactions
usePdfJs: Significant changes to load sequence and timing with docId/loadedPdfName tracking, queueMicrotask deferral, and DOM recreation logic—critical path requiring thorough validation
Heterogeneous scope: Mix of simple prop forwarding (ChecklistWithPdf, LocalChecklistView) and substantial refactoring (ChecklistYjsWrapper removal of deletion flow, PdfSelector simplification) creates varied reasoning requirements
Cross-component threading: Props propagate through multiple layers (ChecklistWithPdf → PdfViewer → PdfToolbar → PdfSelector); verify all handler chains work correctly
State transition edge cases: Verify that PDF switching, re-selection of same file, and data retention during transitions work as intended across all wrapper components

Possibly related PRs

PR #79: Modifies the same PDF UI codepaths (PdfViewer, PdfToolbar, usePdfJs, checklist wrappers) to add multi-PDF selection props and change selection/loading/deletion wiring
PR #57: Alters usePdfJs with loadingSourceId and ArrayBuffer cloning for safe PDF.js loads, sharing the same critical PDF loading logic file
PR #83: Implements multi-PDF support with related props (pdfs, selectedPdfId, onPdfSelect) and study-card PDF list modifications at the code level

Poem

🐰 With docId hops and selectors spry,
PDF pages dance and multiply!
No more deletion, just a thoughtful view—
When PDFs swap, the DOM's brand new! ✨
Multi-choice magic, smooth and clean,
The finest checklist ever seen! 📋

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main functionality added: users can now select and switch between multiple PDFs in the PDF viewer, which is the primary change across all modified components.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (3)

packages/web/src/components/checklist-ui/pdf/PdfSelector.jsx (1)
16-25: Consider extracting shared PDF sorting logic.

The sorting logic here duplicates the implementation in PdfList.jsx (lines 25-35). To align with DRY principles, consider extracting this into a shared utility function.
Example extraction
// In a shared utility, e.g., packages/web/src/utils/pdfUtils.js
export function sortPdfsByTag(pdfs) {
  const tagOrder = { primary: 0, protocol: 1, secondary: 2 };
  return [...pdfs].sort((a, b) => {
    const tagA = tagOrder[a.tag] ?? 2;
    const tagB = tagOrder[b.tag] ?? 2;
    return tagA - tagB;
  });
}
packages/web/src/components/checklist-ui/compare/ReconciliationWrapper.jsx (1)
76-82: Consider guarding against stale selection.

The auto-select effect sets selectedPdfId when defaultPdf is available and no selection exists. However, if a previously selected PDF is removed from studyPdfs, the selectedPdfId signal will retain a stale ID. While currentPdf falls back to defaultPdf() in this case (line 68), the stale ID persists in state.

Consider clearing selectedPdfId when the selected PDF no longer exists in studyPdfs:
Optional improvement
  createEffect(() => {
    const pdf = defaultPdf();
-   if (pdf && !selectedPdfId()) {
+   const selected = selectedPdfId();
+   const pdfs = studyPdfs();
+   
+   // Clear stale selection if PDF no longer exists
+   if (selected && !pdfs.find(p => p.id === selected)) {
+     setSelectedPdfId(null);
+   }
+   
+   // Auto-select default if no valid selection
+   if (pdf && !selectedPdfId()) {
      setSelectedPdfId(pdf.id);
    }
  });
packages/web/src/components/checklist-ui/pdf/usePdfJs.js (1)
223-233: Consider strengthening source identification for concurrent load prevention.

Using byteLength as a source identifier is weak—two different PDFs could have identical sizes. While the risk is low (the identifier is cleared after loading completes), rapid switching between same-size PDFs could theoretically skip a load.

Consider hashing a portion of the data or using a counter/timestamp for uniqueness:
Suggested approach
+let loadCounter = 0;
+
 async function loadPdf(source) {
   if (!pdfjsLib) return;

-  // Generate a unique ID for this source to prevent duplicate loads
-  const sourceId = source.data ? source.data.byteLength : JSON.stringify(source);
+  // Generate a unique ID for this load operation
+  const sourceId = ++loadCounter;

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 38a48f2 and 36f9bad.

📒 Files selected for processing (10)

.gitignore (1 hunks)
packages/web/src/components/checklist-ui/ChecklistWithPdf.jsx (2 hunks)
packages/web/src/components/checklist-ui/ChecklistYjsWrapper.jsx (4 hunks)
packages/web/src/components/checklist-ui/LocalChecklistView.jsx (1 hunks)
packages/web/src/components/checklist-ui/compare/ReconciliationWithPdf.jsx (2 hunks)
packages/web/src/components/checklist-ui/compare/ReconciliationWrapper.jsx (3 hunks)
packages/web/src/components/checklist-ui/pdf/PdfSelector.jsx (1 hunks)
packages/web/src/components/checklist-ui/pdf/PdfToolbar.jsx (3 hunks)
packages/web/src/components/checklist-ui/pdf/PdfViewer.jsx (6 hunks)
packages/web/src/components/checklist-ui/pdf/usePdfJs.js (7 hunks)

🧰 Additional context used

📓 Path-based instructions (8)

**/*.{jsx,tsx,js,ts}