You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/09/07 02:30:02 UTC
[PR] fix(chart): include echart_options in Matrixify render-trigger allowlist [superset]
rusackas opened a new pull request, #43946:
URL: https://github.com/apache/superset/pull/43946
### SUMMARY
`StatefulChart` (used to render each Matrixify grid cell) keeps its own hardcoded `RENDER_TRIGGER_SHARED_CONTROLS` allowlist of shared controls that only affect rendering, not data fetching. It exists because string-based control references (e.g. `['echart_options']`) can't be introspected for their `renderTrigger` flag without importing `@superset-ui/chart-controls` into `@superset-ui/core`, which would create a circular dependency.
That allowlist only listed 5 of the 10 controls that `sharedControls.tsx` actually marks `renderTrigger: true`, missing `echart_options`, `color_picker`, `linear_color_scheme`, `x_axis_time_format`, and `x_axis_number_format`. Since the Timeseries Line/Bar/SmoothLine/Area and MixedTimeseries control panels reference `echart_options` as a bare string, `shouldRefetchData()` couldn't tell it apart from a data-affecting control, so editing the "ECharts Options" field on a chart embedded in a Matrixify grid forced a full per-cell data requery instead of a plain re-render (matches the "works outside Matrixify, breaks once it's active" repro in #39008).
This adds the missing entries to the allowlist and a comment noting it needs to stay in sync with `sharedControls.tsx`.
### TESTING INSTRUCTIONS
- Added a unit test in `StatefulChart.test.tsx` that reproduces the bug: with a control panel referencing `echart_options` as a string (as Timeseries charts do), changing `echart_options` should not trigger a data refetch, only a re-render. Fails before this change, passes after.
- `npm run test -- StatefulChart.test.tsx` (from `superset-frontend/`)
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #39008
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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(chart): include echart_options in Matrixify render-trigger allowlist [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43946:
URL: https://github.com/apache/superset/pull/43946#issuecomment-5580789346
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 an incremental AI Review.
- `/review full` - 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(chart): include echart_options in Matrixify render-trigger allowlist [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #43946:
URL: https://github.com/apache/superset/pull/43946#discussion_r3953015978
##########
superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx:
##########
@@ -50,13 +50,21 @@ type LoadingState = 'uninitialized' | 'loading' | 'loaded' | 'error';
* This list is needed because string-based control references (e.g., ['zoomable'])
* cannot be introspected for their renderTrigger property without importing
* sharedControls, which would create a circular dependency.
+ *
+ * Keep this list in sync with the `renderTrigger: true` entries in
+ * @superset-ui/chart-controls's sharedControls.tsx.
*/
const RENDER_TRIGGER_SHARED_CONTROLS = new Set([
'zoomable',
'color_scheme',
'time_shift_color',
'y_axis_format',
'currency_format',
+ 'color_picker',
+ 'linear_color_scheme',
Review Comment:
Good catch — fixed. `shouldRefetchData` now applies the chart's `controlOverrides` on top of the shared-control fallback, so Country Map's `renderTrigger: false` on `linear_color_scheme` still forces a refetch. Added a regression test for 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
Re: [PR] fix(chart): include echart_options in Matrixify render-trigger allowlist [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #43946:
URL: https://github.com/apache/superset/pull/43946
--
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(chart): include echart_options in Matrixify render-trigger allowlist [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #43946:
URL: https://github.com/apache/superset/pull/43946#discussion_r3952406225
##########
superset-frontend/packages/superset-ui-core/src/chart/components/StatefulChart.tsx:
##########
@@ -50,13 +50,21 @@ type LoadingState = 'uninitialized' | 'loading' | 'loaded' | 'error';
* This list is needed because string-based control references (e.g., ['zoomable'])
* cannot be introspected for their renderTrigger property without importing
* sharedControls, which would create a circular dependency.
+ *
+ * Keep this list in sync with the `renderTrigger: true` entries in
+ * @superset-ui/chart-controls's sharedControls.tsx.
*/
const RENDER_TRIGGER_SHARED_CONTROLS = new Set([
'zoomable',
'color_scheme',
'time_shift_color',
'y_axis_format',
'currency_format',
+ 'color_picker',
+ 'linear_color_scheme',
Review Comment:
This now treats every bare `linear_color_scheme` reference as render-only, but Country Map explicitly overrides that control with `renderTrigger: false`. A Country Map Matrixify cell can therefore keep its old query result when this setting changes. Should `shouldRefetchData` honor control overrides before applying the shared fallback?
--
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(chart): include echart_options in Matrixify render-trigger allowlist [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43946:
URL: https://github.com/apache/superset/pull/43946#issuecomment-5564297147
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43946?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 79.80%. Comparing base ([`c03f344`](https://app.codecov.io/gh/apache/superset/commit/c03f3441bd565b9fd5f082d0b875c467eeae441c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`404cf1a`](https://app.codecov.io/gh/apache/superset/commit/404cf1a287d57115a9b7cc0942fb683a461466aa?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 #43946 +/- ##
=======================================
Coverage 79.80% 79.80%
=======================================
Files 2914 2914
Lines 170449 170449
Branches 39512 39512
=======================================
Hits 136035 136035
Misses 31892 31892
Partials 2522 2522
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43946/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/43946/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.35% <ø> (ø)` | |
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/43946?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(chart): include echart_options in Matrixify render-trigger allowlist [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43946:
URL: https://github.com/apache/superset/pull/43946#issuecomment-5576475296
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 41a7b6a71d40f7c00d7bef9a575075496ea3a3c9 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a9f4255677fc500082f1a6b |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43946--superset-docs-preview.netlify.app](https://deploy-preview-43946--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcxNDsIgEEDhu8zaiViFWm_gMSgMOkn5CQPGpvHusnuLl--AUnMsDR4AJ1irTe49OvD3zCKd8LoodUeOZaNIqaHP0lFwDPihyoHJo31ZTtLQ9WF4Klven34oxi7hNmlt5jk4rQY0hYs1K_z-) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43946--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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(chart): include echart_options in Matrixify render-trigger allowlist [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43946:
URL: https://github.com/apache/superset/pull/43946#issuecomment-5564220246
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(chart): include echart_options in Matrixify render-trigger allowlist [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43946:
URL: https://github.com/apache/superset/pull/43946#issuecomment-5575257415
<!-- Bito Reply -->
The current implementation of `RENDER_TRIGGER_SHARED_CONTROLS` in `StatefulChart.tsx` is designed to handle string-based control references that cannot be introspected directly. Since `StatefulChart` relies on this static set to determine whether a change should trigger a data refetch, it does not currently account for individual control overrides like `renderTrigger: false` defined in specific chart configurations. To support this, the logic would need to be updated to check the specific control's configuration if available, rather than relying solely on the shared fallback list.
--
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