Conversation
Reviewer's GuideThis PR refactors the SelectCity component to support dynamic search toggling by modularizing JS initialization/disposal logic, adds C# detection for ShowSearch changes to invoke the new resetSearch bridge, and bumps the package version to 9.0.9. Sequence diagram for dynamic search toggle in SelectCitysequenceDiagram
participant Blazor as Blazor (C#)
participant JS as JavaScript
Blazor->>JS: init(id, invoke, options)
JS->>JS: initSearch(region)
JS->>JS: Data.set(id, region)
Note over Blazor,JS: On ShowSearch change
Blazor->>JS: resetSearch(id, ShowSearch)
alt ShowSearch is true
JS->>JS: initSearch(region)
else ShowSearch is false
JS->>JS: disposeSearch(region)
end
Updated class diagram for SelectCity componentclassDiagram
class SelectCity {
- HashSet<string> _values
- string? _searchText
- bool _showSearch
+ OnParametersSet()
+ OnAfterRenderAsync(bool firstRender)
}
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 they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/components/BootstrapBlazor.Region/Components/SelectCity.razor.cs:100` </location>
<code_context>
+ _showSearch = ShowSearch;
+ }
+
+ if (!_showSearch != ShowSearch)
+ {
+ _showSearch = ShowSearch;
</code_context>
<issue_to_address>
**issue:** The logic in the conditional is confusing and may not behave as intended.
The condition uses a double negative, which is unclear and may cause logic errors. Simplifying it to `if (_showSearch != ShowSearch)` will improve readability and correctness.
</issue_to_address>
### Comment 2
<location> `src/components/BootstrapBlazor.Region/Components/SelectCity.razor.js:26` </location>
<code_context>
+}
+
+export function resetSearch(id, search) {
+ const region = Data.get(id);
+
+ if (search) {
</code_context>
<issue_to_address>
**issue:** No null check for region object retrieved from Data.
Add a check to handle cases where Data.get(id) returns null or undefined to prevent runtime errors.
</issue_to_address>
### Comment 3
<location> `src/components/BootstrapBlazor.Region/Components/SelectCity.razor.js:20` </location>
<code_context>
}
});
+ const region = { el, invoke, options, popover };
+ initSearch(region);
+ Data.set(id, region);
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring to use closures for search logic and only store necessary functions on the region object.
Here’s a way to collapse the extra indirection into two small closures and only store what you really need on your `region` object—no more raw `el`/`invoke`/`options` dancing around, just the popover and two self-contained functions:
```js
export function init(id, invoke, options) {
const el = document.getElementById(id);
if (!el) return;
const popover = Popover.init(el, {
shownCallback: () => searchInput?.focus()
});
const searchInput = el.querySelector('.search-text');
const clearIcon = el.querySelector('.dropdown-menu-search .clear-icon');
// capture searchInput/clearIcon/invoke/options in closure
const initSearch = () => {
if (searchInput) {
Input.composition(searchInput, v =>
invoke.invokeMethodAsync(options.triggerSearch, v)
);
}
if (clearIcon) {
EventHandler.on(clearIcon, 'click', () => {
searchInput.value = '';
invoke.invokeMethodAsync(options.triggerSearch, '');
});
}
};
const disposeSearch = () => {
if (searchInput) Input.dispose(searchInput);
if (clearIcon) EventHandler.off(clearIcon, 'click');
};
initSearch();
Data.set(id, { popover, initSearch, disposeSearch });
}
export function resetSearch(id, enable) {
const region = Data.get(id);
region.disposeSearch();
if (enable) region.initSearch();
}
export function hide(id) {
const { popover } = Data.get(id) || {};
popover?.hide();
}
export function dispose(id) {
const region = Data.get(id);
Data.remove(id);
if (!region) return;
Popover.dispose(region.popover);
region.disposeSearch();
}
```
Benefits:
- No shared mutable `region` fields for `el`/`invoke`/`options`.
- `initSearch` / `disposeSearch` are closures, so you only store two functions.
- `resetSearch` stays simple (toggle via those closures).
- All functionality is identical; flow is now linear and easier to follow.
</issue_to_address>
### Comment 4
<location> `src/components/BootstrapBlazor.Region/Components/SelectCity.razor.cs:91` </location>
<code_context>
+ /// </summary>
+ /// <param name="firstRender"></param>
+ /// <returns></returns>
+ protected override async Task OnAfterRenderAsync(bool firstRender)
+ {
+ await base.OnAfterRenderAsync(firstRender);
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the _showSearch field and handling ShowSearch changes in OnParametersSetAsync to simplify state management and JS invocation.
```csharp
// Remove the _showSearch field and the OnAfterRenderAsync override entirely.
// Instead, detect ShowSearch changes in OnParametersSetAsync and invoke JS after the render.
private bool _previousShowSearch;
protected override async Task OnParametersSetAsync()
{
await base.OnParametersSetAsync();
// Reset search text when ShowSearch is turned off
if (ShowSearch == false)
{
_searchText = "";
}
// Only call resetSearch JS when ShowSearch actually changes
if (_previousShowSearch != ShowSearch)
{
_previousShowSearch = ShowSearch;
// This will run after the component has rendered the new state
await InvokeVoidAsync("resetSearch", Id, ShowSearch);
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| _showSearch = ShowSearch; | ||
| } | ||
|
|
||
| if (!_showSearch != ShowSearch) |
There was a problem hiding this comment.
issue: The logic in the conditional is confusing and may not behave as intended.
The condition uses a double negative, which is unclear and may cause logic errors. Simplifying it to if (_showSearch != ShowSearch) will improve readability and correctness.
| /// </summary> | ||
| /// <param name="firstRender"></param> | ||
| /// <returns></returns> | ||
| protected override async Task OnAfterRenderAsync(bool firstRender) |
There was a problem hiding this comment.
issue (complexity): Consider removing the _showSearch field and handling ShowSearch changes in OnParametersSetAsync to simplify state management and JS invocation.
// Remove the _showSearch field and the OnAfterRenderAsync override entirely.
// Instead, detect ShowSearch changes in OnParametersSetAsync and invoke JS after the render.
private bool _previousShowSearch;
protected override async Task OnParametersSetAsync()
{
await base.OnParametersSetAsync();
// Reset search text when ShowSearch is turned off
if (ShowSearch == false)
{
_searchText = "";
}
// Only call resetSearch JS when ShowSearch actually changes
if (_previousShowSearch != ShowSearch)
{
_previousShowSearch = ShowSearch;
// This will run after the component has rendered the new state
await InvokeVoidAsync("resetSearch", Id, ShowSearch);
}
}There was a problem hiding this comment.
Pull Request Overview
This PR bumps the version of the SelectCity component from 9.0.8 to 9.0.9 (and 10.0.0-rc.2.1.0 to 10.0.0-rc.2.1.1) and implements dynamic handling of the ShowSearch parameter, allowing search functionality to be toggled at runtime.
Key Changes:
- Refactored JavaScript code to support dynamic enabling/disabling of search functionality
- Added new
resetSearchJavaScript function to handle search state changes - Implemented
OnAfterRenderAsyncin C# to detect and respond to ShowSearch parameter changes
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| SelectCity.razor.js | Refactored search initialization into reusable helper functions (initSearch, disposeSearch) and added resetSearch export to handle dynamic search toggling |
| SelectCity.razor.cs | Added _showSearch field and OnAfterRenderAsync method to track and respond to ShowSearch parameter changes; clears search text when ShowSearch is disabled |
| BootstrapBlazor.Region.csproj | Version bumped from 9.0.8 to 9.0.9 for VS 17.0 and from 10.0.0-rc.2.1.0 to 10.0.0-rc.2.1.1 for VS 18.0 |
Comments suppressed due to low confidence (1)
src/components/BootstrapBlazor.Region/Components/SelectCity.razor.cs:216
- Write to static field from instance method, property, or constructor.
_provinceItems ??= [.. Provinces.Select(i => new ProvinceItem()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _showSearch = ShowSearch; | ||
| } | ||
|
|
||
| if (!_showSearch != ShowSearch) |
There was a problem hiding this comment.
The boolean logic in this condition is incorrect. The expression !_showSearch != ShowSearch will be evaluated as (!_showSearch) != ShowSearch, which is likely not the intended behavior.
This should be changed to _showSearch != ShowSearch to properly detect when the ShowSearch property has changed.
| if (!_showSearch != ShowSearch) | |
| if (_showSearch != ShowSearch) |
|
|
||
| const popover = Popover.init(el, { | ||
| shownCallback: () => { | ||
| if (searchInput != null) { |
There was a problem hiding this comment.
The variable searchInput is referenced in the callback but is not defined in scope at this point. The searchInput variable is only created later in the initSearch function (line 38), which is called after this popover is initialized.
This will cause a runtime error when the popover is shown. Consider moving the popover initialization to occur after the search input is initialized, or capture the searchInput reference in a way that allows the callback to access it when executed.
| const el = document.getElementById(id) | ||
| const el = document.getElementById(id); | ||
| if (el === null) { | ||
| return |
There was a problem hiding this comment.
Missing semicolon after the return statement. While JavaScript has automatic semicolon insertion, the codebase appears to use explicit semicolons consistently (see line 7, 22, etc.), so this should have a semicolon for consistency.
| return | |
| return; |
| } | ||
| if (search) { | ||
| EventHandler.off(search, 'click'); | ||
| } |
There was a problem hiding this comment.
Missing semicolon after the statement. While JavaScript has automatic semicolon insertion, the codebase appears to use explicit semicolons consistently (see lines 53-54), so this should have a semicolon for consistency.
| } | ||
|
|
||
| Data.set(id, { el, popover, searchInput, search }); | ||
| region.searchInput = searchInput; |
There was a problem hiding this comment.
Missing semicolon after the statement. While JavaScript has automatic semicolon insertion, the codebase appears to use explicit semicolons consistently (see line 54, 60), so this should have a semicolon for consistency.
|
|
||
| Data.set(id, { el, popover, searchInput, search }); | ||
| region.searchInput = searchInput; | ||
| region.search = search; |
There was a problem hiding this comment.
Missing semicolon after the statement. While JavaScript has automatic semicolon insertion, the codebase appears to use explicit semicolons consistently (see lines 53-54, 60), so this should have a semicolon for consistency.
| @@ -46,14 +76,10 @@ export function dispose(id) { | |||
| const region = Data.get(id) | |||
There was a problem hiding this comment.
Missing semicolon after the statement. While JavaScript has automatic semicolon insertion, the codebase appears to use explicit semicolons consistently (see line 77, 81), so this should have a semicolon for consistency.
| const region = Data.get(id) | |
| const region = Data.get(id); |
| const { popover, searchInput, search } = region; | ||
| const { popover } = region; | ||
| if (popover) { | ||
| Popover.dispose(popover); |
There was a problem hiding this comment.
Missing semicolon after the statement. While JavaScript has automatic semicolon insertion, the codebase appears to use explicit semicolons consistently (see line 76-77, 84), so this should have a semicolon for consistency.
| EventHandler.off(search, 'click'); | ||
| } | ||
|
|
||
| disposeSearch(region); |
There was a problem hiding this comment.
Missing semicolon after the statement. While JavaScript has automatic semicolon insertion, the codebase appears to use explicit semicolons consistently (see line 76-77, 81), so this should have a semicolon for consistency.
| _values.Clear(); | ||
| } | ||
|
|
||
| if (ShowSearch == false) |
There was a problem hiding this comment.
The expression 'A == false' can be simplified to '!A'.
| if (ShowSearch == false) | |
| if (!ShowSearch) |
Link issues
fixes #670
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Add runtime toggle for SelectCity search input, refactor JS search management, and bump package version to 9.0.9.
New Features:
Bug Fixes:
Enhancements:
Build: