feat(Select): use DynamicElement ensure release event callback#7894
Merged
feat(Select): use DynamicElement ensure release event callback#7894
Conversation
Contributor
Reviewer's GuideRefactors the Select component’s item rendering and input change handling by routing click events through DynamicElement, centralizing the editable input’s change EventCallback, forcing a re-render after item selection, and cleaning up the callback on disposal to avoid stale references. Sequence diagram for editable Select input change handling with centralized EventCallbacksequenceDiagram
actor User
participant Browser
participant SelectComponent
User->>Browser: Type/select value in input
Browser->>SelectComponent: onchange via _onChangeEventCallback
activate SelectComponent
SelectComponent->>SelectComponent: OnChange(args)
SelectComponent->>SelectComponent: Update CurrentValueAsString
SelectComponent->>SelectComponent: Determine SelectedItem from allItems
SelectComponent-->>Browser: Render updated UI (implicit)
deactivate SelectComponent
Sequence diagram for Select item click routed through DynamicElementsequenceDiagram
actor User
participant Browser
participant DynamicElement
participant SelectComponent
participant BlazorRenderer
User->>Browser: Click dropdown item
Browser->>DynamicElement: OnClick event
DynamicElement->>SelectComponent: OnClickItem(item)
activate SelectComponent
SelectComponent->>SelectComponent: Update _defaultVirtualizedItemText
SelectComponent->>SelectComponent: SelectedItemChanged(item)
SelectComponent->>SelectComponent: StateHasChanged()
SelectComponent-->>BlazorRenderer: Request re-render
BlazorRenderer-->>Browser: Patch DOM with new selection
deactivate SelectComponent
Updated class diagram for Select component with DynamicElement and EventCallback managementclassDiagram
class SimpleSelectBase_TValue_ {
}
class Select_TValue_ {
- bool IsEditable
- string _defaultVirtualizedItemText
- EventCallback~ChangeEventArgs~ _onChangeEventCallback
- SelectedItem SelectedItem
- SelectedItem SelectedRow
+ void OnParametersSet()
+ Task OnClickItem(SelectedItem item)
+ Task SelectedItemChanged(SelectedItem item)
+ Task OnChange(ChangeEventArgs args)
+ SelectedItem GetItemByCurrentValue()
+ ValueTask DisposeAsync(bool disposing)
}
Select_TValue_ --|> SimpleSelectBase_TValue_
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/Components/Select/Select.razor.cs" line_range="205" />
<code_context>
private string _defaultVirtualizedItemText = "";
+ private EventCallback<ChangeEventArgs> _onChangeEventCallback = EventCallback<ChangeEventArgs>.Empty;
+
private SelectedItem? SelectedItem { get; set; }
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing the stateful EventCallback field and explicit StateHasChanged call by handling editability and rendering declaratively or within the handler to keep the component logic simpler and more self-contained.
You can simplify this change by removing the stateful `EventCallback` field and its disposal, and by tightening the click handler.
### 1. Remove `_onChangeEventCallback` field and lifecycle wiring
The field and `OnParametersSet` / `DisposeAsync` logic are only used to conditionally wire `OnChange`. You can achieve the same behavior declaratively in the `.razor` file without extra mutable state or lifecycle coupling.
**Instead of:**
```csharp
// .razor.cs
private EventCallback<ChangeEventArgs> _onChangeEventCallback = EventCallback<ChangeEventArgs>.Empty;
protected override void OnParametersSet()
{
base.OnParametersSet();
PlaceHolder ??= Localizer[nameof(PlaceHolder)];
NoSearchDataText ??= Localizer[nameof(NoSearchDataText)];
DropdownIcon ??= IconTheme.GetIconByKey(ComponentIcons.SelectDropdownIcon);
ClearIcon ??= IconTheme.GetIconByKey(ComponentIcons.SelectClearIcon);
_onChangeEventCallback = IsEditable
? EventCallback.Factory.Create<ChangeEventArgs>(this, OnChange)
: EventCallback<ChangeEventArgs>.Empty;
}
protected override ValueTask DisposeAsync(bool disposing)
{
if (disposing)
{
_onChangeEventCallback = EventCallback<ChangeEventArgs>.Empty;
}
return base.DisposeAsync(disposing);
}
```
Bind directly in the `.razor`:
```razor
<input
@onchange="(IsEditable
? EventCallback.Factory.Create<ChangeEventArgs>(this, OnChange)
: EventCallback<ChangeEventArgs>.Empty)" />
```
Then you can delete:
```csharp
private EventCallback<ChangeEventArgs> _onChangeEventCallback = EventCallback<ChangeEventArgs>.Empty;
```
and the `DisposeAsync` override entirely. The component instance is short‑lived; clearing a value-type callback in `DisposeAsync` brings no real benefit.
If you prefer to keep the binding simple, another option is to short‑circuit inside the handler:
```csharp
private Task OnChange(ChangeEventArgs e)
{
if (!IsEditable)
{
return Task.CompletedTask;
}
// existing logic...
}
```
and then use a straightforward binding:
```razor
<input @onchange="OnChange" />
```
Either pattern removes the extra field and lifecycle complexity while preserving behavior.
### 2. Reconsider explicit `StateHasChanged` in `OnClickItem`
`OnClickItem` currently forces a render:
```csharp
private async Task OnClickItem(SelectedItem item)
{
if (!item.IsDisabled)
{
_defaultVirtualizedItemText = item.Text;
await SelectedItemChanged(item);
}
StateHasChanged();
}
```
If `SelectedItemChanged` flows into `CurrentValue` / parameter updates that already trigger a render (which is common in Blazor forms), the explicit `StateHasChanged()` is redundant and can be removed:
```csharp
private async Task OnClickItem(SelectedItem item)
{
if (!item.IsDisabled)
{
_defaultVirtualizedItemText = item.Text;
await SelectedItemChanged(item);
}
}
```
If you’ve hit a specific scenario where a manual refresh is required (e.g., async external state not bound to the component), consider adding a brief comment so future readers understand why it’s needed:
```csharp
private async Task OnClickItem(SelectedItem item)
{
if (!item.IsDisabled)
{
_defaultVirtualizedItemText = item.Text;
await SelectedItemChanged(item);
}
// Explicit refresh required because <reason>, this is not triggered by normal binding.
StateHasChanged();
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| private string _defaultVirtualizedItemText = ""; | ||
|
|
||
| private EventCallback<ChangeEventArgs> _onChangeEventCallback = EventCallback<ChangeEventArgs>.Empty; |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider removing the stateful EventCallback field and explicit StateHasChanged call by handling editability and rendering declaratively or within the handler to keep the component logic simpler and more self-contained.
You can simplify this change by removing the stateful EventCallback field and its disposal, and by tightening the click handler.
1. Remove _onChangeEventCallback field and lifecycle wiring
The field and OnParametersSet / DisposeAsync logic are only used to conditionally wire OnChange. You can achieve the same behavior declaratively in the .razor file without extra mutable state or lifecycle coupling.
Instead of:
// .razor.cs
private EventCallback<ChangeEventArgs> _onChangeEventCallback = EventCallback<ChangeEventArgs>.Empty;
protected override void OnParametersSet()
{
base.OnParametersSet();
PlaceHolder ??= Localizer[nameof(PlaceHolder)];
NoSearchDataText ??= Localizer[nameof(NoSearchDataText)];
DropdownIcon ??= IconTheme.GetIconByKey(ComponentIcons.SelectDropdownIcon);
ClearIcon ??= IconTheme.GetIconByKey(ComponentIcons.SelectClearIcon);
_onChangeEventCallback = IsEditable
? EventCallback.Factory.Create<ChangeEventArgs>(this, OnChange)
: EventCallback<ChangeEventArgs>.Empty;
}
protected override ValueTask DisposeAsync(bool disposing)
{
if (disposing)
{
_onChangeEventCallback = EventCallback<ChangeEventArgs>.Empty;
}
return base.DisposeAsync(disposing);
}Bind directly in the .razor:
<input
@onchange="(IsEditable
? EventCallback.Factory.Create<ChangeEventArgs>(this, OnChange)
: EventCallback<ChangeEventArgs>.Empty)" />Then you can delete:
private EventCallback<ChangeEventArgs> _onChangeEventCallback = EventCallback<ChangeEventArgs>.Empty;and the DisposeAsync override entirely. The component instance is short‑lived; clearing a value-type callback in DisposeAsync brings no real benefit.
If you prefer to keep the binding simple, another option is to short‑circuit inside the handler:
private Task OnChange(ChangeEventArgs e)
{
if (!IsEditable)
{
return Task.CompletedTask;
}
// existing logic...
}and then use a straightforward binding:
<input @onchange="OnChange" />Either pattern removes the extra field and lifecycle complexity while preserving behavior.
2. Reconsider explicit StateHasChanged in OnClickItem
OnClickItem currently forces a render:
private async Task OnClickItem(SelectedItem item)
{
if (!item.IsDisabled)
{
_defaultVirtualizedItemText = item.Text;
await SelectedItemChanged(item);
}
StateHasChanged();
}If SelectedItemChanged flows into CurrentValue / parameter updates that already trigger a render (which is common in Blazor forms), the explicit StateHasChanged() is redundant and can be removed:
private async Task OnClickItem(SelectedItem item)
{
if (!item.IsDisabled)
{
_defaultVirtualizedItemText = item.Text;
await SelectedItemChanged(item);
}
}If you’ve hit a specific scenario where a manual refresh is required (e.g., async external state not bound to the component), consider adding a brief comment so future readers understand why it’s needed:
private async Task OnClickItem(SelectedItem item)
{
if (!item.IsDisabled)
{
_defaultVirtualizedItemText = item.Text;
await SelectedItemChanged(item);
}
// Explicit refresh required because <reason>, this is not triggered by normal binding.
StateHasChanged();
}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7894 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 764 764
Lines 34177 34200 +23
Branches 4703 4705 +2
=========================================
+ Hits 34177 34200 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Updates Select to use DynamicElement for option rows and adjusts event callback handling to better ensure event callbacks are released/managed correctly (per #7893).
Changes:
- Introduces a cached
@onchangeEventCallbackthat is only wired whenIsEditableis enabled, and clears it on disposal. - Replaces option-row
<div @onclick>rendering with<DynamicElement OnClick=...>for row click handling. - Triggers a re-render after item selection to reflect updated selection state.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/BootstrapBlazor/Components/Select/Select.razor.cs |
Adds cached onchange callback setup/cleanup and forces re-render after selection. |
src/BootstrapBlazor/Components/Select/Select.razor |
Uses _onChangeEventCallback for @onchange and switches row rendering to DynamicElement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
396
to
400
| await SelectedItemChanged(item); | ||
| } | ||
|
|
||
| StateHasChanged(); | ||
| } |
There was a problem hiding this comment.
OnClickItem now calls StateHasChanged(), but ConfirmSelectedItem also calls StateHasChanged() after awaiting OnClickItem. This means selecting via keyboard Enter (JS-invoked) will trigger two renders. Consider removing the extra StateHasChanged() from ConfirmSelectedItem so the selection path renders only once.
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 #7893
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Improve the Select component’s event handling and item rendering behavior to ensure proper callback lifecycle and better integration with DynamicElement.
Bug Fixes:
Enhancements: