From 32fe4f66128d22f611b39d319c72b89ab1bf66f4 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Tue, 1 Sep 2026 13:21:03 -0700 Subject: [PATCH 1/4] fix(echarts): make Bar chart value labels fit-aware and readable (#43144) Co-authored-by: Claude Sonnet 5 --- .../src/MixedTimeseries/transformProps.ts | 3 + .../src/Timeseries/EchartsTimeseries.test.tsx | 2 + .../src/Timeseries/constants.ts | 5 + .../src/Timeseries/transformProps.ts | 10 + .../src/Timeseries/transformers.ts | 272 ++++++++--- .../src/Timeseries/types.ts | 9 + .../plugin-chart-echarts/src/controls.tsx | 31 +- .../MixedTimeseries/transformProps.test.ts | 55 +++ .../test/Timeseries/Bar/controlPanel.test.ts | 86 ++-- .../Timeseries/Bar/transformProps.test.ts | 102 +++++ .../test/Timeseries/transformers.test.ts | 421 +++++++++++++++++- 11 files changed, 893 insertions(+), 103 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts index 10c8014c97af..64cdc7b287af 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -59,6 +59,7 @@ import { LegendOrientation, Refs, } from '../types'; +import { BarValueLabelPosition } from '../Timeseries/types'; import { parseAxisBound } from '../utils/controls'; import { safeParseEChartOptions } from '../utils/safeEChartOptionsParser'; import { @@ -519,6 +520,7 @@ export default function transformProps( areaOpacity: opacity, seriesType, showValue, + valueLabelPosition: BarValueLabelPosition.OutsideEnd, onlyTotal, stack: Boolean(stack), stackIdSuffix: '\na', @@ -608,6 +610,7 @@ export default function transformProps( areaOpacity: opacityB, seriesType: seriesTypeB, showValue: showValueB, + valueLabelPosition: BarValueLabelPosition.OutsideEnd, onlyTotal: onlyTotalB, stack: Boolean(stackB), stackIdSuffix: '\nb', diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.test.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.test.tsx index 0e7fb76bec18..b65e96c1b8c0 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.test.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.test.tsx @@ -40,6 +40,7 @@ import { } from '../types'; import EchartsTimeseries from './EchartsTimeseries'; import { + BarValueLabelPosition, EchartsTimeseriesSeriesType, OrientationType, type EchartsTimeseriesFormData, @@ -159,6 +160,7 @@ const defaultFormData: EchartsTimeseriesFormData & { xAxisLabelRotation: 0, xAxisLabelInterval: 0, showValue: false, + valueLabelPosition: BarValueLabelPosition.Auto, onlyTotal: false, showExtraControls: true, percentageThreshold: 0, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts index 10fcffe704b6..12c83d102b3a 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts @@ -23,6 +23,7 @@ import { import { t } from '@apache-superset/core/translation'; import { LegendOrientation, LegendType } from '../types'; import { + BarValueLabelPosition, OrientationType, EchartsTimeseriesSeriesType, EchartsTimeseriesFormData, @@ -88,6 +89,10 @@ export const DEFAULT_FORM_DATA: EchartsTimeseriesFormData = { xAxisLabelInterval: defaultXAxis.xAxisLabelInterval, groupby: [], showValue: false, + // Legacy charts saved before this field existed have no valueLabelPosition + // in form_data and must keep their pre-existing Outside End placement; + // Auto is opt-in via the Value label position control, not the default. + valueLabelPosition: BarValueLabelPosition.OutsideEnd, labelPosition: 'auto', onlyTotal: false, percentageThreshold: 0, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index 427847854e1c..4882dfff5e16 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -47,6 +47,7 @@ import { NumberFormats, } from '@superset-ui/core'; import { GenericDataType } from '@apache-superset/core/common'; +import { isThemeDark } from '@apache-superset/core/theme'; import { extractExtraMetrics, getOriginalSeries, @@ -63,6 +64,7 @@ import { EchartsTimeseriesChartProps, EchartsTimeseriesFormData, EchartsTimeseriesSeriesType, + BarValueLabelPosition, OrientationType, TimeseriesChartTransformedProps, } from './types'; @@ -295,6 +297,7 @@ export default function transformProps( seriesType, showLegend, showValue, + valueLabelPosition, size, labelPosition, colorByPrimaryAxis, @@ -333,6 +336,8 @@ export default function transformProps( zoomable, stackDimension, }: EchartsTimeseriesFormData = { ...DEFAULT_FORM_DATA, ...formData }; + const resolvedValueLabelPosition = + valueLabelPosition ?? BarValueLabelPosition.OutsideEnd; const refs: Refs = {}; const groupBy = ensureIsArray(groupby); @@ -743,6 +748,7 @@ export default function transformProps( labelMap?.[seriesName]?.[0], ) ?? defaultFormatter), showValue, + valueLabelPosition: resolvedValueLabelPosition, onlyTotal, totalStackedValues: sortedTotalValues, showValueIndexes, @@ -1379,6 +1385,10 @@ export default function transformProps( const echartOptions: EChartsCoreOption = { useUTC: true, + ...(seriesType === EchartsTimeseriesSeriesType.Bar && + resolvedValueLabelPosition === BarValueLabelPosition.Auto + ? { darkMode: isThemeDark(theme) } + : {}), grid: { ...defaultGrid, ...padding, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts index 2ff65e1e7a5a..8545ac64c4dd 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts @@ -35,6 +35,9 @@ import type { CallbackDataParams, DefaultStatesMixin, ItemStyleOption, + LabelLayoutOption, + LabelLayoutOptionCallback, + LabelLayoutOptionCallbackParams, LineStyleOption, OptionName, SeriesLabelOption, @@ -49,6 +52,7 @@ import type { import type { MarkLine1DDataItemOption } from 'echarts/types/src/component/marker/MarkLineModel'; import { extractForecastSeriesContext } from '../utils/forecast'; import { + BarValueLabelPosition, EchartsTimeseriesSeriesType, ForecastSeriesEnum, LabelPositionEnum, @@ -70,6 +74,107 @@ import { TIMESERIES_CONSTANTS, } from '../constants'; +const AUTO_LABEL_FIT_RATIO = 0.8; +const BAR_LABEL_DISTANCE = 5; +// Neither an inside nor an outside placement gives a stacked segment's value +// label legible, non-overlapping room once the segment's own extent drops +// below roughly the label text's height, since the closest available +// placement then collides with a neighboring segment's label regardless of +// which side it's drawn on. Highcharts and D3 apply the same kind of floor. +// The label font size is theme.fontSizeSM (~12px, see series.ts), so 16px +// covers the glyph height plus a couple of pixels of breathing room. +const MIN_LABEL_SEGMENT_SIZE_PX = 16; +// The labelLayout callback only applies align/verticalAlign/width/height/ +// fontSize from its return value (LABEL_OPTION_TO_STYLE_KEYS in ECharts' +// LabelManager) — there is no hide/ignore field, so a zero font size is the +// supported way to suppress an individual label from this callback. +const HIDDEN_LABEL_LAYOUT: LabelLayoutOption = { fontSize: 0 }; + +type BarLabelPosition = + | 'bottom' + | 'inside' + | 'insideBottom' + | 'insideLeft' + | 'insideRight' + | 'insideTop' + | 'left' + | 'right' + | 'top'; + +type NegativeBarLabelPosition = BarLabelPosition | 'outside'; + +/** Resolve the fixed ECharts label position for a bar value. */ +function getBarLabelPosition( + position: BarValueLabelPosition, + isHorizontal: boolean, + isNegative = false, +): BarLabelPosition { + if (position === BarValueLabelPosition.OutsideEnd) { + if (isHorizontal) return isNegative ? 'left' : 'right'; + return isNegative ? 'bottom' : 'top'; + } + if (position === BarValueLabelPosition.InsideCenter) return 'inside'; + const isEnd = position !== BarValueLabelPosition.InsideBase; + const usePositiveEnd = isEnd !== isNegative; + if (isHorizontal) return usePositiveEnd ? 'insideRight' : 'insideLeft'; + return usePositiveEnd ? 'insideTop' : 'insideBottom'; +} + +/** Place a horizontal bar label just beyond its value end. */ +function getHorizontalOutsideLayout( + params: LabelLayoutOptionCallbackParams, + isNegative: boolean, +): LabelLayoutOption { + return { + x: isNegative + ? params.rect.x - BAR_LABEL_DISTANCE + : params.rect.x + params.rect.width + BAR_LABEL_DISTANCE, + y: params.rect.y + params.rect.height / 2, + align: isNegative ? 'right' : 'left', + verticalAlign: 'middle', + }; +} + +/** Place a vertical bar label just beyond its value end. */ +function getVerticalOutsideLayout( + params: LabelLayoutOptionCallbackParams, + isNegative: boolean, +): LabelLayoutOption { + return { + x: params.rect.x + params.rect.width / 2, + y: isNegative + ? params.rect.y + params.rect.height + BAR_LABEL_DISTANCE + : params.rect.y - BAR_LABEL_DISTANCE, + align: 'center', + verticalAlign: isNegative ? 'top' : 'bottom', + }; +} + +/** Keep fitting labels inside, move oversized labels outside the bar, and + * suppress labels for segments too small to legibly fit one either way. */ +export function getAutoBarLabelLayout( + params: LabelLayoutOptionCallbackParams, + isHorizontal: boolean, + isNegative = false, +): LabelLayoutOption { + const segmentSize = isHorizontal + ? Math.abs(params.rect.width) + : Math.abs(params.rect.height); + if (segmentSize < MIN_LABEL_SEGMENT_SIZE_PX) { + return HIDDEN_LABEL_LAYOUT; + } + const fitsWidth = + params.labelRect.width <= + Math.abs(params.rect.width) * AUTO_LABEL_FIT_RATIO; + const fitsHeight = + params.labelRect.height <= + Math.abs(params.rect.height) * AUTO_LABEL_FIT_RATIO; + if (fitsWidth && fitsHeight) return {}; + return isHorizontal + ? getHorizontalOutsideLayout(params, isNegative) + : getVerticalOutsideLayout(params, isNegative); +} + function parseTimeShiftToMs(timeShift?: string | null): number { if (!timeShift) return 0; @@ -168,35 +273,69 @@ export const getBaselineSeriesForStream = ( }; }; +/** Identify object-form ECharts data items. */ +function isDataItemObject( + dataItem: unknown, +): dataItem is Record { + return ( + typeof dataItem === 'object' && + dataItem !== null && + !Array.isArray(dataItem) + ); +} + +/** Return whether an ECharts bar datum is negative on its value axis. */ +function isNegativeBarDataItem( + dataItem: unknown, + isHorizontal: boolean, +): boolean { + const value = isDataItemObject(dataItem) ? dataItem.value : dataItem; + const axisValue = Array.isArray(value) + ? value[isHorizontal ? 0 : 1] + : undefined; + return typeof axisValue === 'number' && axisValue < 0; +} + +/** Create a fit-aware layout callback bound to one bar series. */ +function createAutoBarLabelLayout( + data: unknown, + isHorizontal: boolean, +): LabelLayoutOptionCallback { + return params => { + const dataItem = + Array.isArray(data) && params.dataIndex !== undefined + ? data[params.dataIndex] + : undefined; + return getAutoBarLabelLayout( + params, + isHorizontal, + isNegativeBarDataItem(dataItem, isHorizontal), + ); + }; +} + +/** Apply the value-end label position to a negative bar datum. */ +function transformNegativeLabel( + dataItem: unknown, + isHorizontal: boolean, + negativePosition: NegativeBarLabelPosition, +): unknown { + if (!isNegativeBarDataItem(dataItem, isHorizontal)) return dataItem; + const value = isDataItemObject(dataItem) ? dataItem.value : dataItem; + const item = isDataItemObject(dataItem) ? dataItem : { value }; + const label = isDataItemObject(item.label) ? item.label : {}; + return { ...item, label: { ...label, position: negativePosition } }; +} + +/** Adjust label positions for negative values in a bar series. */ export function transformNegativeLabelsPosition( series: SeriesOption, isHorizontal: boolean, - labelPosition?: string, + negativePosition: NegativeBarLabelPosition = 'outside', ): TimeseriesDataRecord[] { - /* - * Adjusts label position for negative values in bar series - * @param series - Array of series options - * @param isHorizontal - Whether chart is horizontal - * @returns data with adjusted label positions for negative values - */ - const transformValue = (value: any) => { - const [xValue, yValue] = Array.isArray(value) ? value : [null, null]; - const axisValue = isHorizontal ? xValue : yValue; - - return axisValue < 0 - ? { - value, - label: { - position: - labelPosition && labelPosition !== 'auto' - ? labelPosition - : 'outside', - }, - } - : value; - }; - - return (series.data as TimeseriesDataRecord[]).map(transformValue); + return (series.data as unknown[]).map(dataItem => + transformNegativeLabel(dataItem, isHorizontal, negativePosition), + ) as TimeseriesDataRecord[]; } export function applyColorByPrimaryAxis( @@ -242,6 +381,7 @@ export function transformSeries( stackIdSuffix?: string; yAxisIndex?: number; showValue?: boolean; + valueLabelPosition?: BarValueLabelPosition; onlyTotal?: boolean; legendState?: LegendState; formatter?: ValueFormatter; @@ -278,6 +418,7 @@ export function transformSeries( stackIdSuffix, yAxisIndex = 0, showValue, + valueLabelPosition = BarValueLabelPosition.Auto, onlyTotal, formatter, legendState, @@ -399,29 +540,40 @@ export function transformSeries( symbol = opts.lineSymbol || (isDarkMode ? 'circle' : 'emptyCircle'); } + let transformedData = data; + if (Array.isArray(data) && colorByPrimaryAxis) { + transformedData = applyColorByPrimaryAxis( + series, + colorScale, + sliceId, + opacity, + isHorizontal, + ); + } + if (Array.isArray(transformedData) && plotType === 'bar') { + // An explicit labelPosition (set before valueLabelPosition existed, or + // still relevant to a saved chart) takes precedence for negative values; + // otherwise fall back to the fit-aware valueLabelPosition-derived spot. + const negativeLabelPosition: NegativeBarLabelPosition = + labelPosition && labelPosition !== 'auto' + ? (labelPosition as NegativeBarLabelPosition) + : getBarLabelPosition(valueLabelPosition, isHorizontal, true); + transformedData = transformNegativeLabelsPosition( + { ...series, data: transformedData }, + isHorizontal, + negativeLabelPosition, + ); + } + + const isAutoBarLabel = + plotType === 'bar' && valueLabelPosition === BarValueLabelPosition.Auto; + const isInsideBarLabel = + plotType === 'bar' && + valueLabelPosition !== BarValueLabelPosition.OutsideEnd; + return { ...series, - ...(Array.isArray(data) - ? colorByPrimaryAxis - ? { - data: applyColorByPrimaryAxis( - series, - colorScale, - sliceId, - opacity, - isHorizontal, - ), - } - : seriesType === 'bar' && !stack - ? { - data: transformNegativeLabelsPosition( - series, - isHorizontal, - labelPosition, - ), - } - : null - : null), + ...(Array.isArray(data) ? { data: transformedData } : null), connectNulls, queryIndex, yAxisIndex, @@ -454,15 +606,31 @@ export function transformSeries( showSymbol, symbol, symbolSize: symbolSizeFn ?? markerSize, + ...(isAutoBarLabel + ? { + labelLayout: createAutoBarLabelLayout(transformedData, isHorizontal), + } + : {}), label: { show: !!showValue, - position: (labelPosition === 'auto' || !labelPosition - ? isHorizontal - ? LabelPositionEnum.Right - : LabelPositionEnum.Top - : labelPosition) as LabelPositionEnum, + // An explicit labelPosition (the generic control still used by + // MixedTimeseries' bar series, and by standalone bar charts saved + // before valueLabelPosition existed) wins outright. Otherwise bar + // charts fall back to the fit-aware valueLabelPosition control, and + // every other "Show value" chart type falls back to an + // orientation-aware default. + position: + labelPosition && labelPosition !== 'auto' + ? (labelPosition as LabelPositionEnum) + : plotType === 'bar' + ? getBarLabelPosition(valueLabelPosition, isHorizontal) + : isHorizontal + ? LabelPositionEnum.Right + : LabelPositionEnum.Top, + // ECharts derives contrast from the bar fill for inside positions. + // Auto x/y overflow clears the position, selecting its outside fill. + ...(isInsideBarLabel ? {} : { color: theme?.colorText }), ...(plotType === 'bar' ? { overflow: 'truncate' } : {}), - color: theme?.colorText, textBorderWidth: 0, formatter: (params: any) => { // don't show confidence band value labels, as they're already visible on the tooltip diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts index 96df4f6eed04..9f0dd771e740 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts @@ -53,6 +53,14 @@ export enum EchartsTimeseriesSeriesType { End = 'end', } +export enum BarValueLabelPosition { + Auto = 'auto', + InsideEnd = 'insideEnd', + OutsideEnd = 'outsideEnd', + InsideCenter = 'insideCenter', + InsideBase = 'insideBase', +} + export type EchartsTimeseriesFormData = QueryFormData & { annotationLayers: AnnotationLayer[]; area: boolean; @@ -102,6 +110,7 @@ export type EchartsTimeseriesFormData = QueryFormData & { xAxisLabelRotation: number; xAxisLabelInterval: number | string; showValue: boolean; + valueLabelPosition: BarValueLabelPosition; /** * Where the data label sits relative to its data point, applied when * `showValue` is on. diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx b/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx index f32b750a35d8..21bcf5f8339f 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx +++ b/superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx @@ -34,6 +34,7 @@ import { StackControlOptionsWithoutStream, } from './constants'; import { DEFAULT_FORM_DATA } from './Timeseries/constants'; +import { BarValueLabelPosition } from './Timeseries/types'; import { defaultXAxis } from './defaults'; const { legendMargin, legendOrientation, legendType, showLegend } = @@ -140,6 +141,32 @@ export const showValueControl: ControlSetItem = { }, }; +// Bar-only: fit-aware placement (Auto avoids/suppresses colliding labels on +// stacked segments) plus explicit end/center/base positions. Wired into +// showValueSectionWithoutStream (Bar charts) instead of labelPositionControl +// below, which stays the generic picker for every other "Show value" chart. +export const valueLabelPositionControl: ControlSetItem = { + name: 'value_label_position', + config: { + type: 'SelectControl', + freeForm: false, + clearable: false, + label: t('Value label position'), + choices: [ + [BarValueLabelPosition.Auto, t('Auto')], + [BarValueLabelPosition.InsideEnd, t('Inside End')], + [BarValueLabelPosition.OutsideEnd, t('Outside End')], + [BarValueLabelPosition.InsideCenter, t('Inside Center')], + [BarValueLabelPosition.InsideBase, t('Inside Base')], + ], + default: DEFAULT_FORM_DATA.valueLabelPosition, + renderTrigger: true, + description: t('Choose where to display values relative to the bars'), + visibility: ({ controls }: ControlPanelsContainerProps) => + Boolean(controls?.show_value?.value), + }, +}; + export const labelPositionControl: ControlSetItem = { name: 'label_position', config: { @@ -257,9 +284,11 @@ export const showValueSectionWithoutStack: ControlSetRow[] = [ [onlyTotalControl], ]; +// Bar charts (the only consumer of this section) use the fit-aware +// valueLabelPositionControl instead of the generic labelPositionControl. export const showValueSectionWithoutStream: ControlSetRow[] = [ [showValueControl], - [labelPositionControl], + [valueLabelPositionControl], [stackControlWithoutStream], [onlyTotalControl], [percentageThresholdControl], diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts index 4004babdbcba..2288ff19386c 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts @@ -196,6 +196,61 @@ function formatSeriesLabel( }); } +test('bar value labels retain their legacy outside position', () => { + const chartProps = createEchartsTimeseriesTestChartProps< + EchartsMixedTimeseriesFormData, + EchartsMixedTimeseriesProps + >({ + ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS, + defaultQueriesData: queriesData, + formData: { ...formData, showValueB: true }, + queriesData, + }); + + const transformed = transformProps(chartProps); + const barSeries = (transformed.echartOptions.series as SeriesOption[]).filter( + (series): series is BarSeriesOption => series.type === 'bar', + ); + + expect(barSeries).not.toHaveLength(0); + barSeries.forEach(series => { + expect(series.label).toMatchObject({ show: true, position: 'top' }); + expect(series.labelLayout).toBeUndefined(); + }); +}); + +test('negative bar values retain their legacy outside position', () => { + const negativeRows = [ + { boy: -1, girl: -2, ds: 599616000000 }, + { boy: -3, girl: -4, ds: 599916000000 }, + ]; + const negativeQueriesData = [ + createTestQueryData(negativeRows, { label_map: defaultLabelMap }), + createTestQueryData(negativeRows, { label_map: defaultLabelMap }), + ]; + const chartProps = createEchartsTimeseriesTestChartProps< + EchartsMixedTimeseriesFormData, + EchartsMixedTimeseriesProps + >({ + ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS, + defaultQueriesData: negativeQueriesData, + formData: { ...formData, showValueB: true }, + queriesData: negativeQueriesData, + }); + + const transformed = transformProps(chartProps); + const barSeries = (transformed.echartOptions.series as SeriesOption[]).filter( + (series): series is BarSeriesOption => series.type === 'bar', + ); + + expect(barSeries).not.toHaveLength(0); + barSeries.forEach(series => { + expect(series.data?.[0]).toMatchObject({ + label: { position: 'bottom' }, + }); + }); +}); + test('should transform chart props for viz with showQueryIdentifiers=false', () => { const chartProps = createEchartsTimeseriesTestChartProps< EchartsMixedTimeseriesFormData, diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/controlPanel.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/controlPanel.test.ts index a148ac12dcb3..137b960e8833 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/controlPanel.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/controlPanel.test.ts @@ -23,14 +23,10 @@ import { StackControlOptionsWithoutStream, StackControlsValue, } from '../../../src/constants'; -import { OrientationType } from '../../../src/Timeseries/types'; - -// Narrow shape of the control under test: enough to exercise `visibility` -// without reaching for `any`. -type VisibilityControl = { - name: string; - config: { visibility: (props: ControlPanelsContainerProps) => boolean }; -}; +import { + BarValueLabelPosition, + OrientationType, +} from '../../../src/Timeseries/types'; const config = controlPanel; @@ -139,6 +135,41 @@ test('should include stack control in the panel', () => { expect(stackControl).toBeDefined(); }); +test('should expose Auto and manual value label positions for Bar charts', () => { + const valueLabelPositionControl = getControl( + 'value_label_position', + ) as unknown as { + config: { + choices: [BarValueLabelPosition, string][]; + default: BarValueLabelPosition; + visibility: (props: ControlPanelsContainerProps) => boolean; + }; + }; + + expect(valueLabelPositionControl.config.default).toBe( + BarValueLabelPosition.OutsideEnd, + ); + expect( + valueLabelPositionControl.config.choices.map(([value]) => value), + ).toEqual([ + BarValueLabelPosition.Auto, + BarValueLabelPosition.InsideEnd, + BarValueLabelPosition.OutsideEnd, + BarValueLabelPosition.InsideCenter, + BarValueLabelPosition.InsideBase, + ]); + expect( + valueLabelPositionControl.config.visibility({ + controls: { show_value: { value: true } }, + } as unknown as ControlPanelsContainerProps), + ).toBe(true); + expect( + valueLabelPositionControl.config.visibility({ + controls: { show_value: { value: false } }, + } as unknown as ControlPanelsContainerProps), + ).toBe(false); +}); + test('should use StackControlOptionsWithoutStream for stack control', () => { const stackControl: any = getControl('stack'); expect(stackControl).toBeDefined(); @@ -299,42 +330,3 @@ test('x_axis_time_format should be hidden for numeric columns', () => { false, ); }); - -test('should have visibility function for label_position', () => { - const labelPositionCtrl = getControl( - 'label_position', - ) as unknown as VisibilityControl; - expect(labelPositionCtrl).toBeDefined(); - expect(labelPositionCtrl.config.visibility).toBeDefined(); - expect(typeof labelPositionCtrl.config.visibility).toBe('function'); - - expect( - labelPositionCtrl.config.visibility({ - controls: { - show_value: { value: true }, - show_valueB: { value: false }, - }, - } as unknown as ControlPanelsContainerProps), - ).toBe(true); - - // Visibility follows `show_value` alone. No Timeseries panel defines - // `show_valueB` — Mixed declares its own suffixed controls — so it must not - // reveal the control on its own. - expect( - labelPositionCtrl.config.visibility({ - controls: { - show_value: { value: false }, - show_valueB: { value: true }, - }, - } as unknown as ControlPanelsContainerProps), - ).toBe(false); - - expect( - labelPositionCtrl.config.visibility({ - controls: { - show_value: { value: false }, - show_valueB: { value: false }, - }, - } as unknown as ControlPanelsContainerProps), - ).toBe(false); -}); diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts index cd69678b226f..6a95b70d3c59 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Bar/transformProps.test.ts @@ -29,6 +29,7 @@ import type { GridComponentOption, LegendComponentOption, } from 'echarts/components'; +import type { BarSeriesOption } from 'echarts/charts'; import { EchartsTimeseriesChartProps, LegendOrientation, @@ -37,6 +38,7 @@ import { import transformProps from '../../../src/Timeseries/transformProps'; import { DEFAULT_FORM_DATA } from '../../../src/Timeseries/constants'; import { + BarValueLabelPosition, EchartsTimeseriesFormData, OrientationType, EchartsTimeseriesSeriesType, @@ -74,6 +76,106 @@ function createTestQueryData( }; } +test('manual Bar value label position flows through transformProps', () => { + const chartProps = createEchartsTimeseriesTestChartProps< + EchartsTimeseriesFormData, + EchartsTimeseriesChartProps + >({ + defaultFormData: DEFAULT_FORM_DATA, + defaultVizType: 'echarts_timeseries_bar', + formData: { + seriesType: EchartsTimeseriesSeriesType.Bar, + valueLabelPosition: BarValueLabelPosition.OutsideEnd, + metrics: ['Sales'], + xAxis: '__timestamp', + showValue: true, + }, + queriesData: [ + createTestQueryData([{ Sales: 100, __timestamp: 1609459200000 }], { + colnames: ['Sales', '__timestamp'], + coltypes: [GenericDataType.Numeric, GenericDataType.Temporal], + }), + ], + }); + + const { echartOptions } = transformProps(chartProps); + const [series] = echartOptions.series as BarSeriesOption[]; + + expect(series.label).toMatchObject({ position: 'top' }); + expect(series.labelLayout).toBeUndefined(); + expect(echartOptions.darkMode).toBeUndefined(); +}); + +test('Auto Bar labels enable theme-aware ECharts contrast', () => { + const chartProps = createEchartsTimeseriesTestChartProps< + EchartsTimeseriesFormData, + EchartsTimeseriesChartProps + >({ + defaultFormData: DEFAULT_FORM_DATA, + defaultVizType: 'echarts_timeseries_bar', + formData: { + seriesType: EchartsTimeseriesSeriesType.Bar, + valueLabelPosition: BarValueLabelPosition.Auto, + metrics: ['Sales'], + xAxis: '__timestamp', + showValue: true, + }, + queriesData: [ + createTestQueryData([{ Sales: 100, __timestamp: 1609459200000 }], { + colnames: ['Sales', '__timestamp'], + coltypes: [GenericDataType.Numeric, GenericDataType.Temporal], + }), + ], + }); + + const { echartOptions } = transformProps(chartProps); + const [series] = echartOptions.series as BarSeriesOption[]; + + expect(typeof series.labelLayout).toBe('function'); + expect(echartOptions.darkMode).toBe(false); +}); + +test('legacy Bar labels without a saved position keep their pre-existing Outside End placement', () => { + const legacyFormData: Partial = { + ...DEFAULT_FORM_DATA, + }; + delete legacyFormData.valueLabelPosition; + const chartProps = createEchartsTimeseriesTestChartProps< + EchartsTimeseriesFormData, + EchartsTimeseriesChartProps + >({ + defaultFormData: legacyFormData as EchartsTimeseriesFormData, + defaultVizType: 'echarts_timeseries_bar', + formData: { + seriesType: EchartsTimeseriesSeriesType.Bar, + metrics: ['Sales'], + xAxis: '__timestamp', + showValue: true, + }, + queriesData: [ + createTestQueryData([{ Sales: 100, __timestamp: 1609459200000 }], { + colnames: ['Sales', '__timestamp'], + coltypes: [GenericDataType.Numeric, GenericDataType.Temporal], + }), + ], + }); + + expect(chartProps.formData).not.toHaveProperty('valueLabelPosition'); + const { echartOptions } = transformProps(chartProps); + const [series] = echartOptions.series as BarSeriesOption[]; + + expect(series.label).toMatchObject({ position: 'top' }); + expect(series.labelLayout).toBeUndefined(); + + Reflect.set(chartProps.formData, 'valueLabelPosition', undefined); + const undefinedPositionOptions = transformProps(chartProps).echartOptions; + const [undefinedPositionSeries] = + undefinedPositionOptions.series as BarSeriesOption[]; + + expect(undefinedPositionSeries.label).toMatchObject({ position: 'top' }); + expect(undefinedPositionSeries.labelLayout).toBeUndefined(); +}); + describe('Bar Chart X-axis Time Formatting', () => { const baseFormData: SqlaFormData = { ...DEFAULT_FORM_DATA, diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts index 68d3a76c84ce..ba49f4b38450 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts @@ -25,9 +25,13 @@ import { } from '@superset-ui/core'; import { GenericDataType } from '@apache-superset/core/common'; import { supersetTheme } from '@apache-superset/core/theme'; -import type { SeriesOption } from 'echarts'; -import type { ScatterSeriesOption } from 'echarts/charts'; -import { EchartsTimeseriesSeriesType } from '../../src'; +import { init, type SeriesOption } from 'echarts'; +import type { + BarSeriesOption, + LineSeriesOption, + ScatterSeriesOption, +} from 'echarts/charts'; +import { BarValueLabelPosition, EchartsTimeseriesSeriesType } from '../../src'; import { StackControlsValue, TIMESERIES_CONSTANTS } from '../../src/constants'; import { LegendOrientation, @@ -36,6 +40,7 @@ import { import { transformSeries, transformNegativeLabelsPosition, + getAutoBarLabelLayout, getPadding, } from '../../src/Timeseries/transformers'; import transformProps from '../../src/Timeseries/transformProps'; @@ -228,6 +233,416 @@ describe('transformSeries', () => { }); }); +test('Auto bar labels move outside narrow stacked segments', () => { + const result = transformSeries( + { name: 'test-series', type: 'bar', data: [[2026, 1]] }, + mockColorScale, + 'test-key', + { + seriesType: EchartsTimeseriesSeriesType.Bar, + stack: StackControlsValue.Stack, + showValue: true, + }, + ) as BarSeriesOption; + const { labelLayout } = result; + + expect(result.label).toMatchObject({ + show: true, + position: 'insideTop', + }); + expect((result.label as { color?: string }).color).toBeUndefined(); + expect(typeof labelLayout).toBe('function'); + if (typeof labelLayout !== 'function') return; + + expect( + labelLayout({ + dataIndex: 0, + seriesIndex: 0, + text: '1,000', + align: 'center', + verticalAlign: 'middle', + rect: { x: 10, y: 20, width: 12, height: 20 }, + labelRect: { x: 1, y: 22, width: 30, height: 14 }, + }), + ).toEqual({ + x: 16, + y: 15, + align: 'center', + verticalAlign: 'bottom', + }); +}); + +test('Auto labels stay inside when both dimensions fit within 80% of the bar', () => { + const result = transformSeries( + { name: 'test-series', type: 'bar', data: [[2026, 1]] }, + mockColorScale, + 'test-key', + { seriesType: EchartsTimeseriesSeriesType.Bar }, + ) as BarSeriesOption; + const { labelLayout } = result; + + expect(typeof labelLayout).toBe('function'); + if (typeof labelLayout !== 'function') return; + + expect( + labelLayout({ + dataIndex: 0, + seriesIndex: 0, + text: '1,000', + align: 'center', + verticalAlign: 'top', + rect: { x: 10, y: 20, width: 50, height: 40 }, + labelRect: { x: 19, y: 25, width: 32, height: 14 }, + }), + ).toEqual({}); +}); + +test('Auto moves wide labels outside tall narrow vertical bars', () => { + const result = transformSeries( + { name: 'test-series', type: 'bar', data: [[2026, 100]] }, + mockColorScale, + 'test-key', + { seriesType: EchartsTimeseriesSeriesType.Bar }, + ) as BarSeriesOption; + const { labelLayout } = result; + + expect(typeof labelLayout).toBe('function'); + if (typeof labelLayout !== 'function') return; + + expect( + labelLayout({ + dataIndex: 0, + seriesIndex: 0, + text: '1,000', + align: 'center', + verticalAlign: 'top', + rect: { x: 10, y: 20, width: 12, height: 200 }, + labelRect: { x: 1, y: 25, width: 30, height: 14 }, + }), + ).toEqual({ + x: 16, + y: 15, + align: 'center', + verticalAlign: 'bottom', + }); +}); + +test('Auto overflow uses ECharts outside-label text color', () => { + const darkBarColorScale = jest.fn(() => '#111111'); + const series = transformSeries( + { name: 'test-series', type: 'bar', data: [[0, 123456789012]] }, + darkBarColorScale as unknown as CategoricalColorScale, + 'test-key', + { + formatter: getNumberFormatter('d'), + seriesType: EchartsTimeseriesSeriesType.Bar, + showValue: true, + }, + ) as BarSeriesOption; + const chart = init(null, null, { + renderer: 'svg', + ssr: true, + width: 300, + height: 220, + }); + + chart.setOption({ + animation: false, + darkMode: false, + xAxis: { type: 'category', data: ['A'], show: false }, + // A tall bar (well above the segment-legibility floor) whose 12-digit + // label is too wide to fit inside, so ECharts still moves it outside. + yAxis: { type: 'value', max: 250_000_000_000, show: false }, + series: [series], + }); + + expect(chart.renderToSVGString()).toMatch( + /fill="#333"[^>]*>123456789012<\/text>/, + ); + chart.dispose(); +}); + +test('Auto bar labels use horizontal bar length and move to the value end', () => { + const result = transformSeries( + { name: 'test-series', type: 'bar', data: [[1, 2026]] }, + mockColorScale, + 'test-key', + { seriesType: EchartsTimeseriesSeriesType.Bar, isHorizontal: true }, + ) as BarSeriesOption; + const { labelLayout } = result; + + expect(typeof labelLayout).toBe('function'); + if (typeof labelLayout !== 'function') return; + + expect( + labelLayout({ + dataIndex: 0, + seriesIndex: 0, + text: '1,000', + align: 'right', + verticalAlign: 'middle', + rect: { x: 10, y: 20, width: 20, height: 12 }, + labelRect: { x: 0, y: 19, width: 30, height: 14 }, + }), + ).toEqual({ + x: 35, + y: 26, + align: 'left', + verticalAlign: 'middle', + }); +}); + +test.each([ + [BarValueLabelPosition.InsideEnd, 'insideTop'], + [BarValueLabelPosition.OutsideEnd, 'top'], + [BarValueLabelPosition.InsideCenter, 'inside'], + [BarValueLabelPosition.InsideBase, 'insideBottom'], +] as const)( + 'manual %s bar labels use fixed position %s', + (position, expected) => { + const result = transformSeries( + { name: 'test-series', type: 'bar', data: [[2026, 1]] }, + mockColorScale, + 'test-key', + { + seriesType: EchartsTimeseriesSeriesType.Bar, + valueLabelPosition: position, + theme: supersetTheme, + }, + ) as BarSeriesOption; + + expect(result.labelLayout).toBeUndefined(); + expect(result.label).toMatchObject({ position: expected }); + if (position === BarValueLabelPosition.OutsideEnd) { + expect(result.label).toMatchObject({ color: supersetTheme.colorText }); + } else { + expect(result.label).not.toHaveProperty('color'); + } + }, +); + +test('manual Outside End positions negative stacked segments below the bar', () => { + const result = transformSeries( + { name: 'test-series', type: 'bar', data: [[2026, -1]] }, + mockColorScale, + 'test-key', + { + seriesType: EchartsTimeseriesSeriesType.Bar, + stack: StackControlsValue.Stack, + valueLabelPosition: BarValueLabelPosition.OutsideEnd, + }, + ) as BarSeriesOption; + + expect(result.data).toEqual([ + { + value: [2026, -1], + label: { position: 'bottom' }, + }, + ]); + expect(result.labelLayout).toBeUndefined(); +}); + +test('Auto positions negative stacked segments at their inside end', () => { + const result = transformSeries( + { name: 'test-series', type: 'bar', data: [[2026, -1]] }, + mockColorScale, + 'test-key', + { + seriesType: EchartsTimeseriesSeriesType.Bar, + stack: StackControlsValue.Stack, + }, + ) as BarSeriesOption; + + expect(result.data).toEqual([ + { + value: [2026, -1], + label: { position: 'insideBottom' }, + }, + ]); + expect(typeof result.labelLayout).toBe('function'); + if (typeof result.labelLayout !== 'function') return; + expect( + result.labelLayout({ + dataIndex: 0, + seriesIndex: 0, + text: '-1,000', + align: 'center', + verticalAlign: 'bottom', + rect: { x: 10, y: 20, width: 12, height: 30 }, + labelRect: { x: 1, y: 35, width: 30, height: 14 }, + }), + ).toEqual({ + x: 16, + y: 55, + align: 'center', + verticalAlign: 'top', + }); +}); + +test('Auto moves horizontal negative labels beyond their value end', () => { + const result = transformSeries( + { name: 'test-series', type: 'bar', data: [[-1, 2026]] }, + mockColorScale, + 'test-key', + { seriesType: EchartsTimeseriesSeriesType.Bar, isHorizontal: true }, + ) as BarSeriesOption; + + expect(result.data).toEqual([ + { + value: [-1, 2026], + label: { position: 'insideLeft' }, + }, + ]); + expect(typeof result.labelLayout).toBe('function'); + if (typeof result.labelLayout !== 'function') return; + expect( + result.labelLayout({ + dataIndex: 0, + seriesIndex: 0, + text: '-1,000', + align: 'left', + verticalAlign: 'middle', + rect: { x: 10, y: 20, width: 20, height: 12 }, + labelRect: { x: 10, y: 19, width: 30, height: 14 }, + }), + ).toEqual({ + x: 5, + y: 26, + align: 'right', + verticalAlign: 'middle', + }); +}); + +test('Auto label layout does not change non-Bar series', () => { + const result = transformSeries( + { name: 'test-series', type: 'line', data: [[2026, 1]] }, + mockColorScale, + 'test-key', + { + seriesType: EchartsTimeseriesSeriesType.Line, + theme: supersetTheme, + }, + ) as LineSeriesOption; + + expect(result).not.toHaveProperty('labelLayout'); + expect(result.label).toMatchObject({ + position: 'top', + color: supersetTheme.colorText, + }); +}); + +test('Auto suppresses the label for a vertical segment below the legibility floor', () => { + // A 10px-tall stacked segment can't legibly fit its 14px-tall label inside + // or outside without colliding with a neighboring segment's label. + expect( + getAutoBarLabelLayout( + { + dataIndex: 0, + seriesIndex: 0, + text: '0.14', + align: 'center', + verticalAlign: 'middle', + rect: { x: 10, y: 20, width: 40, height: 10 }, + labelRect: { x: 12, y: 22, width: 20, height: 14 }, + }, + false, + ), + ).toEqual({ fontSize: 0 }); +}); + +test('Auto keeps placing labels normally for a vertical segment at the legibility floor', () => { + expect( + getAutoBarLabelLayout( + { + dataIndex: 0, + seriesIndex: 0, + text: '0.14', + align: 'center', + verticalAlign: 'middle', + rect: { x: 10, y: 20, width: 40, height: 16 }, + labelRect: { x: 12, y: 22, width: 20, height: 14 }, + }, + false, + ), + ).not.toEqual({ fontSize: 0 }); +}); + +test('Auto suppresses the label for a horizontal segment below the legibility floor', () => { + // Horizontal bars stack along the x axis, so the value-axis dimension that + // matters is rect.width rather than rect.height. + expect( + getAutoBarLabelLayout( + { + dataIndex: 0, + seriesIndex: 0, + text: '0.14', + align: 'left', + verticalAlign: 'middle', + rect: { x: 10, y: 20, width: 10, height: 40 }, + labelRect: { x: 12, y: 22, width: 20, height: 14 }, + }, + true, + ), + ).toEqual({ fontSize: 0 }); +}); + +test('Auto suppresses labels for tiny adjacent stacked segments end to end', () => { + const result = transformSeries( + { name: 'test-series', type: 'bar', data: [[2026, 0.14]] }, + mockColorScale, + 'test-key', + { + seriesType: EchartsTimeseriesSeriesType.Bar, + stack: StackControlsValue.Stack, + showValue: true, + }, + ) as BarSeriesOption; + const { labelLayout } = result; + + expect(typeof labelLayout).toBe('function'); + if (typeof labelLayout !== 'function') return; + + expect( + labelLayout({ + dataIndex: 0, + seriesIndex: 0, + text: '0.14', + align: 'center', + verticalAlign: 'middle', + rect: { x: 10, y: 20, width: 40, height: 8 }, + labelRect: { x: 12, y: 22, width: 20, height: 14 }, + }), + ).toEqual({ fontSize: 0 }); +}); + +test.each([ + [BarValueLabelPosition.InsideEnd, 'insideTop'], + [BarValueLabelPosition.OutsideEnd, 'top'], + [BarValueLabelPosition.InsideCenter, 'inside'], + [BarValueLabelPosition.InsideBase, 'insideBottom'], +] as const)( + 'manual %s label placement is unaffected by tiny segments (no labelLayout applied)', + (position, expected) => { + const result = transformSeries( + { name: 'test-series', type: 'bar', data: [[2026, 0.14]] }, + mockColorScale, + 'test-key', + { + seriesType: EchartsTimeseriesSeriesType.Bar, + stack: StackControlsValue.Stack, + valueLabelPosition: position, + showValue: true, + theme: supersetTheme, + }, + ) as BarSeriesOption; + + // Manual positions don't use the fit-aware labelLayout callback at all, + // so a tiny segment can't trigger the Auto-only suppression behavior. + expect(result.labelLayout).toBeUndefined(); + expect(result.label).toMatchObject({ position: expected }); + }, +); + describe('transformNegativeLabelsPosition', () => { test('label position bottom of negative value no Horizontal', () => { const isHorizontal = false; From ba50bdff237e28dfd18f0792ffaf60d0c8ca9cae Mon Sep 17 00:00:00 2001 From: Joe Li Date: Tue, 1 Sep 2026 13:23:17 -0700 Subject: [PATCH 2/4] fix(echarts): prevent x-axis time labels from overlapping (#43669) Co-authored-by: Claude Sonnet 5 --- .../src/MixedTimeseries/transformProps.ts | 50 ++---- .../src/Timeseries/transformProps.ts | 56 +++---- .../plugin-chart-echarts/src/constants.ts | 6 + .../src/utils/formatters.ts | 150 ++++++++++++++++++ .../MixedTimeseries/transformProps.test.ts | 52 ++++++ .../test/Timeseries/transformProps.test.ts | 38 +++++ .../test/Timeseries/transformers.test.ts | 49 ++++++ 7 files changed, 333 insertions(+), 68 deletions(-) diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts index 64cdc7b287af..1855b19a202c 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts @@ -103,7 +103,9 @@ import { import { TIMEGRAIN_TO_TIMESTAMP, TIMESERIES_CONSTANTS } from '../constants'; import { getDefaultTooltip } from '../utils/tooltip'; import { + createSpacedXAxisFormatter, getTooltipTimeFormatter, + getXAxisDomain, getXAxisFormatter, getYAxisFormatter, } from '../utils/formatters'; @@ -664,44 +666,26 @@ export default function transformProps( ? getXAxisFormatter(xAxisTimeFormat, resolvedTimeGrain) : String; + // hideOverlap must stay off so the forced boundary label from showMaxLabel + // is never suppressed (#39899). The formatter itself dedupes consecutive + // identical labels and thins out labels that would otherwise visually + // collide, since hideOverlap can no longer do that for us. const showMaxLabel = xAxisType === AxisType.Time && xAxisLabelRotation === 0 && !!resolvedTimeGrain; const deduplicatedFormatter = showMaxLabel - ? (() => { - let lastLabel: string | undefined; - let lastValue: number | undefined; - const wrapper = (value: number | string) => { - // ECharts formats the labels in repeated ascending passes. Reset the - // dedup state when the sequence restarts so a forced boundary label - // (e.g. the min date) isn't blanked by the previous pass's last label - // when both format identically (e.g. a May-to-May range). - if ( - typeof value === 'number' && - lastValue !== undefined && - value <= lastValue - ) { - lastLabel = undefined; - } - if (typeof value === 'number') { - lastValue = value; - } - const label = - typeof xAxisFormatter === 'function' - ? (xAxisFormatter as Function)(value) - : String(value); - if (label === lastLabel) { - return ''; - } - lastLabel = label; - return label; - }; - if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) { - (wrapper as any).id = (xAxisFormatter as any).id; - } - return wrapper; - })() + ? createSpacedXAxisFormatter( + xAxisFormatter, + ...getXAxisDomain( + [ + rebasedDataA as Record[], + rebasedDataB as Record[], + ], + xAxisLabel, + ), + Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0), + ) : xAxisFormatter; const yAxisTitleMarginPx = convertInteger(yAxisTitleMargin); diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts index 4882dfff5e16..169a75631286 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts @@ -126,8 +126,11 @@ import { } from '../constants'; import { getDefaultTooltip } from '../utils/tooltip'; import { + createDedupXAxisFormatter, + createSpacedXAxisFormatter, getPercentFormatter, getTooltipTimeFormatter, + getXAxisDomain, getXAxisFormatter, getYAxisFormatter, } from '../utils/formatters'; @@ -1213,46 +1216,29 @@ export default function transformProps( // When showMaxLabel is true, ECharts may render a label at the axis // boundary that formats identically to the last data-point tick (e.g. - // "2005" appears twice with Year grain). Wrap the formatter to suppress - // consecutive duplicate labels. + // "2005" appears twice with Year grain), and hideOverlap must stay off so + // that forced boundary label is never suppressed (#39899). Wrap the + // formatter to suppress consecutive duplicate labels and to thin out + // labels that would otherwise visually collide, since hideOverlap can no + // longer do that for us. The spacing estimate assumes the axis runs along + // the bottom of the chart (pixel width, character width); a horizontal + // orientation chart puts the time axis on the side instead, so it falls + // back to dedup-only there. const showMaxLabel = xAxisType === AxisType.Time && xAxisLabelRotation === 0 && !!resolvedTimeGrain; const deduplicatedFormatter = showMaxLabel - ? (() => { - let lastLabel: string | undefined; - let lastValue: number | undefined; - const wrapper = (value: number | string) => { - // ECharts formats the labels in repeated ascending passes. Reset the - // dedup state when the sequence restarts so a forced boundary label - // (e.g. the min date) isn't blanked by the previous pass's last label - // when both format identically (e.g. a May-to-May range). - if ( - typeof value === 'number' && - lastValue !== undefined && - value <= lastValue - ) { - lastLabel = undefined; - } - if (typeof value === 'number') { - lastValue = value; - } - const label = - typeof xAxisFormatter === 'function' - ? (xAxisFormatter as Function)(value) - : String(value); - if (label === lastLabel) { - return ''; - } - lastLabel = label; - return label; - }; - if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) { - (wrapper as any).id = (xAxisFormatter as any).id; - } - return wrapper; - })() + ? isHorizontal + ? createDedupXAxisFormatter(xAxisFormatter) + : createSpacedXAxisFormatter( + xAxisFormatter, + ...getXAxisDomain( + [rebasedData as Record[]], + xAxisLabel, + ), + Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0), + ) : xAxisFormatter; const temporalTickValues = resolveTemporalTickValues( diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts b/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts index 2b94967ae003..7454da43b925 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/constants.ts @@ -52,6 +52,12 @@ export const TIMESERIES_CONSTANTS = { microChartHeight: 60, // One y-axis tick per this many pixels of chart height yAxisPixelsPerTick: 80, + // Rough average glyph width (px) used to estimate whether adjacent x-axis + // time labels would visually collide, since the real rendered width isn't + // known until ECharts lays out the axis. + xAxisLabelCharWidthPx: 7, + // Minimum gap (px) to keep between adjacent x-axis time labels. + xAxisLabelMinGapPx: 8, }; export enum OpacityEnum { diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts b/superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts index 485bc2407acd..5fe4dd195604 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts @@ -24,6 +24,7 @@ import { getTimeFormatter, isSavedMetric, NumberFormats, + NumberFormatter, QueryFormMetric, SMART_DATE_DETAILED_ID, SMART_DATE_ID, @@ -32,6 +33,7 @@ import { TimeGranularity, ValueFormatter, } from '@superset-ui/core'; +import { TIMESERIES_CONSTANTS } from '../constants'; export const getSmartDateDetailedFormatter = () => getTimeFormatter(SMART_DATE_DETAILED_ID); @@ -213,3 +215,151 @@ export function getXAxisFormatter( } return String; } + +type XAxisFormatterFn = + | TimeFormatter + | NumberFormatter + | StringConstructor + | ((value: number | string) => string); + +/** + * Wraps an x-axis time formatter so that consecutive ticks that format to + * identical text are blanked (e.g. the boundary label forced by + * showMaxLabel duplicating the last real tick). + * + * Use this instead of createSpacedXAxisFormatter when the axis geometry + * doesn't match the spacing model's horizontal-plot assumptions, e.g. a + * horizontal orientation chart, where the time axis runs vertically along + * the side of the chart rather than along the bottom. + */ +export function createDedupXAxisFormatter( + xAxisFormatter: XAxisFormatterFn | undefined, +): (value: number | string) => string { + let lastLabel: string | undefined; + let lastValue: number | undefined; + const wrapper = (value: number | string) => { + // ECharts formats the labels in repeated ascending passes. Reset the + // dedup state when the sequence restarts so a forced boundary label + // (e.g. the min date) isn't blanked by the previous pass's last label + // when both format identically (e.g. a May-to-May range). + if ( + typeof value === 'number' && + lastValue !== undefined && + value <= lastValue + ) { + lastLabel = undefined; + } + if (typeof value === 'number') { + lastValue = value; + } + const label = + typeof xAxisFormatter === 'function' + ? (xAxisFormatter as Function)(value) + : String(value); + if (label === lastLabel) { + return ''; + } + lastLabel = label; + return label; + }; + if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) { + (wrapper as { id?: unknown }).id = (xAxisFormatter as { id?: unknown }).id; + } + return wrapper; +} + +/** + * Wraps an x-axis time formatter so that: + * - consecutive ticks that format to identical text are blanked (e.g. the + * boundary label forced by showMaxLabel duplicating the last real tick). + * - ticks that would render close enough to visually collide with the + * previously shown label are blanked, since disabling ECharts' + * `hideOverlap` (required to keep the forced boundary label visible, see + * #39899) also disables its native overlap suppression for every other + * label on the axis. + * + * The forced axis boundary labels (domainMin/domainMax) are never blanked by + * the spacing check so they stay visible regardless of density. + */ +export function createSpacedXAxisFormatter( + xAxisFormatter: XAxisFormatterFn | undefined, + domainMin: number | undefined, + domainMax: number | undefined, + plotWidthPx: number, +): (value: number | string) => string { + const pixelsPerMs = + domainMin !== undefined && domainMax !== undefined && domainMax > domainMin + ? plotWidthPx / (domainMax - domainMin) + : undefined; + let lastLabel: string | undefined; + let lastValue: number | undefined; + let lastShownValue: number | undefined; + const wrapper = (value: number | string) => { + // ECharts formats the labels in repeated ascending passes. Reset the + // dedup/spacing state when the sequence restarts so a forced boundary + // label (e.g. the min date) isn't blanked by the previous pass's state + // when both format identically (e.g. a May-to-May range). + if ( + typeof value === 'number' && + lastValue !== undefined && + value <= lastValue + ) { + lastLabel = undefined; + lastShownValue = undefined; + } + if (typeof value === 'number') { + lastValue = value; + } + const label = + typeof xAxisFormatter === 'function' + ? (xAxisFormatter as Function)(value) + : String(value); + if (label === lastLabel) { + return ''; + } + const isBoundary = + typeof value === 'number' && (value === domainMin || value === domainMax); + if ( + !isBoundary && + typeof value === 'number' && + pixelsPerMs !== undefined && + lastShownValue !== undefined && + (value - lastShownValue) * pixelsPerMs < + label.length * TIMESERIES_CONSTANTS.xAxisLabelCharWidthPx + + TIMESERIES_CONSTANTS.xAxisLabelMinGapPx + ) { + return ''; + } + lastLabel = label; + if (typeof value === 'number') { + lastShownValue = value; + } + return label; + }; + if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) { + (wrapper as { id?: unknown }).id = (xAxisFormatter as { id?: unknown }).id; + } + return wrapper; +} + +/** + * Computes the [min, max] of a temporal x-axis column across one or more + * data record arrays, for use with createSpacedXAxisFormatter. + */ +export function getXAxisDomain( + dataRecordArrays: Record[][], + xAxisCol: string, +): [number | undefined, number | undefined] { + let domainMin: number | undefined; + let domainMax: number | undefined; + dataRecordArrays.forEach(records => { + records.forEach(record => { + const value = record[xAxisCol]; + if (typeof value === 'number') { + if (domainMin === undefined || value < domainMin) domainMin = value; + if (domainMax === undefined || value > domainMax) domainMax = value; + } + }); + }); + return [domainMin, domainMax]; +} diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts index 2288ff19386c..be425d16d5f6 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts @@ -1379,6 +1379,58 @@ test('#39899 - x-axis dates do not overlap and last label stays visible at 0° r expect(axisLabel.hideOverlap).toBe(false); }); +test('#39899 - closely spaced x-axis time labels do not visually overlap (mixed)', () => { + const startTime = Date.UTC(2026, 0, 1); + const data = Array.from({ length: 20 }, (_, i) => ({ + __timestamp: startTime + i * 60 * 1000, + sum__num: i, + })); + const queryData = createTestQueryData(data, { + colnames: ['__timestamp', 'sum__num'], + coltypes: [GenericDataType.Temporal, GenericDataType.Numeric], + label_map: { __timestamp: ['__timestamp'], sum__num: ['sum__num'] }, + }); + + const chartProps = createEchartsTimeseriesTestChartProps< + EchartsMixedTimeseriesFormData, + EchartsMixedTimeseriesProps + >({ + ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS, + width: 300, + height: 400, + defaultQueriesData: [queryData, queryData], + formData: { + ...formData, + x_axis: '__timestamp', + xAxisTimeFormat: '%Y-%m-%d %H:%M:%S', + metrics: ['sum__num'], + metricsB: ['sum__num'], + groupby: [], + groupbyB: [], + xAxisLabelRotation: 0, + timeGrainSqla: TimeGranularity.MINUTE, + }, + queriesData: [queryData, queryData], + }); + + const { echartOptions } = transformProps(chartProps); + const { axisLabel } = echartOptions.xAxis as Record; + const labels = data.map(({ __timestamp }) => + axisLabel.formatter(__timestamp), + ); + + // hideOverlap must stay off so ECharts' own collision detection can never + // suppress the forced boundary label (#39899 must not regress). + expect(axisLabel.hideOverlap).toBe(false); + // The formatter itself must thin out labels that are too close together to + // render legibly in the available width. + expect(labels.filter(label => label === '').length).toBeGreaterThan(0); + // The first and last labels are the forced axis boundaries and must always + // stay visible. + expect(labels[0]).not.toBe(''); + expect(labels[labels.length - 1]).not.toBe(''); +}); + test('regression #37921: multi-metric Query A with groupby does not duplicate first metric in series names', () => { // Regression test for https://github.com/apache/superset/issues/37921 // ("Residual" follow-up to #37055). diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts index a24ff415857d..45d782ff4586 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts @@ -3132,6 +3132,44 @@ test('applies gridlines to the value axis after a horizontal orientation swaps i expect((echartOptions.xAxis as any).splitLine.show).toBe(false); }); +test('#39899 - horizontal orientation does not over-thin the time axis labels', () => { + // The spacing formatter estimates label collisions using horizontal plot + // geometry (width, 7px/char). A horizontal chart swaps the time axis onto + // the side of the chart, where that geometry no longer applies, so the + // spacing formatter must not be used there. + const monthData = Array.from({ length: 24 }, (_, i) => ({ + __timestamp: Date.UTC(2020, i, 1), + sales: i, + })); + const { echartOptions } = transformProps( + createTestChartProps({ + formData: { + granularity_sqla: 'ds', + timeGrainSqla: TimeGranularity.MONTH, + xAxisTimeFormat: '%Y-%m', + seriesType: EchartsTimeseriesSeriesType.Bar, + orientation: OrientationType.Horizontal, + }, + width: 800, + queriesData: [ + createTestQueryData(monthData, { + colnames: ['__timestamp', 'sales'], + coltypes: [GenericDataType.Temporal, GenericDataType.Numeric], + }), + ], + }), + ); + // Horizontal swaps the axes, so the time axis ends up as yAxis. + const { axisLabel } = echartOptions.yAxis as Record; + const labels = monthData.map(({ __timestamp }) => + axisLabel.formatter(__timestamp), + ); + + // Every month is a distinct label, so none should be blanked by the + // spacing/dedup formatter on a horizontal chart. + expect(labels.filter(label => label === '')).toHaveLength(0); +}); + test('boundary label alignment is dropped when the orientation moves the time axis to the side', () => { // The alignments position labels against the left and right edges of a // bottom axis. A horizontal chart swaps the axes, so applying them there diff --git a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts index ba49f4b38450..13a8638f1804 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts @@ -834,6 +834,55 @@ test('#39899 - x-axis dates do not overlap and last label stays visible at 0° r expect(axisLabel.hideOverlap).toBe(false); }); +test('#39899 - closely spaced x-axis time labels do not visually overlap', () => { + const formData = { + colorScheme: 'bnbColors', + datasource: '3__table', + granularity_sqla: 'ds', + timeGrainSqla: TimeGranularity.MINUTE, + x_axis_time_format: '%Y-%m-%d %H:%M:%S', + metric: 'sum__num', + viz_type: 'my_viz', + }; + const startTime = new Date('2026-01-01T00:00:00Z').getTime(); + const data = Array.from({ length: 20 }, (_, i) => ({ + sum__num: i, + __timestamp: startTime + i * 60 * 1000, + })); + const chartProps = new ChartProps({ + formData, + width: 300, + height: 400, + queriesData: [ + { + data, + colnames: ['sum__num', '__timestamp'], + coltypes: [GenericDataType.Numeric, GenericDataType.Temporal], + }, + ], + theme: supersetTheme, + }); + + const result = transformProps( + chartProps as unknown as EchartsTimeseriesChartProps, + ); + const { axisLabel } = result.echartOptions.xAxis as Record; + const labels = data.map(({ __timestamp }) => + axisLabel.formatter(__timestamp), + ); + + // hideOverlap must stay off so ECharts' own collision detection can never + // suppress the forced boundary label (#39899 must not regress). + expect(axisLabel.hideOverlap).toBe(false); + // The formatter itself must thin out labels that are too close together to + // render legibly in the available width. + expect(labels.filter(label => label === '').length).toBeGreaterThan(0); + // The first and last labels are the forced axis boundaries and must always + // stay visible. + expect(labels[0]).not.toBe(''); + expect(labels[labels.length - 1]).not.toBe(''); +}); + test('last x-axis date is visible and not cut off when rotated -45°', () => { const lastDataPointTimestamp = new Date('2026-12-01').getTime(); const result = transformProps( From c9f625627dcec9ffd7ac73b215b3023b133a5923 Mon Sep 17 00:00:00 2001 From: Joe Li Date: Tue, 1 Sep 2026 13:45:19 -0700 Subject: [PATCH 3/4] fix(explore): don't drop stashed control values from saved chart params (#43650) Co-authored-by: Claude Sonnet 5 --- .../explore/actions/saveModalActions.test.ts | 127 +++++++++++------- .../src/explore/actions/saveModalActions.ts | 53 +++++--- .../versionHistory/normalization.test.ts | 50 ------- .../features/versionHistory/normalization.ts | 45 +------ 4 files changed, 110 insertions(+), 165 deletions(-) diff --git a/superset-frontend/src/explore/actions/saveModalActions.test.ts b/superset-frontend/src/explore/actions/saveModalActions.test.ts index b60312ce52be..0a0dcbeed6aa 100644 --- a/superset-frontend/src/explore/actions/saveModalActions.test.ts +++ b/superset-frontend/src/explore/actions/saveModalActions.test.ts @@ -250,6 +250,50 @@ test('matches normalization metadata against finalized payload filters', async ( ]); }); +test('updateSlice keeps values stashed by a hidden control section in the saved params', async () => { + fetchMock.put(updateSliceEndpoint, sliceResponsePayload, { + name: updateSliceEndpoint, + }); + const dispatch = jest.fn(); + const getState = () => ({ + explore: { + // The Advanced analytics section is hidden (e.g. the x-axis column + // lost is_dttm), so StashFormDataContainer removed time_compare and + // comparison_type from form_data... + form_data: { + datasource: `${datasourceId}__${datasourceType}`, + viz_type: vizType, + }, + // ...and holds their previously-saved values here until the section + // becomes visible again. + hiddenFormData: { + time_compare: ['1 year ago'], + comparison_type: 'values', + }, + }, + }); + + await updateSlice( + { + ...sliceResponsePayload, + slice_id: sliceId, + form_data: { + ...formData, + time_compare: ['1 year ago'], + comparison_type: 'values', + }, + } as never, + sliceName, + [], + )(dispatch, getState); + + const request = fetchMock.callHistory.lastCall(updateSliceEndpoint); + const body = JSON.parse(request?.options.body as string); + const savedParams = JSON.parse(body.params); + expect(savedParams.time_compare).toEqual(['1 year ago']); + expect(savedParams.comparison_type).toEqual('values'); +}); + /** * Tests updateSlice action */ @@ -377,6 +421,33 @@ test('createSlice handles success', async () => { expect(slice).toEqual(sliceResponsePayload); }); +test('createSlice keeps values stashed by a hidden control section in the saved params', async () => { + fetchMock.post(createSliceEndpoint, sliceResponsePayload, { + name: createSliceEndpoint, + }); + const dispatch = jest.fn(); + const getState = () => ({ + explore: { + form_data: { + datasource: `${datasourceId}__${datasourceType}`, + viz_type: vizType, + }, + hiddenFormData: { + time_compare: ['1 year ago'], + comparison_type: 'values', + }, + }, + }); + + await createSlice(sliceName, [])(dispatch, getState as never); + + const request = fetchMock.callHistory.lastCall(createSliceEndpoint); + const body = JSON.parse(request?.options.body as string); + const savedParams = JSON.parse(body.params); + expect(savedParams.time_compare).toEqual(['1 year ago']); + expect(savedParams.comparison_type).toEqual('values'); +}); + test('createSlice handles failure', async () => { fetchMock.post(createSliceEndpoint, { throws: sampleError }); @@ -885,7 +956,7 @@ describe('getSlicePayload', () => { }); }); -test('existing-chart overwrite covers stash-removed keys as drop transitions', async () => { +test('existing-chart overwrite restores a stash-held value instead of dropping it', async () => { mockedIsFeatureEnabled.mockReturnValue(true); fetchMock.put(updateSliceEndpoint, sliceResponsePayload, { name: updateSliceEndpoint, @@ -926,55 +997,9 @@ test('existing-chart overwrite covers stash-removed keys as drop transitions', a const request = fetchMock.callHistory.lastCall(updateSliceEndpoint); const body = JSON.parse(request?.options.body as string); - expect(body.normalization_changes).toEqual([ - { - control: 'order_desc', - from_present: true, - from_value: true, - to_present: false, - }, - ]); -}); - -test('a stashed value the user changed before hiding is not covered', async () => { - mockedIsFeatureEnabled.mockReturnValue(true); - fetchMock.put(updateSliceEndpoint, sliceResponsePayload, { - name: updateSliceEndpoint, - }); - const dispatch = jest.fn(); - const getState = () => ({ - explore: { - form_data: { - datasource: `${datasourceId}__${datasourceType}`, - viz_type: vizType, - row_limit: 10000, - }, - // Stash holds a USER-edited value; persisted differs, so the removal - // stays recorded. - hiddenFormData: { order_desc: false }, - }, - versionHistory: { - chartNormalization: { - chartId: sliceId, - hydrationSessionId: 'hydration-drop-2', - saveAttemptId: null, - invalidatedControls: {}, - transitions: {}, - }, - }, - }); - - await updateSlice( - { - ...sliceResponsePayload, - slice_id: sliceId, - form_data: { ...formData, order_desc: true }, - } as never, - sliceName, - [], - )(dispatch, getState); - - const request = fetchMock.callHistory.lastCall(updateSliceEndpoint); - const body = JSON.parse(request?.options.body as string); + const savedParams = JSON.parse(body.params); + // The stashed value is written back into the saved params, so there is no + // drop for the normalization tracker to report. + expect(savedParams.order_desc).toBe(true); expect(body.normalization_changes).toBeUndefined(); }); diff --git a/superset-frontend/src/explore/actions/saveModalActions.ts b/superset-frontend/src/explore/actions/saveModalActions.ts index db40fea1f3c8..9e90f92cbbd9 100644 --- a/superset-frontend/src/explore/actions/saveModalActions.ts +++ b/superset-frontend/src/explore/actions/saveModalActions.ts @@ -41,10 +41,7 @@ import type { AutomaticNormalizationTransitions, ChartNormalizationTrackingState, } from 'src/features/versionHistory/types'; -import { - matchingAutomaticNormalizationTransitions, - stashDropNormalizationTransitions, -} from 'src/features/versionHistory/normalization'; +import { matchingAutomaticNormalizationTransitions } from 'src/features/versionHistory/normalization'; export interface PayloadSlice extends Slice { params: string; @@ -263,8 +260,14 @@ export const updateSlice = ) => { const { slice_id: sliceId, editors, form_data: formDataFromSlice } = slice; const initialState = getState(); + // Controls hidden by a form-section's visibility rule are stashed out of + // form_data so they don't affect the live query, but that hiding must not + // permanently delete the user's saved configuration when they save. const formData = JSON.parse( - JSON.stringify(initialState.explore?.form_data ?? {}), + JSON.stringify({ + ...initialState.explore?.hiddenFormData, + ...initialState.explore?.form_data, + }), ) as QueryFormData; const tracking = initialState.versionHistory?.chartNormalization; const saveAttemptId = nanoid(); @@ -289,22 +292,11 @@ export const updateSlice = formDataFromSlice, ); const savedFormData = JSON.parse(payload.params ?? '{}') as QueryFormData; - // Hydration-time transitions that still hold, plus save-time drops of - // keys the stash removed (mutually exclusive per control: a surviving - // hydration transition implies the key is present in the payload, a - // stash drop implies it is absent). + // Hydration-time transitions that still hold. Stashed values are now + // always merged back into the saved payload above, so a save can no + // longer drop a hidden control's value. const matchingTransitions = shouldAttachNormalization - ? { - ...matchingAutomaticNormalizationTransitions( - tracking, - savedFormData, - ), - ...stashDropNormalizationTransitions( - (formDataFromSlice ?? {}) as Record, - initialState.explore?.hiddenFormData, - savedFormData, - ), - } + ? matchingAutomaticNormalizationTransitions(tracking, savedFormData) : {}; if ( shouldAttachNormalization && @@ -345,8 +337,25 @@ export const createSlice = new?: boolean; }, ) => - async (dispatch: Dispatch, getState: () => Partial) => { - const formData = getState().explore?.form_data; + async ( + dispatch: Dispatch, + getState: () => Partial & { + explore?: { + form_data?: QueryFormData; + hiddenFormData?: Record; + }; + }, + ) => { + const exploreState = getState().explore; + // See the comment in updateSlice: stashed values from a hidden control + // section must not be dropped from the saved chart just because the + // section is currently hidden. + const formData = JSON.parse( + JSON.stringify({ + ...exploreState?.hiddenFormData, + ...exploreState?.form_data, + }), + ) as QueryFormData; try { const response = await SupersetClient.post({ endpoint: `/api/v1/chart/`, diff --git a/superset-frontend/src/features/versionHistory/normalization.test.ts b/superset-frontend/src/features/versionHistory/normalization.test.ts index 9a24700b844a..a7e797260a52 100644 --- a/superset-frontend/src/features/versionHistory/normalization.test.ts +++ b/superset-frontend/src/features/versionHistory/normalization.test.ts @@ -20,7 +20,6 @@ import { automaticNormalizationTransitions, isJsonValue, matchingAutomaticNormalizationTransitions, - stashDropNormalizationTransitions, } from './normalization'; test('recognizes only values that JSON can represent faithfully', () => { @@ -107,52 +106,3 @@ test('keeps only valid, unchanged transitions for a save', () => { }), ).toEqual({ row_limit: rowLimit }); }); - -test('covers a stash-removed key still equal to its persisted value', () => { - expect( - stashDropNormalizationTransitions( - { order_desc: true, row_limit: 5000 }, - { order_desc: true }, - { row_limit: 5000 }, - ), - ).toEqual({ - order_desc: { - control: 'order_desc', - from_present: true, - from_value: true, - to_present: false, - }, - }); -}); - -test('does not cover a stashed value the user changed before it was hidden', () => { - expect( - stashDropNormalizationTransitions( - { server_page_length: 10 }, - { server_page_length: 25 }, - {}, - ), - ).toEqual({}); -}); - -test('does not cover stashed keys that were never persisted', () => { - expect( - stashDropNormalizationTransitions({}, { totals_aggregate: 'SUM' }, {}), - ).toEqual({}); -}); - -test('does not cover keys the outgoing payload still carries', () => { - expect( - stashDropNormalizationTransitions( - { order_desc: true }, - { order_desc: true }, - { order_desc: true }, - ), - ).toEqual({}); -}); - -test('drop coverage requires a stash', () => { - expect( - stashDropNormalizationTransitions({ order_desc: true }, undefined, {}), - ).toEqual({}); -}); diff --git a/superset-frontend/src/features/versionHistory/normalization.ts b/superset-frontend/src/features/versionHistory/normalization.ts index ef47dd1f8664..5ee50ce89c6a 100644 --- a/superset-frontend/src/features/versionHistory/normalization.ts +++ b/superset-frontend/src/features/versionHistory/normalization.ts @@ -92,10 +92,9 @@ const automaticNormalizationTransition = ({ // Disappearing keys (!toPresent) are deliberately not covered here: // hydration itself never removes keys from the merged snapshot. Machine - // removals happen later, when StashFormDataContainer stashes invisible - // controls out of form_data — those are covered at save time by - // stashDropNormalizationTransitions, which uses the stash itself - // (explore.hiddenFormData) as the proof the removal was not user-made. + // removals used to happen later, when StashFormDataContainer stashed + // invisible controls out of form_data, but the save path now always + // merges the stash back in, so a save can no longer drop those keys. if (!inputMatchesPersisted || !toPresent || !hydrationChangedValue) { return undefined; } @@ -144,44 +143,6 @@ export const automaticNormalizationTransitions = ( return transitions; }; -/** - * Advisory transitions for keys the stash removed from form_data. - * - * StashFormDataContainer moves an invisible control's value out of - * ``form_data`` into ``explore.hiddenFormData``. That removal is - * machine-made by construction, but it happens in render effects after - * hydration, so hydration-time tracking cannot see it. This computes the - * matching drop transitions at save time: a key counts only when the stash - * holds it, the stashed value still equals the persisted value (a user edit - * before hiding breaks the equality and stays recorded), and the outgoing - * payload no longer carries the key. Keys absent from the stash — e.g. - * removed by a viz-type switch — are never covered. - */ -export const stashDropNormalizationTransitions = ( - persisted: Record, - hiddenFormData: Record | undefined, - outgoingFormData: Record, -): AutomaticNormalizationTransitions => { - const transitions: AutomaticNormalizationTransitions = {}; - if (!hiddenFormData) { - return transitions; - } - Object.keys(hiddenFormData).forEach(control => { - if (!Object.hasOwn(persisted, control)) return; - if (Object.hasOwn(outgoingFormData, control)) return; - const fromValue = persisted[control]; - if (!isJsonValue(fromValue)) return; - if (!jsonValuesEqual(hiddenFormData[control], fromValue)) return; - transitions[control] = { - control, - from_present: true, - from_value: fromValue, - to_present: false, - }; - }); - return transitions; -}; - export const matchingAutomaticNormalizationTransitions = ( tracking: ChartNormalizationTrackingState | null | undefined, formData: Record, From 531587a4b26c9615f069f2c1f848bfb8b2fbb441 Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Tue, 1 Sep 2026 15:04:18 -0700 Subject: [PATCH 4/4] fix(sql): catch sqlglot ParseError when parsing RLS predicates (#43651) --- superset/sql/parse.py | 20 ++++++++++++++++- tests/unit_tests/sql/parse_tests.py | 35 +++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/superset/sql/parse.py b/superset/sql/parse.py index b90cc55c471d..65e9a74aa67d 100644 --- a/superset/sql/parse.py +++ b/superset/sql/parse.py @@ -1539,7 +1539,25 @@ def parse_predicate(self, predicate: str) -> exp.Expression: :return: The parsed predicate. """ _check_script_length(predicate, self.engine) - return sqlglot.parse_one(predicate, dialect=self._dialect) + try: + return sqlglot.parse_one(predicate, dialect=self._dialect) + except sqlglot.errors.ParseError as ex: + kwargs = ( + { + "highlight": ex.errors[0]["highlight"], + "line": ex.errors[0]["line"], + "column": ex.errors[0]["col"], + } + if ex.errors + else {} + ) + raise SupersetParseError(predicate, self.engine, **kwargs) from ex + except sqlglot.errors.SqlglotError as ex: + raise SupersetParseError( + predicate, + self.engine, + message="Unable to parse predicate", + ) from ex def apply_rls( self, diff --git a/tests/unit_tests/sql/parse_tests.py b/tests/unit_tests/sql/parse_tests.py index 9749ef12c9bd..85781372d784 100644 --- a/tests/unit_tests/sql/parse_tests.py +++ b/tests/unit_tests/sql/parse_tests.py @@ -5578,6 +5578,41 @@ def test_parse_predicate_length_check() -> None: stmt.parse_predicate("x" * 101) +def test_parse_predicate_invalid_sql_raises_superset_parse_error() -> None: + """ + A syntactically invalid RLS predicate raises ``SupersetParseError``. + + ``parse_predicate`` is reachable via ``apply_rls`` for any RLS clause + configured on a queried table; an invalid clause must surface as the + typed 422 parse error rather than leaking a raw ``sqlglot`` exception. + """ + stmt = SQLStatement("SELECT 1", "postgresql") + with pytest.raises(SupersetParseError) as excinfo: + stmt.parse_predicate("a >") + assert excinfo.value.status == 422 + + +def test_parse_predicate_sqlglot_error_raises_superset_parse_error( + mocker: MockerFixture, +) -> None: + """ + A non-``ParseError`` ``sqlglot`` failure also surfaces as a typed error. + + ``parse_predicate`` catches the generic ``SqlglotError`` base class as a + fallback so any sqlglot failure (e.g. tokenize errors) is converted into a + ``SupersetParseError`` rather than leaking a raw sqlglot exception. + """ + # Build the statement before patching, since the constructor also parses. + stmt = SQLStatement("SELECT 1", "postgresql") + mocker.patch( + "sqlglot.parse_one", + side_effect=sqlglot.errors.SqlglotError("boom"), + ) + with pytest.raises(SupersetParseError) as excinfo: + stmt.parse_predicate("a > 1") + assert excinfo.value.status == 422 + + @pytest.mark.usefixtures("_small_parse_cap") def test_transpile_to_dialect_length_check() -> None: """