Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates the PdfReader JavaScript to safely interact with optional toolbar and metadata elements so that PDFs still render correctly when the toolbar or title elements are absent. Flow diagram for loadMetadata handling optional docTitle elementflowchart TD
A[loadMetadata called with el, pdfViewer, metadata] --> B[Find filename element .bb-view-pdf-dialog-filename]
B --> C[Find docTitle element .bb-view-subject]
C --> D{docTitle exists?}
D -->|Yes| E[Set filename.textContent to docTitle.textContent]
D -->|No| F[Skip setting filename text]
E --> G[Find filesize element .bb-view-pdf-dialog-filesize]
F --> G[Find filesize element .bb-view-pdf-dialog-filesize]
G --> H["Set filesize.textContent to getFilesize(metadata)"]
Flow diagram for resetToolbarView with optional toolbar elementsflowchart TD
A[resetToolbarView called with el, pdfViewer] --> B[Find scaleEl .bb-view-scale-input]
B --> C{scaleEl exists?}
C -->|Yes| D[updateScaleValue with pdfViewer.currentScale]
C -->|No| E[Skip scale update]
D --> F[Find pageEl .bb-view-num]
E --> F[Find pageEl .bb-view-num]
F --> G{pageEl exists?}
G -->|Yes| H[Set pageEl.value to pdfViewer.currentPageNumber]
G -->|No| I[Skip page value update]
H --> J[Find group .bb-view-group-rotate]
I --> J[Find group .bb-view-group-rotate]
J --> K{group exists?}
K -->|Yes| L[Remove rotate-left and rotate-right classes]
K -->|No| M[Skip rotate reset]
L --> N[Find twoPagesOneView .dropdown-item-pages]
M --> N[Find twoPagesOneView .dropdown-item-pages]
N --> O{twoPagesOneView exists?}
O -->|Yes| P{pdfViewer.spreadMode === 1?}
O -->|No| S[Skip spread mode UI update]
P -->|Yes| Q[Add active class]
P -->|No| R[Remove active class]
Q --> T[Delete el.widths]
R --> T[Delete el.widths]
S --> T[Delete el.widths]
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:
- Now that several toolbar-related elements are optional, consider centralizing the null-check logic (e.g., a small helper like
setTextIfExists(selector, value)) to avoid repetitivequerySelector+ifpatterns and keep future changes less error-prone. - In
resetToolbarView,scaleElandpageElare queried but only used to gate calls toupdateScaleValue/pageEl.value; if those functions can safely handle missing elements, you could pass the element into them or early-return when the toolbar is disabled to simplify the branching.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Now that several toolbar-related elements are optional, consider centralizing the null-check logic (e.g., a small helper like `setTextIfExists(selector, value)`) to avoid repetitive `querySelector` + `if` patterns and keep future changes less error-prone.
- In `resetToolbarView`, `scaleEl` and `pageEl` are queried but only used to gate calls to `updateScaleValue` / `pageEl.value`; if those functions can safely handle missing elements, you could pass the element into them or early-return when the toolbar is disabled to simplify the branching.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 fixes a bug where the PdfReader component fails to render PDFs when the ShowToolbar parameter is set to false. The fix adds null checks for DOM elements that may not exist when the toolbar is hidden, preventing JavaScript runtime errors.
- Adds defensive null checks for toolbar-related DOM elements in
resetToolbarViewfunction - Adds a null check for the
docTitleelement inloadMetadatafunction - Bumps the package version from 10.0.15 to 10.0.16
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js | Adds null checks for toolbar elements (scaleEl, pageEl, twoPagesOneView, docTitle) that may not exist when ShowToolbar is false |
| src/components/BootstrapBlazor.PdfReader/BootstrapBlazor.PdfReader.csproj | Version bump from 10.0.15 to 10.0.16 for the bug fix release |
Comments suppressed due to low confidence (9)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:233
- Multiple DOM elements in loadMetadata are accessed without null checks:
filesize(line 205),created(line 212),application(line 217),producer(line 220),version(line 223),count(line 226), andsize(line 229). These elements are part of a PDF info dialog that may not be rendered when ShowToolbar is false. Without null checks, accessing.textContenton these null elements will cause runtime errors. Add null checks for all these elements similar to the pattern used fordocTitle.
const filesize = el.querySelector('.bb-view-pdf-dialog-filesize');
filesize.textContent = getFilesize(metadata);
const title = el.querySelector('.bb-view-pdf-dialog-title');
const author = el.querySelector('.bb-view-pdf-dialog-author');
const subject = el.querySelector('.bb-view-pdf-dialog-subject');
const keywords = el.querySelector('.bb-view-pdf-dialog-keywords');
const created = el.querySelector('.bb-view-pdf-dialog-created');
created.textContent = parsePdfDate(metadata.info.CreationDate)?.toLocaleString();
const modified = el.querySelector('.bb-view-pdf-dialog-modified');
const application = el.querySelector('.bb-view-pdf-dialog-application');
application.textContent = metadata.info.Creator;
const producer = el.querySelector('.bb-view-pdf-dialog-producer');
producer.textContent = metadata.info.Producer;
const version = el.querySelector('.bb-view-pdf-dialog-version');
version.textContent = metadata.info.PDFFormatVersion;
const count = el.querySelector('.bb-view-pdf-dialog-count');
count.textContent = pdfViewer.pagesCount;
const size = el.querySelector('.bb-view-pdf-dialog-size');
pdfViewer.pdfDocument.getPage(pdfViewer.currentPageNumber).then(page => {
const viewport = page.getViewport({ scale: 1 });
size.textContent = `${(viewport.width / 72).toFixed(2)} * ${(viewport.height / 72).toFixed(2)} in (portrait)`;
});
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:208
- Unused variable title.
const title = el.querySelector('.bb-view-pdf-dialog-title');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:209
- Unused variable author.
const author = el.querySelector('.bb-view-pdf-dialog-author');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:210
- Unused variable subject.
const subject = el.querySelector('.bb-view-pdf-dialog-subject');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:211
- Unused variable keywords.
const keywords = el.querySelector('.bb-view-pdf-dialog-keywords');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:215
- Unused variable modified.
const modified = el.querySelector('.bb-view-pdf-dialog-modified');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:235
- Unused variable webview.
const webview = el.querySelector('.bb-view-pdf-dialog-webview');
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:705
- The initial value of v is unused, since it is always overwritten.
let v = 100;
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:518
- Avoid automated semicolon insertion (95% of all statements in the enclosing function have an explicit semicolon).
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const filename = el.querySelector('.bb-view-pdf-dialog-filename'); | ||
| const docTitle = el.querySelector('.bb-view-subject'); | ||
| filename.textContent = docTitle.textContent; | ||
| if (docTitle) { |
There was a problem hiding this comment.
The null check for docTitle is added, but filename (line 199) is still accessed without a null check. If docTitle is null when ShowToolbar is false, then filename is also likely to be null. This will cause a runtime error when trying to set filename.textContent if docTitle is null. Add a null check for filename as well, or wrap both checks together.
Suggested change
| if (docTitle) { | |
| if (filename && docTitle) { |
| } | ||
| } | ||
|
|
||
| delete el.widths |
There was a problem hiding this comment.
Avoid automated semicolon insertion (91% of all statements in the enclosing function have an explicit semicolon).
Suggested change
| delete el.widths | |
| delete el.widths; |
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 #822
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Bug Fixes: