[charts-pro] Fix zoom pan issue when controlled #20163

JCQuintas · 2025-10-31T13:48:25Z

Main goal is to remove this line startRef.current = store.state.zoom.zoomData; which prevented other interactions to run alongside the pan interaction, since the pan interaction would always "reset" any other changes.

This is mostly an internal change

You can test this in the http://localhost:3001/x/react-charts/zoom-and-pan/#zoom-synchronization
by reverting the changes in the file: packages/x-charts-pro/src/internals/plugins/useChartProZoom/useChartProZoom.ts

JCQuintas · 2025-10-31T13:49:03Z

packages/x-charts-pro/src/internals/plugins/useChartProZoom/gestureHooks/usePanOnDrag.ts

+      accumulatedChange.current.x += event.detail.deltaX;
+      accumulatedChange.current.y += event.detail.deltaY;


We store the accumulated changes outside the raf throttle

JCQuintas · 2025-10-31T13:49:21Z

packages/x-charts-pro/src/internals/plugins/useChartProZoom/gestureHooks/usePanOnDrag.ts

+      const x = accumulatedChange.current.x;
+      const y = accumulatedChange.current.y;
+      accumulatedChange.current.x = 0;
+      accumulatedChange.current.y = 0;


we consume the delta changes when the throttle runs

mui-bot · 2025-10-31T13:50:21Z

Deploy preview: https://deploy-preview-20163--material-ui-x.netlify.app/

Bundle size report

Bundle	Parsed size	Gzip size
@mui/x-data-grid	0B^(0.00%)	0B^(0.00%)
@mui/x-data-grid-pro	0B^(0.00%)	0B^(0.00%)
@mui/x-data-grid-premium	0B^(0.00%)	0B^(0.00%)
@mui/x-charts	▼-42B^(-0.01%)	▼-4B^(0.00%)
@mui/x-charts-pro	▼-37B^(-0.01%)	🔺+55B^(+0.04%)
@mui/x-charts-premium	▼-33B^(-0.01%)	🔺+47B^(+0.04%)
@mui/x-date-pickers	0B^(0.00%)	0B^(0.00%)
@mui/x-date-pickers-pro	0B^(0.00%)	0B^(0.00%)
@mui/x-tree-view	0B^(0.00%)	0B^(0.00%)
@mui/x-tree-view-pro	0B^(0.00%)	0B^(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against fdfe812

JCQuintas · 2025-10-31T13:50:27Z

packages/x-charts-pro/src/internals/plugins/useChartProZoom/useChartProZoom.ts

+        onZoomChange?.(newZoomData);
+        store.set('zoom', {
+          ...store.state.zoom,
+          isInteracting: true,
+        });


we now set isInteracting: true when we send the new zoom to the onZoomChange, this would lead to small de-sync in data over time due to animations turning on/off

JCQuintas · 2025-10-31T13:51:01Z

packages/x-internals/src/rafThrottle/rafThrottle.test.ts

+import { expect } from 'vitest';
+import { rafThrottle } from './rafThrottle';
+
+describe('rafThrottle', () => {


extra tests just in case 🤷

codspeed-hq · 2025-10-31T14:01:03Z

CodSpeed Performance Report

Merging #20163 will not alter performance

_{Comparing JCQuintas:fix-zoom-lag (fdfe812) with master (f775e82)¹}

Summary

✅ 13 untouched

No successful run was found on master (0741821) during the generation of this report, so f775e82 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

…-lag

github-actions · 2025-10-31T15:08:33Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

bernardobelchior · 2025-10-31T16:01:18Z

packages/x-charts-pro/src/internals/plugins/useChartProZoom/gestureHooks/usePanOnDrag.ts

      if (!(event.detail.target as SVGElement)?.closest('[data-charts-zoom-slider]')) {
-        startRef.current = store.state.zoom.zoomData;
+        isInteracting.current = true;
+        accumulatedChange.current = { x: 0, y: 0 };


Couldn't we lose some updates?

Since rafThrottle will delay the reading of accumulatedChange, if another handlePanStart is called before the callback of throttledCallback fires, the callback will fire with accumulatedChange.current = { x: 0, y: 0 }.

To fix it, I think we need to pass accumulatedChange.current to the throttledCallback so that it retains the reference to the old value. (It works because we're updating accumulatedChange.current by mutating x and y. If we were updating the object directly (accumulatedChange.current = { x, y } it wouldn't work.

Couldn't we lose some updates?

In a real environment it would be very unlikely to loose any update at all.

I've changed it to initialize/change inside the hook.

bernardobelchior · 2025-10-31T16:05:10Z

packages/x-charts-pro/src/internals/plugins/useChartProZoom/gestureHooks/usePanOnDrag.ts

  const optionsLookup = useSelector(store, selectorChartZoomOptionsLookup);
-  const startRef = React.useRef<readonly ZoomData[]>(null);
+  const isInteracting = React.useRef<boolean>(false);
+  const accumulatedChange = React.useRef<{ x: number; y: number }>({ x: 0, y: 0 });


I don't think these needs to be refs. Since they're only used inside the useEffect we can create const variable with interior mutability to achieve the same effect.

Basically, we create these variables inside the useEffect similar to a ref: const accumulatedChange = { current: { x: 0, y: 0 }}.

It would also allow us to unit test this logic, which I think we should because it's prone to errors as explained here.

packages/x-internals/src/rafThrottle/rafThrottle.test.ts

…-lag

bernardobelchior · 2025-11-03T14:07:04Z

packages/x-charts-pro/src/internals/plugins/useChartProZoom/gestureHooks/usePanOnDrag.ts

+      const x = accumulatedChange.x;
+      const y = accumulatedChange.y;
+      accumulatedChange.x = 0;
+      accumulatedChange.y = 0;


I think this works, but I'm not sure it's very logical.

IMO, it would be make more sense for throttledCallback to accept the accumulatedChange object and for handlePanStart to create a new { x: 0, y: 0} object when it's fired.

We don't need to reset values inside the throttled function.

The throttle would still fail to always have the correct ref, since older values are dropped.

current:

// - start // - move, update accumulate + scheduleThrottledFn // - move, update accumulate + dropOldThrottledFn + scheduleThrottledFn // - start (values keep accumulating) // - move, update accumulate + dropOldThrottledFn + scheduleThrottledFn // - run scheduled fn + reset accumulate

your suggestion:

// - start, new accumulate (ref1) // - move, update accumulate + scheduleThrottledFn(ref1) // - move, update accumulate + dropOldThrottledFn(ref1) + scheduleThrottledFn(ref1) // - start, new accumulate (ref2) // \/ (ref1) could be completely discarded // - move, update accumulate + dropOldThrottledFn(ref1) + scheduleThrottledFn(ref2) // - run scheduled fn(ref2)

You're right. Your solution looks good, then 👍

[charts-pro] Fix issue zoom pan controlled issue

1336d5a

JCQuintas self-assigned this Oct 31, 2025

JCQuintas requested a review from alexfauquette as a code owner October 31, 2025 13:48

JCQuintas added the type: bug It doesn't behave as expected. label Oct 31, 2025

JCQuintas requested a review from bernardobelchior as a code owner October 31, 2025 13:48

JCQuintas added plan: Pro Impact at least one Pro user. scope: charts Changes related to the charts. labels Oct 31, 2025

JCQuintas commented Oct 31, 2025

View reviewed changes

JCQuintas added 3 commits October 31, 2025 15:09

for

68f7d28

fix test

bde3f5e

Merge commit 'f775e829431b6c623f0bb3afcb52f47feba2144c' into fix-zoom…

de13dd5

…-lag

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 31, 2025

JCQuintas added 2 commits October 31, 2025 16:10

changes

5479548

reset accumulated change on start rather than end

1d8adcc

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Oct 31, 2025

bernardobelchior reviewed Oct 31, 2025

View reviewed changes

JCQuintas added 2 commits November 3, 2025 11:35

Merge commit '07418215c9d7fd2ffac91b99f4cb2c8102db6a68' into fix-zoom…

9062757

…-lag

suggestions

fdfe812

bernardobelchior reviewed Nov 3, 2025

View reviewed changes

bernardobelchior approved these changes Nov 3, 2025

View reviewed changes

JCQuintas merged commit da98471 into mui:master Nov 3, 2025
22 of 23 checks passed

JCQuintas deleted the fix-zoom-lag branch November 3, 2025 15:54

		accumulatedChange.current.x += event.detail.deltaX;
		accumulatedChange.current.y += event.detail.deltaY;

Uh oh!

[charts-pro] Fix zoom pan issue when controlled #20163

[charts-pro] Fix zoom pan issue when controlled #20163

Uh oh!

Conversation

JCQuintas commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mui-bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle size report

Uh oh!

JCQuintas Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codspeed-hq bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #20163 will not alter performance

Summary

Footnotes

Uh oh!

github-actions bot commented Oct 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JCQuintas commented Oct 31, 2025 •

edited

Loading

mui-bot commented Oct 31, 2025 •

edited

Loading

JCQuintas Oct 31, 2025 •

edited

Loading

codspeed-hq bot commented Oct 31, 2025 •

edited

Loading