fix(echarts-timeseries): make dataZoom slider bottom offset responsive#38657
fix(echarts-timeseries): make dataZoom slider bottom offset responsive#38657manojshetty2004 wants to merge 10 commits into
Conversation
codeant-ai-for-open-source
Bot
added
the
size:XS
dosubot
Bot
added
viz:charts:echarts
Sequence DiagramThis 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. 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 |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #eb5663
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts - 1
- Inconsistent zoom bottom across charts · Line 802-805
Review Details
-
Files reviewed - 1 · Commit Range:
1abdd35..1abdd35- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
sfirke
changed the title
codeant-ai-for-open-source
Bot
added
size:XS
|
I merged master branch into this, in an attempt to fix a test that was failing for unrelated reasons. |
|
@sfirke |
|
@manojshetty2004 in the before and after section of the description, can you add images or a small video of the changes? |
There was a problem hiding this comment.
Pull request overview
Adjusts ECharts time-series dataZoom slider positioning to avoid the zoom control being clipped in constrained panel sizes by making the slider’s bottom offset depend on chart height.
Changes:
- Introduces a computed
dynamicZoomBottomvalue derived fromheight. - Uses
dynamicZoomBottominstead ofTIMESERIES_CONSTANTS.zoomBottomfordataZoom.slider.bottom.
You can also share your feedback on Copilot code review. Take the survey.
| type: 'slider', | ||
| start: TIMESERIES_CONSTANTS.dataZoomStart, | ||
| end: TIMESERIES_CONSTANTS.dataZoomEnd, | ||
| bottom: TIMESERIES_CONSTANTS.zoomBottom, | ||
| bottom: dynamicZoomBottom, |
There was a problem hiding this comment.
HI @manojshetty2004!
I agree with Copilot that unit tests need to be added.
There was a problem hiding this 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.
codeant-ai-for-open-source
Bot
added
size:XS
|
Thanks for the suggestion! I’ll add before/after screenshots demonstrating the zoom slider behavior shortly. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Code Review Agent Run #491d05Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
@sadpandajoe Please let me know if you’d like me to try a specific scenario or dataset to better demonstrate the issue. |
codeant-ai-for-open-source
Bot
added
size:XS
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #b7af21Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
codeant-ai-for-open-source
Bot
added
size:XS
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hey @manojshetty2004 could you add a before/after video in the description? |
|
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 Also this needs a rebase, it's conflicting with current |
EnxDev's Review Agent — #38657 · HEAD ad515f1Request 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
🟡 Should-fix
🔵 Nits
🙌 Praise
|
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. |
| const dynamicZoomBottom = Math.max( | ||
| TIMESERIES_CONSTANTS.zoomBottom, | ||
| Math.floor(height * 0.08), | ||
| ); |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Small-height echarts_timeseries charts keep non-responsive zoom slider.
- ⚠️ Zoom slider clipping persists in compact dashboard panels.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| const smallChart = createTestChartProps({ | ||
| height: 300, | ||
| formData: { zoomable: true }, |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Test misses small-height behavior of dataZoom bottom offset.
- ⚠️ Regression in small chart layout may go undetected.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
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! |
|
Holler if you need any help getting this done... would be happy to jump in if I'm able. |
|
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 :) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…transformProps.ts Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
…transformProps.ts Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
…ased on chart height
…/transformProps.test.ts Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
…transformProps.ts Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>
735c821 to
c588e2c
Compare
|
@rusackas |
| expect(symbol1).not.toEqual(symbol2); | ||
| }); | ||
|
|
||
| test('should adjust dataZoom bottom when chart height changes', () => { |
There was a problem hiding this 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]
Severity Level: Major ⚠️
⚠️ Duplicate jest test names complicate CI failure triage.
⚠️ Redundant tests increase suite runtime without added coverage.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| const headerCellFormattedValue = | ||
| dateFormatters?.[attrName]?.( | ||
| convertToNumberIfNumeric(colKey[attrIdx]), | ||
| ) ?? colKey[attrIdx]; |
There was a problem hiding this 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]
Severity Level: Critical 🚨
❌ Frontend build fails, blocking pivot table plugin usage.
⚠️ CI pipelines fail on TypeScript compilation errors.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| attrValueCells.push( | ||
| <th |
There was a problem hiding this 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]
Severity Level: Major ⚠️
❌ Pivot table rows crash with ReferenceError during render.
⚠️ Users see blank pivot tables instead of data.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| onClick={this.clickHeaderHandler( | ||
| pivotData, | ||
| colKey, | ||
| this.props.cols, | ||
| attrIdx, | ||
| this.props.tableOptions.clickColumnHeaderCallback, | ||
| )} |
There was a problem hiding this 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]
Severity Level: Critical 🚨
❌ Pivot table charts fail rendering due to runtime TypeError.
⚠️ Cross-filtering from row headers becomes completely unusable.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
There was a problem hiding this comment.
Code Review Agent Run #bf02fc
Actionable Suggestions - 2
-
superset-frontend/plugins/plugin-chart-echarts/src/constants.ts - 1
- Inconsistent zoom padding across chart types · Line 42-42
-
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts - 1
- Duplicate test case · Line 1871-1892
Review Details
-
Files reviewed - 4 · Commit Range:
dc3f7a2..c588e2c- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/constants.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
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 documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| legendRightTopOffset: 30, | ||
| legendTopRightOffset: 55, | ||
| zoomBottom: 30, | ||
| zoomBottomMin: 10, |
There was a problem hiding this comment.
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))).
Code Review Run #bf02fc
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| 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); | ||
| }); |
There was a problem hiding this comment.
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.
Code Review Run #bf02fc
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
User description
SUMMARY
This change makes the
dataZoomslider 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:
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
ADDITIONAL INFORMATION
CodeAnt-AI Description
Keep the time-series zoom slider visible in short charts
What Changed
Impact
✅ Fewer clipped time-series charts✅ Zoom controls stay visible in small dashboard panels✅ Clearer chart interaction in compact layouts💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.