[charts] Only update store if interaction item is different

[bernardobelchior]

Use fastObjectShallowCompare to compare the previous and next highlighted items to decide whether to update the store and call onHighlightChange.

At the moment, onHighlightChange is called even if the newItem hasn't changed, so this change fixes that behavior as well.

[mui-bot]

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

Generated by 🚫 dangerJS against 9332f68

[codspeed-hq]

CodSpeed Performance Report

Merging #17851 will not alter performance

_{Comparing bernardobelchior:improve-interaction-perf (9332f68) with master (d304ba9)}

Summary

✅ 9 untouched benchmarks

[bernardobelchior]

This should also prevent onHighlightChange from being called when the item would have been the same.

[alexfauquette]

I'm surprised this is needed. Most of the time, the setter get called from pointerEnter, pointerLeave, pointerDown which should always pass a different item.

The exception is the voronoi interaction but in the case I would be in favor of doing the check at the voronoid level.

We already save the index of the last point index to speed up computation (the lastFind ref). We could also save the content of the point.

This would be more coherent with other charts: onHighlight being an event trigger when a user enters the highlight area.

https://github.com/mui/mui-x/blob/master/packages/x-charts/src/internals/plugins/featurePlugins/useChartVoronoi/useChartVoronoi.ts/#L187-L188

[romgrk]

To me, this reads like a code smell that the store doesn't contain the right values, though I don't have enough context to expand on that.

fastObjectShallowCompare was originally written as an escape hatch for useSelector() and this kind of situation (usage: useSelector(store, selector, args, fastObjectShallowCompare)), but in the datagrid we refactored away such cases over time.

The approach here is more efficient than using fOSC at useSelector(), but imo something's not right.

[bernardobelchior]

I'm surprised this is needed. Most of the time, the setter get called from pointerEnter, pointerLeave, pointerDown which should always pass a different item.

Yeah, but the problem is in pointerMove.

The exception is the voronoi interaction but in the case I would be in favor of doing the check at the voronoid level.

We already save the index of the last point index to speed up computation (the lastFind ref). We could also save the content of the point.

Yeah, I think that works as well.

[bernardobelchior]

To me, this reads like a code smell that the store doesn't contain the right values, though I don't have enough context to expand on that.

We need to highlight a specific data point from a series. We could have some data that is "more raw", e.g., the pointer position, but we'd need still to compare it shallowly because it would have two dimensions.

The problem here is that the identifier of a data point is composed of the series ID and its index in that series.

We can create a composed value, i.e., concatenating the series ID with the data index to create a string, for example.

Something like the below should be enough:

const item = `${seriesId}-${dataIndex}`

But then we'd have to split the string again to obtain the data. Would that be worth the effort?

Also, if we want to add more info in the future it might not work well because we'd need to expand the data in this ID. What do you think, @alexfauquette @JCQuintas?

fastObjectShallowCompare was originally written as an escape hatch for useSelector() and this kind of situation (usage: useSelector(store, selector, args, fastObjectShallowCompare)), but in the datagrid we refactored away such cases over time.

Makes sense. How did you refactor such cases in the data grid? I'm wondering if those learnings would also apply here.

The approach here is more efficient than using fOSC at useSelector(), but imo something's not right.

Yeah, I chose this way because it's more efficient, but I also felt that it was a bit weird. I'm ok with moving this to the selector.

[JCQuintas]

We need to highlight a specific data point from a series. We could have some data that is "more raw", e.g., the pointer position, but we'd need still to compare it shallowly because it would have two dimensions.

I've had this idea for quite some time to make the highlighting logic more generic, but I'm not sure it would be more performant. store pointer location > derive which bars/lines below pointer based on location for highlight and click

I assume it could be better for fixing our long-standing issue with the line highlight/click though. But that would also require a voronoi-like solution I guess.

We can create a composed value, i.e., concatenating the series ID with the data index to create a string, for example.

I believe the highlight item is a known shape, and it is small, so we can simply check the values by themselves if we want. Eg prev.series !== new.series || prev.dataIndex !== new.dataIndex

[alexfauquette]

It could be better for fixing our long-standing issue with the line highlight/click though.

It will be necessary for the canvas implementation

[romgrk]

I'm ok with moving this to the selector.

No need, if you're going to use a hack, use the more efficient of the two. But the comment of Jose above is relevant, fOSC isn't really needed for this comparison.

[bernardobelchior]

I believe the highlight item is a known shape, and it is small, so we can simply check the values by themselves if we want. Eg prev.series !== new.series || prev.dataIndex !== new.dataIndex

We can, but I'm concerned that we add a new field to item and then this check will break, but there will be no TypeScript or lint error. That's why I suggested using the isDeepEqual function here as well.

Also, the item in setInteractionType isn't a simple interface, it's a mapped type that depends on the series. I think it's safer to use a shallow compare, to be honest.

Alternatively, we could get rid of these generic comparison functions if there was a way to say to TypeScript ESLint that we want to compare all fields, so that it would show an error if we added more properties without updating the comparison.

I'm not aware of any rule that does this, so we'd probably need to write it ourselves.

Do you have any suggestion on how to fix the problem of adding more fields to the object and forgetting to update the comparison?

[JCQuintas]

I'm ok with fsob here. I don't think we need it to be deep compared too, but either fsob or isDeepEqual is ok

[alexfauquette]

What about moving this logic at the root cause. At least you will do the comparison only once

--- a/packages/x-charts/src/internals/plugins/featurePlugins/useChartVoronoi/useChartVoronoi.ts
+++ b/packages/x-charts/src/internals/plugins/featurePlugins/useChartVoronoi/useChartVoronoi.ts
@@ -35,6 +35,7 @@ export const useChartVoronoi: ChartPlugin<UseChartVoronoiSignature> = ({
   const voronoiRef = React.useRef<Record<string, VoronoiSeries>>({});
   const delauneyRef = React.useRef<Delaunay<any> | undefined>(undefined);
   const lastFind = React.useRef<number | undefined>(undefined);
+  const lastFindPoint = React.useRef<{ seriesId: SeriesId; dataIndex: number } | null>(null);
 
   const defaultXAxisId = xAxisIds[0];
   const defaultYAxisId = yAxisIds[0];
@@ -169,6 +170,12 @@ export const useChartVoronoi: ChartPlugin<UseChartVoronoiSignature> = ({
 
     const handleMouseMove = (event: MouseEvent) => {
       const closestPoint = getClosestPoint(event);
+      const newClosestPoint = typeof closestPoint === 'string' ? null : closestPoint;
+
+      if (newClosestPoint === lastFindPoint.current || fastObjectShallowCompare(newClosestPoint, lastFindPoint.current)) {
+        // ignore pointer move if the result stay the same
+        return;
+      }
+      lastFindPoint.current = newClosestPoint;
 
       if (closestPoint === 'outside-chart') {

[bernardobelchior]

What about moving this logic at the root cause. At least you will do the comparison only once

--- a/packages/x-charts/src/internals/plugins/featurePlugins/useChartVoronoi/useChartVoronoi.ts
+++ b/packages/x-charts/src/internals/plugins/featurePlugins/useChartVoronoi/useChartVoronoi.ts
@@ -35,6 +35,7 @@ export const useChartVoronoi: ChartPlugin<UseChartVoronoiSignature> = ({
   const voronoiRef = React.useRef<Record<string, VoronoiSeries>>({});
   const delauneyRef = React.useRef<Delaunay<any> | undefined>(undefined);
   const lastFind = React.useRef<number | undefined>(undefined);
+  const lastFindPoint = React.useRef<{ seriesId: SeriesId; dataIndex: number } | null>(null);
 
   const defaultXAxisId = xAxisIds[0];
   const defaultYAxisId = yAxisIds[0];
@@ -169,6 +170,12 @@ export const useChartVoronoi: ChartPlugin<UseChartVoronoiSignature> = ({
 
     const handleMouseMove = (event: MouseEvent) => {
       const closestPoint = getClosestPoint(event);
+      const newClosestPoint = typeof closestPoint === 'string' ? null : closestPoint;
+
+      if (newClosestPoint === lastFindPoint.current || fastObjectShallowCompare(newClosestPoint, lastFindPoint.current)) {
+        // ignore pointer move if the result stay the same
+        return;
+      }
+      lastFindPoint.current = newClosestPoint;
 
       if (closestPoint === 'outside-chart') {

I think that works as long as this is the only place where the highlight is set. The benefit of checking against the store value is that we're sure that no other code path has changed the highlight value.

From what I'm seeing there's no overlap. For example, the highlight and item interaction are only set from the voronoi plugin in the scatter chart and from the useInteractionItemProps hook in other charts. However, if we set the highlight from more than one place, this will create a bug that will be hard to track down.

Plus, it requires checking this on every caller, i.e., in useChartVoronoi, useInteractionItemProps and useInteractionAllItemProps.

Additionally, if the component moves from controlled highlighting to uncontrolled, then we might have a bug as well, but I don't think that will happen often.

In summary, moving the check to the caller of setHighlight:

• Code duplication for checking whether the highlighted item has changed;
• Potential bug if, in the future, we have interleaved calls of setHighlight from different callers.

Given those reasons, do you think it's better to move this logic to all the hooks setting the highlight?

I started implementing the suggested change, but with those things in mind I'm having a hard time justifying the change 🤔

[alexfauquette]

Plus, it requires checking this on every caller, i.e., in useChartVoronoi, useInteractionItemProps and useInteractionAllItemProps.

I don't see why you would need it for useInteractionItemProps or useInteractionAllItemProps. They are using pointerEnter/pointerLeave and I guess browsers only call those events once. So the Voronoï is the only remaining one.

For me, this Voronoï hook is the first brick of a hit-box pipeline that get coordinate and dispatch events about the data

1. You provide pointer coordinate (x, y) to the hook
2. The hook looks in the data and the configuration
3. It triggers some onEnter(dataItem) onLeave(dataItem) (For now we don't use this abstraction, we directly call setHighlight, setInteraction)

When moving to canvas we will have to add one for bars, lines, areas, pie-slices, ... because we will not benefit from the browser event handler.

But that's for later

[bernardobelchior]

Tried using the pointerenter event, but then we get a flash of the tooltip when pressing a point.

ScreenRecording_05-22-2025.15-47-11_1.MP4

Merging this PR as is, but let's keep discussing and I'll refactor this if we find a better solution