Copilot commented on code in PR #43189:
URL: https://github.com/apache/superset/pull/43189#discussion_r3797614639
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -68,7 +68,7 @@ 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>();
+const TEXT_WIDTH_CACHE = new Map<string, number>();
Review Comment:
TEXT_WIDTH_CACHE is an unbounded, module-level Map. With `measureTextWidth`
now used beyond legends (e.g., Gantt categories), this can grow without bound
across many distinct labels and lead to memory growth in long-lived sessions.
Consider adding an eviction policy (e.g., capped size + FIFO/LRU eviction) or
periodically clearing the cache when it exceeds a threshold.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -94,9 +94,9 @@ function getLegendLabel(item: LegendDataItem): string {
return String(item.name);
}
-function measureLegendTextWidth(text: string, theme: SupersetTheme): number {
+export function measureTextWidth(text: string, theme: SupersetTheme): number {
const cacheKey = `${theme.fontFamily}:${theme.fontSizeSM}:${text}`;
Review Comment:
`measureTextWidth` hardcodes `theme.fontSizeSM` into both measurement and
caching, which couples the function to a specific typography size that may not
match the actual ECharts label textStyle (e.g., markLine labels vs legend
labels). To keep measurements accurate as usage expands, consider accepting
explicit font options (fontFamily/fontSize/fontWeight) or a `textStyle` subset,
and incorporate those into the cache key.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -94,9 +94,9 @@ function getLegendLabel(item: LegendDataItem): string {
return String(item.name);
}
-function measureLegendTextWidth(text: string, theme: SupersetTheme): number {
+export function measureTextWidth(text: string, theme: SupersetTheme): number {
const cacheKey = `${theme.fontFamily}:${theme.fontSizeSM}:${text}`;
- const cachedWidth = LEGEND_TEXT_WIDTH_CACHE.get(cacheKey);
+ const cachedWidth = TEXT_WIDTH_CACHE.get(cacheKey);
if (cachedWidth !== undefined) {
return cachedWidth;
}
Review Comment:
TEXT_WIDTH_CACHE is an unbounded, module-level Map. With `measureTextWidth`
now used beyond legends (e.g., Gantt categories), this can grow without bound
across many distinct labels and lead to memory growth in long-lived sessions.
Consider adding an eviction policy (e.g., capped size + FIFO/LRU eviction) or
periodically clearing the cache when it exceeds a threshold.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Gantt/transformProps.test.ts:
##########
@@ -372,3 +375,49 @@ describe('legend sorting', () => {
expect((result.echartOptions.legend as any).show).toBe(true);
});
});
+
+test('reserves grid room for category names so they are not clipped (#38844)',
() => {
Review Comment:
This test relies on `.find(...)` returning a series and will throw a
less-informative error if it doesn’t. Add an explicit assertion (e.g.,
`expect(categoryMarkLine).toBeDefined()`) before dereferencing to make failures
clearer and easier to diagnose.
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -112,7 +112,7 @@ function measureLegendTextWidth(text: string, theme:
SupersetTheme): number {
}
}
- LEGEND_TEXT_WIDTH_CACHE.set(cacheKey, width);
+ TEXT_WIDTH_CACHE.set(cacheKey, width);
Review Comment:
TEXT_WIDTH_CACHE is an unbounded, module-level Map. With `measureTextWidth`
now used beyond legends (e.g., Gantt categories), this can grow without bound
across many distinct labels and lead to memory growth in long-lived sessions.
Consider adding an eviction policy (e.g., capped size + FIFO/LRU eviction) or
periodically clearing the cache when it exceeds a threshold.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Gantt/transformProps.test.ts:
##########
@@ -372,3 +375,49 @@ describe('legend sorting', () => {
expect((result.echartOptions.legend as any).show).toBe(true);
});
});
+
+test('reserves grid room for category names so they are not clipped (#38844)',
() => {
+ // The names are drawn as markLine labels, which `grid.containLabel` ignores.
+ const longCategory = 'A very long category name that would be clipped';
+ const props = new ChartProps({
+ ...chartPropsConfig,
+ width: 800,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: queriesData[0].data.map(datum => ({
+ ...datum,
+ 'Y Axis': longCategory,
+ })),
+ },
+ ],
+ });
+
+ const result = transformProps(props as EchartsGanttChartProps);
+ const grid = result.echartOptions.grid as { left: number };
+ const categoryMarkLine = (result.echartOptions.series as any[]).find(
+ series => series.markLine?.label?.formatter === '{b}',
+ );
+
+ const baseline = transformProps(
+ new ChartProps({
+ ...chartPropsConfig,
+ width: 800,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: queriesData[0].data.map(datum => ({ ...datum, 'Y Axis': 'a'
})),
+ },
+ ],
+ }) as EchartsGanttChartProps,
+ );
+
+ // a longer category reserves more left padding than a short one
+ expect(grid.left).toBeGreaterThan(
+ (baseline.echartOptions.grid as { left: number }).left,
+ );
+ // and the label truncates instead of overflowing whatever was reserved
+ expect(categoryMarkLine.markLine.label.overflow).toBe('truncate');
+ expect(categoryMarkLine.markLine.label.width).toBeGreaterThan(0);
Review Comment:
This test relies on `.find(...)` returning a series and will throw a
less-informative error if it doesn’t. Add an explicit assertion (e.g.,
`expect(categoryMarkLine).toBeDefined()`) before dereferencing to make failures
clearer and easier to diagnose.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Gantt/transformProps.test.ts:
##########
@@ -372,3 +375,49 @@ describe('legend sorting', () => {
expect((result.echartOptions.legend as any).show).toBe(true);
});
});
+
+test('reserves grid room for category names so they are not clipped (#38844)',
() => {
+ // The names are drawn as markLine labels, which `grid.containLabel` ignores.
+ const longCategory = 'A very long category name that would be clipped';
+ const props = new ChartProps({
+ ...chartPropsConfig,
+ width: 800,
+ queriesData: [
+ {
+ ...queriesData[0],
+ data: queriesData[0].data.map(datum => ({
+ ...datum,
+ 'Y Axis': longCategory,
+ })),
+ },
+ ],
+ });
+
+ const result = transformProps(props as EchartsGanttChartProps);
+ const grid = result.echartOptions.grid as { left: number };
+ const categoryMarkLine = (result.echartOptions.series as any[]).find(
+ series => series.markLine?.label?.formatter === '{b}',
+ );
Review Comment:
This test relies on `.find(...)` returning a series and will throw a
less-informative error if it doesn’t. Add an explicit assertion (e.g.,
`expect(categoryMarkLine).toBeDefined()`) before dereferencing to make failures
clearer and easier to diagnose.
--
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]
