Conversation
Reviewer's GuideAdds localization support and UI affordances to the PdfReader component while simplifying two-page view wiring and enhancing thumbnails and dropdown behavior. Sequence diagram for two page view toggle in PdfReader toolbarsequenceDiagram
actor User
participant PdfReaderToolbar as PdfReaderToolbar
participant PdfReaderJs as PdfReader_razor_js
participant PdfViewer
User->>PdfReaderToolbar: Click dropdown-item-pages
PdfReaderToolbar->>PdfReaderJs: DOM click event on .dropdown-item-pages
PdfReaderJs->>PdfReaderJs: Toggle active class on clicked item
alt spreadMode is not 1
PdfReaderJs->>PdfViewer: set spreadMode = 1
else spreadMode is 1
PdfReaderJs->>PdfViewer: set spreadMode = 0
end
Class diagram for updated PdfReader localization and view optionsclassDiagram
class PdfReader {
+bool ShowToolbar
+bool ShowDownload
+bool ShowPrint
+bool ShowTwoPagesOneView
+bool ShowPresentationMode
+bool EnableTwoPagesOneView
+uint CurrentPage
+string CurrentScale
+string Url
+string? MoreButtonIcon
+Func~Task~ OnPrintingAsync
+EventCallback OnDownload
+EventCallback OnPrinted
-uint _currentPage
-string _currentScale
-string _url
-string _dropdownItemCheckIcon
-string _dropdownItemDefaultIcon
-IStringLocalizer_PdfReader_ Localizer
+string CurrentPageString
+string CurrentScaleString
+override void OnParametersSet()
+override Task OnAfterRenderAsync(bool firstRender)
+void SetFitMode(PdfReaderFitMode mode)
+void RotateLeft()
+void RotateRight()
}
class PdfReaderFitMode {
<<enumeration>>
PageActual
PageWidth
PageHeight
PageFit
Auto
}
PdfReader --> PdfReaderFitMode : uses
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:
Blocking issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
label.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- In PdfReader.razor.js, the
minus/pluselement queries were removed from thepagesinithandler, but the variables are still referenced later in the same function when attaching click handlers, which will cause a runtime error; either restore the queries or move them to a shared scope that is still valid. - The
EnableTwoPagesOneViewandShowTwoPagesOneViewparameters are no longer wired to any JavaScript calls or internal state updates (the Blazor-side toggle handler was removed and the JS now manages state purely via CSS classes), which can lead to confusion or stale API surface; consider either re-synchronizing these parameters with the client state or deprecating/removing them if they are no longer meant to control behavior. - The presentation mode dropdown item currently only toggles the
.activeclass and has its fullscreen logic commented out; if this is intended to be a functional feature rather than a placeholder, consider either implementing the fullscreen behavior or clearly separating it as a future enhancement to avoid a non-functional UI control.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In PdfReader.razor.js, the `minus`/`plus` element queries were removed from the `pagesinit` handler, but the variables are still referenced later in the same function when attaching click handlers, which will cause a runtime error; either restore the queries or move them to a shared scope that is still valid.
- The `EnableTwoPagesOneView` and `ShowTwoPagesOneView` parameters are no longer wired to any JavaScript calls or internal state updates (the Blazor-side toggle handler was removed and the JS now manages state purely via CSS classes), which can lead to confusion or stale API surface; consider either re-synchronizing these parameters with the client state or deprecating/removing them if they are no longer meant to control behavior.
- The presentation mode dropdown item currently only toggles the `.active` class and has its fullscreen logic commented out; if this is intended to be a functional feature rather than a placeholder, consider either implementing the fullscreen behavior or clearly separating it as a future enhancement to avoid a non-functional UI control.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:125-134` </location>
<code_context>
+ pdfViewer.spreadMode = 0;
+ }
+ });
+ EventHandler.on(controls, "click", ".dropdown-item-presentation", async e => {
+ e.delegateTarget.classList.toggle("active");
+
+ //if (pdfViewer.isInPresentationMode) {
+ // document.exitFullscreen();
+ //}
+ //else {
+ // el.requestFullscreen();
+ //}
+ });
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The presentation mode dropdown currently only toggles CSS state and doesn’t affect the actual viewer presentation mode.
The click handler for `.dropdown-item-presentation` only toggles the `active` class; the code that actually enters/exits fullscreen (`pdfViewer` presentation mode) is fully commented out. Consider either wiring this handler to `pdfViewer`’s presentation/fullscreen API and keeping the UI state in sync, or gating this menu item behind `ShowPresentationMode` (or similar) until it’s fully implemented, so users don’t see a control that appears to do nothing beyond changing the icon state.
```suggestion
EventHandler.on(controls, "click", ".dropdown-item-presentation", async e => {
e.preventDefault();
// If the browser doesn't support the fullscreen API, don't pretend the control works.
if (!document.fullscreenEnabled) {
return;
}
const item = e.delegateTarget;
// Toggle fullscreen on the root element for this viewer.
if (document.fullscreenElement === el) {
await document.exitFullscreen();
} else {
// Some browsers require this to be called from the click handler directly.
await el.requestFullscreen();
}
});
// Keep the "active" state of the presentation dropdown item in sync with fullscreen state.
document.addEventListener("fullscreenchange", () => {
const item = controls.querySelector(".dropdown-item-presentation");
if (!item) {
return;
}
if (document.fullscreenElement === el) {
item.classList.add("active");
} else {
item.classList.remove("active");
}
});
```
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:220` </location>
<code_context>
label.innerHTML = `${i + 1}`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:220` </location>
<code_context>
label.innerHTML = `${i + 1}`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `label.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</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 adds localization support to the PdfReader component, enabling internationalization of the UI elements. The implementation follows the established pattern used in other components by injecting IStringLocalizer<PdfReader> and providing English and Chinese locale files. Additionally, it refactors the two-page view toggle logic from C# to JavaScript, adds a new presentation mode feature (currently non-functional), and enhances the thumbnail display with page numbers.
Key Changes
- Added localization support with English and Chinese locale files for all toolbar buttons and menu items
- Refactored two-page view toggle from C# method-based to JavaScript event-based handling
- Added presentation mode UI (feature implementation incomplete)
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| PdfReader.razor.cs | Added IStringLocalizer injection, removed OnToggleTwoPagesOneView method, added ShowPresentationMode parameter, updated icon field names |
| PdfReader.razor | Replaced hardcoded UI strings with localized strings using @Localizer, added presentation mode menu item, updated dropdown structure with check/default icons |
| PdfReader.razor.js | Removed setPages and resetTwoPagesOneView functions, moved two-page toggle logic to inline event handler, added presentation mode event handler (commented out) |
| PdfReader.razor.css | Removed .init class styling, added dropdown menu active state styling, enhanced thumbnail layout with labels |
| Locales/en.json | New file with English translations for 11 UI elements |
| Locales/zh.json | New file with Chinese translations for 11 UI elements |
| BootstrapBlazor.PdfReader.csproj | Updated version to 10.0.1, added locale file embedding, changed from PackageReference to ProjectReference (problematic) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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 #732
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Localize and enhance the PdfReader toolbar and viewer controls, including dropdown behavior and thumbnail rendering.
New Features:
Enhancements: