codeant-ai-for-open-source[bot] commented on code in PR #38705:
URL: https://github.com/apache/superset/pull/38705#discussion_r2971883760
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1080,12 +1113,16 @@ export class TableRenderer extends Component<
const headerCellFormattedValue =
dateFormatters?.[rowAttrs[i]]?.(r) ?? r;
- const { backgroundColor } = getCellColor(
+ const { backgroundColor, color } = getCellColor(
[rowAttrs[i]],
headerCellFormattedValue,
Review Comment:
**Suggestion:** Row-header color formatting is also evaluated using the
display-formatted value, which can change the value type/content before
`getColorFromValue` runs. This breaks conditional rules for temporal/numeric
row headers because formatters expect raw values from the dataset. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Pivot row-header conditional colors may not apply.
- ⚠️ Date-formatted row labels break comparator rule matching.
- ⚠️ Visual emphasis inconsistent across pivot header axes.
```
</details>
```suggestion
const headerCellRawValue = r;
const headerCellFormattedValue =
dateFormatters?.[rowAttrs[i]]?.(headerCellRawValue) ??
headerCellRawValue;
const { backgroundColor, color } = getCellColor(
[rowAttrs[i]],
headerCellRawValue,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Follow the normal Pivot Table render path: `PivotTableChart` injects
`dateFormatters`
and `cellColorFormatters` into `TableRenderer`
(`src/PivotTableChart.tsx:577-578`).
2. Render row headers in `renderTableRow` (`TableRenderers.tsx:1083+`) where
each row
header value is formatted for display using `dateFormatters`
(`TableRenderers.tsx:1113-1114`).
3. Current code passes that display-formatted row header value to
`getCellColor`
(`TableRenderers.tsx:1116-1119`), which executes formatter comparator logic
through
`getColorFromValue` (`TableRenderers.tsx:202`).
4. For comparator-based rules (from `getColorFormatters.ts:123-157`) the
transformed
display string can stop matching expected raw value semantics, so row-header
conditional
styling does not trigger.
```
</details>
<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-pivot-table/src/react-pivottable/TableRenderers.tsx
**Line:** 1113:1118
**Comment:**
*Logic Error: Row-header color formatting is also evaluated using the
display-formatted value, which can change the value type/content before
`getColorFromValue` runs. This breaks conditional rules for temporal/numeric
row headers because formatters expect raw values from the dataset.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=9c630620148dcfd0664e529eacc74430e1c79fbc4f6bf6bfc1e61a6f35e9dd6b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=9c630620148dcfd0664e529eacc74430e1c79fbc4f6bf6bfc1e61a6f35e9dd6b&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -844,12 +869,16 @@ export class TableRenderer extends Component<
};
const headerCellFormattedValue =
dateFormatters?.[attrName]?.(colKey[attrIdx]) ?? colKey[attrIdx];
- const { backgroundColor } = getCellColor(
+ const { backgroundColor, color } = getCellColor(
[attrName],
headerCellFormattedValue,
Review Comment:
**Suggestion:** Conditional formatters are evaluated against the
display-formatted header value instead of the raw header value. For
temporal/numeric headers, date formatting converts values to strings, so
numeric/date comparator rules in `getColorFromValue` can stop matching and
header formatting silently fails. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Pivot column-header conditional colors can silently fail.
- ⚠️ Temporal header rules mismatch after display date formatting.
- ⚠️ Inconsistent header/body conditional-format behavior in pivot tables.
```
</details>
```suggestion
const headerCellRawValue = colKey[attrIdx];
const headerCellFormattedValue =
dateFormatters?.[attrName]?.(headerCellRawValue) ??
headerCellRawValue;
const { backgroundColor, color } = getCellColor(
[attrName],
headerCellRawValue,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open Pivot Table chart flow where `transformProps` builds both
`dateFormatters` and
`metricColorFormatters` (`src/plugin/transformProps.ts:124-159`), then
`PivotTableChart`
passes them into `tableOptions` (`src/PivotTableChart.tsx:577-578`).
2. Use a temporal grouped column rendered as a column header
(`renderColHeaderRow`) with
conditional formatting operator `>` or `<` (numeric comparators implemented
in
`getColorFormatters.ts:123-157`).
3. In `TableRenderers.tsx:870-875`, header value is first display-formatted
(`dateFormatters`) and that formatted string is then passed into
`getCellColor`, which
calls `formatter.getColorFromValue(...)` (`TableRenderers.tsx:202`).
4. Comparator receives formatted text instead of raw header value; numeric
comparator
checks fail and header conditional color is not applied.
```
</details>
<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-pivot-table/src/react-pivottable/TableRenderers.tsx
**Line:** 870:874
**Comment:**
*Logic Error: Conditional formatters are evaluated against the
display-formatted header value instead of the raw header value. For
temporal/numeric headers, date formatting converts values to strings, so
numeric/date comparator rules in `getColorFromValue` can stop matching and
header formatting silently fails.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=d855902eddc94d88c5cf25efc91458e15cce34e088e4b4ee9c9e3ac3cee32253&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=d855902eddc94d88c5cf25efc91458e15cce34e088e4b4ee9c9e3ac3cee32253&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]
