refactor(Table): updating column state persistence#7954
Merged
Conversation
Contributor
Reviewer's GuideRefactors the Blazor Table component’s column visibility/width persistence and drag/resize behavior, centralizing column list rendering, simplifying visibility state, and making the JS width handling more robust and consistent while wiring it tightly to client-side local storage. Sequence diagram for column drag and visibility persistencesequenceDiagram
actor User
participant Browser as Browser_Table_UI
participant JS as Table_razor_js
participant DotNet as Blazor_Table_component
participant LS as LocalStorage
User->>Browser: Drag column header
Browser->>JS: dragstart / dragover / drop events
JS->>JS: setDraggable registers drop handler
JS->>DotNet: invokeMethodAsync(dragColumnCallback, originIndex, currentIndex)
DotNet->>DotNet: DragColumnCallback(originIndex, currentIndex)
DotNet->>DotNet: Reorder _columnVisibleItems
alt ClientTableName is set
DotNet->>DotNet: Update _tableColumnStateCache.ColumnVisibleStates
end
DotNet->>DotNet: _resetColumns = true, _invoke = true
DotNet->>DotNet: StateHasChanged()
Note over DotNet,JS: Table rerenders with new column order
JS->>JS: resetColumns(table, options)
alt tableName and visibleColumns provided
JS->>LS: saveColumnVisibleList(tableName, visibleColumns)
end
Sequence diagram for column resize and width persistencesequenceDiagram
actor User
participant Browser as Browser_Table_UI
participant JS as Table_razor_js
participant DotNet as Blazor_Table_component
participant LS as LocalStorage
User->>Browser: Drag column resize handle
Browser->>JS: mousedown / mousemove / mouseup
JS->>JS: setResizeListener / setColumnResizingListen
JS->>JS: getWidth(th), getWidth(table)
JS->>DotNet: invokeMethodAsync(resizeColumnCallback, fieldOrIndex, columnWidth, tableWidth)
DotNet->>DotNet: ResizeColumnCallback(name, columnWidth, tableWidth)
alt ClientTableName is set
DotNet->>DotNet: Find ColumnWidthItem by Name
DotNet->>DotNet: Update Width and TableWidth in _tableColumnStateCache.ColumnWidthState
end
DotNet->>DotNet: Invoke resize callbacks (if any)
JS->>JS: saveColumnWidth(table)
JS->>LS: setItem(bb-table-column-width-*, widthState)
Class diagram for updated Table column state managementclassDiagram
class Table~TItem~ {
-string ClientTableName
-TableColumnLocalstorageStatus _tableColumnStateCache
-List~ColumnVisibleItem~ _columnVisibleItems
+Task OnAfterRenderAsync(bool firstRender)
-Task ReloadColumnStatesFromBrowserAsync()
-void BuildTableColumns()
-void RebuildTableColumnFromCache(List~ITableColumn~ cols)
-List~ColumnVisibleItem~ GetColumnVisibleItems(List~ITableColumn~ cols)
-Task OnTableRenderAsync(bool firstRender)
+Task DragColumnCallback(int originIndex, int currentIndex)
+Task ResizeColumnCallback(string name, int columnWidth, int tableWidth)
-RenderFragment RenderColumnList()
-void RebuildVisibleColumnsCache()
}
class ColumnVisibleItem {
+string Name
+string DisplayName
+bool Visible
}
class TableColumnLocalstorageStatus {
+ColumnWidthState ColumnWidthState
+List~ColumnVisibleItem~ ColumnVisibleStates
}
class ColumnWidthState {
+List~ColumnWidthItem~ ColumnWidths
+int TableWidth
}
class ColumnWidthItem {
+string Name
+int Width
}
class ITableColumn {
<<interface>>
+string GetFieldName()
+string GetDisplayName()
+bool GetIgnore()
+bool GetVisible()
+bool? Visible
+int? Width
}
Table~TItem~ --> TableColumnLocalstorageStatus : uses
TableColumnLocalstorageStatus --> ColumnWidthState : has
ColumnWidthState --> ColumnWidthItem : contains
Table~TItem~ --> ColumnVisibleItem : maintains
Table~TItem~ --> ITableColumn : builds from
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
ArgoZhang
changed the title
![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 2 issues, and left some high level feedback:
- In
RebuildTableColumnFromCache/GetColumnVisibleItems,_tableColumnStateCacheis accessed without a null check; ifClientTableNameis empty or no state is loaded from local storage, this can throw, so consider initializing the cache to a non-null default or guarding those accesses. - In
resetTableWidth(Table.razor.js), theindexconstant computed inside thegroup.childrenloop is never used and can be removed to avoid unnecessary work.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `RebuildTableColumnFromCache`/`GetColumnVisibleItems`, `_tableColumnStateCache` is accessed without a null check; if `ClientTableName` is empty or no state is loaded from local storage, this can throw, so consider initializing the cache to a non-null default or guarding those accesses.
- In `resetTableWidth` (Table.razor.js), the `index` constant computed inside the `group.children` loop is never used and can be removed to avoid unnecessary work.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/Components/Table/Table.razor.js" line_range="684" />
<code_context>
setTableDefaultWidth(table);
const widthState = getColumnWidthStateObject(table);
- await table.invoke.invokeMethodAsync(table.options.resizeColumnCallback, index, maxWidth | 0, widthState)
+ await table.invoke.invokeMethodAsync(table.options.resizeColumnCallback, index, maxWidth, widthState)
resetColumnWidthTips(table, col);
</code_context>
<issue_to_address>
**issue (bug_risk):** Using the column index instead of the field name for `resizeColumnCallback` may not match the C# callback signature.
The C# callback is `ResizeColumnCallback(string name, int columnWidth, int tableWidth)`, but here the first arg is the column index and the third is a width state object. If this is meant to call the same .NET handler used in `setResizeListener`, these mismatched argument types/semantics could cause incorrect column resolution or binding issues. Please either align the JS arguments with the C# signature (field name + numeric table width) or call a separate .NET handler that accepts index + width state.
</issue_to_address>
### Comment 2
<location path="src/BootstrapBlazor/Components/Table/Table.razor.js" line_range="423-425" />
<code_context>
let width = 0;
[...group.children].forEach(col => {
let colWidth = parseInt(col.style.width);
+ const index = [...group.children].indexOf(col);
if (isNaN(colWidth)) {
- colWidth = 100;
</code_context>
<issue_to_address>
**nitpick (performance):** Unused `index` variable and repeated array materialization in `resetTableWidth`.
`index` is unused and `[...group.children]` is created twice here (for `forEach` and `indexOf`), which is unnecessary work for large tables. Either drop `index` or materialize `children` once and reuse it (e.g., `const children = [...group.children]; children.forEach((col, index) => { ... });`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| setTableDefaultWidth(table); | ||
| const widthState = getColumnWidthStateObject(table); | ||
| await table.invoke.invokeMethodAsync(table.options.resizeColumnCallback, index, maxWidth | 0, widthState) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Using the column index instead of the field name for resizeColumnCallback may not match the C# callback signature.
The C# callback is ResizeColumnCallback(string name, int columnWidth, int tableWidth), but here the first arg is the column index and the third is a width state object. If this is meant to call the same .NET handler used in setResizeListener, these mismatched argument types/semantics could cause incorrect column resolution or binding issues. Please either align the JS arguments with the C# signature (field name + numeric table width) or call a separate .NET handler that accepts index + width state.
Comment on lines
423
to
+425
| [...group.children].forEach(col => { | ||
| let colWidth = parseInt(col.style.width); | ||
| const index = [...group.children].indexOf(col); |
Contributor
There was a problem hiding this comment.
nitpick (performance): Unused index variable and repeated array materialization in resetTableWidth.
index is unused and [...group.children] is created twice here (for forEach and indexOf), which is unnecessary work for large tables. Either drop index or materialize children once and reuse it (e.g., const children = [...group.children]; children.forEach((col, index) => { ... });).
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Table component’s client-side behavior and column-state persistence, aiming to “redesign table javascript” (issue #7953) while also simplifying column visibility ordering by removing the ColumnVisibleItem.Order concept.
Changes:
- Refactors table column persistence (visibility/width) handling between .NET and JavaScript, including renaming the JS localStorage helper for visible columns.
- Updates column list rendering and visible-column cache rebuilding to rely on
_columnVisibleItemsordering directly (removing theOrderfield). - Adjusts resize/drag JavaScript logic to use
getWidth(...)rather thanoffsetWidth | 0in multiple places.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/Table/Table.razor.Toolbar.cs | Updates visible-columns cache rebuild to use _columnVisibleItems order directly. |
| src/BootstrapBlazor/Components/Table/Table.razor.js | Refactors resize/drag/width persistence logic; renames visible-column persistence helper. |
| src/BootstrapBlazor/Components/Table/Table.razor.cs | Refactors column-state reload and rebuild logic; changes how _columnVisibleItems is rebuilt from cache. |
| src/BootstrapBlazor/Components/Table/Table.razor.Checkbox.cs | Adds safeguard intent comment for “invert selection” column visibility behavior. |
| src/BootstrapBlazor/Components/Table/Table.razor | Moves BuildTableColumns() / RebuildVisibleColumnsCache() to an earlier render location; uses a RenderFragment for column list rendering. |
| src/BootstrapBlazor/Components/Table/ColumnVisibleItem.cs | Removes Order property from persisted column visibility model. |
| src/BootstrapBlazor/_Imports.razor | Moves Virtualization using directive to shared imports. |
Comments suppressed due to low confidence (1)
src/BootstrapBlazor/Components/Table/Table.razor.Checkbox.cs:203
TriggerSelectInvertColumnListcan throw when_columnVisibleItemsis empty:All(...)will return true for an empty sequence, then_columnVisibleItems.First()throws. Add a guard (e.g., checkCount > 0) before forcing the first column visible.
// 如果全部列都不可见了，则至少显示第一列
if (_columnVisibleItems.All(i => i.Visible == false))
{
_columnVisibleItems.First().Visible = true;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
422
to
428
| let width = 0; | ||
| [...group.children].forEach(col => { | ||
| let colWidth = parseInt(col.style.width); | ||
| const index = [...group.children].indexOf(col); | ||
| if (isNaN(colWidth)) { | ||
| colWidth = 100; | ||
| colWidth = getWidth(col); | ||
| } |
Comment on lines
573
to
577
| const field = col.getAttribute('data-bb-field'); | ||
| const th = col.closest('th') | ||
| const width = getWidth(th) | 0; | ||
| const tableWidth = table.tables[0].offsetWidth | 0; | ||
| const width = getWidth(th); | ||
| const tableWidth = getWidth(table.tables[0]); | ||
| table.invoke.invokeMethodAsync(table.options.resizeColumnCallback, field, width, tableWidth); |
Comment on lines
930
to
936
| const getColumnWidthStateObject = table => { | ||
| const cols = table.columns | ||
| const tableWidth = table.tables[0].offsetWidth | 0; | ||
| const tableWidth = getWidth(table.tables[0]); | ||
| return { | ||
| "cols": cols.map(col => { | ||
| return { "width": col.closest('th').offsetWidth | 0, "name": col.getAttribute('data-bb-field') } | ||
| return { "width": getWidth(col.closest('th')), "name": col.getAttribute('data-bb-field') } | ||
| }), |
Comment on lines
682
to
685
| setTableDefaultWidth(table); | ||
| const widthState = getColumnWidthStateObject(table); | ||
| await table.invoke.invokeMethodAsync(table.options.resizeColumnCallback, index, maxWidth | 0, widthState) | ||
| await table.invoke.invokeMethodAsync(table.options.resizeColumnCallback, index, maxWidth, widthState) | ||
|
|
Comment on lines
+830
to
+832
| const orginIndex = index; | ||
| const currentIndex = table.dragColumns.indexOf(col); | ||
| table.invoke.invokeMethodAsync(table.options.dragColumnCallback, orginIndex, currentIndex); |
Comment on lines
+1370
to
+1378
| // 开启客户端持久化后未设置列状态的列默认使用组件参数值 | ||
| return _tableColumnStateCache.ColumnVisibleStates.Count != 0 | ||
| ? _tableColumnStateCache.ColumnVisibleStates | ||
| : [.. cols.Where(i => !i.GetIgnore()).Select(i => new ColumnVisibleItem() | ||
| { | ||
| Name = i.GetFieldName(), | ||
| Visible = i.GetVisible(), | ||
| DisplayName = i.GetDisplayName() | ||
| })]; |
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 #7953
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Refine table column persistence, sizing, and drag behavior, and streamline how visible columns are built and rendered.
Bug Fixes:
Enhancements: