feat(Table): add RenderContentRow improve readability #6917
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
Reviewer's GuideRefactored table row rendering by extracting inline cell and row logic into reusable RenderContentRow and RenderContentCell fragments, backed by a TableContentCellContext struct and helper methods, to improve readability and maintainability. Sequence diagram for refactored table row rendering using RenderContentRow and RenderContentCellsequenceDiagram
participant Table
participant "RenderContentRow()"
participant "RenderContentCell()"
participant "TableContentCellContext"
Table->>"RenderContentRow()": Call for each row
"RenderContentRow()"->>"GetCellArgs()": Get cell args for each column
"RenderContentRow()"->>"GetTreeInfo()": Get tree info for first column
"RenderContentRow()"->>"TableContentCellContext": Create context
"RenderContentRow()"->>"RenderContentCell()": Render cell with context
"RenderContentCell()"-->>Table: Rendered cell fragment
Class diagram for new TableContentCellContext struct and related typesclassDiagram
class TableContentCellContext~TItem~ {
+TItem Item
+int Colspan
+ITableColumn Col
+string? CellClass
+bool HasTreeChildren
+bool IsInCell
+int Degree
+bool IsExpand
+bool IsFirstColOfTree
+RenderFragment? ValueTemplate
+string? Value
}
class ITableColumn {
}
class RenderFragment {
}
TableContentCellContext --> ITableColumn : Col
TableContentCellContext --> RenderFragment : ValueTemplate
Class diagram for new RenderContentRow and RenderContentCell fragmentsclassDiagram
class Table {
+RenderFragment RenderContentRow(TItem item)
+RenderFragment<TableContentCellContext<TItem>> RenderContentCell
+TableContentCellContext<TItem> GetCellArgs(TItem item, ITableColumn col, ref int colIndex)
+(bool, int, bool, bool) GetTreeInfo(TItem item, int index)
}
Table --> TableContentCellContext : uses
Table --> ITableColumn : uses
Table --> RenderFragment : uses
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.
Pull Request Overview
This PR refactors the Table component's row rendering logic by extracting inline cell rendering code into a dedicated RenderContentRow method and introducing a new TableContentCellContext record struct. The refactoring improves code readability and maintainability by reducing the complexity of the Razor template and moving rendering logic to the code-behind file.
Key Changes:
- Introduced
TableContentCellContext<TItem>record struct to encapsulate cell rendering state - Extracted row rendering logic from Razor template into
RenderContentRowmethod in code-behind - Created
RenderContentCellrender fragment to handle individual cell rendering using the new context
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| TableContentCellContext.cs | New record struct defining the context/state for rendering table content cells |
| Table.razor.cs | Added RenderContentRow method containing the extracted row rendering logic |
| Table.razor | Refactored to use the new RenderContentRow method and RenderContentCell fragment, removing inline rendering code |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/BootstrapBlazor/Components/Table/TableContentCellContext.cs
Outdated
Show resolved
Hide resolved
src/BootstrapBlazor/Components/Table/TableContentCellContext.cs
Outdated
Show resolved
Hide resolved
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Rename the
HesTreeChildrenproperty toHasTreeChildrenfor clarity and to avoid confusion. - Don’t reuse the same sequence number (0) for every call to builder.AddContent; use unique, incrementing sequence values to ensure Blazor diffing works correctly.
- Creating a new TableContentCellContext and RenderFragment for every cell may impact performance—consider reusing or caching context data where possible to reduce allocations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Rename the `HesTreeChildren` property to `HasTreeChildren` for clarity and to avoid confusion.
- Don’t reuse the same sequence number (0) for every call to builder.AddContent; use unique, incrementing sequence values to ensure Blazor diffing works correctly.
- Creating a new TableContentCellContext and RenderFragment for every cell may impact performance—consider reusing or caching context data where possible to reduce allocations.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/Table/Table.razor.cs:1373-1382` </location>
<code_context>
+ foreach (var col in GetVisibleColumns())
</code_context>
<issue_to_address>
**suggestion:** The colIndex logic may be unclear and could be refactored for readability.
Consider refactoring the colIndex logic or adding a comment to clarify its role in column rendering and to prevent off-by-one mistakes.
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Components/Table/Table.razor.cs:1367` </location>
<code_context>
};
#endregion
+ private RenderFragment RenderContentRow(TItem item) => builder =>
+ {
+ var index = 0;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring RenderContentRow by extracting cell rendering and tree logic into helper methods for clarity and maintainability.
Here’s one way to collapse the two big chunks (`OnCellRender` / colspan logic and the tree-info logic) into small helpers so that your `RenderContentRow` is just a simple loop + a call into each helper.
```csharp
private TableCellArgs GetCellArgs(TItem item, ITableColumn col, ref int colIndex)
{
var cell = new TableCellArgs { Row = item, ColumnName = col.GetFieldName() };
col.OnCellRender?.Invoke(cell);
colIndex = cell.Colspan;
return cell;
}
private (bool isFirstColOfTree, int degree, bool isExpand, bool hasChildren)
GetTreeInfo(TItem item, int index)
{
var isFirstColOfTree = IsTree && index == 0;
if (!isFirstColOfTree)
return (false, 0, false, false);
var degree = 0;
var treeItem = TreeNodeCache.Find(TreeRows, item, out degree);
return ( isFirstColOfTree,
degree,
treeItem?.IsExpand ?? false,
treeItem?.HasChildren ?? false );
}
```
And then rewrite `RenderContentRow` to just call those:
```csharp
private RenderFragment RenderContentRow(TItem item) => builder =>
{
var index = 0;
var colIndex = 0;
var isInCell = InCellMode && SelectedRows.FirstOrDefault() == item;
foreach (var col in GetVisibleColumns())
{
if (colIndex > 1)
{
colIndex--;
continue;
}
// 1) pull out any OnCellRender / colspan logic
var cellArgs = GetCellArgs(item, col, ref colIndex);
// 2) pull out tree node logic
var (isTreeCol, degree, isExpand, hasChildren) = GetTreeInfo(item, index++);
var hasTreeChildren = isTreeCol && hasChildren;
// 3) one-liner to create context
var ctx = new TableContentCellContext<TItem>
{
Item = item,
Col = col,
ColSpan = cellArgs.Colspan,
CellClass = cellArgs.Class,
Value = cellArgs.Value,
ValueTemplate = cellArgs.ValueTemplate,
IsInCell = isInCell,
IsFirstColOfTree = isTreeCol,
Degree = degree,
IsExpend = isExpand,
HesTreeChildren = hasTreeChildren
};
builder.AddContent(0, RenderContentCell(ctx));
}
};
```
Benefits:
- RenderContentRow is now ~15 lines instead of ~40.
- All colspan/OnCellRender details live in `GetCellArgs`.
- All tree-node lookup lives in `GetTreeInfo`.
- Easy to read, test, or tweak each piece in isolation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6917 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 739 740 +1
Lines 31776 31848 +72
Branches 4468 4468
=========================================
+ Hits 31776 31848 +72
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:
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/Table/Table.razor.cs:1367` </location>
<code_context>
};
#endregion
+ private RenderFragment RenderContentRow(TItem item) => builder =>
+ {
+ var index = 0;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the loop by extracting cell rendering, tree state, and context-building logic into helper methods for clarity.
Here’s one way to pull most of the “plumbing” out of your loop into three small helpers:
```csharp
private RenderFragment RenderContentRow(TItem item) => builder =>
{
var skipCols = 0;
var treeIndex = 0;
var isInCell = InCellMode && SelectedRows.FirstOrDefault() == item;
foreach (var col in GetVisibleColumns())
{
if (skipCols > 0)
{
skipCols--;
continue;
}
// ← Extracted cell-render hook + colspan logic
var cellArgs = ProcessOnCellRender(col, item, ref skipCols);
// ← Extracted “tree” state logic
var (isFirstTreeCol, degree, isExpand, hasChildren) = EvaluateTreeState(item, treeIndex++);
// ← Extracted context-build
var context = BuildCellContext(item, col, cellArgs, isInCell,
isFirstTreeCol, degree, isExpand, hasChildren);
builder.AddContent(0, RenderContentCell(context));
}
};
```
And the three helpers:
```csharp
private TableCellArgs ProcessOnCellRender(
ITableColumn col,
TItem item,
ref int skipCols)
{
var args = new TableCellArgs { Row = item, ColumnName = col.GetFieldName() };
col.OnCellRender?.Invoke(args);
if (args.Colspan > 1)
{
// skip the next (Colspan-1) columns
skipCols = args.Colspan - 1;
}
return args;
}
private (bool isFirstTreeCol, int degree, bool isExpand, bool hasChildren)
EvaluateTreeState(TItem item, int currentIndex)
{
if (!IsTree || currentIndex != 0)
return (false, 0, false, false);
var treeItem = TreeNodeCache.Find(TreeRows, item, out var degree);
return (true,
degree,
treeItem?.IsExpand == true,
treeItem?.HasChildren == true);
}
private TableContentCellContext<TItem> BuildCellContext(
TItem item,
ITableColumn col,
TableCellArgs cellArgs,
bool isInCell,
bool isFirstTreeCol,
int degree,
bool isExpand,
bool hasChildren)
{
return new TableContentCellContext<TItem>()
{
Item = item,
Col = col,
CellClass = cellArgs.Class,
ColSpan = cellArgs.Colspan,
Value = cellArgs.Value,
ValueTemplate = cellArgs.ValueTemplate,
IsInCell = isInCell,
IsFirstColOfTree = isFirstTreeCol,
Degree = degree,
IsExpand = isExpand,
HasTreeChildren = isFirstTreeCol && hasChildren
};
}
```
This keeps your loop body very flat, each helper has one clear job, and overall behavior is unchanged.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Use unique, incrementing sequence values when calling builder.AddContent in RenderContentRow to ensure Blazor diffing works correctly.
- Consider renaming the loop variables 'index' and 'colIndex' in RenderContentRow to more descriptive names (e.g. 'cellIndex' and 'remainingSpan') to clarify their purposes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Use unique, incrementing sequence values when calling builder.AddContent in RenderContentRow to ensure Blazor diffing works correctly.
- Consider renaming the loop variables 'index' and 'colIndex' in RenderContentRow to more descriptive names (e.g. 'cellIndex' and 'remainingSpan') to clarify their purposes.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor/Components/Table/Table.razor.cs:1408` </location>
<code_context>
+ }
+ };
+
+ private static TableCellArgs GetCellArgs(TItem item, ITableColumn col, ref int colIndex)
+ {
+ var cell = new TableCellArgs { Row = item, ColumnName = col.GetFieldName() };
</code_context>
<issue_to_address>
**suggestion:** Reconsider using ref parameter for colIndex in GetCellArgs.
Returning both TableCellArgs and the updated colIndex, or encapsulating the logic, would improve code clarity and reduce the risk of bugs from ref usage.
Suggested implementation:
```csharp
private static (TableCellArgs cell, int colIndex) GetCellArgs(TItem item, ITableColumn col, int colIndex)
{
var cell = new TableCellArgs { Row = item, ColumnName = col.GetFieldName() };
col.OnCellRender?.Invoke(cell);
colIndex = cell.Colspan;
return (cell, colIndex);
}
```
You will need to update all calls to `GetCellArgs` throughout the file to:
- Remove the `ref` keyword from the `colIndex` argument.
- Use tuple deconstruction to get both the `TableCellArgs` and the updated `colIndex`, e.g.:
```csharp
var (cell, colIndex) = GetCellArgs(item, col, colIndex);
```
</issue_to_address>
### Comment 2
<location> `src/BootstrapBlazor/Components/Table/Table.razor.cs:1416` </location>
<code_context>
+ return cell;
+ }
+
+ private (bool isFirstColOfTree, int degree, bool isExpand, bool hasChildren) GetTreeInfo(TItem item, int index)
+ {
+ var isFirstColOfTree = IsTree && index == 0;
</code_context>
<issue_to_address>
**suggestion:** Tuple return type could be replaced with a dedicated struct for tree info.
Using a struct or record for the return type will make the code easier to read and maintain, particularly if more fields are added in the future.
Suggested implementation:
```csharp
private struct TreeInfo
{
public bool IsFirstColOfTree { get; init; }
public int Degree { get; init; }
public bool IsExpand { get; init; }
public bool HasChildren { get; init; }
}
private TreeInfo GetTreeInfo(TItem item, int index)
{
var isFirstColOfTree = IsTree && index == 0;
if (!isFirstColOfTree)
{
return new TreeInfo
{
IsFirstColOfTree = false,
Degree = 0,
IsExpand = false,
HasChildren = false
};
}
var treeItem = TreeNodeCache.Find(TreeRows, item, out var degree);
return new TreeInfo
{
IsFirstColOfTree = isFirstColOfTree,
Degree = degree,
IsExpand = treeItem?.IsExpand ?? false,
HasChildren = treeItem?.HasChildren ?? false
};
```
If there are usages of `GetTreeInfo` elsewhere in the file or project, you will need to update them to use the new `TreeInfo` struct fields (e.g., `.IsFirstColOfTree`, `.Degree`, etc.) instead of tuple deconstruction.
</issue_to_address>
### Comment 3
<location> `src/BootstrapBlazor/Components/Table/Table.razor:747-756` </location>
<code_context>
+ RenderFragment<TableContentCellContext<TItem>> RenderContentCell => context =>
</code_context>
<issue_to_address>
**suggestion:** Consider extracting inner logic of RenderContentCell to a helper method for readability.
Moving the complex logic from the lambda to a helper method will make the code easier to read and maintain as future changes are made.
</issue_to_address>
### Comment 4
<location> `src/BootstrapBlazor/Components/Table/Table.razor.cs:1367` </location>
<code_context>
};
#endregion
+ private RenderFragment RenderContentRow(TItem item) => builder =>
+ {
+ var index = 0;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the cell rendering loop to use a for-loop with Colspan-based index advancement and helper methods for context and tree info creation.
```suggestion
The manual `colIndex` skipping and tuple‐based `GetTreeInfo` can be inlined/simplified by using a `for` loop that advances by `Colspan`, and moving the context building into its own helper. This removes the `if (colIndex > 1) … continue` pattern and flattens the nesting.
```csharp
private RenderFragment RenderContentRow(TItem item) => builder =>
{
var cols = GetVisibleColumns().ToList();
var seq = 0;
for (int i = 0; i < cols.Count; i++)
{
var col = cols[i];
// get cell args and colspan
var cellArgs = GetCellArgs(item, col);
// build full context (including tree info if needed)
var context = BuildCellContext(item, col, cellArgs, seq++);
builder.AddContent(0, RenderContentCell(context));
// skip merged columns
i += cellArgs.Colspan - 1;
}
};
private TableCellArgs GetCellArgs(TItem item, ITableColumn col)
{
var args = new TableCellArgs { Row = item, ColumnName = col.GetFieldName() };
col.OnCellRender?.Invoke(args);
return args;
}
private TableContentCellContext<TItem> BuildCellContext(
TItem item,
ITableColumn col,
TableCellArgs cellArgs,
int cellIndex)
{
var isFirstTreeCol = IsTree && cellIndex == 0;
var (degree, isExpand, hasChildren) = isFirstTreeCol
? GetTreeNodeInfo(item)
: (0, false, false);
return new TableContentCellContext<TItem>
{
Item = item,
Col = col,
Colspan = cellArgs.Colspan,
CellClass = cellArgs.Class,
Value = cellArgs.Value,
ValueTemplate = cellArgs.ValueTemplate,
HasTreeChildren = isFirstTreeCol && hasChildren,
IsInCell = InCellMode && SelectedRows.FirstOrDefault() == item,
Degree = degree,
IsExpand = isExpand,
IsFirstColOfTree= isFirstTreeCol
};
}
private (int degree, bool isExpand, bool hasChildren) GetTreeNodeInfo(TItem item)
{
var node = TreeNodeCache.Find(TreeRows, item, out var degree);
return (degree,
node?.IsExpand ?? false,
node?.HasChildren ?? false);
}
```
Steps:
1. Switch to a `for`‐loop over a `List<ITableColumn>` so you can `i += Colspan - 1`.
2. Inline skipping logic via the loop increment.
3. Extract tree‐info and context‐building into `BuildCellContext` and `GetTreeNodeInfo` for clearer separation.
4. Keep `GetCellArgs` focused on `col.OnCellRender` and `Colspan` determination.
This preserves all functionality but makes the flow linear and each helper single‐purpose.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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 #6916
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Refactor Table component to extract and streamline content row rendering into separate methods and context struct, enhancing readability and maintainability.
Enhancements: