feat(charts): Adding new chart range selection hook #103748
Conversation
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| setState(null); | ||
|
|
||
| onClearSelection?.(); | ||
| }, [chartRef, onClearSelection, state?.selection]); |
There was a problem hiding this comment.
Bug: Stale closure in clearSelection passed to onSelectionEnd
The clearSelection function passed to onSelectionEnd captures stale state. When onBrushEnd calls onSelectionEnd with clearSelection on line 235, the function has the old state in its closure (before setState on line 230 takes effect). If this is the first selection, state is undefined, causing the early return on line 163 to prevent clearing. Consumers calling clearSelection immediately after onSelectionEnd will find it non-functional. The original implementation avoided this by not checking state in clearSelection.
| if (tooltipFrameRef.current) cancelAnimationFrame(tooltipFrameRef.current); | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [state, disabled, enableBrushMode, chartRef.current, chartsGroupName, deps]); |
There was a problem hiding this comment.
Bug: Ref current property used in dependency array
The useEffect dependency array includes chartRef.current, but React doesn't track changes to ref .current properties. If the chart mounts after the hook initializes, the effect won't re-run when chartRef.current changes from null to the chart instance, preventing brush mode from being activated. The ref object itself (chartRef) is stable and shouldn't be in the dependency array, or the effect should rely on the deps parameter for re-activation.
| }: ChartXRangeSelectionProps): BoxSelectionOptions { | ||
| const [state, setState] = useState<State>(); | ||
| const tooltipFrameRef = useRef<number | null>(null); | ||
| const enabbleBrushModeFrameRef = useRef<number | null>(null); |
There was a problem hiding this comment.
| const enabbleBrushModeFrameRef = useRef<number | null>(null); | |
| const enableBrushModeFrameRef = useRef<number | null>(null); |
| const enabbleBrushModeFrameRef = useRef<number | null>(null); | ||
|
|
||
| const onBrushStart = useCallback<EChartBrushStartHandler>( | ||
| (_evt: any, chartInstance: any) => { |
There was a problem hiding this comment.
Why not just let typescript inherit type from EChartBrushStartHandler instead typing this these args as any?
There was a problem hiding this comment.
@edwardgou-sentry the typing around echarts event handlers seem outdated, judging from echarts.d.ts vs printing out properties in the console. Casting as any is a common pattern in this space:
https://github.com/getsentry/sentry/blob/master/static/app/components/charts/chartZoom.tsx#L266-L267
https://github.com/getsentry/sentry/blob/master/static/app/components/charts/barChartZoom.tsx#L105-L106
On that note i have removed the any castes in my latest commit, added some comments around retrieving the axis extremes for the chart.
| }, [chartRef, onClearSelection, state?.selection]); | ||
|
|
||
| const onBrushEnd = useCallback<EChartBrushEndHandler>( | ||
| (evt: any, chartInstance: any) => { |
There was a problem hiding this comment.
Same question about types as onBrushStart
There was a problem hiding this comment.
Addressed in the comment above.
| if (tooltipFrameRef.current) cancelAnimationFrame(tooltipFrameRef.current); | ||
| }; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [state, disabled, enableBrushMode, chartRef.current, chartsGroupName, deps]); |
There was a problem hiding this comment.
Would chartRef.current trigger useEffect if it's a ref? Sounds like it might not.
Would it make sense to use chartRef instead as a dep to satisfy eslint?
There was a problem hiding this comment.
Yep, I think we should remove this too, great catch
edwardgou-sentry
left a comment
edwardgou-sentry
left a comment
There was a problem hiding this comment.
I think a couple of the useEffects could use a comment to explain what they are responsible for handling for clarity. Code changes seem reasonable to me though.
…#103814) - This PR uses the new `useChartXRangeSelection` to enable range selection only in explore charts - Removes the old `useBoxChartSelection` hook <img width="1214" height="663" alt="Screenshot 2025-11-21 at 11 23 04 AM" src="https://github.com/user-attachments/assets/0c837d4f-0af5-4088-8bbb-75fea46b4832" /> --------- Co-authored-by: Abdullah Khan <abdullahkhan@PG9Y57YDXQ.local>
| cancelAnimationFrame(enableBrushModeFrameRef.current); | ||
| if (tooltipFrameRef.current) cancelAnimationFrame(tooltipFrameRef.current); | ||
| }; | ||
| }, [state, disabled, enableBrushMode, chartRef, chartsGroupName, deps]); |
There was a problem hiding this comment.
Bug: Dependency array causes effect to run every render
The deps array is included directly in the useEffect dependency list, but React compares arrays by reference. Since callers pass a new array like deps: [props.plottables] on every render, this causes the effect to run unnecessarily on every render, triggering repeated brush mode reactivations and potential performance issues. The deps should be spread into the dependency array using ...deps instead.
This hook generalizes the functionality introduced by: https://github.com/getsentry/sentry/blob/master/static/app/views/explore/hooks/useChartBoxSelect.tsx
Design Notes:
Flexible callbacks:
onSelectionStart,onSelectionEnd, andonClearSelectionlet consumers tap into the brushing lifecycle without embedding chart-specific logic in the hook.Custom action menu:
actionMenuRendererallows callers to fully control the menu’s UI while the hook manages placement, portals, and clear behavior. Passes selected range and utils likeclearSelectionas paramsControlled redraws: A
depsarray lets callers decide when the selection should reactivate or repaint, keeping multi-chart views in sync.Brush mode management: The hook handles enabling brush mode, hiding tooltips during drag, reconnecting chart groups, and restoring visuals as needed.
Robust outside-click behavior: click handler clears the selection unless the click is inside a marked “safe” region (data-explore-chart-selection-region). This avoids accidental clears while still allowing intuitive dismissal.
Improvements:
Added tests
The cursor comments are valid, I will be addressing them