codeant-ai-for-open-source[bot] commented on code in PR #38675:
URL: https://github.com/apache/superset/pull/38675#discussion_r2942453859
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -55,6 +55,352 @@ 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));
+ }
+ }
+
+ LEGEND_TEXT_WIDTH_CACHE.set(cacheKey, width);
+ return width;
+}
+
+function getLegendLabels(items: LegendDataItem[]): string[] {
+ return items.map(getLegendLabel).filter(Boolean);
Review Comment:
**Suggestion:** Using `filter(Boolean)` drops valid empty-string legend
labels, which makes width estimation skip those legend entries entirely. This
underestimates plain legend space requirements and can keep plain mode when it
should fall back to scroll, causing clipping for datasets containing empty
string category names. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Plain legends may overflow with empty-string categories.
- ❌ Dashboard legend clipping persists for affected datasets.
```
</details>
```suggestion
return items
.filter(
item =>
item !== null &&
item !== undefined &&
(typeof item !== 'object' ||
(item.name !== null && item.name !== undefined)),
)
.map(getLegendLabel);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use a chart path where legend names come from query groupby values (e.g.
Pie builds
each name via `extractGroupbyLabel` at `src/Pie/transformProps.ts:265`;
Funnel uses same
function at `src/Funnel/transformProps.ts:174`).
2. `extractGroupbyLabel` delegates to `formatSeriesName`, which returns
string values
unchanged (including empty string) at `src/utils/series.ts:708` and
`src/utils/series.ts:754`.
3. Those names are passed into `getLegendLayoutResult` as `legendItems` (e.g.
`src/Pie/transformProps.ts:393`, `src/Timeseries/transformProps.ts:719`),
then
`getLegendLabels()` runs `filter(Boolean)` at `src/utils/series.ts:126`,
dropping `''`.
4. Layout estimation undercounts legend entries, so plain legend can be kept
when it
should switch to scroll; actual legend rendering still uses full
`legendData` (e.g.
`src/Pie/transformProps.ts:113`), causing clipping in tight-width dashboard
panels.
```
</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:** 126:126
**Comment:**
*Logic Error: Using `filter(Boolean)` drops valid empty-string legend
labels, which makes width estimation skip those legend entries entirely. This
underestimates plain legend space requirements and can keep plain mode when it
should fall back to scroll, causing clipping for datasets containing empty
string category names.
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=602416eb7fe14b66cf864df5826237a07aa8785b4097f79004401dfd51ae2b71&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=602416eb7fe14b66cf864df5826237a07aa8785b4097f79004401dfd51ae2b71&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts:
##########
@@ -891,32 +893,227 @@ describe('getLegendProps', () => {
});
});
- test('should default plain legends to scroll for bottom orientation', () => {
+ test('should return the correct props for plain type with bottom
orientation', () => {
expect(
getLegendProps(LegendType.Plain, LegendOrientation.Bottom, false, theme),
).toEqual({
show: false,
bottom: 0,
+ right: 0,
orient: 'horizontal',
- type: 'scroll',
+ type: 'plain',
...expectedThemeProps,
});
});
- test('should default plain legends to scroll for top orientation', () => {
+ test('should return the correct props for plain type with top orientation',
() => {
expect(
getLegendProps(LegendType.Plain, LegendOrientation.Top, false, theme),
).toEqual({
show: false,
top: 0,
right: 0,
orient: 'horizontal',
- type: 'scroll',
+ type: 'plain',
...expectedThemeProps,
});
});
});
+describe('getLegendLayoutResult', () => {
+ test('keeps plain horizontal legends when they fit within two rows', () => {
+ expect(
+ getLegendLayoutResult({
+ chartHeight: 400,
+ chartWidth: 800,
+ legendItems: ['Alpha', 'Beta', 'Gamma', 'Delta'],
+ legendMargin: null,
+ orientation: LegendOrientation.Top,
+ show: true,
+ theme,
+ type: LegendType.Plain,
+ }),
+ ).toEqual({
+ effectiveMargin: defaultLegendPadding[LegendOrientation.Top],
+ effectiveType: LegendType.Plain,
+ });
+ });
+
+ test('adds extra margin for wrapped plain horizontal legends', () => {
+ expect(
+ getLegendLayoutResult({
+ chartHeight: 400,
+ chartWidth: 640,
+ legendItems: [
+ 'This is a long legend label',
+ 'Another long legend label',
+ 'Third long legend label',
+ ],
+ legendMargin: null,
+ orientation: LegendOrientation.Top,
+ show: true,
+ theme,
+ type: LegendType.Plain,
+ }),
+ ).toEqual({
+ effectiveMargin: 44,
+ effectiveType: LegendType.Plain,
+ });
Review Comment:
**Suggestion:** This test hardcodes `effectiveMargin: 44`, but that value
depends on runtime text measurement behavior (canvas/context/font
availability). In environments where `measureText` is available or fonts
differ, the computed margin can change and make this test fail even when
behavior is correct. Assert the stable contract (plain legend is kept and
margin grows beyond default) instead of an exact pixel value. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ ECharts plugin unit tests fail across differing runtimes.
- ⚠️ CI/local parity reduced for legend layout tests.
```
</details>
```suggestion
const layout = getLegendLayoutResult({
chartHeight: 400,
chartWidth: 640,
legendItems: [
'This is a long legend label',
'Another long legend label',
'Third long legend label',
],
legendMargin: null,
orientation: LegendOrientation.Top,
show: true,
theme,
type: LegendType.Plain,
});
expect(layout).toMatchObject({
effectiveType: LegendType.Plain,
});
expect(layout.effectiveMargin).toBeGreaterThan(
defaultLegendPadding[LegendOrientation.Top],
);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the unit test `adds extra margin for wrapped plain horizontal
legends` in
`superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts:942-962`
(via
Jest configured in `superset-frontend/jest.config.js:21-53`).
2. The test calls `getLegendLayoutResult()` (`series.ts:343-402`), which
routes horizontal
legends to `getHorizontalPlainLegendLayout()` (`series.ts:237-280`).
3. That path computes item widths through `measureLegendTextWidth()`
(`series.ts:103-123`), which switches behavior at `series.ts:112-118`:
fallback estimate
vs `canvas.getContext('2d').measureText(...)`.
4. In environments where canvas/text measurement differs, computed
`requiredMargin`
changes (`series.ts:260-277`), so strict `effectiveMargin: 44` assertion
fails although
`effectiveType: Plain` behavior is still correct.
```
</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/utils/series.test.ts
**Line:** 943:961
**Comment:**
*Logic Error: This test hardcodes `effectiveMargin: 44`, but that value
depends on runtime text measurement behavior (canvas/context/font
availability). In environments where `measureText` is available or fonts
differ, the computed margin can change and make this test fail even when
behavior is correct. Assert the stable contract (plain legend is kept and
margin grows beyond default) instead of an exact pixel value.
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=a9d95300ad31bfd58d8c152406c5f404980df224343ad8b4c87d9b3fee6e0e23&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=a9d95300ad31bfd58d8c152406c5f404980df224343ad8b4c87d9b3fee6e0e23&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts:
##########
@@ -891,32 +893,227 @@ describe('getLegendProps', () => {
});
});
- test('should default plain legends to scroll for bottom orientation', () => {
+ test('should return the correct props for plain type with bottom
orientation', () => {
expect(
getLegendProps(LegendType.Plain, LegendOrientation.Bottom, false, theme),
).toEqual({
show: false,
bottom: 0,
+ right: 0,
orient: 'horizontal',
- type: 'scroll',
+ type: 'plain',
...expectedThemeProps,
});
});
- test('should default plain legends to scroll for top orientation', () => {
+ test('should return the correct props for plain type with top orientation',
() => {
expect(
getLegendProps(LegendType.Plain, LegendOrientation.Top, false, theme),
).toEqual({
show: false,
top: 0,
right: 0,
orient: 'horizontal',
- type: 'scroll',
+ type: 'plain',
...expectedThemeProps,
});
});
});
+describe('getLegendLayoutResult', () => {
+ test('keeps plain horizontal legends when they fit within two rows', () => {
+ expect(
+ getLegendLayoutResult({
+ chartHeight: 400,
+ chartWidth: 800,
+ legendItems: ['Alpha', 'Beta', 'Gamma', 'Delta'],
+ legendMargin: null,
+ orientation: LegendOrientation.Top,
+ show: true,
+ theme,
+ type: LegendType.Plain,
+ }),
+ ).toEqual({
+ effectiveMargin: defaultLegendPadding[LegendOrientation.Top],
+ effectiveType: LegendType.Plain,
+ });
+ });
+
+ test('adds extra margin for wrapped plain horizontal legends', () => {
+ expect(
+ getLegendLayoutResult({
+ chartHeight: 400,
+ chartWidth: 640,
+ legendItems: [
+ 'This is a long legend label',
+ 'Another long legend label',
+ 'Third long legend label',
+ ],
+ legendMargin: null,
+ orientation: LegendOrientation.Top,
+ show: true,
+ theme,
+ type: LegendType.Plain,
+ }),
+ ).toEqual({
+ effectiveMargin: 44,
+ effectiveType: LegendType.Plain,
+ });
+ });
+
+ test('falls back to scroll when horizontal plain legends exceed two rows',
() => {
+ expect(
+ getLegendLayoutResult({
+ chartHeight: 400,
+ chartWidth: 240,
+ legendItems: [
+ 'This is a long legend label',
+ 'Another long legend label',
+ 'Third long legend label',
+ ],
+ legendMargin: null,
+ orientation: LegendOrientation.Top,
+ show: true,
+ theme,
+ type: LegendType.Plain,
+ }),
+ ).toEqual({
+ effectiveType: LegendType.Scroll,
+ });
+ });
+
+ test('falls back to scroll when a single horizontal plain legend item
exceeds available width', () => {
+ expect(
+ getLegendLayoutResult({
+ chartHeight: 400,
+ chartWidth: 260,
+ legendItems: [
+ 'This is a ridiculously long legend label that should not fit on one
line',
+ ],
+ legendMargin: null,
+ orientation: LegendOrientation.Top,
+ show: true,
+ theme,
+ type: LegendType.Plain,
+ }),
+ ).toEqual({
+ effectiveType: LegendType.Scroll,
+ });
+ });
+
+ test('falls back to scroll when reserved horizontal width reduces plain
legend capacity', () => {
+ const availableWidth = getHorizontalLegendAvailableWidth({
+ chartWidth: 265,
+ orientation: LegendOrientation.Top,
+ padding: { left: 20 },
+ zoomable: true,
+ });
+
+ expect(
+ getLegendLayoutResult({
+ availableWidth,
+ chartHeight: 400,
+ chartWidth: 265,
+ legendItems: ['Alpha', 'Beta', 'Gamma'],
+ legendMargin: null,
+ orientation: LegendOrientation.Top,
+ show: true,
+ theme,
+ type: LegendType.Plain,
+ }),
+ ).toEqual({
+ effectiveType: LegendType.Scroll,
+ });
+ });
+
+ test('falls back to scroll when horizontal legend selectors alone exceed
available width', () => {
+ expect(
+ getLegendLayoutResult({
+ chartHeight: 400,
+ chartWidth: 95,
+ legendItems: ['A'],
+ legendMargin: null,
+ orientation: LegendOrientation.Top,
+ show: true,
+ theme,
+ type: LegendType.Plain,
+ }),
+ ).toEqual({
+ effectiveType: LegendType.Scroll,
+ });
+ });
+
+ test('keeps plain vertical legends when they fit within a single column', ()
=> {
+ expect(
+ getLegendLayoutResult({
+ chartHeight: 400,
+ chartWidth: 800,
+ legendItems: ['Alpha', 'Beta', 'Gamma'],
+ legendMargin: null,
+ orientation: LegendOrientation.Left,
+ show: true,
+ theme,
+ type: LegendType.Plain,
+ }),
+ ).toEqual({
+ effectiveMargin: defaultLegendPadding[LegendOrientation.Left],
+ effectiveType: LegendType.Plain,
+ });
+ });
+
+ test('adds extra margin for wide vertical plain legends', () => {
+ expect(
+ getLegendLayoutResult({
+ chartHeight: 400,
+ chartWidth: 800,
+ legendItems: ['This is a very long legend label'],
+ legendMargin: null,
+ orientation: LegendOrientation.Left,
+ show: true,
+ theme,
+ type: LegendType.Plain,
+ }),
+ ).toEqual({
+ effectiveMargin: 284,
+ effectiveType: LegendType.Plain,
+ });
Review Comment:
**Suggestion:** This assertion hardcodes `effectiveMargin: 284`, which is
derived from text-width estimation and can vary by environment. That creates
brittle tests that can fail due to rendering/runtime differences rather than
actual regressions; validate that the legend remains plain and requires margin
expansion relative to default. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Vertical legend layout test may fail non-functionally.
- ⚠️ Frontend PR checks become environment-sensitive.
```
</details>
```suggestion
const layout = getLegendLayoutResult({
chartHeight: 400,
chartWidth: 800,
legendItems: ['This is a very long legend label'],
legendMargin: null,
orientation: LegendOrientation.Left,
show: true,
theme,
type: LegendType.Plain,
});
expect(layout).toMatchObject({
effectiveType: LegendType.Plain,
});
expect(layout.effectiveMargin).toBeGreaterThan(
defaultLegendPadding[LegendOrientation.Left],
);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run test `adds extra margin for wide vertical plain legends` in
`superset-frontend/plugins/plugin-chart-echarts/test/utils/series.test.ts:1064-1080`.
2. This executes `getLegendLayoutResult()` (`series.ts:343-402`) and then
vertical branch
`getVerticalPlainLegendLayout()` (`series.ts:282-341`).
3. Vertical required margin uses measured longest label width
(`series.ts:320`) from
`measureLegendTextWidth()` (`series.ts:103-123`), which may use real canvas
`measureText`
(`series.ts:112-118`) depending runtime.
4. Because `requiredMargin` is `Math.ceil(...)` (`series.ts:318-323`), small
measurement
differences can change exact value from `284`, making this strict assertion
brittle while
legend mode remains correctly `Plain`.
```
</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/utils/series.test.ts
**Line:** 1065:1079
**Comment:**
*Logic Error: This assertion hardcodes `effectiveMargin: 284`, which is
derived from text-width estimation and can vary by environment. That creates
brittle tests that can fail due to rendering/runtime differences rather than
actual regressions; validate that the legend remains plain and requires margin
expansion relative to default.
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=1bb6c47f5f6de23f9ed4c19ca15f958b13eb63119df5d840108f6467b53ec3e1&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38675&comment_hash=1bb6c47f5f6de23f9ed4c19ca15f958b13eb63119df5d840108f6467b53ec3e1&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]
