Conversation
Reviewer's GuideAdds dynamic URL/data switching support to PdfReader by introducing setUrl/setData JS interop APIs, centralizing PDF load/disposal logic, and wiring the Blazor component to call setUrl when the Url parameter changes. Sequence diagram for PdfReader Url change using setUrlsequenceDiagram
actor User
participant Blazor as PdfReaderComponent
participant JSRuntime
participant JsModule as PdfReader_razor_js
participant DataStore as Data
participant PdfContext as PdfObject
User->>Blazor: Update Url parameter
Blazor->>Blazor: OnAfterRenderAsync detects Url changed
Blazor->>JSRuntime: InvokeVoidAsync setUrl(Id, Url)
JSRuntime->>JsModule: setUrl(id, url)
JsModule->>DataStore: get(id)
DataStore-->>JsModule: PdfObject
JsModule->>JsModule: disposePdf(PdfObject)
alt url is falsy
JsModule-->>JSRuntime: return
else url is truthy
JsModule->>JsModule: options = PdfObject.options
JsModule->>JsModule: options.url = url
JsModule->>JsModule: options.data = null
JsModule->>JsModule: loadPdf(PdfObject)
JsModule->>DataStore: set(id, PdfObject with pdfViewer, loadingTask, observer)
end
Updated class diagram for PdfReader JS interop and PDF contextclassDiagram
class PdfReaderComponent {
string Id
string Url
int CurrentPage
string _url
int _currentPage
OnAfterRenderAsync(firstRender bool) Task
}
class JsPdfReaderModule {
init(id string, invoke any, options any) Promise
setUrl(id string, url string) Promise
setData(id string, data any) Promise
setScaleValue(id string, value number) void
resetThumbnails(id string) void
loadPdf(pdf PdfContext) Promise
disposePdf(pdf PdfContext) void
loadMetadata(el HTMLElement, pdfViewer any, metadata any) void
addToolbarEventHandlers(el HTMLElement, pdfViewer any, invoke any, options any) void
removeToolbarEventHandlers(el HTMLElement) void
resetToolbarView(el HTMLElement, pdfViewer any) void
printPdf(url string) void
dispose(id string) void
}
class PdfContext {
HTMLElement el
any invoke
any options
any pdfViewer
any observer
any loadingTask
}
class DataStore {
+set(id string, pdf PdfContext) void
+get(id string) PdfContext
+remove(id string) void
}
PdfReaderComponent ..> JsPdfReaderModule : JS interop
JsPdfReaderModule o--> PdfContext : manages
JsPdfReaderModule ..> DataStore : stores PdfContext
DataStore o--> PdfContext : holds instances
note for JsPdfReaderModule "Represents PdfReader.razor.js exported functions"
note for PdfReaderComponent "C# Blazor component calling setUrl when Url changes"
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 - here's some feedback:
- Both
setUrl/setDataanddisposecalldisposePdf(pdf)without checking whetherData.get(id)returned a valid object, so it would be safer to add a null/undefined guard indisposePdf(or at the call sites) to avoid runtime errors when an unknown or already-disposed id is passed. - In
removeToolbarEventHandlers,document.querySelector('.bb-view-print-iframe')operates at the document level and may affect other instances; consider scoping this toel(or an instance-specific container) to avoid cross-instance interference.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Both `setUrl`/`setData` and `dispose` call `disposePdf(pdf)` without checking whether `Data.get(id)` returned a valid object, so it would be safer to add a null/undefined guard in `disposePdf` (or at the call sites) to avoid runtime errors when an unknown or already-disposed id is passed.
- In `removeToolbarEventHandlers`, `document.querySelector('.bb-view-print-iframe')` operates at the document level and may affect other instances; consider scoping this to `el` (or an instance-specific container) to avoid cross-instance interference.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:21-24` </location>
<code_context>
- if (options.url === null) {
+ Data.set(id, { el, invoke, options });
+
+ if (options.url) {
+ setUrl(id, options.url);
+ }
+ else {
+ setData(id, options.data);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The async `setUrl`/`setData` calls in `init` are not awaited, changing initialization ordering semantics.
Previously `init` only resolved after `loadPdf` completed, so callers could treat it as “viewer is ready.” Now `init` returns as soon as it fires `setUrl`/`setData`, which may run asynchronously, changing that contract and potentially introducing race conditions. To keep the old behavior, `await setUrl(...)` / `await setData(...)`; if the change is intentional, clarify the async semantics in the API (e.g., via naming or docs).
</issue_to_address>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 pull request adds a setUrl method to the PdfReader component to enable dynamic URL changes without requiring full component reinitialization. The implementation refactors the initialization logic to separate data storage from PDF loading, allowing the URL or data to be updated independently after the component is mounted.
Key changes:
- Introduces
setUrlandsetDataexport functions in JavaScript for dynamic PDF content updates - Refactors
initto store component state first, then delegate tosetUrl/setDatafor loading - Extracts cleanup logic into
disposePdfandremoveToolbarEventHandlershelper functions for reuse
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| PdfReader.razor.js | Adds setUrl/setData methods, refactors init and loadPdf to support dynamic updates, extracts cleanup logic into disposePdf and removeToolbarEventHandlers |
| PdfReader.razor.cs | Updates URL change detection to call setUrl instead of reinitializing the entire component |
Comments suppressed due to low confidence (10)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:206
- Equality checks on floating point values can yield unexpected results.
if (_currentRotation != CurrentRotation)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:179
- Unused variable title.
const title = el.querySelector('.bb-view-pdf-dialog-title');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:180
- Unused variable author.
const author = el.querySelector('.bb-view-pdf-dialog-author');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:181
- Unused variable subject.
const subject = el.querySelector('.bb-view-pdf-dialog-subject');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:182
- Unused variable keywords.
const keywords = el.querySelector('.bb-view-pdf-dialog-keywords');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:186
- Unused variable modified.
const modified = el.querySelector('.bb-view-pdf-dialog-modified');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:206
- Unused variable webview.
const webview = el.querySelector('.bb-view-pdf-dialog-webview');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:670
- The initial value of v is unused, since it is always overwritten.
let v = 100;
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:489
- Avoid automated semicolon insertion (95% of all statements in the enclosing function have an explicit semicolon).
})
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:593
- 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.
| if (!i.classList.contains("pdfViewer")) { | ||
| i.remove(); | ||
| } | ||
| }) |
There was a problem hiding this comment.
Missing semicolon at the end of the statement. While JavaScript allows this due to automatic semicolon insertion, it's a best practice to include semicolons for consistency with the rest of the codebase.
Suggested change
| }) | |
| }); |
Comment on lines
+22
to
+25
| setUrl(id, options.url); | ||
| } | ||
| else { | ||
| setData(id, options.data); |
There was a problem hiding this comment.
The setUrl function is called without await in the init function, but setUrl is an async function that performs important operations like loadPdf. This means the init function may complete before the PDF is fully loaded, which could cause race conditions or unexpected behavior. Either await the call or ensure the function doesn't need to complete before init returns.
Suggested change
| setUrl(id, options.url); | |
| } | |
| else { | |
| setData(id, options.data); | |
| await setUrl(id, options.url); | |
| } | |
| else { | |
| await setData(id, options.data); |
| setUrl(id, options.url); | ||
| } | ||
| else { | ||
| setData(id, options.data); |
There was a problem hiding this comment.
The setData function is called without await in the init function. Similar to the setUrl issue, this could cause race conditions or unexpected behavior.
Suggested change
| setData(id, options.data); | |
| await setData(id, options.data); |
| } | ||
|
|
||
| export async function setUrl(id, url) { | ||
| const pdf = Data.get(id); |
There was a problem hiding this comment.
The setUrl function doesn't check if the pdf object exists before calling disposePdf. If Data.get(id) returns undefined or null (e.g., if init was never called or the component was already disposed), this will cause disposePdf to receive an invalid value, potentially leading to runtime errors when trying to destructure properties.
Suggested change
| const pdf = Data.get(id); | |
| const pdf = Data.get(id); | |
| if (!pdf) { | |
| return; | |
| } |
|
|
||
| export async function setData(id, data) { | ||
| const pdf = Data.get(id); | ||
| disposePdf(pdf); |
There was a problem hiding this comment.
The setData function doesn't check if the pdf object exists before calling disposePdf. If Data.get(id) returns undefined or null, this will cause similar issues as in setUrl.
Suggested change
| disposePdf(pdf); | |
| if (pdf) { | |
| disposePdf(pdf); | |
| } |
| pdf.observer = setObserver(el); | ||
| } | ||
|
|
||
| const disposePdf = pdf => { |
There was a problem hiding this comment.
The disposePdf function doesn't check if the pdf parameter is null or undefined before destructuring. If called with an invalid value, this will cause a runtime error.
Suggested change
| const disposePdf = pdf => { | |
| const disposePdf = pdf => { | |
| if (!pdf) { | |
| return; | |
| } |
| EventHandler.off(closeButton, "click"); | ||
| } | ||
| } | ||
| disposePdf(pdf); |
There was a problem hiding this comment.
The dispose function doesn't check if the pdf object exists before calling disposePdf. If Data.get(id) returns undefined, disposePdf will be called with an invalid value.
Suggested change
| disposePdf(pdf); | |
| if (pdf) { | |
| disposePdf(pdf); | |
| } |
| } | ||
|
|
||
| const resetToolbarView = (el, pdfViewer) => { | ||
| const scaleEl = el.querySelector(".bb-view-scale-input"); |
There was a problem hiding this comment.
Unused variable scaleEl.
Suggested change
| const scaleEl = el.querySelector(".bb-view-scale-input"); |
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 #814
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add support for dynamically changing the PDF source in PdfReader and centralize PDF disposal and toolbar cleanup.
New Features:
Enhancements: