Conversation
Reviewer's GuideRefactors the PdfReader print and download behaviors to work with both URL-based and in-memory PDF streams by centralizing blob/URL handling into a new helper, updating toolbar event handlers, and ensuring print iframes are scoped and cleaned up per component instance. Sequence diagram for PdfReader print flow with URL or streamsequenceDiagram
actor User
participant PdfReaderToolbar as PdfReaderToolbar
participant PdfReaderJs as PdfReaderJsModule
participant getPdfUrl as getPdfUrl
participant Iframe as PrintIframe
User->>PdfReaderToolbar: Click bb_view_print button
PdfReaderToolbar->>PdfReaderJs: printPdf(el, options)
alt No existing print iframe in component
PdfReaderJs->>PdfReaderJs: Create iframe
PdfReaderJs->>PdfReaderJs: Append iframe to el
else Existing iframe found
PdfReaderJs->>PdfReaderJs: Reuse iframe
end
PdfReaderJs->>getPdfUrl: getPdfUrl(options, callback)
alt options.url present
getPdfUrl-->>PdfReaderJs: callback(options.url)
else options.data present
getPdfUrl->>getPdfUrl: Create Blob from options.data
getPdfUrl->>getPdfUrl: CreateObjectURL(blob) -> url
getPdfUrl-->>PdfReaderJs: await callback(url)
getPdfUrl->>getPdfUrl: RevokeObjectURL(url)
end
PdfReaderJs->>Iframe: Set iframe.src = url
Iframe->>Iframe: onload
Iframe->>Iframe: contentWindow.focus()
Iframe->>Iframe: contentWindow.print()
PdfReaderToolbar->>PdfReaderJs: invoke.invokeMethodAsync Printing
Sequence diagram for PdfReader download flow with URL or streamsequenceDiagram
actor User
participant PdfReaderToolbar as PdfReaderToolbar
participant PdfReaderJs as PdfReaderJsModule
participant getPdfUrl as getPdfUrl
participant Anchor as DownloadAnchor
User->>PdfReaderToolbar: Click bb_view_download button
PdfReaderToolbar->>PdfReaderJs: downloadPdf(options, fileName)
alt fileName attribute present on element
PdfReaderJs->>PdfReaderJs: Use data_bb_download
else fileName not set
PdfReaderJs->>PdfReaderJs: Read .bb_view_subject text
alt subject found
PdfReaderJs->>PdfReaderJs: Use subject text
else subject not found
PdfReaderJs->>PdfReaderJs: Use default download.pdf
end
end
PdfReaderJs->>getPdfUrl: getPdfUrl(options, callback)
alt options.url present
getPdfUrl-->>PdfReaderJs: callback(options.url)
else options.data present
getPdfUrl->>getPdfUrl: Create Blob from options.data
getPdfUrl->>getPdfUrl: CreateObjectURL(blob) -> url
getPdfUrl-->>PdfReaderJs: await callback(url)
getPdfUrl->>getPdfUrl: RevokeObjectURL(url)
end
PdfReaderJs->>Anchor: Create anchor element
PdfReaderJs->>Anchor: Set href = url, download = fileName
PdfReaderJs->>Anchor: Append to document.body
PdfReaderJs->>Anchor: anchor.click()
PdfReaderJs->>Anchor: Remove from document.body
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 they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:778-780` </location>
<code_context>
+}
- document.body.appendChild(iframe);
+const getPdfUrl = async (options, callback) => {
+ if (options.url) {
+ callback(options.url);
+ }
+ else if (options.data) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The `url` branch of `getPdfUrl` ignores any promise returned by `callback`, leading to asymmetric behavior.
In `getPdfUrl`, the `options.url` branch calls `callback(options.url)` without `await`, but the `options.data` branch uses `await callback(url)`. With promise-returning callbacks (as in current callers), this leads to inconsistent behavior and timing. Please either `return await callback(options.url);` in the URL branch, or remove the `await` in the data branch and make `getPdfUrl` non‑`async`, depending on whether the callback should be awaited or fire-and-forget.
Suggested implementation:
```javascript
const getPdfUrl = async (options, callback) => {
if (options.url) {
return await callback(options.url);
}
else if (options.data) {
```
Elsewhere in the same `getPdfUrl` function, update the `options.data` branch so that `await callback(url);` becomes `return await callback(url);`. This will ensure both branches return the awaited callback result, giving callers consistent behavior regardless of whether they pass `url` or `data`.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:571` </location>
<code_context>
}
-const downloadPdf = (url, fileName) => {
+const downloadPdf = async (options, fileName) => {
if (fileName === null) {
fileName = "download.pdf";
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the callback-based getPdfUrl and scattered iframe handling with simpler async-returning functions and small helpers to keep URL resolution and print iframe lifecycle linear and self-contained.
The new callback-based `getPdfUrl` and the way it’s used in `downloadPdf`/`printPdf` does add avoidable complexity without adding real value. You can keep all new functionality (support for `options.data`, iframe reuse under `el`) while simplifying the async flow and iframe lifecycle.
### 1. Simplify `getPdfUrl`, `downloadPdf`, and `printPdf` async flow
You don’t need a callback here; a plain `async` function that returns the URL is enough. That also removes the artificial `new Promise` wrappers.
```js
// Replace getPdfUrl(options, callback) with:
const getPdfUrl = async (options) => {
if (options.url) {
return options.url;
}
if (options.data) {
const blob = new Blob([options.data], { type: "application/pdf" });
const url = window.URL.createObjectURL(blob);
return url;
}
return null; // or throw if neither url nor data is expected to be missing
};
```
Then simplify `downloadPdf`:
```js
const downloadPdf = async (options, fileName) => {
if (fileName === null) {
fileName = "download.pdf";
}
const url = await getPdfUrl(options);
if (!url) return; // or handle error
const anchorElement = document.createElement("a");
anchorElement.href = url;
anchorElement.download = fileName;
document.body.appendChild(anchorElement);
anchorElement.click();
document.body.removeChild(anchorElement);
if (options.data) {
window.URL.revokeObjectURL(url);
}
};
```
And `printPdf`:
```js
const printPdf = async (el, options) => {
const iframe = getOrCreatePrintIframe(el);
const url = await getPdfUrl(options);
if (!url) return; // or handle error
iframe.onload = () => {
iframe.contentWindow.focus();
iframe.contentWindow.print();
if (options.data) {
window.URL.revokeObjectURL(url);
}
};
iframe.src = url;
};
```
This keeps `downloadPdf`/`printPdf` `async` only because they truly `await` something, and makes the control flow linear and easier to reason about.
### 2. Encapsulate iframe lifecycle
Right now iframe creation/reuse is embedded in `printPdf`, and removal happens in `disposePdf` via DOM queries. A tiny helper removes that mental overhead and keeps iframe logic in one place.
```js
const getOrCreatePrintIframe = (el) => {
let iframe = el.querySelector(".bb-view-print-iframe");
if (!iframe) {
iframe = document.createElement("iframe");
iframe.classList.add("bb-view-print-iframe");
iframe.style.position = "fixed";
iframe.style.right = "100%";
iframe.style.bottom = "100%";
el.appendChild(iframe);
}
return iframe;
};
const removePrintIframe = (el) => {
const iframe = el.querySelector(".bb-view-print-iframe");
if (iframe) {
iframe.remove();
}
};
```
Then use `removePrintIframe(el)` inside `disposePdf` instead of manually querying/removing the iframe there. This keeps all iframe lifecycle concerns self-contained and easier to maintain.
</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 enhances the PdfReader component to support printing PDFs from streams (data blobs) in addition to URLs. The changes refactor the print and download functionality to use a common helper function that handles both URL and data-based PDFs.
Key Changes
- Refactored
printPdfanddownloadPdffunctions to support both URL and data (stream) sources - Introduced a new
getPdfUrlhelper function to abstract URL vs. data handling - Updated iframe cleanup to be scoped to the component element instead of document-wide
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| PdfReader.razor.js | Refactored print and download functions to support stream-based PDFs; added getPdfUrl helper; improved iframe lifecycle management |
| BootstrapBlazor.PdfReader.csproj | Version bump from 10.0.20 to 10.0.21 |
Comments suppressed due to low confidence (9)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:213
- Unused variable title.
const title = el.querySelector('.bb-view-pdf-dialog-title');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:214
- Unused variable author.
const author = el.querySelector('.bb-view-pdf-dialog-author');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:215
- Unused variable subject.
const subject = el.querySelector('.bb-view-pdf-dialog-subject');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:216
- Unused variable keywords.
const keywords = el.querySelector('.bb-view-pdf-dialog-keywords');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:220
- Unused variable modified.
const modified = el.querySelector('.bb-view-pdf-dialog-modified');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:240
- Unused variable webview.
const webview = el.querySelector('.bb-view-pdf-dialog-view');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:729
- The initial value of v is unused, since it is always overwritten.
let v = 100;
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:188
- Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
})
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:652
- 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.
Comment on lines
+772
to
+774
| return new Promise((resolve, reject) => { | ||
| resolve(); | ||
| }); |
There was a problem hiding this comment.
The Promise created here is unnecessary and doesn't serve any purpose. The promise is immediately resolved synchronously without waiting for any asynchronous operation. The callback should either return a meaningful promise (e.g., one that resolves when iframe.onload completes) or not return anything at all since the caller doesn't appear to need the return value.
Suggested change
| return new Promise((resolve, reject) => { | |
| resolve(); | |
| }); |
Comment on lines
+583
to
+586
|
|
||
| return new Promise((resolve, reject) => { | ||
| resolve(); | ||
| }); |
There was a problem hiding this comment.
The Promise created here is unnecessary and doesn't serve any purpose. The promise is immediately resolved synchronously without waiting for any asynchronous operation. The callback should either return a meaningful promise or not return anything at all since the caller doesn't appear to need the return value.
Suggested change
| return new Promise((resolve, reject) => { | |
| resolve(); | |
| }); |
| const blob = new Blob([options.data], { type: 'application/pdf' }); | ||
| var url = window.URL.createObjectURL(blob); | ||
| await callback(url); | ||
| window.URL.revokeObjectURL(url); |
There was a problem hiding this comment.
The blob URL is revoked immediately after calling the callback, which may cause issues if the callback uses the URL asynchronously. For example, in printPdf, the URL is assigned to iframe.src, but the actual loading happens asynchronously via the onload event. Revoking the URL before the iframe finishes loading could result in a failed load. Consider revoking the URL only after the asynchronous operation completes, or adding a delay before revocation.
| } | ||
| else if (options.data) { | ||
| const blob = new Blob([options.data], { type: 'application/pdf' }); | ||
| var url = window.URL.createObjectURL(blob); |
There was a problem hiding this comment.
Use 'const' or 'let' instead of 'var' to declare the url variable. The use of 'var' is inconsistent with modern JavaScript best practices and can lead to unexpected scoping issues.
Suggested change
| var url = window.URL.createObjectURL(blob); | |
| const url = window.URL.createObjectURL(blob); |
| document.body.appendChild(iframe); | ||
| const getPdfUrl = async (options, callback) => { | ||
| if (options.url) { | ||
| callback(options.url); |
There was a problem hiding this comment.
When options.url is provided, the callback is invoked but its return value is not awaited. This is inconsistent with line 785 where the callback is awaited when options.data is used. This inconsistency could lead to race conditions or unexpected behavior. Either await the callback in both cases or remove the await in both cases, depending on whether the callback is expected to be asynchronous.
Suggested change
| callback(options.url); | |
| await callback(options.url); |
| printPdf(options.url); | ||
| await printPdf(el, options); | ||
| await invoke.invokeMethodAsync("Printing"); | ||
| }) |
There was a problem hiding this comment.
Avoid automated semicolon insertion (95% of all statements in the enclosing function have an explicit semicolon).
Suggested change
| }) | |
| }); |
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 #840
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Support printing and downloading PDFs from either URLs or in-memory data streams in the PdfReader component.
New Features:
Enhancements: