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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
