You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "manojshetty2004 (via GitHub)" <gi...@apache.org> on 2026/03/15 16:04:08 UTC
[PR] fix(echarts-timeseries): make dataZoom slider bottom offset responsive (#37286) [superset]
manojshetty2004 opened a new pull request, #38657:
URL: https://github.com/apache/superset/pull/38657
### SUMMARY
This change makes the `dataZoom` slider bottom offset responsive to the chart height in ECharts time-series charts.
Previously, the slider used a fixed offset:
TIMESERIES_CONSTANTS.zoomBottom
In smaller chart panels or dashboards with limited height, this could cause the zoom slider to appear clipped or overflow outside the visible chart area.
This PR replaces the fixed offset with a responsive calculation based on the chart height:
Math.max(TIMESERIES_CONSTANTS.zoomBottom, Math.floor(height * 0.08))
This ensures:
- The zoom slider keeps the existing minimum spacing.
- The control adapts to smaller panel heights.
- The zoom control remains visible and correctly positioned.
---
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
The zoom slider could overflow or appear clipped when the chart panel height is small.
After:
The zoom slider position adjusts dynamically based on chart height and remains visible.
---
### TESTING INSTRUCTIONS
1. Run Superset locally.
2. Open **Explore β ECharts Time-series chart**.
3. Enable **Zoomable**.
4. Reduce the chart height or place the chart in a smaller dashboard panel.
5. Verify that the zoom slider remains visible and positioned correctly.
---
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #37286
- [ ] Required feature flags
- [x] 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(echarts-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4100911848
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(echarts-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4162239914
Superset uses Git pre-commit hooks courtesy of [pre-commit](https://pre-commit.com/). To install run the following:
pip3 install -r requirements/development.txt
pre-commit install
A series of checks will now run when you make a git commit.
Alternatively it is possible to run pre-commit by running pre-commit manually:
pre-commit run --all-files
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "manojshetty2004 (via GitHub)" <gi...@apache.org>.
manojshetty2004 commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4180483288
@SBIN2010 @EnxDev @sadpandajoe
All checks are passing now
The earlier Playwright failure appeared to be a flaky CI issue. I re-ran the workflow and confirmed that all tests pass consistently.
Happy to address any feedback or make further improvements if needed.
https://github.com/user-attachments/assets/0c202698-a150-4f8f-9a1e-c6183990b87e
Before: dataZoom slider overlaps or appears too close to the X-axis on smaller layouts.
After: responsive bottom offset ensures proper spacing and avoids overlap.
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4103397664
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | 956de686402dc049abf381df4b6b082f44ccdd7c |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69bea30b0750060008a92e85 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-38657--superset-docs-preview.netlify.app](https://deploy-preview-38657--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> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-38657--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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4387556149
Hey @manojshetty2004 could you add a before/after video in the description?
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "manojshetty2004 (via GitHub)" <gi...@apache.org>.
manojshetty2004 commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4810702983
> Hey @manojshetty2004 could you add a before/after video in the description?
Hi! The before/after video has already been added to the description. Please let me know if you'd like me to update it further or if there's a specific section you'd like me to highlight.
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "manojshetty2004 (via GitHub)" <gi...@apache.org>.
manojshetty2004 commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4097846595
@sadpandajoe
Thanks for the suggestion. I wasnβt able to reliably reproduce the exact clipping issue locally, but I validated the change by testing with smaller chart heights where the zoom slider area becomes constrained. The dynamic bottom offset ensures additional spacing is allocated, preventing potential overlap or clipping in tighter layouts.
Please let me know if youβd like me to try a specific scenario or dataset to better demonstrate the issue.
--
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-timeseries): make dataZoom slider bottom offset responsive (#37286) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4063290001
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4110187240
HI @manojshetty2004, would it make sense to add before-and-after videos and some tests?
--
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-timeseries): make dataZoom slider bottom offset responsive [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 #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3482214631
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1662,6 +1662,35 @@ test('should assign distinct dash patterns for multiple time offsets consistentl
expect(symbol1).not.toEqual(symbol2);
});
+test('should adjust dataZoom bottom when chart height changes', () => {
+ const smallChart = createTestChartProps({
+ height: 300,
+ formData: { zoomable: true },
Review Comment:
**Suggestion:** This test gives false confidence for the reported bug because the "small" case (`height: 300`) still hits the fixed minimum branch (`30`) and never validates behavior for truly small panels where clipping was reported. Add an assertion using a smaller height that exercises the intended small-chart path and verifies the expected direction of change (not just inequality). [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ Test misses small-height behavior of dataZoom bottom offset.
- β οΈ Regression in small chart layout may go undetected.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. In
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:112-137`,
`createTestChartProps` builds `EchartsTimeseriesChartProps` using `DEFAULT_FORM_DATA` and
a passed `height`, exercising the same transform as production charts.
2. The new test at `transformProps.test.ts:1665-1693` constructs `smallChart` with
`height: 300` and `largeChart` with `height: 600`, both `zoomable: true`, then calls
`transformProps` and inspects `echartOptions.dataZoom[0].bottom`.
3. Using the implementation in `Timeseries/transformProps.ts:1018-1021` and
`constants.ts:33-55`, compute `dynamicZoomBottom`: for `height = 300`, `Math.floor(300 *
0.08) = 24`, so `bottom = max(30, 24) = 30`; for `height = 600`, `Math.floor(600 * 0.08) =
48`, so `bottom = 48`. The assertion `expect(smallBottom).not.toEqual(largeBottom)`
therefore passes while the "small" case still uses the old fixed offset.
4. Because the test never exercises the truly small-height range where clipping was
reported (e.g., `height < 100` used elsewhere in the suite for compact behavior) and does
not assert the expected direction of change (e.g., that small charts move the slider
further inside the visible area), it can pass even if the small-panel clipping bug is not
fixed, giving false confidence about the regression the PR aims to guard.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=24d0a04fe1334d32baf05fe0919f6ecd&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=24d0a04fe1334d32baf05fe0919f6ecd&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-echarts/test/Timeseries/transformProps.test.ts
**Line:** 1666:1668
**Comment:**
*Incomplete Implementation: This test gives false confidence for the reported bug because the "small" case (`height: 300`) still hits the fixed minimum branch (`30`) and never validates behavior for truly small panels where clipping was reported. Add an assertion using a smaller height that exercises the intended small-chart path and verifies the expected direction of change (not just inequality).
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%2F38657&comment_hash=252a351da1d997957be10f3527b394150d534a728106cdb5626c36e1b652ced9&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=252a351da1d997957be10f3527b394150d534a728106cdb5626c36e1b652ced9&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1015,6 +1015,11 @@ export default function transformProps(
}
}
+ const dynamicZoomBottom = Math.max(
+ TIMESERIES_CONSTANTS.zoomBottom,
+ Math.floor(height * 0.08),
+ );
Review Comment:
**Suggestion:** The responsive offset calculation is inverted for the stated goal: `Math.max` keeps the old fixed value for shorter charts and only increases spacing on taller charts. With `zoomBottom = 30`, any chart height under 375px still gets `30`, so small panels remain non-responsive and can still clip. Use a formula that actually changes behavior for smaller heights (e.g., clamp with `Math.min` or otherwise reduce the offset as height shrinks). [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ Small-height echarts_timeseries charts keep non-responsive zoom slider.
- β οΈ Zoom slider clipping persists in compact dashboard panels.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. In
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:181-276`,
note that `transformProps` reads `height` from `chartProps` and uses it later to drive
layout and zoom controls.
2. At `constants.ts:33-55`, observe `TIMESERIES_CONSTANTS.zoomBottom = 30` and the
compact/micro chart thresholds (`compactChartHeight = 100`, `microChartHeight = 60`),
showing the intent to adapt behavior for small heights.
3. At `Timeseries/transformProps.ts:1018-1021`, `dynamicZoomBottom` is computed as
`Math.max(TIMESERIES_CONSTANTS.zoomBottom, Math.floor(height * 0.08))`. For any `height <
375` (since `30 / 0.08 = 375`), `Math.floor(height * 0.08) < 30`, so `dynamicZoomBottom`
resolves to `30`, identical to the old fixed offset.
4. At `Timeseries/transformProps.ts:1181-1188`, the dataZoom slider config uses `bottom:
dynamicZoomBottom`. For zoomable small charts (e.g., `height = 80` or `height = 300` used
throughout `transformProps.test.ts`), the slider still gets `bottom = 30`, meaning the
PR's "responsive to smaller panel heights" behavior never triggers for the actual
small-height range where clipping was reported.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=625e23399a4845bd9fbc893f84dfe450&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=625e23399a4845bd9fbc893f84dfe450&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-echarts/src/Timeseries/transformProps.ts
**Line:** 1018:1021
**Comment:**
*Logic Error: The responsive offset calculation is inverted for the stated goal: `Math.max` keeps the old fixed value for shorter charts and only increases spacing on taller charts. With `zoomBottom = 30`, any chart height under 375px still gets `30`, so small panels remain non-responsive and can still clip. Use a formula that actually changes behavior for smaller heights (e.g., clamp with `Math.min` or otherwise reduce the offset as height shrinks).
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%2F38657&comment_hash=973bcc47cdaef6c0f2919dfd92a559ab7bb707391db017f0993c1cb8f600a0d0&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=973bcc47cdaef6c0f2919dfd92a559ab7bb707391db017f0993c1cb8f600a0d0&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-timeseries): make dataZoom slider bottom offset responsive [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 #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3006206515
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -947,6 +947,11 @@ export default function transformProps(
}
}
+ const dynamicZoomBottom = Math.min(
Review Comment:
**Suggestion:** The responsive zoom slider offset uses `Math.min` instead of `Math.max`, which reduces the bottom offset below the intended minimum on short charts and can cause the slider to be closer to (or clipped by) the chart edge instead of preserving a minimum spacing while growing with height. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ ECharts Time-series zoom slider too close to bottom.
- β οΈ Zoom slider can be partially clipped in short panels.
- β οΈ Degraded usability of zoom control in Explore.
```
</details>
```suggestion
const dynamicZoomBottom = Math.max(
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Render an ECharts Time-series chart using the `EchartsTimeseriesChartPlugin` defined in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/index.ts:34-74`, which
wires `transformProps` from `./transformProps` into the plugin.
2. Ensure the chart is configured with zoom enabled so that `zoomable` is true and passed
into `transformProps` (see the `zoomable` field in the `EchartsTimeseriesFormData`
destructuring in `Timeseries/transformProps.ts:217-276` and the `dataZoom` configuration
at `Timeseries/transformProps.ts:1108-1129`).
3. Place the chart in a short container (for example a small dashboard row or a resized
Storybook demo) so that the `height` prop passed into `transformProps` (see
`Timeseries/transformProps.ts:181-197` and `Timeseries/EchartsTimeseries.tsx:41-61`) is
relatively small, e.g. around 200 pixels.
4. When `transformProps` runs, it computes `dynamicZoomBottom` as
`Math.min(TIMESERIES_CONSTANTS.zoomBottom, Math.floor(height * 0.08))` at
`Timeseries/transformProps.ts:950-953`, with `TIMESERIES_CONSTANTS.zoomBottom` defined as
`30` in `plugins/plugin-chart-echarts/src/constants.ts:33-42`. For `height = 200`, this
yields `Math.min(30, 16) = 16`, which is *less* than the intended minimum spacing of 30.
This value is then used as `bottom: dynamicZoomBottom` for the slider dataZoom component
at `Timeseries/transformProps.ts:1110-1115`, causing the slider to render closer to (or
partially clipped by) the bottom edge of the chart container in short charts instead of
preserving a minimum spacing. Using `Math.max` as described in the PR text would keep the
offset at least `zoomBottom` and avoid this regression.
```
</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-echarts/src/Timeseries/transformProps.ts
**Line:** 950:950
**Comment:**
*Logic Error: The responsive zoom slider offset uses `Math.min` instead of `Math.max`, which reduces the bottom offset below the intended minimum on short charts and can cause the slider to be closer to (or clipped by) the chart edge instead of preserving a minimum spacing while growing with height.
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%2F38657&comment_hash=22126d0fb8575f2bfe2edf0286323fc89622e28cff737d6c6b7ef7add4260dd8&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=22126d0fb8575f2bfe2edf0286323fc89622e28cff737d6c6b7ef7add4260dd8&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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3602399156
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1837,3 +1867,26 @@ describe('Tooltip with long labels', () => {
expect(result).toContain('599616000000');
});
});
+
+test('should adjust dataZoom bottom when chart height changes', () => {
+ const smallChart = createTestChartProps({
+ height: 300,
+ formData: { zoomable: true },
+ });
+ const largeChart = createTestChartProps({
+ height: 600,
+ formData: { zoomable: true },
+ });
+
+ const smallResult = transformProps(smallChart);
+ const largeResult = transformProps(largeChart);
+
+ const smallBottom = (smallResult.echartOptions.dataZoom as any[])[0].bottom;
+ const largeBottom = (largeResult.echartOptions.dataZoom as any[])[0].bottom;
+
+ expect(smallBottom).toBeDefined();
+ expect(largeBottom).toBeDefined();
+
+ // Bottom should vary with height
+ expect(smallBottom).not.toEqual(largeBottom);
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate test case</b></div>
<div id="fix">
This test is semantically duplicated - identical test already exists at lines 1744-1772. Having two tests with the same setup, assertions, and purpose increases maintenance burden and creates divergence risk if one is updated without the other.
</div>
</div>
<small><i>Code Review Run #bf02fc</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
##########
superset-frontend/plugins/plugin-chart-echarts/src/constants.ts:
##########
@@ -39,6 +39,7 @@ export const TIMESERIES_CONSTANTS = {
legendRightTopOffset: 30,
legendTopRightOffset: 55,
zoomBottom: 30,
+ zoomBottomMin: 10,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent zoom padding across chart types</b></div>
<div id="fix">
The new `zoomBottomMin` constant (10) is only applied to Timeseries charts, while Gantt and MixedTimeseries charts continue using the hardcoded `zoomBottom` value (30) directly. This creates inconsistent behavior where small-height Gantt/MixedTimeseries charts will have excessive 30px zoom padding instead of the minimum 10px. Update both files to use `Math.min(TIMESERIES_CONSTANTS.zoomBottom, Math.max(TIMESERIES_CONSTANTS.zoomBottomMin, Math.floor(height * 0.08)))`.
</div>
</div>
<small><i>Code Review Run #bf02fc</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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4809575888
## EnxDev's Review Agent β apache/superset#38657 Β· HEAD ad515f1
**Request changes** β the formula is a no-op for the short panels it's meant to fix, so #37286 isn't addressed, and the new test hides that.
### π΄ Functional
- **`src/Timeseries/transformProps.ts:1001`** β `Math.max(TIMESERIES_CONSTANTS.zoomBottom, Math.floor(height * 0.08))` with `zoomBottom = 30` (`constants.ts:41`) crosses over at `height = 375`. For **every height β€ 375px** it resolves to exactly `30` β identical to the old fixed offset. Issue #37286 is a *short panel* clipping bug, i.e. precisely the range where this changes nothing. The only heights it affects are tall charts (>375px), which weren't the problem and lose a little plot area as the slider shifts up. So the fix doesn't fix the linked issue. What's the intended direction here β should the offset *shrink* (or the grid's `gridOffsetBottomZoomable` adapt) on short charts so the slider actually fits?
- **`test/Timeseries/transformProps.test.ts:1635`** β the test passes only because it compares 300px (`bottom=30`) to 600px (`bottom=48`); the inequality is driven entirely by the tall chart. It asserts `smallBottom !== largeBottom` but never checks the short-panel value, so it would still pass with the no-op-on-short-charts behavior above. It gives false confidence that the bug is fixed. (test: assert the actual short-panel `bottom` and that it differs from the pre-PR `30`.)
### π‘ Should-fix
- **`test/Timeseries/transformProps.test.ts:1655`** β assert the exact computed values (e.g. `30` and `48`) rather than just inequality, so the formula is actually pinned by the test.
### π΅ Nits
- `src/Timeseries/transformProps.ts:1001` β Gantt/MixedTimeseries still use the fixed `zoomBottom`; if the responsive offset is kept, apply it consistently (already raised by a bot).
- `test/Timeseries/transformProps.test.ts:1647` β `as any[]` casts; prefer typing against the dataZoom option shape.
### π Praise
- Added a focused `transformProps` test with `zoomable: true` in response to reviewer feedback β right instinct, just needs sharper assertions.
<!-- enxdev-review-agent:ad515f1 -->
_Reviewed by EnxDev's Review Agent β @EnxDev Β· HEAD ad515f1._
--
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-timeseries): make dataZoom slider bottom offset responsive (#37286) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r2936953905
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -799,6 +799,11 @@ export default function transformProps(
}
}
+ const dynamicZoomBottom = Math.max(
+ TIMESERIES_CONSTANTS.zoomBottom,
+ Math.floor(height * 0.08),
+ );
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent zoom bottom across charts</b></div>
<div id="fix">
The change introduces a dynamic zoom bottom calculation for Timeseries charts based on height, but Gantt and MixedTimeseries charts still use the fixed TIMESERIES_CONSTANTS.zoomBottom value. This creates inconsistency in zoom control positioning across different chart types, potentially leading to a confusing user experience. Consider applying the same dynamic calculation to other chart types for consistency.
</div>
</div>
<small><i>Code Review Run #eb5663</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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4100673120
### <span aria-hidden="true">β
</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">π¨</span> Latest commit | bc1b579a3af72f7b7715c7a1c3e23ac7dea6812c |
|<span aria-hidden="true">π</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69bda5f8f6091400089fe4a9 |
|<span aria-hidden="true">π</span> Deploy Preview | [https://deploy-preview-38657--superset-docs-preview.netlify.app](https://deploy-preview-38657--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> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-38657--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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "manojshetty2004 (via GitHub)" <gi...@apache.org>.
manojshetty2004 commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4810756617
> Thanks @manojshetty2004, appreciate you sticking with this one.
>
> I'm a little confused on the direction though. #37286 is about the zoom control clipping in _short_ panels, but `Math.max(zoomBottom, Math.floor(height * 0.08))` only raises `bottom` as height grows, so a 300px panel still resolves to the old `30` and the test's `smallBottom` is unchanged. Taller charts get a bigger bottom margin instead. Can you walk me through how that fixes the clipped case? :)
>
> Also this needs a rebase, it's conflicting with current `master` so we can't merge as-is.
Thanks for the clarification and for pointing that out. You're rightβmy current approach doesn't address the clipping issue in shorter panels, and I misunderstood the root cause.
I'll revisit the issue, investigate the clipping behavior again, and work on a more appropriate fix. I'll also rebase the PR onto the latest master and push an updated implementation once I have a better solution. Thanks again for the detailed review and guidance!
--
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-timeseries): make dataZoom slider bottom offset responsive [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 #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3601134174
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1741,6 +1741,36 @@ test('should assign distinct dash patterns for multiple time offsets consistentl
expect(symbol1).not.toEqual(symbol2);
});
+test('should adjust dataZoom bottom when chart height changes', () => {
+ const smallChart = createTestChartProps({
+ height: 300,
+ formData: { zoomable: true },
+ });
+ const largeChart = createTestChartProps({
+ height: 600,
+ formData: { zoomable: true },
+ });
+
+ const smallResult = transformProps(smallChart);
+ const largeResult = transformProps(largeChart);
+
+ const smallDataZoom = smallResult.echartOptions.dataZoom as any[];
+ const largeDataZoom = largeResult.echartOptions.dataZoom as any[];
Review Comment:
**Suggestion:** Replace this `any[]` cast with a concrete typed shape for `dataZoom` entries to preserve static type checking. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This is a direct `any[]` cast in changed TypeScript code, which violates the no-any-types rule.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9e90091d8925489f94852523c854131d&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=9e90091d8925489f94852523c854131d&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-echarts/test/Timeseries/transformProps.test.ts
**Line:** 1758:1758
**Comment:**
*Custom Rule: Replace this `any[]` cast with a concrete typed shape for `dataZoom` entries to preserve static type checking.
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%2F38657&comment_hash=766e18b74b6fe19f452c918139929f42728b22bd8cdf5199f2df76631c9d507c&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=766e18b74b6fe19f452c918139929f42728b22bd8cdf5199f2df76631c9d507c&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1741,6 +1741,36 @@ test('should assign distinct dash patterns for multiple time offsets consistentl
expect(symbol1).not.toEqual(symbol2);
});
+test('should adjust dataZoom bottom when chart height changes', () => {
+ const smallChart = createTestChartProps({
+ height: 300,
+ formData: { zoomable: true },
+ });
+ const largeChart = createTestChartProps({
+ height: 600,
+ formData: { zoomable: true },
+ });
+
+ const smallResult = transformProps(smallChart);
+ const largeResult = transformProps(largeChart);
+
+ const smallDataZoom = smallResult.echartOptions.dataZoom as any[];
Review Comment:
**Suggestion:** Replace this `any[]` cast with a concrete typed shape for `dataZoom` entries so the test remains type-safe. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
The rule forbids new or changed TypeScript code that uses `any`. This line directly casts to `any[]`, so the violation is real and matches the rule.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5b74432b3eed4fdc8a67b9e3b7a1fc3c&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=5b74432b3eed4fdc8a67b9e3b7a1fc3c&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-echarts/test/Timeseries/transformProps.test.ts
**Line:** 1757:1757
**Comment:**
*Custom Rule: Replace this `any[]` cast with a concrete typed shape for `dataZoom` entries so the test remains type-safe.
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%2F38657&comment_hash=138defed174e182be109c667fb396ee1da7c38f0d87564e0c6004700101505eb&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=138defed174e182be109c667fb396ee1da7c38f0d87564e0c6004700101505eb&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1837,3 +1867,26 @@ describe('Tooltip with long labels', () => {
expect(result).toContain('599616000000');
});
});
+
+test('should adjust dataZoom bottom when chart height changes', () => {
+ const smallChart = createTestChartProps({
+ height: 300,
+ formData: { zoomable: true },
+ });
+ const largeChart = createTestChartProps({
+ height: 600,
+ formData: { zoomable: true },
+ });
+
+ const smallResult = transformProps(smallChart);
+ const largeResult = transformProps(largeChart);
+
+ const smallBottom = (smallResult.echartOptions.dataZoom as any[])[0].bottom;
Review Comment:
**Suggestion:** Replace the inline `any[]` assertion with a concrete type before indexing into `dataZoom`. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
The changed line uses `any[]` explicitly, so it is a real TypeScript `any` usage covered by the rule.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b27333f670e349a383a85e00ba43f2d4&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=b27333f670e349a383a85e00ba43f2d4&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-echarts/test/Timeseries/transformProps.test.ts
**Line:** 1884:1884
**Comment:**
*Custom Rule: Replace the inline `any[]` assertion with a concrete type before indexing into `dataZoom`.
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%2F38657&comment_hash=69bb24392b186b96e53f476cab92ffa9f8e31c49b971be266f2312636bf4905d&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=69bb24392b186b96e53f476cab92ffa9f8e31c49b971be266f2312636bf4905d&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1837,3 +1867,26 @@ describe('Tooltip with long labels', () => {
expect(result).toContain('599616000000');
});
});
+
+test('should adjust dataZoom bottom when chart height changes', () => {
+ const smallChart = createTestChartProps({
+ height: 300,
+ formData: { zoomable: true },
+ });
+ const largeChart = createTestChartProps({
+ height: 600,
+ formData: { zoomable: true },
+ });
+
+ const smallResult = transformProps(smallChart);
+ const largeResult = transformProps(largeChart);
+
+ const smallBottom = (smallResult.echartOptions.dataZoom as any[])[0].bottom;
+ const largeBottom = (largeResult.echartOptions.dataZoom as any[])[0].bottom;
Review Comment:
**Suggestion:** Replace the inline `any[]` assertion with an explicit typed array element definition for `dataZoom` entries. [custom_rule]
**Severity Level:** Minor π§Ή
<details>
<summary><b>Why it matters? β </b></summary>
This line contains an inline `as any[]` assertion in new TypeScript code, which directly violates the no-any-types rule.
</details>
<details>
<summary><b>Rule source π </b></summary>
.cursor/rules/dev-standard.mdc (line 16)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=50a375bdd3954e3a957ca843e151923a&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=50a375bdd3954e3a957ca843e151923a&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-echarts/test/Timeseries/transformProps.test.ts
**Line:** 1885:1885
**Comment:**
*Custom Rule: Replace the inline `any[]` assertion with an explicit typed array element definition for `dataZoom` entries.
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%2F38657&comment_hash=1dbf6ea3d1d44f6c5ed07cb4d8dd082c7d3d64f6bab98ea764dfefc2f098bb22&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=1dbf6ea3d1d44f6c5ed07cb4d8dd082c7d3d64f6bab98ea764dfefc2f098bb22&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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4988714166
Holler if you need any help getting this done... would be happy to jump in if I'm able.
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "sfirke (via GitHub)" <gi...@apache.org>.
sfirke commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4067883943
I merged master branch into this, in an attempt to fix a test that was failing for unrelated reasons.
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "manojshetty2004 (via GitHub)" <gi...@apache.org>.
manojshetty2004 commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4119527329
@EnxDev @sadpandajoe
https://github.com/user-attachments/assets/90d8540c-2d26-484b-9d23-ec5f0da81e09
Before: dataZoom slider overlaps or appears too close to the X-axis on smaller layouts.
After: responsive bottom offset ensures proper spacing and avoids overlap.
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "SBIN2010 (via GitHub)" <gi...@apache.org>.
SBIN2010 commented on code in PR #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3012279342
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -962,7 +967,7 @@ export default function transformProps(
type: 'slider',
start: TIMESERIES_CONSTANTS.dataZoomStart,
end: TIMESERIES_CONSTANTS.dataZoomEnd,
- bottom: TIMESERIES_CONSTANTS.zoomBottom,
+ bottom: dynamicZoomBottom,
Review Comment:
HI @manojshetty2004!
I agree with Copilot that unit tests need to be added.
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4180553563
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(echarts-timeseries): make dataZoom slider bottom offset responsive [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 #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r2992716825
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -865,6 +865,11 @@ export default function transformProps(
}
}
+ const dynamicZoomBottom = Math.max(
Review Comment:
**Suggestion:** The `dynamicZoomBottom` calculation uses `Math.max`, which only increases the bottom offset for taller charts and leaves it fixed for smaller ones, so the zoom slider does not actually adapt to short chart heights as intended and can remain clipped; using `Math.min` instead makes the offset shrink with chart height while capping it at the existing constant spacing. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Short-height ECharts timeseries charts keep non-responsive zoom slider offset.
- β οΈ Zoomable control still clipped in small Explore panels.
```
</details>
```suggestion
const dynamicZoomBottom = Math.min(
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open an ECharts Timeseries chart in Explore or a dashboard; this uses
`EchartsTimeseriesChartPlugin` from
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/index.ts:3-11`, which in
turn uses `transformProps` from
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts`.
2. Ensure the **Zoomable** control is enabled so `zoomable` is true and a `dataZoom`
slider is added (see `transformProps.ts:1020-1034`, where `dataZoom` is configured when
`zoomable` is truthy).
3. Use a short chart height (e.g., small dashboard row or a resized Explore panel), so the
`height` passed via `EchartsTimeseriesChartProps` into `transformProps` is below ~375px;
`transformProps.ts:69-72` computes `dynamicZoomBottom =
Math.max(TIMESERIES_CONSTANTS.zoomBottom, Math.floor(height * 0.08))`.
4. Observe from the code that for all such short heights `Math.floor(height * 0.08) <=
TIMESERIES_CONSTANTS.zoomBottom` (`zoomBottom` is 30 per
`superset-frontend/plugins/plugin-chart-echarts/src/constants.ts:33-41`), so
`dynamicZoomBottom` always resolves to 30; therefore the slider `bottom` value at
`transformProps.ts:1023-1027` is identical to the pre-PR fixed offset and does not change
with height in the exact "small chart" range the PR description claims to fix, meaning the
zoom slider layout issues for short charts are not actually addressed.
```
</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-echarts/src/Timeseries/transformProps.ts
**Line:** 868:868
**Comment:**
*Logic Error: The `dynamicZoomBottom` calculation uses `Math.max`, which only increases the bottom offset for taller charts and leaves it fixed for smaller ones, so the zoom slider does not actually adapt to short chart heights as intended and can remain clipped; using `Math.min` instead makes the offset shrink with chart height while capping it at the existing constant spacing.
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%2F38657&comment_hash=132544f99f34a8d7b0d08389801ab2a68b7967cfb0e387037ee3a66e77537927&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=132544f99f34a8d7b0d08389801ab2a68b7967cfb0e387037ee3a66e77537927&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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4155838496
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/38657?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.50%. Comparing base ([`d331a04`](https://app.codecov.io/gh/apache/superset/commit/d331a043a3a7a790134ba2b373b0148c8c237a6f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2fc3e29`](https://app.codecov.io/gh/apache/superset/commit/2fc3e29ed9976166fa11fb0677789ac562327ea3?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 #38657 +/- ##
=======================================
Coverage 64.50% 64.50%
=======================================
Files 2536 2536
Lines 130823 130824 +1
Branches 30348 30348
=======================================
+ Hits 84382 84383 +1
Misses 44978 44978
Partials 1463 1463
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/38657/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/38657/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.83% <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 Sentry](https://app.codecov.io/gh/apache/superset/pull/38657?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(echarts-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4748913666
Thanks @manojshetty2004, appreciate you sticking with this one.
I'm a little confused on the direction though. #37286 is about the zoom control clipping in *short* panels, but `Math.max(zoomBottom, Math.floor(height * 0.08))` only raises `bottom` as height grows, so a 300px panel still resolves to the old `30` and the test's `smallBottom` is unchanged. Taller charts get a bigger bottom margin instead. Can you walk me through how that fixes the clipped case? :)
Also this needs a rebase, it's conflicting with current `master` so we can't merge as-is.
--
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-timeseries): make dataZoom slider bottom offset responsive [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 #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3029260126
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -996,6 +996,11 @@ export default function transformProps(
}
}
+ const dynamicZoomBottom = Math.min(
Review Comment:
**Suggestion:** The responsive bottom offset calculation uses `Math.min`, which lowers the slider offset below the legacy baseline on short charts and breaks the "minimum spacing" contract; this can push the control too close to the container edge and reintroduce clipping/overlap in constrained layouts. Use `Math.max` so the previous offset remains the minimum while still allowing larger offsets on taller charts. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β οΈ Zoomable ECharts timeseries charts show reduced slider spacing.
- β οΈ Short dashboard panels risk zoom slider near container edge.
- β οΈ Violates zoomBottom minimum spacing semantics in constants.ts.
```
</details>
```suggestion
const dynamicZoomBottom = Math.max(
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open an ECharts Time-series chart in Explore (this chart uses
`TimeseriesTransformProps` exported from
`superset-frontend/plugins/plugin-chart-echarts/src/index.ts:55` and registered via
`EchartsTimeseriesChartPlugin`, so all ECharts time-series charts call `transformProps` in
`src/Timeseries/transformProps.ts`).
2. Ensure the chart is zoomable by enabling the "Zoomable" control (this sets
`formData.zoomable=true`, which causes `transformProps` to add a `dataZoom` slider with
`bottom: dynamicZoomBottom` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:1161-1165`).
3. Render the chart in a panel whose rendered height is below ~375px (e.g., 300px as in
the unit test at `test/Timeseries/transformProps.test.ts:1514-1538` or smaller like 80px
as in `test/Timeseries/transformProps.test.ts:1391-1396`); this height is passed as
`height` into `transformProps`.
4. At runtime, `transformProps` computes `dynamicZoomBottom` using `Math.min` at
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:999-1002`,
so for a 300px chart `dynamicZoomBottom = Math.min(30, Math.floor(300*0.08)) = 24`, which
is **less** than `TIMESERIES_CONSTANTS.zoomBottom = 30` defined in
`src/constants.ts:4-16`. The resulting ECharts option sets `dataZoom[0].bottom` to 24,
shrinking spacing below the established `zoomBottom` baseline instead of preserving it as
a minimum; this contradicts the PR's own description (which specifies
`Math.max(TIMESERIES_CONSTANTS.zoomBottom, Math.floor(height * 0.08))`) and can place the
slider closer to the container edge than the legacy minimum on short charts.
```
</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-echarts/src/Timeseries/transformProps.ts
**Line:** 999:999
**Comment:**
*Logic Error: The responsive bottom offset calculation uses `Math.min`, which lowers the slider offset below the legacy baseline on short charts and breaks the "minimum spacing" contract; this can push the control too close to the container edge and reintroduce clipping/overlap in constrained layouts. Use `Math.max` so the previous offset remains the minimum while still allowing larger offsets on taller charts.
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%2F38657&comment_hash=2044f7a172c9c1660447bda280c8d9a61aa8230931fe6257ba7cf2e841e1d073&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=2044f7a172c9c1660447bda280c8d9a61aa8230931fe6257ba7cf2e841e1d073&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-timeseries): make dataZoom slider bottom offset responsive [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 #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3601185252
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1310,6 +1310,62 @@ export function TableRenderer(props: TableRendererProps) {
tableOptions.clickRowHeaderCallback,
)}
onContextMenu={handleContextMenu}
+ };
+ const headerCellFormattedValue =
+ dateFormatters?.[attrName]?.(
+ convertToNumberIfNumeric(colKey[attrIdx]),
+ ) ?? colKey[attrIdx];
Review Comment:
**Suggestion:** This new block references variables that are not defined in this row-header scope (`attrName`, `colKey`, `attrIdx`), which will throw when rendering. Use the row-scope variables (`settingsRowAttrs[i]`, `r`, and the row index) instead of column-header-only variables. [incorrect variable usage]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
β Frontend build fails, blocking pivot table plugin usage.
β οΈ CI pipelines fail on TypeScript compilation errors.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx`
and locate the `renderColHeaderRow` callback around lines 860-1135 (from the `Read` tool),
where `attrName`, `attrIdx`, `colKey`, `colLabelClass`, and related variables are defined
and used to build column header cells.
2. In the same file, inspect the `renderTableRow` callback starting at line 116 in the
`Read` output (diff hunk around 1316) that renders row header cells; within the
`rowKey.map((r, i) => { ... })` callback, the newly added block at diff lines 1314-1317
defines `const headerCellFormattedValue =
dateFormatters?.[attrName]?.(convertToNumberIfNumeric(colKey[attrIdx])) ??
colKey[attrIdx];`.
3. Verify from the surrounding `renderTableRow` scope (lines 120-145 from `Read`) that
`settingsRowAttrs`, `rowKey`, and `dateFormatters` are defined, but there is no definition
of `attrName`, `colKey`, or `attrIdx` in this function; those identifiers only exist as
parameters to `renderColHeaderRow` and are not in scope inside `renderTableRow`.
4. Run the TypeScript type-check or frontend build (for example `cd superset-frontend &&
npm run build`), and the compiler will flag `attrName`, `colKey`, and `attrIdx` in
`TableRenderers.tsx` (diff lines 1314-1317) as undefined identifiers, causing compilation
to fail and blocking the pivot table plugin from being built or used.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=26c6c5be958e462ba49c881c593fb2a4&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=26c6c5be958e462ba49c881c593fb2a4&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-pivot-table/src/react-pivottable/TableRenderers.tsx
**Line:** 1314:1317
**Comment:**
*Incorrect Variable Usage: This new block references variables that are not defined in this row-header scope (`attrName`, `colKey`, `attrIdx`), which will throw when rendering. Use the row-scope variables (`settingsRowAttrs[i]`, `r`, and the row index) instead of column-header-only variables.
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%2F38657&comment_hash=07bbfc667ba25d0d450c865fbf46730d74f764d22ecb66af5dca7aa9656fc1b2&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=07bbfc667ba25d0d450c865fbf46730d74f764d22ecb66af5dca7aa9656fc1b2&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1741,6 +1741,36 @@ test('should assign distinct dash patterns for multiple time offsets consistentl
expect(symbol1).not.toEqual(symbol2);
});
+test('should adjust dataZoom bottom when chart height changes', () => {
Review Comment:
**Suggestion:** The exact same test case name and behavior is added twice, creating duplicate coverage and ambiguous failure reporting in CI. Keep only one copy of this test and remove the duplicate block. [code quality]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
β οΈ Duplicate jest test names complicate CI failure triage.
β οΈ Redundant tests increase suite runtime without added coverage.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts`
and locate the first test named `should adjust dataZoom bottom when chart height changes`
added in the hunk starting at diff line 1744, which constructs small and large charts and
compares their `dataZoom[0].bottom` values.
2. In the same file, scroll down to the later hunk starting around diff line 1871 and
observe a second test with the identical name `should adjust dataZoom bottom when chart
height changes` that also creates `smallChart` and `largeChart`, calls `transformProps`,
and asserts that the `dataZoom[0].bottom` values differ.
3. Run the frontend test suite (for example `cd superset-frontend && npm test --
plugin-chart-echarts/test/Timeseries/transformProps.test.ts`) and note that Jest reports
two separate tests with the exact same description string, both invoking `transformProps`
and asserting the bottom offset behavior.
4. When a regression occurs in this behavior, CI output will show a failure for `should
adjust dataZoom bottom when chart height changes` without distinguishing which of the two
blocks failed, making diagnosis harder and executing redundant assertions for the same
scenario.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=541283171f49486daeec0fdf713569f5&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=541283171f49486daeec0fdf713569f5&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-echarts/test/Timeseries/transformProps.test.ts
**Line:** 1744:1744
**Comment:**
*Code Quality: The exact same test case name and behavior is added twice, creating duplicate coverage and ambiguous failure reporting in CI. Keep only one copy of this test and remove the duplicate block.
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%2F38657&comment_hash=2825673bf71c31f58a684cf39081d8b528076ec44cca7f9d0b1a4373d5a17073&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=2825673bf71c31f58a684cf39081d8b528076ec44cca7f9d0b1a4373d5a17073&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1310,6 +1310,62 @@ export function TableRenderer(props: TableRendererProps) {
tableOptions.clickRowHeaderCallback,
)}
onContextMenu={handleContextMenu}
+ };
+ const headerCellFormattedValue =
+ dateFormatters?.[attrName]?.(
+ convertToNumberIfNumeric(colKey[attrIdx]),
+ ) ?? colKey[attrIdx];
+ const { backgroundColor, color } = getCellColor(
+ [attrName],
+ headerCellFormattedValue,
+ cellColorFormatters,
+ isActiveHeader ? activeHeaderBackgroundColor : cellBackgroundColor,
+ );
+ const style = {
+ backgroundColor,
+ ...(color ? { color } : {}),
+ };
+ attrValueCells.push(
+ <th
Review Comment:
**Suggestion:** Pushing into `attrValueCells` from inside the callback that is used to initialize `attrValueCells` creates a self-reference during initialization and breaks the row rendering flow. Keep this path as a pure `map` return (as before) and avoid mutating the array being constructed inside its own initializer. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
β Pivot table rows crash with ReferenceError during render.
β οΈ Users see blank pivot tables instead of data.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. In `TableRenderers.tsx`, within the `renderTableRow` callback (lines 148-199 from the
`Read` output), note that `const attrValueCells = rowKey.map((r: string, i: number) => {
... });` initializes `attrValueCells` by mapping over `rowKey`, so `attrValueCells` is
defined by the result of this `map` call.
2. Inside the same `rowKey.map` callback, inspect the newly added block at diff lines
1328-1329 where `attrValueCells.push(<th ... > ...)` is called, meaning the callback
attempts to push additional header cells into `attrValueCells` while `attrValueCells`
itself is still being initialized.
3. From JavaScript semantics, referencing `attrValueCells` inside its own initializer
(`const attrValueCells = rowKey.map(() => { attrValueCells.push(...); })`) places that
reference in the temporal dead zone; at runtime, when `rowKey.map` invokes the callback
for the first element, `attrValueCells` has not yet been assigned, so any read or `push`
on `attrValueCells` triggers a `ReferenceError: Cannot access 'attrValueCells' before
initialization`.
4. Once the TypeScript compile-time issues in this block are resolved and a pivot table
chart (rendered via `PivotTable.tsx` which delegates to `TableRenderer`) reaches this
path, rendering rows will immediately crash with the temporal-dead-zone `ReferenceError`,
preventing any pivot table rows from displaying.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4381993ad6cd48fd99bc7c17dc7ad8ed&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=4381993ad6cd48fd99bc7c17dc7ad8ed&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-pivot-table/src/react-pivottable/TableRenderers.tsx
**Line:** 1328:1329
**Comment:**
*Logic Error: Pushing into `attrValueCells` from inside the callback that is used to initialize `attrValueCells` creates a self-reference during initialization and breaks the row rendering flow. Keep this path as a pure `map` return (as before) and avoid mutating the array being constructed inside its own initializer.
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%2F38657&comment_hash=d7e337fbf08add829bf1fee109dc1636e555d0d597cacbefeb2222552ca9b9f1&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=d7e337fbf08add829bf1fee109dc1636e555d0d597cacbefeb2222552ca9b9f1&reaction=dislike'>π</a>
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -1310,6 +1310,62 @@ export function TableRenderer(props: TableRendererProps) {
tableOptions.clickRowHeaderCallback,
)}
onContextMenu={handleContextMenu}
+ };
+ const headerCellFormattedValue =
+ dateFormatters?.[attrName]?.(
+ convertToNumberIfNumeric(colKey[attrIdx]),
+ ) ?? colKey[attrIdx];
+ const { backgroundColor, color } = getCellColor(
+ [attrName],
+ headerCellFormattedValue,
+ cellColorFormatters,
+ isActiveHeader ? activeHeaderBackgroundColor : cellBackgroundColor,
+ );
+ const style = {
+ backgroundColor,
+ ...(color ? { color } : {}),
+ };
+ attrValueCells.push(
+ <th
+ className={colLabelClass}
+ key={`colKey-${flatColKey}`}
+ style={style}
+ colSpan={colSpan}
+ rowSpan={rowSpan}
+ role="columnheader button"
+ onClick={this.clickHeaderHandler(
+ pivotData,
+ colKey,
+ this.props.cols,
+ attrIdx,
+ this.props.tableOptions.clickColumnHeaderCallback,
+ )}
Review Comment:
**Suggestion:** This component is a function component, so using `this`-based handlers/state will fail at runtime because `this` is `undefined`. Replace these calls with the already scoped hook/local variables (`clickHeaderHandler`, `cols`, `tableOptions`, `collapsedCols`, `activeSortColumn`, `sortingOrder`) used elsewhere in this file. [api mismatch]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
β Pivot table charts fail rendering due to runtime TypeError.
β οΈ Cross-filtering from row headers becomes completely unusable.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. In
`superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx`,
confirm that `TableRenderer` is a function component using React hooks, defined at lines
334-453 (from the `export function TableRenderer(props: TableRendererProps)` declaration
and subsequent `useState` and `useCallback` hooks returned by the `Read` tool).
2. Within the same file, inspect the `renderTableRow` callback (useCallback starting
around diff line 1316) where row header cells are rendered; the new hunk at diff lines
1336-1342 sets `onClick={this.clickHeaderHandler(pivotData, colKey, this.props.cols,
attrIdx, this.props.tableOptions.clickColumnHeaderCallback)}` on a header `<th>` element
instead of using the `clickHeaderHandler` hook defined earlier.
3. Observe via the code that `clickHeaderHandler` in this file is a top-level hook-scoped
function (lines 59-88 from the `Read` output) and there is no `this` binding or class
instance for `TableRenderer`; therefore, evaluating `this.clickHeaderHandler` inside the
render path will attempt to read a property from `this`, which is `undefined` for function
components.
4. Render any pivot table chart (for example by using the `PivotTable` wrapper in
`plugins/plugin-chart-pivot-table/src/react-pivottable/PivotTable.tsx`, which returns
`<TableRenderer {...props} />` at lines 5-8) and when React executes `renderTableRow`, the
expression `this.clickHeaderHandler(...)` is evaluated during render, causing a runtime
TypeError (`Cannot read properties of undefined`) and preventing the pivot table from
rendering.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8b4d3990568340a383e116e456c017a2&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=8b4d3990568340a383e116e456c017a2&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-pivot-table/src/react-pivottable/TableRenderers.tsx
**Line:** 1336:1342
**Comment:**
*Api Mismatch: This component is a function component, so using `this`-based handlers/state will fail at runtime because `this` is `undefined`. Replace these calls with the already scoped hook/local variables (`clickHeaderHandler`, `cols`, `tableOptions`, `collapsedCols`, `activeSortColumn`, `sortingOrder`) used elsewhere in this file.
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%2F38657&comment_hash=f1fbd3fbca398d1f8a5aaf42e3257720022dfe35c9b4d1726b4af4b68908964f&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=f1fbd3fbca398d1f8a5aaf42e3257720022dfe35c9b4d1726b4af4b68908964f&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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "manojshetty2004 (via GitHub)" <gi...@apache.org>.
manojshetty2004 commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-5000117167
@rusackas
Thanks for the feedback. I'll close this PR and work on a fresh implementation in a new PR that properly addresses the issue. This PR has accumulated unrelated changes during development, so starting fresh will make the review cleaner and easier. I'll link the new PR to this issue once it's ready.
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "manojshetty2004 (via GitHub)" <gi...@apache.org>.
manojshetty2004 closed pull request #38657: fix(echarts-timeseries): make dataZoom slider bottom offset responsive
URL: https://github.com/apache/superset/pull/38657
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "manojshetty2004 (via GitHub)" <gi...@apache.org>.
manojshetty2004 commented on code in PR #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3015365945
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -962,7 +967,7 @@ export default function transformProps(
type: 'slider',
start: TIMESERIES_CONSTANTS.dataZoomStart,
end: TIMESERIES_CONSTANTS.dataZoomEnd,
- bottom: TIMESERIES_CONSTANTS.zoomBottom,
+ bottom: dynamicZoomBottom,
Review Comment:
@SBIN2010
Thanks for the suggestion! I've added a unit test to verify that the dataZoom bottom value adjusts dynamically based on chart height. Also resolved merge conflicts. Please let me know if anything else is needed.
--
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-timeseries): make dataZoom slider bottom offset responsive [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 #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3029252093
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1520,3 +1520,26 @@ test('should assign distinct dash patterns for multiple time offsets consistentl
// must be different patterns
expect(symbol1).not.toEqual(symbol2);
});
+test('should adjust dataZoom bottom when chart height changes', () => {
+ const smallChart = createTestChartProps({ height: 300 });
+ const largeChart = createTestChartProps({ height: 600 });
Review Comment:
**Suggestion:** The new test assumes `dataZoom` exists, but `createTestChartProps` uses `DEFAULT_FORM_DATA` where `zoomable` is `false`, so `dataZoom` is an empty array and this test fails before validating bottom offset behavior. Enable `zoomable` in both chart configs so the test exercises the intended logic. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Timeseries transformProps test suite fails consistently in CI.
- β οΈ Responsive dataZoom bottom logic lacks effective test coverage.
- β οΈ Contributors blocked by failing tests unrelated to runtime behavior.
```
</details>
```suggestion
const smallChart = createTestChartProps({
height: 300,
formData: { zoomable: true },
});
const largeChart = createTestChartProps({
height: 600,
formData: { zoomable: true },
});
```
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Run the Jest test suite for
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts`
(e.g. via `npm test -- transformProps.test.ts`), which executes the test `should adjust
dataZoom bottom when chart height changes` defined around line 1523.
2. Inside that test, `createTestChartProps` is called with only a `height` override (lines
1524β1525), so `formData.zoomable` is not provided;
`createEchartsTimeseriesTestChartProps` in `test/helpers.ts:71-109` merges the partial
formData into `DEFAULT_FORM_DATA`, leaving `zoomable` at its default `false` value from
`src/Timeseries/constants.ts:36-91` (specifically line 79).
3. `transformProps` in `src/Timeseries/transformProps.ts` merges `{ ...DEFAULT_FORM_DATA,
...formData }` into a full `EchartsTimeseriesFormData` at lines 217-276, so `zoomable`
remains `false`; later, the ECharts options are built with `dataZoom: zoomable ? [ ... ] :
[]` at lines 1158-1180, resulting in `smallResult.echartOptions.dataZoom` and
`largeResult.echartOptions.dataZoom` both being empty arrays.
4. Back in the test file, the assertions at lines 1533-1537 expect `smallDataZoom.length`
and `largeDataZoom.length` to be greater than 0, causing the test to fail immediately
(because both lengths are 0) before it can validate that `bottom` differs between small
and large charts, so the new responsive `dynamicZoomBottom` behavior in
`transformProps.ts:999-1002` is never actually exercised.
```
</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-echarts/test/Timeseries/transformProps.test.ts
**Line:** 1524:1525
**Comment:**
*Logic Error: The new test assumes `dataZoom` exists, but `createTestChartProps` uses `DEFAULT_FORM_DATA` where `zoomable` is `false`, so `dataZoom` is an empty array and this test fails before validating bottom offset behavior. Enable `zoomable` in both chart configs so the test exercises the intended logic.
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%2F38657&comment_hash=feefe3847d95c3cea15a07f7951551600fcfab03414a1d018516aab9f615ca3d&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=feefe3847d95c3cea15a07f7951551600fcfab03414a1d018516aab9f615ca3d&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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "manojshetty2004 (via GitHub)" <gi...@apache.org>.
manojshetty2004 commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4068598388
@sfirke
Thanks for merging `master` into the branch to address the unrelated failing test. I can confirm that all checks are now passing on my side as well. Please let me know if any further adjustments are needed for this 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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4194859745
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/24050123130) for [e334a49](https://github.com/apache/superset/commit/e334a49)
--
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-timeseries): make dataZoom slider bottom offset responsive [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 #38657:
URL: https://github.com/apache/superset/pull/38657#discussion_r3482192044
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:
##########
@@ -1662,6 +1662,35 @@ test('should assign distinct dash patterns for multiple time offsets consistentl
expect(symbol1).not.toEqual(symbol2);
});
+test('should adjust dataZoom bottom when chart height changes', () => {
+ const smallChart = createTestChartProps({
+ height: 300,
+ formData: { zoomable: true },
+ });
+ const largeChart = createTestChartProps({
+ height: 600,
+ formData: { zoomable: true },
+ });
+
+ const smallResult = transformProps(smallChart);
+ const largeResult = transformProps(largeChart);
+
+ const smallDataZoom = smallResult.echartOptions.dataZoom as any[];
+ const largeDataZoom = largeResult.echartOptions.dataZoom as any[];
Review Comment:
**Suggestion:** Replace these `any[]` assertions with a specific data-zoom option type (or a narrow local interface with only the accessed fields) so the test remains type-safe. [custom_rule]
**Severity Level:** Minor β οΈ
<details>
<summary><b>Why it matters? π€ </b></summary>
This is a real violation of the custom TypeScript rule: the new test code uses `any[]` in type assertions. The rule explicitly requires a concrete type, reusable existing type, or narrower generic instead of `any`.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2eba6e058a8f4687aae4afb24f3edab3&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=2eba6e058a8f4687aae4afb24f3edab3&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-echarts/test/Timeseries/transformProps.test.ts
**Line:** 1678:1679
**Comment:**
*Custom Rule: Replace these `any[]` assertions with a specific data-zoom option type (or a narrow local interface with only the accessed fields) so the test remains type-safe.
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%2F38657&comment_hash=ecca10222b1496ebca5315fa44034419b04a66aa5214fc7bd130064c3194bfc0&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38657&comment_hash=ecca10222b1496ebca5315fa44034419b04a66aa5214fc7bd130064c3194bfc0&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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "manojshetty2004 (via GitHub)" <gi...@apache.org>.
manojshetty2004 commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4113087658
Hi @EnxDev, Iβm currently unable to run Superset locally to capture before/after videos. Iβll try to demonstrate the behavior with a minimal example or add tests for this change. Please let me know if that works.
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4141867716
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(echarts-timeseries): make dataZoom slider bottom offset responsive (#37286) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4063291916
## **Sequence Diagram**
This PR updates the time series chart option builder to compute the data zoom slider bottom offset from chart height instead of always using a fixed value. The result is that zoom controls stay visible in shorter chart panels while preserving the previous minimum spacing.
```mermaid
sequenceDiagram
participant User
participant Chart
participant TransformProps
participant ECharts
User->>Chart: Open small time series panel with zoom enabled
Chart->>TransformProps: Build chart options with panel height
TransformProps->>TransformProps: Compute zoom bottom as max of minimum and height based offset
TransformProps-->>Chart: Return options with data zoom slider bottom
Chart->>ECharts: Render chart using updated options
ECharts-->>User: Show visible zoom slider in panel
```
---
*Generated by [CodeAnt AI](https://codeant.ai)*
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "manojshetty2004 (via GitHub)" <gi...@apache.org>.
manojshetty2004 commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4148500565
Hi , just checking in β is there any feedback or changes needed from my side? Happy to update this 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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4194962732
πͺ [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/24050123130) for [e334a49](https://github.com/apache/superset/commit/e334a49)
β’ **Environment:** http://44.244.220.43:8080 (admin/admin)
β’ **Lifetime:** 48h auto-cleanup
β’ **Updates:** New commits create fresh environments automatically
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4179554183
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(echarts-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4162314661
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(echarts-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-4997267448
Thanks for the PR, Manoj! CI's failing right now, looks like the new test in transformProps.test.ts is missing its closing brace before the describe('Tooltip with long labels'...) block, so the file can't even parse, that's what's tripping up pre-commit, lint, and one of the jest shards.
There's also a bot thread still open pointing out that Math.max(zoomBottom, height * 0.08) only starts changing anything once the chart's taller than a few hundred px, so the small panels this is meant to fix (the #37286 case) might still get the old fixed offset. Worth confirming that's not the case before this goes in.
Mind fixing the syntax break and settling that logic question? Happy to take another look once CI's green. Holler if I can help more here, and push a commit or whatever :)
--
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-timeseries): make dataZoom slider bottom offset responsive [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38657:
URL: https://github.com/apache/superset/pull/38657#issuecomment-5002049287
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