add robustness to fetching logic - #93
Conversation
WalkthroughIntroduces a timeout utility and refactors PDF extraction into staged, timeout-protected steps; adds per-PDF metadataLoading and error states, a retryPdfExtraction API, and UI branches for extracting/error/success with per-item loading and retry controls. Changes
Sequence DiagramsequenceDiagram
participant UI as User/UI
participant Hook as useAddStudies (handler)
participant Reader as FileReader
participant Extractor as pdfUtils (extractors)
participant Timeout as withTimeout
participant DoiAPI as DOI Metadata API
participant State as PDF State
UI->>Hook: Select PDF
Hook->>Reader: Read file blob
Reader-->>Hook: file data / error
Hook->>Timeout: wrap extractPdfTitle(file) (PDF_EXTRACT_TIMEOUT)
Timeout->>Extractor: run extractPdfTitleInternal
Extractor-->>Timeout: title / error
Timeout-->>Hook: title result
Hook->>State: set title / set extracting state
Hook->>Timeout: wrap extractPdfDoi(file) (PDF_EXTRACT_TIMEOUT)
Timeout->>Extractor: run extractPdfDoiInternal
Extractor-->>Timeout: doi / none
Timeout-->>Hook: doi result
alt DOI found
Hook->>State: set doi, set metadataLoading=true
par Background DOI metadata fetch
Hook->>DoiAPI: fetch metadata (withTimeout DOI_FETCH_TIMEOUT)
DoiAPI-->>Hook: metadata / error
Hook->>State: update metadata fields, set metadataLoading=false
end
else No DOI / extraction error
Hook->>State: set error
UI->>User: show error state + Retry button
end
UI->>Hook: Retry button clicked
Hook->>Hook: retryPdfExtraction(id) -> rerun extraction flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (9)**/*.{js,jsx,ts,tsx,md,json,sql}📄 CodeRabbit inference engine (.cursorrules)
Files:
packages/web/src/**/*.{jsx,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
packages/web/src/**/*.{jsx,tsx,js,ts}📄 CodeRabbit inference engine (.cursorrules)
Files:
**/*.{jsx,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
packages/web/src/components/**/*.{jsx,tsx}📄 CodeRabbit inference engine (.cursorrules)
Files:
packages/web/src/**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/web/src/components/**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
packages/web/src/components/**📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (4)📚 Learning: 2025-12-19T01:58:03.919ZApplied to files:
📚 Learning: 2025-12-18T22:13:00.528ZApplied to files:
📚 Learning: 2025-12-19T01:58:03.919ZApplied to files:
📚 Learning: 2025-12-18T22:13:00.529ZApplied to files:
🧬 Code graph analysis (1)packages/web/src/lib/pdfUtils.js (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (14)
Comment |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
corates | 87c620a | Commit Preview URL | Dec 19 2025, 01:58 AM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/web/src/primitives/useAddStudies.js (2)
241-264: Background DOI fetch may update state after component unmount.The fire-and-forget pattern for DOI metadata fetching (lines 243-263) could attempt to update state after the component unmounts, potentially causing warnings or unexpected behavior in development mode.
Consider tracking the fetch with an AbortController or checking if the PDF still exists before updating state.
Defensive check before state update
withTimeout(fetchFromDOI(doi), DOI_FETCH_TIMEOUT, 'DOI metadata fetch') .then(refData => { + // Check if PDF still exists before updating + const stillExists = uploadedPdfs.find(p => p.id === pdf.id); + if (!stillExists) return; + if (refData) { setUploadedPdfs(p => p.id === pdf.id, {
268-311: Fragile duplicate merging approach in retryPdfExtraction.The current implementation calls
handlePdfSelectwhich creates a new entry, then manually merges the duplicate back. This is complex and could have race conditions ifhandlePdfSelectis async and multiple retries happen simultaneously.Consider extracting the extraction logic into a shared helper function that both
handlePdfSelectandretryPdfExtractioncan call directly, avoiding the duplicate creation/merging pattern.Proposed refactor outline
// Extract core extraction logic const extractPdfMetadata = async (pdfId, file) => { // Reset state setUploadedPdfs(p => p.id === pdfId, { extracting: true, error: null, // ... }); // Perform extraction steps (current lines 197-264) // ... }; // Use in handlePdfSelect for (const pdf of newPdfs) { await extractPdfMetadata(pdf.id, pdf.file); } // Use in retryPdfExtraction const retryPdfExtraction = async id => { const pdf = uploadedPdfs.find(p => p.id === id); if (!pdf?.file) return; await extractPdfMetadata(id, pdf.file); };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/web/src/components/project-ui/add-studies/PdfUploadSection.jsx(3 hunks)packages/web/src/lib/pdfUtils.js(4 hunks)packages/web/src/primitives/useAddStudies.js(4 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx,md,json,sql}
📄 CodeRabbit inference engine (.cursorrules)
Do not use emojis in code, comments, documentation, or commit messages
Files:
packages/web/src/lib/pdfUtils.jspackages/web/src/components/project-ui/add-studies/PdfUploadSection.jsxpackages/web/src/primitives/useAddStudies.js
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use aliases for imports when appropriate to improve readability
Prefer using config files rather than hardcoding values
Each file should handle one coherent responsibility
Use Zod for schema and input validation
**/*.{js,jsx,ts,tsx}: Prefer modern ES6+ syntax and features
Use Zod for schema and input validation
Files:
packages/web/src/lib/pdfUtils.jspackages/web/src/components/project-ui/add-studies/PdfUploadSection.jsxpackages/web/src/primitives/useAddStudies.js
packages/web/src/**/*.{jsx,tsx,js,ts}
📄 CodeRabbit inference engine (.cursorrules)
Ensure browser compatibility for all frontend code, particularly for Safari
packages/web/src/**/*.{jsx,tsx,js,ts}: For UI icons, use thesolid-iconslibrary or SVGs only. Do not use emojis
Ensure browser compatibility for all frontend code with special attention to Safari
Use createMemo for derived values in SolidJS and createStore for complex state to ensure reactivity and performance
Files:
packages/web/src/lib/pdfUtils.jspackages/web/src/components/project-ui/add-studies/PdfUploadSection.jsxpackages/web/src/primitives/useAddStudies.js
packages/web/src/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/**/*.{jsx,tsx}: For UI icons, use thesolid-iconslibrary or SVGs only, do not use emojis
Use responsive design principles for UI components
Do NOT prop-drill application state. Shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
Components should receive at most 1–5 props, and only for local configuration, not shared state
Do not destructure props in SolidJS components as it breaks reactivity. Instead, access props directly or wrap them in a function to ensure reactivity
Use createMemo for derived values in SolidJS to ensure reactivity
Use Solid's createStore for complex state or state objects for better performance and reactivity
Create reusable logic in primitives (hooks) that can be shared across components to keep components clean and focused on rendering
Components should be lean and focused. Do not implement business logic; move that into stores, utilities, or primitives
Never have a component act as a 'God component' coordinating multiple large concerns
packages/web/src/**/*.{jsx,tsx}: Use responsive design principles for UI components in the web package
Use Zag.js for UI components and design system in the web package
Do not prop-drill application state—shared or cross-feature state must live in external stores under packages/web/src/stores/ or relative to the component file
Components should receive at most 1–5 props for local configuration, not shared state; move shared data into external stores, primitives, or Solid context
In SolidJS, do not destructure props as it breaks reactivity—access props directly from the props object or wrap them in a function
Files:
packages/web/src/components/project-ui/add-studies/PdfUploadSection.jsx
**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
Keep files small, focused, and modular. Extract sub-modules into folders with index.jsx and helper components, or move complex logic into separate utility files or primitives
Files:
packages/web/src/components/project-ui/add-studies/PdfUploadSection.jsx
packages/web/src/components/**/*.{jsx,tsx}
📄 CodeRabbit inference engine (.cursorrules)
packages/web/src/components/**/*.{jsx,tsx}: Group related components in subdirectories with an index.js barrel export
Use Zag.js for UI components and design system
Files:
packages/web/src/components/project-ui/add-studies/PdfUploadSection.jsx
packages/web/src/components/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Group related components in subdirectories with an index.js barrel export
Files:
packages/web/src/components/project-ui/add-studies/PdfUploadSection.jsx
🧠 Learnings (3)
📚 Learning: 2025-12-18T22:13:22.395Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-18T22:13:22.395Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx,js,ts} : For UI icons, use the `solid-icons` library or SVGs only. Do not use emojis
Applied to files:
packages/web/src/components/project-ui/add-studies/PdfUploadSection.jsx
📚 Learning: 2025-12-18T22:13:00.528Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-18T22:13:00.528Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx} : For UI icons, use the `solid-icons` library or SVGs only, do not use emojis
Applied to files:
packages/web/src/components/project-ui/add-studies/PdfUploadSection.jsx
📚 Learning: 2025-12-18T22:13:00.529Z
Learnt from: CR
Repo: InfinityBowman/corates PR: 0
File: .cursorrules:0-0
Timestamp: 2025-12-18T22:13:00.529Z
Learning: Applies to packages/web/src/**/*.{jsx,tsx} : Use Solid's createStore for complex state or state objects for better performance and reactivity
Applied to files:
packages/web/src/components/project-ui/add-studies/PdfUploadSection.jsx
🧬 Code graph analysis (2)
packages/web/src/components/project-ui/add-studies/PdfUploadSection.jsx (7)
packages/web/src/lib/pdfUtils.js (2)
packages/web/src/components/project-ui/ProjectView.jsx (1)
studies(52-52)packages/web/src/primitives/useProjectPdfHandlers.js (1)
studies(25-25)packages/web/src/components/project-ui/ReviewerAssignment.jsx (1)
studies(37-37)packages/web/src/components/project-ui/add-studies/ReferenceImportSection.jsx (1)
studies(20-20)packages/web/src/components/project-ui/add-studies/DoiLookupSection.jsx (1)
studies(21-21)packages/web/src/components/project-ui/add-studies/GoogleDriveSection.jsx (1)
studies(14-14)
packages/web/src/primitives/useAddStudies.js (2)
packages/web/src/lib/pdfUtils.js (7)
title(120-123)readFileAsArrayBuffer(200-207)extractPdfTitle(70-72)extractPdfDoi(215-217)withTimeout(24-35)packages/web/src/lib/referenceLookup.js (1)
fetchFromDOI(139-170)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Workers Builds: corates
🔇 Additional comments (7)
packages/web/src/lib/pdfUtils.js (2)
24-35: LGTM - Clean timeout utility implementation.The
withTimeoutfunction correctly usesPromise.raceand cleans up the timer infinally, preventing timer leaks regardless of resolution or rejection.
248-259:_metadataMapis a private property that may break with pdfjs-dist updates.The code accesses
metadata.metadata._metadataMapto iterate over all metadata entries. While this works, it relies on internal implementation details (indicated by the underscore prefix) that could change in future versions.The suggested
getAll()method does not exist in the pdfjs-dist API. If metadata iteration is necessary for your use case, consider adding a defensive fallback or wrapping this access in a try-catch to handle potential breaking changes gracefully:// Check custom metadata - if (metadata?.metadata?._metadataMap) { - for (const [key, value] of metadata.metadata._metadataMap) { + const metadataMap = metadata?.metadata?._metadataMap; + if (metadataMap && typeof metadataMap[Symbol.iterator] === 'function') { + for (const [key, value] of metadataMap) {packages/web/src/components/project-ui/add-studies/PdfUploadSection.jsx (2)
52-67: LGTM - Error state UI with retry functionality.The error state properly displays the error message, shows the filename for context, and provides a retry button with appropriate styling. The optional chaining on
retryPdfExtractionis good defensive coding.
69-127: LGTM - Well-structured extraction and success state UI.The component correctly handles the three mutually exclusive states (error, extracting, success) with appropriate visual feedback. The metadata loading indicator and display are cleanly implemented with proper conditional rendering.
packages/web/src/primitives/useAddStudies.js (3)
24-25: LGTM - Timeout constant for DOI metadata fetch.The 10-second timeout is reasonable for external API calls to CrossRef.
1046-1060: New PDF state fields (error, metadataLoading) not included in serialization.The
errorandmetadataLoadingfields added to PDF state (lines 185-186) are not serialized ingetSerializableState. WhilemetadataLoading: falseis reasonable (fetch state is lost), consider whethererrorshould be preserved so users see extraction failures after OAuth redirects.If errors should persist:
const serializedPdfs = uploadedPdfs.map(pdf => ({ id: pdf.id, title: pdf.title, extracting: pdf.extracting, data: cloneArrayBuffer(pdf.data), doi: pdf.doi, + error: pdf.error || null, metadata: pdf.metadata ? { ...pdf.metadata } : null,
1211-1218: LGTM - New handler properly exported.The
retryPdfExtractionfunction is correctly added to the hook's return object, making it available to consuming components.
1 participant
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.