Skip to content

Commit 40f0081

Browse files
rusackasclaude
andcommitted
fix(timeseries): fix category-axis snapping, persist dragged baseline, throttle drag updates
Address three review findings on the percent-change draggable baseline: - snapToNearestX no longer coerces category-axis x values with Number(), which produced NaN and broke baseline positioning/snapping on non-temporal x-axes; it now validates the (already-snapped) category value against the known points instead. - the dragged baseline position is now persisted in a ref so effect reruns (resizes, other option changes) don't silently snap it back to the first point. - ondrag updates are coalesced to one setOption per animation frame instead of rebasing every series on every raw pointer-move event. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent b74b3eb commit 40f0081

3 files changed

Lines changed: 58 additions & 15 deletions

File tree

‎superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx‎

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,10 @@ export default function EchartsTimeseries({
8181
const rebaseEnabled = Boolean(
8282
(formData as { rebasePercentChange?: boolean }).rebasePercentChange,
8383
);
84+
// Persists the dragged baseline across effect reruns (e.g. resizes or
85+
// other option changes) so those don't silently snap it back to the
86+
// first point.
87+
const baselineXRef = useRef<number | string | null>(null);
8488
useEffect(() => {
8589
if (!rebaseEnabled) return undefined;
8690
const chart = echartRef.current?.getEchartInstance?.();
@@ -94,14 +98,27 @@ export default function EchartsTimeseries({
9498
? (s.data.filter(Array.isArray) as SeriesDataPoint[])
9599
: [],
96100
);
97-
const xs = Array.from(
98-
new Set(baseSeries.flat().map(([x]) => Number(x))),
99-
).sort((a, b) => a - b);
101+
// Preserve the axis' native x type: numeric for time/value axes,
102+
// string for category axes (coercing categories with Number() would
103+
// turn them into NaN and break snapping/positioning below).
104+
const xs = Array.from(new Set(baseSeries.flat().map(([x]) => x)));
100105
if (xs.length === 0) return undefined;
101-
let baselineX = xs[0];
106+
if (typeof xs[0] === 'number') {
107+
(xs as number[]).sort((a, b) => a - b);
108+
}
109+
let baselineX =
110+
baselineXRef.current !== null && xs.includes(baselineXRef.current)
111+
? baselineXRef.current
112+
: xs[0];
113+
baselineXRef.current = baselineX;
114+
// Coalesces drag updates to at most one setOption per animation
115+
// frame; rebasing every series on every raw pointer-move event
116+
// can stutter on large charts.
117+
let dragFrame: ReturnType<typeof requestAnimationFrame> | null = null;
102118

103-
const applyBaseline = (newX: number) => {
119+
const applyBaseline = (newX: number | string) => {
104120
baselineX = newX;
121+
baselineXRef.current = newX;
105122
chart.setOption({
106123
series: baseSeries.map(data => ({
107124
data: rebaseSeriesData(data, newX),
@@ -133,11 +150,15 @@ export default function EchartsTimeseries({
133150
const dataX = chart.convertFromPixel(
134151
{ xAxisIndex: 0 },
135152
this.x + 4,
136-
) as number;
137-
const snapped = snapToNearestX(xs, dataX);
138-
if (snapped !== undefined && snapped !== baselineX) {
139-
applyBaseline(snapped);
140-
}
153+
) as number | string;
154+
if (dragFrame !== null) return;
155+
dragFrame = requestAnimationFrame(() => {
156+
dragFrame = null;
157+
const snapped = snapToNearestX(xs, dataX);
158+
if (snapped !== undefined && snapped !== baselineX) {
159+
applyBaseline(snapped);
160+
}
161+
});
141162
},
142163
ondragend: () => drawHandle(),
143164
children: [
@@ -159,6 +180,9 @@ export default function EchartsTimeseries({
159180
drawHandle();
160181

161182
return () => {
183+
if (dragFrame !== null) {
184+
cancelAnimationFrame(dragFrame);
185+
}
162186
chart.setOption({
163187
graphic: [{ id: 'percent-change-baseline', $action: 'remove' }],
164188
});

‎superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/percentChange.ts‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -89,13 +89,25 @@ export function rebaseSeriesData(
8989
return data.map(([x, y]) => [x, y == null ? null : (1 + y) / divisor - 1]);
9090
}
9191

92-
/** Snaps a dragged x position to the nearest available data x. */
92+
/**
93+
* Snaps a dragged x position to the nearest available data x. Time/value
94+
* axes report a continuous pixel-derived number, so the nearest point is
95+
* found by numeric distance. Category axes report the exact category
96+
* value already snapped by ECharts; there's no numeric scale to measure
97+
* distance against, so it's only validated against the known x values
98+
* (falling back to the first one if the pixel landed outside the axis).
99+
*/
93100
export function snapToNearestX(
94-
xs: number[],
95-
target: number,
96-
): number | undefined {
101+
xs: (number | string)[],
102+
target: number | string,
103+
): number | string | undefined {
97104
if (xs.length === 0) return undefined;
98-
return xs.reduce((best, x) =>
105+
if (typeof target === 'string') {
106+
return xs.includes(target) ? target : xs[0];
107+
}
108+
const numericXs = xs.filter((x): x is number => typeof x === 'number');
109+
if (numericXs.length === 0) return undefined;
110+
return numericXs.reduce((best, x) =>
99111
Math.abs(x - target) < Math.abs(best - target) ? x : best,
100112
);
101113
}

‎superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/percentChange.test.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,3 +89,10 @@ test('snaps to the nearest available x', () => {
8989
expect(snapToNearestX([10, 20, 30], 26)).toBe(30);
9090
expect(snapToNearestX([], 5)).toBeUndefined();
9191
});
92+
93+
test('validates a category-axis x instead of coercing it to a number', () => {
94+
// category axes report their exact (already-snapped) value on drag
95+
expect(snapToNearestX(['a', 'b', 'c'], 'b')).toBe('b');
96+
// a pixel that landed outside the axis falls back to the first category
97+
expect(snapToNearestX(['a', 'b', 'c'], 'unknown')).toBe('a');
98+
});

0 commit comments

Comments
 (0)