Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the PDF reader’s file size handling to support stream-based content by ensuring content length is set when missing and by improving the human-readable file size formatter and its usage. Sequence diagram for updated PDF metadata and file size handlingsequenceDiagram
actor User
participant PdfReader as PdfReaderComponent
participant PdfJs as PdfJsLibrary
participant PdfDoc as PdfDocument
participant Dom as DialogElement
User->>PdfReader: openPdf(options)
PdfReader->>PdfJs: loadPdf(options.data)
PdfJs->>PdfDoc: createPdfDocument(options.data)
PdfJs->>PdfDoc: getMetadata()
PdfDoc-->>PdfJs: metadata
PdfJs->>PdfReader: metadata
alt metadata.contentLength is null
PdfReader->>PdfReader: metadata.contentLength = options.data.length
end
PdfReader->>Dom: loadMetadata(Dom, pdfViewer, metadata)
PdfReader->>PdfReader: getFileSize(metadata)
PdfReader-->>Dom: filesize textContent
Dom-->>User: show updated file size in dialog
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ArgoZhang
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
getFileSize, consider guarding againstmetadata.contentLengthbeingnull/undefinedor non-numeric before doing numeric comparisons, to avoid runtime errors when metadata is incomplete or streamed. - When falling back to
options.data.lengthformetadata.contentLength, verify thatoptionsandoptions.dataalways exist and thatlengthreflects the byte length (e.g., usebyteLengthfor ArrayBuffer/Uint8Array) so file size is computed accurately for streams.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `getFileSize`, consider guarding against `metadata.contentLength` being `null`/`undefined` or non-numeric before doing numeric comparisons, to avoid runtime errors when metadata is incomplete or streamed.
- When falling back to `options.data.length` for `metadata.contentLength`, verify that `options` and `options.data` always exist and that `length` reflects the byte length (e.g., use `byteLength` for ArrayBuffer/Uint8Array) so file size is computed accurately for streams.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:155-163` </location>
<code_context>
pdfViewer.setDocument(pdfDocument);
pdfDocument.getMetadata().then(metadata => {
+ if (metadata.contentLength === null) {
+ metadata.contentLength = options.data.length;
+ }
loadMetadata(el, pdfViewer, metadata);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Broaden the null check for `contentLength` to cover `undefined` as well.
A strict `=== null` check misses the common case where `metadata.contentLength` is `undefined`, so `getFileSize` receives `undefined`, leading to `NaN` and a fallback of `0B`. Use a nullish check like `metadata.contentLength == null` (or similar explicit guard) so both `null` and `undefined` are handled before calling `loadMetadata`.
```suggestion
pdfViewer.setDocument(pdfDocument);
pdfDocument.getMetadata().then(metadata => {
if (metadata.contentLength == null) {
metadata.contentLength = options.data.length;
}
loadMetadata(el, pdfViewer, metadata);
});
}
```
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:297-298` </location>
<code_context>
}
-const getFilesize = metadata => {
+const getFileSize = metadata => {
const length = metadata.contentLength;
+ let val = 0;
+ let unit = 'B';
</code_context>
<issue_to_address>
**issue:** Guard `getFileSize` against missing or non-numeric `contentLength` to avoid `NaN`-based output.
When `metadata.contentLength` is `undefined`, `null`, or non-numeric, none of the branches will run and `val` remains `0`, producing a misleading `"0B"` for an unknown size. Consider either early-returning a placeholder (e.g. empty string or `"-"`) when `length == null` or `Number.isNaN(length)`, or coercing once with `const length = Number(metadata.contentLength)` and validating before use.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
155
to
163
| pdfViewer.setDocument(pdfDocument); | ||
|
|
||
| pdfDocument.getMetadata().then(metadata => { | ||
| if (metadata.contentLength === null) { | ||
| metadata.contentLength = options.data.length; | ||
| } | ||
| loadMetadata(el, pdfViewer, metadata); | ||
| }); | ||
|
|
There was a problem hiding this comment.
suggestion (bug_risk): Broaden the null check for contentLength to cover undefined as well.
A strict === null check misses the common case where metadata.contentLength is undefined, so getFileSize receives undefined, leading to NaN and a fallback of 0B. Use a nullish check like metadata.contentLength == null (or similar explicit guard) so both null and undefined are handled before calling loadMetadata.
Suggested change
| pdfViewer.setDocument(pdfDocument); | |
| pdfDocument.getMetadata().then(metadata => { | |
| if (metadata.contentLength === null) { | |
| metadata.contentLength = options.data.length; | |
| } | |
| loadMetadata(el, pdfViewer, metadata); | |
| }); | |
| pdfViewer.setDocument(pdfDocument); | |
| pdfDocument.getMetadata().then(metadata => { | |
| if (metadata.contentLength == null) { | |
| metadata.contentLength = options.data.length; | |
| } | |
| loadMetadata(el, pdfViewer, metadata); | |
| }); | |
| } |
Comment on lines
+297
to
298
| const getFileSize = metadata => { | ||
| const length = metadata.contentLength; |
There was a problem hiding this comment.
issue: Guard getFileSize against missing or non-numeric contentLength to avoid NaN-based output.
When metadata.contentLength is undefined, null, or non-numeric, none of the branches will run and val remains 0, producing a misleading "0B" for an unknown size. Consider either early-returning a placeholder (e.g. empty string or "-") when length == null or Number.isNaN(length), or coercing once with const length = Number(metadata.contentLength) and validating before use.
There was a problem hiding this comment.
Pull request overview
This PR enhances the getFileSize function to support PDF streams by adding a fallback mechanism when contentLength is null, and improves file size formatting with consistent decimal precision.
Key Changes:
- Added null check for
contentLengthwith fallback tooptions.data.lengthfor stream-based PDFs - Refactored
getFileSizeto apply consistent rounding (2 decimal places) across all file size units
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| unit = 'GB'; | ||
| val = length / 1024 / 1024 / 1024; | ||
| } | ||
| return `${Math.round(val * 100) / 100}${unit}`; |
There was a problem hiding this comment.
The rounding calculation Math.round(val * 100) / 100 will show inconsistent decimal places. For values like 1.5, it will display '1.5KB' instead of '1.50KB'. Consider using val.toFixed(2) for consistent two-decimal formatting across all file sizes.
Suggested change
| return `${Math.round(val * 100) / 100}${unit}`; | |
| return `${val.toFixed(2)}${unit}`; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #842
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Improve PDF reader file size handling and display in the JavaScript viewer.
Bug Fixes:
Enhancements: