Fix item order issues in multi-api components (#12653)#12659
Open
msynk wants to merge 14 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSeveral Blazor components now delegate option rendering to dedicated item components, refresh registered options after state changes, and preserve conditional option order. Breadcrumb adds DOM markers and JavaScript-based ordering synchronization. New bUnit tests cover ordering and interactions. ChangesOption rendering and state refresh
Estimated code review effort: 4 (Complex) | ~75 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor (1)
38-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPreserve state and focus with a
@keyattribute.When rendering a list of Blazor components dynamically, it is highly recommended to bind a
@keyattribute. This helps Blazor's diffing algorithm preserve DOM elements, component internal state, and user focus when list items are added, removed, or reordered.♻️ Proposed fix
`@foreach` (var item in _items) { - <_BitChoiceGroupItem Item="item" ChoiceGroup="this" /> + <_BitChoiceGroupItem `@key`="item" Item="item" ChoiceGroup="this" /> }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor` around lines 38 - 41, Update the foreach rendering in BitChoiceGroup to add a stable `@key` for each _BitChoiceGroupItem, using the item’s unique identity, while preserving the existing Item and ChoiceGroup parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cs`:
- Around line 150-155: Guard the Parent.RegisterOption call in
OnInitializedAsync with a null check, matching BuildRenderTree’s behavior when
no BitButtonGroup parent exists. Update Dispose to call Parent.UnregisterOption
only when Parent is not null, while preserving the existing disposal state
handling.
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs`:
- Around line 211-222: The internal selection update paths do not refresh navbar
options, leaving active styling stale. Add RefreshOptions() after
SetSelectedItemByCurrentUrl() in OnLocationChanged, and after
OnSelectItem.InvokeAsync(item) in SetSelectedItem, before StateHasChanged(),
while preserving the existing selection guards and assignment flow.
---
Nitpick comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor`:
- Around line 38-41: Update the foreach rendering in BitChoiceGroup to add a
stable `@key` for each _BitChoiceGroupItem, using the item’s unique identity,
while preserving the existing Item and ChoiceGroup parameters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d957eb77-e5ec-486f-be74-324e0b55dd3d
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs (1)
224-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCall
RefreshOptions()when the selection is updated internally.The internal selection update paths do not explicitly refresh the navbar options, leaving the active styling stale. When a user clicks an item or the URL changes in automatic mode, the selection state updates internally, but the child option components are not re-rendered (similar to how
BitNavhandles it).
src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs#L224-L229: Add a call toRefreshOptions();right beforeStateHasChanged();inOnLocationChanged.src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs#L257-L266: Add a call toRefreshOptions();right beforeStateHasChanged();inSetSelectedItem.🛠️ Proposed fixes
private void OnLocationChanged(object? sender, LocationChangedEventArgs args) { SetSelectedItemByCurrentUrl(); + RefreshOptions(); StateHasChanged(); }private async Task SetSelectedItem(TItem item) { if (item == SelectedItem && Reselectable is false) return; if (await AssignSelectedItem(item) is false) return; await OnSelectItem.InvokeAsync(item); + RefreshOptions(); StateHasChanged(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs` around lines 224 - 229, Refresh navbar option components after internal selection updates: in src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs lines 224-229, add RefreshOptions() before StateHasChanged() in OnLocationChanged; make the same change in SetSelectedItem at lines 257-266.src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cs (1)
339-342: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck
Optionsas an alias forChildContent.The
Optionsparameter is documented as an alias forChildContent. If a user providesOptionsinstead ofChildContent,ChildContentwill benull. Consequently, this logic will bypass the early return and overwrite the_itemscollection withItems, discarding any options registered via the<BitMenuButtonOption>elements.🐛 Proposed fix
- if (ChildContent is not null || Items.Any() is false || Items == _oldItems) return; + if (ChildContent is not null || Options is not null || Items.Any() is false || Items == _oldItems) return; _oldItems = Items;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cs` around lines 339 - 342, Update the items-refresh guard in the relevant menu-button lifecycle method to treat the documented Options parameter as an alias for ChildContent. When either ChildContent or Options is provided, return before replacing _items; preserve the existing empty-Items and unchanged-reference checks.
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cs (1)
272-283: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueRemove
asyncfromHandleChangeto resolve compiler warning.The
HandleChangemethod is marked asasync Taskbut contains noawaitexpressions, which will cause a CS1998 compiler warning. Because the method performs only synchronous state updates, consider changing the return type tovoidor explicitly returningTask.CompletedTask.♻️ Proposed refactor to use
void- internal async Task HandleChange(TItem item) + internal void HandleChange(TItem item) { if (IsEnabled is false || ReadOnly || GetIsEnabled(item) is false) return;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cs` around lines 272 - 283, Update HandleChange to remove the unnecessary async modifier and use a synchronous return type, preserving its existing state-update logic and call behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cs`:
- Around line 78-83: Guard the Parent calls in
BitMenuButtonOption.OnInitializedAsync and Dispose with null-conditional
invocation: update RegisterOption and UnregisterOption so both lifecycle methods
safely handle a missing Parent. Apply this in
src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cs
at lines 78-83 and 103-110.
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs`:
- Around line 570-576: Update ShouldRenderOptionItem to avoid calling
GetSearchedItems for every option. When SearchFunction is not provided, evaluate
the current item directly against _searchText using the existing item-text
comparison logic; retain the SearchFunction path for custom filtering and
preserve the no-search behavior.
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cs`:
- Around line 95-100: Guard nullable Parent access in
BitDropdownOption.OnInitializedAsync by registering with
Parent?.RegisterOption(this), and apply the same null-safe access in Dispose
using Parent?.UnregisterOption(this). Update both affected locations in
src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cs (lines
95-100 and 126-133).
In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor`:
- Line 21: The class attribute in _BitTimelineItem should separate the base
bit-tln-itm class from the classes returned by Timeline.GetItemClasses(Item).
Add a literal space before that interpolation while preserving the existing
Timeline.Classes?.Item concatenation.
---
Outside diff comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cs`:
- Around line 339-342: Update the items-refresh guard in the relevant
menu-button lifecycle method to treat the documented Options parameter as an
alias for ChildContent. When either ChildContent or Options is provided, return
before replacing _items; preserve the existing empty-Items and
unchanged-reference checks.
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs`:
- Around line 224-229: Refresh navbar option components after internal selection
updates: in src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs
lines 224-229, add RefreshOptions() before StateHasChanged() in
OnLocationChanged; make the same change in SetSelectedItem at lines 257-266.
---
Nitpick comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cs`:
- Around line 272-283: Update HandleChange to remove the unnecessary async
modifier and use a synchronous return type, preserving its existing state-update
logic and call behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1494a02e-8424-4729-8773-f6830c453d1c
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 14 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs (1)
224-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCall
RefreshOptions()when the selection is updated internally.
RefreshOptions()correctly ensures that option components re-render when the navbar state changes. However, becauseOnSetSelectedItemis only triggered when theSelectedItemparameter is updated by a parent component (via the[CallOnSet]attribute), internal selection updates bypass this refresh mechanism.When a user clicks an item (triggering
SetSelectedItem) or the URL changes in automatic mode (triggeringOnLocationChanged), the selection state updates internally but the option components are not explicitly refreshed. As a result, the active/selected CSS class will fail to update on the rendered UI items.Apply the following changes to ensure internal state updates also propagate to the options (similar to how
BitNav.razor.cshandles it):🐛 Proposed fix
private void OnLocationChanged(object? sender, LocationChangedEventArgs args) { SetSelectedItemByCurrentUrl(); + RefreshOptions(); StateHasChanged(); }private async Task SetSelectedItem(TItem item) { if (item == SelectedItem && Reselectable is false) return; if (await AssignSelectedItem(item) is false) return; await OnSelectItem.InvokeAsync(item); + RefreshOptions(); StateHasChanged(); }Also applies to: 257-266
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs` around lines 224 - 229, Update the internal selection flows in BitNavBar, specifically SetSelectedItem and OnLocationChanged, to call RefreshOptions() after SetSelectedItemByCurrentUrl or other selection state updates. Preserve the existing StateHasChanged behavior and ensure both user-triggered selection and automatic URL-driven selection refresh the option components.
♻️ Duplicate comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor (1)
21-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a space before the
@(Timeline.GetItemClasses(Item))call.Without a space, the base class
bit-tln-itmwill concatenate directly with the first class returned byGetItemClasses(e.g., resulting inbit-tln-itmcustom-class), breaking both the base styling and the applied class.🐛 Proposed fix
- class="bit-tln-itm@(Timeline.GetItemClasses(Item)) `@Timeline.Classes`?.Item"> + class="bit-tln-itm @(Timeline.GetItemClasses(Item)) `@Timeline.Classes`?.Item">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor` at line 21, Update the class attribute in _BitTimelineItem to include a separating space between the base class “bit-tln-itm” and the output of Timeline.GetItemClasses(Item), preserving the existing Timeline.Classes?.Item expression.
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor (1)
17-22: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd
@keyto the looped items to prevent rendering bugs.In Blazor, looping over components without a
@keycan lead to state leakage or DOM patching bugs when items are added, removed, or reordered. Consider adding a@key(e.g.,@key="item") to_BitButtonGroupItem, similar to how it was done safely inBitChoiceGroup.razor.⚡ Proposed fix
else { `@foreach` (var item in _items) { - <_BitButtonGroupItem ButtonGroup="this" Item="item" /> + <_BitButtonGroupItem `@key`="item" ButtonGroup="this" Item="item" /> }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor` around lines 17 - 22, Update the foreach rendering of _BitButtonGroupItem in the ButtonGroup component to include a stable `@key` based on item, preserving correct component state and DOM updates when _items changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs`:
- Around line 1051-1052: Remove RefreshOptions() from OnAfterRenderAsync, then
invoke it synchronously at the end of OnParametersSetAsync and immediately after
internal state mutations in SearchItems, ClearSearchBox, ClearComboBoxInput,
AddOrRemoveSelectedItem, UpdateSelectedItemsFromValues, and HandleOnClearClick,
following the established BitChoiceGroup pattern.
In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs`:
- Around line 122-133: The child components receive stable parameter references
and do not refresh when parent-driven state changes. In
src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs lines
122-133, update BuildRenderTree and its _BitTimelineItem usage to provide a
changing parameter or explicit refresh trigger; in
src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor lines 5-21,
apply the equivalent change to _BitNavChild so childItems.Any() is reevaluated
after nested options register and the display wrapper updates correctly.
---
Outside diff comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs`:
- Around line 224-229: Update the internal selection flows in BitNavBar,
specifically SetSelectedItem and OnLocationChanged, to call RefreshOptions()
after SetSelectedItemByCurrentUrl or other selection state updates. Preserve the
existing StateHasChanged behavior and ensure both user-triggered selection and
automatic URL-driven selection refresh the option components.
---
Duplicate comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor`:
- Line 21: Update the class attribute in _BitTimelineItem to include a
separating space between the base class “bit-tln-itm” and the output of
Timeline.GetItemClasses(Item), preserving the existing Timeline.Classes?.Item
expression.
---
Nitpick comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor`:
- Around line 17-22: Update the foreach rendering of _BitButtonGroupItem in the
ButtonGroup component to include a stable `@key` based on item, preserving correct
component state and DOM updates when _items changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6aa8ff5-80eb-460b-ac21-d96453e0b6b5
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs (1)
216-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCall
RefreshOptions()when the selection is updated internally.
RefreshOptions()correctly ensures that option components re-render when the navbar state changes. However, becauseOnSetSelectedItemis only triggered when theSelectedItemparameter is updated by a parent component (via the[CallOnSet]attribute), internal selection updates bypass this refresh mechanism.When a user clicks an item (triggering
SetSelectedItem) or the URL changes in automatic mode (triggeringOnLocationChanged), the selection state updates internally but the option components are not explicitly refreshed. As a result, the active/selected CSS class will fail to update on the rendered UI items.Apply the following changes to ensure internal state updates also propagate to the options (similar to how
BitNav.razor.cshandles it).🐛 Proposed fix for internal selection updates
private void OnLocationChanged(object? sender, LocationChangedEventArgs args) { SetSelectedItemByCurrentUrl(); + RefreshOptions(); StateHasChanged(); }Also, apply a similar change to
SetSelectedItemfurther down in the file:private async Task SetSelectedItem(TItem item) { if (item == SelectedItem && Reselectable is false) return; if (await AssignSelectedItem(item) is false) return; await OnSelectItem.InvokeAsync(item); RefreshOptions(); // <-- Add this StateHasChanged(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs` around lines 216 - 229, Update the internal selection flows to refresh navbar options after state changes: call RefreshOptions() in SetSelectedItem after OnSelectItem.InvokeAsync succeeds and before StateHasChanged, and call it in OnLocationChanged after SetSelectedItemByCurrentUrl(). Preserve the existing parent-driven OnSetSelectedItem behavior and event ordering.src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs (1)
1051-1052: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftMove
RefreshOptions()out ofOnAfterRenderAsyncto avoid UI flicker and double rendering.Calling
StateHasChanged()(viaRefreshOptions()) on child components from withinOnAfterRenderAsyncforces them to re-render after the browser has already painted the parent component's updates. This causes an unnecessary double-render cycle on every parent render and will introduce a visible one-frame UI flicker when search text or selections change.Remove this call from
OnAfterRenderAsync. Instead, invokeRefreshOptions()synchronously at the end ofOnParametersSetAsync()and exactly at the sites where the internal state mutates (such as insideSearchItems(),ClearSearchBox(),ClearComboBoxInput(),AddOrRemoveSelectedItem(),UpdateSelectedItemsFromValues(), andHandleOnClearClick()). This matches the pattern safely implemented inBitChoiceGroup.razor.cs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs` around lines 1051 - 1052, Remove RefreshOptions() from OnAfterRenderAsync, then invoke it synchronously at the end of OnParametersSetAsync and immediately after internal state changes in SearchItems, ClearSearchBox, ClearComboBoxInput, AddOrRemoveSelectedItem, UpdateSelectedItemsFromValues, and HandleOnClearClick, following the established BitChoiceGroup pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs`:
- Around line 122-133: Make the child item components refreshable by passing a
changing state parameter to _BitChoiceGroupItem in BitChoiceGroupOption.cs
(lines 122-133) and to _BitTimelineItem in BitTimelineOption.cs (lines 122-133).
Add or reuse a StateId value incremented by InternalStateHasChanged(), and pass
it alongside the existing Parent and Item parameters so each parent-driven state
update bypasses parameter equality checks.
---
Duplicate comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs`:
- Around line 1051-1052: Remove RefreshOptions() from OnAfterRenderAsync, then
invoke it synchronously at the end of OnParametersSetAsync and immediately after
internal state changes in SearchItems, ClearSearchBox, ClearComboBoxInput,
AddOrRemoveSelectedItem, UpdateSelectedItemsFromValues, and HandleOnClearClick,
following the established BitChoiceGroup pattern.
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs`:
- Around line 216-229: Update the internal selection flows to refresh navbar
options after state changes: call RefreshOptions() in SetSelectedItem after
OnSelectItem.InvokeAsync succeeds and before StateHasChanged, and call it in
OnLocationChanged after SetSelectedItemByCurrentUrl(). Preserve the existing
parent-driven OnSetSelectedItem behavior and event ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2edcbc57-cddd-43ac-abde-9a00a9304a4b
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cs (1)
67-78: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnsure option rendering updates when
SingleExpandis false.When
SingleExpandis false,ToggleItemupdates the item's expansion state but skips callingRefreshOptions()andStateHasChanged(). As a result, the UI will not re-render the nested option children to reflect the toggled expansion state. Move the refresh calls outside theif (SingleExpand)block so that both modes correctly trigger a re-render.💡 Proposed fix
- RefreshOptions(); - StateHasChanged(); - _currentItem = Item; } else { SetItemExpanded(Item, isExpanded); } + RefreshOptions(); + StateHasChanged(); + await OnItemToggle.InvokeAsync(Item);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cs` around lines 67 - 78, Update ToggleItem so RefreshOptions() and StateHasChanged() execute after both the SingleExpand and non-SingleExpand branches, ensuring expansion changes re-render nested options in either mode. Keep the existing _currentItem assignment limited to the SingleExpand path.
♻️ Duplicate comments (2)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs (1)
122-130: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the child item components refreshable.
The child components receive stable parameter references and will not refresh when parent-driven state changes.
_BitChoiceGroupItemand_BitTimelineItemreceive the sameItem="this"and parent references on each render. WhenInternalStateHasChanged()is called, Blazor's parameter diffing sees that the references haven't changed and skips re-rendering the inner component.To force a refresh, provide a changing parameter (such as a
StateIdinteger that increments inInternalStateHasChanged) or trigger an explicit refresh method on the inner component.
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs#L122-L130: pass a changing parameter (e.g.,StateId) to bypass equality checks for_BitChoiceGroupItem.src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs#L124-L132: pass a changing parameter to bypass equality checks for_BitTimelineItem.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs` around lines 122 - 130, Child item components do not refresh when parent state changes because their parameter references remain stable. In BitChoiceGroupOption.cs lines 122-130, add and pass a changing StateId (or equivalent refresh trigger) to _BitChoiceGroupItem, and ensure InternalStateHasChanged increments it; apply the same change to _BitTimelineItem in BitTimelineOption.cs lines 124-132. Preserve existing ChoiceGroup and Item parameters while ensuring each internal state update changes the refresh parameter.src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor (1)
21-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a space before the
@(Timeline.GetItemClasses(Item))call.Without a space, the base class
bit-tln-itmwill concatenate directly with the first class returned byGetItemClasses(e.g., resulting inbit-tln-itmcustom-class), breaking both the base styling and the applied class.🐛 Proposed fix
- class="bit-tln-itm@(Timeline.GetItemClasses(Item)) `@Timeline.Classes`?.Item"> + class="bit-tln-itm @(Timeline.GetItemClasses(Item)) `@Timeline.Classes`?.Item">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor` at line 21, Add a separating space after the base class `bit-tln-itm` in the class attribute of `_BitTimelineItem`, before the `Timeline.GetItemClasses(Item)` output, so the base and dynamic classes remain distinct.
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cs (1)
330-339: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate redundant
RefreshOptions()calls.The
OnSetSelectedItemmethod callsRefreshOptions()twice. SinceToggleItemAndParentsonly updates the internal dictionary state without yielding or blocking, a singleRefreshOptions()call at the end is sufficient to apply both the new selection state and the updated expansion states to the rendered options.♻️ Proposed refactor
private void OnSetSelectedItem() { - // The selection affects the previously and newly selected items, which render themselves in - // the options mode, so push a re-render to all of them. - RefreshOptions(); - - if (SelectedItem is null) return; - - ToggleItemAndParents(_items, SelectedItem, true); + if (SelectedItem is not null) + { + ToggleItemAndParents(_items, SelectedItem, true); + } + // The selection affects the previously and newly selected items, which render themselves in + // the options mode, so push a re-render to all of them. RefreshOptions(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cs` around lines 330 - 339, Remove the first RefreshOptions() call in OnSetSelectedItem and retain a single call after ToggleItemAndParents. Preserve the existing null-selection early return and ensure the remaining refresh applies both selection and expansion-state updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor`:
- Around line 18-21: Update the foreach loop rendering _BitTimelineItem in the
Timeline component to include `@key`="item", preserving stable component mapping
and state when _items changes.
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cs`:
- Around line 246-266: The breadcrumb update flow can render after disposal. In
BitBreadcrumb.razor.cs, add an IsDisposed guard in UnregisterOptions (lines
148-155) and check IsDisposed after the JS await in OnAfterRenderAsync (lines
246-266), before ReorderOptions executes; leave the existing exception handling
and ordering behavior unchanged.
---
Outside diff comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cs`:
- Around line 67-78: Update ToggleItem so RefreshOptions() and StateHasChanged()
execute after both the SingleExpand and non-SingleExpand branches, ensuring
expansion changes re-render nested options in either mode. Keep the existing
_currentItem assignment limited to the SingleExpand path.
---
Duplicate comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs`:
- Around line 122-130: Child item components do not refresh when parent state
changes because their parameter references remain stable. In
BitChoiceGroupOption.cs lines 122-130, add and pass a changing StateId (or
equivalent refresh trigger) to _BitChoiceGroupItem, and ensure
InternalStateHasChanged increments it; apply the same change to _BitTimelineItem
in BitTimelineOption.cs lines 124-132. Preserve existing ChoiceGroup and Item
parameters while ensuring each internal state update changes the refresh
parameter.
In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor`:
- Line 21: Add a separating space after the base class `bit-tln-itm` in the
class attribute of `_BitTimelineItem`, before the
`Timeline.GetItemClasses(Item)` output, so the base and dynamic classes remain
distinct.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cs`:
- Around line 330-339: Remove the first RefreshOptions() call in
OnSetSelectedItem and retain a single call after ToggleItemAndParents. Preserve
the existing null-selection early return and ensure the remaining refresh
applies both selection and expansion-state updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a9ff4aef-05c6-4ca4-b6da-3e7b7890a59d
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs (1)
224-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCall
RefreshOptions()when the selection is updated internally.Because
OnSetSelectedItemis only triggered when theSelectedItemparameter is updated by a parent component, internal selection updates bypass the refresh mechanism. This leaves the active styling stale on the rendered UI items. CallRefreshOptions()after updating the selection state to ensure option components re-render.
src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs#L224-L229: AddRefreshOptions();afterSetSelectedItemByCurrentUrl();inOnLocationChanged.src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs#L257-L266: AddRefreshOptions();afterawait OnSelectItem.InvokeAsync(item);inSetSelectedItem.🐛 Proposed fixes
Update
OnLocationChanged:private void OnLocationChanged(object? sender, LocationChangedEventArgs args) { SetSelectedItemByCurrentUrl(); + RefreshOptions(); StateHasChanged(); }Update
SetSelectedItem:private async Task SetSelectedItem(TItem item) { if (item == SelectedItem && Reselectable is false) return; if (await AssignSelectedItem(item) is false) return; await OnSelectItem.InvokeAsync(item); + RefreshOptions(); StateHasChanged(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs` around lines 224 - 229, Internal selection updates in BitNavBar bypass option refreshes, leaving active styling stale. In src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs lines 224-229, update OnLocationChanged to call RefreshOptions() after SetSelectedItemByCurrentUrl(); at lines 257-266, update SetSelectedItem to call RefreshOptions() after await OnSelectItem.InvokeAsync(item).
♻️ Duplicate comments (2)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs (1)
122-130: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake the delegated inner item components refreshable.
The option and item components receive stable object references (
this,Parent, oritem) as parameters. When parent state changes and explicitly triggersInternalStateHasChanged()on the options (e.g., viaRefreshOptions()), Blazor's parameter diffing (ReferenceEquals) sees unchanged parameters and skips re-rendering the inner item components (_BitChoiceGroupItem, etc.). Consequently, parent-driven updates (like selection changes or disable toggles) will fail to reflect in the DOM.
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs#L122-L130: Add a changing state parameter (e.g., aStateIdinteger that increments inInternalStateHasChanged) to_BitChoiceGroupItemor trigger an explicit refresh method to bypass Blazor's parameter equality skip.src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs#L124-L132: Pass a changing state parameter to_BitTimelineItemto ensure it refreshes on parent state changes.src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cs#L87-L95: Pass a changing state parameter to_BitMenuButtonItemto ensure it refreshes on parent state changes.src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor#L38-L41: Pass the same changing state parameter to_BitChoiceGroupItemin theItemsloop to ensure it re-renders when parent parameters change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs` around lines 122 - 130, The delegated item components currently receive unchanged references, so parent-triggered refreshes skip their renders. Add and maintain a changing state parameter (such as a StateId incremented by InternalStateHasChanged), then pass it to _BitChoiceGroupItem in src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs lines 122-130 and BitChoiceGroup.razor lines 38-41, to _BitTimelineItem in src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs lines 124-132, and to _BitMenuButtonItem in src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cs lines 87-95, ensuring all parent-driven state changes re-render the inner items.src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs (1)
1054-1055: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftMove
RefreshOptions()out ofOnAfterRenderAsyncto avoid UI flicker and double rendering.Calling
StateHasChanged()(viaRefreshOptions()) on child components from withinOnAfterRenderAsyncforces them to re-render after the browser has already painted the parent component's updates. This causes an unnecessary double-render cycle on every parent render and will introduce a visible one-frame UI flicker when search text or selections change.Remove this call from
OnAfterRenderAsync. Instead, invokeRefreshOptions()synchronously at the end ofOnParametersSetAsync()and exactly at the sites where the internal state mutates (such as insideSearchItems(),ClearSearchBox(),ClearComboBoxInput(),AddOrRemoveSelectedItem(),UpdateSelectedItemsFromValues(), andHandleOnClearClick()). This matches the pattern safely implemented inBitChoiceGroup.razor.csand correctly applied in this PR withinBitTimeline.razor.cs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs` around lines 1054 - 1055, The RefreshOptions call in OnAfterRenderAsync causes an unnecessary post-paint render; remove it there and invoke RefreshOptions synchronously at the end of OnParametersSetAsync and at each internal state mutation site, including SearchItems, ClearSearchBox, ClearComboBoxInput, AddOrRemoveSelectedItem, UpdateSelectedItemsFromValues, and HandleOnClearClick.
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor (1)
19-22: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer using a stable key property instead of the item reference.
For better rendering stability and consistency with other components (like
BitAccordionList), consider usingGetItemKey(item)as the loop key instead of the item reference itself. Using a consistent primitive key prevents unnecessary element recreation if object references change during updates.♻️ Proposed refactor
`@foreach` (var item in _items) { - <_BitButtonGroupItem `@key`="item" ButtonGroup="this" Item="item" /> + <_BitButtonGroupItem `@key`="GetItemKey(item)" ButtonGroup="this" Item="item" /> }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor` around lines 19 - 22, Update the foreach rendering in BitButtonGroup to use GetItemKey(item) for the `@key` value instead of the item reference. Preserve the existing ButtonGroup and Item bindings while ensuring the key is a stable primitive consistent with other components.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cs`:
- Around line 28-29: Update the public CollapseAll and ExpandAll methods in
BitNav so each calls StateHasChanged() immediately after RefreshOptions(); apply
the change at both specified locations in
src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cs (lines 28-29 and
40-41).
---
Outside diff comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs`:
- Around line 224-229: Internal selection updates in BitNavBar bypass option
refreshes, leaving active styling stale. In
src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs lines
224-229, update OnLocationChanged to call RefreshOptions() after
SetSelectedItemByCurrentUrl(); at lines 257-266, update SetSelectedItem to call
RefreshOptions() after await OnSelectItem.InvokeAsync(item).
---
Duplicate comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs`:
- Around line 122-130: The delegated item components currently receive unchanged
references, so parent-triggered refreshes skip their renders. Add and maintain a
changing state parameter (such as a StateId incremented by
InternalStateHasChanged), then pass it to _BitChoiceGroupItem in
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs
lines 122-130 and BitChoiceGroup.razor lines 38-41, to _BitTimelineItem in
src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs lines
124-132, and to _BitMenuButtonItem in
src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cs
lines 87-95, ensuring all parent-driven state changes re-render the inner items.
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs`:
- Around line 1054-1055: The RefreshOptions call in OnAfterRenderAsync causes an
unnecessary post-paint render; remove it there and invoke RefreshOptions
synchronously at the end of OnParametersSetAsync and at each internal state
mutation site, including SearchItems, ClearSearchBox, ClearComboBoxInput,
AddOrRemoveSelectedItem, UpdateSelectedItemsFromValues, and HandleOnClearClick.
---
Nitpick comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor`:
- Around line 19-22: Update the foreach rendering in BitButtonGroup to use
GetItemKey(item) for the `@key` value instead of the item reference. Preserve the
existing ButtonGroup and Item bindings while ensuring the key is a stable
primitive consistent with other components.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8d9f3b39-5a7b-4f46-a095-b6623feac125
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs`:
- Around line 570-581: Update ShouldRenderOptionItem to cache the
GetSearchedItems() result for the current _searchText and reuse that cached
collection for every item during a render cycle, ensuring SearchFunction is
evaluated only once and avoiding repeated O(N) allocations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e8d155df-ad81-4887-b9b7-2739db6a3f05
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cs (1)
132-137: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRecalculate indices when an option is unregistered.
When an option is removed,
UnregisterOptionremoves it from_itemsbut does not callSetIndexItems(). This leaves the remaining options with stale, non-contiguousIndexvalues, which can corrupt index-based keyboard navigation logic oraria-posinsetaccessibility properties.🐛 Proposed fix
internal void UnregisterOption(BitChoiceGroupOption<TValue> option) { _items.Remove((option as TItem)!); + + SetIndexItems(); StateHasChanged(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cs` around lines 132 - 137, Update UnregisterOption to call SetIndexItems() immediately after removing the option from _items, before StateHasChanged(), so remaining options receive contiguous indices.
♻️ Duplicate comments (3)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs (1)
120-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the child item components refreshable.
The child item components receive stable parameter references (
Parentandthis) and will not refresh when parent-driven state changes. WhenInternalStateHasChanged()is called to force an update, Blazor's parameter equality checks detect that the object references haven't mutated and skip re-rendering the inner component, thereby breaking the intendedRefreshOptions()behavior.To force a refresh, provide a changing parameter (such as a
StateIdinteger that increments inInternalStateHasChanged) to the inner item component so each parent-driven state update bypasses parameter equality checks.
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs#L120-L131: pass a changing parameter to_BitChoiceGroupItem.src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cs#L99-L110: pass a changing parameter to_BitAccordionListItem.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs` around lines 120 - 131, Make the child item components refreshable by adding and passing a changing StateId-style parameter, incremented by InternalStateHasChanged, to _BitChoiceGroupItem in src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs lines 120-131 and to _BitAccordionListItem in src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cs lines 99-110. Preserve the existing Parent/ChoiceGroup and Item parameter wiring while ensuring each parent-driven update changes the value and bypasses parameter equality checks.src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor (1)
21-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a space before the
@(Timeline.GetItemClasses(Item))call.Without a space, the base class
bit-tln-itmwill concatenate directly with the first class returned byGetItemClasses(e.g., resulting inbit-tln-itmcustom-class), breaking both the base styling and the applied class.🐛 Proposed fix
- class="bit-tln-itm@(Timeline.GetItemClasses(Item)) `@Timeline.Classes`?.Item"> + class="bit-tln-itm @(Timeline.GetItemClasses(Item)) `@Timeline.Classes`?.Item">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor` at line 21, Update the class attribute in the BitTimelineItem markup to insert a space between the base class `bit-tln-itm` and the value produced by `Timeline.GetItemClasses(Item)`, preserving the existing `Timeline.Classes?.Item` class handling.src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs (1)
122-133: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winChild item components receive stable parameter references and will skip re-rendering.
When
InternalStateHasChanged()triggers a re-render of the option component, itsBuildRenderTreeexecutes and re-emits the child item component. However, because Blazor's defaultSetParametersAsyncuses reference equality for object parameters, and both theParent(orNavBar) andItemreferences remain unchanged, Blazor will skip re-rendering the child item component. As a result, parent-driven state updates won't be reflected in the UI.To force a re-render, pass a primitive parameter that changes (e.g., an internal
_renderCountincremented inInternalStateHasChanged()) or implement an explicit refresh method on the item component that can be invoked via an@refequivalent.
src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs#L122-L133: Ensure_BitTimelineItemis forced to re-render whenBitTimelineOption's state changes.src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cs#L109-L120: Ensure_BitNavBarItemis forced to re-render whenBitNavBarOption's state changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs` around lines 122 - 133, The child item components are not re-rendered when their option state changes because all emitted parameters retain the same references. In BitTimelineOption.cs lines 122-133, update the BitTimelineOption render/state flow to pass a changing primitive render marker to _BitTimelineItem, incremented by InternalStateHasChanged; apply the same forced-refresh mechanism to _BitNavBarItem in BitNavBarOption.cs lines 109-120.
🧹 Nitpick comments (2)
src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cs (1)
30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider updating the JS interop mock result after the state change.
When
ShowMiddleis set tofalse, the breadcrumb re-renders and may invoke the JS interop call again. Currently, the handler will continue returning the 3 marker IDs from the previousSetResultcall. Updating the result ensures the component receives the correct DOM state for the removal case.♻️ Proposed refactor
component.Render(parameters => parameters.Add(p => p.ShowMiddle, false)); + handler.SetResult(GetMarkerIds(component)); + component.WaitForAssertion(() => CollectionAssert.AreEqual(new[] { "First", "Last" }, GetItemTexts(component)));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cs` around lines 30 - 32, Update the JS interop mock result in the breadcrumb test after setting ShowMiddle to false, before the assertion, so subsequent interop calls return the two-marker DOM state representing the removed middle item. Keep the existing SetResult setup and assertions unchanged.src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs (1)
48-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant null check.
In bUnit, the
Findmethod will throw anElementNotFoundExceptionif no element matches the specified CSS selector. Therefore, theAssert.IsNotNull(btn);check is redundant and can be safely removed.♻️ Proposed refactor
var btn = component.Find(".bit-btg-itm"); - Assert.IsNotNull(btn); btn.Click();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs` around lines 48 - 51, Remove the redundant Assert.IsNotNull check after component.Find(".bit-btg-itm") in the button group test, leaving the Find call and subsequent btn.Click() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cs`:
- Around line 132-137: Update UnregisterOption to call SetIndexItems()
immediately after removing the option from _items, before StateHasChanged(), so
remaining options receive contiguous indices.
---
Duplicate comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs`:
- Around line 120-131: Make the child item components refreshable by adding and
passing a changing StateId-style parameter, incremented by
InternalStateHasChanged, to _BitChoiceGroupItem in
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs
lines 120-131 and to _BitAccordionListItem in
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cs
lines 99-110. Preserve the existing Parent/ChoiceGroup and Item parameter wiring
while ensuring each parent-driven update changes the value and bypasses
parameter equality checks.
In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor`:
- Line 21: Update the class attribute in the BitTimelineItem markup to insert a
space between the base class `bit-tln-itm` and the value produced by
`Timeline.GetItemClasses(Item)`, preserving the existing
`Timeline.Classes?.Item` class handling.
In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs`:
- Around line 122-133: The child item components are not re-rendered when their
option state changes because all emitted parameters retain the same references.
In BitTimelineOption.cs lines 122-133, update the BitTimelineOption render/state
flow to pass a changing primitive render marker to _BitTimelineItem, incremented
by InternalStateHasChanged; apply the same forced-refresh mechanism to
_BitNavBarItem in BitNavBarOption.cs lines 109-120.
---
Nitpick comments:
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs`:
- Around line 48-51: Remove the redundant Assert.IsNotNull check after
component.Find(".bit-btg-itm") in the button group test, leaving the Find call
and subsequent btn.Click() unchanged.
In
`@src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cs`:
- Around line 30-32: Update the JS interop mock result in the breadcrumb test
after setting ShowMiddle to false, before the assertion, so subsequent interop
calls return the two-marker DOM state representing the removed middle item. Keep
the existing SetResult setup and assertions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4fe8360f-1312-4c2b-a6a0-efb8015be608
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cs (1)
163-189: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard against null
Navcascading parameter in lifecycle methods.While
BitNavOption.razorcorrectly guards its rendering block with@if (Nav is not null), the lifecycle methods in the code-behind assumeNavis always present. If a developer mistakenly places this component outside a<BitNav>,Navmethod calls will throw aNullReferenceExceptionand crash the application during initialization or disposal.Use null-conditional operators or wrap the logic, similar to how the
Parentparameter is correctly guarded inBitBreadcrumbOption.
src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cs#L163-L189: wrapNav-dependent initialization logic in a null check and useNav?.RegisterOption(this).src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cs#L199-L217: useNav?.UnregisterOption(this)andNav?.SetSelectedItemByCurrentUrl().🛡️ Proposed fixes for OnInitializedAsync and Dispose
protected override async Task OnInitializedAsync() { if (Parent is null) { - Nav.RegisterOption(this); + Nav?.RegisterOption(this); } else { Parent.ChildItems.Add(this); } - if (Nav.AllExpanded) - { - Nav.SetIsExpanded(this, true); - } - - Nav.SetItemExpanded(this, Nav.GetIsExpanded(this) ?? false); - - if (Nav.Mode == BitNavMode.Automatic) - { - Nav.SetSelectedItemByCurrentUrl(); + if (Nav is not null) + { + if (Nav.AllExpanded) + { + Nav.SetIsExpanded(this, true); + } + + Nav.SetItemExpanded(this, Nav.GetIsExpanded(this) ?? false); + + if (Nav.Mode == BitNavMode.Automatic) + { + Nav.SetSelectedItemByCurrentUrl(); + } } Parent?.StateHasChanged(); await base.OnInitializedAsync(); }protected virtual void Dispose(bool disposing) { if (_disposed || disposing is false) return; if (Parent is null) { - Nav.UnregisterOption(this); + Nav?.UnregisterOption(this); } else { Parent.ChildItems.Remove(this); } - Nav.SetSelectedItemByCurrentUrl(); + Nav?.SetSelectedItemByCurrentUrl(); Parent?.StateHasChanged(); _disposed = true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cs` around lines 163 - 189, Guard all Nav-dependent lifecycle logic in BitNavOption: in OnInitializedAsync, use Nav?.RegisterOption(this) and execute the remaining Nav operations only when Nav is not null; in Dispose, use Nav?.UnregisterOption(this) and Nav?.SetSelectedItemByCurrentUrl(). Apply these changes at src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cs lines 163-189 and 199-217.src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cs (1)
251-251: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude
Optionsin the null check to prevent overwriting registered options.The Razor markup correctly prioritizes rendering
OptionsorChildContentoverItems(inBitButtonGroup.razor). However, this parameter-set check only verifies ifChildContentis null.If a consumer uses the
<Options>fragment (leavingChildContentnull) but also supplies theItemsparameter, this block will evaluate to true. Consequently, it will blow away the_itemslist that holds the registered option components and replace it withItems, completely breaking option state synchronization and re-rendering viaRefreshOptions().🐛 Proposed fix
- if (ChildContent is null && Items is not null && Items.Any()) + if (ChildContent is null && Options is null && Items is not null && Items.Any())🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cs` at line 251, Update the parameter-set condition in the button-group initialization logic to require both ChildContent and Options to be null before replacing _items from Items. Preserve the existing Items population behavior only when neither fragment is supplied, so registered option components remain intact and RefreshOptions() continues synchronizing them.
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cs (1)
258-262: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer indexer over
ElementAtforList<TItem>.Since
_itemsis guaranteed to be aList<TItem>, using the direct indexer_items[i]avoids the slight overhead of the LINQ extension method and is more idiomatic.♻️ Proposed refactor
for (int i = 0; i < _items.Count; i++) { - if (GetItemKey(_items.ElementAt(i)).HasValue()) continue; + if (GetItemKey(_items[i]).HasValue()) continue; - SetItemKey(_items.ElementAt(i), i.ToString()); + SetItemKey(_items[i], i.ToString()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cs` around lines 258 - 262, Update the loop in the ButtonGroup item-key assignment logic to use the List<TItem> indexer `_items[i]` instead of calling `_items.ElementAt(i)` in both GetItemKey and SetItemKey. Preserve the existing iteration and key-assignment behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor`:
- Line 21: Update the foreach rendering in BitButtonGroup to use the existing
GetItemKey method for the _BitButtonGroupItem `@key` value instead of the item
object, preserving stable component tracking when equivalent item instances are
recreated.
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cs`:
- Around line 85-96: The child item components currently receive unchanged
Parent and Item references, so parent-driven state updates do not refresh them.
In BuildRenderTree for BitMenuButtonOption, pass a changing refresh parameter
such as the option’s incrementing StateId to _BitMenuButtonItem; apply the same
change in
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs
lines 120-131 for _BitChoiceGroupItem and
src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs lines
122-133 for _BitTimelineItem, using each component’s existing refresh-state
symbol.
---
Outside diff comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cs`:
- Line 251: Update the parameter-set condition in the button-group
initialization logic to require both ChildContent and Options to be null before
replacing _items from Items. Preserve the existing Items population behavior
only when neither fragment is supplied, so registered option components remain
intact and RefreshOptions() continues synchronizing them.
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cs`:
- Around line 163-189: Guard all Nav-dependent lifecycle logic in BitNavOption:
in OnInitializedAsync, use Nav?.RegisterOption(this) and execute the remaining
Nav operations only when Nav is not null; in Dispose, use
Nav?.UnregisterOption(this) and Nav?.SetSelectedItemByCurrentUrl(). Apply these
changes at src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cs
lines 163-189 and 199-217.
---
Nitpick comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cs`:
- Around line 258-262: Update the loop in the ButtonGroup item-key assignment
logic to use the List<TItem> indexer `_items[i]` instead of calling
`_items.ElementAt(i)` in both GetItemKey and SetItemKey. Preserve the existing
iteration and key-assignment behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6ba776cc-26b9-4173-93ca-2bfc53081728
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
💤 Files with no reviewable changes (1)
- src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs (1)
121-133: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against disposal and refresh options on selection changes.
There are two issues in these registration methods related to the new option rendering lifecycle:
UnregisterOptioncan be called during teardown after the parent is disposed, which will throw when it reachesStateHasChanged(). Add anif (IsDisposed) return;guard.- Both methods call
SetSelectedItemByCurrentUrl(), which can alter the internally trackedSelectedItem. Without explicitly callingRefreshOptions(), existing options won't re-render to reflect the newly active selection state, leaving their CSS classes stale.🐛 Proposed fix
internal void RegisterOption(BitNavBarOption option) { _items.Add((option as TItem)!); SetSelectedItemByCurrentUrl(); + RefreshOptions(); StateHasChanged(); } internal void UnregisterOption(BitNavBarOption option) { + if (IsDisposed) return; + _items.Remove((option as TItem)!); SetSelectedItemByCurrentUrl(); + RefreshOptions(); StateHasChanged(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs` around lines 121 - 133, Update RegisterOption and UnregisterOption to return immediately from UnregisterOption when IsDisposed is true before performing state updates. In both methods, call RefreshOptions() after SetSelectedItemByCurrentUrl() so existing options re-render with the current selection state, while preserving the existing registration, selection, and StateHasChanged behavior.src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cs (1)
93-98: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard option unregistration against updates after disposal.
During component teardown, if child options are disposed after the parent, this method will call
StateHasChanged()on a disposed component. Add anif (IsDisposed) return;guard (similar to the one successfully added inBitBreadcrumb) to prevent teardown exceptions.🛡️ Proposed fix
internal void UnregisterOption(BitNavOption option) { + if (IsDisposed) return; + _items.Remove((option as TItem)!); StateHasChanged(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cs` around lines 93 - 98, Add an early IsDisposed guard at the start of BitNav.UnregisterOption, returning before modifying _items or calling StateHasChanged when the parent component has been disposed. Preserve the existing unregistration behavior for active components.
♻️ Duplicate comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs (1)
1069-1070: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftMove
RefreshOptions()out ofOnAfterRenderAsyncto avoid UI flicker and double rendering.Calling
StateHasChanged()(viaRefreshOptions()) on child components from withinOnAfterRenderAsyncforces them to re-render after the browser has already painted the parent component's updates. This causes an unnecessary double-render cycle on every parent render and will introduce a visible one-frame UI flicker when search text or selections change.Remove this call from
OnAfterRenderAsync. Instead, invokeRefreshOptions()synchronously at the end ofOnParametersSetAsync()and exactly at the sites where the internal state mutates (such as insideSearchItems(),ClearSearchBox(),ClearComboBoxInput(),AddOrRemoveSelectedItem(),UpdateSelectedItemsFromValues(), andHandleOnClearClick()), following the establishedBitChoiceGrouppattern.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs` around lines 1069 - 1070, Remove RefreshOptions() from OnAfterRenderAsync and invoke it synchronously at the end of OnParametersSetAsync and immediately after internal state mutations in SearchItems(), ClearSearchBox(), ClearComboBoxInput(), AddOrRemoveSelectedItem(), UpdateSelectedItemsFromValues(), and HandleOnClearClick(), following the BitChoiceGroup pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor`:
- Around line 9-15: Update the button element in _BitButtonGroupItem.razor to
explicitly set type="button", preserving the existing click handling and
attributes so ButtonGroup item clicks do not submit an enclosing form.
In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor`:
- Around line 16-21: Update the style attribute in the Timeline item markup to
separate Timeline.GetStyle(Item) and Timeline.Styles?.Item with a semicolon,
preserving both existing style values.
---
Outside diff comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cs`:
- Around line 93-98: Add an early IsDisposed guard at the start of
BitNav.UnregisterOption, returning before modifying _items or calling
StateHasChanged when the parent component has been disposed. Preserve the
existing unregistration behavior for active components.
In `@src/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cs`:
- Around line 121-133: Update RegisterOption and UnregisterOption to return
immediately from UnregisterOption when IsDisposed is true before performing
state updates. In both methods, call RefreshOptions() after
SetSelectedItemByCurrentUrl() so existing options re-render with the current
selection state, while preserving the existing registration, selection, and
StateHasChanged behavior.
---
Duplicate comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs`:
- Around line 1069-1070: Remove RefreshOptions() from OnAfterRenderAsync and
invoke it synchronously at the end of OnParametersSetAsync and immediately after
internal state mutations in SearchItems(), ClearSearchBox(),
ClearComboBoxInput(), AddOrRemoveSelectedItem(),
UpdateSelectedItemsFromValues(), and HandleOnClearClick(), following the
BitChoiceGroup pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3b88e6e-9be0-4538-899f-e971cda12c13
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
💤 Files with no reviewable changes (1)
- src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor (1)
21-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
GetItemKeyfor the@keyattribute to ensure stable component tracking.The
foreachloop currently uses@key="item", which relies on object identity. If theItemscollection is updated with new instances that represent the same logical items, the component will lose element tracking and unnecessarily re-create the DOM elements.Consistent with
BitAccordionList.razor, utilizing the explicitly managed string keys preserves element state across renders correctly.💚 Proposed fix
- <_BitButtonGroupItem `@key`="item" ButtonGroup="this" Item="item" /> + <_BitButtonGroupItem `@key`="GetItemKey(item)" ButtonGroup="this" Item="item" />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor` at line 21, Update the foreach rendering in BitButtonGroup to use the existing GetItemKey method for the _BitButtonGroupItem `@key` value instead of the item object, preserving stable component tracking when equivalent item instances are recreated.
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor (1)
107-110: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse a
forloop to prevent allocations during rendering.Using LINQ
.Select()in the render tree builder creates a new enumerator and closure on every render cycle. Since_itemsis inherently aList, you can use a simpleforloop to avoid these allocations and slightly improve rendering performance.⚡ Proposed refactor
- `@foreach` (var (item, idx) in _items.Select((item, idx) => (item, idx))) - { - <_BitMenuButtonItem `@key`="`@GetItemKey`(item, idx.ToString())" MenuButton="this" Item="item" /> - } + `@for` (var i = 0; i < _items.Count; i++) + { + var item = _items[i]; + <_BitMenuButtonItem `@key`="`@GetItemKey`(item, i.ToString())" MenuButton="this" Item="item" /> + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor` around lines 107 - 110, Replace the LINQ Select-based foreach in the menu item rendering block with an indexed for loop over _items, passing each item and its index to GetItemKey and _BitMenuButtonItem while preserving the existing rendering and key behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor`:
- Line 21: Update the foreach rendering in BitButtonGroup to use the existing
GetItemKey method for the _BitButtonGroupItem `@key` value instead of the item
object, preserving stable component tracking when equivalent item instances are
recreated.
---
Nitpick comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor`:
- Around line 107-110: Replace the LINQ Select-based foreach in the menu item
rendering block with an indexed for loop over _items, passing each item and its
index to GetItemKey and _BitMenuButtonItem while preserving the existing
rendering and key behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bce56625-f844-4bdd-a45c-2d7dc064c8a0
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
💤 Files with no reviewable changes (1)
- src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs
Member
Author
|
@coderabbitai full-review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cs (1)
82-110: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftChild item components will not re-render due to reference equality.
When
RefreshOptions()callsInternalStateHasChanged()on an option, the option'sBuildRenderTreeexecutes and outputs the child component (e.g.,_BitAccordionListItem) with the exact sameParentandthisobject references. Because Blazor's diffing engine uses reference equality for objects, it will conclude that the parameters haven't changed and will skip rendering the child component.As a result, parent-driven state changes (like expanding an accordion item or changing button group states) will leave the previously active items' UI completely stale, because their item components are bypassed by the renderer.
To force Blazor to re-render the child component, pass an explicitly changing primitive parameter (like a render count) whenever the option needs to refresh. You should apply this pattern across all delegated item components in this PR (e.g., ButtonGroup, ChoiceGroup, Dropdown, etc.):
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cs#L82-L110: Add an_internalRenderCountfield, increment it insideInternalStateHasChanged(), and pass it to the child inBuildRenderTree().src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cs#L3-L8: Add[Parameter] public int RenderCount { get; set; }to receive the changing value.src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs#L108-L133: Apply the same render count pattern here.🛠️ Proposed fix pattern for Option components
For example, in
BitAccordionListOption.cs:+ private int _internalRenderCount; + internal void InternalStateHasChanged() { + _internalRenderCount++; StateHasChanged(); } ... protected override void BuildRenderTree(RenderTreeBuilder builder) { if (Parent is null) return; builder.OpenComponent<_BitAccordionListItem<BitAccordionListOption>>(0); builder.AddComponentParameter(1, nameof(_BitAccordionListItem<BitAccordionListOption>.AccordionList), Parent); builder.AddComponentParameter(2, nameof(_BitAccordionListItem<BitAccordionListOption>.Item), this); + builder.AddComponentParameter(3, "RenderCount", _internalRenderCount); builder.CloseComponent(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cs` around lines 82 - 110, Add an incrementing internal render-count parameter so delegated child components re-render when options refresh. In src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cs lines 82-110, add and increment the count in InternalStateHasChanged and pass it from BuildRenderTree; in src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cs lines 3-8, add the corresponding RenderCount parameter; apply the same pattern in src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs lines 108-133.src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs (1)
615-628: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
UnregisterOptionmissingIsDisposedguard.Unlike
BitBreadcrumb.UnregisterOptions,BitNav.UnregisterOption, andBitNavBar.UnregisterOption(all updated in this PR to guard against post-disposalStateHasChanged()calls), this method has noIsDisposedcheck, so it can still mutate state and callStateHasChanged()during teardown races.🛡️ Proposed fix
internal void UnregisterOption(BitDropdownOption<TValue> option) { + if (IsDisposed) return; + var item = (option as TItem)!; Items!.Remove(item); _searchedItemsCache = null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs` around lines 615 - 628, Update BitDropdown.UnregisterOption to return immediately when IsDisposed is true, before mutating Items, _selectedItems, or invoking StateHasChanged. Preserve the existing unregister behavior for active components.
♻️ Duplicate comments (2)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs (1)
120-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winChild item components will not re-render when the option's state changes.
Because
_BitChoiceGroupItemand_BitNavChildreceive only stable parameter references (such asParent,this, andDepth), Blazor's parameter equality checks will evaluate them as unchanged. Consequently, whenStateHasChanged()is invoked on the option, the nested item component will skip re-rendering, causing internal state updates to not reflect in the UI.To force a refresh, bypass parameter equality checks by passing a changing parameter (such as an incrementing
StateIdinteger) to the child component, or expose an explicit refresh method on the inner component to manually invoke.
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs#L120-L131: Pass a changing parameter to_BitChoiceGroupItem.src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor#L3-L21: Pass a changing parameter to_BitNavChild.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs` around lines 120 - 131, The child components receive only stable parameters and therefore skip updates when their option state changes. In BitChoiceGroupOption.BuildRenderTree, pass a changing refresh parameter such as an incrementing StateId to _BitChoiceGroupItem; apply the same changing-parameter approach to _BitNavChild in src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor lines 3-21, preserving the existing rendering behavior.src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor (1)
21-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a space before the
@(Timeline.GetItemClasses(Item))call.Without a space, the base class
bit-tln-itmwill concatenate directly with the first class returned byGetItemClasses, breaking both the base styling and the applied class.🐛 Proposed fix
- class="bit-tln-itm@(Timeline.GetItemClasses(Item)) `@Timeline.Classes`?.Item"> + class="bit-tln-itm @(Timeline.GetItemClasses(Item)) `@Timeline.Classes`?.Item">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor` at line 21, Update the class attribute in the BitTimelineItem markup to insert a space between the base class `bit-tln-itm` and the `Timeline.GetItemClasses(Item)` result, preserving separate CSS class tokens while leaving the remaining class expressions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs`:
- Around line 573-595: Reset _searchedItemsCache in OnParametersSet whenever
component parameters are applied, so changes to Items cannot reuse search
results from a previous parameter set. Update the comment above
ShouldRenderOptionItem to describe this explicit refresh and parameter-driven
cache invalidation flow.
---
Outside diff comments:
In
`@src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cs`:
- Around line 82-110: Add an incrementing internal render-count parameter so
delegated child components re-render when options refresh. In
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cs
lines 82-110, add and increment the count in InternalStateHasChanged and pass it
from BuildRenderTree; in
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cs
lines 3-8, add the corresponding RenderCount parameter; apply the same pattern
in src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cs
lines 108-133.
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs`:
- Around line 615-628: Update BitDropdown.UnregisterOption to return immediately
when IsDisposed is true, before mutating Items, _selectedItems, or invoking
StateHasChanged. Preserve the existing unregister behavior for active
components.
---
Duplicate comments:
In
`@src/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cs`:
- Around line 120-131: The child components receive only stable parameters and
therefore skip updates when their option state changes. In
BitChoiceGroupOption.BuildRenderTree, pass a changing refresh parameter such as
an incrementing StateId to _BitChoiceGroupItem; apply the same
changing-parameter approach to _BitNavChild in
src/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor lines 3-21,
preserving the existing rendering behavior.
In `@src/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor`:
- Line 21: Update the class attribute in the BitTimelineItem markup to insert a
space between the base class `bit-tln-itm` and the
`Timeline.GetItemClasses(Item)` result, preserving separate CSS class tokens
while leaving the remaining class expressions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0554e8b5-8ad1-440c-b666-5cc66f41bcdf
📒 Files selected for processing (64)
src/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionList.razor.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/BitAccordionListOption.cssrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razorsrc/BlazorUI/Bit.BlazorUI.Extras/Components/AccordionList/_BitAccordionListItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/BitButtonGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/ButtonGroup/_BitButtonGroupItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButton.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/BitMenuButtonOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Buttons/MenuButton/_BitMenuButtonItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroup.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ChoiceGroup/BitChoiceGroupOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdownOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimeline.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/BitTimelineOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Lists/Timeline/_BitTimelineItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumb.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Breadcrumb/BitBreadcrumbOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNav.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/BitNavOption.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/Nav/_BitNavChild.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBar.razor.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/BitNavBarOption.cssrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razorsrc/BlazorUI/Bit.BlazorUI/Components/Navs/NavBar/_BitNavBarItem.razor.cssrc/BlazorUI/Bit.BlazorUI/Extensions/JsInterop/UtilsJsRuntimeExtensions.cssrc/BlazorUI/Bit.BlazorUI/Scripts/Utils.tssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/Button/BitButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/MenuButton/BitMenuButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ToggleButton/BitToggleButtonTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Extras/AccordionList/BitAccordionListOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/ChoiceGroup/BitChoiceGroupOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Inputs/Dropdown/BitDropdownOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Lists/Timeline/BitTimelineOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Breadcrumb/BitBreadcrumbOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/Nav/BitNavOptionsOrderTests.cssrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarOptionsTest.razorsrc/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Navs/NavBar/BitNavBarTests.cs
💤 Files with no reviewable changes (1)
- src/BlazorUI/Tests/Bit.BlazorUI.Tests/Components/Buttons/ButtonGroup/BitButtonGroupTests.cs
Comment on lines
+573
to
+595
| // The search text and the selected items change in many different places, so instead of pushing a | ||
| // re-render to the options at each of those sites, refresh them after every render of the dropdown | ||
| // itself (Blazor skips re-rendering them otherwise, since their own parameters do not change). | ||
| internal bool ShouldRenderOptionItem(TItem item) | ||
| { | ||
| if (_searchText.HasNoValue()) return true; | ||
|
|
||
| if (SearchFunction is not null) | ||
| { | ||
| // SearchFunction runs over the whole item set, so evaluate it once per search text and reuse | ||
| // the result for every option during the render cycle instead of re-running it for each item. | ||
| if (_searchedItemsCache is null || _searchedItemsCacheKey != _searchText) | ||
| { | ||
| _searchedItemsCacheKey = _searchText; | ||
| _searchedItemsCache = [.. GetSearchedItems()]; | ||
| } | ||
|
|
||
| return _searchedItemsCache.Contains(item); | ||
| } | ||
|
|
||
| return GetItemType(item) == BitDropdownItemType.Normal && | ||
| GetText(item)?.Contains(_searchText!, StringComparison.OrdinalIgnoreCase) is true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of the component.
sed -n '540,640p' src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs
printf '\n---- OnParametersSet and search-related calls ----\n'
sed -n '1040,1365p' src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs | sed -n '1,340p'
printf '\n---- Search cache usage ----\n'
rg -n "_searchedItemsCache|_searchedItemsCacheKey|ShouldRenderOptionItem|RefreshOptions\\(|RegisterOption\\(|UnregisterOption\\(" src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.csRepository: bitfoundation/bitplatform
Length of output: 13393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read any related tests or sibling patterns for cache invalidation/Disposed guards.
rg -n "IsDisposed return;|_searchedItemsCache = null|RefreshOptions\\(" src/BlazorUI/Bit.BlazorUI/Components -g '*.cs'Repository: bitfoundation/bitplatform
Length of output: 3809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact lines around the cache and disposal logic.
nl -ba src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs | sed -n '560,635p'
printf '\n---- nearby cache reset points ----\n'
rg -n "_searchedItemsCache = null|_searchedItemsCacheKey = null|_searchedItemsCacheKey != _searchText|IsDisposed" src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.csRepository: bitfoundation/bitplatform
Length of output: 203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the search implementation and any disposal guard in this component.
rg -n "GetSearchedItems\\(|IsDisposed|Dispose|DisposeAsync" src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs
printf '\n---- GetSearchedItems section ----\n'
sed -n '650,760p' src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.csRepository: bitfoundation/bitplatform
Length of output: 3422
Invalidate the search cache when parameters change.
ShouldRenderOptionItem keys _searchedItemsCache only by _searchText, so Items updates can keep reusing stale search results until the text changes. Reset the cache in OnParametersSet, and update the comment here to match the explicit-refresh flow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/BlazorUI/Bit.BlazorUI/Components/Inputs/Dropdown/BitDropdown.razor.cs`
around lines 573 - 595, Reset _searchedItemsCache in OnParametersSet whenever
component parameters are applied, so changes to Items cannot reuse search
results from a previous parameter set. Update the comment above
ShouldRenderOptionItem to describe this explicit refresh and parameter-driven
cache invalidation flow.
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
1 participant
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.
closes #12653
Summary by CodeRabbit
Bug Fixes
Tests