Repository navigation
fix(explore): honor explicit List legend selection instead of flipping to scroll - #41749
Conversation
dosubot
Bot
added
explore:design
Code Review Agent Run #6dd655Actionable Suggestions - 0Additional Suggestions - 2
Review 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
c33dbf5 to
866e0ed
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #41749 +/- ##
==========================================
- Coverage 65.73% 65.72% -0.01%
==========================================
Files 2843 2843
Lines 162677 162655 -22
Branches 37255 37247 -8
==========================================
- Hits 106935 106911 -24
- Misses 53650 53651 +1
- Partials 2092 2093 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
866e0ed to
b1b2f70
Compare
![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 #8b737a
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts - 1
- Unused parameter creates dead code · Line 323-323
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts - 1
- Wrong padding property used · Line 950-952
Review Details
-
Files reviewed - 7 · Commit Range:
b1b2f70..b1b2f70- superset-frontend/plugins/plugin-chart-echarts/src/utils/legendLayout.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Gantt/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.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
| // Raw chart dimensions. Use availableWidth when other chart UI elements | ||
| // reserve legend space before ECharts lays out the legend. | ||
| availableWidth?: number; | ||
| chartHeight: number; |
There was a problem hiding this comment.
The chartHeight parameter is destructured (line 309) but never referenced in the function body. Keeping an unused parameter in the type definition creates misleading API documentation and suggests callers should provide a value that's ignored. Remove chartHeight: number; from line 323 alongside the destructuring removal.
Code Review Run #8b737a
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
chartHeight is still used here. Line 356 reads it as the fallback for availableHeight (availableHeight ?? chartHeight) when a caller does not pass a separate height, so dropping it from the signature would break that path.
There was a problem hiding this comment.
The reviewer's suggestion to remove chartHeight is incorrect in this context. As you noted, chartHeight is used as a fallback for availableHeight (availableHeight ?? chartHeight), so removing it from the function signature would indeed break that logic.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const result = transformProps(props as EchartsGanttChartProps); | ||
|
|
||
| expect((result.echartOptions.legend as any).type).toBe(LegendType.Scroll); | ||
| expect((result.echartOptions.legend as any).type).toBe(LegendType.Plain); |
There was a problem hiding this comment.
Suggestion: Replace the any cast with a concrete legend option type (or a narrowed type assertion) before reading type. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The new TypeScript test code explicitly casts result.echartOptions.legend to any before accessing type, which matches the custom rule prohibiting any in changed TypeScript code. A concrete legend option type or narrower assertion should be used instead.
(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/Gantt/transformProps.test.ts
**Line:** 347:347
**Comment:**
*Custom Rule: Replace the `any` cast with a concrete legend option type (or a narrowed type assertion) before reading `type`.
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 fixThere was a problem hiding this comment.
Fixed on 14 Jul in f7e1080. That assertion uses LegendComponentOption now instead of the any cast.
| const rowsForMargin = Number.isFinite(rowCount) | ||
| ? rowCount | ||
| : legendLabels.length; | ||
| const requiredMargin = | ||
| defaultLegendPadding[orientation] + | ||
| Math.max(0, rowCount - 1) * LEGEND_HORIZONTAL_ROW_HEIGHT; | ||
| const maxLegendHeight = | ||
| availableHeight > 0 | ||
| ? availableHeight * LEGEND_HORIZONTAL_MAX_HEIGHT_RATIO | ||
| : Infinity; | ||
|
|
||
| if ( | ||
| !Number.isFinite(rowCount) || | ||
| rowCount > LEGEND_HORIZONTAL_MAX_ROWS || | ||
| requiredMargin > maxLegendHeight | ||
| ) { | ||
| return SCROLL_LEGEND_LAYOUT; | ||
| } | ||
| Math.max(0, rowsForMargin - 1) * LEGEND_HORIZONTAL_ROW_HEIGHT; |
There was a problem hiding this comment.
Suggestion: When row estimation overflows (Infinity), this fallback uses the full legend item count as row count with no cap, which can produce extremely large effectiveMargin values (thousands of pixels for large series lists) and collapse the plot area. Clamp the computed rows/margin against chart height (or a sane max rows threshold) before returning the margin. [logic error]
Severity Level: Major ⚠️
❌ Small-width timeseries charts can collapse to near-zero height.
❌ MixedTimeseries legends can reserve disproportionate vertical space.
⚠️ Gantt charts with many series may show barely visible plot.Steps of Reproduction ✅
1. In the timeseries chart pipeline, `resolveLegendLayout()` at
`superset-frontend/plugins/plugin-chart-echarts/src/utils/legendLayout.ts:19-36` calls
`getLegendLayoutResult()` with `availableWidth` computed in
`Timeseries/transformProps.ts:28-38` via `getHorizontalLegendAvailableWidth({ chartWidth:
width, orientation: legendOrientation, padding, zoomable })`.
2. When the chart container is extremely narrow, or left padding from `getPadding()`
equals/exceeds `width`, `getHorizontalLegendAvailableWidth()` in
`src/utils/series.ts:62-79` returns `availableWidth = 0` (it does `chartWidth -
(padding?.left ?? 0)` then `Math.max(availableWidth, 0)`).
3. `getLegendLayoutResult()` in `src/utils/series.ts:110-161` receives this
`availableWidth` and, for a horizontal legend (`LegendOrientation.Top`/`Bottom`) with
`type: LegendType.Plain`, calls `getHorizontalPlainLegendLayout()` (lines 42-56) with
non-empty `legendItems` (e.g. many series names from
`Timeseries/transformProps.ts:40-43`).
4. Inside `getHorizontalPlainLegendLayout()`, `estimateHorizontalLegendRows()` at
`src/utils/series.ts:3-15` sees `availableWidth === 0` and returns `Infinity`. The new
fallback at lines 262-265 sets `rowsForMargin = legendLabels.length` when `rowCount` is
non-finite. For a legend with many items (e.g. 100 series), `requiredMargin =
defaultLegendPadding[orientation] + (rowsForMargin - 1) * LEGEND_HORIZONTAL_ROW_HEIGHT`
(lines 265-267) becomes thousands of pixels (99 * 24px ≈ 2376 + padding). This
`effectiveMargin` is propagated via `resolveLegendLayout()` and then used as
`effectiveLegendMargin` in `Timeseries/transformProps.ts:60-75` and
`MixedTimeseries/transformProps.ts:39-49` when computing grid padding, causing the ECharts
grid height to be squeezed down to a tiny or zero drawable area on small-width charts with
many legend items.
(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/utils/series.ts
**Line:** 262:267
**Comment:**
*Logic Error: When row estimation overflows (`Infinity`), this fallback uses the full legend item count as row count with no cap, which can produce extremely large `effectiveMargin` values (thousands of pixels for large series lists) and collapse the plot area. Clamp the computed rows/margin against chart height (or a sane max rows threshold) before returning the margin.
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 fixThere was a problem hiding this comment.
Code Review Agent Run #7d4056Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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 |
|
Thanks @jesperct. Honoring an explicit List choice instead of flipping to scroll makes sense, and the common wide-chart case looks fine. The thing I'd want to sort before merging is the unbounded margin the bot flagged... on a narrow, tall chart with a lot of series, |
b87c1e7 to
3b36cfb
Compare
|
Good call. Bounded it: when the legend overflows the available width, the reserved margin is now capped at 40% of the chart height instead of a row per series, so a large legend can't push |
3b36cfb to
b67c168
Compare
Code Review Agent Run #9f484eActionable 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 |
b67c168 to
f7e1080
Compare
Code Review Agent Run #b720abActionable Suggestions - 0Additional Suggestions - 1
Review 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
left a comment
sadpandajoe
left a comment
There was a problem hiding this comment.
Left inline comments on legend layout bounds and regression coverage.
| } | ||
|
|
||
| return { | ||
| effectiveMargin: Math.max(currentMargin, requiredMargin), |
There was a problem hiding this comment.
This computed margin can exceed the chart width for long left/right labels, leaving no usable plot area. Could we pass the available width into this path, cap the automatic vertical margin as the horizontal path does, and cover it with a long-label regression?
There was a problem hiding this comment.
The vertical path had no cap at all. It reserved whatever the longest label measured, so one long series name could ask for more width than the chart has. It now takes availableWidth and applies the same 40% cap as the horizontal path. Test written first: 1533px reserved on a 1000px chart before, 400px after.
| ); | ||
| const rowsForMargin = Number.isFinite(rowCount) | ||
| ? rowCount | ||
| : legendLabels.length; |
There was a problem hiding this comment.
When a single label is wider than the available width, rowCount is infinite but this still preserves Plain without constraining the text, so ECharts renders outside the viewport. Could we bound or truncate oversized horizontal labels while keeping the explicit List selection?
There was a problem hiding this comment.
Agreed, a finite margin does not mean the label fits. The fallback keeps the arithmetic sane but leaves the text unconstrained, so it can still paint outside the viewport.
Truncating it properly means adding a width to the layout result and applying textStyle.width plus overflow in the legend builder for the 8 charts that use this helper (Radar, Gantt, Bubble, Pie, Timeseries, MixedTimeseries, Graph, Funnel). That about doubles this diff and pulls in six chart types unrelated to the original bug, so I would rather file it separately and pick it up next. Say the word if you want it here instead.
|
|
||
| expect(layout.effectiveType).toBe(LegendType.Plain); | ||
| expect(Number.isFinite(layout.effectiveMargin)).toBe(true); | ||
| expect(layout.effectiveMargin as number).toBeLessThan(chartHeight); |
There was a problem hiding this comment.
This only proves the margin is below 100% of the chart height, so changing the intended 40% cap to 99% would still pass. Could we assert chartHeight * 0.4 (80px here) directly?
There was a problem hiding this comment.
Fixed, it asserts 80 directly now, which is the 40% cap on the 200px chart height. Changing the ratio fails the test.
| // reduce clipping instead of the legend flipping to scroll. | ||
| expect(layout.effectiveMargin).toEqual(expect.any(Number)); | ||
| expect(Number.isFinite(layout.effectiveMargin)).toBe(true); | ||
| expect(grid.bottom as number).toBeGreaterThanOrEqual( |
There was a problem hiding this comment.
>= allows the single-row baseline, so this passes even if the wrapped legend margin stops reaching the final grid. Could this assert a strict increase, or the exact padding derived from layout.effectiveMargin?
There was a problem hiding this comment.
Fixed. It derives the expected padding from layout.effectiveMargin and asserts grid.bottom matches it exactly, plus a strict greater-than against the single row baseline.
| ); | ||
| }); | ||
|
|
||
| test('getLegendLayoutResult honors an explicit List selection with many series (SC-101478)', () => { |
There was a problem hiding this comment.
This test name includes an internal ticket identifier in public source. Could we remove it or replace it with a public GitHub issue reference?
There was a problem hiding this comment.
Removed. I rewrote the commit that introduced it and force pushed, so it is out of the branch history rather than just fixed on top.
| ]; | ||
| const expandedPadding = getPadding( | ||
|
|
||
| const layout = getBottomLegendLayout(chartWidth, legendItems, null); |
There was a problem hiding this comment.
Suggestion: The layout helper is invoked with a null legend type, so this assertion path is not actually exercising the explicit List/Plain selection scenario described by the test. This can let a regression slip through because the helper call is testing default behavior instead of the user-selected type; pass the explicit plain type to the helper in this check. [logic error]
Severity Level: Major ⚠️
- ⚠️ Bottom legend List selection regression tests may miss failures.
- ⚠️ ECharts timeseries bar legend-layout helper less protected.Steps of Reproduction ✅
1. Open
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts`
and locate the test `honors an explicit List selection for horizontal bottom legends and
reserves margin` starting around line 855.
2. At lines 895–901, observe `regressionFormData` is built with `legendType:
LegendType.Plain`, representing an explicit List/Plain legend selection by the user.
3. At lines 916–919, the test calls `transformProps(chartProps)` and extracts `legend`
from `transformedProps.echartOptions.legend`, then later asserts at line 943 that
`legend.type` is `LegendType.Plain`, confirming the ECharts config carries the explicit
plain type.
4. At line 924, the layout helper is invoked as `const layout =
getBottomLegendLayout(chartWidth, legendItems, null);`, passing `null` as the third
argument instead of `LegendType.Plain`, so the subsequent assertion at line 944 that
`layout.effectiveType` is `LegendType.Plain` only exercises the helper’s default/null
behavior; any regression specifically in how `getBottomLegendLayout` handles an explicit
`LegendType.Plain` (the bug described in the PR) would not be covered by this test path.
(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/Bar/transformProps.test.ts
**Line:** 924:924
**Comment:**
*Logic Error: The layout helper is invoked with a null legend type, so this assertion path is not actually exercising the explicit List/Plain selection scenario described by the test. This can let a regression slip through because the helper call is testing default behavior instead of the user-selected type; pass the explicit plain type to the helper in this check.
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 fixThere was a problem hiding this comment.
I do not think this one holds. The third parameter of getBottomLegendLayout is legendMargin (line 826), not the legend type, and the helper hardcodes type: LegendType.Plain in the options it builds (line 852). Passing null means no user-set margin, so the default padding applies, which is the case this test is meant to cover. The explicit Plain selection is still exercised end to end.
| expect(layout.effectiveMargin).toEqual(expect.any(Number)); | ||
| expect(Number.isFinite(layout.effectiveMargin)).toBe(true); | ||
| expect(grid.bottom as number).toBeGreaterThanOrEqual( |
There was a problem hiding this comment.
Suggestion: The test comment says wrapped rows should reserve margin beyond the baseline, but the assertion allows equality, which would also pass when no extra margin is reserved. This weakens the regression guard and can miss the exact clipping issue the test is meant to catch. [comment mismatch]
Severity Level: Major ⚠️
- ⚠️ Legend margin regression may slip past automated tests.
- ⚠️ Wrapped bottom legends risk clipping without failing tests.Steps of Reproduction ✅
1. In
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts`,
within the `honors an explicit List selection...` test, observe at lines 925–937 that
`basePadding` is obtained via `getPadding(...)` and then its `bottom` and `left` are
swapped so `basePadding.bottom` represents the single-row baseline bottom margin for a
bottom legend.
2. At lines 917–919, note that `grid` is read from `transformedProps.echartOptions.grid`
as the actual chart grid configuration after legend layout is applied.
3. At lines 948–950, the assertion `expect(grid.bottom as
number).toBeGreaterThanOrEqual(basePadding.bottom as number);` only checks that
`grid.bottom` is not less than the baseline margin.
4. In the multi-row legend scenario described by the test comments (lines 902–904 and
947–948), if a future change stops reserving extra space and `grid.bottom` collapses back
to exactly `basePadding.bottom` (no additional margin beyond the baseline), this
`toBeGreaterThanOrEqual` assertion still passes, so the test would not fail even though
the wrapped legend rows are no longer reserving extra margin as intended.
(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/Bar/transformProps.test.ts
**Line:** 948:950
**Comment:**
*Comment Mismatch: The test comment says wrapped rows should reserve margin beyond the baseline, but the assertion allows equality, which would also pass when no extra margin is reserved. This weakens the regression guard and can miss the exact clipping issue the test is meant to catch.
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 fixThere was a problem hiding this comment.
Fixed in 56f1ba4. The assertion derives the expected padding from layout.effectiveMargin and asserts grid.bottom matches it exactly, plus a strict greater-than over the single row baseline.
Code Review Agent Run #de7ed8Actionable 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 |
535ce3b to
56f1ba4
Compare
Code Review Agent Run #2ff107Actionable 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 |
…'t collapse the plot When a plain (List) horizontal legend overflows the available width, the row estimate is Infinity and the fallback reserved one row of margin per label. On a narrow, short chart with many series this pushed the reserved margin past the chart height and collapsed the plot area. Cap the reserved margin at a fraction of the chart height, using the chartHeight already passed to the layout helper.
…block It asserts legend type selection, not sorting, so it belongs at file level alongside the other two plain-legend cases.
…t assertions The horizontal legend path caps the margin it reserves at a fraction of the chart height, but the vertical path reserved whatever the longest label measured. A single long series name could therefore reserve more width than the chart has, leaving no usable plot area. Apply the same cap to the vertical path using the available width already resolved by the layout helper. Two existing assertions were loose enough to pass against the wrong value: the horizontal bound only checked the margin stayed under the full chart height, and the Bar grid check allowed the single-row baseline. Both now assert the derived value.
…ogical bounds Eight legend layout assertions compared effectiveMargin against defaultLegendPadding for the same orientation. Since effectiveMargin is Math.max(currentMargin, ...) and currentMargin falls back to that same padding when no explicit margin is set, the comparison held for every possible implementation and could not fail. Replacing the entire margin reservation with a pass-through left 92 of 96 tests in this file green. Each of the eight now asserts the exact reserved margin, with the derivation recorded alongside it, which brings that number to 89.
2ee7e03 to
8bc109f
Compare
| // Cap the reserved horizontal legend margin so an overflowing legend can't eat the plot. | ||
| const MAX_LEGEND_MARGIN_RATIO = 0.4; |
There was a problem hiding this comment.
Suggestion: The comment says the cap is only for horizontal legends, but the same constant is now used for both horizontal and vertical layout code paths; this mismatch is misleading and can cause incorrect future maintenance changes. Update the comment to reflect both orientations or split constants/comments per orientation. [comment mismatch]
Severity Level: Minor 🧹
- ⚠️ Comment misstates cap usage across legend orientations.
- ⚠️ Could mislead maintainers making orientation-specific changes.Steps of Reproduction ✅
1. A developer reads the comment above `MAX_LEGEND_MARGIN_RATIO` in
`superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:63-64`, which states
the cap is for the reserved horizontal legend margin.
2. The same `MAX_LEGEND_MARGIN_RATIO` constant is used to cap margins in
`getHorizontalPlainLegendLayout()` at `series.ts:268-271` and also in
`getVerticalPlainLegendLayout()` at `series.ts:308-311`, affecting both horizontal and
vertical legends.
3. This means the comment at `series.ts:63` is factually inaccurate: the cap applies to
all plain legends, not only horizontal ones.
4. While this does not affect runtime behavior, the misleading comment can confuse future
maintainers and lead to incorrect, orientation-specific changes that overlook the vertical
usage of the constant.
(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/utils/series.ts
**Line:** 63:64
**Comment:**
*Comment Mismatch: The comment says the cap is only for horizontal legends, but the same constant is now used for both horizontal and vertical layout code paths; this mismatch is misleading and can cause incorrect future maintenance changes. Update the comment to reflect both orientations or split constants/comments per orientation.
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 #12858b
Actionable Suggestions - 2
-
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts - 2
- Swapped padding corrupts baseline · Line 937-940
- Second swap on reservedPadding · Line 963-966
Additional Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts - 1
-
CWE-0: Incomplete Comment Scope · Line 63-63The comment at line 63 says 'Cap the reserved horizontal legend margin' but the `MAX_LEGEND_MARGIN_RATIO` constant is now used for both horizontal and vertical legend bounding. Consider clarifying this comment to reflect its dual purpose, or split into separate constants (e.g., `MAX_HORIZONTAL_MARGIN_RATIO` and `MAX_VERTICAL_MARGIN_RATIO`) with precise comments for each use case.
-
Review Details
-
Files reviewed - 6 · Commit Range:
dfcea12..8bc109f- superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Gantt/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Pie/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.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
| [basePadding.bottom, basePadding.left] = [ | ||
| basePadding.left, | ||
| basePadding.bottom, | ||
| ]; |
There was a problem hiding this comment.
The destructuring-swap [basePadding.bottom, basePadding.left] = [basePadding.left, basePadding.bottom] on lines 937-940 mutates basePadding in-place, corrupting basePadding.bottom before it is used in the assertion on line 969. This makes the comparison toBeGreaterThan(basePadding.bottom) check the original left value instead of the original bottom value — defeating the intended regression check.
Code Review Run #12858b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| [reservedPadding.bottom, reservedPadding.left] = [ | ||
| reservedPadding.left, | ||
| reservedPadding.bottom, | ||
| ]; |
There was a problem hiding this comment.
The second destructuring-swap on lines 963-966 mutates reservedPadding after the first swap mutated basePadding. Since grid.bottom is asserted to equal reservedPadding.bottom on line 968, and both basePadding and reservedPadding are computed identically (both pass layout.effectiveMargin), swapping reservedPadding is unnecessary and conflates two separate concerns. It also means the value grid.bottom is checked against is reservedPadding.left, not reservedPadding.bottom.
Code Review Run #12858b
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
Bito Automatic Review Skipped – PR Already Merged |
|
Thank you @jesperct ! I ran down this bug earlier today when one of my users complained and was very pleased to see you fixed and that this will arrive in 7.0.0 😁 |
SUMMARY
When a chart's legend Type is set to List (plain), Superset could silently override that choice and render a scrollable, paginated legend instead. Whenever a plain legend was estimated not to fit in the available space, the shared legend-layout helper flipped
legend.typefromplaintoscroll. The visible effect: picking List and clicking Update chart appeared to do nothing for charts with many series — the legend stayed a single-row scroll paginator.This change makes an explicit List selection authoritative. The layout helper now always keeps
legend.type: 'plain'when the user picks List. The margin reservation that shrinks the plot area to reduce legend clipping (added alongside the original auto-flip) is kept, so long legends still get vertical room reserved for them without overriding the user's choice.The fix lives in the shared
getLegendLayoutResulthelper, so it applies to every ECharts chart that routes through it (Timeseries line/bar, Pie, Bubble, Funnel, Graph, Gantt). No form-data migration is needed — the stored option valueplainis unchanged.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Line chart, 50 series, legend Type = List, orientation Top.
Before — List selection silently flipped to a single-row Scroll paginator (
◀ 1/7 ▶):After — List honored, full multi-row legend:
TESTING INSTRUCTIONS
birth_namesdataset. Set X-axisds, metricsum__num, and Dimensionsnamewith Series limit 50 so the legend has many entries.Automated coverage:
plugins/plugin-chart-echarts/test/utils/series.test.tsplus the per-charttransformPropstests (Timeseries, Pie, Gantt, Bar) assert an explicit List selection is preserved and a finite margin is reserved. Full plugin suite passes (639 tests).ADDITIONAL INFORMATION