codeant-ai-for-open-source[bot] commented on code in PR #38675:
URL: https://github.com/apache/superset/pull/38675#discussion_r2939905129
##########
superset-frontend/plugins/plugin-chart-echarts/src/Gantt/transformProps.ts:
##########
@@ -245,8 +250,10 @@ export default function transformProps(chartProps:
EchartsGanttChartProps) {
.second(time.second());
}
+ const showLegendForChart = showLegend && seriesMap.size > 1;
Review Comment:
**Suggestion:** This change overrides the `showLegend` control by hiding the
legend whenever there is only one series. That breaks the existing chart
contract (legend visibility should follow the user setting), and causes
dashboards with `showLegend=true` to silently lose legend rendering and
interaction. Keep legend visibility driven by `showLegend`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Gantt legend toggle ignored for single-series results.
- ⚠️ Dashboard users lose expected legend visibility behavior.
- ⚠️ Gantt behavior diverges from other ECharts plugins.
```
</details>
```suggestion
const showLegendForChart = showLegend;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open Gantt chart in Explore/Dashboard; plugin wiring uses
`transformProps` from
`src/Gantt/index.ts:59`.
2. In chart controls, enable legend via `show_legend` (defined as "Whether
to display a
legend" in `src/controls.tsx:42-50`).
3. Use query result producing one legend series (e.g., one distinct `series`
value or no
effective split), then `groupData(...)` creates `seriesMap` and
`transformProps` computes
`showLegendForChart` at `src/Gantt/transformProps.ts:253`.
4. Because line 253 applies `showLegend && seriesMap.size > 1`,
`legend.show` is forced
false in `getLegendProps(..., showLegendForChart, ...)` at
`src/Gantt/transformProps.ts:209-217`, so legend is hidden despite user
enabling it.
```
</details>
<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/Gantt/transformProps.ts
**Line:** 253:253
**Comment:**
*Logic Error: This change overrides the `showLegend` control by hiding
the legend whenever there is only one series. That breaks the existing chart
contract (legend visibility should follow the user setting), and causes
dashboards with `showLegend=true` to silently lose legend rendering and
interaction. Keep legend visibility driven by `showLegend`.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=264c1a797aa7cd17eaffa637b30b8ace5fb4f852086a8215edb95cd3d587c7fb&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=264c1a797aa7cd17eaffa637b30b8ace5fb4f852086a8215edb95cd3d587c7fb&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -583,13 +586,58 @@ export default function transformProps(
convertInteger(yAxisTitleMargin) !== 0;
const addXAxisTitleOffset =
!!xAxisTitle && convertInteger(xAxisTitleMargin) !== 0;
+ const baseChartPadding = getPadding(
+ showLegend,
+ legendOrientation,
+ addYAxisTitleOffset,
+ zoomable,
+ legendMargin,
+ addXAxisTitleOffset,
+ yAxisTitlePosition,
+ convertInteger(yAxisTitleMargin),
+ convertInteger(xAxisTitleMargin),
+ );
+ const legendData = series
+ .filter(
+ entry =>
+ extractForecastSeriesContext((entry.name || '') as string).type ===
+ ForecastSeriesEnum.Observation,
+ )
+ .map(entry => entry.id || entry.name || '')
+ .concat(extractAnnotationLabels(annotationLayers))
+ .sort((a: string, b: string) => {
+ if (!legendSort) return 0;
+ return legendSort === 'asc' ? a.localeCompare(b) : b.localeCompare(a);
+ });
Review Comment:
**Suggestion:** Legend entries are currently built from `id` when `name` is
missing, which pulls in internal annotation helper series (like event/interval
marker lines) that are not meant to appear in the legend. This can surface
incorrect legend items and distort legend layout/fallback decisions. Build
legend items from `name` only and skip unnamed series. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Mixed chart legends show non-data annotation helper items.
- ⚠️ Plain legend overflow estimation becomes artificially inflated.
- ❌ Scroll fallback may trigger earlier than necessary.
```
</details>
```suggestion
const legendData = series
.filter(
entry =>
!!entry.name &&
extractForecastSeriesContext(String(entry.name)).type ===
ForecastSeriesEnum.Observation,
)
.map(entry => String(entry.name))
.concat(extractAnnotationLabels(annotationLayers))
.sort((a: string, b: string) => {
if (!legendSort) return 0;
return legendSort === 'asc' ? a.localeCompare(b) : b.localeCompare(a);
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a Mixed Chart with legend enabled and add an Event/Interval
annotation
(supported by plugin metadata at
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/index.ts:64-68`).
2. During render, `transformProps()` processes annotation layers and calls
`transformIntervalAnnotation()` / `transformEventAnnotation()`
(`.../MixedTimeseries/transformProps.ts:375-392`).
3. Those transformers push helper series with `id` but no `name`
(`.../Timeseries/transformers.ts:115-116` and `:210-211`), so entries are
unnamed.
4. Legend building in Mixed Timeseries filters with
`extractForecastSeriesContext(entry.name || '')` and maps `entry.id ||
entry.name`
(`.../MixedTimeseries/transformProps.ts:600-607`); empty `name` is treated
as observation
by `extractForecastSeriesContext` (`.../utils/forecast.ts:13`), so helper
IDs like `Event
- ...`/`Interval - ...` get included.
5. `getLegendLayoutResult()` then uses this inflated `legendItems` list for
plain-vs-scroll decision (`.../utils/series.ts:331-390`), causing premature
fallback or
extra legend entries unrelated to real metric series.
```
</details>
<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/MixedTimeseries/transformProps.ts
**Line:** 600:611
**Comment:**
*Logic Error: Legend entries are currently built from `id` when `name`
is missing, which pulls in internal annotation helper series (like
event/interval marker lines) that are not meant to appear in the legend. This
can surface incorrect legend items and distort legend layout/fallback
decisions. Build legend items from `name` only and skip unnamed series.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=b15f23f01477a3bf8c35e8eb5bd2094c304fd39c776ecb09064eadf01efc6101&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=b15f23f01477a3bf8c35e8eb5bd2094c304fd39c776ecb09064eadf01efc6101&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -55,6 +55,340 @@ function isDefined<T>(value: T | undefined | null): boolean
{
return value !== undefined && value !== null;
}
+const DEFAULT_LEGEND_ITEM_GAP = 10;
+const DEFAULT_LEGEND_ICON_WIDTH = 25;
+const LEGEND_ICON_LABEL_GAP = 5;
+const LEGEND_HORIZONTAL_SIDE_GUTTER = 16;
+const LEGEND_HORIZONTAL_ROW_HEIGHT = 24;
+const LEGEND_HORIZONTAL_MAX_ROWS = 2;
+const LEGEND_HORIZONTAL_MAX_HEIGHT_RATIO = 0.25;
+const LEGEND_VERTICAL_SIDE_GUTTER = 16;
+const LEGEND_VERTICAL_ROW_HEIGHT = 24;
+const LEGEND_VERTICAL_MAX_WIDTH_RATIO = 0.4;
+const LEGEND_SELECTOR_GAP = 10;
+const LEGEND_MARGIN_GUTTER = 45;
+// ECharts does not expose pre-render measurements for plain legends, so these
+// values intentionally overestimate selector space to avoid clipping.
+const ESTIMATED_LEGEND_SELECTOR_WIDTH = 112;
+const LEGEND_TEXT_WIDTH_CACHE = new Map<string, number>();
+
+type LegendDataItem =
+ | string
+ | number
+ | null
+ | undefined
+ | { name?: string | number | null };
+
+export type LegendLayoutResult = {
+ effectiveMargin?: number;
+ effectiveType: LegendType;
+};
+
+const SCROLL_LEGEND_LAYOUT: LegendLayoutResult = {
+ effectiveType: LegendType.Scroll,
+};
+
+function getLegendLabel(item: LegendDataItem): string {
+ if (typeof item === 'string' || typeof item === 'number') {
+ return String(item);
+ }
+
+ if (item?.name === undefined || item.name === null) {
+ return '';
+ }
+
+ return String(item.name);
+}
+
+function measureLegendTextWidth(text: string, theme: SupersetTheme): number {
+ const cacheKey = `${theme.fontFamily}:${theme.fontSizeSM}:${text}`;
+ const cachedWidth = LEGEND_TEXT_WIDTH_CACHE.get(cacheKey);
+ if (cachedWidth !== undefined) {
+ return cachedWidth;
+ }
+
+ let width = text.length * theme.fontSizeSM * 0.62;
+
+ if (typeof document !== 'undefined') {
+ const canvas = document.createElement('canvas');
+ const context = canvas.getContext('2d');
+ if (context) {
+ context.font = `${theme.fontSizeSM}px ${theme.fontFamily}`;
+ width = context.measureText(text).width;
+ }
+ }
+
+ LEGEND_TEXT_WIDTH_CACHE.set(cacheKey, width);
+ return width;
+}
+
+function getLegendLabels(items: LegendDataItem[]): string[] {
+ return items.map(getLegendLabel).filter(Boolean);
+}
+
+function getLegendItemWidths(labels: string[], theme: SupersetTheme): number[]
{
+ return labels.map(
+ label =>
+ DEFAULT_LEGEND_ICON_WIDTH +
+ LEGEND_ICON_LABEL_GAP +
+ measureLegendTextWidth(label, theme),
+ );
+}
+
+function estimateHorizontalLegendRows(
+ labels: string[],
+ chartWidth: number,
+ showSelectors: boolean,
+ theme: SupersetTheme,
+): number {
+ const availableWidth = Math.max(
+ chartWidth - LEGEND_HORIZONTAL_SIDE_GUTTER,
+ 0,
+ );
+ if (availableWidth === 0) {
+ return Infinity;
+ }
+
+ const legendItemWidths = getLegendItemWidths(labels, theme);
+
+ if (legendItemWidths.length === 0) {
+ return 1;
+ }
+
+ let rows = 1;
+ let rowWidth = 0;
+
+ for (const itemWidth of legendItemWidths) {
+ if (itemWidth > availableWidth) {
+ return Infinity;
+ }
+
+ const nextWidth =
+ rowWidth === 0
+ ? itemWidth
+ : rowWidth + DEFAULT_LEGEND_ITEM_GAP + itemWidth;
+ if (rowWidth > 0 && nextWidth > availableWidth) {
+ rows += 1;
+ rowWidth = itemWidth;
+ } else {
+ rowWidth = nextWidth;
+ }
+ }
+
+ if (showSelectors) {
+ const selectorWidth =
+ rowWidth === 0
+ ? ESTIMATED_LEGEND_SELECTOR_WIDTH
+ : rowWidth + LEGEND_SELECTOR_GAP + ESTIMATED_LEGEND_SELECTOR_WIDTH;
+ if (rowWidth > 0 && selectorWidth > availableWidth) {
+ rows += 1;
+ }
+ }
Review Comment:
**Suggestion:** The horizontal layout estimator can still keep plain legends
when selector buttons alone are wider than the available width. In narrow
dashboards this returns a two-row plain layout even though the selector row
itself cannot fit, so the selector controls get clipped. Treat this as overflow
and force scroll mode. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Narrow dashboard legends clip All/Inverse controls.
- ❌ Plain legend fallback decision wrong on tiny widths.
```
</details>
```suggestion
if (showSelectors) {
if (ESTIMATED_LEGEND_SELECTOR_WIDTH > availableWidth) {
return Infinity;
}
const selectorWidth =
rowWidth === 0
? ESTIMATED_LEGEND_SELECTOR_WIDTH
: rowWidth + LEGEND_SELECTOR_GAP + ESTIMATED_LEGEND_SELECTOR_WIDTH;
if (rowWidth > 0 && selectorWidth > availableWidth) {
rows += 1;
}
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In Explore, select an ECharts plugin using shared legend controls (legend
type
selectable in `src/controls.tsx:67-76`, including `'plain'` list mode), then
save chart to
a narrow dashboard tile.
2. Rendering enters plugin transform path (for example
`src/Timeseries/index.ts:73` ->
`src/Timeseries/transformProps.ts:704`) which calls
`getLegendLayoutResult(...)` with
horizontal width constraints (`transformProps.ts:705-713`).
3. `getLegendLayoutResult` calls `estimateHorizontalLegendRows`
(`src/utils/series.ts:138`), where selector overflow currently only
increments `rows`
(`series.ts:178-185`) but does not force overflow when selector controls
alone exceed
width.
4. For very small `availableWidth` (from dashboard sizing/padding), layout
can remain
plain (2 rows allowed at `series.ts:264`) and `getLegendProps` still emits
selector
controls (`series.ts:784`), producing clipped All/Inverse controls.
```
</details>
<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/utils/series.ts
**Line:** 178:186
**Comment:**
*Logic Error: The horizontal layout estimator can still keep plain
legends when selector buttons alone are wider than the available width. In
narrow dashboards this returns a two-row plain layout even though the selector
row itself cannot fit, so the selector controls get clipped. Treat this as
overflow and force scroll mode.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=a1828b0f3965e2574fca0727fec2c53f8b58c8ef39d52cd0c15e46d379ea64db&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=a1828b0f3965e2574fca0727fec2c53f8b58c8ef39d52cd0c15e46d379ea64db&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:
##########
@@ -634,4 +722,137 @@ describe('Bar Chart X-axis Time Formatting', () => {
expect(hiddenSeries.length).toBe(3);
});
});
+
+ describe('Legend layout regressions', () => {
+ const getBottomLegendLayout = (
+ chartWidth: number,
+ legendItems: string[],
+ legendMargin?: string | number | null,
+ ) =>
+ getLegendLayoutResult({
+ availableWidth: getHorizontalLegendAvailableWidth({
+ chartWidth,
+ orientation: LegendOrientation.Bottom,
+ padding: getPadding(
+ true,
+ LegendOrientation.Bottom,
+ false,
+ false,
+ legendMargin,
+ false,
+ undefined,
+ undefined,
+ undefined,
+ true,
+ ),
+ }),
+ chartHeight: baseChartPropsConfig.height,
+ chartWidth,
+ legendItems,
+ legendMargin,
+ orientation: LegendOrientation.Bottom,
+ show: true,
+ theme: supersetTheme,
+ type: LegendType.Plain,
+ });
+
+ test('should fall back to scroll for horizontal bottom legends after
margin expansion reduces available width', () => {
+ const legendLabels = [
+ 'This is a long sales legend',
+ 'This is a long marketing legend',
+ 'This is a long operations legend',
+ ];
+ const longLegendData: ChartDataResponseResult[] = [
+ createTestQueryData(
+ [
+ {
+ [legendLabels[0]]: 100,
+ __timestamp: 1609459200000,
+ },
+ {
+ [legendLabels[1]]: 150,
+ __timestamp: 1612137600000,
+ },
+ {
+ [legendLabels[2]]: 200,
+ __timestamp: 1614556800000,
+ },
Review Comment:
**Suggestion:** The regression dataset is shaped incorrectly: each row only
contains one metric key, but series extraction derives legend series from row
keys and expects all metric columns in each row. This can collapse the legend
to a single item and make the test fail or stop validating the intended
overflow behavior. Populate each timestamp row with all legend metric fields.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Regression test covers wrong legend-width scenario.
- ⚠️ Real clipping regressions can slip through CI.
```
</details>
```suggestion
{
[legendLabels[0]]: 100,
[legendLabels[1]]: 150,
[legendLabels[2]]: 200,
__timestamp: 1609459200000,
},
{
[legendLabels[0]]: 110,
[legendLabels[1]]: 160,
[legendLabels[2]]: 210,
__timestamp: 1612137600000,
},
{
[legendLabels[0]]: 120,
[legendLabels[1]]: 170,
[legendLabels[2]]: 220,
__timestamp: 1614556800000,
},
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the regression test `should fall back to scroll...` in
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts:759`;
it builds `longLegendData` at lines 765-791 where each row contains only one
metric key.
2. The test calls `transformProps` at line 810, which calls
`extractSeries(...)` at
`src/Timeseries/transformProps.ts:265`.
3. `extractSeries` calls `sortAndFilterSeries` (`src/utils/series.ts:215`),
and series
names come from `Object.keys(rows[0])` (`src/utils/series.ts:55-57`), so
only the first
row's metric key becomes a series.
4. `legendData` is then built from `rawSeries` names
(`src/Timeseries/transformProps.ts:250-257`), collapsing legend items and
making the
overflow test validate an undersized legend scenario instead of the intended
multi-item
clipping path.
```
</details>
<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:** 768:779
**Comment:**
*Logic Error: The regression dataset is shaped incorrectly: each row
only contains one metric key, but series extraction derives legend series from
row keys and expects all metric columns in each row. This can collapse the
legend to a single item and make the test fail or stop validating the intended
overflow behavior. Populate each timestamp row with all legend metric fields.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=78c6b94ebc802613f90376f5619de8f8e8a635551465b9146d67d6d7fd0ec92e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=78c6b94ebc802613f90376f5619de8f8e8a635551465b9146d67d6d7fd0ec92e&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -55,6 +55,340 @@ function isDefined<T>(value: T | undefined | null): boolean
{
return value !== undefined && value !== null;
}
+const DEFAULT_LEGEND_ITEM_GAP = 10;
+const DEFAULT_LEGEND_ICON_WIDTH = 25;
+const LEGEND_ICON_LABEL_GAP = 5;
+const LEGEND_HORIZONTAL_SIDE_GUTTER = 16;
+const LEGEND_HORIZONTAL_ROW_HEIGHT = 24;
+const LEGEND_HORIZONTAL_MAX_ROWS = 2;
+const LEGEND_HORIZONTAL_MAX_HEIGHT_RATIO = 0.25;
+const LEGEND_VERTICAL_SIDE_GUTTER = 16;
+const LEGEND_VERTICAL_ROW_HEIGHT = 24;
+const LEGEND_VERTICAL_MAX_WIDTH_RATIO = 0.4;
+const LEGEND_SELECTOR_GAP = 10;
+const LEGEND_MARGIN_GUTTER = 45;
+// ECharts does not expose pre-render measurements for plain legends, so these
+// values intentionally overestimate selector space to avoid clipping.
+const ESTIMATED_LEGEND_SELECTOR_WIDTH = 112;
+const LEGEND_TEXT_WIDTH_CACHE = new Map<string, number>();
+
+type LegendDataItem =
+ | string
+ | number
+ | null
+ | undefined
+ | { name?: string | number | null };
+
+export type LegendLayoutResult = {
+ effectiveMargin?: number;
+ effectiveType: LegendType;
+};
+
+const SCROLL_LEGEND_LAYOUT: LegendLayoutResult = {
+ effectiveType: LegendType.Scroll,
+};
+
+function getLegendLabel(item: LegendDataItem): string {
+ if (typeof item === 'string' || typeof item === 'number') {
+ return String(item);
+ }
+
+ if (item?.name === undefined || item.name === null) {
+ return '';
+ }
+
+ return String(item.name);
+}
+
+function measureLegendTextWidth(text: string, theme: SupersetTheme): number {
+ const cacheKey = `${theme.fontFamily}:${theme.fontSizeSM}:${text}`;
+ const cachedWidth = LEGEND_TEXT_WIDTH_CACHE.get(cacheKey);
+ if (cachedWidth !== undefined) {
+ return cachedWidth;
+ }
+
+ let width = text.length * theme.fontSizeSM * 0.62;
+
+ if (typeof document !== 'undefined') {
+ const canvas = document.createElement('canvas');
+ const context = canvas.getContext('2d');
+ if (context) {
+ context.font = `${theme.fontSizeSM}px ${theme.fontFamily}`;
+ width = context.measureText(text).width;
+ }
+ }
+
+ LEGEND_TEXT_WIDTH_CACHE.set(cacheKey, width);
+ return width;
+}
+
+function getLegendLabels(items: LegendDataItem[]): string[] {
+ return items.map(getLegendLabel).filter(Boolean);
+}
+
+function getLegendItemWidths(labels: string[], theme: SupersetTheme): number[]
{
+ return labels.map(
+ label =>
+ DEFAULT_LEGEND_ICON_WIDTH +
+ LEGEND_ICON_LABEL_GAP +
+ measureLegendTextWidth(label, theme),
+ );
+}
+
+function estimateHorizontalLegendRows(
+ labels: string[],
+ chartWidth: number,
+ showSelectors: boolean,
+ theme: SupersetTheme,
+): number {
+ const availableWidth = Math.max(
+ chartWidth - LEGEND_HORIZONTAL_SIDE_GUTTER,
+ 0,
+ );
+ if (availableWidth === 0) {
+ return Infinity;
+ }
+
+ const legendItemWidths = getLegendItemWidths(labels, theme);
+
+ if (legendItemWidths.length === 0) {
+ return 1;
+ }
+
+ let rows = 1;
+ let rowWidth = 0;
+
+ for (const itemWidth of legendItemWidths) {
+ if (itemWidth > availableWidth) {
+ return Infinity;
+ }
+
+ const nextWidth =
+ rowWidth === 0
+ ? itemWidth
+ : rowWidth + DEFAULT_LEGEND_ITEM_GAP + itemWidth;
+ if (rowWidth > 0 && nextWidth > availableWidth) {
+ rows += 1;
+ rowWidth = itemWidth;
+ } else {
+ rowWidth = nextWidth;
+ }
+ }
+
+ if (showSelectors) {
+ const selectorWidth =
+ rowWidth === 0
+ ? ESTIMATED_LEGEND_SELECTOR_WIDTH
+ : rowWidth + LEGEND_SELECTOR_GAP + ESTIMATED_LEGEND_SELECTOR_WIDTH;
+ if (rowWidth > 0 && selectorWidth > availableWidth) {
+ rows += 1;
+ }
+ }
+
+ return rows;
+}
+
+export function getHorizontalLegendAvailableWidth({
+ chartWidth,
+ orientation,
+ padding,
+ zoomable = false,
+}: {
+ chartWidth: number;
+ orientation: LegendOrientation.Top | LegendOrientation.Bottom;
+ padding?: LegendPaddingType;
+ zoomable?: boolean;
+}): number {
+ let availableWidth = chartWidth - (padding?.left ?? 0);
+
+ if (orientation === LegendOrientation.Top && zoomable) {
+ availableWidth -= TIMESERIES_CONSTANTS.legendTopRightOffset;
+ }
+
+ return Math.max(availableWidth, 0);
+}
+
+function getLongestLegendLabelWidth(
+ labels: string[],
+ theme: SupersetTheme,
+): number {
+ return labels.reduce(
+ (maxWidth, label) =>
+ Math.max(maxWidth, measureLegendTextWidth(label, theme)),
+ 0,
+ );
+}
+
+function isHorizontalLegendOrientation(
+ orientation: LegendOrientation,
+): orientation is LegendOrientation.Top | LegendOrientation.Bottom {
+ return (
+ orientation === LegendOrientation.Top ||
+ orientation === LegendOrientation.Bottom
+ );
+}
+
+function getHorizontalPlainLegendLayout({
+ availableHeight,
+ availableWidth,
+ currentMargin,
+ legendLabels,
+ orientation,
+ showSelectors,
+ theme,
+}: {
+ availableHeight: number;
+ availableWidth: number;
+ currentMargin: number;
+ legendLabels: string[];
+ orientation: LegendOrientation.Top | LegendOrientation.Bottom;
+ showSelectors: boolean;
+ theme: SupersetTheme;
+}): LegendLayoutResult {
+ const rowCount = estimateHorizontalLegendRows(
+ legendLabels,
+ availableWidth,
+ showSelectors,
+ theme,
+ );
+ 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;
+ }
+
+ return {
+ effectiveMargin: Math.max(currentMargin, requiredMargin),
+ effectiveType: LegendType.Plain,
+ };
+}
+
+function getVerticalPlainLegendLayout({
+ availableHeight,
+ availableWidth,
+ currentMargin,
+ legendLabels,
+ showSelectors,
+ theme,
+}: {
+ availableHeight: number;
+ availableWidth: number;
+ currentMargin: number;
+ legendLabels: string[];
+ showSelectors: boolean;
+ theme: SupersetTheme;
+}): LegendLayoutResult {
+ if (legendLabels.length === 0) {
+ return {
+ effectiveMargin: currentMargin,
+ effectiveType: LegendType.Plain,
+ };
+ }
+
+ const selectorHeight = showSelectors
+ ? LEGEND_VERTICAL_ROW_HEIGHT + LEGEND_SELECTOR_GAP
+ : 0;
+ const effectiveAvailableHeight = Math.max(
+ availableHeight - LEGEND_VERTICAL_SIDE_GUTTER - selectorHeight,
+ 0,
+ );
+ const rowsPerColumn = Math.floor(
+ (effectiveAvailableHeight + DEFAULT_LEGEND_ITEM_GAP) /
+ (LEGEND_VERTICAL_ROW_HEIGHT + DEFAULT_LEGEND_ITEM_GAP),
+ );
+ const requiredMargin = Math.ceil(
+ getLongestLegendLabelWidth(legendLabels, theme) + LEGEND_MARGIN_GUTTER,
+ );
Review Comment:
**Suggestion:** Vertical plain legend width calculation ignores selector
button width and only measures text labels. For short labels with selectors
enabled, this underestimates required margin and can keep plain mode while the
selector UI clips. Include selector width in required margin estimation. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Left/right legends clip selector buttons with short labels.
- ❌ Scroll fallback skipped for vertical constrained layouts.
```
</details>
```suggestion
const requiredMargin = Math.ceil(
Math.max(
getLongestLegendLabelWidth(legendLabels, theme) + LEGEND_MARGIN_GUTTER,
showSelectors
? ESTIMATED_LEGEND_SELECTOR_WIDTH + LEGEND_MARGIN_GUTTER
: 0,
),
);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Choose legend type `plain` (list) from shared control
(`src/controls.tsx:67-76`) and
set legend orientation Left/Right in any ECharts plugin using shared layout
(`Graph/Pie/Funnel/Bubble/Radar/Gantt transformProps`, e.g.
`src/Graph/transformProps.ts:306-315`).
2. Plugin transform calls `getLegendLayoutResult(...)` with `type:
legendType` and `show:
showLegend` (e.g. `Graph/transformProps.ts:306-315`), then applies
`getLegendProps(...)`
with selector buttons enabled by default (`src/utils/series.ts:784`).
3. Vertical sizing path (`src/utils/series.ts:276-329`) subtracts selector
height
(`series.ts:298-303`) but computes width only from longest label
(`series.ts:309-311`),
ignoring selector button width.
4. With short labels and narrow chart width, function can keep
`LegendType.Plain` though
selector controls require more width; legend UI clips instead of switching
to scroll
(`series.ts:317-323` not triggered).
```
</details>
<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/utils/series.ts
**Line:** 309:311
**Comment:**
*Logic Error: Vertical plain legend width calculation ignores selector
button width and only measures text labels. For short labels with selectors
enabled, this underestimates required margin and can keep plain mode while the
selector UI clips. Include selector width in required margin estimation.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=37f12587e9901004203a6adc19e7df45db318449066d4f7b66a82d3fb248f765&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=37f12587e9901004203a6adc19e7df45db318449066d4f7b66a82d3fb248f765&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]
