Conversation
# Conflicts: # src/BootstrapBlazor/BootstrapBlazor.csproj # src/BootstrapBlazor/Dynamic/DataTableDynamicContext.cs # src/BootstrapBlazor/Dynamic/DynamicObject.cs
Reviewer's GuideRefactors the DataTable-based dynamic table pipeline to avoid runtime type emission, operate directly on DataRow-backed dynamic objects, and streamline value propagation and editing workflows for better performance and consistency. Sequence diagram for dynamic DataTable cell edit flowsequenceDiagram
actor User
participant Table as Table
participant Validate as ValidateBase_T
participant Utility as Utility
participant DynObj as DataTableDynamicObject
participant Context as DataTableDynamicContext
participant Row as DataRow
User->>Table: Edit cell in dynamic DataTable row
Table->>Validate: Render editor with model as IDynamicObject
Note over Validate,Utility: For IDynamicObject model, use OnValueChanged instead of ValueChanged
Validate->>Utility: GenerateOnValueChanged(model, fieldName, fieldType)
Utility-->>Validate: Func newValue => DynObj.SetValue(fieldName, newValue)
User->>Validate: Change field value
Validate->>Validate: Validate and update Value
Validate->>DynObj: OnValueChanged(newValue)
DynObj->>Row: SetValue(propertyName, newValue)
Note over DynObj,Row: DataRow updated directly, no emitted runtime type
User->>Table: Save edit in dialog
Table->>DynObj: Accept()
DynObj->>Row: Table.AcceptChanges()
User->>Table: Cancel edit in dialog (without save)
Table->>DynObj: Cancel()
DynObj->>Row: Table.RejectChanges()
Sequence diagram for dynamic DataTable add and delete operationssequenceDiagram
actor User
participant Table as Table
participant Context as DataTableDynamicContext
participant DynObj as DataTableDynamicObject
participant DT as DataTable
participant Row as DataRow
User->>Table: Click Add
Table->>Context: AddAsync(SelectedRows as IDynamicObject)
alt OnAddAsync provided
Context->>Context: OnAddAsync callback
else default add
Context->>DT: NewRow()
DT-->>Context: Row
Context->>DT: Rows.InsertAt(row, index)
Context->>DT: AcceptChanges()
Context->>DynObj: new DataTableDynamicObject(row)
Context->>DynObj: set DynamicObjectPrimaryKey
Context->>Context: _dataCache.Add(primaryKey, DynObj)
Context->>Table: OnChanged(DynamicObjectContextArgs with DynObj)
Table->>Table: QueryAsync(false) to refresh
end
User->>Table: Click Delete
Table->>Context: DeleteAsync(SelectedRows as IDynamicObject)
alt OnDeleteAsync provided
Context->>Context: OnDeleteAsync(items)
Context->>Context: _items = null
else default delete
loop for each item
Context->>Context: _dataCache.TryRemove(primaryKey)
Context->>DT: Rows.Remove(row)
opt _items not null
Context->>Context: remove from _items by primaryKey
end
end
Context->>DT: AcceptChanges()
Context->>Table: OnChanged(DynamicObjectContextArgs with deleted items)
end
Table->>Table: TriggerDeleteCallback()
Table->>Table: QueryAsync(SelectedRowsChanged.HasDelegate)
Table->>Table: SelectedRows.Clear()
Class diagram for refactored dynamic DataTable pipelineclassDiagram
direction LR
class IDynamicObject {
<<interface>>
Guid DynamicObjectPrimaryKey
+object GetValue(string propertyName)
+void SetValue(string propertyName, object value)
+void Cancel()
+void Accept()
}
class DynamicObject {
+Guid DynamicObjectPrimaryKey
+DynamicObject()
+virtual object GetValue(string propertyName)
+virtual void SetValue(string propertyName, object value)
+virtual void Cancel()
+virtual void Accept()
}
class DataTableDynamicObject {
+DataTableDynamicObject(DataRow row)
+DataRow Row
+override object GetValue(string propertyName)
+override void SetValue(string propertyName, object value)
+override void Cancel()
+override void Accept()
}
class DynamicColumnsObject {
+DynamicColumnsObject(Dictionary~string, object~ columnsData)
+DynamicColumnsObject()
+Dictionary~string, object~ Columns
+virtual object GetValue(string propertyName)
+virtual void SetValue(string propertyName, object value)
+virtual void Cancel()
+virtual void Accept()
}
class DataTableDynamicContext {
+DataTable DataTable
+bool UseCache
+Func~IEnumerable~IDynamicObject~~, Task~bool~~ OnDeleteAsync
-ConcurrentDictionary~Guid, DataTableDynamicObject~ _dataCache
-List~ITableColumn~ _columns
-Action~DataTableDynamicContext, ITableColumn~ _addAttributesCallback
-List~IDynamicObject~ _items
+DataTableDynamicContext(DataTable table, Action~DataTableDynamicContext, ITableColumn~ addAttributesCallback, IEnumerable~string~ invisibleColumns, IEnumerable~string~ shownColumns, IEnumerable~string~ hiddenColumns)
+override IEnumerable~IDynamicObject~ GetItems()
+override IEnumerable~ITableColumn~ GetColumns()
-List~IDynamicObject~ BuildItems()
-void BuildTableColumns(IEnumerable~string~ invisibleColumns, IEnumerable~string~ shownColumns, IEnumerable~string~ hiddenColumns)
-static void GetShownColumns(ITableColumn col, IEnumerable~string~ invisibleColumns, IEnumerable~string~ shownColumns, IEnumerable~string~ hiddenColumns)
+internal void ApplyColumnAttribute~TAttribute~(string columnName, IEnumerable~KeyValuePair~string, object~~ parameters)
+internal void ApplyDisplayName(string columnName, string displayName)
+override Task AddAsync(IEnumerable~IDynamicObject~ selectedItems)
+override Task~bool~ DeleteAsync(IEnumerable~IDynamicObject~ items)
-Task OnCellValueChanged(IDynamicObject item, ITableColumn column, object val)
}
class InternalTableColumn {
+string FieldName
+Type FieldType
+string Text
+bool Visible
+bool Ignore
+InternalTableColumn(string fieldName, Type fieldType, string fieldText)
+string GetDisplayName()
}
class CacheManager {
<<static>>
+static TResult GetPropertyValue~TModel, TResult~(TModel model, string fieldName)
+static void SetPropertyValue~TModel, TValue~(TModel model, string fieldName, TValue value)
}
class Utility {
<<static>>
+static object GenerateValue(object model, string fieldName)
+static object GenerateValueChanged(ComponentBase component, object model, string fieldName, Type fieldType)
+static object GenerateOnValueChanged(object model, string fieldName, Type fieldType)
+static EventCallback~TType~ CreateCallback~TType~(ComponentBase component, object model, string fieldName)
-static Expression~Func~ComponentBase, object, string, object~~ CreateValueChangedLambda(Type fieldType)
-static Expression~Func~object, string, object~~ CreateDynamicObjectOnValueChangedLambda(Type fieldType)
-static Func~TValue, Task~ CreateDynamicObjectOnValueChangedCallback~TValue~(object model, string fieldName)
}
class DataRowExtensions {
<<static>>
+static bool IsDeletedOrDetached(this DataRow row)
}
class ValidateBase_T~T~ {
+T Value
+EventCallback~T~ ValueChanged
+Expression ValueExpression
+Func~T, Task~ OnValueChanged
+bool ShowRequired
+string RequiredErrorMessage
+bool SkipValidate
}
class Table_TItem~TItem~ {
+DynamicObjectContext DynamicContext
+IEnumerable~TItem~ SelectedRows
+object EditModel
+Task AddAsync()
+Task DeleteAsync()
+Task ShowEditDialog(ItemChangedType changedType)
-Task OnCloseEditDialogCallbackAsync(bool saved)
+Task QueryAsync(bool searchInPage)
}
IDynamicObject <|.. DynamicObject
DynamicObject <|-- DataTableDynamicObject
DynamicObject <|-- DynamicColumnsObject
DataTableDynamicContext --> DataTableDynamicObject : uses
DataTableDynamicContext --> InternalTableColumn : builds
DataTableDynamicContext --> DataRow : backs_items
DataTableDynamicObject --> DataRow : wraps
DataRowExtensions ..> DataRow : extends
CacheManager ..> IDynamicObject : uses GetValue SetValue
Utility ..> CacheManager : uses
Utility ..> IDynamicObject : GenerateOnValueChanged
Table_TItem --> DynamicObjectContext : uses
Table_TItem --> IDynamicObject : SelectedRows, EditModel
ValidateBase_T --> IDynamicObject : model when dynamic
ValidateBase_T ..> Utility : GenerateOnValueChanged, GenerateValueChanged
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- The new DataRowExtensions class is not valid C# (the
extension(DataRow row)construct won’t compile); this should be implemented as a standard extension method, e.g.public static bool IsDeletedOrDetached(this DataRow row)inside the static class. - In DynamicObjectContextExtensions,
AddDescriptionAttributecallstableContext.ApplyDisplayName(columnName, description)instead of a description-specific method, which looks unintentional and will overwrite the display name rather than storing a description. - In Utility.CreateComponentByFieldType, when the model is an IDynamicObject you no longer set ShowRequired, RequiredErrorMessage, or SkipValidate on ValidateBase; confirm this behavioral change is intentional, as dynamic-object fields will now skip those validation-related attributes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new DataRowExtensions class is not valid C# (the `extension(DataRow row)` construct won’t compile); this should be implemented as a standard extension method, e.g. `public static bool IsDeletedOrDetached(this DataRow row)` inside the static class.
- In DynamicObjectContextExtensions, `AddDescriptionAttribute` calls `tableContext.ApplyDisplayName(columnName, description)` instead of a description-specific method, which looks unintentional and will overwrite the display name rather than storing a description.
- In Utility.CreateComponentByFieldType, when the model is an IDynamicObject you no longer set ShowRequired, RequiredErrorMessage, or SkipValidate on ValidateBase; confirm this behavioral change is intentional, as dynamic-object fields will now skip those validation-related attributes.
## Individual Comments
### Comment 1
<location path="src/BootstrapBlazor/Extensions/DataRowExtensions.cs" line_range="10-12" />
<code_context>
+
+namespace BootstrapBlazor.Components;
+
+internal static class DataRowExtensions
+{
+ extension(DataRow row)
+ {
+ public bool IsDeletedOrDetached() => row.RowState == DataRowState.Deleted || row.RowState == DataRowState.Detached;
</code_context>
<issue_to_address>
**issue (bug_risk):** DataRowExtensions is not valid C# syntax for an extension method and will not compile
`extension(DataRow row)` is invalid C# and prevents this class from compiling. For an extension method, declare a static class and a static method with a `this` parameter, e.g.:
```csharp
internal static class DataRowExtensions
{
public static bool IsDeletedOrDetached(this DataRow row) =>
row.RowState == DataRowState.Deleted || row.RowState == DataRowState.Detached;
}
```
As written, all calls to `row.IsDeletedOrDetached()` in `DataTableDynamicContext` and `DataTableDynamicObject` will not compile.
</issue_to_address>
### Comment 2
<location path="src/BootstrapBlazor/Dynamic/DataTableDynamicObject.cs" line_range="78-87" />
<code_context>
+ /// <para lang="zh">撤销数据更改</para>
+ /// <para lang="en">Cancels data changes</para>
+ /// </summary>
+ public override void Cancel()
+ {
+ row.Table.RejectChanges();
+ }
+
+ /// <summary>
+ /// <para lang="zh">接受数据更改</para>
+ /// <para lang="en">Accepts data changes</para>
+ /// </summary>
+ public override void Accept()
+ {
+ row.Table.AcceptChanges();
+ }
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Cancel/Accept operate on the entire DataTable instead of the current row
Both methods call `row.Table.RejectChanges()` / `row.Table.AcceptChanges()`, which applies to every row in the `DataTable`, not just the current row. For a per-row editing context, these should operate on `row` itself:
```csharp
public override void Cancel() => row.RejectChanges();
public override void Accept() => row.AcceptChanges();
```
Otherwise, cancelling/accepting one item will also commit or roll back changes on other rows in the table.
</issue_to_address>
### Comment 3
<location path="src/BootstrapBlazor/Utils/Utility.cs" line_range="740-742" />
<code_context>
- builder.AddAttribute(41, nameof(ValidateBase<>.ShowRequired), GetRequired(item, changedType));
- builder.AddAttribute(42, nameof(ValidateBase<>.RequiredErrorMessage), item.RequiredErrorMessage);
+
+ if (model is IDynamicObject d)
+ {
+ var fieldOnValueChanged = GenerateOnValueChanged(model, fieldName, fieldType);
+ builder.AddAttribute(21, nameof(ValidateBase<>.OnValueChanged), fieldOnValueChanged);
+ }
</code_context>
<issue_to_address>
**question (bug_risk):** Dynamic-object branch no longer wires ValueChanged/ValueExpression/validation attributes
In the `IDynamicObject` path we only set `OnValueChanged`, so `ValueChanged`, `ValueExpression`, `ShowRequired` / `RequiredErrorMessage`, and `SkipValidate` are no longer wired, unlike the non-dynamic path. This may change validation and notification behavior for dynamic rows (e.g., no validation or missing UI updates).
If the intent is just to change the update mechanism, please ensure the dynamic branch still configures `ValueExpression`, `ShowRequired`, `RequiredErrorMessage`, and `SkipValidate`, and verify whether `ValueChanged` must also be set alongside `OnValueChanged`.
</issue_to_address>
### Comment 4
<location path="src/BootstrapBlazor/Extensions/DynamicObjectContextExtensions.cs" line_range="164-168" />
<code_context>
/// <para lang="en">Description</para>
/// </param>
- public static void AddDescriptionAttribute(this DynamicObjectContext context, string columnName, string description) => context.AddAttribute<DescriptionAttribute>(columnName, [typeof(string)], [description]);
+ public static void AddDescriptionAttribute(this DynamicObjectContext context, string columnName, string description)
+ {
+ if (context is DataTableDynamicContext tableContext)
+ {
+ tableContext.ApplyDisplayName(columnName, description);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** AddDescriptionAttribute uses ApplyDisplayName, which looks like a copy-paste mistake
`AddDescriptionAttribute` currently routes to `tableContext.ApplyDisplayName(columnName, description)`, which updates the display name, not a description field. To align with the method name and XML docs, this should use a description-specific API (e.g., `ApplyDescription`) rather than repurposing `ApplyDisplayName`, otherwise calling `AddDescriptionAttribute` will overwrite the column’s display text instead of its description.
</issue_to_address>
### Comment 5
<location path="src/BootstrapBlazor/Dynamic/DataTableDynamicContext.cs" line_range="200-204" />
<code_context>
+ //}
+ }
+
+ internal void ApplyDisplayName(string columnName, string displayName)
+ {
+ //if (InternalColumns.TryGetValue(columnName, out var column))
+ //{
+ // column.Text = displayName;
+ //}
}
</code_context>
<issue_to_address>
**issue (bug_risk):** ApplyDisplayName and ApplyColumnAttribute are effectively no-ops, but are now used by the extensions
Because these methods are effectively no-ops, any calls from `DynamicObjectContextExtensions` (e.g., `AddDisplayNameAttribute`, `AddRequiredAttribute`) won’t update the in-memory `InternalTableColumn` state for `DataTableDynamicContext`. That’s a behavior change from when attributes lived on the dynamic type and is likely a regression. If the goal is to move attribute handling into `InternalTableColumn`, these method bodies should be implemented rather than commented out so the column configuration is actually updated.
</issue_to_address>
### Comment 6
<location path="src/BootstrapBlazor/Services/CacheManager.cs" line_range="547-548" />
<code_context>
return propertyInfo != null;
}
- public static TResult GetPropertyValue<TModel, TResult>(TModel model, string fieldName) => (model is IDynamicColumnsObject d)
+ public static TResult GetPropertyValue<TModel, TResult>(TModel model, string fieldName) => (model is IDynamicObject d)
? (TResult)d.GetValue(fieldName)!
: GetValue<TModel, TResult>(model, fieldName);
</code_context>
<issue_to_address>
**question (bug_risk):** Changing GetPropertyValue to use IDynamicObject may alter behavior for non-column-backed properties
`GetPropertyValue` used to special-case `IDynamicColumnsObject`, but now applies to all `IDynamicObject`. Combined with `DynamicObject.GetValue` now returning `null` by default, any `IDynamicObject` that still relies on regular CLR properties (without overriding `GetValue` accordingly) may now resolve those properties differently. Consider either keeping the special case to `IDynamicColumnsObject` or verifying that all `IDynamicObject` implementations override `GetValue` to maintain the previous behavior.
</issue_to_address>
### Comment 7
<location path="src/BootstrapBlazor/Dynamic/DataTableDynamicContext.cs" line_range="157" />
<code_context>
+ }
+ }
+
+ internal void ApplyColumnAttribute<TAttribute>(string columnName, IEnumerable<KeyValuePair<string, object?>> parameters) where TAttribute : Attribute
+ {
+ //if (!InternalColumns.TryGetValue(columnName, out var column))
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing or simplifying the partially implemented column-configuration helpers so that column setup logic is explicit, local, and free of dead code.
The biggest complexity increase here comes from the partially‑implemented / dead abstractions around column configuration. You can cut that down without losing any functionality.
### 1. Remove or fully implement no‑op attribute APIs
`ApplyColumnAttribute<TAttribute>` and `ApplyDisplayName` are currently public-ish entry points with only large commented‑out bodies. They read as “important” but actually do nothing, which adds cognitive overhead.
If you don’t need them yet, delete them and re‑introduce later from git history:
```csharp
// REMOVE these until you actually wire them through:
// internal void ApplyColumnAttribute<TAttribute>(string columnName, IEnumerable<KeyValuePair<string, object?>> parameters) where TAttribute : Attribute
// { ... }
// internal void ApplyDisplayName(string columnName, string displayName)
// { ... }
```
If you *do* need them as part of the contract, make their behavior explicit and minimal instead of commented out. For example, implement only the parts you actually support today and drive it off the existing `_columns`:
```csharp
internal void ApplyDisplayName(string columnName, string displayName)
{
var column = _columns.FirstOrDefault(c =>
c.GetFieldName().Equals(columnName, StringComparison.OrdinalIgnoreCase));
if (column is null) return;
column.Text = displayName;
}
internal void ApplyColumnAttribute<TAttribute>(
string columnName,
IEnumerable<KeyValuePair<string, object?>> parameters
) where TAttribute : Attribute
{
var column = _columns.FirstOrDefault(c =>
c.GetFieldName().Equals(columnName, StringComparison.OrdinalIgnoreCase));
if (column is null) return;
if (typeof(TAttribute) == typeof(RequiredAttribute))
{
column.Required = true;
var message = parameters.FirstOrDefault(p => p.Key == nameof(RequiredAttribute.ErrorMessage)).Value as string;
if (!string.IsNullOrEmpty(message))
{
column.RequiredErrorMessage = message;
}
return;
}
// Add more cases only as needed, or explicitly no-op:
// else if (typeof(TAttribute) == typeof(...)) { ... }
}
```
This removes the “mystery commented code” and makes it obvious what is and isn’t supported.
### 2. Collapse `GetShownColumns` indirection into `BuildTableColumns`
`GetShownColumns` currently mutates `col.Ignore`/`Visible` and is called only from `BuildTableColumns`. That two‑step (mutate, then check `Ignore`) is more indirect than necessary.
You can inline the logic and drop the helper without changing behavior:
```csharp
private void BuildTableColumns(
IEnumerable<string>? invisibleColumns = null,
IEnumerable<string>? shownColumns = null,
IEnumerable<string>? hiddenColumns = null)
{
_columns.Clear();
foreach (DataColumn col in DataTable.Columns)
{
var column = new InternalTableColumn(col.ColumnName, col.DataType);
var columnName = column.GetFieldName();
if (invisibleColumns != null &&
invisibleColumns.Any(c => c.Equals(columnName, StringComparison.OrdinalIgnoreCase)))
{
// skip entirely
continue;
}
if (hiddenColumns != null &&
hiddenColumns.Any(c => c.Equals(columnName, StringComparison.OrdinalIgnoreCase)))
{
column.Visible = false;
}
if (shownColumns != null &&
shownColumns.Any(c => c.Equals(columnName, StringComparison.OrdinalIgnoreCase)))
{
column.Visible = true;
}
OnColumnCreating(column);
_columns.Add(column);
}
}
```
Then you can safely remove `GetShownColumns`:
```csharp
// REMOVE:
// private static void GetShownColumns(ITableColumn col, ...)
// { ... }
```
This keeps all existing behaviors (invisible → not added; hidden/shown → adjust `Visible`) while making the column‑building flow local and easier to follow.
</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 aims to improve Table performance (fixes #7912) primarily by optimizing DataTable-backed dynamic table editing/rendering and reducing per-cell binding overhead for dynamic models.
Changes:
- Refactors DataTable dynamic support to wrap
DataRowdirectly (DataTableDynamicObject) and rebuild columns/items more efficiently. - Updates editor component binding to use
ValidateBase.OnValueChangedforIDynamicObjectmodels instead of generatingValueChanged/ValueExpression. - Extends
IDynamicObjectwithCancel()/Accept()and wires these into the Table edit dialog close/save flow.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor/Utils/Utility.cs | Adjusts binding logic for IDynamicObject editors; adds GenerateOnValueChanged helper |
| src/BootstrapBlazor/Services/CacheManager.cs | Changes dynamic property access handling (Get vs Set) |
| src/BootstrapBlazor/Extensions/DynamicObjectContextExtensions.cs | Routes attribute helper calls into DataTableDynamicContext hooks |
| src/BootstrapBlazor/Extensions/DataRowExtensions.cs | Adds DataRow helper to detect Deleted/Detached state |
| src/BootstrapBlazor/Dynamic/IDynamicObject.cs | Adds Cancel() / Accept() to the public interface |
| src/BootstrapBlazor/Dynamic/DynamicObject.cs | Updates base IDynamicObject implementation behavior |
| src/BootstrapBlazor/Dynamic/DynamicColumnsObject.cs | Implements new IDynamicObject members |
| src/BootstrapBlazor/Dynamic/DataTableDynamicObject.cs | Introduces row-wrapping dynamic object + Accept/Cancel behavior |
| src/BootstrapBlazor/Dynamic/DataTableDynamicContext.cs | Major refactor of DataTable dynamic context (columns/items/cache) |
| src/BootstrapBlazor/Components/Table/Table.razor.Toolbar.cs | Uses query refresh after add/delete; integrates Accept/Cancel into dialog flow |
| src/BootstrapBlazor/Components/Table/InternalTableColumn.cs | Makes display name fall back to field name |
| src/BootstrapBlazor.Server/Components/Samples/Table/TablesDynamic.razor.cs | Minor formatting change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7913 +/- ##
===========================================
- Coverage 100.00% 99.98% -0.02%
===========================================
Files 764 765 +1
Lines 34337 34325 -12
Branches 4711 4710 -1
===========================================
- Hits 34337 34321 -16
- Misses 0 2 +2
- Partials 0 2 +2
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 #7912
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Optimize dynamic DataTable handling and dynamic object editing to improve table performance and consistency.
Bug Fixes:
Enhancements: