Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsures table column drag-and-drop event handlers are properly disposed by calling the disposal helper on the full dragColumns collection instead of each individual column before event wiring. Sequence diagram for updated table drag column initializationsequenceDiagram
participant TableComponent as TableComponent
participant setDraggable as setDraggable
participant disposeDragColumns as disposeDragColumns
participant DragColumns as dragColumns
TableComponent->>setDraggable: initializeDrag(table)
setDraggable->>setDraggable: dragColumns = querySelectorAll(draggable th)
setDraggable->>disposeDragColumns: disposeDragColumns(dragColumns)
disposeDragColumns->>DragColumns: remove existing drag event handlers
disposeDragColumns-->>setDraggable: disposal complete
loop for each col in dragColumns
setDraggable->>DragColumns: EventHandler.on(col, dragstart, handler)
setDraggable->>DragColumns: EventHandler.on(col, dragover, handler)
setDraggable->>DragColumns: EventHandler.on(col, drop, handler)
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider guarding
table.tables[0]and thequerySelectorAllresult before callingdisposeDragColumns(table.dragColumns)to avoid potential null/undefined access when the table has no header ortables[0]is not present. - If
disposeDragColumnsmutates the passed array (e.g., by clearing or replacing it), it may be safer to call it with a shallow copy (e.g.,disposeDragColumns([...table.dragColumns])) to ensure the subsequentforEachstill iterates the intended columns.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider guarding `table.tables[0]` and the `querySelectorAll` result before calling `disposeDragColumns(table.dragColumns)` to avoid potential null/undefined access when the table has no header or `tables[0]` is not present.
- If `disposeDragColumns` mutates the passed array (e.g., by clearing or replacing it), it may be safer to call it with a shallow copy (e.g., `disposeDragColumns([...table.dragColumns])`) to ensure the subsequent `forEach` still iterates the intended columns.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/Table/Table.razor.js:933` </location>
<code_context>
let dragItem = null;
let index = 0
table.dragColumns = [...table.tables[0].querySelectorAll('thead > tr > th')].filter(i => i.draggable)
+ disposeDragColumns(table.dragColumns);
table.dragColumns.forEach(col => {
- disposeDragColumns(col);
</code_context>
<issue_to_address>
**issue (bug_risk):** Disposing the newly computed dragColumns likely leaves handlers on the previous set and no-ops on the current one.
Because `setDraggable` can be called repeatedly, disposing *after* reassigning `table.dragColumns` disposes the new set instead of the previous one. That leaves old columns with active drag listeners, and clears the new ones before their listeners are attached.
You likely want to dispose the previous `table.dragColumns` before recomputing it:
```js
const setDraggable = table => {
let dragItem = null;
let index = 0;
// dispose old handlers, if any
if (table.dragColumns) {
disposeDragColumns(table.dragColumns);
}
// compute new draggable columns
table.dragColumns = [...table.tables[0].querySelectorAll('thead > tr > th')]
.filter(i => i.draggable);
table.dragColumns.forEach(col => {
EventHandler.on(col, 'dragstart', ...);
// ...
});
};
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the Table component's drag column functionality. The disposeDragColumns function was being called incorrectly with individual column elements instead of the entire array of columns, which would cause improper event handler cleanup.
- Moved
disposeDragColumnscall outside the forEach loop to pass the full array of columns - Bumped version from 10.1.4-beta01 to 10.1.4-beta02
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/Table/Table.razor.js | Fixed disposeDragColumns to be called with the full column array before attaching new event handlers, rather than being called for each individual column |
| src/BootstrapBlazor/BootstrapBlazor.csproj | Version bump to 10.1.4-beta02 for the bug fix release |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7297 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 745 745
Lines 32629 32629
Branches 4520 4520
=========================================
Hits 32629 32629
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:
|
Link issues
fixes #7295
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Bug Fixes: