codeant-ai-for-open-source[bot] commented on code in PR #43181:
URL: https://github.com/apache/superset/pull/43181#discussion_r3786698858
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts:
##########
@@ -219,9 +224,39 @@ export default function transformProps(chartProps:
ChartProps<QueryFormData>) {
config.colorScheme !== ColorSchemeEnum.Green &&
config.colorScheme !== ColorSchemeEnum.Red,
);
+ // Conditional formatting colors the values the pivot renders -- the leaf
+ // cells *and* every subtotal cell -- so the reference distribution a color
+ // scale is built from has to be that same set of values, not the raw rows of
+ // the response. Feeding it raw rows makes the scale disagree with the cells:
+ // the additive path returns leaf rows only, so subtotal cells fall outside
+ // the scale's [min, max] and end up with no color at all even though they
+ // are the largest values in their column, while the GROUPING SETS path
+ // returns every rollup level, so the grand total inflates the domain and
+ // squashes the leaf cells. Either way colors stop tracking value order
+ // within a column. `data` already holds one frame per rollup level, so use
+ // those frames -- minus the levels that only feed the "Total" row/column,
+ // which TableRenderers never color formats.
+ //
+ // The metric pseudo-dimension always occupies one axis, so it is the *other*
+ // axis that decides: a level whose prefix there is fully collapsed lands in
+ // the uncolored total row/column. A dimension list that is empty to begin
+ // with is itself the leaf level, so it still renders as value cells.
+ const displayRows = ensureIsArray(
+ transposePivot ? groupbyColumns : groupbyRows,
+ );
+ const displayColumns = ensureIsArray(
+ transposePivot ? groupbyRows : groupbyColumns,
+ );
+ const isColorFormattedLevel = ({ groupby }: QueryData) =>
+ metricsLayout === MetricsLayoutEnum.ROWS
+ ? groupby.columns.length > 0 || displayColumns.length === 0
+ : groupby.rows.length > 0 || displayRows.length === 0;
+ const colorFormattedValues = data
+ .filter(isColorFormattedLevel)
Review Comment:
**Suggestion:** The predicate includes fully collapsed rollup levels
whenever the opposite display axis is empty. With metrics in rows and no
display columns, `displayColumns.length === 0` makes every level—including
`groupby.rows.length === 0`, which supplies the uncolored Total row—part of
`colorFormattedValues`; with metrics in columns, the analogous condition
includes the uncolored Total column. These values expand the formatter domain
even though `TableRenderers` never calls `getCellColor` for those cells, so
visible leaf and subtotal colors are still based on values that are not
rendered with conditional formatting. Exclude total-only levels (and forced
percent-denominator levels) from the distribution while retaining actual leaf
and subtotal levels. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Pivot colors are distorted when columns are empty.
- ⚠️ Grand totals influence uncolored cell distributions.
- ⚠️ Percent denominator levels can squash visible gradients.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bd034b5426a642acb2e364d1fb18499a&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=bd034b5426a642acb2e364d1fb18499a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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/plugin/transformProps.ts
**Line:** 250:255
**Comment:**
*Incorrect Condition Logic: The predicate includes fully collapsed
rollup levels whenever the opposite display axis is empty. With metrics in rows
and no display columns, `displayColumns.length === 0` makes every
level—including `groupby.rows.length === 0`, which supplies the uncolored Total
row—part of `colorFormattedValues`; with metrics in columns, the analogous
condition includes the uncolored Total column. These values expand the
formatter domain even though `TableRenderers` never calls `getCellColor` for
those cells, so visible leaf and subtotal colors are still based on values that
are not rendered with conditional formatting. Exclude total-only levels (and
forced percent-denominator levels) from the distribution while retaining actual
leaf and subtotal levels.
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%2F43181&comment_hash=de962f2216ae39881c78e1d3568b84179adb366e47a38c5990fb78d62ff7d8a6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43181&comment_hash=de962f2216ae39881c78e1d3568b84179adb366e47a38c5990fb78d62ff7d8a6&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]
