feat(Table): throw exception when set IsTree to true in virtualize mode #5721
Conversation
Reviewer's Guide by SourceryThis pull request renames the Sequence diagram for RefreshDataAsync call in Virtual ScrollModesequenceDiagram
participant Table
participant Virtualize
Table->>Virtualize: RefreshDataAsync()
Virtualize-->>Table: Task
Updated class diagram for the Table componentclassDiagram
class Table~TItem~ {
-Virtualize~TItem~? VirtualizeElement
+Virtualize~TItem~? _virtualizeElement
}
note for Table~TItem~ "VirtualizeElement field renamed to _virtualizeElement and made private with [NotNull] attribute"
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ArgoZhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using
nameofwhen referring to the private field_virtualizeElementin the test to avoid hardcoding the string.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| /// <summary> | ||
| /// 获得/设置 内置虚拟化组件实例 | ||
| /// </summary> | ||
| protected Virtualize<TItem>? VirtualizeElement { get; set; } |
There was a problem hiding this comment.
question (bug_risk): Using [NotNull] with conditional assignment may be misleading.
The _virtualizeElement field is marked with [NotNull] yet may remain null depending on the rendering branch. Please verify that this attribute accurately reflects its lifecycle or consider handling the nullability to avoid potential warnings or runtime issues.
| var fieldInfo = GetType().BaseType!.GetField("_virtualizeElement", BindingFlags.NonPublic | BindingFlags.Instance)!; | ||
| if (ScrollMode == ScrollMode.Virtual && fieldInfo.GetValue(this) is Virtualize<Foo> element) |
There was a problem hiding this comment.
suggestion (testing): Avoid testing private fields with reflection.
Testing private fields using reflection makes tests brittle and tightly coupled to implementation details. Consider exposing the necessary information through a protected property or method if it's essential for testing. Alternatively, refactor the test to interact with the component's public API and verify the expected behavior without relying on internal state.
Suggested implementation:
if (ScrollMode == ScrollMode.Virtual && VirtualizeElement is Virtualize<Foo> element)Ensure that the component exposes a protected property VirtualizeElement (if not already present) that returns the value of the private _virtualizeElement field. For example, in the component's base class you may add:
protected Virtualize? VirtualizeElement => _virtualizeElement;
This change avoids testing private fields via reflection and makes the test less brittle while still exposing the necessary state for verifying the component behavior.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5721 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 657 657
Lines 29830 29829 -1
Branches 4229 4229
=========================================
- Hits 29830 29829 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Link issues
fixes #5685
Summary By Copilot
This pull request includes several changes to the
Tablecomponent in theBootstrapBlazorlibrary. The main focus is on renaming theVirtualizeElementfield to_virtualizeElementand ensuring it is properly referenced throughout the codebase.Changes to
Tablecomponent:src/BootstrapBlazor/Components/Table/Table.razor: RenamedVirtualizeElementto_virtualizeElementin theVirtualizecomponent references.src/BootstrapBlazor/Components/Table/Table.razor.Edit.cs: Updated theOnClickCardViewmethod to use_virtualizeElementinstead ofVirtualizeElement.src/BootstrapBlazor/Components/Table/Table.razor.cs: Changed theVirtualizeElementfield to a private field_virtualizeElementand added the[NotNull]attribute.Test updates:
test/UnitTest/Components/TableTest.cs: Modified theTestLoopQueryAsyncmethod to reflect the field name change to_virtualizeElementand updated the reflection logic accordingly.Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Modify the Table component to improve virtualization handling and add null checking for the virtualize element
Bug Fixes:
Enhancements:
Tests: