fix(select): select value(s) provided in @bind-Value(s) after async initialization#246
Conversation
WalkthroughRemoves the old internal Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Page as Page/Parent
participant Select as LumexSelect<TValue>
participant Notifier as ItemsCollectedNotifier<TValue>
participant Ctx as SelectContext<TValue>
User->>Page: Navigate / Render
Page->>Select: Render with @bind-Value / @bind-Values
Note over Select: First render
Select->>Notifier: Instantiate non-visual notifier
Notifier->>Ctx: If IsMultipleSelect -> UpdateSelectedItems(Select.Values)
Notifier->>Ctx: Else -> UpdateSelectedItem(Select.Value)
Note over Ctx: Selection state initialized
Note over Select: Later parameter updates (sync in OnParametersSet)
Page->>Select: Parameters updated (e.g., set in OnInitializedAsync)
Select->>Ctx: UpdateSelectedItem(s) before slot rebuild
Select->>Page: Re-render with correct selected value(s)
sequenceDiagram
autonumber
participant Test as Test Runner
participant Wrap as OnInitializedAsyncWrapper
participant Select as LumexSelect<TValue>
participant Ctx as SelectContext<TValue>
Test->>Wrap: Render wrapper with ChildContent (Select) and Action
Wrap->>Wrap: OnInitializedAsync (delay)
Wrap-->>Wrap: Invoke Action() to set Value/Values
Wrap->>Select: Parameter update
Select->>Ctx: OnParametersSet -> UpdateSelectedItem(s)
Test->>Test: WaitForAssertion
Test-->>Select: Assert selected value(s) present
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #246 +/- ##
==========================================
- Coverage 96.95% 93.13% -3.82%
==========================================
Files 70 144 +74
Lines 1542 2562 +1020
Branches 150 390 +240
==========================================
+ Hits 1495 2386 +891
- Misses 28 92 +64
- Partials 19 84 +65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/LumexUI/Components/Select/SelectContext.cs (1)
42-55: Avoid stale selection when single-value doesn’t match any itemIf
currentValueisnullor not found inItems,SelectedItemsremains as-is. That can leave a stale selection after items or value change. Clear it to reflect “no selection.”Apply:
public void UpdateSelectedItem( TValue? currentValue ) { - if( currentValue is null ) - { - return; - } - - var item = Items.Find( i => EqualityComparer<TValue>.Default.Equals( currentValue, i.Value ) ); - if( item is not null ) - { - SelectedItems.Clear(); - SelectedItems.Add( item ); - } + if( currentValue is null ) + { + SelectedItems.Clear(); + return; + } + + var item = Items.Find( i => EqualityComparer<TValue>.Default.Equals( currentValue, i.Value ) ); + SelectedItems.Clear(); + if( item is not null ) + { + SelectedItems.Add( item ); + } }
🧹 Nitpick comments (6)
src/LumexUI/Components/Select/SelectContext.cs (1)
57-69: Minor: drop null-forgiving and use sets for marginally cleaner logicIf
i.Valuecan be nullable forTValue,Contains(i.Value)is safe withEqualityComparer<TValue>.Default. Consider a set for membership checks; current approach is fine performance-wise given small N.- var matchedItems = Items.Where( i => valueSet.Contains( i.Value! ) ).ToList(); + var matchedItems = Items.Where( i => valueSet.Contains( i.Value ) ).ToList();src/LumexUI/Components/Select/LumexSelect.razor.cs (1)
171-217: Document fixed selection mode constraint
Selection mode (_context.IsMultipleSelect) is inferred only once on the firstSetParametersAsynccall (guarded by_hasInitializedParameters). If you ever need to support runtime switching between single- and multiple-select, explicitly document that changes toValue/Valuesafter initialization won’t update the mode.tests/LumexUI.Tests/Wrappers/OnInitializedAsyncWrapper.razor (1)
9-14: Make the wrapper deterministic and async-friendlyA fixed 500ms delay slows tests and can be flaky. Prefer a parameterized delay and
Func<Task>callback so tests can await deterministic signals.@code { - [Parameter] public RenderFragment? ChildContent { get; set; } - [Parameter] public Action? Action { get; set; } + [Parameter] public RenderFragment? ChildContent { get; set; } + [Parameter] public Func<Task>? Action { get; set; } + [Parameter] public int DelayMs { get; set; } = 1; protected override async Task OnInitializedAsync() { - await Task.Delay( 500 ); - - Action?.Invoke(); + await Task.Delay( DelayMs ); + if( Action is not null ) + { + await Action(); + } } }src/LumexUI/Components/Select/Internal/ItemsCollectedNotifier.cs (3)
30-52: Simplify SetParametersAsync with early return.Minor readability: invert the condition and drop the else.
-Task IComponent.SetParametersAsync( ParameterView parameters ) -{ - if( _isFirstRender ) - { - parameters.SetParameterProperties( this ); - ... - _isFirstRender = false; - return Task.CompletedTask; - } - else - { - return Task.CompletedTask; - } -} +Task IComponent.SetParametersAsync( ParameterView parameters ) +{ + if( !_isFirstRender ) + return Task.CompletedTask; + + parameters.SetParameterProperties( this ); + ... + _isFirstRender = false; + return Task.CompletedTask; +}
36-43: Optional: Skip work when no items are collected yet.For multi-select, updating with an empty
Itemsset does nothing but adds cycles. Guarding keeps intent explicit.- if( Context.IsMultipleSelect ) + if( Context.IsMultipleSelect ) { - Context.UpdateSelectedItems( Select.Values ); + if( Context.Items.Count > 0 ) + Context.UpdateSelectedItems( Select.Values ); } else { - Context.UpdateSelectedItem( Select.Value ); + if( Context.Items.Count > 0 ) + Context.UpdateSelectedItem( Select.Value ); }
5-10: Remove the redundant IComponent alias.
using Microsoft.AspNetCore.Components;already bringsIComponentinto scope.using System.ComponentModel; using Microsoft.AspNetCore.Components; -using IComponent = Microsoft.AspNetCore.Components.IComponent; -
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
src/LumexUI/Components/Select/Core/_ItemsCollectedNotifier.cs(0 hunks)src/LumexUI/Components/Select/Internal/ItemsCollectedNotifier.cs(1 hunks)src/LumexUI/Components/Select/LumexSelect.razor(2 hunks)src/LumexUI/Components/Select/LumexSelect.razor.cs(1 hunks)src/LumexUI/Components/Select/SelectContext.cs(1 hunks)tests/LumexUI.Tests/Components/Select/SelectTests.razor(9 hunks)tests/LumexUI.Tests/Wrappers/OnInitializedAsyncWrapper.razor(1 hunks)tests/LumexUI.Tests/_Imports.razor(1 hunks)
💤 Files with no reviewable changes (1)
- src/LumexUI/Components/Select/Core/_ItemsCollectedNotifier.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-30T00:22:33.596Z
Learnt from: desmondinho
PR: LumexUI/lumexui#159
File: src/LumexUI.Motion/LumexUI.Motion.csproj:9-11
Timestamp: 2025-01-30T00:22:33.596Z
Learning: LumexUI.Motion is planned to be distributed as a separate NuGet package with its own test coverage, hence the assembly-wide coverage exclusion in the project.
Applied to files:
tests/LumexUI.Tests/_Imports.razor
🧬 Code graph analysis (2)
src/LumexUI/Components/Select/Internal/ItemsCollectedNotifier.cs (2)
src/LumexUI/Components/Select/SelectContext.cs (3)
SelectContext(5-70)UpdateSelectedItems(57-69)UpdateSelectedItem(42-55)src/LumexUI/Components/Select/Core/_ItemsCollectedNotifier.cs (1)
_ItemsCollectedNotifier(17-57)
src/LumexUI/Components/Select/LumexSelect.razor.cs (1)
src/LumexUI/Components/Select/SelectContext.cs (3)
SelectContext(5-70)UpdateSelectedItem(42-55)UpdateSelectedItems(57-69)
🔇 Additional comments (11)
tests/LumexUI.Tests/_Imports.razor (1)
11-12: LGTM: imports for test helpers/wrappersBrings the new namespaces into scope for the tests as intended.
src/LumexUI/Components/Select/SelectContext.cs (1)
12-12: Rename reads better and aligns with common nomenclature
IsMultipleSelectis clearer thanIsMultiSelect. No functional concerns.src/LumexUI/Components/Select/LumexSelect.razor (2)
98-153: LGTM: multi/single paths switched to IsMultipleSelectConditional rendering now matches the renamed flag. Looks consistent with context behavior.
16-16: No action required: Internal namespace already importedThe
@using LumexUI.Internaldirective is present insrc/LumexUI/_Imports.razor, soItemsCollectedNotifierwill be discovered without additional qualifiers.src/LumexUI/Components/Select/LumexSelect.razor.cs (4)
141-144: LGTM: HasValue logic matches single/multiple semanticsUses
IsMultipleSelectand checks count vs. null appropriately.
196-207: Good guard: disallow binding both Value and Values; set selection mode onceThe validation and the initial
IsMultipleSelectinference are solid.
279-286: LGTM: single-value change flow updates context and closes popoverMatches expected UX and keeps context in sync.
288-292: LGTM: multi-value change flow updates contextConsistent with single-value path.
tests/LumexUI.Tests/Components/Select/SelectTests.razor (3)
143-161: Great coverage for async initialization paths.These two tests capture the regression described in #245 and validate the fix for both single and multiple selection.
Also applies to: 163-181
168-169: No action needed: C# 12 collection expressions are supported by net9.0
The test project targets net9.0, which defaults to LatestMajor C# (C# 12+), so the["penguin"]expression compiles without adding a LangVersion.
148-155: Ensure OnInitializedAsyncWrapper triggers a re-render after invoking the Action. Confirm that the wrapper implementation awaitsInvokeAsync(StateHasChanged)immediately after executing the providedAction.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/LumexUI/Components/Select/LumexSelect.razor.cs (1)
224-231: Good fix on single vs. multiple branch selectionBranching on
_context.IsMultipleSelectresolves the earlier multi-select path being skipped whenTValueis a struct.
🧹 Nitpick comments (2)
src/LumexUI/Components/Select/LumexSelect.razor.cs (2)
141-144: Make HasValue reflect actual selection state (avoids default(TValue) ambiguity)Optional but safer: compute from
_context.SelectedItemsso UI “filled” state mirrors actual selection.-private bool HasValue => _context.IsMultipleSelect - ? CurrentValues is { Count: > 0 } - : CurrentValue is not null; +private bool HasValue => _context.SelectedItems.Count > 0;
274-279: Also gate trigger on DisabledPrevents opening the popover when the component is disabled.
- if( _popoverRef is null || ReadOnly ) + if( _popoverRef is null || ReadOnly || Disabled ) { return Task.CompletedTask; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/LumexUI/Components/Select/LumexSelect.razor.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/LumexUI/Components/Select/LumexSelect.razor.cs (1)
src/LumexUI/Components/Select/SelectContext.cs (3)
SelectContext(5-70)UpdateSelectedItem(42-55)UpdateSelectedItems(57-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test
🔇 Additional comments (3)
src/LumexUI/Components/Select/LumexSelect.razor.cs (3)
208-214: LGTM: sensible default for LabelPlacementDefaulting to Outside when neither Label nor LabelPlacement is provided improves UX and matches expectations.
282-295: LGTM: selection state stays in sync after user interactionUpdating the context after committing value(s) and closing the popover is correct and keeps UI consistent.
191-217: Confirm runtime switching between single and multiple is unsupported
_context.IsMultipleSelectis latched on first parameter set. If consumers switch from@bind-Valueto@bind-Values(or vice versa) later, behavior is undefined. If unsupported, consider documenting; if supported, recompute each call.Would you like me to add a doc comment and guard that throws if the binding mode changes after initialization?
| private bool _isOpen; | ||
| private bool _hasInitializedParameters; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Track parameter presence flags for use during OnParametersSet
Needed by the previous comment to align behavior with the PR description.
private bool _isOpen;
private bool _hasInitializedParameters;
+ private bool _valueParamProvided;
+ private bool _valuesParamProvided;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private bool _isOpen; | |
| private bool _hasInitializedParameters; | |
| private bool _isOpen; | |
| private bool _hasInitializedParameters; | |
| private bool _valueParamProvided; | |
| private bool _valuesParamProvided; |
| /// <inheritdoc /> | ||
| public override async Task SetParametersAsync( ParameterView parameters ) | ||
| { | ||
| await base.SetParametersAsync( parameters ); | ||
|
|
||
| if( !_hasInitializedParameters ) | ||
| { | ||
| if( parameters.TryGetValue<TValue>( nameof( Value ), out var _ ) && | ||
| parameters.TryGetValue<ICollection<TValue>>( nameof( Values ), out var _ ) ) | ||
| { | ||
| throw new InvalidOperationException( | ||
| $"{GetType()} requires one of {nameof( Value )} or {nameof( Values )}, but both were specified." ); | ||
| } | ||
|
|
||
| // Set the selection mode depending on the 2-way bindable parameter. | ||
| _context.IsMultipleSelect = parameters.TryGetValue<ICollection<TValue>>( nameof( Values ), out _ ); | ||
|
|
||
| // Set the LabelPlacement to 'Outside' by default if both Label and LabelPlacement are not specified. | ||
| if( !parameters.TryGetValue<string>( nameof( Label ), out var _ ) && | ||
| !parameters.TryGetValue<LabelPlacement>( nameof( LabelPlacement ), out var _ ) ) | ||
| { | ||
| LabelPlacement = LabelPlacement.Outside; | ||
| } | ||
|
|
||
| _hasInitializedParameters = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix Value/Values presence detection (value-type bug) and enforce the mutual-exclusion check on every parameter set
- Using
TryGetValue<TValue>will not detect[Parameter] TValue? ValuewhenTValueis a non-nullable struct (e.g.,int), so the “both specified” guard can silently fail. - The mutual-exclusion check currently runs only once; it should run on every
SetParametersAsynccall to catch later updates.
Apply:
public override async Task SetParametersAsync( ParameterView parameters )
{
await base.SetParametersAsync( parameters );
- if( !_hasInitializedParameters )
- {
- if( parameters.TryGetValue<TValue>( nameof( Value ), out var _ ) &&
- parameters.TryGetValue<ICollection<TValue>>( nameof( Values ), out var _ ) )
- {
- throw new InvalidOperationException(
- $"{GetType()} requires one of {nameof( Value )} or {nameof( Values )}, but both were specified." );
- }
-
- // Set the selection mode depending on the 2-way bindable parameter.
- _context.IsMultipleSelect = parameters.TryGetValue<ICollection<TValue>>( nameof( Values ), out _ );
+ // Detect presence of parameters for this render (works for both reference and value types)
+ var hasValueParam = parameters.TryGetValue<object>( nameof( Value ), out _ );
+ var hasValuesParam = parameters.TryGetValue<ICollection<TValue>>( nameof( Values ), out _ );
+
+ // Persist for use in OnParametersSet
+ _valueParamProvided = hasValueParam;
+ _valuesParamProvided = hasValuesParam;
+
+ // Enforce mutual exclusivity on every update
+ if( hasValueParam && hasValuesParam )
+ {
+ throw new InvalidOperationException(
+ $"{GetType()} requires one of {nameof( Value )} or {nameof( Values )}, but both were specified." );
+ }
+
+ if( !_hasInitializedParameters )
+ {
+ // Set the selection mode depending on the 2-way bindable parameter.
+ _context.IsMultipleSelect = hasValuesParam;
// Set the LabelPlacement to 'Outside' by default if both Label and LabelPlacement are not specified.
if( !parameters.TryGetValue<string>( nameof( Label ), out var _ ) &&
!parameters.TryGetValue<LabelPlacement>( nameof( LabelPlacement ), out var _ ) )
{
LabelPlacement = LabelPlacement.Outside;
}
_hasInitializedParameters = true;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// <inheritdoc /> | |
| public override async Task SetParametersAsync( ParameterView parameters ) | |
| { | |
| await base.SetParametersAsync( parameters ); | |
| if( !_hasInitializedParameters ) | |
| { | |
| if( parameters.TryGetValue<TValue>( nameof( Value ), out var _ ) && | |
| parameters.TryGetValue<ICollection<TValue>>( nameof( Values ), out var _ ) ) | |
| { | |
| throw new InvalidOperationException( | |
| $"{GetType()} requires one of {nameof( Value )} or {nameof( Values )}, but both were specified." ); | |
| } | |
| // Set the selection mode depending on the 2-way bindable parameter. | |
| _context.IsMultipleSelect = parameters.TryGetValue<ICollection<TValue>>( nameof( Values ), out _ ); | |
| // Set the LabelPlacement to 'Outside' by default if both Label and LabelPlacement are not specified. | |
| if( !parameters.TryGetValue<string>( nameof( Label ), out var _ ) && | |
| !parameters.TryGetValue<LabelPlacement>( nameof( LabelPlacement ), out var _ ) ) | |
| { | |
| LabelPlacement = LabelPlacement.Outside; | |
| } | |
| _hasInitializedParameters = true; | |
| } | |
| } | |
| /// <inheritdoc /> | |
| public override async Task SetParametersAsync(ParameterView parameters) | |
| { | |
| await base.SetParametersAsync(parameters); | |
| // Detect presence of parameters for this render (works for both reference and value types) | |
| var hasValueParam = parameters.TryGetValue<object>(nameof(Value), out _); | |
| var hasValuesParam = parameters.TryGetValue<ICollection<TValue>>(nameof(Values), out _); | |
| // Persist for use in OnParametersSet | |
| _valueParamProvided = hasValueParam; | |
| _valuesParamProvided = hasValuesParam; | |
| // Enforce mutual exclusivity on every update | |
| if (hasValueParam && hasValuesParam) | |
| { | |
| throw new InvalidOperationException( | |
| $"{GetType()} requires one of {nameof(Value)} or {nameof(Values)}, but both were specified."); | |
| } | |
| if (!_hasInitializedParameters) | |
| { | |
| // Set the selection mode depending on the 2-way bindable parameter. | |
| _context.IsMultipleSelect = hasValuesParam; | |
| // Set the LabelPlacement to 'Outside' by default if both Label and LabelPlacement are not specified. | |
| if (!parameters.TryGetValue<string>(nameof(Label), out _) && | |
| !parameters.TryGetValue<LabelPlacement>(nameof(LabelPlacement), out _)) | |
| { | |
| LabelPlacement = LabelPlacement.Outside; | |
| } | |
| _hasInitializedParameters = true; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/LumexUI/Components/Select/LumexSelect.razor.cs around lines 191–217, the
presence checks use the generic TryGetValue<TValue> which fails for non-nullable
value types and the mutual-exclusion guard only runs once; change the presence
detection to use parameters.TryGetValue<object>(nameof(Value), out _) and
parameters.TryGetValue<object>(nameof(Values), out _) so presence is detected
regardless of TValue being a struct, move the mutual-exclusion check to run on
every SetParametersAsync invocation (before returning and not gated by
_hasInitializedParameters), and also update _context.IsMultipleSelect on every
call (leave the LabelPlacement defaulting behavior as required, but ensure its
presence check uses the same object-based TryGetValue).
| if( _context.Items.Count > 0 ) | ||
| { | ||
| if( !_context.IsMultipleSelect ) | ||
| { | ||
| _context.UpdateSelectedItem( Value ); | ||
| } | ||
| else | ||
| { | ||
| _context.UpdateSelectedItems( Values ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Gate selection sync on whether the corresponding parameter was provided to avoid accidental default(TValue) picks
Without checking parameter presence, single-select can auto-select an item whose value equals default(TValue) (e.g., 0) even when the user didn’t bind Value. This violates the PR’s “only when Value/Values are provided” intent.
Apply:
- if( _context.Items.Count > 0 )
- {
- if( !_context.IsMultipleSelect )
- {
- _context.UpdateSelectedItem( Value );
- }
- else
- {
- _context.UpdateSelectedItems( Values );
- }
- }
+ if( _context.Items.Count > 0 )
+ {
+ if( !_context.IsMultipleSelect )
+ {
+ if( _valueParamProvided )
+ {
+ _context.UpdateSelectedItem( Value );
+ }
+ }
+ else
+ {
+ if( _valuesParamProvided )
+ {
+ _context.UpdateSelectedItems( Values );
+ }
+ }
+ }Note: This relies on the two new flags set in SetParametersAsync.
🤖 Prompt for AI Agents
In src/LumexUI/Components/Select/LumexSelect.razor.cs around lines 222-232, the
selection synchronization currently updates the selected item(s) unconditionally
when items exist, which can cause a single-select to pick a default(TValue) when
the Value parameter was not provided; change the gates so you only call
UpdateSelectedItem/UpdateSelectedItems when the corresponding parameter was
actually provided by checking the two flags set in SetParametersAsync (e.g.,
only call UpdateSelectedItem(Value) if the Value-was-set flag is true, and only
call UpdateSelectedItems(Values) if the Values-was-set flag is true), keeping
the Items.Count > 0 check as-is.
1 participant
Closes #245
This PR fixes an issue where values provided via @bind-Value(s) were not selected after async initialization:
The fix ensures that selected item(s) are always updated when the component’s parameters are set, if:
Value/Valuesare providedItemscollection is not emptyChecklist
Summary by CodeRabbit