codeant-ai-for-open-source[bot] commented on code in PR #42115:
URL: https://github.com/apache/superset/pull/42115#discussion_r3601207056
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts:
##########
@@ -717,9 +720,32 @@ const transformProps = (
const passedData = isUsingTimeComparison ? comparisonData || [] : data;
const passedColumns = isUsingTimeComparison ? comparisonColumns : columns;
- const basicColorFormatters =
+ // Increase/decrease formatters from the "Comparison color" toggle, keyed by
+ // metric column key.
+ const comparisonColorFormatters =
comparisonColorEnabled && getBasicColorFormatter(baseQuery?.data, columns);
+ // Custom conditional-formatting rules using the Green (increase) / Red
+ // (decrease) color scheme on a time-comparison table. These were computed
but
+ // never consumed by the AG Grid renderer, so the colors/arrows never showed
+ // (the classic plugin-chart-table does consume them). Route them through the
+ // same increase/decrease path, keyed by metric column key (see above).
+ const basicColorColumnFormatters = getBasicColorFormatterForColumn(
+ baseQuery?.data,
+ columns,
+ conditionalFormatting,
+ );
+
+ // Merge both per-row into a single map so the existing row-attached
formatter
+ // drives the renderer for either source.
+ const basicColorFormatters =
+ comparisonColorFormatters || basicColorColumnFormatters
+ ? (baseQuery?.data ?? []).map((_row, index) => ({
+ ...(comparisonColorFormatters || [])[index],
+ ...(basicColorColumnFormatters || [])[index],
+ }))
+ : comparisonColorFormatters;
Review Comment:
**Suggestion:** This merge can produce sparse per-row formatter objects when
custom Green/Red rules target only some metrics. Because AG Grid treats
`basicColorFormatters` as globally enabled for all metric columns, those
missing keys later overwrite previously computed conditional-format backgrounds
with `undefined`, so non-Green/Red formatting disappears on untouched metric
columns. Ensure this path only enables/returns formatter entries for columns
that actually have basic rules, or preserve existing background color when a
row/column formatter key is absent. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ getCellStyle overrides gradient formatters for unaffected metrics.
- ⚠️ Interactive Table time-comparison styling inconsistent for mixed
schemes.
- ⚠️ Users lose non-Green/Red conditional formatting unexpectedly.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure an Interactive Table (Table v2) chart, which renders via
`TableChart` in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTableChart.tsx:55-91`,
using a time comparison so `isUsingTimeComparison` is true in
`transformProps.ts` (see
comparison handling around lines 88-99 and 121-123 in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts`).
2. In the chart’s conditional formatting controls (mapped to the
`conditionalFormatting`
form field consumed in `transformProps.ts:73-78` and
`transformProps.ts:129-138`),
configure: (a) at least one Green/Red (increase/decrease) rule targeting
metric A, and (b)
a non-Green/Red rule (e.g. gradient or alert scheme) targeting metric B.
This produces
both `basicColorColumnFormatters` (for Green/Red) and
`columnColorFormatters` (for the
other scheme).
3. When `transformProps` runs, `basicColorColumnFormatters` is built via
`getBasicColorFormatterForColumn` at `transformProps.ts:59-74` and invoked at
`transformProps.ts:134-138`, returning an array of per-row objects that only
contain keys
for metrics with Green/Red rules (metric A). `columnColorFormatters` is
built from
non-Green/Red rules at `transformProps.ts:71-80`. The merge at
`transformProps.ts:741-747`
creates `basicColorFormatters` as `(baseQuery?.data ?? []).map((_row, index)
=> ({
...(comparisonColorFormatters || [])[index], ...(basicColorColumnFormatters
|| [])[index]
}))`, so many per-row formatter objects are sparse (they omit metric B’s key
entirely).
4. In `AgGridTableChart`, these merged `basicColorFormatters` and
`columnColorFormatters`
are passed into the grid via props at `AgGridTableChart.tsx:81-90`, then
into `useColDefs`
at `useColDefs.ts:179-197`. For every numeric metric column,
`hasBasicColorFormatters` is
set to true whenever the table has any basic formatters (regardless of
whether that
specific metric has a Green/Red rule) at `useColDefs.ts:220-228`. In
`getCellStyle`
(`superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:41-93`),
the function first applies `columnColorFormatters` and may set
`backgroundColor` for
metric B (lines 55-80). It then unconditionally executes the basic-color
path whenever
`hasBasicColorFormatters` is true for that column (lines 83-93), assigning
`backgroundColor = getRowBasicColorFormatter(node, rowIndex,
basicColorFormatters)?.[col.metricName]?.backgroundColor;`. Because the
per-row formatter
object often lacks an entry for metric B (sparse map from step 3), this
assignment sets
`backgroundColor` to `undefined`, overwriting the previously computed
non-Green/Red
background. The returned style at `getCellStyle.ts:106-112` uses
`backgroundColor:
backgroundColor || ''`, so metric B’s conditional formatting silently
disappears whenever
any Green/Red rule exists on the table.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e46696bfdf5d4d199182d77db53525ab&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e46696bfdf5d4d199182d77db53525ab&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
**Line:** 741:747
**Comment:**
*Logic Error: This merge can produce sparse per-row formatter objects
when custom Green/Red rules target only some metrics. Because AG Grid treats
`basicColorFormatters` as globally enabled for all metric columns, those
missing keys later overwrite previously computed conditional-format backgrounds
with `undefined`, so non-Green/Red formatting disappears on untouched metric
columns. Ensure this path only enables/returns formatter entries for columns
that actually have basic rules, or preserve existing background color when a
row/column formatter key is absent.
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42115&comment_hash=1ce53b62348e1cc683a70cfd1e28ff78082f829e051cf25baca236c6a61d724e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42115&comment_hash=1ce53b62348e1cc683a70cfd1e28ff78082f829e051cf25baca236c6a61d724e&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
