Skip to content
Merged
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 @@ -102,7 +102,9 @@ import {
import { TIMEGRAIN_TO_TIMESTAMP, TIMESERIES_CONSTANTS } from '../constants';
import { getDefaultTooltip } from '../utils/tooltip';
import {
createSpacedXAxisFormatter,
getTooltipTimeFormatter,
getXAxisDomain,
getXAxisFormatter,
getYAxisFormatter,
} from '../utils/formatters';
Expand Down Expand Up @@ -661,44 +663,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<string, unknown>[],
rebasedDataB as Record<string, unknown>[],
],
xAxisLabel,
),
Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0),
)
: xAxisFormatter;

const yAxisTitleMarginPx = convertInteger(yAxisTitleMargin);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,11 @@ import {
} from '../constants';
import { getDefaultTooltip } from '../utils/tooltip';
import {
createDedupXAxisFormatter,
createSpacedXAxisFormatter,
getPercentFormatter,
getTooltipTimeFormatter,
getXAxisDomain,
getXAxisFormatter,
getYAxisFormatter,
} from '../utils/formatters';
Expand Down Expand Up @@ -1207,46 +1210,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<string, unknown>[]],
xAxisLabel,
),
Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0),
)
: xAxisFormatter;

const temporalTickValues = resolveTemporalTickValues(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Comment on lines +58 to +60

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: The collision threshold assumes every rendered character is exactly seven pixels wide, but the formatter is also used for localized and custom time formats. Wide glyphs or long localized month names can exceed this estimate, causing the wrapper to retain labels whose actual rendered footprints overlap; narrow glyphs can also cause valid labels to be unnecessarily removed. Use measured text width or a conservative width calculation based on the actual formatter output and font. [possible bug]

Severity Level: Major ⚠️
- ⚠️ Custom time formats can remain visually overlapped.
- ⚠️ Localized month labels may be thinned inaccurately.
- ⚠️ Non-overlapping labels may be unnecessarily hidden.

Use CodeAnt Skill Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/src/constants.ts
**Line:** 58:60
**Comment:**
	*Possible Bug: The collision threshold assumes every rendered character is exactly seven pixels wide, but the formatter is also used for localized and custom time formats. Wide glyphs or long localized month names can exceed this estimate, causing the wrapper to retain labels whose actual rendered footprints overlap; narrow glyphs can also cause valid labels to be unnecessarily removed. Use measured text width or a conservative width calculation based on the actual formatter output and font.

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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Acknowledged — per rebenitez1802's review, the fixed 7px/char estimate is a real but acceptable heuristic (worst case still beats no suppression). A measureText-based estimate would be a nice follow-up, not blocking this fix.

};

export enum OpacityEnum {
Expand Down
150 changes: 150 additions & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
getTimeFormatter,
isSavedMetric,
NumberFormats,
NumberFormatter,
QueryFormMetric,
SMART_DATE_DETAILED_ID,
SMART_DATE_ID,
Expand All @@ -32,6 +33,7 @@ import {
TimeGranularity,
ValueFormatter,
} from '@superset-ui/core';
import { TIMESERIES_CONSTANTS } from '../constants';

export const getSmartDateDetailedFormatter = () =>
getTimeFormatter(SMART_DATE_DETAILED_ID);
Expand Down Expand Up @@ -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<string, unknown>[][],
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;
}
});
Comment on lines +357 to +362

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: getXAxisDomain only recognizes numeric values, but temporal query rows can contain timestamp strings or Date objects. For those charts both bounds remain undefined, so pixelsPerMs is disabled and, because hideOverlap is simultaneously set to false, no overlap suppression is applied at all. Normalize temporal values to epoch milliseconds before calculating the domain. [logic error]

Severity Level: Major ⚠️
- ❌ Dense string-timestamp charts retain overlapping labels.
- ❌ Date-valued query records bypass spacing suppression.
- ⚠️ Timeseries and MixedTimeseries are both affected.

Use CodeAnt Skill Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts
**Line:** 311:316
**Comment:**
	*Logic Error: `getXAxisDomain` only recognizes numeric values, but temporal query rows can contain timestamp strings or `Date` objects. For those charts both bounds remain undefined, so `pixelsPerMs` is disabled and, because `hideOverlap` is simultaneously set to false, no overlap suppression is applied at all. Normalize temporal values to epoch milliseconds before calculating the domain.

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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Acknowledged — per rebenitez1802's review, this looks like a false positive on the standard pipeline: the chart-data API serializes temporal columns to epoch-ms numbers, so the numeric-only filter in getXAxisDomain is correct there. Not blocking this fix.

});
return [domainMin, domainMax];
}
Original file line number Diff line number Diff line change
Expand Up @@ -1324,6 +1324,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<string, any>;
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).
Expand Down
Loading
Loading