bito-code-review[bot] commented on code in PR #41749:
URL: https://github.com/apache/superset/pull/41749#discussion_r3641823411
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:
##########
@@ -968,30 +934,41 @@ describe('Bar Chart X-axis Time Formatting', () => {
undefined,
true,
);
- [expectedPadding.bottom, expectedPadding.left] = [
- expectedPadding.left,
- expectedPadding.bottom,
+ [basePadding.bottom, basePadding.left] = [
+ basePadding.left,
+ basePadding.bottom,
];
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Swapped padding corrupts baseline</b></div>
<div id="fix">
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.
</div>
</div>
<small><i>Code Review Run #12858b</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/test/Timeseries/Bar/transformProps.test.ts:
##########
@@ -968,30 +934,41 @@ describe('Bar Chart X-axis Time Formatting', () => {
undefined,
true,
);
- [expectedPadding.bottom, expectedPadding.left] = [
- expectedPadding.left,
- expectedPadding.bottom,
+ [basePadding.bottom, basePadding.left] = [
+ basePadding.left,
+ basePadding.bottom,
];
- const expandedPadding = getPadding(
+
+ // The explicit List selection is honored end-to-end (never flips).
+ expect(legend.type).toBe(LegendType.Plain);
+ expect(layout.effectiveType).toBe(LegendType.Plain);
+
+ // #38675's margin reservation is retained: the wrapped rows reserve a
+ // finite margin beyond the single-row baseline, so the grid shrinks to
+ // reduce clipping instead of the legend flipping to scroll.
+ expect(Number.isFinite(layout.effectiveMargin)).toBe(true);
+
+ const reservedPadding = getPadding(
true,
LegendOrientation.Bottom,
false,
false,
- expandedLegendMargin,
+ layout.effectiveMargin,
false,
undefined,
undefined,
undefined,
true,
);
- [expandedPadding.bottom, expandedPadding.left] = [
- expandedPadding.left,
- expandedPadding.bottom,
+ [reservedPadding.bottom, reservedPadding.left] = [
+ reservedPadding.left,
+ reservedPadding.bottom,
];
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Second swap on reservedPadding</b></div>
<div id="fix">
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`.
</div>
</div>
<small><i>Code Review Run #12858b</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: [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]
