fix(Table): reset columns after toggle visible (#7180) #7186
Conversation
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
Contributor
Reviewer's GuideEnsures table column drag-and-drop handlers are correctly reset when toggling column visibility, and extends unit tests to cover toolbar, column list visibility toggling, and column drag behavior regression. Sequence diagram for resetting column drag handlers after visibility togglesequenceDiagram
actor User
participant Toolbar as ColumnToolbar
participant Table as TableGenericTItem
participant JS as TableJsModule
participant DOM as TableDom
User->>Toolbar: click toggle visibility for column
Toolbar->>Table: OnToggleColumnVisible(columnName, visible)
alt column visibility changed
Table->>Table: _resetColumns = true
end
alt AllowDragColumn and visible is true
Table->>Table: _resetColDragListener = true
end
alt ClientTableName is not empty
Table->>JS: InvokeVoidAsync saveColumnList(ClientTableName, _visibleColumns)
end
Table->>Table: OnAfterRenderAsync(firstRender)
alt _resetColumns is true
Table->>JS: InvokeVoidAsync resetColumn(Id)
Table->>Table: _resetColumns = false
end
alt _resetColDragListener is true
Table->>JS: InvokeVoidAsync resetColDragListener(Id)
Table->>Table: _resetColDragListener = false
end
JS->>DOM: resetColDragListener(id)
JS->>DOM: setDraggable(table)
loop for each draggable header cell
JS->>DOM: disposeDragColumns(col)
JS->>DOM: attach dragstart, dragenter, dragover, dragend handlers
end
Class diagram for updated Table column visibility and drag reset logicclassDiagram
class TableGenericTItem {
- bool _resetColumns
- bool _resetColDragListener
- bool AllowDragColumn
- string ClientTableName
- string Id
- IEnumerable~string~ _visibleColumns
+ Task OnToggleColumnVisible(string columnName, bool visible)
+ Task OnAfterRenderAsync(bool firstRender)
+ Task InvokeVoidAsync(string identifier, object arg1)
+ Task InvokeVoidAsync(string identifier, object arg1, object arg2)
}
class TableJsModule {
+ void resetColumn(string id)
+ void resetColDragListener(string id)
+ void bindResizeColumn(string id)
+ void setDraggable(TableDom table)
+ void disposeDragColumns(TableHeaderCell col)
}
class TableDom {
+ TableHeaderCollection tables
+ TableHeaderCollection dragColumns
}
class TableHeaderCell {
+ bool draggable
}
class TableHeaderCollection {
+ TableHeaderCell getItem(int index)
+ int length
}
TableGenericTItem --> TableJsModule : uses_JSInterop
TableJsModule --> TableDom : manipulates
TableDom o-- TableHeaderCollection : contains
TableHeaderCollection o-- TableHeaderCell : contains
TableJsModule --> TableHeaderCell : attaches_drag_handlers
File-Level Changes
Assessment against 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 there - I've reviewed your changes - here's some feedback:
- In the
AllowDragColumn_Oktest, the new Count column usesbuilder.AddAttribute(7, "Field", foo.Count);whereas the other columns pass the property name string (e.g., "Address"); consider passing the property name (e.g., "Count") instead of the runtime value to keepFieldusage consistent with the component API. - The new
_resetColDragListenerflag andresetColDragListenerJS function partially duplicate the existing column reset flow; consider whether this can be integrated with the existing_resetColumns/resetColumnbehavior to avoid having multiple separate reset paths that need to stay in sync. - In
setDraggable, you now calldisposeDragColumns(col)inside theforEachloop before registering listeners; ifdisposeDragColumnswas originally intended to work on the table or a collection, double-check that the per-column call pattern aligns with its contract and doesn’t leave any stale listeners on other elements.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `AllowDragColumn_Ok` test, the new Count column uses `builder.AddAttribute(7, "Field", foo.Count);` whereas the other columns pass the property name string (e.g., "Address"); consider passing the property name (e.g., "Count") instead of the runtime value to keep `Field` usage consistent with the component API.
- The new `_resetColDragListener` flag and `resetColDragListener` JS function partially duplicate the existing column reset flow; consider whether this can be integrated with the existing `_resetColumns`/`resetColumn` behavior to avoid having multiple separate reset paths that need to stay in sync.
- In `setDraggable`, you now call `disposeDragColumns(col)` inside the `forEach` loop before registering listeners; if `disposeDragColumns` was originally intended to work on the table or a collection, double-check that the per-column call pattern aligns with its contract and doesn’t leave any stale listeners on other elements.
## Individual Comments
### Comment 1
<location> `test/UnitTest/Components/TableTest.cs:8524-8532` </location>
<code_context>
await table.Instance.DragColumnCallback(2, 3);
});
+
+ // 更改可见列
+ var checkbox = cut.Find(".dropdown-item .form-check-input");
+
+ await cut.InvokeAsync(() => checkbox.Click());
+ await cut.InvokeAsync(() => checkbox.Click());
+
+ await cut.InvokeAsync(async () =>
+ {
+ await table.Instance.DragColumnCallback(3, 4);
+ });
}
</code_context>
<issue_to_address>
**issue (testing):** Add assertions after the final DragColumnCallback to prove the regression is fixed.
The final `DragColumnCallback(3, 4)` isn’t validated. To ensure this test actually covers #7176, please add assertions that:
* `OnDragColumnEndAsync` is invoked with the expected field name/index after toggling visibility, and/or
* the `<th>` header order remains correct after the visibility toggle and final drag.
You can reuse or add a `name` variable to capture the callback argument and re-query the header cells to verify their order after the last drag operation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Copilot finished reviewing on behalf of
ArgoZhang
November 26, 2025 08:30
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7186 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 745 745
Lines 32574 32583 +9
Branches 4513 4515 +2
=========================================
+ Hits 32574 32583 +9
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 fixes an issue where column drag listeners were not properly reset after toggling column visibility in the Table component. The fix ensures that when a column is made visible and drag functionality is enabled, the drag event listeners are reinitialized.
- Added a new
_resetColDragListenerflag to track when drag listeners need to be reset - Implemented
resetColDragListenerJavaScript function to reinitialize drag listeners - Extended test coverage to verify column toggling works correctly with drag functionality
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.cs |
Adds logic to reset column drag listeners in OnAfterRenderAsync when the _resetColDragListener flag is set |
src/BootstrapBlazor/Components/Table/Table.razor.Checkbox.cs |
Sets _resetColDragListener flag when columns are toggled visible and drag is enabled |
src/BootstrapBlazor/Components/Table/Table.razor.js |
Implements new resetColDragListener function and adds cleanup call in setDraggable to dispose old listeners before reattaching |
test/UnitTest/Components/TableTest.cs |
Extends AllowDragColumn_Ok test to verify drag functionality works after toggling column visibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (_resetColDragListener) | ||
| { | ||
| _resetColDragListener= false; |
There was a problem hiding this comment.
Missing space after assignment operator. Should be _resetColDragListener = false; instead of _resetColDragListener= false;
Suggested change
| _resetColDragListener= false; | |
| _resetColDragListener = false; |
| let index = 0 | ||
| table.dragColumns = [...table.tables[0].querySelectorAll('thead > tr > th')].filter(i => i.draggable) | ||
| table.dragColumns.forEach(col => { | ||
| disposeDragColumns(col); |
There was a problem hiding this comment.
The disposeDragColumns function expects an array of columns (as seen on line 983 where it uses (columns ?? []).forEach(...)), but here it's being called with a single column element col. This should be wrapped in an array: disposeDragColumns([col]); or the function should be refactored to handle both single elements and arrays.
Suggested change
| disposeDragColumns(col); | |
| disposeColumnDrag([col]); |
| builder.CloseComponent(); | ||
|
|
||
| builder.OpenComponent<TableColumn<Foo, int>>(6); | ||
| builder.AddAttribute(7, "Field", foo.Count); |
There was a problem hiding this comment.
The Field attribute should be set to the field name as a string "Count", not the field value foo.Count. This is inconsistent with the pattern used on lines 8491 and 8496 where field names are passed as strings ("Name", "Address").
Suggested change
| builder.AddAttribute(7, "Field", foo.Count); | |
| builder.AddAttribute(7, "Field", "Count"); |
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 #7176
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Ensure table column drag behavior is correctly reset after toggling column visibility.
Bug Fixes:
Tests: