feat(PdfReader): remove async/await getPdfUrl#845
Merged
Conversation
Reviewer's GuideRefactors the PdfReader client-side logic to remove unnecessary async/await usage for printing and downloading, introduces explicit backdrop show/hide helpers for the document properties dialog, and adjusts the markup/CSS to support a single shared backdrop overlay outside the dialog container. Sequence diagram for PdfReader toolbar download and print interactionssequenceDiagram
actor User
participant Toolbar
participant PdfReaderJs
participant getPdfUrl
participant Browser
User->>Toolbar: click bb-view-download
Toolbar->>PdfReaderJs: downloadPdf(options, fileName)
PdfReaderJs->>getPdfUrl: getPdfUrl(options, callback)
alt options.url is set
getPdfUrl-->>PdfReaderJs: callback(options.url)
else options.data is set
getPdfUrl->>Browser: createObjectURL(Blob(options.data))
Browser-->>getPdfUrl: objectUrl
getPdfUrl-->>PdfReaderJs: callback(objectUrl)
getPdfUrl->>Browser: revokeObjectURL(objectUrl)
end
PdfReaderJs->>Browser: create <a>, set href, download, click
User->>Toolbar: click bb-view-print
Toolbar->>PdfReaderJs: printPdf(el, options)
PdfReaderJs->>PdfReaderJs: ensure print iframe exists
PdfReaderJs->>getPdfUrl: getPdfUrl(options, callback)
alt options.url is set
getPdfUrl-->>PdfReaderJs: callback(options.url)
else options.data is set
getPdfUrl->>Browser: createObjectURL(Blob(options.data))
Browser-->>getPdfUrl: objectUrl
getPdfUrl-->>PdfReaderJs: callback(objectUrl)
getPdfUrl->>Browser: revokeObjectURL(objectUrl)
end
PdfReaderJs->>Browser: set iframe.src = url
Browser-->>PdfReaderJs: iframe.onload
PdfReaderJs->>Browser: iframe.contentWindow.print()
PdfReaderJs->>Toolbar: invoke.invokeMethodAsync(Printing)
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:
- The new
.bb-view-pdf-backdropelement is rendered with theinvisibleclass butshowBackdrop/hideBackdroponly toggleshow, so depending on your CSS for.invisiblethe backdrop may remain hidden even whenshowis added; consider either droppinginvisiblefrom the markup or explicitly removing/adding it in the show/hide helpers. - Now that
printPdfis synchronous, the print toolbar handler no longer needs to beasyncorawaitthe call toinvoke.invokeMethodAsync("Printing"); you can simplify this handler to a non-async function.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `.bb-view-pdf-backdrop` element is rendered with the `invisible` class but `showBackdrop`/`hideBackdrop` only toggle `show`, so depending on your CSS for `.invisible` the backdrop may remain hidden even when `show` is added; consider either dropping `invisible` from the markup or explicitly removing/adding it in the show/hide helpers.
- Now that `printPdf` is synchronous, the print toolbar handler no longer needs to be `async` or `await` the call to `invoke.invokeMethodAsync("Printing")`; you can simplify this handler to a non-async function.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor:104` </location>
<code_context>
</div>
</div>
</div>
+ <div class="bb-view-pdf-backdrop invisible"></div>
<div class="bb-view-pdf-info invisible">
- <div class="bb-view-pdf-backdrop"></div>
</code_context>
<issue_to_address>
**issue (bug_risk):** The backdrop will likely never become visible because it keeps the `invisible` class even when `.show` is toggled.
`bb-view-pdf-backdrop` is rendered with `class="bb-view-pdf-backdrop invisible"`, but `showBackdrop(el)` only adds `show` and never removes `invisible`. Since `.invisible` (in Bootstrap) uses `visibility: hidden !important`, it will override `.show`, so the backdrop likely never appears. Please either drop `invisible` from the markup or have `showBackdrop`/`hideBackdrop` toggle `invisible` instead of/along with `show`.
</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 refactors the PDF reader component by removing unnecessary async/await from the getPdfUrl function and its callers, as the operations are synchronous. Additionally, it improves the backdrop functionality for the document properties dialog by extracting backdrop management into dedicated helper functions and restructuring the HTML for better separation of concerns.
Key Changes:
- Removed async/await from
getPdfUrl,downloadPdf, andprintPdffunctions since these operations don't require asynchronous handling - Added
showBackdropandhideBackdrophelper functions to manage the backdrop display - Restructured the backdrop element in the markup to be a sibling of the info dialog rather than nested within it
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| PdfReader.razor.js | Removed async/await from getPdfUrl and related functions; added showBackdrop/hideBackdrop helper functions for backdrop management |
| PdfReader.razor | Moved backdrop element from inside bb-view-pdf-info to be a sibling element for better DOM structure |
| pdf_reader.css | Repositioned .bb-view-pdf-backdrop CSS rule to appear before .bb-view-pdf-info for better logical ordering |
| BootstrapBlazor.PdfReader.csproj | Bumped version from 10.0.21 to 10.0.22 |
Comments suppressed due to low confidence (9)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:216
- Unused variable title.
const title = el.querySelector('.bb-view-pdf-dialog-title');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:217
- Unused variable author.
const author = el.querySelector('.bb-view-pdf-dialog-author');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:218
- Unused variable subject.
const subject = el.querySelector('.bb-view-pdf-dialog-subject');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:219
- Unused variable keywords.
const keywords = el.querySelector('.bb-view-pdf-dialog-keywords');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:223
- Unused variable modified.
const modified = el.querySelector('.bb-view-pdf-dialog-modified');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:243
- Unused variable webview.
const webview = el.querySelector('.bb-view-pdf-dialog-view');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:738
- The initial value of v is unused, since it is always overwritten.
let v = 100;
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:191
- Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
})
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:661
- 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
+805
to
806
| callback(url); | ||
| window.URL.revokeObjectURL(url); |
There was a problem hiding this comment.
The callback is invoked synchronously, but the URL is immediately revoked on the next line. This creates a race condition where the URL may be revoked before it's fully used by the callback (e.g., before the download anchor is clicked or the iframe loads). Consider making the callback asynchronous or using a timeout/event listener to ensure the URL is used before being revoked.
| await printPdf(el, options); | ||
| 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).
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 #844
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Simplify the PdfReader print and download flow by removing unnecessary async/await usage and improving the document properties backdrop behavior.
Enhancements: