Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the PdfReader to load PDFs directly from URLs or raw data without intermediate object URLs, simplifies disposal and DOM cleanup, and makes minor safety and state-reset adjustments for progress reporting and stream tracking. Sequence diagram for loading PDFs via URL or raw datasequenceDiagram
actor User
participant BlazorComponent as PdfReaderComponent
participant JSInterop
participant PdfModule as PdfReaderJs
participant PdfJs as PdfJsLibrary
User->>BlazorComponent: Set Url or Data parameters
BlazorComponent->>JSInterop: InvokeVoidAsync setUrl(Id, url) or setData(Id, data)
JSInterop->>PdfModule: setUrl(id, url)
activate PdfModule
PdfModule->>PdfModule: pdf = Data.get(id)
PdfModule->>PdfModule: options = pdf.options
PdfModule->>PdfModule: options.url = url
PdfModule->>PdfModule: options.data = null
PdfModule->>PdfModule: loadPdf(pdf)
deactivate PdfModule
JSInterop->>PdfModule: setData(id, data)
activate PdfModule
PdfModule->>PdfModule: pdf = Data.get(id)
PdfModule->>PdfModule: options = pdf.options
PdfModule->>PdfModule: options.url = null
PdfModule->>PdfModule: options.data = data
PdfModule->>PdfModule: loadPdf(pdf)
deactivate PdfModule
PdfModule->>PdfJs: getDocument(options)
activate PdfJs
PdfJs-->>PdfModule: loadingTask
PdfJs-->>PdfModule: pdfDocument
deactivate PdfJs
PdfModule->>PdfModule: pdf.pdfViewer.setDocument(pdfDocument)
PdfModule-->>User: PDF rendered without intermediate object URLs
Sequence diagram for disposing the PdfReader and cleaning DOMsequenceDiagram
actor User
participant BlazorComponent as PdfReaderComponent
participant JSInterop
participant PdfModule as PdfReaderJs
participant Dom as BrowserDom
User->>BlazorComponent: Dispose PdfReader
BlazorComponent->>JSInterop: InvokeVoidAsync dispose(id)
JSInterop->>PdfModule: disposePdf(pdf)
activate PdfModule
PdfModule->>PdfModule: el = pdf.el
PdfModule->>PdfModule: observer = pdf.observer
PdfModule->>PdfModule: loadingTask = pdf.loadingTask
alt Has observer
PdfModule->>PdfModule: observer.disconnect()
end
alt Has loadingTask
PdfModule->>PdfModule: loadingTask.destroy()
end
PdfModule->>Dom: query .bb-view-container
Dom-->>PdfModule: viewContainer
loop For each child in viewContainer.children
alt child has class pdfViewer
PdfModule->>Dom: child.innerHTML = ""
else
PdfModule->>Dom: child.remove()
end
end
PdfModule-->>JSInterop: PdfReader disposed without revoking object URLs
deactivate PdfModule
Flow diagram for clamping PDF load progress valueflowchart TD
A[Start progress update] --> B[Compute val = loaded / total * 100]
B --> C[Clamp value using Math min of val and 100]
C --> D[Set CSS variable --bb-view-progress-val to clamped value]
D --> E[Schedule optional progressHandler timeout]
E --> F[End progress update]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
setData, there’s a minor typo (options.data = data;;) that should be cleaned up to a single semicolon. - In
loadPdf, consider guarding againsttotalbeing zero or undefined before computingloaded / total * 100to avoidNaNprogress values when total length is not known.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `setData`, there’s a minor typo (`options.data = data;;`) that should be cleaned up to a single semicolon.
- In `loadPdf`, consider guarding against `total` being zero or undefined before computing `loaded / total * 100` to avoid `NaN` progress values when total length is not known.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR simplifies code and improves performance in the PdfReader component by eliminating unnecessary object URL creation/revocation when handling PDF data. The changes allow pdf.js to process byte arrays directly, reducing memory overhead and improving efficiency.
Key Changes:
- Removed intermediate object URL creation when handling PDF byte data, passing data directly to pdf.js
- Refactored disposal logic to clear pdfViewer content instead of removing the element entirely
- Simplified progress bar value calculation using Math.min
- Added proper state reset for _lastStreamLength when switching between URL and stream modes
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| PdfReader.razor.js | Removed object URL helper functions, modified setData to pass byte arrays directly to pdf.js, simplified progress calculation, and improved disposal logic |
| PdfReader.razor.cs | Added _lastStreamLength reset when URL changes to ensure proper state management |
| BootstrapBlazor.PdfReader.csproj | Bumped version from 10.0.14 to 10.0.15 |
Comments suppressed due to low confidence (11)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:211
- Equality checks on floating point values can yield unexpected results.
if (_currentRotation != CurrentRotation)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:206
- Unused variable title.
const title = el.querySelector('.bb-view-pdf-dialog-title');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:207
- Unused variable author.
const author = el.querySelector('.bb-view-pdf-dialog-author');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:208
- Unused variable subject.
const subject = el.querySelector('.bb-view-pdf-dialog-subject');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:209
- Unused variable keywords.
const keywords = el.querySelector('.bb-view-pdf-dialog-keywords');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:213
- Unused variable modified.
const modified = el.querySelector('.bb-view-pdf-dialog-modified');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:233
- Unused variable webview.
const webview = el.querySelector('.bb-view-pdf-dialog-webview');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:596
- Unused variable scaleEl.
const scaleEl = el.querySelector(".bb-view-scale-input");
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:697
- The initial value of v is unused, since it is always overwritten.
let v = 100;
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:516
- Avoid automated semicolon insertion (95% of all statements in the enclosing function have an explicit semicolon).
})
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:620
- Avoid automated semicolon insertion (91% of all statements in the enclosing function have an explicit semicolon).
delete el.widths
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| options.url = objectUrl; | ||
| options.data = null; | ||
| options.url = null; | ||
| options.data = data;; |
There was a problem hiding this comment.
There is an extra semicolon at the end of this line. Remove one of the semicolons.
| options.data = data;; | |
| options.data = data; |
Link issues
fixes #820
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Streamline PdfReader loading logic to handle URL and byte data sources more efficiently and cleanly reset viewer state.
Bug Fixes:
Enhancements: