codeant-ai-for-open-source[bot] commented on code in PR #41390:
URL: https://github.com/apache/superset/pull/41390#discussion_r3535456111
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts:
##########
@@ -703,6 +704,23 @@ const transformProps = (
const basicColorFormatters =
comparisonColorEnabled && getBasicColorFormatter(baseQuery?.data, columns);
+
+ // Attach each row's basic (increase/decrease) color formatter to the row
data
+ // object so it travels with the row through AG Grid client-side sorting.
+ // basicColorFormatters is built in the original query order and was
previously
+ // read positionally by the displayed rowIndex, which applied colors to the
+ // wrong rows once the table was sorted (#105973). The property is
+ // non-enumerable so it never leaks into exports, cross-filters or spreads.
+ if (basicColorFormatters) {
+ passedData.forEach((row, index) => {
+ Object.defineProperty(row, BASIC_COLOR_FORMATTERS_ROW_KEY, {
+ value: basicColorFormatters[index],
+ enumerable: false,
+ configurable: true,
+ writable: true,
+ });
Review Comment:
**Suggestion:** Attaching the formatter under a string key can overwrite
real row data when the query already contains a column named
`__basicColorFormatters__` (or an alias that resolves to that key). That
silently corrupts the row payload and can break rendering/filtering for that
column. Use a collision-proof key (for example a `Symbol`) or guard against
existing user fields before writing this property. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Table V2 charts misrender column __basicColorFormatters__ data.
- ⚠️ Sorting/filtering on colliding column uses wrong values.
- ⚠️ User metric silently replaced by internal formatter metadata.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a Table V2 chart using the AgGridTableChartPlugin (registered in
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/index.ts:74-85`)
on a dataset
whose query or virtual column alias is named `__basicColorFormatters__`, so
each result
row (`DataRecord` from
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts:32`)
contains a field
with that key.
2. Run the chart with a time comparison and enable “Basic conditional
formatting → Green
for increase, red for decrease”, which causes `comparisonColorEnabled` and
`basicColorFormatters` to be truthy; `transformProps` at
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts:34-47`
reads
`baseQuery?.data` into `data` and then `passedData` at lines 53-54,
preserving the user’s
`__basicColorFormatters__` field on each `row`.
3. When `basicColorFormatters` is present, the block at
`transformProps.ts:56-74`
executes; inside it `Object.defineProperty(row,
BASIC_COLOR_FORMATTERS_ROW_KEY, { ... })`
at line 716 redefines the `__basicColorFormatters__` property on each row
object using the
fixed string key from
`superset-frontend/plugins/plugin-chart-ag-grid-table/src/consts.ts:51`,
overwriting any
existing user field with that name and making it non-enumerable.
4. Observe that when the chart renders and AG Grid consumes `data:
passedData` from
`transformProps` (returned at `transformProps.ts:137-140`), the column named
`__basicColorFormatters__` no longer exposes the original metric values:
sorting or
filtering on that column uses the attached formatter object instead of the
real data,
resulting in corrupted values and broken table behavior for that column.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=203550d133704753b9f2619ee92d9f5d&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=203550d133704753b9f2619ee92d9f5d&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:** 716:721
**Comment:**
*Logic Error: Attaching the formatter under a string key can overwrite
real row data when the query already contains a column named
`__basicColorFormatters__` (or an alias that resolves to that key). That
silently corrupts the row payload and can break rendering/filtering for that
column. Use a collision-proof key (for example a `Symbol`) or guard against
existing user fields before writing this property.
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%2F41390&comment_hash=c593c71bdaf15deff3b28dd0573b4112f84e1202600cc5ff1f5b5999e248b274&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41390&comment_hash=c593c71bdaf15deff3b28dd0573b4112f84e1202600cc5ff1f5b5999e248b274&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]
