feat(Table): redesign javascript invoke logic#7964
Open
Conversation
Contributor
Reviewer's GuideRefactors the Table component’s client-side column state management and fixed-column layout logic to use a unified TableColumnClientStatus model, simplifies column-building on render, and updates JS interop plus tests and samples accordingly. Sequence diagram for the new column resize and auto-fit JS interopsequenceDiagram
actor User
participant Browser
participant TableJs
participant BlazorTable as Table
participant AppCallback
User->>Browser: Drag column separator / click auto-fit
Browser->>TableJs: DOM event on column header
TableJs->>TableJs: Recalculate col width and table width
TableJs->>TableJs: Build TableColumnClientStatus state
TableJs->>Browser: Save state to localStorage
alt Resize column
TableJs->>BlazorTable: invokeMethodAsync(resizeColumnCallback, name, state)
BlazorTable->>BlazorTable: UpdateTableColumnState(state)
BlazorTable->>BlazorTable: UpdateTableWidth()
opt OnResizeColumnAsync is set
BlazorTable->>AppCallback: OnResizeColumnAsync(name, state)
end
else Auto-fit column width
TableJs->>BlazorTable: invokeMethodAsync(autoFitColumnWidthCallback, fieldName, state)
BlazorTable->>BlazorTable: UpdateTableColumnState(state)
BlazorTable->>BlazorTable: UpdateTableWidth()
opt OnAutoFitColumnWidthCallback is set
BlazorTable->>AppCallback: OnAutoFitColumnWidthCallback(fieldName, state)
end
end
Updated class diagram for Table column client state and callbacksclassDiagram
class Table {
List~ITableColumn~ Columns
TableColumnClientStatus _tableColumnStateCache
List~TableColumnState~ _columnVisibleItems
BreakPoint _screenSize
string ClientTableName
List~ITableColumn~ GetTableColumns()
Task TriggerColumnCreating(List~ITableColumn~ cols)
Task BuildTableColumns()
void RebuildTableColumns()
Task ReloadColumnStatesFromBrowserAsync()
void RebuildTableColumnFromCache()
List~TableColumnState~ GetColumnVisibleItems(List~ITableColumn~ cols)
Task OnTableRenderAsync(bool firstRender)
void RebuildVisibleColumnsCache()
Task ResizeColumnCallback(string name, TableColumnClientStatus columnState)
Task AutoFitColumnWidthCallback(string fieldName, TableColumnClientStatus columnState)
void UpdateTableColumnState(TableColumnClientStatus columnState)
void UpdateTableWidth()
void ResetVisibleColumns(IEnumerable~TableColumnState~ columns)
List~ITableColumn~ GetVisibleColumns()
}
class TableColumnClientStatus {
List~TableColumnState~ Columns
int TableWidth
}
class TableColumnState {
string Name
bool Visible
string DisplayName
int Width
}
class ITable {
<<interface>>
List~ITableColumn~ GetVisibleColumns()
}
class ITableColumn {
<<interface>>
bool Fixed
bool DefaultSort
bool Sortable
bool Visible
int Width
BreakPoint ShownWithBreakPoint
string GetFieldName()
string GetDisplayName()
bool GetVisible()
bool GetIgnore()
}
Table ..|> ITable
Table "1" o-- "*" ITableColumn
Table "1" o-- "1" TableColumnClientStatus
TableColumnClientStatus "1" o-- "*" TableColumnState
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 3 issues, and left some high level feedback:
- In
ResetVisibleColumnsyou updateColumnsbut never propagate the new visibility/width values into_tableColumnStateCache.Columns, so subsequent JS interop and persistence can operate on stale state; consider updating_tableColumnStateCachethere to keep server and client column state in sync. - The change from
IsTailtoIsFixRightalters the semantics of right-fixed detection to!columns.Take(index).All(i => i.Fixed), which will classify any fixed column that has a non-fixed column before it as right-fixed; please double-check this logic against the intended UX for mixed fixed/non‑fixed columns, as it may misclassify left vs right fixed positions. - The removal of
@key="col"on the<DynamicElement TagName="th" ...>inTable.razorcan affect diffing and drag/reorder behavior of header cells; if the key is still needed to stabilize Blazor’s DOM updates for draggable columns, consider keeping a stable key (e.g., field name) on those elements.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ResetVisibleColumns` you update `Columns` but never propagate the new visibility/width values into `_tableColumnStateCache.Columns`, so subsequent JS interop and persistence can operate on stale state; consider updating `_tableColumnStateCache` there to keep server and client column state in sync.
- The change from `IsTail` to `IsFixRight` alters the semantics of right-fixed detection to `!columns.Take(index).All(i => i.Fixed)`, which will classify any fixed column that has a non-fixed column before it as right-fixed; please double-check this logic against the intended UX for mixed fixed/non‑fixed columns, as it may misclassify left vs right fixed positions.
- The removal of `@key="col"` on the `<DynamicElement TagName="th" ...>` in `Table.razor` can affect diffing and drag/reorder behavior of header cells; if the key is still needed to stabilize Blazor’s DOM updates for draggable columns, consider keeping a stable key (e.g., field name) on those elements.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/Components/Table/Table.razor.js" line_range="859-863" />
<code_context>
const columnWidthState = getColumnWidthState(tableName);
if (columnWidthState) {
+ removeColumnVisibleState(tableName);
+
const columnVisibleStates = getColumnVisibleState(tableName);
if (columnVisibleStates) {
- //removeColumnVisibleState(tableName);
+ removeColumnWidthState(tableName);
- //removeColumnWidthState(tableName);
</code_context>
<issue_to_address>
**issue (bug_risk):** LocalStorage column visibility state is cleared before it is read, which likely breaks state merging.
Because `removeColumnVisibleState(tableName)` is called before `getColumnVisibleState(tableName)`, `columnVisibleStates` will always be `null`. As a result, width and visibility state are never merged and `removeColumnWidthState(tableName)` is effectively dead code. This seems reversed from the previous (commented) logic; you likely want to first read both states, then merge, then remove the old keys if needed.
</issue_to_address>
### Comment 2
<location path="src/BootstrapBlazor/Components/Table/Table.razor" line_range="306-308" />
<code_context>
{
var fieldName = col.GetFieldName();
var displayName = col.GetDisplayName();
- <DynamicElement TagName="th" @key="col" class="@GetHeaderClassString(col)"
+ <DynamicElement TagName="th" class="@GetHeaderClassString(col)"
style="@GetFixedCellStyleString(col, ActualScrollWidth)"
TriggerClick="col.GetSortable()" OnClick="@OnClickHeader(col)"
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Removing the @key from header cells can lead to Blazor diffing issues when columns change.
Without `@key="col"`, these header cells depend on Blazor’s default diffing. With dynamic column order, visibility toggling, or drag-and-drop, Blazor may incorrectly reuse DOM nodes (e.g., headers or sort icons attached to the wrong column). Unless the key was causing a specific problem, keeping a stable `@key` based on the column identity (e.g., field name) is safer to ensure correct reconciliation.
</issue_to_address>
### Comment 3
<location path="src/BootstrapBlazor/Components/Table/Table.razor.cs" line_range="1310" />
<code_context>
+ }
+ }
+
+ private async Task BuildTableColumns()
+ {
+ // 构建列信息
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the column-building and client-state application logic into shared helper methods to avoid duplicated responsibilities and make future changes easier to maintain.
You can reduce the new complexity without changing behavior by tightening up the column-build and state-apply pipeline.
### 1. Unify `BuildTableColumns` and `RebuildTableColumns`
Both methods now do the same work except for the async `OnColumnCreating` callback. You can push all shared logic into one core method and keep thin wrappers, so future changes only need to touch one place.
```csharp
private void ApplyColumns(List<ITableColumn> cols, bool applyClientState = true)
{
Columns.Clear();
Columns.AddRange(cols.OrderFunc());
// set default sortName
var col = Columns.Find(i => i is { Sortable: true, DefaultSort: true });
if (col != null)
{
SortName = col.GetFieldName();
SortOrder = col.DefaultSortOrder;
}
if (applyClientState)
{
RebuildTableColumnFromCache();
}
}
private async Task BuildTableColumns()
{
var cols = GetTableColumns();
// 触发列创建事件
if (OnColumnCreating != null)
{
await OnColumnCreating(cols);
}
ApplyColumns(cols, applyClientState: true);
}
private void RebuildTableColumns()
{
var cols = GetTableColumns();
ApplyColumns(cols, applyClientState: true);
}
```
If you ever need a path that doesn’t apply client state, you can reuse `ApplyColumns(cols, applyClientState: false)` instead of adding another method.
### 2. Centralize “apply client state + rebuild visible cache”
Right now multiple methods mutate related structures:
- `RebuildTableColumnFromCache`
- `ResetVisibleColumns`
- `UpdateTableColumnState`
- `_tableColumnStateCache.TableWidth` is updated in `UpdateTableColumnState` but width adjustment logic is in `UpdateTableWidth`, which isn’t used.
You can treat “apply client state + rebuild visible items” as a single responsibility and make one method the source of truth, then call it from the different entry points.
For example:
```csharp
private void ApplyClientStateAndRebuild()
{
// apply column states to Columns
if (_tableColumnStateCache.Columns.Count != 0)
{
foreach (var col in Columns)
{
var fieldName = col.GetFieldName();
var column = _tableColumnStateCache.Columns.Find(i => i.Name == fieldName);
if (column != null)
{
col.Width = column.Width;
col.Visible = column.Visible;
column.DisplayName = col.GetDisplayName();
}
}
}
// rebuild visible items
_columnVisibleItems.Clear();
_columnVisibleItems.AddRange(GetColumnVisibleItems(Columns));
RebuildVisibleColumnsCache();
// keep table width logic in one place
UpdateTableWidth();
}
private void RebuildTableColumnFromCache()
{
ApplyClientStateAndRebuild();
}
```
Then:
- `BuildTableColumns` / `RebuildTableColumns` call `ApplyClientStateAndRebuild` via `RebuildTableColumnFromCache`.
- `ResetVisibleColumns` can just update `Columns` and call `ApplyClientStateAndRebuild()` instead of duplicating part of the responsibility:
```csharp
public void ResetVisibleColumns(IEnumerable<TableColumnState> columns)
{
foreach (var col in columns)
{
var column = Columns.Find(i => i.GetFieldName() == col.Name);
if (column != null)
{
column.Visible = col.Visible;
column.Width = col.Width;
}
}
ApplyClientStateAndRebuild();
_resetColumns = true;
_invoke = true;
StateHasChanged();
}
```
This keeps all “Columns ↔ client-state ↔ visible cache” synchronization in one small, well-defined place, while still preserving your new behaviors (callbacks, client width, etc.) and avoiding further fragmentation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| } | ||
|
|
||
| private async Task BuildTableColumns() |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider refactoring the column-building and client-state application logic into shared helper methods to avoid duplicated responsibilities and make future changes easier to maintain.
You can reduce the new complexity without changing behavior by tightening up the column-build and state-apply pipeline.
1. Unify BuildTableColumns and RebuildTableColumns
Both methods now do the same work except for the async OnColumnCreating callback. You can push all shared logic into one core method and keep thin wrappers, so future changes only need to touch one place.
private void ApplyColumns(List<ITableColumn> cols, bool applyClientState = true)
{
Columns.Clear();
Columns.AddRange(cols.OrderFunc());
// set default sortName
var col = Columns.Find(i => i is { Sortable: true, DefaultSort: true });
if (col != null)
{
SortName = col.GetFieldName();
SortOrder = col.DefaultSortOrder;
}
if (applyClientState)
{
RebuildTableColumnFromCache();
}
}
private async Task BuildTableColumns()
{
var cols = GetTableColumns();
// 触发列创建事件
if (OnColumnCreating != null)
{
await OnColumnCreating(cols);
}
ApplyColumns(cols, applyClientState: true);
}
private void RebuildTableColumns()
{
var cols = GetTableColumns();
ApplyColumns(cols, applyClientState: true);
}If you ever need a path that doesn’t apply client state, you can reuse ApplyColumns(cols, applyClientState: false) instead of adding another method.
2. Centralize “apply client state + rebuild visible cache”
Right now multiple methods mutate related structures:
RebuildTableColumnFromCacheResetVisibleColumnsUpdateTableColumnState_tableColumnStateCache.TableWidthis updated inUpdateTableColumnStatebut width adjustment logic is inUpdateTableWidth, which isn’t used.
You can treat “apply client state + rebuild visible items” as a single responsibility and make one method the source of truth, then call it from the different entry points.
For example:
private void ApplyClientStateAndRebuild()
{
// apply column states to Columns
if (_tableColumnStateCache.Columns.Count != 0)
{
foreach (var col in Columns)
{
var fieldName = col.GetFieldName();
var column = _tableColumnStateCache.Columns.Find(i => i.Name == fieldName);
if (column != null)
{
col.Width = column.Width;
col.Visible = column.Visible;
column.DisplayName = col.GetDisplayName();
}
}
}
// rebuild visible items
_columnVisibleItems.Clear();
_columnVisibleItems.AddRange(GetColumnVisibleItems(Columns));
RebuildVisibleColumnsCache();
// keep table width logic in one place
UpdateTableWidth();
}
private void RebuildTableColumnFromCache()
{
ApplyClientStateAndRebuild();
}Then:
BuildTableColumns/RebuildTableColumnscallApplyClientStateAndRebuildviaRebuildTableColumnFromCache.ResetVisibleColumnscan just updateColumnsand callApplyClientStateAndRebuild()instead of duplicating part of the responsibility:
public void ResetVisibleColumns(IEnumerable<TableColumnState> columns)
{
foreach (var col in columns)
{
var column = Columns.Find(i => i.GetFieldName() == col.Name);
if (column != null)
{
column.Visible = col.Visible;
column.Width = col.Width;
}
}
ApplyClientStateAndRebuild();
_resetColumns = true;
_invoke = true;
StateHasChanged();
}This keeps all “Columns ↔ client-state ↔ visible cache” synchronization in one small, well-defined place, while still preserving your new behaviors (callbacks, client width, etc.) and avoiding further fragmentation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7964 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 766 765 -1
Lines 34113 34105 -8
Branches 4696 4682 -14
=========================================
- Hits 34113 34105 -8
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 refactors the Table component’s JavaScript interop and client-side persistence model by introducing a new client state payload (TableColumnClientStatus) and updating the .NET/JS callback contracts accordingly, with accompanying updates to fixed-column calculations, unit tests, and a server demo.
Changes:
- Replace
JsonElement-based JS callback payloads with a strongly-typedTableColumnClientStatusand update resize/auto-fit callback flows. - Update fixed-column positioning logic and visible-column access patterns (including
ITable.GetVisibleColumns()contract changes). - Update unit tests and samples to align with the new persistence and callback behavior.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/UnitTest/Dynamic/ChangeDetectionCleanTaskTest.cs | Update mock ITable.GetVisibleColumns() signature to match new API. |
| test/UnitTest/Converters/JsonDescriptionEnumConverterTest.cs | Remove a converter test that no longer matches the updated column-state model. |
| test/UnitTest/Converters/ColumnVisibleItemConverterTest.cs | Remove obsolete converter test file for the prior column visible item model. |
| test/UnitTest/Components/TableTest.cs | Adapt tests to new client state object and updated JS callback signatures; add new fixed-column scenarios. |
| test/UnitTest/Components/TableNumberFilterTest.cs | Update mock GetVisibleColumns() return type. |
| test/UnitTest/Components/TableNotSupportFilterTest.cs | Update mock GetVisibleColumns() return type. |
| test/UnitTest/Components/TableMultiSelectFilterTest.cs | Update mock GetVisibleColumns() return type. |
| test/UnitTest/Components/TableMultiFilterTest.cs | Update mock GetVisibleColumns() return type. |
| test/UnitTest/Components/TableLookupFilterTest.cs | Update mock GetVisibleColumns() return type. |
| test/UnitTest/Components/TableEnumFilterTest.cs | Update mock GetVisibleColumns() return type. |
| test/UnitTest/Components/TableDateTimeFilterTest.cs | Update mock GetVisibleColumns() return type. |
| test/UnitTest/Components/TableColumnFilterTest.cs | Update mock GetVisibleColumns() return type. |
| test/UnitTest/Components/TableBoolFilterTest.cs | Update mock GetVisibleColumns() return type. |
| src/BootstrapBlazor/Extensions/JsonElementExtesions.cs | Remove the JsonElement parsing helper now that typed payloads are used. |
| src/BootstrapBlazor/Components/Table/TableColumnClientStatus.cs | Add strongly-typed client state container for persisted column/table state. |
| src/BootstrapBlazor/Components/Table/Table.razor.Toolbar.cs | Change GetVisibleColumns() to return List<ITableColumn>. |
| src/BootstrapBlazor/Components/Table/Table.razor.Sort.cs | Rework “fixed-right” detection and fixed-position style calculations. |
| src/BootstrapBlazor/Components/Table/Table.razor.js | Update persistence keys/state shape and invoke updated .NET callbacks (resize/auto-fit). |
| src/BootstrapBlazor/Components/Table/Table.razor.cs | Refactor column building; replace JSInvokable payloads with TableColumnClientStatus; update public callback parameter types. |
| src/BootstrapBlazor/Components/Table/Table.razor.Checkbox.cs | Update width recomputation and table-width bookkeeping after visibility changes. |
| src/BootstrapBlazor/Components/Table/Table.razor | Adjust render flow; remove @key from header th dynamic element. |
| src/BootstrapBlazor/Components/Table/ITable.cs | Change GetVisibleColumns() return type to List<ITableColumn>. |
| src/BootstrapBlazor.Server/Components/Samples/Table/TablesColumnResizing.razor | Update demo to exercise new toolbar/column state behaviors. |
Comments suppressed due to low confidence (2)
src/BootstrapBlazor/Components/Table/Table.razor.js:875
- In
getColumnStates,removeColumnVisibleState(tableName)is called beforegetColumnVisibleState(tableName), which guaranteescolumnVisibleStateswill be null and prevents the intended migration/merge from running. Read the old visible-state first, merge intocolumnWidthState, then remove the legacy keys only after a successful migration.
const columnWidthState = getColumnWidthState(tableName);
if (columnWidthState) {
removeColumnVisibleState(tableName);
const columnVisibleStates = getColumnVisibleState(tableName);
if (columnVisibleStates) {
removeColumnWidthState(tableName);
for (const item of columnWidthState.cols) {
const { name } = item;
const column = columnVisibleStates.find(i => i.name === name);
if (column) {
item.visible = column.visible;
}
else {
item.visible = true;
}
}
}
src/BootstrapBlazor/Components/Table/Table.razor:312
- The header
<DynamicElement TagName="th">lost its@key. Since the table supports dynamic column changes (visibility toggles, drag-reorder, etc.), removing the key can cause Blazor to reuse/mismatch header DOM nodes across renders, leading to incorrect state/event wiring. Reintroduce a stable key (e.g., field name) for each header cell.
@foreach (var col in GetVisibleColumns())
{
var fieldName = col.GetFieldName();
var displayName = col.GetDisplayName();
<DynamicElement TagName="th" class="@GetHeaderClassString(col)"
style="@GetFixedCellStyleString(col, ActualScrollWidth)"
TriggerClick="col.GetSortable()" OnClick="@OnClickHeader(col)"
draggable="@DraggableString">
<div class="@GetHeaderWrapperClassString(col)">
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| [Fact] | ||
| private async Task UpdateTableState_Ok() |
Comment on lines
881
to
924
| @@ -919,7 +919,7 @@ const getLocalStorageValue = key => { | |||
| const saveColumnStateToLocalstorage = (table, state) => { | |||
| const { options: { tableName } } = table; | |||
| if (tableName) { | |||
| const columnStateKey = `bb-table-column-${tableName}`; | |||
| const columnStateKey = `bb-table-${tableName}`; | |||
| const columnState = state ?? getColumnStateObject(table); | |||
| localStorage.setItem(columnStateKey, JSON.stringify(columnState)); | |||
| /// <para lang="en">Gets visible columns collection configured in ITable instance</para> | ||
| /// </summary> | ||
| IEnumerable<ITableColumn> GetVisibleColumns(); | ||
| List<ITableColumn> GetVisibleColumns(); |
Comment on lines
1486
to
+1501
| /// <param name="columns"></param> | ||
| public void ResetVisibleColumns(IEnumerable<TableColumnState> columns) | ||
| { | ||
| // https://github.com/dotnetcore/BootstrapBlazor/issues/6823 | ||
| foreach (var col in columns) | ||
| { | ||
| // 使用 for + break 性能更好 | ||
| for (var index = 0; index < _columnVisibleItems.Count; index++) | ||
| var column = Columns.Find(i => i.GetFieldName() == col.Name); | ||
| if (column != null) | ||
| { | ||
| var item = _columnVisibleItems[index]; | ||
| if (item.Name == col.Name) | ||
| { | ||
| item.Visible = col.Visible; | ||
| break; | ||
| } | ||
| column.Visible = col.Visible; | ||
| column.Width = col.Width; | ||
| } | ||
| } | ||
|
|
||
| // 重置可见缓存 | ||
| RebuildTableColumnFromCache(); | ||
|
|
Comment on lines
1853
to
1867
| /// <summary> | ||
| /// <para lang="zh">获得/设置 设置列宽回调方法</para> | ||
| /// <para lang="en">Gets or sets Resize Column Callback</para> | ||
| /// </summary> | ||
| [Parameter] | ||
| public Func<string, int?, Task>? OnResizeColumnAsync { get; set; } | ||
| public Func<string, TableColumnClientStatus, Task>? OnResizeColumnAsync { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// <para lang="zh">获得/设置 自动调整列宽回调方法</para> | ||
| /// <para lang="en">Gets or sets Auto Fit Column Width Callback</para> | ||
| /// | ||
| /// </summary> | ||
| [Parameter] | ||
| public Func<string, int, Task<int>>? OnAutoFitColumnWidthCallback { get; set; } | ||
| public Func<string, TableColumnClientStatus, Task>? OnAutoFitColumnWidthCallback { get; set; } | ||
|
|
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 #7963
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Refactor the table component’s client-side state management and fixed-column layout logic to use a unified column state model and streamlined JS interop.
Enhancements:
Documentation:
Tests: