fix(dashboards): Only trigger echarts dispatch sync for visible widgets#110683
Conversation
Uses an intersection observer to register echarts instances to groups when the charts are visible in the viewport. This way, we don't dispatch cursor sync calls to widgets that are not visible, bypassing some of the load echarts needs to do to properly place the crosshairs
|
@gggritso could I get your review on this? I think you have the most context about widget sync. I think this should be a relatively simple change, in my testing it seems like it's helping with hovering perf because it minimizes how much we need to dispatch to other charts since they're not included in the groups |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
- ✅ Fixed: Observer loses tracked charts on groupName re-render
- Used useState for stable groupName and re-observe all charts when observer changes in useEffect.
- ✅ Fixed: Registered charts never cleaned up from map
- Changed register to return an unregister function that removes charts from Map and calls unobserve.
- ✅ Fixed: Observer is null when child charts first register
- Created observer synchronously using useMemo instead of useEffect so it exists before children mount.
Or push these changes by commenting:
@cursor push e106592da0
Preview (e106592da0)
diff --git a/static/app/views/dashboards/contexts/widgetSyncContext.tsx b/static/app/views/dashboards/contexts/widgetSyncContext.tsx
--- a/static/app/views/dashboards/contexts/widgetSyncContext.tsx
+++ b/static/app/views/dashboards/contexts/widgetSyncContext.tsx
@@ -1,12 +1,12 @@
import type {ReactNode} from 'react';
-import {useCallback, useEffect, useRef} from 'react';
+import {useCallback, useEffect, useMemo, useRef, useState} from 'react';
import type {EChartsType} from 'echarts';
import * as echarts from 'echarts';
import {uniqueId} from 'sentry/utils/guid';
import {createDefinedContext} from 'sentry/utils/performance/contexts/utils';
-type RegistrationFunction = (chart: EChartsType) => void;
+type RegistrationFunction = (chart: EChartsType) => () => void;
interface WidgetSyncContext {
groupName: string;
@@ -26,13 +26,17 @@
export function WidgetSyncContextProvider({
children,
- groupName = uniqueId(),
+ groupName: groupNameProp,
}: WidgetSyncContextProviderProps) {
+ // Use useState to ensure stable groupName when not provided as prop
+ const [stableGroupName] = useState(() => groupNameProp ?? uniqueId());
+ const groupName = groupNameProp ?? stableGroupName;
+
const chartsRef = useRef<Map<Element, EChartsType>>(new Map());
- const observerRef = useRef<IntersectionObserver | null>(null);
- useEffect(() => {
- observerRef.current = new IntersectionObserver(entries => {
+ // Create observer synchronously so it exists before children mount (fixes Bug 3)
+ const observerRef = useMemo(() => {
+ return new IntersectionObserver(entries => {
for (const entry of entries) {
const chart = chartsRef.current.get(entry.target);
if (!chart) {
@@ -48,27 +52,41 @@
echarts?.connect(groupName);
});
+ }, [groupName]);
+ // Re-observe all existing charts when observer changes (fixes Bug 1)
+ useEffect(() => {
+ const charts = Array.from(chartsRef.current.entries());
+ for (const [dom] of charts) {
+ observerRef.observe(dom);
+ }
+
return () => {
- observerRef.current?.disconnect();
+ observerRef.disconnect();
};
- }, [groupName]);
+ }, [observerRef]);
const register = useCallback(
(chart: EChartsType) => {
const dom = chart.getDom();
if (!dom) {
- return;
+ return () => {};
}
chartsRef.current.set(dom, chart);
- observerRef.current?.observe(dom);
+ observerRef.observe(dom);
// Set the group immediately for charts that may already be visible
chart.group = groupName;
echarts?.connect(groupName);
+
+ // Return unregister function for cleanup (fixes Bug 2)
+ return () => {
+ chartsRef.current.delete(dom);
+ observerRef.unobserve(dom);
+ };
},
- [groupName]
+ [groupName, observerRef]
);
return (
@@ -89,7 +107,7 @@
if (!context) {
// The provider was not registered, return a dummy function
return {
- register: (_p: any) => null,
+ register: (_p: any) => () => {},
groupName: '',
};
}
diff --git a/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx b/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx
--- a/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx
+++ b/static/app/views/dashboards/widgets/timeSeriesWidget/timeSeriesWidgetVisualization.tsx
@@ -1,4 +1,4 @@
-import {useCallback, useMemo, useRef, useState} from 'react';
+import {useCallback, useEffect, useMemo, useRef, useState} from 'react';
import {useTheme} from '@emotion/react';
import {mergeRefs} from '@react-aria/utils';
import * as Sentry from '@sentry/react';
@@ -137,6 +137,7 @@
const chartRef = useRef<ReactEchartsRef | null>(null);
const {register: registerWithWidgetSyncContext, groupName} = useWidgetSyncContext();
+ const unregisterRef = useRef<(() => void) | null>(null);
const pageFilters = usePageFilters();
const {start, end, period, utc} =
@@ -445,11 +446,18 @@
const handleChartReady = useCallback(
(instance: echarts.ECharts) => {
onChartReadyZoom(instance);
- registerWithWidgetSyncContext(instance);
+ unregisterRef.current = registerWithWidgetSyncContext(instance);
},
[onChartReadyZoom, registerWithWidgetSyncContext]
);
+ // Cleanup: unregister chart on unmount
+ useEffect(() => {
+ return () => {
+ unregisterRef.current?.();
+ };
+ }, []);
+
const showXAxisProp = props.showXAxis ?? 'auto';
const showXAxis = showXAxisProp === 'auto';This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
|
Oh my God sorry, wrong PR! 🙇🏻 |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 4 total unresolved issues (including 3 from previous reviews).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
gggritso
left a comment
There was a problem hiding this comment.
Overall this LGreatTM it's a nice optimization! I agree with Cursor's comments about race conditions. I think it might be safer to:
- When the
IntersectionObserveris initialized, check if there's anything already inchartsRef, and immediately observe anything else. - When charts register themselves, they should check if the intersection observer exists. If not, they can add themselves to
chartRef, and the intersection observer will pick them up when it's initialized - Adding an
unregisterfunction and havingTimeSeriesWidgetVisualizationcall that on unmount. This is not directly related to your PR, but should be simple and worth it!
Aside: I'm not confident whether calling.observeon an element is idempotent, maybe worth checking!
Or something along those lines, anyway!
- lazily creates an intersection observer if one isn't created when registering - stabilizes the group name when initialized to prevent unnecessary clean up of the intersection observer - add an unregister function to clean up state after unmounting
|
Thanks for the review @gggritso! I think I addressed your comments through the following:
I dropped
Added! It'll clean up references from the intersection observer when unmounting. |
gggritso
left a comment
There was a problem hiding this comment.
Awesome! LGTM, I left one question but it's not blocking, just double-checking 👍


Uses an intersection observer to register echarts instances to groups when the charts are visible in the viewport. This way, we don't dispatch cursor sync calls to widgets that are not visible, bypassing some of the load echarts needs to do to properly place the crosshairs
This only impacts hover performance when a user has scrolled and triggered all of the widgets to load in, which we expect users to scroll on this page. I crudely profiled this interaction by initializing all of the widgets on the page and then hovering over and around a single timeseries chart to show a difference in magnitude over multiple events. This was done with a production build locally (i.e.
NODE_ENV=production pnpm dev-ui) and I consistently witnessed better performance with the new strategy.These screenshots following are filtered to
mousemoveeventsBefore:

After:
