Conversation
Reviewer's GuideAdds a configurable EnableThumbnails option to PdfReader that controls whether the thumbnails sidebar is available and ensures the JS viewer correctly initializes, toggles, and resets thumbnail state when the option changes. Sequence diagram for updating thumbnails when EnableThumbnails changessequenceDiagram
actor User
participant ParentComponent
participant PdfReader
participant JsRuntime
participant PdfReaderRazorJs as PdfReader_razor_js
User ->> ParentComponent: Change thumbnail setting in UI
ParentComponent ->> PdfReader: Render with EnableThumbnails = value
PdfReader ->> PdfReader: OnAfterRenderAsync(firstRender = false)
PdfReader ->> PdfReader: Detect _enableThumbnails != EnableThumbnails
PdfReader ->> PdfReader: _enableThumbnails = EnableThumbnails
alt EnableThumbnails is true
PdfReader ->> JsRuntime: InvokeVoidAsync(resetThumbnails, Id)
JsRuntime ->> PdfReaderRazorJs: resetThumbnails(Id)
PdfReaderRazorJs ->> PdfReaderRazorJs: Data.get(Id)
PdfReaderRazorJs ->> PdfReaderRazorJs: resetThumbnailsView(el, pdfViewer)
PdfReaderRazorJs ->> User: Thumbnails sidebar reset and shown correctly
else EnableThumbnails is false
PdfReader ->> User: Thumbnails disabled, no JS reset
end
Class diagram for updated PdfReader thumbnail configurationclassDiagram
class PdfReader {
string~?~ _url
string~?~ _currentScale
string~?~ _dropdownItemCheckIcon
string~?~ _dropdownItemDefaultIcon
bool _enableThumbnails
bool EnableThumbnails
Task OnAfterRenderAsync(bool firstRender)
ValueTask InvokeVoidAsync(string identifier, string id)
}
class PdfReaderRazorJs {
void init(string id, bool enableThumbnails)
void scale(string id, string scale)
void resetThumbnails(string id)
void dispose(string id)
void addEventListener(Element el, object pdfViewer, object eventBus, Delegate invoke, PdfReaderOptions options)
}
class PdfReaderOptions {
bool fitMode
bool enableThumbnails
}
PdfReader --> PdfReaderRazorJs : uses_JS_interop
PdfReader --> PdfReaderOptions : passes
PdfReaderOptions ..> PdfReaderRazorJs : configures_behavior
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:
- In
addEventListener, theif (options.enableThumbnails || false)check is effectively the same asif (options.enableThumbnails)and adds noise; consider dropping the|| falseor normalizingoptions.enableThumbnailsearlier if you need a strict boolean. - When
EnableThumbnailsis toggled from true to false, the JS side doesn’t proactively hide or reset the thumbnail sidebar (only the toolbar button disappears); consider also collapsing or resetting the thumbnails container in this path to avoid leaving an open, now-unsupported sidebar visible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `addEventListener`, the `if (options.enableThumbnails || false)` check is effectively the same as `if (options.enableThumbnails)` and adds noise; consider dropping the `|| false` or normalizing `options.enableThumbnails` earlier if you need a strict boolean.
- When `EnableThumbnails` is toggled from true to false, the JS side doesn’t proactively hide or reset the thumbnail sidebar (only the toolbar button disappears); consider also collapsing or resetting the thumbnails container in this path to avoid leaving an open, now-unsupported sidebar visible.
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.PdfReader/PdfReader.razor.cs:246-251` </location>
<code_context>
_currentScale = CurrentScale;
await InvokeVoidAsync("scale", Id, _currentScale);
}
+ if (_enableThumbnails != EnableThumbnails)
+ {
+ _enableThumbnails = EnableThumbnails;
+ if (_enableThumbnails)
+ {
+ await InvokeVoidAsync("resetThumbnails", Id);
+ }
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** When thumbnails are disabled, the UI state of the sidebar may remain visible and inconsistent.
This logic only reacts when `EnableThumbnails` becomes `true`. When it changes from `true` to `false`, no JS runs, so an open thumbnails sidebar (with `show` class) can remain visible even though the icon is gone. Please also hide/reset the thumbnails panel on disable (e.g., via JS interop that removes `show`) to keep the UI consistent with `EnableThumbnails`.
Suggested implementation:
```csharp
if (_enableThumbnails != EnableThumbnails)
{
_enableThumbnails = EnableThumbnails;
if (_enableThumbnails)
{
// Thumbnails enabled: ensure sidebar is in a clean, visible state.
await InvokeVoidAsync("resetThumbnails", Id);
}
else
{
// Thumbnails disabled: ensure any open sidebar is hidden to keep UI consistent.
await InvokeVoidAsync("hideThumbnails", Id);
}
}
```
To fully implement this behavior, you will also need to:
1. Add a `hideThumbnails` JavaScript function in the corresponding JS file used by `PdfReader` (where `resetThumbnails` is defined). That function should:
- Locate the thumbnails sidebar element.
- Remove any `show` (or equivalent) class.
- Ensure it is visually hidden/closed.
2. Verify that `hideThumbnails` is correctly exported/available under the same JS interop namespace used by `resetThumbnails`, so `InvokeVoidAsync("hideThumbnails", Id)` successfully resolves.
</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 support for dynamically enabling/disabling PDF thumbnails in the PdfReader component by making the EnableThumbnails parameter reactive to runtime changes.
Key Changes:
- Added runtime detection and handling of
EnableThumbnailsparameter changes in C# code - Introduced a new
resetThumbnailsJavaScript function to re-initialize thumbnails when enabled - Modified event handlers to use event delegation pattern for better flexibility
- Made thumbnail sidebar toggle button conditionally render based on
EnableThumbnailsstate
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| PdfReader.razor.cs | Added _enableThumbnails field and logic in OnAfterRenderAsync to detect parameter changes and call resetThumbnails when thumbnails are enabled |
| PdfReader.razor.js | Added resetThumbnails export function, changed event handler from direct attachment to event delegation pattern, and added redundant ` |
| PdfReader.razor | Made the thumbnail toggle button (.bb-view-bar) conditionally render only when EnableThumbnails is true |
Comments suppressed due to low confidence (1)
src/components/BootstrapBlazor.PdfReader/PdfReader.razor.js:153
- Potential null reference error. When
options.enableThumbnailsis truthy andthumbnailsContainerexists but has no element with the.activeclass, line 153 will throw a "Cannot read property 'classList' of null" error becauseactiveis not checked for null before callingclassList.remove(). This should be guarded with a null check:if (active) { active.classList.remove('active'); }.
const active = thumbnailsContainer.querySelector('.active');
active.classList.remove('active');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if (options.enableThumbnails) { | ||
| if (options.enableThumbnails || false) { |
There was a problem hiding this comment.
The expression options.enableThumbnails || false is redundant. Since options.enableThumbnails is already a boolean, the || false doesn't change the behavior. If enableThumbnails is true, it returns true; if it's false, the expression evaluates to false. This should be simplified to just options.enableThumbnails.
Suggested change
| if (options.enableThumbnails || false) { | |
| if (options.enableThumbnails) { |
| if (titleEl) { | ||
| EventHandler.on(titleEl, "click", '.bb-view-bar', e => { | ||
| const thumbnailsEl = el.querySelector(".bb-view-thumbnails"); | ||
| thumbnailsEl.classList.toggle("show"); |
There was a problem hiding this comment.
Missing null check for thumbnailsEl. The code queries for .bb-view-thumbnails but doesn't verify it exists before calling classList.toggle(). While the event delegation pattern ensures this handler only fires when .bb-view-bar is clicked, there's a timing issue: during dynamic state changes (e.g., when EnableThumbnails transitions from true to false), the .bb-view-bar element might still exist briefly while .bb-view-thumbnails is already removed, leading to a null reference error. Add a null check: if (thumbnailsEl) { thumbnailsEl.classList.toggle("show"); }.
Suggested change
| thumbnailsEl.classList.toggle("show"); | |
| if (thumbnailsEl) { | |
| thumbnailsEl.classList.toggle("show"); | |
| } |
|
|
||
| export function resetThumbnails(id) { | ||
| const { el, pdfViewer } = Data.get(id); | ||
| if (pdfViewer) { |
There was a problem hiding this comment.
Missing null check for el parameter. The function retrieves el from Data.get(id) but only checks if pdfViewer exists before calling resetThumbnailsView(el, pdfViewer). If el is null or undefined, it will be passed to resetThumbnailsView, which will fail when trying to call el.querySelector() at line 206. Add a check: if (el && pdfViewer) { ... }.
Suggested change
| if (pdfViewer) { | |
| if (el && pdfViewer) { |
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 #738
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add configurable thumbnail sidebar support to the PdfReader component and adjust its UI interactions accordingly.
New Features:
Enhancements: