Skip to content

fix(dashboards): Only trigger echarts dispatch sync for visible widgets#110683

Merged
narsaynorath merged 3 commits intomasterfrom
nar/fix/dashboards-widget-sync-only-charts-in-view
Mar 16, 2026
Merged

fix(dashboards): Only trigger echarts dispatch sync for visible widgets#110683
narsaynorath merged 3 commits intomasterfrom
nar/fix/dashboards-widget-sync-only-charts-in-view

Conversation

@narsaynorath
Copy link
Copy Markdown
Member

@narsaynorath narsaynorath commented Mar 13, 2026

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 mousemove events

Before:
Screenshot 2026-03-13 at 4 39 21 PM

After:
Screenshot 2026-03-13 at 4 39 02 PM

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
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Mar 13, 2026
@narsaynorath narsaynorath marked this pull request as ready for review March 13, 2026 21:04
@narsaynorath narsaynorath requested a review from a team as a code owner March 13, 2026 21:04
@narsaynorath
Copy link
Copy Markdown
Member Author

@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

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Create PR

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.

@gggritso gggritso closed this Mar 15, 2026
@gggritso gggritso reopened this Mar 15, 2026
@gggritso
Copy link
Copy Markdown
Member

Oh my God sorry, wrong PR! 🙇🏻

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 4 total unresolved issues (including 3 from previous reviews).

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Copy link
Copy Markdown
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this LGreatTM it's a nice optimization! I agree with Cursor's comments about race conditions. I think it might be safer to:

  1. When the IntersectionObserver is initialized, check if there's anything already in chartsRef, and immediately observe anything else.
  2. 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
  3. Adding an unregister function and having TimeSeriesWidgetVisualization call 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 .observe on 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
@narsaynorath
Copy link
Copy Markdown
Member Author

Thanks for the review @gggritso! I think I addressed your comments through the following:

  1. When the IntersectionObserver is initialized, check if there's anything already in chartsRef, and immediately observe anything else.
  2. 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

I dropped chartsRef because there's an API echarts has that can do the reverse lookup, so we don't need to keep track of it manually. I also added a helper that inits the IntersectionObserver on the fly and store it in a ref if it doesn't already exist, which should get rid of the race condition

  1. Adding an unregister function and having TimeSeriesWidgetVisualization call that on unmount.

Added! It'll clean up references from the intersection observer when unmounting.

@narsaynorath narsaynorath requested a review from gggritso March 16, 2026 15:10
Copy link
Copy Markdown
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! LGTM, I left one question but it's not blocking, just double-checking 👍

@narsaynorath narsaynorath merged commit 2ca4ada into master Mar 16, 2026
63 checks passed
@narsaynorath narsaynorath deleted the nar/fix/dashboards-widget-sync-only-charts-in-view branch March 16, 2026 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants