Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in enum selection where FirstOrDefault() would incorrectly return the default enum value (e.g., 0) instead of null, causing selection state to be incorrectly evaluated. The fix refactors the selection matching logic to use a TryFind pattern with explicit boolean returns.
Key Changes:
- Replaced
FindSelected()method withTryFindSelected()that returns a boolean and uses anoutparameter - Changed from LINQ
FirstOrDefault()to explicitforeachloop to avoid default value issues with enums and value types - Added comprehensive test suite (
EnumTests.cs) with 4 tests covering enum selection scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/SimpleBlazorMultiselect/SimpleMultiselect.razor.cs |
Refactored selection matching logic from FindSelected() to TryFindSelected() with boolean return pattern; added early return optimization for empty collections; includes minor formatting cleanup |
src/SimpleBlazorMultiselect.Tests/EnumTests.cs |
New test file with comprehensive coverage for enum selection scenarios: empty selection, prefilled options, unchecking, and checking operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| SelectedOptions.TryGetValue(option, out var existing); | ||
| return existing; | ||
| return SelectedOptions.TryGetValue(option, out existing!); |
There was a problem hiding this comment.
[nitpick] The null-forgiving operator ! is redundant here. Since SelectedOptions.TryGetValue already returns a non-nullable TItem through the out parameter when it returns true, and the [NotNullWhen(true)] attribute on the method signature already indicates that existing will be non-null when the method returns true, the null-forgiving operator is unnecessary.
Consider removing it:
return SelectedOptions.TryGetValue(option, out existing);| return SelectedOptions.TryGetValue(option, out existing!); | |
| return SelectedOptions.TryGetValue(option, out existing); |
| { | ||
| if (StringSelector(selected) == optionString) | ||
| { | ||
| existing = selected!; |
There was a problem hiding this comment.
[nitpick] The null-forgiving operator ! is redundant here. Since the code only reaches this line when a match is found (the string selector values are equal), and we're assigning a value that comes from iterating over SelectedOptions (which contains non-null items), the null-forgiving operator serves no purpose.
Consider removing it:
existing = selected;| existing = selected!; | |
| existing = selected; |
| var optionText = item.TextContent.Trim(); | ||
| if (optionText == nameof(TestEnum.OptionB)) | ||
| { | ||
| checkbox.Should().NotBeNull(); |
There was a problem hiding this comment.
Redundant null check. The checkbox is already verified to be non-null on line 63. This duplicate assertion on line 68 is unnecessary and should be removed.
Consider removing this line since it's already checked above.
| checkbox.Should().NotBeNull(); |
| foreach (var selected in SelectedOptions) | ||
| { | ||
| if (StringSelector(selected) == optionString) | ||
| { | ||
| existing = selected!; | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
Fix selection issues when using enums caused by FirstOrDefault returning default, might have been an issue with other value types as well. Closes #14 .