Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts TreeView disabled and expansion behavior so that CanExpandWhenDisabled works when the TreeView itself is disabled, by centralizing disabled-state logic in TreeViewRow, removing an extension helper, and updating click handling and tests accordingly. Sequence diagram for TreeView click and expand when disabledsequenceDiagram
actor User
participant TreeViewRow as TreeViewRow_TItem_
participant TreeView as TreeView_TItem_
participant TreeViewItem as TreeViewItem_TItem_
User->>TreeViewRow: ClickNode()
TreeViewRow->>TreeViewRow: GetDisabledStatus()
alt disabled and cannot expand
TreeViewRow-->>User: IgnoreClick()
else enabled or can expand when disabled
TreeViewRow->>TreeView: OnClick(TreeViewItem)
TreeView->>TreeView: check ClickToggleNode
alt ClickToggleNode is true
TreeView->>TreeView: OnToggleNodeAsync(TreeViewItem)
else ClickToggleNode is false
TreeView-->>User: NoToggle()
end
end
Class diagram for updated TreeView disabled and expand behaviorclassDiagram
class TreeViewRow_TItem_ {
+bool IsDisabled
+bool CanExpandWhenDisabled
+TreeViewItem_TItem_ Item
+bool GetDisabledStatus()
+bool CanTriggerClickNode
+bool ItemDisabledState
}
class TreeView_TItem_ {
+bool IsDisabled
+bool CanExpandWhenDisabled
+bool ClickToggleNode
+Task OnClick(TreeViewItem_TItem_ item)
+Task OnToggleNodeAsync(TreeViewItem_TItem_ item)
}
class TreeViewItem_TItem_ {
+bool IsDisabled
+bool HasChildren
+bool IsExpand
+List_TreeViewItem_TItem__ Items
}
class TreeViewExtensions {
-bool CanTriggerClickNode_TItem_(TreeViewItem_TItem_ item, bool isDisabled, bool canExpandWhenDisabled)
}
TreeViewExtensions : <<removed>>
TreeViewRow_TItem_ --> TreeViewItem_TItem_ : uses
TreeView_TItem_ --> TreeViewItem_TItem_ : manages
TreeViewRow_TItem_ --> TreeView_TItem_ : parentTreeView
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
TreeView.razor.cs, dropping theitem.CanTriggerClickNode(IsDisabled, CanExpandWhenDisabled)check inOnClickmeans node toggling no longer respects disabled state orCanExpandWhenDisabled; consider reintroducing a guard (e.g., via the newCanTriggerClickNodelogic) so click behavior matches the intended enabled/disabled semantics. - The new
GetDisabledStatusmethod encodes both disabled state andCanExpandWhenDisabledinto a single boolean, which is then negated inCanTriggerClickNode; consider either renaming it to reflect the combined semantics or inlining/clarifying the logic to make it easier to understand how disabled and expansion behavior interact.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TreeView.razor.cs`, dropping the `item.CanTriggerClickNode(IsDisabled, CanExpandWhenDisabled)` check in `OnClick` means node toggling no longer respects disabled state or `CanExpandWhenDisabled`; consider reintroducing a guard (e.g., via the new `CanTriggerClickNode` logic) so click behavior matches the intended enabled/disabled semantics.
- The new `GetDisabledStatus` method encodes both disabled state and `CanExpandWhenDisabled` into a single boolean, which is then negated in `CanTriggerClickNode`; consider either renaming it to reflect the combined semantics or inlining/clarifying the logic to make it easier to understand how disabled and expansion behavior interact.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/TreeView/TreeView.razor.cs:608` </location>
<code_context>
if (confirm)
{
_activeItem = item;
- if (ClickToggleNode && item.CanTriggerClickNode(IsDisabled, CanExpandWhenDisabled))
+ if (ClickToggleNode)
{
await OnToggleNodeAsync(item);
</code_context>
<issue_to_address>
**issue (bug_risk):** Removing the `CanTriggerClickNode` check means disabled trees/items may still toggle on click.
With this change, `OnClick` ignores `IsDisabled`, `Item.IsDisabled`, and `CanExpandWhenDisabled` and only checks `ClickToggleNode`, so disabled nodes (or a disabled tree) can still be expanded/collapsed. If disabled state is meant to prevent interaction, consider keeping a predicate here (e.g., the same one used by `TreeViewRow` for clickability) so disabled nodes don’t respond to clicks.
</issue_to_address>
### Comment 2
<location> `test/UnitTest/Components/TreeViewTest.cs:893` </location>
<code_context>
});
node = cut.Find(".node-icon");
- Assert.Contains("disabled", node.ClassList);
+ Assert.DoesNotContain("disabled", node.ClassList);
}
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests to cover all combinations of `IsDisabled`, `Item.IsDisabled`, and `CanExpandWhenDisabled` for the icon disabled CSS state.
This now covers `IsDisabled = true` and `CanExpandWhenDisabled = true`, but the new `GetDisabledStatus()` logic has additional important combinations that aren’t exercised:
- `IsDisabled = false`, `Item.IsDisabled = true`, `CanExpandWhenDisabled = true` → icon should **not** be disabled
- `IsDisabled = true`, `Item.IsDisabled = false`, `CanExpandWhenDisabled = false` → icon should be disabled
- `IsDisabled = false`, `Item.IsDisabled = true`, `CanExpandWhenDisabled = false` → icon should be disabled
- All false → icon should not be disabled
Please add tests (e.g., a `[Theory]` with `[InlineData]`) that assert the `disabled` class presence/absence on `.node-icon` for these cases to fully cover `GetDisabledStatus()` and prevent regressions.
Suggested implementation:
```csharp
pb.Add(a => a.IsDisabled, true);
});
node = cut.Find(".node-icon");
Assert.DoesNotContain("disabled", node.ClassList);
}
[Theory]
// All false → icon should not be disabled
[InlineData(false, false, false, false)]
// IsDisabled = true, Item.IsDisabled = false, CanExpandWhenDisabled = false → icon should be disabled
[InlineData(true, false, false, true)]
// IsDisabled = false, Item.IsDisabled = true, CanExpandWhenDisabled = false → icon should be disabled
[InlineData(false, true, false, true)]
// IsDisabled = false, Item.IsDisabled = true, CanExpandWhenDisabled = true → icon should not be disabled
[InlineData(false, true, true, false)]
public void NodeIcon_DisabledClass_Combinations(
bool isDisabled,
bool itemIsDisabled,
bool canExpandWhenDisabled,
bool expectedIconDisabled)
{
// Arrange
using var ctx = new TestContext();
var treeItem = new TreeItem<string>
{
Text = "root",
Value = "root",
IsDisabled = itemIsDisabled,
};
var cut = ctx.RenderComponent<TreeView<string>>(pb =>
{
pb.Add(a => a.Items, new[] { treeItem });
pb.Add(a => a.IsDisabled, isDisabled);
pb.Add(a => a.CanExpandWhenDisabled, canExpandWhenDisabled);
});
// Act
var node = cut.Find(".node-icon");
// Assert
if (expectedIconDisabled)
{
Assert.Contains("disabled", node.ClassList);
}
else
{
Assert.DoesNotContain("disabled", node.ClassList);
}
}
[Fact]
```
If the actual generic parameter type or item type in your `TreeView` tests differs (e.g., you use a custom model instead of `string` or a different `TreeItem<T>` type), adjust `TreeItem<string>` / `TreeView<string>` and the `Text`/`Value` assignments to match the existing tests in `TreeViewTest.cs`. Also ensure `TestContext`, `TreeItem<T>`, and `TreeView<T>` are already imported; if not, add the appropriate `using` statements at the top of the file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7417 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 748 748
Lines 32793 32798 +5
Branches 4551 4551
=========================================
+ Hits 32793 32798 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #7378 by making the CanExpandWhenDisabled property work correctly when the TreeView's IsDisabled property is set to true. Previously, CanExpandWhenDisabled only worked for individually disabled items but was overridden when the entire TreeView was disabled.
- Refactored disabled state logic by introducing a new
GetDisabledStatus()method in TreeViewRow that properly considersCanExpandWhenDisabledfor both tree-level and item-level disabled states - Removed the
CanTriggerClickNodeextension method as its logic has been moved into the TreeViewRow component - Updated test assertions to reflect the new expected behavior where the node-icon should not have the "disabled" class when
CanExpandWhenDisabledis true, even whenIsDisabledis true - Incremented version to 10.1.5-beta02
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTest/Components/TreeViewTest.cs | Updated test assertion to expect node-icon NOT to have "disabled" class when IsDisabled=true and CanExpandWhenDisabled=true |
| src/BootstrapBlazor/Extensions/TreeViewExtensions.cs | Removed the CanTriggerClickNode extension method as its logic was moved to TreeViewRow component |
| src/BootstrapBlazor/Components/TreeView/TreeViewRow.razor.cs | Added GetDisabledStatus() method with refactored logic that properly handles CanExpandWhenDisabled for both tree-level and item-level disabled states |
| src/BootstrapBlazor/Components/TreeView/TreeView.razor.cs | Simplified OnClick method by removing the redundant disabled state check that was previously performed by the extension method |
| src/BootstrapBlazor/BootstrapBlazor.csproj | Version bumped from 10.1.5-beta01 to 10.1.5-beta02 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Link issues
fixes #7378
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Adjust TreeView node disabled and expansion behavior to honor the CanExpandWhenDisabled setting when items or the whole tree are disabled.
New Features:
Enhancements:
Tests: