[FIX] Add error fallback for PDF viewer when document fails to load#1781
[FIX] Add error fallback for PDF viewer when document fails to load#1781gaya3-zipstack merged 6 commits intomainfrom
Conversation
Previously, if a PDF failed to load due to an invalid/unavailable URL or corrupted file, the viewer would render a blank screen with no feedback. This adds a user-friendly error fallback with a retry option, and handles the case when no file URL is provided. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds error-state styles and retry/error-handling to the PDF viewer: new CSS for error presentation; PdfViewer gains Changes
Sequence DiagramsequenceDiagram
participant User as User
participant PdfViewer as PdfViewer
participant Viewer as Viewer
participant Parent as Parent
User->>PdfViewer: Request load/open PDF (fileUrl)
PdfViewer->>Viewer: Render with current `key`/fileUrl
Viewer-->>PdfViewer: Emit load error (callback)
PdfViewer->>PdfViewer: set `loadError`, call `renderError`
PdfViewer->>Parent: call `onError(loadError)` (if provided)
PdfViewer->>User: display AntD Result error UI with Retry
User->>PdfViewer: click Retry
PdfViewer->>PdfViewer: increment `retryKey`, clear `loadError` (forces remount)
PdfViewer->>Viewer: re-render with new `key` to retry load
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
vishnuszipstack
requested review from
Deepak-Kesavan,
athul-rs,
jagadeeswaran-zipstack and
jaseemjaskp
and removed request for
jagadeeswaran-zipstack and
jaseemjaskp
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx`:
- Around line 34-66: renderError currently calls the onError callback during
render which causes a side effect; instead, have renderError set a local error
state (useState) and remove the direct onError call, then add a useEffect that
watches that error state and invokes onError(error) when the state changes
(include onError in the effect deps and guard for non-null error). Update
references to renderError, onError, handleRetry and the new error state variable
so renderError remains pure and the notification happens in useEffect.
🧹 Nitpick comments (1)
frontend/src/components/custom-tools/pdf-viewer/Highlight.css (1)
11-36: Consider using Ant Design theme tokens instead of hardcoded colors.The colors (
#fafafa,#262626,#8c8c8c, etc.) are hardcoded, which won't adapt if the application ever adopts a dark theme or customizes its Ant Design theme tokens. Additionally, directly overriding.ant-result-title/.ant-result-subtitleclass names couples this to antd's internal markup — these can break on major antd upgrades.Not a blocker given the rest of the file already uses hardcoded values, but worth noting for future maintainability.
jaseemjaskp
left a comment
jaseemjaskp
left a comment
There was a problem hiding this comment.
LGTM. please review the coderabbit comment and address it if valid. otherwise resolve it
Avoid side effects during render by storing the error in state and notifying the parent via useEffect instead of calling onError directly inside renderError. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve import ordering conflict with main branch and fix biome lint warnings (missing block statements, trailing comma). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx (1)
45-52:⚠️ Potential issue | 🟠 MajorKeep
renderErrorside-effect free.Line 52 still updates
PdfViewerstate from insideViewer's render callback. React can warn on this (Cannot update a component while rendering a different component), and if the PDF library hands back a fresherrorobject each render you'll re-notify the parent repeatedly. Please move the error capture to a post-render/library event path and letrenderErroronly return UI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx` around lines 45 - 52, renderError currently mutates PdfViewer state by calling setLoadError inside the Viewer render callback; remove that side-effect so renderError only returns UI, and instead capture/notify errors in a post-render path (e.g., attach the PDF library's onDocumentLoadError/onError callback or use a useEffect that watches an errorRef/state set from the library event). Concretely: remove setLoadError from renderError, add a dedicated handler (e.g., handleDocumentError) wired to the library's event props or update a ref/errorState from that handler, then use useEffect inside PdfViewer to call setLoadError/notify parent when that handler updates the ref/state; keep references to renderError, setLoadError, and the new handleDocumentError/useEffect to locate and modify the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx`:
- Around line 45-52: renderError currently mutates PdfViewer state by calling
setLoadError inside the Viewer render callback; remove that side-effect so
renderError only returns UI, and instead capture/notify errors in a post-render
path (e.g., attach the PDF library's onDocumentLoadError/onError callback or use
a useEffect that watches an errorRef/state set from the library event).
Concretely: remove setLoadError from renderError, add a dedicated handler (e.g.,
handleDocumentError) wired to the library's event props or update a
ref/errorState from that handler, then use useEffect inside PdfViewer to call
setLoadError/notify parent when that handler updates the ref/state; keep
references to renderError, setLoadError, and the new
handleDocumentError/useEffect to locate and modify the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 124c7964-8ea5-472e-9daf-dcde9d38c141
📒 Files selected for processing (1)
frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx (1)
22-22: Consider wiringonErrorin parent components for coordinated error handling.The relevant code snippets show that
OutputAnalyzerCardandDocumentManagercurrently invokePdfViewerwithout passingonError. While the internal error UI provides user feedback, parent components could benefit from being notified to update their own state (e.g., resettingisContentAvailableinDocumentViewer). This can be addressed as a follow-up enhancement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx` at line 22, PdfViewer currently accepts an onError prop but parent components (notably OutputAnalyzerCard and DocumentManager) are not passing it, preventing parents from reacting to viewer errors; update the parent components that render PdfViewer (e.g., where PdfViewer(...) is invoked in OutputAnalyzerCard and DocumentManager) to accept or create an onError callback and pass it down (for example a handler that updates parent state such as resetting isContentAvailable or triggering their own error UI), and ensure PdfViewer calls that onError whenever it currently triggers its internal error UI so parents are notified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx`:
- Around line 28-42: The component retains a stale loadError when fileUrl
changes, which can cause the existing useEffect (that notifies onError) to send
outdated errors; add a new effect that resets loadError by calling
setLoadError(null) whenever fileUrl changes (so the state and notifications
reflect the current file), or modify the existing effects to include fileUrl in
their dependency and clear loadError before notifying; update references to
retryKey/handleRetry only if they rely on the old loadError semantics.
---
Nitpick comments:
In `@frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx`:
- Line 22: PdfViewer currently accepts an onError prop but parent components
(notably OutputAnalyzerCard and DocumentManager) are not passing it, preventing
parents from reacting to viewer errors; update the parent components that render
PdfViewer (e.g., where PdfViewer(...) is invoked in OutputAnalyzerCard and
DocumentManager) to accept or create an onError callback and pass it down (for
example a handler that updates parent state such as resetting isContentAvailable
or triggering their own error UI), and ensure PdfViewer calls that onError
whenever it currently triggers its internal error UI so parents are notified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91ef3ba0-a250-4830-bfcc-87e40bc4806b
📒 Files selected for processing (2)
frontend/src/components/custom-tools/pdf-viewer/Highlight.cssfrontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/custom-tools/pdf-viewer/Highlight.css
…ions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx`:
- Around line 42-47: PdfViewer load failures aren't propagated because the
wrapper render paths create PdfViewer without the onError prop and some wrapper
components short-circuit when fileUrl is empty; wire the onError handler through
the wrapper layer where PdfViewer is instantiated (look for the PdfViewer render
sites inside DocumentManager and DocumentViewer wrappers) so the parent receives
loadError notifications, or alternatively move the "No PDF Available"
empty-state rendering into the wrapper that decides whether to render PdfViewer;
update the render branches that short-circuit on empty fileUrl to either pass a
noop/onError callback into PdfViewer or handle the empty-state UI at that
wrapper level (ensure symbols PdfViewer and the wrapper render functions in
DocumentManager/DocumentViewer propagate the onError prop).
- Around line 49-79: renderError currently calls setLoadError inside the render
callback which is a render-phase side effect; remove the setLoadError call from
the useCallback (renderError) so it only returns JSX, and instead add a
useEffect that watches the loadError state (the state paired with setLoadError)
and performs any notification/side-effects to the parent when loadError changes;
keep renderError signature and dependencies (handleRetry) intact and ensure the
new useEffect sets/handles the error outside of render.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 441e3e05-8eef-444c-abc0-978123a65a91
📒 Files selected for processing (2)
frontend/src/components/custom-tools/pdf-viewer/Highlight.cssfrontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/custom-tools/pdf-viewer/Highlight.css
Move error UI and reporting into a separate PdfLoadError component that uses useEffect for setLoadError, keeping renderError pure (returns JSX only, no state setters during render). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Frontend Lint Report (Biome)✅ All checks passed! No linting or formatting issues found. |
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx (1)
22-45: Add PropTypes for thePdfLoadErrorcomponent.SonarCloud flags missing prop validation for
error,onRetry, andreportError. While this is an internal component, adding PropTypes maintains consistency withPdfViewerand aids documentation.💡 Proposed fix to add PropTypes
+PdfLoadError.propTypes = { + error: PropTypes.shape({ + message: PropTypes.string, + }), + onRetry: PropTypes.func.isRequired, + reportError: PropTypes.func.isRequired, +}; + function PdfViewer({ fileUrl, highlightData, currentHighlightIndex, onError }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx` around lines 22 - 45, PdfLoadError is missing PropTypes for its props; import PropTypes at the top of the file and add a PdfLoadError.propTypes definition that validates error (object or null), onRetry (function) and reportError (function), marking onRetry and reportError as required if they must always be provided; keep types consistent with how PdfViewer uses these props.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx`:
- Around line 22-45: PdfLoadError is missing PropTypes for its props; import
PropTypes at the top of the file and add a PdfLoadError.propTypes definition
that validates error (object or null), onRetry (function) and reportError
(function), marking onRetry and reportError as required if they must always be
provided; keep types consistent with how PdfViewer uses these props.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c64a38a-3497-42ae-a379-fd655f3e7773
📒 Files selected for processing (1)
frontend/src/components/custom-tools/pdf-viewer/PdfViewer.jsx
4 participants
What
PdfViewercomponent that displays a user-friendly error message with a Retry button when a PDF fails to load (corrupted file, network error, invalid URL)fileUrlis provided, showing a "No PDF Available" warning instead of a blank screenWhy
How
renderErrorprop of@react-pdf-viewer/coreViewer component to catch PDF load failures and render a custom error UI using Ant Design'sResultcomponentretryKeystate to force the Viewer to re-mount when the Retry button is clickedfileUrlis falsy to show the "No PDF Available" stateonErrorcallback prop so parent components can be notified of PDF load errorsCan this PR break any existing features. If yes, please list possible items. If no, please explain why.
onErrorprop is optional and does not affect existing usage.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
antdand@ant-design/iconspackages.Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.