feat(PdfReader): add OnPageChangedAsync parameter#715
Merged
Conversation
Reviewer's GuideRefactors PdfReader to use a new PdfReaderOptions configuration object, adds async callbacks for initialization and page changes, and wires the Blazor component, JS interop, and styling to support current-page binding, optional toolbar, and event-based updates from pdf.js. Sequence diagram for PdfReader initialization with options and OnInitAsyncsequenceDiagram
actor User
participant AppComponent
participant PdfReader
participant DotNetRuntime
participant PdfReaderJs
participant PdfJsViewer
User ->> AppComponent: NavigateToPageWithPdf()
AppComponent ->> PdfReader: Render with Options(Url, IsFitToPage, OnInitAsync, OnPageChangedAsync)
activate PdfReader
PdfReader ->> PdfReader: OnParametersSet()
PdfReader ->> PdfReader: Options ??= new PdfReaderOptions()
PdfReader ->> PdfReader: Ensure Options.CurrentPage >= 1
PdfReader ->> PdfReader: _docTitle = Path.GetFileName(Options.Url)
deactivate PdfReader
loop FirstRender
PdfReader ->> PdfReader: OnAfterRenderAsync(firstRender = true)
activate PdfReader
PdfReader ->> PdfReader: _isFitToPage = Options.IsFitToPage
PdfReader ->> PdfReader: _currentPage = Options.CurrentPage
PdfReader ->> PdfReader: _url = Options.Url
PdfReader ->> PdfReader: InvokeInitAsync()
PdfReader ->> DotNetRuntime: InvokeVoidAsync init(Id, Interop, { Url, IsFitToPage, TriggerPagesInit, TriggerPageChanged })
deactivate PdfReader
DotNetRuntime ->> PdfReaderJs: init(id, invoke, options)
activate PdfReaderJs
PdfReaderJs ->> PdfReaderJs: addLink css pdf_viewer.css
PdfReaderJs ->> PdfReaderJs: Check options.url not null
PdfReaderJs ->> PdfJsViewer: pdfjsLib.getDocument(options)
PdfJsViewer -->> PdfReaderJs: loadingTask.promise resolved
PdfReaderJs ->> PdfJsViewer: setDocument(pdfDocument)
PdfReaderJs ->> PdfReaderJs: addEventListener(pdfViewer, eventBus, invoke, options)
PdfJsViewer ->> PdfReaderJs: pagesinit event
PdfReaderJs ->> PdfReaderJs: Set scale based on options.isFitToPage
PdfReaderJs ->> PdfReaderJs: Update toolbar DOM (pagesCount, remove init)
alt options.triggerPagesInit is true
PdfReaderJs ->> DotNetRuntime: invokeMethodAsync pagesInit(numPages)
DotNetRuntime ->> PdfReader: PagesInit(int pagesCount)
activate PdfReader
PdfReader ->> PdfReader: if Options.OnInitAsync != null
PdfReader ->> AppComponent: await Options.OnInitAsync(pagesCount)
deactivate PdfReader
end
deactivate PdfReaderJs
end
Sequence diagram for page change flow with OnPageChangedAsync and CurrentPage bindingsequenceDiagram
actor User
participant AppComponent
participant PdfReader
participant DotNetRuntime
participant PdfReaderJs
participant PdfJsViewer
rect rgb(230,230,250)
note over User,PdfJsViewer: User driven page change in viewer
User ->> PdfJsViewer: Scroll or navigate to another page
PdfJsViewer ->> PdfReaderJs: pagechanging event
activate PdfReaderJs
PdfReaderJs ->> PdfReaderJs: Update bb-view-num input value
alt options.triggerPageChanged is true
PdfReaderJs ->> DotNetRuntime: invokeMethodAsync pageChanged(page)
DotNetRuntime ->> PdfReader: PageChanged(uint pageIndex)
activate PdfReader
PdfReader ->> PdfReader: _currentPage = pageIndex
PdfReader ->> PdfReader: Options.CurrentPage = pageIndex
alt Options.OnPageChangedAsync != null
PdfReader ->> AppComponent: await Options.OnPageChangedAsync(pageIndex)
end
deactivate PdfReader
end
deactivate PdfReaderJs
end
rect rgb(220,245,220)
note over AppComponent,PdfReaderJs: App component changes page programmatically
AppComponent ->> PdfReader: NavigateToPageAsync(newPage)
activate PdfReader
PdfReader ->> DotNetRuntime: InvokeVoidAsync navigateToPage(Id, newPage)
deactivate PdfReader
DotNetRuntime ->> PdfReaderJs: navigateToPage(id, pageNumber)
activate PdfReaderJs
PdfReaderJs ->> PdfJsViewer: Navigate to specified page
deactivate PdfReaderJs
end
Class diagram for updated PdfReader and new PdfReaderOptionsclassDiagram
class PdfReader {
+PdfReaderOptions Options
-string ClassString
-string StyleString
-string ViewBodyString
-string _docTitle
-bool _isFitToPage
-uint _currentPage
-string _url
+string CurrentPageString
+OnParametersSet()
+OnAfterRenderAsync(bool firstRender)
+InvokeInitAsync() Task
+NavigateToPageAsync(uint pageNumber) Task
+FitToPage() void
+FitToWidth() void
+RotateLeft() Task
+RotateRight() Task
+PagesInit(int pagesCount) Task
+PageChanged(uint pageIndex) Task
}
class PdfReaderOptions {
+bool ShowToolbar
+string Url
+string ViewHeight
+uint CurrentPage
+bool IsFitToPage
+Func~int,Task~ OnInitAsync
+Func~uint,Task~ OnPageChangedAsync
}
PdfReader --> PdfReaderOptions : 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 and found some issues that need to be addressed.
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
countEl.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- The JS interop callbacks appear to be mismatched: JavaScript calls
invoke.invokeMethodAsync("pagesInit", ...)andinvoke.invokeMethodAsync("pageChanged", ...)but the C# methods are[JSInvokable] public async Task PagesInit(...)andPageChanged(...), so either the JS names or the[JSInvokable]identifiers should be aligned to avoid runtime failures. - The
Optionsparameter is treated as non-null in members likeStyleString,ViewBodyString, andCurrentPageStringbut is only defaulted inOnParametersSet, so consider making it non-nullable with a default instance or guarding all usages to avoid potential null-reference issues during initial render or misuse by consumers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The JS interop callbacks appear to be mismatched: JavaScript calls `invoke.invokeMethodAsync("pagesInit", ...)` and `invoke.invokeMethodAsync("pageChanged", ...)` but the C# methods are `[JSInvokable] public async Task PagesInit(...)` and `PageChanged(...)`, so either the JS names or the `[JSInvokable]` identifiers should be aligned to avoid runtime failures.
- The `Options` parameter is treated as non-null in members like `StyleString`, `ViewBodyString`, and `CurrentPageString` but is only defaulted in `OnParametersSet`, so consider making it non-nullable with a default instance or guarding all usages to avoid potential null-reference issues during initial render or misuse by consumers.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:81-90` </location>
<code_context>
+ eventBus.on("pagesinit", async () => {
</code_context>
<issue_to_address>
**issue (bug_risk):** JSInvokable method names in JS don’t match the C# method names, which will break interop calls.
The JS currently calls `invoke.invokeMethodAsync(
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:122` </location>
<code_context>
/// <param name="pageNumber"></param>
/// <returns></returns>
- public Task NavigateToPageAsync(int pageNumber) => InvokeVoidAsync("navigateToPage", Id, pageNumber);
+ public Task NavigateToPageAsync(uint pageNumber) => InvokeVoidAsync("navigateToPage", Id, pageNumber);
/// <summary>
</code_context>
<issue_to_address>
**suggestion (bug_risk):** CurrentPage/initial page support is only applied after changes, so the initial page value on first load is effectively ignored.
Because `_currentPage` is set from `Options.CurrentPage` only on first render and no navigation is triggered then, the viewer never actually jumps to a non-default initial page. To honor an initial `CurrentPage`, either pass it to JS and set `pdfViewer.currentPageNumber` in `pagesinit`, or, once initialization completes, call `NavigateToPageAsync(Options.CurrentPage)` when `Options.CurrentPage > 1`.
Suggested implementation:
```csharp
TriggerPagesInit = Options.OnInitAsync != null,
TriggerPageChanged = Options.OnPageChangedAsync != null
});
// Honor an initial non-default CurrentPage by navigating after initialization completes
if (Options is not null && Options.CurrentPage > 1)
{
await NavigateToPageAsync((uint)Options.CurrentPage);
}
```
I assumed this block is inside an async initialization method (likely OnAfterRenderAsync or a similar lifecycle/initialization method) where:
- "InvokeVoidAsync" initializes the JS PDF viewer, and
- "Options.CurrentPage" is an int or similar numeric type.
You may need to:
1) Ensure the enclosing method is async and can await the new NavigateToPageAsync call.
2) Adjust the cast to uint if Options.CurrentPage is already an unsigned type.
3) If the initialization can be called multiple times, consider guarding this navigation with a "firstRender" or similar flag so the initial navigation only happens once.
4) If you prefer the "pass to JS and handle in pagesinit" approach instead, add "InitialPage = Options.CurrentPage" to the object passed to JS here and then update the JS side to set "pdfViewer.currentPageNumber" in the "pagesinit" handler using that value.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.PdfReader/PdfReaderOptions.cs:38-45` </location>
<code_context>
+ public bool IsFitToPage { get; set; }
+
+ /// <summary>
+ /// 页面初始化回调方法
+ /// </summary>
+ public Func<int, Task>? OnInitAsync { get; set; }
+
+ /// <summary>
+ /// 页面初始化回调方法
+ /// </summary>
+ public Func<uint, Task>? OnPageChangedAsync { get; set; }
+}
</code_context>
<issue_to_address>
**nitpick:** The summary for OnPageChangedAsync doesn’t reflect that it is a page-changed callback.
The XML comment for `OnPageChangedAsync` is copied from `OnInitAsync` but this delegate is called on page changes, not initialization. Please update the description (e.g., to “页面变更回调方法”) so its behavior is accurately documented for consumers.
</issue_to_address>
### Comment 4
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:93` </location>
<code_context>
countEl.innerHTML = numPages;
</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 5
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:93` </location>
<code_context>
countEl.innerHTML = numPages;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `countEl.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.
| /// <param name="pageNumber"></param> | ||
| /// <returns></returns> | ||
| public Task NavigateToPageAsync(int pageNumber) => InvokeVoidAsync("navigateToPage", Id, pageNumber); | ||
| public Task NavigateToPageAsync(uint pageNumber) => InvokeVoidAsync("navigateToPage", Id, pageNumber); |
There was a problem hiding this comment.
suggestion (bug_risk): CurrentPage/initial page support is only applied after changes, so the initial page value on first load is effectively ignored.
Because _currentPage is set from Options.CurrentPage only on first render and no navigation is triggered then, the viewer never actually jumps to a non-default initial page. To honor an initial CurrentPage, either pass it to JS and set pdfViewer.currentPageNumber in pagesinit, or, once initialization completes, call NavigateToPageAsync(Options.CurrentPage) when Options.CurrentPage > 1.
Suggested implementation:
TriggerPagesInit = Options.OnInitAsync != null,
TriggerPageChanged = Options.OnPageChangedAsync != null
});
// Honor an initial non-default CurrentPage by navigating after initialization completes
if (Options is not null && Options.CurrentPage > 1)
{
await NavigateToPageAsync((uint)Options.CurrentPage);
}I assumed this block is inside an async initialization method (likely OnAfterRenderAsync or a similar lifecycle/initialization method) where:
- "InvokeVoidAsync" initializes the JS PDF viewer, and
- "Options.CurrentPage" is an int or similar numeric type.
You may need to:
- Ensure the enclosing method is async and can await the new NavigateToPageAsync call.
- Adjust the cast to uint if Options.CurrentPage is already an unsigned type.
- If the initialization can be called multiple times, consider guarding this navigation with a "firstRender" or similar flag so the initial navigation only happens once.
- If you prefer the "pass to JS and handle in pagesinit" approach instead, add "InitialPage = Options.CurrentPage" to the object passed to JS here and then update the JS side to set "pdfViewer.currentPageNumber" in the "pagesinit" handler using that value.
| const numPages = pdfViewer.pagesCount; | ||
| const countEl = el.querySelector(".bb-view-pagesCount"); | ||
| if (countEl) { | ||
| countEl.innerHTML = numPages; |
There was a problem hiding this comment.
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
| const numPages = pdfViewer.pagesCount; | ||
| const countEl = el.querySelector(".bb-view-pagesCount"); | ||
| if (countEl) { | ||
| countEl.innerHTML = numPages; |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a countEl.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
There was a problem hiding this comment.
Pull request overview
This PR adds the OnPageChangedAsync callback parameter to the PdfReader component and refactors the component's parameter structure by introducing a PdfReaderOptions class. The refactoring consolidates individual parameters (Url, ViewHeight, IsFitToPage) into a single options object for better maintainability and adds support for both initialization and page change callbacks.
Key Changes:
- Introduced
PdfReaderOptionsclass to encapsulate all PdfReader configuration properties - Added
OnPageChangedAsyncandOnInitAsynccallback parameters to enable component event handling - Refactored component to use options pattern instead of individual parameters
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| PdfReaderOptions.cs | New options class containing all PdfReader configuration properties including the new OnPageChangedAsync callback |
| PdfReader.razor.cs | Refactored to use PdfReaderOptions parameter; added callback method implementations (PagesInit, PageChanged) and state management for URL, page, and fit mode changes |
| PdfReader.razor.js | Moved CSS import earlier in initialization; extracted event listener setup into separate function; added callbacks for page initialization and page changes |
| PdfReader.razor | Updated template to conditionally render toolbar and bind current page input to new CurrentPageString property |
| PdfReader.razor.css | Removed CSS import (moved to JavaScript); added styling for toolbar initialization state |
Comments suppressed due to low confidence (1)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:143
- Unused class Pdf.
class Pdf {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+98
to
102
| if (_currentPage != Options.CurrentPage) | ||
| { | ||
| _currentPage = Options.CurrentPage; | ||
| await NavigateToPageAsync(_currentPage); | ||
| } |
There was a problem hiding this comment.
There's a potential race condition here. If NavigateToPageAsync is called while the JavaScript pageChanged callback is executing, the page state could become inconsistent. Consider adding a flag to prevent updating _currentPage and Options.CurrentPage in the PageChanged method when the navigation was triggered from C# code rather than user interaction.
Comment on lines
+47
to
+53
| private void SetCurrentPage(string value) | ||
| { | ||
| if (uint.TryParse(value, NumberStyles.Integer, CultureInfo.InvariantCulture, out var num)) | ||
| { | ||
| Options.CurrentPage = num; | ||
| } | ||
| } |
There was a problem hiding this comment.
The SetCurrentPage method silently ignores invalid input without providing feedback. Consider adding validation error handling or at least logging when the parse fails, so developers and users understand why page navigation might not work as expected.
| public Func<int, Task>? OnInitAsync { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// 页面初始化回调方法 |
There was a problem hiding this comment.
The documentation comment is incorrect. This describes "页面初始化回调方法" (page initialization callback method), but it should describe "页面切换回调方法" (page changed callback method) or similar, to differentiate it from the OnInitAsync property above.
Suggested change
| /// 页面初始化回调方法 | |
| /// 页面切换回调方法 |
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 #714
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Introduce a configurable options model for the PdfReader component and enhance its page navigation and toolbar behavior.
New Features:
Enhancements: