Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjusts table header checkbox selection logic to use value equality rather than reference-based containment when clearing selected rows, with locale files touched but not functionally changed in this diff. Class diagram for updated Table OnHeaderCheck selection logicclassDiagram
class TableTItem {
List~TItem~ SelectedRows
IEnumerable~TItem~ Rows
Func~TItem, bool~ ShowRowCheckboxCallback
Task OnHeaderCheck(CheckboxState state, TItem val)
CheckboxState HeaderCheckState()
}
class CheckboxState {
<<enumeration>>
Unchecked
Checked
Indeterminate
}
TableTItem --> CheckboxState
Flow diagram for updated header checkbox selection logic in Tableflowchart TD
A[OnHeaderCheck called with state, val] --> B[Compute items = intersection of Rows and SelectedRows]
B --> C["Remove from SelectedRows all i where any item in items satisfies Equals(item, i)"]
C --> D{state == Checked?}
D -- Yes --> E{ShowRowCheckboxCallback is null?}
E -- Yes --> F[Add all Rows to SelectedRows]
E -- No --> G[Filter Rows with ShowRowCheckboxCallback and add to SelectedRows]
F --> H[End]
G --> H[End]
D -- No --> H[End]
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}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
OnHeaderCheckimplementation introduces an O(n²) pattern by callingitems.AnyinsideRemoveAll; consider materializingitemsinto aHashSet<TItem>(with the appropriate equality comparer) and usingRemoveAll(i => set.Contains(i))instead. - Since the intent is to use value equality, it may be clearer and safer to pass an explicit
IEqualityComparer<TItem>(or useEqualityComparer<TItem>.Default) rather than relying onobject.EqualsinEquals(item, i).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `OnHeaderCheck` implementation introduces an O(n²) pattern by calling `items.Any` inside `RemoveAll`; consider materializing `items` into a `HashSet<TItem>` (with the appropriate equality comparer) and using `RemoveAll(i => set.Contains(i))` instead.
- Since the intent is to use value equality, it may be clearer and safer to pass an explicit `IEqualityComparer<TItem>` (or use `EqualityComparer<TItem>.Default`) rather than relying on `object.Equals` in `Equals(item, i)`.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/Table/Table.razor.Checkbox.cs:108-109` </location>
<code_context>
protected virtual async Task OnHeaderCheck(CheckboxState state, TItem val)
{
- SelectedRows.RemoveAll(Rows.Intersect(SelectedRows).Contains);
+ var items = Rows.Intersect(SelectedRows);
+ SelectedRows.RemoveAll(i => items.Any(item => Equals(item, i)));
if (state == CheckboxState.Checked)
{
</code_context>
<issue_to_address>
**issue (bug_risk):** The new intersection logic can still enumerate `SelectedRows` while mutating it and adds an O(n²) performance cost.
Because `Intersect` is lazy, the original `RemoveAll(Rows.Intersect(SelectedRows).Contains)` could enumerate `SelectedRows` while `RemoveAll` is mutating it, and `Contains` re-enumerates the sequence each time. The new version still closes over a lazy `items` and, for each element in `SelectedRows`, calls `items.Any(...)`, which again walks `SelectedRows`. This preserves the risk of `InvalidOperationException` (modifying a collection during enumeration) and adds an O(n²) cost.
A safer and faster approach is to materialize the intersection first and then remove by direct lookup:
```csharp
var intersected = Rows.Intersect(SelectedRows).ToHashSet();
SelectedRows.RemoveAll(i => intersected.Contains(i));
```
This avoids enumerating `SelectedRows` while mutating it and reduces the removal step to roughly O(n).
</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 #7648 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 750 750
Lines 33185 33186 +1
Branches 4605 4605
=========================================
+ Hits 33185 33186 +1
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:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to adjust table header checkbox selection behavior to use the component’s equality semantics (instead of default Contains behavior), and adds a localized resource string used by the Table selection sample UI.
Changes:
- Updates
OnHeaderCheckremoval logic for selected rows in the Table checkbox behavior. - Adds
TablesSelectionCountTextlocalization entries for the Table selection sample (zh-CN/en-US).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/Table/Table.razor.Checkbox.cs | Changes how selected rows are removed when toggling the header checkbox. |
| src/BootstrapBlazor.Server/Locales/zh-CN.json | Adds zh-CN localized text for displaying selected-row count in the selection sample. |
| src/BootstrapBlazor.Server/Locales/en-US.json | Adds en-US localized text for displaying selected-row count in the selection sample. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+108
to
+109
| var items = Rows.Intersect(SelectedRows); | ||
| SelectedRows.RemoveAll(i => items.Any(item => Equals(item, i))); |
There was a problem hiding this comment.
This predicate re-enumerates a deferred Intersect sequence for every element in SelectedRows, turning the removal into O(n*m) and repeatedly iterating SelectedRows while it is being mutated by RemoveAll. Materialize the rows-to-remove once (e.g., via ToList()/HashSet with an appropriate comparer) and then perform the removals against that snapshot.
Suggested change
| var items = Rows.Intersect(SelectedRows); | |
| SelectedRows.RemoveAll(i => items.Any(item => Equals(item, i))); | |
| var items = new HashSet<TItem>(Rows.Intersect(SelectedRows)); | |
| SelectedRows.RemoveAll(i => items.Contains(i)); |
| "TablesSearchTitle": "Table Search" | ||
| }, | ||
| "BootstrapBlazor.Server.Components.Samples.Table.TablesSelection": { | ||
| "TablesSelectionCountText": "Count：{0}", |
There was a problem hiding this comment.
The English string uses a full-width colon character (：), which is inconsistent with the rest of en-US resources (e.g., "Keep State:") and may look incorrect in UI. Consider changing it to an ASCII colon and spacing (e.g., "Count: {0}").
Suggested change
| "TablesSelectionCountText": "Count：{0}", | |
| "TablesSelectionCountText": "Count: {0}", |
Comment on lines
+108
to
+109
| var items = Rows.Intersect(SelectedRows); | ||
| SelectedRows.RemoveAll(i => items.Any(item => Equals(item, i))); |
There was a problem hiding this comment.
Rows.Intersect(SelectedRows) uses EqualityComparer<TItem>.Default and will ignore the table's model equality logic (ModelEqualityComparer / CustomKeyAttribute / DynamicContext.EqualityComparer). This means some selected rows may not be removed correctly, so the change may not actually fix the reported selection mismatch.
Consider building the removal set using the table's model comparer (e.g., new ModelHashSetComparer<TItem>(this)) and then removing via HashSet.Contains, or otherwise ensure the intersection/removal uses Table.Equals(TItem,TItem) semantics throughout.
Suggested change
| var items = Rows.Intersect(SelectedRows); | |
| SelectedRows.RemoveAll(i => items.Any(item => Equals(item, i))); | |
| // Build removal set using the table's model equality comparer to ensure | |
| // that intersection logic respects custom model equality semantics. | |
| var removalSet = new HashSet<TItem>(new ModelHashSetComparer<TItem>(this)); | |
| foreach (var row in Rows) | |
| { | |
| removalSet.Add(row); | |
| } | |
| SelectedRows.RemoveAll(i => removalSet.Contains(i)); |
This was referenced Feb 25, 2026
This was referenced Mar 10, 2026
This was referenced Mar 23, 2026
This was referenced Apr 8, 2026
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
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.
Link issues
fixes #7647
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Bug Fixes: