feat: add timeseries range selection #36689

eschutho · 2025-12-17T06:15:00Z

SUMMARY

This PR adds a click-and-drag time range selection feature to all timeseries chart types (Line, Area, Bar, Scatter, SmoothLine, Step). Users can draw a rectangle on the chart to select a time range, which automatically applies as a cross-filter.

Key features:

Automatic enablement: The brush selection is automatically enabled for charts with a time-based x-axis. No configuration required.
Rectangle selection: Users draw a rectangle to select a time range (vertical extent is ignored for filtering but provides visual flexibility).
Cross-filter integration: Selected time range creates a cross-filter with >= and <= operators, affecting other charts on the dashboard.
Touch device handling: Disabled on touch devices to avoid interfering with scrolling gestures.
Visual feedback: Selection area has a semi-transparent blue overlay with a subtle border.

Technical implementation:

Uses ECharts' brush component with brushType: 'rect'
Brush configuration is conditionally added when xAxisType === AxisType.time
brushSelected event handler extracts time range from coordRange and calls setDataMask
Filter label displays formatted time range (e.g., "Jan 1, 2024 - Mar 15, 2024")
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: Users had no way to directly select a time range on timeseries charts.

After: Users can click and drag to draw a rectangle, creating a time range cross-filter.

TESTING INSTRUCTIONS

Create a dashboard with a timeseries chart (Line, Area, Bar, etc.) using a time-based x-axis
Add another chart to the dashboard that shares the same time column
Click and drag on the timeseries chart to draw a rectangle selection
Verify:
A blue selection rectangle appears while dragging
After releasing, a cross-filter badge appears on the chart
Other charts on the dashboard are filtered to the selected time range
The filter label shows the formatted time range
Click elsewhere on the chart to clear the selection
Test on a mobile device or emulator to verify the feature is disabled (no brush on touch)
Test with a category x-axis to verify brush is not enabled (only for time axis)

ADDITIONAL INFORMATION

codeant-ai-for-open-source · 2025-12-17T06:15:06Z

CodeAnt AI is reviewing your PR.

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

codeant-ai-for-open-source · 2025-12-17T06:19:01Z

Nitpicks 🔍

🔒 No security issues identified
⚡ Recommended areas for review Brush axis targeting The newly added `brush` configuration always targets `xAxisIndex: 0`. When the chart is rendered horizontal (the code swaps x/y axes earlier), the time axis may no longer be the chart's "x" axis. As a result the brush selection may target the wrong axis or not behave as expected. Verify the brush targets the time axis in both orientations. Inconsistent date types in filters The brush handler uses coordRange values directly to build filters. Depending on the axis, these values may be Date objects or numbers. Other code paths (e.g. table filters) normalize Date -> timestamp before sending to backend. Verify and normalize brush values to the expected type (e.g., epoch ms) to avoid backend mismatches. Optional property may be undefined `isHorizontal` is declared optional in the transformed props. Consumers of TimeseriesChartTransformedProps may assume a boolean and not guard against `undefined`, causing conditional rendering bugs at runtime. Consider making it non-optional in the transformed props or ensure it is always set by the transform function with a sensible default. Naming / duplication mismatch Form input uses `orientation?: OrientationType` while transformed props add `isHorizontal?: boolean`. This duplication of layout intent can lead to inconsistent usage and confusion. Verify a single source of truth and ensure the transform reliably maps `orientation` -> `isHorizontal` (or rename for consistency). Missing edge case: inverted coordRange There is no test covering coordRange where start > end or where start === end. The handler assumes coordRange[0] contains [start, end] in order. Add tests to validate handling of inverted ranges or degenerate (zero-length) selections.

codeant-ai-for-open-source · 2025-12-17T06:19:28Z

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

  xValueFormatter,
  xAxis,
  refs,
  emitCrossFilters,
  coltypeMapping,
-  onLegendScroll,
+  isHorizontal,


Suggestion: The newly added isHorizontal prop is never used; leaving an unused prop in the function signature may trigger linter warnings and is misleading—rename it to _isHorizontal (or remove it) to signal it's intentionally unused. [possible bug]

Severity Level: Critical 🚨

Suggested change

isHorizontal,

_isHorizontal,

Why it matters? ⭐

The prop isHorizontal is indeed declared in the destructured props but never referenced anywhere in the component. Renaming to _isHorizontal (or removing it) is a small, correct change that avoids linter warnings and clarifies intent. This is a harmless, localized cleanup.

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx **Line:** 57:57 **Comment:** *Possible Bug: The newly added `isHorizontal` prop is never used; leaving an unused prop in the function signature may trigger linter warnings and is misleading—rename it to `_isHorizontal` (or remove it) to signal it's intentionally unused. 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.

codeant-ai-for-open-source · 2025-12-17T06:19:28Z

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

+
+      // Disable brush selection on touch devices to avoid interfering with scrolling
+      const isTouchDevice =
+        'ontouchstart' in window || navigator.maxTouchPoints > 0;


Suggestion: Accessing window and navigator directly will throw during server-side rendering (when window/navigator is undefined). Add guards using typeof window !== 'undefined' and typeof navigator !== 'undefined' before checking ontouchstart or maxTouchPoints. [possible bug]

Severity Level: Critical 🚨

Suggested change

'ontouchstart' in window || navigator.maxTouchPoints > 0;

(typeof window !== 'undefined' && 'ontouchstart' in window) ||

(typeof navigator !== 'undefined' && navigator.maxTouchPoints > 0);

Why it matters? ⭐

The handler reads global browser APIs; while this code runs only when the brush event fires in the browser, guarding with typeof window !== 'undefined' and typeof navigator !== 'undefined' is a robust defensive change that prevents potential ReferenceErrors in non-browser environments (SSR/test runners). The suggested improved code is sensible and low-risk.

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx **Line:** 177:177 **Comment:** *Possible Bug: Accessing `window` and `navigator` directly will throw during server-side rendering (when `window`/`navigator` is undefined). Add guards using `typeof window !== 'undefined'` and `typeof navigator !== 'undefined'` before checking `ontouchstart` or `maxTouchPoints`. 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.

codeant-ai-for-open-source · 2025-12-17T06:19:28Z

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

+    brush:
+      xAxisType === AxisType.time
+        ? {
+            toolbox: [],


Suggestion: Configuration risk: brush.toolbox was set to an empty array ([]), but the ECharts brush toolbox option expects an object (or to be omitted) — using an invalid type can lead to runtime errors in some ECharts versions; remove or set it to an object/undefined to avoid invalid configuration. [possible bug]

Severity Level: Critical 🚨

Suggested change

toolbox: [],

toolbox: undefined,

Why it matters? ⭐

The brush.toolbox in ECharts expects an object (or omitted/undefined). Passing an array is likely an invalid config and can cause runtime warnings or errors. Replacing it with undefined (or a proper object) is a safe, small fix that prevents malformed options from being passed to ECharts.

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts **Line:** 540:540 **Comment:** *Possible Bug: Configuration risk: `brush.toolbox` was set to an empty array (`[]`), but the ECharts brush `toolbox` option expects an object (or to be omitted) — using an invalid type can lead to runtime errors in some ECharts versions; remove or set it to an object/undefined to avoid invalid configuration. 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.

codeant-ai-for-open-source · 2025-12-17T06:19:28Z

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

+  const baseXAxis = {
+    label: '__timestamp',


Suggestion: The test uses the string literal '__timestamp' in multiple places which duplicates the real code's DTTM alias; define and use a single local constant DTTM_ALIAS so the test stays correct if the alias is ever refactored and to avoid divergence between test and implementation. [logic error]

Severity Level: Minor ⚠️

Suggested change

const baseXAxis = {

label: '__timestamp',

const DTTM_ALIAS = '__timestamp';

const baseXAxis = {

label: DTTM_ALIAS,

Why it matters? ⭐

Good, harmless suggestion. Centralizing the DTTM alias in a test reduces duplication and the risk of the test diverging from the implementation constant. It doesn't fix a bug but improves maintainability and makes future refactors simpler.

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/EchartsTimeseries.test.tsx **Line:** 30:31 **Comment:** *Logic Error: The test uses the string literal '__timestamp' in multiple places which duplicates the real code's DTTM alias; define and use a single local constant `DTTM_ALIAS` so the test stays correct if the alias is ever refactored and to avoid divergence between test and implementation. 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.

codeant-ai-for-open-source · 2025-12-17T06:19:28Z

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

+      const startFormatted = xValueFormatter(startValue);
+      const endFormatted = xValueFormatter(endValue);


Suggestion: xValueFormatter is typed to accept a number but coordRange values may arrive as strings from the chart; coerce startValue and endValue to numbers before passing to the formatter to avoid type errors or unexpected formatting results. [type error]

Severity Level: Minor ⚠️

Suggested change

const startFormatted = xValueFormatter(startValue);

const endFormatted = xValueFormatter(endValue);

const startFormatted = xValueFormatter(Number(startValue));

const endFormatted = xValueFormatter(Number(endValue));

Why it matters? ⭐

Coercing to Number before calling a formatter typed for numbers is reasonable defensive coding — it avoids a runtime type mismatch if coordRange can sometimes be strings. It's harmless in tests and can catch cases where the chart library emits strings. Note this diverges slightly from the production handler which doesn't coerce; if you want exact parity you could also update production code, but as a test hardening it's acceptable.

Prompt for AI Agent 🤖

This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/EchartsTimeseries.test.tsx **Line:** 84:85 **Comment:** *Type Error: `xValueFormatter` is typed to accept a number but `coordRange` values may arrive as strings from the chart; coerce `startValue` and `endValue` to numbers before passing to the formatter to avoid type errors or unexpected formatting results. 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.

codeant-ai-for-open-source · 2025-12-17T06:19:36Z

CodeAnt AI finished reviewing your PR.

Add click-and-drag time range selection to timeseries charts. When enabled, users can select a time range on the chart which automatically applies a time filter. This works for individual charts and integrates with dashboard filters. Changes: - Add timeRangeSelection form data option and default (false) - Add ECharts brush configuration for horizontal range selection - Add brushSelected event handler to capture selection and apply filter - Add "Time Range Filter Selection" checkbox to all timeseries chart control panels (Line, Area, Bar, Scatter, SmoothLine, Step) - Support both vertical and horizontal chart orientations

Add comprehensive tests for the time range brush selection feature: transformProps.test.ts: - Test brush configuration is enabled when x-axis type is time - Test brush has correct style properties - Test brush is NOT enabled when x-axis type is category - Test isHorizontal prop is correctly set EchartsTimeseries.test.tsx: - Test brush events are ignored for non-time axis charts - Test brush events are ignored on touch devices - Test filter is reset when brush selection is cleared - Test time range filter is created with correct format - Test custom time column is used when x-axis label is not __timestamp - Test range operators (>= and <=) are used correctly - Test label is formatted using xValueFormatter

- Remove unused isHorizontal prop from component destructuring - Add SSR safety guards for window and navigator access

pull-request-size bot added the size/XL label Dec 17, 2025

github-actions bot added the plugins label Dec 17, 2025

eschutho marked this pull request as draft December 17, 2025 06:15

codeant-ai-for-open-source bot added the size:XL This PR changes 500-999 lines, ignoring generated files label Dec 17, 2025

eschutho closed this Dec 17, 2025

eschutho force-pushed the claude/add-timeseries-range-selection-zcDmj branch from 524454f to ea3d247 Compare December 17, 2025 06:17

pull-request-size bot added size/XS and removed size/XL labels Dec 17, 2025

codeant-ai-for-open-source bot reviewed Dec 17, 2025

View reviewed changes

claude added 2 commits December 17, 2025 06:30

eschutho reopened this Dec 17, 2025

pull-request-size bot added size/XL and removed size/XS labels Dec 17, 2025

eschutho changed the title ~~Claude/add timeseries range selection zc dmj~~ feat: add timeseries range selection Dec 17, 2025

sadpandajoe added the review:draft label Dec 17, 2025

fix(charts): address code review issues for time range selection

fd41ee7

- Remove unused isHorizontal prop from component destructuring - Add SSR safety guards for window and navigator access

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add timeseries range selection #36689

feat: add timeseries range selection #36689

eschutho commented Dec 17, 2025 •

edited

Loading

Uh oh!

codeant-ai-for-open-source bot commented Dec 17, 2025

Uh oh!

codeant-ai-for-open-source bot commented Dec 17, 2025

Uh oh!

codeant-ai-for-open-source bot Dec 17, 2025

Uh oh!

codeant-ai-for-open-source bot Dec 17, 2025

Uh oh!

codeant-ai-for-open-source bot Dec 17, 2025

Uh oh!

codeant-ai-for-open-source bot Dec 17, 2025

Uh oh!

codeant-ai-for-open-source bot Dec 17, 2025

Uh oh!

codeant-ai-for-open-source bot commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	'ontouchstart' in window \|\| navigator.maxTouchPoints > 0;
	(typeof window !== 'undefined' && 'ontouchstart' in window) \|\|
	(typeof navigator !== 'undefined' && navigator.maxTouchPoints > 0);

		const startFormatted = xValueFormatter(startValue);
		const endFormatted = xValueFormatter(endValue);

feat: add timeseries range selection #36689

Are you sure you want to change the base?

feat: add timeseries range selection #36689

Conversation

eschutho commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

Key features:

Technical implementation:

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

codeant-ai-for-open-source bot commented Dec 17, 2025

Thanks for using CodeAnt! 🎉

Uh oh!

codeant-ai-for-open-source bot commented Dec 17, 2025

Nitpicks 🔍

Uh oh!

codeant-ai-for-open-source bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source bot Dec 17, 2025

Choose a reason for hiding this comment

Uh oh!

codeant-ai-for-open-source bot commented Dec 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eschutho commented Dec 17, 2025 •

edited

Loading