feat(PdfReader): add OnPageChangedAsync parameter#719
Merged
Conversation
Reviewer's GuideAdds configurable zoom/scale behavior to PdfReader, wiring Blazor options and bindings through to the underlying PDF.js viewer, introducing toolbar controls for zoom in/out with proper state, and cleaning up legacy canvas-based navigation/zoom logic. Sequence diagram for PdfReader zoom interaction and scale synchronizationsequenceDiagram
actor User
participant Toolbar as PdfReaderToolbar
participant JS as PdfReader_razor_js
participant PdfJs as PDFJS_Viewer
participant Blazor as PdfReaderComponent
User->>Toolbar: Click zoom plus icon
Toolbar->>JS: DOM click event on bb_page_plus
JS->>JS: updateScale(pdfViewer, button, 1)
JS->>PdfJs: set currentScaleValue
PdfJs-->>JS: scalechanging event
JS->>Toolbar: Update bb_view_scale input value
Toolbar->>Blazor: Bound CurrentScaleString changed
Blazor->>Blazor: SetCurrentScale(value)
Blazor->>JS: InvokeVoidAsync scale(Id, Options.CurrentScale)
JS->>PdfJs: Apply currentScaleValue from Options.CurrentScale
User->>Toolbar: Type custom zoom in scale input
Toolbar->>Blazor: CurrentScaleString updated via binding
Blazor->>Blazor: SetCurrentScale(value)
Blazor->>JS: InvokeVoidAsync scale(Id, Options.CurrentScale)
JS->>PdfJs: Apply currentScaleValue from Options.CurrentScale
Updated class diagram for PdfReader zoom and scale optionsclassDiagram
class PdfReaderOptions {
+string? Url
+uint CurrentPage
+string? CurrentScale
+bool IsFitToPage
+bool IsFitToWidth
}
class PdfReader {
-bool _isFitToPage
-uint _currentPage
-string? _url
-string? _currentScale
-HashSet~string~ AllowedScaleValues
string CurrentPageString
void SetCurrentPage(string value)
string CurrentScaleString
void SetCurrentScale(string value)
Task OnAfterRenderAsync(bool firstRender)
}
PdfReader --> PdfReaderOptions : uses
File-Level Changes
Assessment against 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:
- Several JS functions now destructure
Data.get(id)directly (e.g.fitToWidth,fitToPage,rotate,navigateToPage,scale), which will throw if no entry exists for that id; consider null-checking the result before destructuring as was done previously. - In
dispose,minusandpluselements are queried and passed toEventHandler.offwithout null-checks; it would be safer to guard against the elements not being found to avoid runtime errors on partially rendered or already-removed DOM. - The
AllowedScaleValuesfield inPdfReader.razor.csis currently unused; either wire it into the scale parsing/validation logic (e.g., to supportpage-width,page-fit, etc.) or remove it to avoid dead code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several JS functions now destructure `Data.get(id)` directly (e.g. `fitToWidth`, `fitToPage`, `rotate`, `navigateToPage`, `scale`), which will throw if no entry exists for that id; consider null-checking the result before destructuring as was done previously.
- In `dispose`, `minus` and `plus` elements are queried and passed to `EventHandler.off` without null-checks; it would be safer to guard against the elements not being found to avoid runtime errors on partially rendered or already-removed DOM.
- The `AllowedScaleValues` field in `PdfReader.razor.cs` is currently unused; either wire it into the scale parsing/validation logic (e.g., to support `page-width`, `page-fit`, etc.) or remove it to avoid dead code.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:150-151` </location>
<code_context>
-
- pdfInstances[this.id] = this;
- }
+ EventHandler.on(minus, "click", e => updateScale(pdfViewer, e.target, -1));
+ EventHandler.on(plus, "click", e => updateScale(pdfViewer, e.target, 1));
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Using e.target in the click handler means the disabled-state check will run against the inner <i> element instead of the button container.
As a result, `updateScale` receives the inner `<i>` instead of the `.bb-view-icon` element that actually has the `disabled` class, so `button.classList.contains('disabled')` never succeeds and scaling still occurs when the control appears disabled. Please pass the element that owns the `disabled` class instead (e.g., via `e.currentTarget` or `e.target.closest('.bb-view-icon')`).
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:161-170` </location>
<code_context>
- renderPage(pdf, num);
+ const scale = pdfViewer.currentScale;
+ const current = Math.round(parseFloat(scale * 100), 0);
+ const step = [25, 33, 50, 67, 75, 80, 90, 100, 110, 125, 150, 175, 200, 250, 300, 400, 500];
+ const findValues = step.filter(s => rate > 0 ? current < s : current > s);
+ let v = 100;
+ if (rate > 0) {
+ v = findValues.shift();
}
+ else {
+ v = findValues.pop();
+ }
+ pdfViewer.currentScaleValue = v / 100;
}
</code_context>
<issue_to_address>
**issue:** Scaling beyond min/max steps can produce NaN when there is no next/previous step.
At 25% with another minus click, or at 500% with another plus click, `findValues` is empty. `shift()`/`pop()` then return `undefined`, and `v / 100` becomes `NaN`, putting `currentScaleValue` into an invalid state. Please clamp to the nearest bound when `findValues` is empty (e.g., keep the current value or explicitly set 25/500) and ensure the disabled state reflects that limit.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:173-174` </location>
<code_context>
+ Data.remove(id);
- invoke.invokeMethodAsync('SetPdfViewerMetaData', { pagesCount: pdf.pagesCount, pageNumber: pdf.pageNum });
+ const el = document.getElementById(id);
+ if (el) {
+ const minus = el.querySelector(".bb-page-minus");
+ const plus = el.querySelector(".bb-page-plus");
+ EventHandler.off(minus, "click");
+ EventHandler.off(plus, "click");
+ }
}
</code_context>
<issue_to_address>
**suggestion:** dispose assumes toolbar elements always exist, which can throw if the markup changes.
In `dispose`, `minus` and `plus` may be `null` if the expected toolbar elements are missing or the template changes, leading to `EventHandler.off` being called with `null`. Please guard these calls (e.g., `if (minus) EventHandler.off(minus, "click");` / `if (plus) ...`) to avoid potential runtime errors.
```suggestion
export function dispose(id) {
Data.remove(id);
const el = document.getElementById(id);
if (!el) {
return;
}
const minus = el.querySelector(".bb-page-minus");
if (minus) {
EventHandler.off(minus, "click");
}
const plus = el.querySelector(".bb-page-plus");
if (plus) {
EventHandler.off(plus, "click");
}
```
</issue_to_address>
### Comment 4
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor:17` </location>
<code_context>
- <input type="text" class="bb-view-scale" value="100%" />
- <div class="bb-view-icon"><i class="fa-solid fa-plus"></i></div>
+ <div class="bb-view-icon bb-page-minus"><i class="fa-solid fa-minus"></i></div>
+ <input type="text" class="bb-view-scale" @bind="CurrentScaleString" />
+ <div class="bb-view-icon bb-page-plus"><i class="fa-solid fa-plus"></i></div>
<div class="bb-view-divider"></div>
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The JS-driven scale changes don’t propagate back to the Blazor-bound CurrentScaleString, which can desync component state.
The `scalechanging` handler updates `scaleEl.value` but never fires `input`/`change` or calls back into .NET, so Blazor is unaware of zoom changes made via the buttons or viewer controls. That means `CurrentScaleString` / `_currentScale` / `Options.CurrentScale` can diverge from the actual viewer scale. To keep them in sync, either dispatch an `input` event on `scaleEl` after updating it, or invoke a .NET method from JS (like `pageChanged`) to update `Options.CurrentScale`.
Suggested implementation:
```
eventBus.on('scalechanging', function (evt) {
var predefinedValue = evt.presetValue || evt.scale;
var scaleSelect = container.querySelector('.bb-view-scale');
if (!scaleSelect) {
return;
}
if (predefinedValue) {
scaleSelect.value = predefinedValue;
} else if (evt.scale) {
scaleSelect.value = Math.round(evt.scale * 100) + '%';
}
// Ensure Blazor's @bind="CurrentScaleString" is updated when the scale is changed from JS
try {
if (typeof Event === 'function') {
// Fire both input and change so any Blazor/value-change handlers are triggered
scaleSelect.dispatchEvent(new Event('input', { bubbles: true }));
scaleSelect.dispatchEvent(new Event('change', { bubbles: true }));
}
} catch (e) {
// Fallback for very old browsers; Blazor apps generally don't target these, so no-op is fine
}
});
```
Because I only see a fragment of the Razor markup and not the JavaScript block, you may need to adjust the SEARCH section to match your actual `scalechanging` handler. Concretely:
1. Locate the code that subscribes to the PDF.js `scalechanging` event (or equivalent) inside `PdfReader.razor` (often in a `<script>` block or a referenced JS file if inlined via @inject IJSRuntime).
2. Within that handler, immediately after you assign to the `.bb-view-scale` element’s `value` (the element bound via `@bind="CurrentScaleString"`), add:
- `scaleSelect.dispatchEvent(new Event('input', { bubbles: true }));`
- `scaleSelect.dispatchEvent(new Event('change', { bubbles: true }));`
3. Ensure that the query selector (`container.querySelector('.bb-view-scale')`) is targeting the same input you bound with `@bind="CurrentScaleString"`; if your variable is named `scaleEl` instead of `scaleSelect`, update the snippet accordingly.
4. If your `scalechanging` handler currently uses different variable names or structure, keep the existing logic and only add the two `dispatchEvent` calls right after the `value` assignment to keep Blazor’s `CurrentScaleString` / `_currentScale` / `Options.CurrentScale` in sync with the actual viewer scale.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+161
to
+170
| const step = [25, 33, 50, 67, 75, 80, 90, 100, 110, 125, 150, 175, 200, 250, 300, 400, 500]; | ||
| const findValues = step.filter(s => rate > 0 ? current < s : current > s); | ||
| let v = 100; | ||
| if (rate > 0) { | ||
| v = findValues.shift(); | ||
| } | ||
| else { | ||
| v = findValues.pop(); | ||
| } | ||
| pdfViewer.currentScaleValue = v / 100; |
There was a problem hiding this comment.
issue: Scaling beyond min/max steps can produce NaN when there is no next/previous step.
At 25% with another minus click, or at 500% with another plus click, findValues is empty. shift()/pop() then return undefined, and v / 100 becomes NaN, putting currentScaleValue into an invalid state. Please clamp to the nearest bound when findValues is empty (e.g., keep the current value or explicitly set 25/500) and ensure the disabled state reflects that limit.
Comment on lines
+173
to
+174
| export function dispose(id) { | ||
| Data.remove(id); |
There was a problem hiding this comment.
suggestion: dispose assumes toolbar elements always exist, which can throw if the markup changes.
In dispose, minus and plus may be null if the expected toolbar elements are missing or the template changes, leading to EventHandler.off being called with null. Please guard these calls (e.g., if (minus) EventHandler.off(minus, "click"); / if (plus) ...) to avoid potential runtime errors.
Suggested change
| export function dispose(id) { | |
| Data.remove(id); | |
| export function dispose(id) { | |
| Data.remove(id); | |
| const el = document.getElementById(id); | |
| if (!el) { | |
| return; | |
| } | |
| const minus = el.querySelector(".bb-page-minus"); | |
| if (minus) { | |
| EventHandler.off(minus, "click"); | |
| } | |
| const plus = el.querySelector(".bb-page-plus"); | |
| if (plus) { | |
| EventHandler.off(plus, "click"); | |
| } |
| <input type="text" class="bb-view-scale" value="100%" /> | ||
| <div class="bb-view-icon"><i class="fa-solid fa-plus"></i></div> | ||
| <div class="bb-view-icon bb-page-minus"><i class="fa-solid fa-minus"></i></div> | ||
| <input type="text" class="bb-view-scale" @bind="CurrentScaleString" /> |
There was a problem hiding this comment.
suggestion (bug_risk): The JS-driven scale changes don’t propagate back to the Blazor-bound CurrentScaleString, which can desync component state.
The scalechanging handler updates scaleEl.value but never fires input/change or calls back into .NET, so Blazor is unaware of zoom changes made via the buttons or viewer controls. That means CurrentScaleString / _currentScale / Options.CurrentScale can diverge from the actual viewer scale. To keep them in sync, either dispatch an input event on scaleEl after updating it, or invoke a .NET method from JS (like pageChanged) to update Options.CurrentScale.
Suggested implementation:
eventBus.on('scalechanging', function (evt) {
var predefinedValue = evt.presetValue || evt.scale;
var scaleSelect = container.querySelector('.bb-view-scale');
if (!scaleSelect) {
return;
}
if (predefinedValue) {
scaleSelect.value = predefinedValue;
} else if (evt.scale) {
scaleSelect.value = Math.round(evt.scale * 100) + '%';
}
// Ensure Blazor's @bind="CurrentScaleString" is updated when the scale is changed from JS
try {
if (typeof Event === 'function') {
// Fire both input and change so any Blazor/value-change handlers are triggered
scaleSelect.dispatchEvent(new Event('input', { bubbles: true }));
scaleSelect.dispatchEvent(new Event('change', { bubbles: true }));
}
} catch (e) {
// Fallback for very old browsers; Blazor apps generally don't target these, so no-op is fine
}
});
Because I only see a fragment of the Razor markup and not the JavaScript block, you may need to adjust the SEARCH section to match your actual scalechanging handler. Concretely:
- Locate the code that subscribes to the PDF.js
scalechangingevent (or equivalent) insidePdfReader.razor(often in a<script>block or a referenced JS file if inlined via @Inject IJSRuntime). - Within that handler, immediately after you assign to the
.bb-view-scaleelement’svalue(the element bound via@bind="CurrentScaleString"), add:scaleSelect.dispatchEvent(new Event('input', { bubbles: true }));scaleSelect.dispatchEvent(new Event('change', { bubbles: true }));
- Ensure that the query selector (
container.querySelector('.bb-view-scale')) is targeting the same input you bound with@bind="CurrentScaleString"; if your variable is namedscaleElinstead ofscaleSelect, update the snippet accordingly. - If your
scalechanginghandler currently uses different variable names or structure, keep the existing logic and only add the twodispatchEventcalls right after thevalueassignment to keep Blazor’sCurrentScaleString/_currentScale/Options.CurrentScalein sync with the actual viewer scale.
There was a problem hiding this comment.
Pull request overview
This PR adds scale/zoom functionality to the PdfReader component, allowing users to adjust the PDF view scale through UI controls and programmatic API. Despite the PR title mentioning "OnPageChangedAsync", this callback already existed and the primary changes focus on implementing interactive zoom controls with predefined scale steps (25%-500%).
Key Changes:
- Added
CurrentScaleproperty to control PDF zoom level programmatically - Implemented interactive scale controls (plus/minus buttons) with disabled state management
- Refactored JavaScript to support scale operations and proper event handling with cleanup
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| PdfReaderOptions.cs | Added CurrentScale string property to store zoom percentage |
| PdfReader.razor.js | Added scale control functions (scale, navigateToPage), refactored Data storage to include both element and viewer, implemented updateScale with predefined steps, and added proper disposal with EventHandler cleanup |
| PdfReader.razor.cs | Added CurrentScaleString property with validation (25%-500% range), implemented scale change detection in OnAfterRenderAsync, added unused AllowedScaleValues field |
| PdfReader.razor | Updated toolbar to bind scale input and add CSS classes for plus/minus buttons, corrected icon rotation for fit-to-page button |
| PdfReader.razor.css | Added .disabled state styling for icons |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static instances = pdfInstances; | ||
| static getPdf = getPdf; | ||
| eventBus.on("scalechanging", evt => { | ||
| scaleEl.value = `${Math.round(evt.scale * 100, 0)}%`; |
There was a problem hiding this comment.
The scalechanging event handler only updates the scale text value but doesn't update the disabled state of the minus/plus buttons. This means when the scale reaches 25% or 500% through user interaction (clicking buttons or typing a value), the buttons won't be disabled until the C# side triggers the scale function. Consider adding button state updates here, similar to lines 92-101, to ensure immediate UI feedback:
const scaleValue = Math.round(evt.scale * 100, 0);
if (scaleValue <= 25) {
minus.classList.add("disabled");
} else if (scaleValue >= 500) {
plus.classList.add("disabled");
} else {
minus.classList.remove("disabled");
plus.classList.remove("disabled");
}
Suggested change
| scaleEl.value = `${Math.round(evt.scale * 100, 0)}%`; | |
| const scaleValue = Math.round(evt.scale * 100, 0); | |
| scaleEl.value = `${scaleValue}%`; | |
| if (scaleValue <= 25) { | |
| minus.classList.add("disabled"); | |
| } else { | |
| minus.classList.remove("disabled"); | |
| } | |
| if (scaleValue >= 500) { | |
| plus.classList.add("disabled"); | |
| } else { | |
| plus.classList.remove("disabled"); | |
| } |
Comment on lines
+180
to
+181
| EventHandler.off(minus, "click"); | ||
| EventHandler.off(plus, "click"); |
There was a problem hiding this comment.
The dispose function doesn't check if minus and plus elements exist before calling EventHandler.off on them. If the toolbar isn't shown (Options.ShowToolbar = false), these elements won't exist and this could cause errors. Add null checks:
if (minus) EventHandler.off(minus, "click");
if (plus) EventHandler.off(plus, "click");
Suggested change
| EventHandler.off(minus, "click"); | |
| EventHandler.off(plus, "click"); | |
| if (minus) EventHandler.off(minus, "click"); | |
| if (plus) EventHandler.off(plus, "click"); |
Comment on lines
54
to
65
| export function fitToWidth(id) { | ||
| const pdfViewer = Data.get(id); | ||
| const { pdfViewer } = Data.get(id); | ||
| if (pdfViewer) { | ||
| pdfViewer.currentScaleValue = 1.0; | ||
| pdfViewer.currentScaleValue = "page-height"; | ||
| } | ||
| } | ||
|
|
||
| export function fitToPage(id) { | ||
| const pdfViewer = Data.get(id); | ||
| const { pdfViewer } = Data.get(id); | ||
| if (pdfViewer) { | ||
| pdfViewer.currentScaleValue = "page-width"; | ||
| } |
There was a problem hiding this comment.
The scale values appear to be swapped between fitToWidth and fitToPage. Based on the UI (PdfReader.razor lines 20-21), fitToPage uses a vertically-oriented icon (fa-rotate-90) suggesting it should fit to page height, but it's setting "page-width" (line 64). Similarly, fitToWidth uses a horizontally-oriented icon and should fit to width, but it's setting "page-height" (line 57). These should be swapped: fitToWidth should use "page-width" and fitToPage should use "page-height".
| const scale = pdfViewer.currentScale; | ||
| const current = Math.round(parseFloat(scale * 100), 0); | ||
| const step = [25, 33, 50, 67, 75, 80, 90, 100, 110, 125, 150, 175, 200, 250, 300, 400, 500]; | ||
| const findValues = step.filter(s => rate > 0 ? current < s : current > s); |
There was a problem hiding this comment.
Extra space in the filter expression before the < comparison operator.
Suggested change
| const findValues = step.filter(s => rate > 0 ? current < s : current > s); | |
| const findValues = step.filter(s => rate > 0 ? current < s : current > s); |
Comment on lines
+42
to
43
| private readonly HashSet<string> AllowedScaleValues = ["page-actual", "page-width", "page-height", "page-fit", "auto"]; | ||
|
|
There was a problem hiding this comment.
The AllowedScaleValues field is defined but never used. If this is intended for future validation of scale values (to allow special values like "page-actual", "page-width", etc.), it should be integrated into the SetCurrentScale method. Otherwise, it should be removed to avoid confusion.
Suggested change
| private readonly HashSet<string> AllowedScaleValues = ["page-actual", "page-width", "page-height", "page-fit", "auto"]; |
| public uint CurrentPage { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// 获得/设置 当前缩放倍率 默认 null 使用 100% |
There was a problem hiding this comment.
The documentation comment describes this as "当前缩放倍率" (current zoom multiplier/scale factor), but the actual implementation treats it as a percentage value stored as a string (e.g., "100" for 100%). The documentation should clarify that this is a string representation of the zoom percentage (e.g., "100" for 100%, "150" for 150%), not a multiplier like "1.0" or "1.5".
Suggested change
| /// 获得/设置 当前缩放倍率 默认 null 使用 100% | |
| /// 获得/设置 当前缩放百分比（字符串形式，如 "100" 表示 100%，"150" 表示 150%），默认 null 使用 100% |
| } | ||
|
|
||
| .bb-view-icon.disabled { | ||
| color: #6c757d; |
There was a problem hiding this comment.
The .bb-view-icon.disabled class only changes the color but still maintains cursor: pointer from the parent .bb-view-icon rule (line 31). When an icon is disabled, the cursor should be changed to cursor: not-allowed or cursor: default to provide better visual feedback to users.
Suggested change
| color: #6c757d; | |
| color: #6c757d; | |
| cursor: not-allowed; |
| } | ||
| else { | ||
| v = findValues.pop(); | ||
| } |
There was a problem hiding this comment.
The updateScale function doesn't handle edge cases where findValues is empty. If the current scale is beyond the min/max boundaries of the step array, shift() or pop() will return undefined, causing pdfViewer.currentScaleValue to be set to NaN. This could happen if the scale is manually set to a value outside the 25-500 range or if there's a race condition. Add a check: if (!v) return; after line 169, or ensure v defaults to the current scale value if undefined.
Suggested change
| } | |
| } | |
| if (typeof v === "undefined") { | |
| v = current; | |
| } |
| const current = Math.round(parseFloat(scale * 100), 0); | ||
| const step = [25, 33, 50, 67, 75, 80, 90, 100, 110, 125, 150, 175, 200, 250, 300, 400, 500]; | ||
| const findValues = step.filter(s => rate > 0 ? current < s : current > s); | ||
| let v = 100; |
There was a problem hiding this comment.
The initial value of v is unused, since it is always overwritten.
Suggested change
| let v = 100; | |
| let v; |
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 #718
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add configurable zoom support and modernize the PdfReader viewer interactions.
New Features:
Enhancements: