codeant-ai-for-open-source[bot] commented on code in PR #41749:
URL: https://github.com/apache/superset/pull/41749#discussion_r3630173373
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:
##########
@@ -899,99 +899,57 @@ describe('Bar Chart X-axis Time Formatting', () => {
legendType: LegendType.Plain,
showLegend: true,
};
- const baselineChartProps = createEchartsTimeseriesTestChartProps<
- EchartsTimeseriesFormData,
- EchartsTimeseriesChartProps
- >({
- defaultFormData: regressionFormData,
- defaultVizType: 'echarts_timeseries_bar',
- defaultQueriesData: longLegendData,
- width: baseChartPropsConfig.width,
- height: baseChartPropsConfig.height,
- });
- const baselineTransformed = transformProps(baselineChartProps);
- const legendItems = (
- (baselineTransformed.echartOptions.legend as LegendComponentOption)
- .data as Array<string | { name: string }>
- ).map(item => (typeof item === 'string' ? item : item.name));
- let chartWidth: number | undefined;
- let expandedLegendMargin: number | null = null;
-
- for (let width = 300; width <= 700; width += 1) {
- const initialLayout = getBottomLegendLayout(width, legendItems, null);
-
- if (initialLayout.effectiveType !== LegendType.Plain) {
- continue;
- }
-
- const refinedLayout = getBottomLegendLayout(
- width,
- legendItems,
- initialLayout.effectiveMargin ?? null,
- );
-
- if (refinedLayout.effectiveType === LegendType.Scroll) {
- chartWidth = width;
- expandedLegendMargin = initialLayout.effectiveMargin ?? null;
- break;
- }
- }
-
- expect(chartWidth).toBeDefined();
- expect(expandedLegendMargin).not.toBeNull();
- const resolvedChartWidth = chartWidth ?? baseChartPropsConfig.width;
-
+ // A narrow chart forces the long-label bottom legend to wrap onto
+ // multiple rows — the case that previously flipped List to scroll.
+ const chartWidth = 320;
const chartProps = createEchartsTimeseriesTestChartProps<
EchartsTimeseriesFormData,
EchartsTimeseriesChartProps
>({
defaultFormData: regressionFormData,
defaultVizType: 'echarts_timeseries_bar',
defaultQueriesData: longLegendData,
- width: resolvedChartWidth,
+ width: chartWidth,
height: baseChartPropsConfig.height,
});
const transformedProps = transformProps(chartProps);
const legend = transformedProps.echartOptions
.legend as LegendComponentOption;
const grid = transformedProps.echartOptions.grid as GridComponentOption;
- const expectedPadding = getPadding(
- true,
- LegendOrientation.Bottom,
- false,
- false,
- null,
- false,
- undefined,
- undefined,
- undefined,
- true,
+ const legendItems = (legend.data as Array<string | { name: string
}>).map(
+ item => (typeof item === 'string' ? item : item.name),
);
- [expectedPadding.bottom, expectedPadding.left] = [
- expectedPadding.left,
- expectedPadding.bottom,
- ];
- const expandedPadding = getPadding(
+
+ const layout = getBottomLegendLayout(chartWidth, legendItems, null);
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Bottom legend List selection regression tests may miss failures.
- ⚠️ ECharts timeseries bar legend-layout helper less protected.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0312a4eeac6141d09f26d344b8a8d831&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=0312a4eeac6141d09f26d344b8a8d831&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/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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41749&comment_hash=44122f6185a80502d60c4d5d65732c487aed91a2df08a489086ddbafbac5bbde&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41749&comment_hash=44122f6185a80502d60c4d5d65732c487aed91a2df08a489086ddbafbac5bbde&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:
##########
@@ -899,99 +899,57 @@ describe('Bar Chart X-axis Time Formatting', () => {
legendType: LegendType.Plain,
showLegend: true,
};
- const baselineChartProps = createEchartsTimeseriesTestChartProps<
- EchartsTimeseriesFormData,
- EchartsTimeseriesChartProps
- >({
- defaultFormData: regressionFormData,
- defaultVizType: 'echarts_timeseries_bar',
- defaultQueriesData: longLegendData,
- width: baseChartPropsConfig.width,
- height: baseChartPropsConfig.height,
- });
- const baselineTransformed = transformProps(baselineChartProps);
- const legendItems = (
- (baselineTransformed.echartOptions.legend as LegendComponentOption)
- .data as Array<string | { name: string }>
- ).map(item => (typeof item === 'string' ? item : item.name));
- let chartWidth: number | undefined;
- let expandedLegendMargin: number | null = null;
-
- for (let width = 300; width <= 700; width += 1) {
- const initialLayout = getBottomLegendLayout(width, legendItems, null);
-
- if (initialLayout.effectiveType !== LegendType.Plain) {
- continue;
- }
-
- const refinedLayout = getBottomLegendLayout(
- width,
- legendItems,
- initialLayout.effectiveMargin ?? null,
- );
-
- if (refinedLayout.effectiveType === LegendType.Scroll) {
- chartWidth = width;
- expandedLegendMargin = initialLayout.effectiveMargin ?? null;
- break;
- }
- }
-
- expect(chartWidth).toBeDefined();
- expect(expandedLegendMargin).not.toBeNull();
- const resolvedChartWidth = chartWidth ?? baseChartPropsConfig.width;
-
+ // A narrow chart forces the long-label bottom legend to wrap onto
+ // multiple rows — the case that previously flipped List to scroll.
+ const chartWidth = 320;
const chartProps = createEchartsTimeseriesTestChartProps<
EchartsTimeseriesFormData,
EchartsTimeseriesChartProps
>({
defaultFormData: regressionFormData,
defaultVizType: 'echarts_timeseries_bar',
defaultQueriesData: longLegendData,
- width: resolvedChartWidth,
+ width: chartWidth,
height: baseChartPropsConfig.height,
});
const transformedProps = transformProps(chartProps);
const legend = transformedProps.echartOptions
.legend as LegendComponentOption;
const grid = transformedProps.echartOptions.grid as GridComponentOption;
- const expectedPadding = getPadding(
- true,
- LegendOrientation.Bottom,
- false,
- false,
- null,
- false,
- undefined,
- undefined,
- undefined,
- true,
+ const legendItems = (legend.data as Array<string | { name: string
}>).map(
+ item => (typeof item === 'string' ? item : item.name),
);
- [expectedPadding.bottom, expectedPadding.left] = [
- expectedPadding.left,
- expectedPadding.bottom,
- ];
- const expandedPadding = getPadding(
+
+ const layout = getBottomLegendLayout(chartWidth, legendItems, null);
+ const basePadding = getPadding(
true,
LegendOrientation.Bottom,
false,
false,
- expandedLegendMargin,
+ null,
false,
undefined,
undefined,
undefined,
true,
);
- [expandedPadding.bottom, expandedPadding.left] = [
- expandedPadding.left,
- expandedPadding.bottom,
+ [basePadding.bottom, basePadding.left] = [
+ basePadding.left,
+ basePadding.bottom,
];
- expect(legend.type).toBe(LegendType.Scroll);
- expect(grid.bottom).toBe(expectedPadding.bottom);
- expect(grid.bottom).not.toBe(expandedPadding.bottom);
+ // 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(layout.effectiveMargin).toEqual(expect.any(Number));
+ expect(Number.isFinite(layout.effectiveMargin)).toBe(true);
+ expect(grid.bottom as number).toBeGreaterThanOrEqual(
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Legend margin regression may slip past automated tests.
- ⚠️ Wrapped bottom legends risk clipping without failing tests.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f57a4fc44ac24264b2967b6a4b5d7a93&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=f57a4fc44ac24264b2967b6a4b5d7a93&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/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 fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41749&comment_hash=dffecc2a197e819f444c09380b52a0d4796deb0d84b6a47399d48e64dd72e2af&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41749&comment_hash=dffecc2a197e819f444c09380b52a0d4796deb0d84b6a47399d48e64dd72e2af&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]
