feat(PdfReader): support download #773
Merged
Conversation
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
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds built-in client-side download support to PdfReader by wiring the existing toolbar download button directly to a JavaScript handler that downloads the current PDF from its URL, while removing the previous .NET callback-based download hook and a debug log line. Sequence diagram for PdfReader toolbar download interactionsequenceDiagram
actor User
participant Browser
participant PdfReaderRazor as PdfReader_razor
participant PdfReaderJs as PdfReader_razor_js
User->>Browser: Click bb_view_download button
Browser->>PdfReaderJs: DOM click event on .bb_view_download
PdfReaderJs->>PdfReaderJs: EventHandler.on toolbar click handler
alt options.url is set
PdfReaderJs->>PdfReaderJs: Read docTitle from .bb_view_subject
PdfReaderJs->>PdfReaderJs: Create anchorElement
PdfReaderJs->>PdfReaderJs: Set href to options.url
PdfReaderJs->>PdfReaderJs: Set download to docTitle
PdfReaderJs->>Browser: anchorElement.click()
PdfReaderJs->>PdfReaderJs: Remove anchorElement
else options.url is not set
PdfReaderJs-->>User: No download started
end
Updated class diagram for PdfReader download-related membersclassDiagram
class PdfReader {
+string Id
+bool ShowDownload
+bool ShowPrint
+string MoreButtonIcon
+Task RotateRight()
+Task RotateLeft()
+Task Print()
+Task ZoomIn()
+Task ZoomOut()
+Task FitPage()
+Task FitWidth()
}
class PdfReaderDownloadBehaviorBefore {
+Func~Task~ OnDownloadAsync
-Task OnDownload()
}
PdfReader <|-- PdfReaderDownloadBehaviorBefore
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:
- Removing the
OnDownloadAsyncparameter is a breaking API change; consider either keeping it as an optional/obsolete path that delegates to the built-in behavior or documenting/messaging a clear migration path for consumers that relied on custom download logic. - In the toolbar click handler for
.bb-view-download, add a null/empty check for.bb-view-subjectandtextContentand fall back to a safe default filename to avoid runtime errors or empty filenames when the title element is missing or not yet populated.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Removing the `OnDownloadAsync` parameter is a breaking API change; consider either keeping it as an optional/obsolete path that delegates to the built-in behavior or documenting/messaging a clear migration path for consumers that relied on custom download logic.
- In the toolbar click handler for `.bb-view-download`, add a null/empty check for `.bb-view-subject` and `textContent` and fall back to a safe default filename to avoid runtime errors or empty filenames when the title element is missing or not yet populated.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:440-447` </location>
<code_context>
pdfViewer.spreadMode = 0;
}
});
+ EventHandler.on(toolbar, "click", ".bb-view-download", e => {
+ if (options.url) {
+ const docTitle = el.querySelector('.bb-view-subject');
+ const anchorElement = document.createElement('a');
+ anchorElement.href = options.url;
+ anchorElement.download = docTitle.textContent;
+ anchorElement.click();
+ anchorElement.remove();
+ }
+ });
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against missing `.bb-view-subject` element when deriving the download filename.
If `.bb-view-subject` is missing, `querySelector` returns `null` and `docTitle.textContent` will throw. Please handle the missing/empty case and fall back to a default filename, e.g. deriving it from `options.url` (such as `new URL(options.url).pathname.split('/').pop()`), before setting `anchorElement.download`.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:443-447` </location>
<code_context>
+ EventHandler.on(toolbar, "click", ".bb-view-download", e => {
+ if (options.url) {
+ const docTitle = el.querySelector('.bb-view-subject');
+ const anchorElement = document.createElement('a');
+ anchorElement.href = options.url;
+ anchorElement.download = docTitle.textContent;
+ anchorElement.click();
+ anchorElement.remove();
+ }
+ });
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider appending the temporary anchor to the DOM before triggering `click()` for better cross‑browser reliability.
Some browsers don’t reliably trigger `click()` on elements that aren’t in the DOM. To avoid compatibility issues, append the anchor to `document.body` before clicking and then remove it, e.g. `document.body.appendChild(anchorElement); anchorElement.click(); anchorElement.remove();`.
```suggestion
const anchorElement = document.createElement('a');
anchorElement.href = options.url;
anchorElement.download = docTitle.textContent;
document.body.appendChild(anchorElement);
anchorElement.click();
anchorElement.remove();
```
</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 PR implements client-side PDF download functionality by replacing the previous callback-based approach with a direct JavaScript implementation. The change removes the C# OnDownloadAsync parameter and implements the download directly in the browser using a dynamically created anchor element.
Key Changes:
- Removed
OnDownloadAsyncparameter andOnDownload()method from C# code - Added JavaScript event handler for
.bb-view-downloadbutton that creates an anchor element with the PDF URL and triggers download - Cleaned up debug console.log statement from metadata loading function
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| PdfReader.razor.js | Added click handler for download button that creates anchor element with download attribute; removed debug console.log |
| PdfReader.razor.cs | Removed OnDownloadAsync parameter and OnDownload() callback method (breaking change) |
| PdfReader.razor | Removed @onclick binding from download button div to use JavaScript event handler instead |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const docTitle = el.querySelector('.bb-view-subject'); | ||
| const anchorElement = document.createElement('a'); | ||
| anchorElement.href = options.url; | ||
| anchorElement.download = docTitle.textContent; |
There was a problem hiding this comment.
The docTitle element should be null-checked before accessing its textContent property. If the element doesn't exist, this will throw a runtime error. Consider adding a null check:
const docTitle = el.querySelector('.bb-view-subject');
if (docTitle && docTitle.textContent) {
anchorElement.download = docTitle.textContent;
}
Suggested change
| anchorElement.download = docTitle.textContent; | |
| anchorElement.download = (docTitle && docTitle.textContent) ? docTitle.textContent : ''; |
Comment on lines
+440
to
+447
| EventHandler.on(toolbar, "click", ".bb-view-download", e => { | ||
| if (options.url) { | ||
| const docTitle = el.querySelector('.bb-view-subject'); | ||
| const anchorElement = document.createElement('a'); | ||
| anchorElement.href = options.url; | ||
| anchorElement.download = docTitle.textContent; | ||
| anchorElement.click(); | ||
| anchorElement.remove(); |
There was a problem hiding this comment.
The download implementation using anchor element with download attribute will not work for cross-origin URLs due to CORS restrictions. When the PDF URL is from a different origin, the browser will navigate to the URL instead of downloading it. This is different from the print functionality which uses an iframe. Consider:
- Documenting this limitation in the component documentation, or
- Implementing a server-side proxy download mechanism similar to how the previous
OnDownloadAsynccallback allowed, or - Using a fetch + Blob approach for same-origin URLs:
if (options.url) {
const docTitle = el.querySelector('.bb-view-subject');
try {
const response = await fetch(options.url);
const blob = await response.blob();
const url = window.URL.createObjectURL(blob);
const anchorElement = document.createElement('a');
anchorElement.href = url;
anchorElement.download = docTitle?.textContent || 'document.pdf';
anchorElement.click();
window.URL.revokeObjectURL(url);
anchorElement.remove();
} catch (error) {
// Fallback to direct link for cross-origin URLs
const anchorElement = document.createElement('a');
anchorElement.href = options.url;
anchorElement.download = docTitle?.textContent || 'document.pdf';
anchorElement.click();
anchorElement.remove();
}
}
Suggested change
| EventHandler.on(toolbar, "click", ".bb-view-download", e => { | |
| if (options.url) { | |
| const docTitle = el.querySelector('.bb-view-subject'); | |
| const anchorElement = document.createElement('a'); | |
| anchorElement.href = options.url; | |
| anchorElement.download = docTitle.textContent; | |
| anchorElement.click(); | |
| anchorElement.remove(); | |
| EventHandler.on(toolbar, "click", ".bb-view-download", async e => { | |
| if (options.url) { | |
| const docTitle = el.querySelector('.bb-view-subject'); | |
| try { | |
| const response = await fetch(options.url); | |
| if (!response.ok) throw new Error("Network response was not ok"); | |
| const blob = await response.blob(); | |
| const url = window.URL.createObjectURL(blob); | |
| const anchorElement = document.createElement('a'); | |
| anchorElement.href = url; | |
| anchorElement.download = docTitle?.textContent || 'document.pdf'; | |
| document.body.appendChild(anchorElement); | |
| anchorElement.click(); | |
| window.URL.revokeObjectURL(url); | |
| anchorElement.remove(); | |
| } catch (error) { | |
| // Fallback to direct link for cross-origin URLs | |
| const anchorElement = document.createElement('a'); | |
| anchorElement.href = options.url; | |
| anchorElement.download = docTitle?.textContent || 'document.pdf'; | |
| document.body.appendChild(anchorElement); | |
| anchorElement.click(); | |
| anchorElement.remove(); | |
| } |
Comment on lines
121
to
122
| [Parameter] | ||
| public string? MoreButtonIcon { get; set; } |
There was a problem hiding this comment.
Removing the OnDownloadAsync parameter is a breaking API change that will affect existing consumers of this component. Users who were relying on this callback to implement custom download logic (e.g., logging, authentication checks, custom file sources) will no longer be able to do so.
Consider:
- Deprecating this parameter instead of removing it immediately, marking it as
[Obsolete]in this version - Supporting both the new client-side download and the callback-based approach, allowing the callback to override the default behavior
- Documenting this as a breaking change in the release notes
If this breaking change is intentional, ensure it's clearly documented in the changelog and migration guide.
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 #772
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add built-in client-side download support to the PdfReader toolbar using the existing PDF URL and simplify related component parameters.
New Features:
Enhancements: