Copilot commented on code in PR #42792:
URL: https://github.com/apache/superset/pull/42792#discussion_r3718979820
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -257,15 +357,28 @@ export default function EchartsTimeseries({
// Cross-filter by dimension (original behavior)
const { seriesName: name } = props;
handleChange(name);
- } else if (canCrossFilterByXAxis && props.componentType === 'series') {
+ } else if (
+ canCrossFilterByXAxis &&
+ xAxis.type === AxisType.Category &&
+ props.componentType === 'series'
+ ) {
// Cross-filter by X-axis value when no dimensions (issue #25334)
- const categoryAxisValue = getCategoryAxisValue(
+ const categoryAxisValue = getXAxisValue(
props.data,
props.name,
);
if (categoryAxisValue !== undefined) {
handleXAxisChange(categoryAxisValue);
}
+ } else if (
+ canCrossFilterByXAxis &&
+ xAxis.type === AxisType.Time &&
+ props.componentType === 'series'
+ ) {
+ const timeAxisValue = getXAxisValue(props.data, props.name);
+ if (typeof timeAxisValue === 'number') {
+ handleTimeAxisChange(timeAxisValue);
+ }
}
Review Comment:
`getXAxisValue` can return a `string | number`, but the time-series click
path only handles `number`. If ECharts provides time values as ISO strings (or
if `props.name` is a formatted date string), temporal cross-filtering will
silently no-op on point clicks while axis-label clicks still work. Consider
accepting `string` as well and parsing it (e.g., via the same parsing helper
used for axis labels) so point clicks and label clicks behave consistently.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -40,6 +42,29 @@ import { formatSeriesName } from '../utils/series';
import { ExtraControls } from '../components/ExtraControls';
const TIMER_DURATION = 300;
+const getTimestampFromTimeAxisLabel = (value: string | number) => {
+ if (typeof value === 'number') {
+ return Number.isFinite(value) ? value : undefined;
+ }
+ const timestamp = Date.parse(value);
+ if (Number.isNaN(timestamp)) {
+ console.warn('Unable to parse time axis label for cross-filtering', value);
+ }
+ return Number.isNaN(timestamp) ? undefined : timestamp;
+};
+
+const formatDateTime = (date: Date) =>
+ [
+ date.getUTCFullYear(),
+ String(date.getUTCMonth() + 1).padStart(2, '0'),
+ String(date.getUTCDate()).padStart(2, '0'),
+ ].join('-') +
+ 'T' +
+ [
+ String(date.getUTCHours()).padStart(2, '0'),
+ String(date.getUTCMinutes()).padStart(2, '0'),
+ String(date.getUTCSeconds()).padStart(2, '0'),
+ ].join(':');
Review Comment:
This hand-rolled formatter duplicates common ISO/UTC formatting logic and
drops milliseconds/timezone markers. Since the temporal range is computed with
a `+ 1` ms adjustment, omitting milliseconds can accidentally erase the
exclusivity adjustment if `exclusiveEnd` is not exactly on a second boundary.
Consider switching to a single canonical UTC formatter already used elsewhere
in the codebase (or explicitly include milliseconds / document the assumptions)
to avoid subtle parsing/semantic mismatches.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -40,6 +42,29 @@ import { formatSeriesName } from '../utils/series';
import { ExtraControls } from '../components/ExtraControls';
const TIMER_DURATION = 300;
+const getTimestampFromTimeAxisLabel = (value: string | number) => {
+ if (typeof value === 'number') {
+ return Number.isFinite(value) ? value : undefined;
+ }
+ const timestamp = Date.parse(value);
+ if (Number.isNaN(timestamp)) {
+ console.warn('Unable to parse time axis label for cross-filtering', value);
+ }
+ return Number.isNaN(timestamp) ? undefined : timestamp;
Review Comment:
Using `console.warn` in production UI code can produce noisy logs and is
hard to route/disable consistently. Prefer the project’s standard logging
utility (or a debug-only guard) so warnings can be managed centrally and
(optionally) include chart context for troubleshooting.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.test.tsx:
##########
@@ -510,41 +511,438 @@ test('does not emit duplicate cross-filter for generic
axis label clicks', async
expect(setDataMaskMock).not.toHaveBeenCalled();
});
-test('does not emit cross-filter when no dimensions and time-based X-axis',
async () => {
+test('emits TEMPORAL_RANGE cross-filter from time axis label click on day
bucket', () => {
const setDataMaskMock = jest.fn();
const propsWithTimeXAxis: TimeseriesChartTransformedProps = {
...defaultProps,
emitCrossFilters: true,
setDataMask: setDataMaskMock,
groupby: [], // No dimensions
+ formData: {
+ ...defaultFormData,
+ granularitySqla: 'ds',
+ timeGrainSqla: TimeGranularity.DAY,
+ },
xAxis: {
- label: '__timestamp',
- type: AxisType.Time, // Time-based X-axis (not categorical)
+ label: DTTM_ALIAS,
+ type: AxisType.Time,
},
};
render(<EchartsTimeseries {...propsWithTimeXAxis} />);
- const lastCall = mockEchart.mock.calls.at(-1);
- expect(lastCall).toBeDefined();
- const [props] = lastCall as [EchartsProps];
+ const labelClickHandler = getLatestEchartProps().queryEventHandlers?.find(
+ ({ query }) => query === 'xAxis',
+ )?.handler;
+ expect(labelClickHandler).toBeDefined();
+ labelClickHandler?.({
+ targetType: 'axisLabel',
+ value: '2021-01-01',
+ } as ECElementEvent);
- // Simulate a click event
- const clickHandler = props.eventHandlers?.click;
- if (clickHandler) {
- clickHandler({
- componentType: 'series',
- seriesName: 'Sales',
- data: [1609459200000, 100], // Timestamp
- name: '2021-01-01',
- dataIndex: 0,
- });
+ expect(setDataMaskMock.mock.calls[0][0].extraFormData.filters).toEqual([
+ {
+ col: 'ds',
+ op: 'TEMPORAL_RANGE',
+ val: '2021-01-01T00:00:00 : 2021-01-02T00:00:00',
+ },
+ ]);
+});
- // Wait a bit and verify setDataMask was NOT called
- await new Promise(resolve => setTimeout(resolve, 400));
- expect(setDataMaskMock).not.toHaveBeenCalled();
- }
+test('emits TEMPORAL_RANGE cross-filter from time axis label click on month
bucket', () => {
+ const setDataMaskMock = jest.fn();
+
+ render(
+ <EchartsTimeseries
+ {...defaultProps}
+ emitCrossFilters
+ setDataMask={setDataMaskMock}
+ groupby={[]}
+ formData={{
+ ...defaultFormData,
+ granularitySqla: 'ds',
+ timeGrainSqla: TimeGranularity.MONTH,
+ }}
+ xAxis={{
+ label: DTTM_ALIAS,
+ type: AxisType.Time,
+ }}
+ />,
+ );
+
+ const labelClickHandler = getLatestEchartProps().queryEventHandlers?.find(
+ ({ query }) => query === 'xAxis',
+ )?.handler;
+ expect(labelClickHandler).toBeDefined();
+ labelClickHandler?.({
+ targetType: 'axisLabel',
+ value: '2021-01-01',
+ } as ECElementEvent);
+
+ expect(setDataMaskMock.mock.calls[0][0].extraFormData.filters).toEqual([
+ {
+ col: 'ds',
+ op: 'TEMPORAL_RANGE',
+ val: '2021-01-01T00:00:00 : 2021-02-01T00:00:00',
+ },
+ ]);
+});
+
+test('emits TEMPORAL_RANGE cross-filter from time axis label click on year
bucket', () => {
+ const setDataMaskMock = jest.fn();
+
+ render(
+ <EchartsTimeseries
+ {...defaultProps}
+ emitCrossFilters
+ setDataMask={setDataMaskMock}
+ groupby={[]}
+ formData={{
+ ...defaultFormData,
+ granularitySqla: 'ds',
+ timeGrainSqla: TimeGranularity.YEAR,
+ }}
+ xAxis={{
+ label: DTTM_ALIAS,
+ type: AxisType.Time,
+ }}
+ />,
+ );
+
+ const labelClickHandler = getLatestEchartProps().queryEventHandlers?.find(
+ ({ query }) => query === 'xAxis',
+ )?.handler;
+ expect(labelClickHandler).toBeDefined();
+ labelClickHandler?.({
+ targetType: 'axisLabel',
+ value: '2021-01-01',
+ } as ECElementEvent);
+
+ expect(setDataMaskMock.mock.calls[0][0].extraFormData.filters).toEqual([
+ {
+ col: 'ds',
+ op: 'TEMPORAL_RANGE',
+ val: '2021-01-01T00:00:00 : 2022-01-01T00:00:00',
+ },
+ ]);
+});
+
+test('emits upper-exclusive TEMPORAL_RANGE from time point click on month
bucket', async () => {
+ const setDataMaskMock = jest.fn();
+
+ render(
+ <EchartsTimeseries
+ {...defaultProps}
+ emitCrossFilters
+ setDataMask={setDataMaskMock}
+ groupby={[]}
+ formData={{
+ ...defaultFormData,
+ granularitySqla: 'ds',
+ timeGrainSqla: TimeGranularity.MONTH,
+ }}
+ xAxis={{
+ label: DTTM_ALIAS,
+ type: AxisType.Time,
+ }}
+ />,
+ );
+
+ const clickHandler = getLatestEchartProps().eventHandlers?.click;
+ expect(clickHandler).toBeDefined();
+ clickHandler?.({
+ componentType: 'series',
+ seriesName: 'Sales',
+ data: [Date.UTC(2021, 0, 1), 100],
+ name: '2021-01-01',
+ dataIndex: 0,
+ });
+
+ await waitFor(
+ () => {
+ expect(setDataMaskMock).toHaveBeenCalled();
+ },
+ { timeout: 500 },
+ );
Review Comment:
Several tests wait on real timeouts to accommodate the 300ms click debounce,
which can slow the suite and introduce CI flakiness under load. Consider using
Jest fake timers and advancing time deterministically (or otherwise avoiding
real-time waits) to make these tests faster and more reliable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
