You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "luizotavio32 (via GitHub)" <gi...@apache.org> on 2026/09/30 00:13:26 UTC
[PR] fix(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
luizotavio32 opened a new pull request, #44810:
URL: https://github.com/apache/superset/pull/44810
### SUMMARY
Backport of #40173 to 6.2.
On 6.2, when a user zooms into an ECharts chart with the zoom slider and then clicks a value to cross-filter, the chart redraws and the zoom jumps back to the full range. This keeps the zoom range the user had set across redraws.
Cherry-picked cleanly; the only adjustment is Prettier formatting on one line, since 6.2 formats with Prettier rather than oxfmt.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
### TESTING INSTRUCTIONS
Unit tests from the original PR are included.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
๐ค Generated with [Claude Code](https://claude.com/claude-code)
--
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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
Posted by "luizotavio32 (via GitHub)" <gi...@apache.org>.
luizotavio32 commented on PR #44810:
URL: https://github.com/apache/superset/pull/44810#issuecomment-5901918815
> Relying solely on the length of the `dataZoom` array to determine if components match is insufficient, as reordered or replaced components could lead to incorrect viewport restoration by index.
Already addressed in https://github.com/apache/superset/pull/44810#discussion_r4139794808: the dataZoom array has a fixed shape per chart type, so equal length means the same components. No change is needed in this backport.
--
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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
Posted by "luizotavio32 (via GitHub)" <gi...@apache.org>.
luizotavio32 commented on PR #44810:
URL: https://github.com/apache/superset/pull/44810#issuecomment-5901838383
/review
--
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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44810:
URL: https://github.com/apache/superset/pull/44810#issuecomment-5901494046
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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #44810:
URL: https://github.com/apache/superset/pull/44810#issuecomment-5901547066
### <span aria-hidden="true">โ
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">๐จ</span> Latest commit | cb4569a04161203ca8e95a867e83a86469458b1a |
|<span aria-hidden="true">๐</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6abc5427d819360008e77e98 |
|<span aria-hidden="true">๐</span> Deploy Preview | [https://deploy-preview-44810--superset-docs-preview.netlify.app](https://deploy-preview-44810--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#DctJDsIwDADAv_hMIF1IUn7QZziJEYi2tkxUdRF_J7e5zAmiPEuBB8AFouKSXtXP93aLmD7CWkxvG98ZUfqSrmQyFjyYZ-OubT2ZZOJ9zHU5jOnetz6HZuictTaQ9zQE-P0B) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-44810--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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44810:
URL: https://github.com/apache/superset/pull/44810#issuecomment-5901900733
<!-- Bito Reply -->
The flagged issue is correct. Relying solely on the length of the `dataZoom` array to determine if components match is insufficient, as reordered or replaced components could lead to incorrect viewport restoration by index. To resolve this, you should verify that the components themselves are compatible (e.g., by comparing their IDs or types) rather than just their count.
Here is a concise implementation to improve the validation:
```typescript
// Replace the length check with a more robust validation
const newZoom = (chartRef.current?.getOption() as { dataZoom?: DataZoomComponentOption[] })?.dataZoom;
// Ensure both arrays have the same length and verify component identity if possible
if (newZoom?.length === previousZoom.length &&
newZoom.every((dz, i) => dz.id === previousZoom[i].id)) {
// ... proceed with batch dispatch
}
```
There are no other comments available in this PR to review. Would you like me to perform any further analysis?
**superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx**
```
const newZoom = (chartRef.current?.getOption() as { dataZoom?: DataZoomComponentOption[] })?.dataZoom;
// Ensure both arrays have the same length and verify component identity if possible
if (newZoom?.length === previousZoom.length &&
newZoom.every((dz, i) => dz.id === previousZoom[i].id)) {
// ... proceed with batch dispatch
}
```
--
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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44810:
URL: https://github.com/apache/superset/pull/44810#discussion_r4139942958
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -289,12 +290,59 @@ function Echart(
const notMerge = !isDashboardRefreshing;
chartRef.current?.dispatchAction({ type: 'hideTip' });
+ // setOption(notMerge:true) replaces the dataZoom config, dropping any
+ // range the user has engaged. Preserve it across the call.
+ const previousZoom = notMerge
+ ? (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom
+ : undefined;
chartRef.current?.setOption(themedEchartOptions, {
notMerge,
replaceMerge: notMerge ? undefined : ['series'],
// lazyUpdate defers render, causing tooltip crashes on stale shapes (#39247)
lazyUpdate: false,
});
+ if (previousZoom?.length) {
+ // Skip restore when the new option reshapes dataZoom (different count
+ // means index-based restore could land on the wrong component).
+ const newZoom = (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom;
+ if (newZoom?.length === previousZoom.length) {
+ const batch = previousZoom
+ .map((dz, dataZoomIndex) => ({
+ dataZoomIndex,
+ start: dz.start,
+ end: dz.end,
+ startValue: dz.startValue,
+ endValue: dz.endValue,
+ }))
+ .filter(b => {
+ const hasAny =
+ b.start !== undefined ||
+ b.end !== undefined ||
+ b.startValue !== undefined ||
+ b.endValue !== undefined;
+ if (!hasAny) return false;
+ // Default full-range zoom is functionally identical to the
+ // fresh state setOption already produces โ skip the dispatch.
+ const isDefaultRange =
+ b.start === 0 &&
+ b.end === 100 &&
+ b.startValue === undefined &&
+ b.endValue === undefined;
+ return !isDefaultRange;
+ });
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion highlights a potential edge case where the `previousZoom` state might override an intentional `dataZoom` range provided in new `echartOptions`. While the current implementation correctly restores user-dragged ranges, it does not distinguish between a default state and an intentional configuration change. Addressing this by checking if `newZoom` reflects an intentional range or tracking user-initiated zoom events would improve robustness and prevent unintended side effects.
**superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx**
```
if (previousZoom?.length) {
// Skip restore when the new option reshapes dataZoom (different count
// means index-based restore could land on the wrong component).
const newZoom = (
chartRef.current?.getOption() as {
dataZoom?: DataZoomComponentOption[];
}
)?.dataZoom;
```
--
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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
Posted by "luizotavio32 (via GitHub)" <gi...@apache.org>.
luizotavio32 commented on code in PR #44810:
URL: https://github.com/apache/superset/pull/44810#discussion_r4139794808
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -289,12 +290,59 @@ function Echart(
const notMerge = !isDashboardRefreshing;
chartRef.current?.dispatchAction({ type: 'hideTip' });
+ // setOption(notMerge:true) replaces the dataZoom config, dropping any
+ // range the user has engaged. Preserve it across the call.
+ const previousZoom = notMerge
+ ? (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom
+ : undefined;
chartRef.current?.setOption(themedEchartOptions, {
notMerge,
replaceMerge: notMerge ? undefined : ['series'],
// lazyUpdate defers render, causing tooltip crashes on stale shapes (#39247)
lazyUpdate: false,
});
+ if (previousZoom?.length) {
+ // Skip restore when the new option reshapes dataZoom (different count
+ // means index-based restore could land on the wrong component).
+ const newZoom = (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom;
+ if (newZoom?.length === previousZoom.length) {
+ const batch = previousZoom
+ .map((dz, dataZoomIndex) => ({
+ dataZoomIndex,
+ start: dz.start,
+ end: dz.end,
+ startValue: dz.startValue,
+ endValue: dz.endValue,
Review Comment:
Not an issue in practice: within a mounted chart the `dataZoom` array has a fixed shape per chart type. For Timeseries (`transformProps.ts`), a zoomable chart always emits the same three components in the same order (slider, inside-y, inside-x); disabling zoom removes them all, which changes the length and skips the restore. The only same-length change is flipping orientation, which moves the slider between x and y, but in both cases it targets the category axis, so the restored range still lands on the right axis. Changing viz type remounts the chart, so nothing is carried over.
Matching by type/axis instead of index would be a robustness refactor, not a behavior fix. This PR is a straight backport of #40173 and master has the identical check, so any change there belongs in a separate PR against master.
--
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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #44810:
URL: https://github.com/apache/superset/pull/44810#discussion_r4139882946
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -289,12 +290,59 @@ function Echart(
const notMerge = !isDashboardRefreshing;
chartRef.current?.dispatchAction({ type: 'hideTip' });
+ // setOption(notMerge:true) replaces the dataZoom config, dropping any
+ // range the user has engaged. Preserve it across the call.
+ const previousZoom = notMerge
+ ? (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom
+ : undefined;
chartRef.current?.setOption(themedEchartOptions, {
notMerge,
replaceMerge: notMerge ? undefined : ['series'],
// lazyUpdate defers render, causing tooltip crashes on stale shapes (#39247)
lazyUpdate: false,
});
+ if (previousZoom?.length) {
+ // Skip restore when the new option reshapes dataZoom (different count
+ // means index-based restore could land on the wrong component).
+ const newZoom = (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom;
+ if (newZoom?.length === previousZoom.length) {
+ const batch = previousZoom
+ .map((dz, dataZoomIndex) => ({
+ dataZoomIndex,
+ start: dz.start,
+ end: dz.end,
+ startValue: dz.startValue,
+ endValue: dz.endValue,
+ }))
+ .filter(b => {
+ const hasAny =
+ b.start !== undefined ||
+ b.end !== undefined ||
+ b.startValue !== undefined ||
+ b.endValue !== undefined;
+ if (!hasAny) return false;
+ // Default full-range zoom is functionally identical to the
+ // fresh state setOption already produces โ skip the dispatch.
+ const isDefaultRange =
+ b.start === 0 &&
+ b.end === 100 &&
+ b.startValue === undefined &&
+ b.endValue === undefined;
+ return !isDefaultRange;
+ });
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Stale zoom overrides new config</b></div>
<div id="fix">
The same-count guard doesn't catch same-count config changes: if `echartOptions` arrives with a deliberately different `dataZoom` range (e.g. an explore control edit), the stale interactive range in `previousZoom` silently overrides it. Consider skipping the restore when `newZoom` already reflects an intentional range, or tracking user-initiated zoom via `datazoom` events; this also avoids redundant `dispatchAction` event side effects.
</div>
</div>
<small><i>Code Review Run #7ebc2e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #44810:
URL: https://github.com/apache/superset/pull/44810
--
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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [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 #44810:
URL: https://github.com/apache/superset/pull/44810#discussion_r4139786575
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -289,12 +290,59 @@ function Echart(
const notMerge = !isDashboardRefreshing;
chartRef.current?.dispatchAction({ type: 'hideTip' });
+ // setOption(notMerge:true) replaces the dataZoom config, dropping any
+ // range the user has engaged. Preserve it across the call.
+ const previousZoom = notMerge
+ ? (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom
+ : undefined;
chartRef.current?.setOption(themedEchartOptions, {
notMerge,
replaceMerge: notMerge ? undefined : ['series'],
// lazyUpdate defers render, causing tooltip crashes on stale shapes (#39247)
lazyUpdate: false,
});
+ if (previousZoom?.length) {
+ // Skip restore when the new option reshapes dataZoom (different count
+ // means index-based restore could land on the wrong component).
+ const newZoom = (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom;
+ if (newZoom?.length === previousZoom.length) {
+ const batch = previousZoom
+ .map((dz, dataZoomIndex) => ({
+ dataZoomIndex,
+ start: dz.start,
+ end: dz.end,
+ startValue: dz.startValue,
+ endValue: dz.endValue,
Review Comment:
**Suggestion:** Equal lengths do not prove matching dataZoom components. Reordered or replaced components receive the previous range by index, moving the viewport to the wrong component.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Rarely` ยท ๐ท๏ธ `Logic error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9ad1a7f905db4cb4941a001a3b85c662&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=9ad1a7f905db4cb4941a001a3b85c662&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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-echarts/src/components/Echart.tsx
**Line:** 316:323
**Comment:**
*Logic Error: Equal lengths do not prove matching dataZoom components. Reordered or replaced components receive the previous range by index, moving the viewport to the wrong component.
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%2F44810&comment_hash=aea3e3c38dd75ccce1c85d79239362de4ebd467590d4e1960c34c7c1d1205c1d&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F44810&comment_hash=aea3e3c38dd75ccce1c85d79239362de4ebd467590d4e1960c34c7c1d1205c1d&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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [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 #44810:
URL: https://github.com/apache/superset/pull/44810#discussion_r4139796623
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -289,12 +290,59 @@ function Echart(
const notMerge = !isDashboardRefreshing;
chartRef.current?.dispatchAction({ type: 'hideTip' });
+ // setOption(notMerge:true) replaces the dataZoom config, dropping any
+ // range the user has engaged. Preserve it across the call.
+ const previousZoom = notMerge
+ ? (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom
+ : undefined;
chartRef.current?.setOption(themedEchartOptions, {
notMerge,
replaceMerge: notMerge ? undefined : ['series'],
// lazyUpdate defers render, causing tooltip crashes on stale shapes (#39247)
lazyUpdate: false,
});
+ if (previousZoom?.length) {
+ // Skip restore when the new option reshapes dataZoom (different count
+ // means index-based restore could land on the wrong component).
+ const newZoom = (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom;
+ if (newZoom?.length === previousZoom.length) {
+ const batch = previousZoom
+ .map((dz, dataZoomIndex) => ({
+ dataZoomIndex,
+ start: dz.start,
+ end: dz.end,
+ startValue: dz.startValue,
+ endValue: dz.endValue,
Review Comment:
โ
**Customized review instruction saved!**
**Instruction:**
> Do not flag equal-length dataZoom restoration as a logic error when chart types emit a fixed, ordered dataZoom shape and remounting prevents state carryover; matching by type or axis is only a separate robustness refactor.
**Applied to:**
- `superset-frontend/plugins/plugin-chart-echarts/**`
---
๐ก *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #44810:
URL: https://github.com/apache/superset/pull/44810#issuecomment-5901840011
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(echarts): backport preserve dataZoom range across setOption to 6.2 (#40173) [superset]
Posted by "luizotavio32 (via GitHub)" <gi...@apache.org>.
luizotavio32 commented on code in PR #44810:
URL: https://github.com/apache/superset/pull/44810#discussion_r4139942161
##########
superset-frontend/plugins/plugin-chart-echarts/src/components/Echart.tsx:
##########
@@ -289,12 +290,59 @@ function Echart(
const notMerge = !isDashboardRefreshing;
chartRef.current?.dispatchAction({ type: 'hideTip' });
+ // setOption(notMerge:true) replaces the dataZoom config, dropping any
+ // range the user has engaged. Preserve it across the call.
+ const previousZoom = notMerge
+ ? (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom
+ : undefined;
chartRef.current?.setOption(themedEchartOptions, {
notMerge,
replaceMerge: notMerge ? undefined : ['series'],
// lazyUpdate defers render, causing tooltip crashes on stale shapes (#39247)
lazyUpdate: false,
});
+ if (previousZoom?.length) {
+ // Skip restore when the new option reshapes dataZoom (different count
+ // means index-based restore could land on the wrong component).
+ const newZoom = (
+ chartRef.current?.getOption() as {
+ dataZoom?: DataZoomComponentOption[];
+ }
+ )?.dataZoom;
+ if (newZoom?.length === previousZoom.length) {
+ const batch = previousZoom
+ .map((dz, dataZoomIndex) => ({
+ dataZoomIndex,
+ start: dz.start,
+ end: dz.end,
+ startValue: dz.startValue,
+ endValue: dz.endValue,
+ }))
+ .filter(b => {
+ const hasAny =
+ b.start !== undefined ||
+ b.end !== undefined ||
+ b.startValue !== undefined ||
+ b.endValue !== undefined;
+ if (!hasAny) return false;
+ // Default full-range zoom is functionally identical to the
+ // fresh state setOption already produces โ skip the dispatch.
+ const isDefaultRange =
+ b.start === 0 &&
+ b.end === 100 &&
+ b.startValue === undefined &&
+ b.endValue === undefined;
+ return !isDefaultRange;
+ });
Review Comment:
Not an issue in practice: no chart config can carry an intentional zoom range. Every zoomable ECharts chart (Timeseries, Mixed Timeseries, Gantt) always emits the fixed default `start: 0, end: 100` from `TIMESERIES_CONSTANTS`, and there is no Explore control for the range. So an incoming `echartOptions` never has a deliberate range to override; the only non-default range is the one the user dragged, which is exactly what this restores. Nothing in Superset subscribes to `datazoom` events either, so the restore dispatch has no side effects.
This is a straight backport of #40173 and master has the identical logic, so any change there belongs in a separate PR against master.
--
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