Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ import {
xAxisBounds,
xAxisLabelRotation,
xAxisLabelInterval,
forceMaxInterval,
forceMaxInterval, showColorByXAxisSection,
} from '../../../controls';

import { OrientationType } from '../../types';
Expand Down Expand Up @@ -325,6 +325,7 @@ const config: ControlPanelConfig = {
['color_scheme'],
['time_shift_color'],
...showValueSection,
...showColorByXAxisSection,
[
{
name: 'stackDimension',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ export default function transformProps(
seriesType,
showLegend,
showValue,
showColorByXAxis,
sliceId,
sortSeriesType,
sortSeriesAscending,
Expand Down Expand Up @@ -356,6 +357,7 @@ export default function transformProps(
timeCompare: array,
timeShiftColor,
theme,
showColorByXAxis,
},
);
if (transformedSeries) {
Expand All @@ -373,6 +375,53 @@ export default function transformProps(
}
});

// Add x-axis color legend when showColorByXAxis is enabled
if (showColorByXAxis && groupBy.length === 0 && series.length > 0) {
// Hide original series from legend
series.forEach(s => {
s.legendHoverLink = false;
});

// Get x-axis values from the first series
const firstSeries = series[0];
if (firstSeries && Array.isArray(firstSeries.data)) {
const xAxisValues: (string | number)[] = [];

// Extract x-axis values
(firstSeries.data as any[]).forEach(point => {
let xValue;
if (point && typeof point === 'object' && 'value' in point) {
const val = point.value;
xValue = Array.isArray(val) ? val[0] : val;
} else if (Array.isArray(point)) {
xValue = point[0];
} else {
xValue = point;
}
xAxisValues.push(xValue);
});

// Create hidden series for legend (using 'line' type to not affect bar width)
xAxisValues.forEach(xValue => {
const colorKey = String(xValue);
series.push({
name: String(xValue),
Comment on lines +405 to +408
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Performance & correctness: when creating hidden legend series you append one series per x-axis data point without deduplicating, which can create duplicate legend entries and a large number of unnecessary series; deduplicate x-axis values first before pushing hidden series. [performance]

Severity Level: Major ⚠️
- ❌ Timeseries chart legend shows duplicate entries.
- ⚠️ Increased series[] size slows ECharts rendering.
- ⚠️ Legend clutter harms user color interpretation.
Suggested change
xAxisValues.forEach(xValue => {
const colorKey = String(xValue);
series.push({
name: String(xValue),
// Deduplicate x-axis values to avoid duplicate legend entries and excessive series
const uniqueXValues = Array.from(
new Set(
xAxisValues
.map(v => (v === null || v === undefined ? String(v) : String(v))),
),
);
uniqueXValues.forEach(xValue => {
const colorKey = String(xValue);
series.push({
name: xValue,
Steps of Reproduction ✅
1. Open the Timeseries chart code path: transformProps in
plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts (function transformProps).
The x-axis legend augmentation code lives at lines ~378-423 and the hidden-series creation
is at lines 404-421.

2. Enable the new option in formData: showColorByXAxis = true and ensure groupBy is empty
(groupBy.length === 0). These formData values are read at the top of transformProps
(extracted into showColorByXAxis and groupBy).

3. Provide a dataset such that the first rendered series (series[0]) contains repeated x
values (e.g., firstSeries.data contains multiple points with the same x value).
transformProps extracts xAxisValues by iterating firstSeries.data and pushing every x it
finds (lines 390-402).

4. transformProps then executes the hidden-series creation loop at lines 404-421, pushing
one hidden series per xAxisValues entry (no deduplication). Observe duplicate legend items
in the rendered chart legend (legend.data is later driven by legendData built from the
same firstSeries.data at lines ~565-582), and a larger-than-necessary series[] length
causing extra work when ECharts processes series.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 405:408
**Comment:**
	*Performance: Performance & correctness: when creating hidden legend series you append one series per x-axis data point without deduplicating, which can create duplicate legend entries and a large number of unnecessary series; deduplicate x-axis values first before pushing hidden 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.

type: 'line', // Use line type to not affect bar positioning
data: [], // Empty - doesn't render
itemStyle: {
color: colorScale(colorKey, sliceId),
},
lineStyle: {
color: colorScale(colorKey, sliceId),
},
silent: true,
legendHoverLink: false,
showSymbol: false,
});
});
}
}

if (stack === StackControlsValue.Stream) {
const baselineSeries = getBaselineSeriesForStream(
series.map(entry => entry.data) as [string | number, number][][],
Expand Down Expand Up @@ -513,14 +562,34 @@ export default function transformProps(
isHorizontal,
);

const legendData = rawSeries
.filter(
entry =>
extractForecastSeriesContext(entry.name || '').type ===
ForecastSeriesEnum.Observation,
)
.map(entry => entry.name || '')
.concat(extractAnnotationLabels(annotationLayers));
const legendData = showColorByXAxis && groupBy.length === 0 && series.length > 0
? // When showColorByXAxis is enabled, show only x-axis values
(() => {
const firstSeries = series[0];
if (firstSeries && Array.isArray(firstSeries.data)) {
return (firstSeries.data as any[]).map(point => {
if (point && typeof point === 'object' && 'value' in point) {
const val = point.value;
return String(Array.isArray(val) ? val[0] : val);
} else if (Array.isArray(point)) {
return String(point[0]);
} else {
return String(point);
}
});
}
Comment on lines +566 to +580
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Logic/robustness: legendData is built from firstSeries.data and can include duplicate or empty/undefined entries; filter out empty/undefined names and deduplicate them so legend items are stable and meaningful. [logic error]

Severity Level: Major ⚠️
- ❌ Timeseries legend shows empty or duplicated labels.
- ⚠️ Legend sorting and selection behave inconsistently.
- ⚠️ User confusion interpreting color-by-x legend.
Suggested change
? // When showColorByXAxis is enabled, show only x-axis values
(() => {
const firstSeries = series[0];
if (firstSeries && Array.isArray(firstSeries.data)) {
return (firstSeries.data as any[]).map(point => {
if (point && typeof point === 'object' && 'value' in point) {
const val = point.value;
return String(Array.isArray(val) ? val[0] : val);
} else if (Array.isArray(point)) {
return String(point[0]);
} else {
return String(point);
}
});
}
? // When showColorByXAxis is enabled, show only x-axis values (filtered + deduped)
(() => {
const firstSeries = series[0];
if (firstSeries && Array.isArray(firstSeries.data)) {
const names = (firstSeries.data as any[]).map(point => {
if (point && typeof point === 'object' && 'value' in point) {
const val = point.value;
return String(Array.isArray(val) ? val[0] : val);
} else if (Array.isArray(point)) {
return String(point[0]);
} else {
return String(point);
}
})
// filter out empty/invalid entries and deduplicate
.filter(name => name !== '' && name !== 'undefined' && name !== 'null');
return Array.from(new Set(names));
Steps of Reproduction ✅
1. Call transformProps in plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
with formData.showColorByXAxis = true and no groupBy (groupBy.length === 0). The
legendData computation is at lines ~565-592.

2. If the first rendered series (series[0]) contains points where the x value is
null/undefined/empty, the mapping at lines 569-578 will convert those to the strings
"null" or "undefined" or empty strings, and include them in the returned array.

3. If the firstSeries.data contains duplicate x values, the current map returns duplicates
(no deduplication). These values are later fed to echartOptions.legend.data (see legend
configuration at lines ~771-792), producing duplicate or meaningless legend entries in the
chart UI.

4. Reproduce in UI: render a Timeseries chart with showColorByXAxis enabled and a dataset
where firstSeries.data includes duplicate or empty x values; verify duplicate/empty legend
items appear. Fixing by filtering out empty values and deduplicating (as suggested) yields
stable, meaningful legend items.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 566:580
**Comment:**
	*Logic Error: Logic/robustness: `legendData` is built from `firstSeries.data` and can include duplicate or empty/undefined entries; filter out empty/undefined names and deduplicate them so legend items are stable and meaningful.

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.

return [];
})()
: // Otherwise show original series names
rawSeries
.filter(
entry =>
extractForecastSeriesContext(entry.name || '').type ===
ForecastSeriesEnum.Observation,
)
.map(entry => entry.name || '')
.concat(extractAnnotationLabels(annotationLayers));


let xAxis: any = {
type: xAxisType,
Expand Down Expand Up @@ -702,10 +771,24 @@ export default function transformProps(
padding,
),
scrollDataIndex: legendIndex || 0,
data: legendData.sort((a: string, b: string) => {
if (!legendSort) return 0;
return legendSort === 'asc' ? a.localeCompare(b) : b.localeCompare(a);
}) as string[],
data: showColorByXAxis && groupBy.length === 0
? // When showColorByXAxis, configure legend items with roundRect icons
legendData.map(name => ({
name,
icon: 'roundRect',
}))
: // Otherwise use normal legend data
legendData.sort((a: string, b: string) => {
if (!legendSort) return 0;
return legendSort === 'asc' ? a.localeCompare(b) : b.localeCompare(a);
}),
// Disable legend selection and buttons when showColorByXAxis is enabled
...(showColorByXAxis && groupBy.length === 0
? {
selectedMode: false, // Disable clicking legend items
selector: false, // Hide All/Invert buttons
}
: {}),
},
series: dedupSeries(reorderForecastSeries(series) as SeriesOption[]),
toolbox: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,27 @@ export function transformNegativeLabelsPosition(
return (series.data as TimeseriesDataRecord[]).map(transformValue);
}

export function applyColorByXAxis(
series: SeriesOption,
colorScale: CategoricalColorScale,
sliceId: number | undefined,
opacity: number,
): TimeseriesDataRecord[] {
return (series.data as [string | number, number][]).map(value => {
// Use x-axis value as color key so same values get same colors across charts
const colorKey = String(value[0]);

return {
value,
itemStyle: {
color: colorScale(colorKey, sliceId),
opacity,
borderWidth: 0,
},
};
});
}

export function transformSeries(
series: SeriesOption,
colorScale: CategoricalColorScale,
Expand Down Expand Up @@ -196,6 +217,7 @@ export function transformSeries(
timeCompare?: string[];
timeShiftColor?: boolean;
theme?: SupersetTheme;
showColorByXAxis?: boolean;
},
): SeriesOption | undefined {
const { name, data } = series;
Expand Down Expand Up @@ -226,6 +248,7 @@ export function transformSeries(
timeCompare = [],
timeShiftColor,
theme,
showColorByXAxis = false,
} = opts;
const contexts = seriesContexts[name || ''] || [];
const hasForecast =
Expand Down Expand Up @@ -327,14 +350,18 @@ export function transformSeries(

return {
...series,
...(Array.isArray(data) && seriesType === 'bar' && !stack
? { data: transformNegativeLabelsPosition(series, isHorizontal) }
...(Array.isArray(data)
? showColorByXAxis
? { data: applyColorByXAxis(series, colorScale, sliceId, opacity) }
: seriesType === 'bar' && !stack
? { data: transformNegativeLabelsPosition(series, isHorizontal) }
: null
: null),
connectNulls,
queryIndex,
yAxisIndex,
name: forecastSeries.name,
itemStyle,
...(showColorByXAxis ? {} : { itemStyle }),
// @ts-ignore
type: plotType,
smooth: seriesType === 'smooth',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ export type EchartsTimeseriesFormData = QueryFormData & {
onlyTotal: boolean;
showExtraControls: boolean;
percentageThreshold: number;
showColorByXAxis?: boolean;
orientation?: OrientationType;
} & LegendFormData &
TitleFormData;
Expand Down
18 changes: 18 additions & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,20 @@ export const showValueControl: ControlSetItem = {
},
};

export const showColorbyXAxisControl: ControlSetItem = {
name: 'show_color_by_x_axis',
config: {
type: 'CheckboxControl',
label: t('Color By X-Axis'),
default: false,
renderTrigger: true,
description: t('Color bars by x-axis'),
visibility: ({ controls }: { controls: any }) =>
(!controls?.stack?.value || controls?.stack?.value === null) &&
(!controls?.groupby?.value || controls?.groupby?.value?.length === 0),
},
};

export const stackControl: ControlSetItem = {
name: 'stack',
config: {
Expand Down Expand Up @@ -187,6 +201,10 @@ export const showValueSection: ControlSetRow[] = [
[percentageThresholdControl],
];

export const showColorByXAxisSection: ControlSetRow[] = [
[showColorbyXAxisControl],
];

export const showValueSectionWithoutStack: ControlSetRow[] = [
[showValueControl],
[onlyTotalControl],
Expand Down
Loading
Loading