You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "amaannawab923 (via GitHub)" <gi...@apache.org> on 2026/06/24 12:56:20 UTC
[PR] fix(plugin-chart-ag-grid-table): keep basic conditional formatting aligned after sort [superset]
amaannawab923 opened a new pull request, #41390:
URL: https://github.com/apache/superset/pull/41390
### SUMMARY
With time-comparison "basic" conditional formatting (green for increase / red for decrease), sorting the interactive table made the colors and arrows land on the wrong rows — they kept reflecting the pre-sort order.
The basic color formatters are computed in the original query order, but they were being looked up by the grid's *displayed* row index. Once you sort, displayed row N no longer points at the same underlying record, so the formatter for one row ended up painted on another.
The fix attaches each row's formatter to its own row-data object (non-enumerable, so it never leaks into CSV exports, cross-filters or object spreads) and resolves it from the node's data instead of the display index — so it travels with the row through sorting. There's still a positional fallback for safety. Both the cell background (`getCellStyle`) and the arrow indicator (`NumericCellRenderer`) now go through the same helper.
### TESTING INSTRUCTIONS
1. Create an interactive table with a dimension and a metric, apply a time-shift comparison.
2. Enable **Basic conditional formatting → Green for increase, red for decrease**.
3. Sort the dimension column ascending/descending.
4. The increase/decrease colors and arrows should stay attached to the correct rows after sorting.
Unit tests added for the row-formatter resolver (uses the attached formatter over the display index, falls back positionally, and confirms the attached property is non-enumerable).
### ADDITIONAL INFORMATION
- [x] Changes UI
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ag-grid): keep basic conditional formatting aligned after sort [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41390:
URL: https://github.com/apache/superset/pull/41390#issuecomment-4789601512
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41390?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 64.42%. Comparing base ([`a57b5f6`](https://app.codecov.io/gh/apache/superset/commit/a57b5f6078056e411278cbb93d3595d4cdebe62a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`348d924`](https://app.codecov.io/gh/apache/superset/commit/348d924c92a503014b6b7f3cf1a8cc2c17d72db9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41390 +/- ##
=======================================
Coverage 64.42% 64.42%
=======================================
Files 2654 2655 +1
Lines 145386 145398 +12
Branches 33557 33560 +3
=======================================
+ Hits 93658 93670 +12
Misses 50031 50031
Partials 1697 1697
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41390/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/41390/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `68.73% <100.00%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41390?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ag-grid): keep basic conditional formatting aligned after sort [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41390:
URL: https://github.com/apache/superset/pull/41390#issuecomment-4903498114
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(plugin-chart-ag-grid-table): keep basic conditional formatting aligned after sort [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41390:
URL: https://github.com/apache/superset/pull/41390#issuecomment-4789515372
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ag-grid): keep basic conditional formatting aligned after sort [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #41390:
URL: https://github.com/apache/superset/pull/41390#issuecomment-4834297025
## EnxDev's Review Agent — apache/superset#41390 · HEAD 348d924
**request changes** — the fix approach is sound, but it breaks the TypeScript build (CI red on `lint-frontend` + `validate-frontend`).
The diagnosis and design are good: attaching the formatter to the row-data object (non-enumerable, resolved from `node.data`) is the right way to make it survive client-side sorting, and the index alignment matches what the old positional lookup already assumed at transform time. One blocking type error stops it from compiling.
### 🔴 Functional
- **`src/renderers/NumericCellRenderer.tsx:172-173`** · _High_ — `getRowBasicColorFormatter()` returns `RowFormatters | undefined`, so `rowFormatter?.mainArrow` / `rowFormatter?.arrowColor?.toLowerCase()` are `string | undefined`, assigned to `arrow`/`arrowColor` which are typed `string` (`let arrow = ''`). `tsc --build` fails with TS2322 (confirmed: both `lint-frontend` and `validate-frontend` are red). The old expression resolved to `string` because every link in the chain was non-nullable, so the new helper's `| undefined` is what broke it. Fix: `arrow = rowFormatter?.mainArrow ?? '';` and `arrowColor = rowFormatter?.arrowColor?.toLowerCase() ?? '';`. **regression test:** the build itself — CI must go green; the existing jest suite already passed, so the only gap is type-checking.
### 🟡 Should-fix
- **`test/utils/getRowBasicColorFormatter.test.ts:26`** — `const positional = [green, red] as any;` uses `any`, which the project bans. Type it as `RowFormatters[]` (or the helper's param type) so the test also guards the signature.
### 🙌 Praise
- `src/utils/getRowBasicColorFormatter.ts` / `transformProps.ts:704` — non-enumerable attach is a clean choice: keeps the formatter travelling with the row through sort while staying out of CSV exports, cross-filters, and spreads, and the positional fallback is a sensible safety net. Test for non-enumerability is a nice touch.
<!-- enxdev-review-agent:348d924 -->
_Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 348d924._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ag-grid): keep basic conditional formatting aligned after sort [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ag-grid): keep basic conditional formatting aligned after sort [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev merged PR #41390:
URL: https://github.com/apache/superset/pull/41390
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ag-grid): keep basic conditional formatting aligned after sort [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41390:
URL: https://github.com/apache/superset/pull/41390#issuecomment-4902580159
<!-- Bito Reply -->
The flagged issue is correct. Using a fixed string key for `Object.defineProperty` can indeed cause collisions if the user's dataset already contains a field named `__basicColorFormatters__`. To resolve this, you should use a `Symbol` as the key, which is guaranteed to be unique and will not collide with any user-defined string keys.
Here is the corrected implementation for `superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts`:
```typescript
// Define a unique symbol for the formatter key
export const BASIC_COLOR_FORMATTERS_ROW_KEY = Symbol('basicColorFormatters');
// ... inside transformProps ...
if (basicColorFormatters) {
passedData.forEach((row, index) => {
Object.defineProperty(row, BASIC_COLOR_FORMATTERS_ROW_KEY, {
value: basicColorFormatters[index],
enumerable: false,
configurable: true,
writable: true,
});
});
}
```
I have checked the PR comments, but there are no other comments available in the provided context to address. If you have other comments you would like me to review, please provide them.
**superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts**
```
if (basicColorFormatters) {
passedData.forEach((row, index) => {
Object.defineProperty(row, BASIC_COLOR_FORMATTERS_ROW_KEY, {
value: basicColorFormatters[index],
enumerable: false,
configurable: true,
writable: true,
});
});
}
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(ag-grid): keep basic conditional formatting aligned after sort [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41390:
URL: https://github.com/apache/superset/pull/41390#issuecomment-4905740602
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org