Conversation
Reviewer's GuideImplements a visual top progress bar for PdfReader while a PDF document is loading and tightens PDF object URL lifecycle to revoke URLs on disposal instead of on each data set. Sequence diagram for PdfReader load with progress barsequenceDiagram
actor User
participant PdfReaderComponent
participant PdfReaderJsModule
participant pdfjsLib
participant ProgressBar
User->>PdfReaderComponent: OpenPdf(file)
PdfReaderComponent->>PdfReaderJsModule: setData(id, data)
PdfReaderJsModule->>PdfReaderJsModule: createObjectURLFromByte(data)
PdfReaderJsModule->>PdfReaderJsModule: pdf.objectUrl = objectUrl
PdfReaderJsModule->>PdfReaderJsModule: options.url = objectUrl
PdfReaderJsModule->>PdfReaderJsModule: options.data = null
PdfReaderJsModule->>PdfReaderJsModule: loadPdf(pdf)
PdfReaderJsModule->>ProgressBar: Show .bb-view-progress
PdfReaderJsModule->>ProgressBar: Set --bb-view-progress-val = 0
PdfReaderJsModule->>pdfjsLib: getDocument(options)
activate pdfjsLib
pdfjsLib-->>PdfReaderJsModule: loadingTask
loop While loading
pdfjsLib-->>PdfReaderJsModule: onProgress(loaded, total)
PdfReaderJsModule->>PdfReaderJsModule: val = loaded / total * 100 (capped at 100)
PdfReaderJsModule->>ProgressBar: Update --bb-view-progress-val = val%
end
pdfjsLib-->>PdfReaderJsModule: load complete
deactivate pdfjsLib
PdfReaderJsModule->>ProgressBar: Schedule hide after 300ms
PdfReaderJsModule-->>PdfReaderComponent: Document ready
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 and found some issues that need to be addressed.
- In
setData, the previousobjectUrlis no longer revoked before creating a new one, which can leak object URLs when setting data multiple times on the same instance; consider revoking any existingpdf.objectUrlbefore assigning a new one. - In
loadPdf,const val = loaded / total * 100; if (val > 100) { val = 100; }attempts to reassign aconst; useletorconst val = Math.min(loaded / total * 100, 100)instead. - The progress bar is currently hidden via a timeout started on the first
onProgressevent; consider tying the hide logic toloadingTask.promiseresolution/rejection so the indicator reflects actual completion rather than an arbitrary delay.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `setData`, the previous `objectUrl` is no longer revoked before creating a new one, which can leak object URLs when setting data multiple times on the same instance; consider revoking any existing `pdf.objectUrl` before assigning a new one.
- In `loadPdf`, `const val = loaded / total * 100; if (val > 100) { val = 100; }` attempts to reassign a `const`; use `let` or `const val = Math.min(loaded / total * 100, 100)` instead.
- The progress bar is currently hidden via a timeout started on the first `onProgress` event; consider tying the hide logic to `loadingTask.promise` resolution/rejection so the indicator reflects actual completion rather than an arbitrary delay.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:136-138` </location>
<code_context>
+ bar.style.setProperty('--bb-view-progress-val', '0');
+ }
+
+ let progressHandler = null;
loadingTask.onProgress = function (progressData) {
+ const { loaded, total } = progressData;
+ if (bar) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against missing `progressEl` in the timeout callback.
Because this callback runs later, `progressEl` might be `null` (element missing or DOM changed), which would cause `progressEl.classList.remove('show')` to throw. Add a null check inside the `setTimeout` before accessing `classList`.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:138-141` </location>
<code_context>
+
+ let progressHandler = null;
loadingTask.onProgress = function (progressData) {
+ const { loaded, total } = progressData;
+ if (bar) {
+ const val = loaded / total * 100;
+ if (val > 100) {
+ val = 100;
</code_context>
<issue_to_address>
**suggestion:** Handle zero or missing `total` when computing progress percentage.
When `total` is `0` or missing, `loaded / total` becomes `Infinity` or `NaN` and ends up as an invalid CSS value. Consider guarding with `if (!total) { return; }` or using an indeterminate state instead of computing a percentage in that case.
Suggested implementation:
```javascript
let progressHandler = null;
loadingTask.onProgress = function (progressData) {
const { loaded, total } = progressData;
if (bar) {
// When total is zero or missing, avoid computing a percentage and
// fall back to an indeterminate state by clearing the CSS variable.
if (!total) {
bar.style.removeProperty('--bb-view-progress-val');
return;
}
let val = loaded / total * 100;
```
```javascript
if (val > 100) {
val = 100;
}
bar.style.setProperty('--bb-view-progress-val', `${val}%`);
```
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:141-142` </location>
<code_context>
+ const { loaded, total } = progressData;
+ if (bar) {
+ const val = loaded / total * 100;
+ if (val > 100) {
+ val = 100;
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Use `let` instead of `const` for `val`, since it is reassigned.
Reassigning `val` after declaring it with `const` will throw at runtime. Either switch it to `let` or compute the clamped value in a single expression (e.g., using `Math.min`).
</issue_to_address>
### Comment 4
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:147-150` </location>
<code_context>
+ }
+ bar.style.setProperty('--bb-view-progress-val', `${val}%`);
+
+ if (progressHandler === null) {
+ progressHandler = setTimeout(() => {
+ clearTimeout(progressHandler);
+ progressEl.classList.remove('show');
+ }, 300);
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Hide the progress bar based on completion rather than first progress event.
The hide timeout starts on the first `onProgress` event, so for large PDFs the bar can disappear while loading is still ongoing. Consider starting the timer only when progress reaches 100% or when `loadingTask.promise` resolves so the bar reflects the real loading state.
Suggested implementation:
```javascript
loadingTask.onProgress = function (progressData) {
const { loaded, total } = progressData;
if (bar) {
let val = loaded / total * 100;
if (!Number.isFinite(val)) {
val = 0;
}
if (val > 100) {
val = 100;
}
bar.style.setProperty('--bb-view-progress-val', `${val}%`);
// Only start the hide timer once loading is complete so the bar reflects the real loading state.
if (val >= 100 && progressHandler === null) {
progressHandler = setTimeout(() => {
progressEl.classList.remove('show');
clearTimeout(progressHandler);
progressHandler = null;
}, 300);
}
```
If there are code paths where `loadingTask.onProgress` is never called with a `total` value or never reaches 100%, you may also want to attach a `.then`/`.finally` handler to `loadingTask.promise` elsewhere in this file to trigger the same "hide" behavior in those cases, ensuring the progress bar always hides when loading completes.
</issue_to_address>
### Comment 5
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:50` </location>
<code_context>
export async function setData(id, data) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Releasing the previous `objectUrl` in `setData` was removed, which can leak object URLs.
`setData` used to revoke the existing `objectUrl` before creating a new one, but now revocation only occurs in `disposePdf`. If `setData` is called multiple times on the same instance without disposal, blob URLs may leak. Please either restore the revocation in `setData` or otherwise ensure each new URL replaces and revokes the previous one.
</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
+138
to
+141
| const { loaded, total } = progressData; | ||
|
|
||
| if (bar) { | ||
| const val = loaded / total * 100; |
There was a problem hiding this comment.
suggestion: Handle zero or missing total when computing progress percentage.
When total is 0 or missing, loaded / total becomes Infinity or NaN and ends up as an invalid CSS value. Consider guarding with if (!total) { return; } or using an indeterminate state instead of computing a percentage in that case.
Suggested implementation:
let progressHandler = null;
loadingTask.onProgress = function (progressData) {
const { loaded, total } = progressData;
if (bar) {
// When total is zero or missing, avoid computing a percentage and
// fall back to an indeterminate state by clearing the CSS variable.
if (!total) {
bar.style.removeProperty('--bb-view-progress-val');
return;
}
let val = loaded / total * 100; if (val > 100) {
val = 100;
}
bar.style.setProperty('--bb-view-progress-val', `${val}%`);There was a problem hiding this comment.
Pull request overview
This PR adds a visual loading progress indicator to the PdfReader component, displaying a progress bar at the top of the viewer while PDF documents are being loaded. This enhancement improves user experience by providing visual feedback during potentially long loading operations.
Key Changes:
- Added CSS styling for a horizontal progress bar displayed at the top of the PDF viewer
- Implemented JavaScript progress tracking using the PDF.js library's onProgress callback
- Refactored objectUrl cleanup logic to the disposePdf function
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| pdf_reader.css | Added CSS classes for progress bar styling with Bootstrap theming and transition effects |
| PdfReader.razor.js | Implemented progress tracking logic in loadPdf function, refactored objectUrl handling, and added cleanup in disposePdf |
| PdfReader.razor | Added HTML structure for progress bar container and bar element |
| BootstrapBlazor.PdfReader.csproj | Version bump from 10.0.13 to 10.0.14 |
Comments suppressed due to low confidence (1)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:143
- Assignment to variable val, which is declared constant.
val = 100;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (progressHandler === null) { | ||
| progressHandler = setTimeout(() => { | ||
| clearTimeout(progressHandler); |
There was a problem hiding this comment.
Calling clearTimeout inside the timeout callback is unnecessary since the timeout has already fired. This line has no effect and should be removed.
Suggested change
| clearTimeout(progressHandler); |
| if (progressHandler === null) { | ||
| progressHandler = setTimeout(() => { | ||
| clearTimeout(progressHandler); | ||
| progressEl.classList.remove('show'); |
There was a problem hiding this comment.
The progressEl variable is accessed without a null check, but it was checked for null on line 128. If progressEl is null, this will cause a runtime error. Add a null check before accessing progressEl.
Suggested change
| progressEl.classList.remove('show'); | |
| if (progressEl) { | |
| progressEl.classList.remove('show'); | |
| } |
Comment on lines
+141
to
+143
| const val = loaded / total * 100; | ||
| if (val > 100) { | ||
| val = 100; |
There was a problem hiding this comment.
Division by 'total' can result in division by zero or infinity if total is 0 or undefined. Add a check to ensure total is a positive number before performing the division.
Suggested change
| const val = loaded / total * 100; | |
| if (val > 100) { | |
| val = 100; | |
| let val = 0; | |
| if (typeof total === 'number' && total > 0) { | |
| val = loaded / total * 100; | |
| if (val > 100) { | |
| val = 100; | |
| } |
Comment on lines
+141
to
+145
| const val = loaded / total * 100; | ||
| if (val > 100) { | ||
| val = 100; | ||
| } | ||
| bar.style.setProperty('--bb-view-progress-val', `${val}%`); |
There was a problem hiding this comment.
The variable 'val' is declared as const on line 141 but is being reassigned on line 143. This will cause a runtime error. Change the declaration from 'const' to 'let' to allow reassignment.
Comment on lines
+141
to
+152
| const val = loaded / total * 100; | ||
| if (val > 100) { | ||
| val = 100; | ||
| } | ||
| bar.style.setProperty('--bb-view-progress-val', `${val}%`); | ||
|
|
||
| if (progressHandler === null) { | ||
| progressHandler = setTimeout(() => { | ||
| clearTimeout(progressHandler); | ||
| progressEl.classList.remove('show'); | ||
| }, 300); | ||
| } |
There was a problem hiding this comment.
The condition 'progressHandler === null' will only be true once, meaning the timeout to hide the progress bar is set only on the first progress update. Subsequent progress updates won't reset or update this timer. This could cause the progress bar to disappear prematurely while loading is still in progress. Consider clearing the existing timeout before setting a new one, or move this logic to after the loading completes.
Suggested change
| const val = loaded / total * 100; | |
| if (val > 100) { | |
| val = 100; | |
| } | |
| bar.style.setProperty('--bb-view-progress-val', `${val}%`); | |
| if (progressHandler === null) { | |
| progressHandler = setTimeout(() => { | |
| clearTimeout(progressHandler); | |
| progressEl.classList.remove('show'); | |
| }, 300); | |
| } | |
| let val = loaded / total * 100; | |
| if (val > 100) { | |
| val = 100; | |
| } | |
| bar.style.setProperty('--bb-view-progress-val', `${val}%`); | |
| // Always clear and reset the timeout on each progress update | |
| if (progressHandler !== null) { | |
| clearTimeout(progressHandler); | |
| } | |
| progressHandler = setTimeout(() => { | |
| progressHandler = null; | |
| if (progressEl) { | |
| progressEl.classList.remove('show'); | |
| } | |
| }, 300); |
| @@ -47,14 +47,12 @@ export async function setData(id, data) { | |||
| return; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The previous objectUrl is no longer being revoked before creating a new one in the setData function. This can lead to memory leaks if setData is called multiple times, as the old blob URLs will remain in memory. Consider revoking the old objectUrl before assigning the new one.
Suggested change
| // Revoke previous object URL if it exists to prevent memory leaks | |
| if (pdf.objectUrl) { | |
| URL.revokeObjectURL(pdf.objectUrl); | |
| } |
| height: 100%; | ||
| background-color: var(--bs-primary); | ||
| width: var(--bb-view-progress-val, 0); | ||
| transition: width .3s linear; |
There was a problem hiding this comment.
The transition property uses '.3s' which is less readable than '0.3s'. Consider using '0.3s' for consistency and clarity.
Suggested change
| transition: width .3s linear; | |
| transition: width 0.3s linear; |
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 #818
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add a visible loading progress indicator to the PdfReader component and improve cleanup of PDF object URLs.
New Features:
Bug Fixes:
Enhancements: