feat(table): apply conditional formatting to the totals row to table and agGridTable - #43370
feat(table): apply conditional formatting to the totals row to table and agGridTable#43370SBIN2010 wants to merge 84 commits into
Conversation
This reverts commit 35aaeb6.
Code Review Agent Run #371144Actionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| emitCrossFilters, | ||
| onChangeFilter, | ||
| columnColorFormatters, | ||
| applyConditionalFormattingToTotals, |
There was a problem hiding this comment.
Suggestion: When this flag is enabled, totals formatting is driven only by columnColorFormatters, but the preceding formatter construction explicitly removes the Green and Red conditional-formatting schemes. Those schemes are handled separately through basicColorColumnFormatters for data cells, while the totals renderer does not consume that formatter, so enabling this option silently fails to format totals for Green/Red conditional rules. Include the basic conditional formatter path when constructing totals styling, or route all conditional-formatting schemes through the totals formatter. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Green/Red rules fail on enabled totals rows.
- ⚠️ Data and totals cells display inconsistent formatting.
- ⚠️ The new totals-formatting option is incomplete.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/transformProps.ts
**Line:** 825:825
**Comment:**
*Incomplete Implementation: When this flag is enabled, totals formatting is driven only by `columnColorFormatters`, but the preceding formatter construction explicitly removes the Green and Red conditional-formatting schemes. Those schemes are handled separately through `basicColorColumnFormatters` for data cells, while the totals renderer does not consume that formatter, so enabling this option silently fails to format totals for Green/Red conditional rules. Include the basic conditional formatter path when constructing totals styling, or route all conditional-formatting schemes through the totals formatter.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The issue flagged in the review comment is correct. The current implementation of To resolve this, you should update I have validated the issue and the proposed fix direction. Would you like me to fetch all other comments on this PR to check if there are other issues you would like to address? |
| columnColorFormatters | ||
| .filter(formatter => { | ||
| if (formatter.columnFormatting) { | ||
| return formatter.columnFormatting === columnKey; | ||
| } | ||
| return formatter.column === columnKey; | ||
| }) | ||
| .forEach(formatter => { | ||
| const valueToFormat = formatter.columnFormatting | ||
| ? record[formatter.column as string] | ||
| : value; | ||
| applyFormatter(formatter, valueToFormat); | ||
| }); |
There was a problem hiding this comment.
Suggestion: Comparison totals are keyed as Main <metric>, # <metric>, △ <metric>, and % <metric>, but this lookup always reads record[formatter.column], where formatter.column remains the original metric key. Consequently, a rule configured with columnFormatting can match a comparison totals column but receives undefined instead of that totals cell's value, so the rule is skipped or produces incorrect formatting. Resolve the formatter's source value using the corresponding comparison key before calling getColorFromValue. [incorrect variable usage]
Severity Level: Major ⚠️
- ⚠️ Comparison totals cells ignore matching conditional rules.
- ⚠️ Time-comparison summary formatting appears inconsistent.
- ⚠️ Comparison data cells use the same incorrect source lookup.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
**Line:** 149:161
**Comment:**
*Incorrect Variable Usage: Comparison totals are keyed as `Main <metric>`, `# <metric>`, `△ <metric>`, and `% <metric>`, but this lookup always reads `record[formatter.column]`, where `formatter.column` remains the original metric key. Consequently, a rule configured with `columnFormatting` can match a comparison totals column but receives `undefined` instead of that totals cell's value, so the rule is skipped or produces incorrect formatting. Resolve the formatter's source value using the corresponding comparison key before calling `getColorFromValue`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #43370 +/- ##
==========================================
+ Coverage 66.66% 66.68% +0.02%
==========================================
Files 2873 2873
Lines 163654 163765 +111
Branches 37771 37832 +61
==========================================
+ Hits 109093 109208 +115
+ Misses 52432 52428 -4
Partials 2129 2129
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/review |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #000e4c
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx - 1
- Incomplete totals color handling · Line 1162-1165
Additional Suggestions - 5
-
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx - 1
-
Test key mismatch in assertion · Line 701-768The test expects `totalsBasicColorFormatters?.metric_1` but the comparison totals processing in `transformProps` (line 129-148) produces keys prefixed with `Main ` (e.g., `Main metric_1`, `# metric_1`). The actual computed formatter keys may not align with the assertion path. Compare with the test at line 781 which correctly uses `sum__num` as the key since it's not using comparison totals.
-
-
superset-frontend/plugins/plugin-chart-table/src/transformProps.ts - 1
-
Non-i18n hardcoded string · Line 153-153Hardcoded English string 'Main' should use `t('Main')` for i18n consistency. The `t` function is already imported at line 20 and used elsewhere in the file. The parallel implementation in `plugin-chart-ag-grid-table` at line 135 already uses `t('Main')`.
-
-
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx - 1
-
Duplicate formatting logic creates divergence · Line 1228-1248The footer rendering logic (lines 1120-1199) duplicates conditional formatting application from lines 1228-1248 using different code paths. Footer manually implements filtering and color resolution while Cell renderer uses centralized `getConditionalFormattingColors()`. This creates divergence risk where one path may receive updates while the other doesn't.
-
-
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx - 1
-
Missing component unit test coverage · Line 85-85This prop is extracted on line 85 and passed to `useColDefs` on line 282, but `AgGridTableChart.test.tsx` (892 lines) has zero test coverage for `applyConditionalFormattingToTotals`. Tests exist for transformProps and controlPanel, but the component-level test that actually renders `AgGridTableChart` never exercises this prop path.
-
-
superset-frontend/plugins/plugin-chart-table/src/types.ts - 1
-
Type consistency mismatch · Line 123-128Type pattern inconsistency: `totalsBasicColorFormatters` and `totalsBasicColorColumnFormatters` use plain objects `{ [Key: string]: BasicColorFormatterType }` while existing `basicColorFormatters` and `basicColorColumnFormatters` (lines 121-122) use arrays `{ [Key: string]: BasicColorFormatterType }[]`. Runtime usage at lines 800-810 and 1153-1157 confirms object access works correctly, but this structural mismatch increases maintenance burden and may confuse future contributors.
-
Review Details
-
Files reviewed - 15 · Commit Range:
19151d1..84fdcce- superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/test/controlPanel.test.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/test/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getCellStyle.test.ts
- superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
- superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-table/src/transformProps.ts
- superset-frontend/plugins/plugin-chart-table/src/types.ts
- superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
- superset-frontend/plugins/plugin-chart-table/test/controlPanel.test.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
There was a problem hiding this comment.
Code Review Agent Run #d79b5e
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx - 1
- Missing unit tests for new color-formatting helpers · Line 111-252
Additional Suggestions - 2
-
superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getCellStyle.test.ts - 1
-
Misleading test name · Line 151-151The test name 'does not color totals from the first data row' is misleading — the passing condition stems from `node.data[BASIC_COLOR_FORMATTERS_ROW_KEY]` being absent on the pinned row, not from which data row the formatters come from. A more precise name: 'does not color pinned summary row using array-based formatters when row-specific formatter is missing'.
-
-
superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts - 1
-
Duplicate key transformation logic · Line 129-147The `toOriginalMetricTotals` function duplicates key transformation logic already present in `processComparisonDataRecords` (lines 191-229). Both functions transform `Main ${key}` → `key` and `# ${key}` → `${key}__${suffix}`. If the key format changes (e.g., `t('Main')` localization), both must be updated. Extract shared logic to reduce maintenance divergence risk.
-
Review Details
-
Files reviewed - 16 · Commit Range:
19151d1..3b68a1f- superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/useColDefs.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/test/AgGridTableChart.test.tsx
- superset-frontend/plugins/plugin-chart-ag-grid-table/test/controlPanel.test.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/test/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getCellStyle.test.ts
- superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx
- superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-table/src/transformProps.ts
- superset-frontend/plugins/plugin-chart-table/src/types.ts
- superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
- superset-frontend/plugins/plugin-chart-table/test/controlPanel.test.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✖︎ Failed
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| type ConditionalFormattingColors = { | ||
| backgroundColor?: string; | ||
| color?: string; | ||
| backgroundColorCellBar?: string; | ||
| skipValueRange?: boolean; | ||
| }; | ||
|
|
||
| function getConditionalFormattingColors( | ||
| columnColorFormatters: ColorFormatters, | ||
| record: DataRecord, | ||
| columnKey: string, | ||
| value: DataRecordValue, | ||
| applyCellBars = false, | ||
| ): ConditionalFormattingColors { | ||
| const colors: ConditionalFormattingColors = {}; | ||
| const applyFormatter = ( | ||
| formatter: ColorFormatters[number], | ||
| valueToFormat: DataRecordValue, | ||
| ) => { | ||
| const formatterResult = formatter.getColorFromValue( | ||
| valueToFormat as number | string | boolean | null, | ||
| ); | ||
| if (!formatterResult) return; | ||
|
|
||
| if ( | ||
| formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR || | ||
| formatter.toTextColor | ||
| ) { | ||
| colors.color = formatterResult; | ||
| } else if (formatter.objectFormatting === ObjectFormattingEnum.CELL_BAR) { | ||
| if (applyCellBars) { | ||
| colors.backgroundColorCellBar = forceHexAlpha(formatterResult); | ||
| } | ||
| } else { | ||
| colors.backgroundColor = formatterResult; | ||
| colors.skipValueRange = true; | ||
| } | ||
| }; | ||
|
|
||
| columnColorFormatters | ||
| .filter(formatter => { | ||
| if (formatter.columnFormatting) { | ||
| return formatter.columnFormatting === columnKey; | ||
| } | ||
| return formatter.column === columnKey; | ||
| }) | ||
| .forEach(formatter => { | ||
| const valueToFormat = formatter.columnFormatting | ||
| ? record[formatter.column as string] | ||
| : value; | ||
| applyFormatter(formatter, valueToFormat); | ||
| }); | ||
|
|
||
| columnColorFormatters | ||
| .filter( | ||
| formatter => | ||
| formatter.columnFormatting === ObjectFormattingEnum.ENTIRE_ROW, | ||
| ) | ||
| .forEach(formatter => | ||
| applyFormatter(formatter, record[formatter.column as string]), | ||
| ); | ||
|
|
||
| return colors; | ||
| } | ||
|
|
||
| function lookupRowBasicColor( | ||
| formatters: { [key: string]: BasicColorFormatterType } | undefined, | ||
| columnKey: string, | ||
| originKey: string, | ||
| ) { | ||
| return ( | ||
| formatters?.[columnKey] || (originKey ? formatters?.[originKey] : undefined) | ||
| ); | ||
| } | ||
|
|
||
| function resolveCellColorFormatting({ | ||
| hasColumnColorFormatters, | ||
| columnColorFormatters, | ||
| record, | ||
| columnKey, | ||
| value, | ||
| applyCellBars, | ||
| comparisonColorFormatters, | ||
| greenRedFormatters, | ||
| originKey, | ||
| }: { | ||
| hasColumnColorFormatters: boolean; | ||
| columnColorFormatters?: ColorFormatters; | ||
| record: DataRecord; | ||
| columnKey: string; | ||
| value: DataRecordValue; | ||
| applyCellBars: boolean; | ||
| comparisonColorFormatters?: { [key: string]: BasicColorFormatterType }; | ||
| greenRedFormatters?: { [key: string]: BasicColorFormatterType }; | ||
| originKey: string; | ||
| }): ConditionalFormattingColors { | ||
| const colors: ConditionalFormattingColors = {}; | ||
|
|
||
| if (!hasColumnColorFormatters) { | ||
| const comparison = lookupRowBasicColor( | ||
| comparisonColorFormatters, | ||
| columnKey, | ||
| originKey, | ||
| ); | ||
| if (comparison?.backgroundColor) { | ||
| colors.backgroundColor = comparison.backgroundColor; | ||
| } | ||
| } | ||
|
|
||
| if (hasColumnColorFormatters && columnColorFormatters) { | ||
| const formatting = getConditionalFormattingColors( | ||
| columnColorFormatters, | ||
| record, | ||
| columnKey, | ||
| value, | ||
| applyCellBars, | ||
| ); | ||
| if (formatting.color) { | ||
| colors.color = formatting.color; | ||
| } | ||
| if (formatting.backgroundColor) { | ||
| colors.backgroundColor = formatting.backgroundColor; | ||
| } | ||
| if (formatting.backgroundColorCellBar) { | ||
| colors.backgroundColorCellBar = formatting.backgroundColorCellBar; | ||
| } | ||
| if (formatting.skipValueRange) { | ||
| colors.skipValueRange = formatting.skipValueRange; | ||
| } | ||
| } | ||
|
|
||
| const greenRed = lookupRowBasicColor( | ||
| greenRedFormatters, | ||
| columnKey, | ||
| originKey, | ||
| ); | ||
| if (greenRed?.backgroundColor) { | ||
| colors.backgroundColor = greenRed.backgroundColor; | ||
| } | ||
|
|
||
| return colors; | ||
| } |
There was a problem hiding this comment.
The three new pure helper functions — getConditionalFormattingColors (line 118), lookupRowBasicColor (line 176), and resolveCellColorFormatting (line 186) — collectively contain 4 conditional branches and 2 filtering paths with no dedicated unit tests. Per rule 6262, these should be tested in isolation at the unit level, not solely through the integration-style TableChart.test.tsx render tests.
Code Review Run #d79b5e
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
1 participant
SUMMARY
Adds an opt-in control that applies existing conditional formatting rules to the summary row in table charts.
Previously, color rules ran on data cells only. The totals row stayed unformatted. This change introduces a checkbox, Apply conditional formatting to totals, defaulting to off so existing charts keep their current look.
When enabled, the same conditional formatting rules (background / text color) are applied to totals-row metric cells:
Table
Table (AG Grid)
The control is visible only when totals are enabled - Show summary.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Open a Table or Table (AG Grid) chart in Explore, enable Show summary, and add a numeric conditional formatting rule.
Confirm the totals row is unchanged while Apply conditional formatting to totals is off.
Enable the flag and confirm matching totals cells get the same background/text color as data cells.
Save the chart, add it to a dashboard, and confirm the flag persists after reload.
run test
npm run test superset-frontend/plugins/plugin-chart-ag-grid-table/test/utils/getCellStyle.test.ts
npm run test superset-frontend/plugins/plugin-chart-ag-grid-table/test/controlPanel.test.ts
npm run test superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx
ADDITIONAL INFORMATION