NEW - allow the review page (panviewer) to render a pdf #11041
Conversation
This comment has been minimized.
This comment has been minimized.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| const [dx] = useState(0) | ||
| const [dy] = useState(0) | ||
|
|
||
| const isPdf = image.toLowerCase().endsWith('.pdf') |
There was a problem hiding this comment.
Bug: PDFs Hidden by URL Query Strings
The PDF detection using image.toLowerCase().endsWith('.pdf') fails when the URL contains query parameters. Based on the MINIO_REGEX pattern in documents.ts and actual usage in the codebase, URLs can include query strings like ?X-Amz-Algorithm=..., which would cause endsWith('.pdf') to return false even for valid PDF URLs, preventing PDFs from being opened.
| window.open(blobUrl, '_blank') | ||
| }) | ||
| } | ||
| }, [image, isPdf]) |
There was a problem hiding this comment.
Bug: Unrevoked Blob URLs Cause Memory Leaks
The blob URL created by URL.createObjectURL(blob) is never revoked with URL.revokeObjectURL(), causing a memory leak. Each time a PDF is viewed, a new blob URL is created and retained in memory until the page is closed, which can accumulate if users view multiple PDFs in a session.
| window.open(blobUrl, '_blank') | ||
| }) | ||
| } | ||
| }, [image, isPdf]) |
There was a problem hiding this comment.
Bug: Duplicate Tabs: PDF Re-opens Unnecessarily
The useEffect runs every time the image prop changes without tracking whether the PDF has already been opened. This causes the same PDF to be fetched and opened in a new tab multiple times if the component re-renders with the same image URL, creating duplicate tabs and unnecessary network requests.
| const [dx] = useState(0) | ||
| const [dy] = useState(0) | ||
|
|
||
| const isPdf = image.toLowerCase().endsWith('.pdf') |
There was a problem hiding this comment.
Bug: Encoded PDFs Undetected, Feature Malfunctions
Base64-encoded PDFs in the format data:application/pdf;base64,... are not detected by the endsWith('.pdf') check. These PDFs will be rendered as images instead of being opened in a new tab, which browsers may not handle correctly, causing the feature to fail for base64 PDF data.
| type="positive" | ||
| onClick={handleOpenPdf} | ||
| > | ||
| Open PDF in a new tab |
There was a problem hiding this comment.
Needs translation keya
| {!isPdf ? ( | ||
| <img | ||
| src={image} | ||
| alt="Supporting Document" |
| - `RECORD_REGISTRATION_VERIFY_CERTIFIED_COPIES` | ||
| - `PROFILE_UPDATE` | ||
|
|
||
| - Review page now supports PDF rendering when a PDF link is clicked allowing to verify the uploaded PDF |
There was a problem hiding this comment.
Add test case for this?
| const handleOpenPdf = async () => { | ||
| try { | ||
| const res = await fetch(image, { cache: 'default' }) | ||
| const blob = await res.blob() | ||
| const blobUrl = URL.createObjectURL(blob) | ||
| window.open(blobUrl, '_blank') | ||
| } catch (err) { | ||
| console.error('Failed to open PDF', err) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Looking at the name of the component "pan viewer" it makes me think this might not be the right place for this logic. Pan viewer, if I'm not mistaken, is the component that makes it possible to zoom / pan the image that is previewed. Should this be one level up? @pankaj-pant @bvenceslas
There was a problem hiding this comment.
@rikukissa you are right. It should be moved a level up. Although we have discovered internally that the PDF upload scope got accidentally introduced, and we are reverting back to image uploads for a cert that has been printed + signed + stamped (so a image taken with a phone/tablet). With that in mind, if this PR helps @tareq89 in the scope of this ticket #3613, please use the general gist of the fix. I think @euanmillar already updated in the issue 3613 the status of @bvenceslas's availability
|
This PR has been marked with label stale Since it has been inactive for 20 days. It will automatically be closed in 10 days if no further activity occurs. |
Description
Issue #10983
Link this pull request to the GitHub issue (and optionally name the branch
ocrvs-<issue #>)Checklist
Note
Adds PDF support to the review document viewer (opens PDFs in a new tab) and updates document URL validation to allow PDF files.
PanViewer.tsx): Detects.pdfimages; opens fetched PDF in a new tab; shows placeholder text instead of inline image.packages/commons/src/documents.ts): ExtendsMINIO_REGEXto includepdf.Written by Cursor Bugbot for commit 332cafb. This will update automatically on new commits. Configure here.