codeant-ai-for-open-source[bot] commented on code in PR #42088:
URL: https://github.com/apache/superset/pull/42088#discussion_r3645746964
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:
##########
@@ -55,29 +55,63 @@ const getCellStyle = (params: CellStyleParams) => {
let backgroundColor;
let color;
if (hasColumnColorFormatters) {
+ const applyFormatter = (
+ formatter: ColorFormatters[number],
+ valueToFormat: typeof value,
+ ) => {
+ const formatterResult =
+ valueToFormat || valueToFormat === 0
+ ? formatter.getColorFromValue(valueToFormat)
+ : false;
+ if (formatterResult) {
+ if (
+ formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR ||
+ formatter.toTextColor
+ ) {
+ color = formatterResult;
+ } else if (
+ formatter.objectFormatting !== ObjectFormattingEnum.CELL_BAR
+ ) {
+ backgroundColor = formatterResult;
+ }
+ }
+ };
+
+ // formatter.column can be a legacy display label ("Main colname") for
+ // time-comparison columns rather than the row's actual data key, so
+ // resolve it to the real field id before using it to read row values.
+ const resolveColumnKey = (columnKey: string) =>
+ columnKey.includes('Main')
+ ? columnKey.replace('Main', '').trim()
+ : columnKey;
Review Comment:
**Suggestion:** The key resolver removes `Main` anywhere in the string, so
legitimate column names containing that substring are mutated and row lookups
read the wrong field (or undefined). Restrict this transformation to the exact
legacy prefix pattern (for example only when the label starts with `Main `)
instead of a broad substring replace. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Column conditional formatting fails for columns containing Main.
- ⚠️ Users see unformatted cells despite valid rules.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Define a dataset and Table V2 chart where a column id or display label
used in
`columnColorFormatters[i].column` contains the substring `Main` as part of a
legitimate
name (for example `Domain Score` or `Customer Main Segment`); this formatter
is then
passed into `getCellStyle` at
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:41`.
2. Render the chart so AG Grid calls `getCellStyle(params)` for each cell;
inside the
`hasColumnColorFormatters` block, the helper `resolveColumnKey` at lines
83–86 is used
both to match the formatter’s target column against `colDef.field` (line 93)
and to read
row data via `node?.data?.[resolveColumnKey(formatter.column)]` (line 97).
3. When `resolveColumnKey` receives a column key containing `Main`, it
executes
`columnKey.replace('Main', '').trim()`, removing `Main` anywhere in the
string, not just
the legacy `Main ` prefix described in the comment at lines 80–82; this
produces a mutated
key that does not exist in `node.data` or as `colDef.field`.
4. Because the mutated key does not match the actual column id,
`filter(...)` at lines
88–93 may fail to select the formatter, and
`node?.data?.[resolveColumnKey(formatter.column)]` at line 97 reads
`undefined`, causing
`applyFormatter` at lines 58–77 to skip applying colors; the configured
conditional
formatting for that column therefore never appears in the rendered table.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f279e63d3f2947fa8e177c7a6113c25f&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=f279e63d3f2947fa8e177c7a6113c25f&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/utils/getCellStyle.ts
**Line:** 83:86
**Comment:**
*Logic Error: The key resolver removes `Main` anywhere in the string,
so legitimate column names containing that substring are mutated and row
lookups read the wrong field (or undefined). Restrict this transformation to
the exact legacy prefix pattern (for example only when the label starts with
`Main `) instead of a broad substring replace.
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%2F42088&comment_hash=a97abdee4926f6959457333707bac9d1cd486a0116e26a8beabbe2c98bbda225&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=a97abdee4926f6959457333707bac9d1cd486a0116e26a8beabbe2c98bbda225&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:
##########
@@ -55,29 +55,63 @@ const getCellStyle = (params: CellStyleParams) => {
let backgroundColor;
let color;
if (hasColumnColorFormatters) {
+ const applyFormatter = (
+ formatter: ColorFormatters[number],
+ valueToFormat: typeof value,
+ ) => {
+ const formatterResult =
+ valueToFormat || valueToFormat === 0
+ ? formatter.getColorFromValue(valueToFormat)
+ : false;
+ if (formatterResult) {
+ if (
+ formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR ||
+ formatter.toTextColor
+ ) {
+ color = formatterResult;
+ } else if (
+ formatter.objectFormatting !== ObjectFormattingEnum.CELL_BAR
+ ) {
+ backgroundColor = formatterResult;
+ }
+ }
+ };
+
+ // formatter.column can be a legacy display label ("Main colname") for
+ // time-comparison columns rather than the row's actual data key, so
+ // resolve it to the real field id before using it to read row values.
+ const resolveColumnKey = (columnKey: string) =>
+ columnKey.includes('Main')
+ ? columnKey.replace('Main', '').trim()
+ : columnKey;
+
columnColorFormatters!
.filter(formatter => {
- const colTitle = formatter?.column?.includes('Main')
- ? formatter?.column?.replace('Main', '').trim()
- : formatter?.column;
- return colTitle === colDef.field;
+ if (formatter.columnFormatting) {
+ return formatter.columnFormatting === colDef.field;
+ }
+ return resolveColumnKey(formatter.column) === colDef.field;
Review Comment:
**Suggestion:** The filter compares a formatting mode field to the current
column id, which are different concepts and types; this causes column
formatters with `columnFormatting` set to be skipped for normal cells. Use the
formatter’s target column key for column matching, and keep `columnFormatting`
only for scope/mode checks (for example ENTIRE_ROW). [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Non-entire-row column formatters silently never apply.
- ⚠️ Users lose expected cell highlighting in Table V2.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Table V2 (AG Grid) chart with a column color formatter whose
`columnFormatting` is set to a non-empty mode (for example a value from
`ObjectFormattingEnum`), and whose `column` targets a visible metric column;
this
formatter will be present in `columnColorFormatters` passed into
`getCellStyle` at
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:41`.
2. Open the dashboard containing this chart so AG Grid renders cells and
calls
`getCellStyle(params)`; inside the function, execution reaches the
`hasColumnColorFormatters` block at lines 57–105, and the formatter is
included in
`columnColorFormatters!`.
3. At lines 88–93, `columnColorFormatters!.filter(...)` runs: because
`formatter.columnFormatting` is truthy, the branch `return
formatter.columnFormatting ===
colDef.field;` (line 91) is used to decide if the formatter applies to the
current column,
comparing a formatting-mode value against `colDef.field` (the column id).
4. This comparison evaluates to false for normal columns, so the formatter
is filtered out
and never passed to `applyFormatter` (lines 95–100); as a result, the
configured
column-level color formatting does not affect any cells even though the rule
is valid and
the column is visible.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=52ee244e0fea41d3b02a69a48520a276&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=52ee244e0fea41d3b02a69a48520a276&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/utils/getCellStyle.ts
**Line:** 90:93
**Comment:**
*Logic Error: The filter compares a formatting mode field to the
current column id, which are different concepts and types; this causes column
formatters with `columnFormatting` set to be skipped for normal cells. Use the
formatter’s target column key for column matching, and keep `columnFormatting`
only for scope/mode checks (for example ENTIRE_ROW).
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%2F42088&comment_hash=19c6e2d8e170dd74f875ada18e92bba53a7981eae3391dfcaf5a89e64f8c44cf&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42088&comment_hash=19c6e2d8e170dd74f875ada18e92bba53a7981eae3391dfcaf5a89e64f8c44cf&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]
