codeant-ai-for-open-source[bot] commented on code in PR #38705:
URL: https://github.com/apache/superset/pull/38705#discussion_r2957129625
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/utils/getCellStyle.ts:
##########
@@ -72,9 +90,28 @@ const getCellStyle = (params: CellStyleParams) => {
const textAlign =
col?.config?.horizontalAlign || (col?.isNumeric ? 'right' : 'left');
+ const resolvedTextColor = getTextColorForBackground(
+ { backgroundColor, color },
+ cellSurfaceColor,
+ );
+ const hoverResolvedTextColor = getTextColorForBackground(
+ { backgroundColor, color },
+ hoverCellSurfaceColor,
+ );
return {
backgroundColor: backgroundColor || '',
+ ...(resolvedTextColor
+ ? {
+ color: 'var(--ag-cell-value-color)',
+ '--ag-cell-value-color': resolvedTextColor,
+ }
+ : {}),
+ ...(hoverResolvedTextColor
+ ? {
+ '--ag-cell-value-hover-color': hoverResolvedTextColor,
+ }
+ : {}),
Review Comment:
**Suggestion:** The new conditional spreads only set `--ag-cell-value-color`
/ `--ag-cell-value-hover-color` when a formatter resolves a color, but they do
not clear those CSS variables when later cells have no formatter match. In AG
Grid, cell DOM nodes are reused and style keys are applied incrementally, so
stale CSS custom properties can leak text colors into unrelated cells. Always
write these style keys (and reset them to empty values when unresolved) so
formatting cannot persist across recycled cells. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ AG Grid table text color can appear incorrect.
- ⚠️ Hover text color may leak between unrelated rows.
- ⚠️ Conditional formatting consistency degrades during scrolling/sorting.
```
</details>
```suggestion
color: resolvedTextColor ? 'var(--ag-cell-value-color)' : '',
'--ag-cell-value-color': resolvedTextColor || '',
'--ag-cell-value-hover-color': hoverResolvedTextColor || '',
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open AG Grid Table chart rendering path: `AgGridTableChart.tsx:227`
builds `colDefs`
via `useColDefs()`, then `useColDefs.ts:285-307` assigns per-cell
`cellStyle: p =>
getCellStyle(...)`.
2. Configure a conditional formatter that only colors some values (same
behavior as
formatter callbacks used in code/tests: `getColorFromValue(value)` may
return undefined
for non-matching cells), so some cells resolve text color and others do not.
3. For colored cells, `getCellStyle.ts:104-113` returns CSS vars
`--ag-cell-value-color`/`--ag-cell-value-hover-color`; for non-colored
cells, those keys
are omitted entirely.
4. Because rendered text color is driven by CSS vars in
`styles/index.tsx:313-320`
(`color: var(--ag-cell-value-color, inherit)`), when AG Grid reuses cell DOM
during
virtualization/scroll/sort, omitted keys are not explicitly cleared, so
prior variable
values can persist and appear on unrelated cells.
```
</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-ag-grid-table/src/utils/getCellStyle.ts
**Line:** 104:114
**Comment:**
*Possible Bug: The new conditional spreads only set
`--ag-cell-value-color` / `--ag-cell-value-hover-color` when a formatter
resolves a color, but they do not clear those CSS variables when later cells
have no formatter match. In AG Grid, cell DOM nodes are reused and style keys
are applied incrementally, so stale CSS custom properties can leak text colors
into unrelated cells. Always write these style keys (and reset them to empty
values when unresolved) so formatting cannot persist across recycled cells.
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=5b407ae561a629ed834bd63aedc656b252016a12961765ec603517aec23292e4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38705&comment_hash=5b407ae561a629ed834bd63aedc656b252016a12961765ec603517aec23292e4&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]
