Skip to content

[charts] Fix selector behavior affecting datagrid intergration #20083

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
JCQuintas wants to merge 3 commits into from
Closed

[charts] Fix selector behavior affecting datagrid intergration #20083

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
48fbb68
simplify md
JCQuintas Oct 24, 2025
e4edca0
try fix
JCQuintas Oct 24, 2025
3c73437
Revert "simplify md"
JCQuintas Oct 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions .../plugins/featurePlugins/useChartCartesianAxis/useChartCartesianAxisRendering.selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,11 +328,6 @@ export const selectorChartFilteredXDomains = createSelector(

return filteredDomains;
},
{
memoizeOptions: {
resultEqualityCheck: (a: any, b: any) => isDeepEqual(a, b),
},
},
);

export const selectorChartFilteredYDomains = createSelector(
Expand Down Expand Up @@ -398,11 +393,6 @@ export const selectorChartFilteredYDomains = createSelector(

return filteredDomains;
},
{
memoizeOptions: {
resultEqualityCheck: (a: any, b: any) => isDeepEqual(a, b),
},
},
);

export const selectorChartNormalizedXScales = createSelector(
Expand All @@ -419,6 +409,11 @@ export const selectorChartNormalizedXScales = createSelector(

return scales;
},
{
memoizeOptions: {
equalityCheck: isDeepEqual,
Copy link
Member

@bernardobelchior bernardobelchior Oct 24, 2025

Choose a reason for hiding this comment

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

I think this only works because getNormalizedAxisScale always returns a new scale, making this isDeepEqual unnecessary because it will always return false.

The reason for isDeepEqual in the normalized domain was to avoid recomputing scale selectors if the domain didn't change, so I think this is negating the performance improvements from #19790 since we'll always be re-creating the Flatbush data structure.

Copy link
Member Author

@JCQuintas JCQuintas Oct 24, 2025

Choose a reason for hiding this comment

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

There is resultEqualityCheck and equalityCheck. This is checking the parameters, not the result. The in the original, the selectorChartFilteredXDomains always re-runs regardless of value, then it checks if the result is equal and return it if they are different.

This is kinda useless, since in selectorChartNormalizedXScales, which is the only place selectorChartFilteredXDomains is used, we depend on selectorChartRawXAxis which is pretty much always different.

Our options are to either memoize the parameters of selectorChartNormalizedXScales, at the cost of traversing and checking the scales, or not memoize at all at the cost of re-render.

The general cause seems to be that @arminmeh mentioned in the body of the issue #20082

But I fail to understand why exactly it happens.

It seems that due to the memoization, the incoming xAxis[].data gets desynced from the actual data used in the scaleBand(domain), where the original arrays change, but their content doesn't.

I believe the solution proposed here solves that by syncing the result of the nomalizer selector to retrigger downstream selectors whenever the deep equal of its inputs changes, at the cost of having to deep traverse the axes every time.

Copy link
Member

@bernardobelchior bernardobelchior Oct 24, 2025

Choose a reason for hiding this comment

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

This is checking the parameters, not the result

I didn't realize that, thanks for calling it out. I wonder if it'll end up being more expensive, though, as checking an object of size-2 arrays is probably cheaper than doing a deep equal of all the arguments. Especially since selectorChartRawXAxis can contain the data array in line and bar charts, which can be a large array.

This is kinda useless, since in selectorChartNormalizedXScales, which is the only place selectorChartFilteredXDomains is used, we depend on selectorChartRawXAxis which is pretty much always different.

selectorChartFilteredXDomains indirectly depends on selectorChartZoomMap. The use case for memoizing the result of the filtered domain was to avoid re-creating the normalized scales when the zoom map changed but didn't cause the domains to change. In that case, we don't want to re-create the scales because it'll cause the flatbush to be re-indexed which is somewhat expensive.

When the axis changes, it'll recompute the world, it's true. We need additional selector work to fix that.

The general cause seems to be that @arminmeh mentioned in the body of the issue #20082

But I fail to understand why exactly it happens.

It seems the root cause isn't entirely known. I'll spend some time investigating this to understand the issue

Copy link
Member

@bernardobelchior bernardobelchior Oct 24, 2025

Choose a reason for hiding this comment

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

I think I found the issue and why this fix works.

It seems that the line @arminmeh mentioned (xScale(baseValue)) finds the baseValue within the xScale.domain().

d3's ordinal scale uses internmap to find compare non-primitives, but it relies on valueOf(). For dates, this works because its valueOf() is a unix timestamp, which is a primitive that can be compared. For arrays, however, this doesn't work (['a','b'].valueOf() === ['a','b'].valueOf() returns false).

Our data is an array of arrays because we're using grouping. The initial render works fine because they're the same reference. However, due to domain memoization, whenever a re-render happens, the arrays in data will be different from the arrays in the scale's domains, causing xScale(baseValue) to return an invalid value.

The change from this PR fixes the issue by breaking memoization whenever the axis changes, which causes the reference to be the same, but to be honest this doesn't address the root cause.

To me, the issue at the root of this problem is how d3 is comparing values when they aren't primitives. IMO, that's what we need to fix.

You can reproduce my assertion by adding the following code here:

if (dataIndex === 0) {
  console.log(xScale.domain()[0], baseValue, xScale.domain()[0] === baseValue);
}

Copy link
Contributor

@arminmeh arminmeh Oct 27, 2025

Choose a reason for hiding this comment

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

One more piece of information that might help
When I was debugging this, I noticed that the data from useBarPlotData is calculated twice when I do the change that breaks the chart.
First time the data seems to be correct, but the second time the x value becomes NaN (reasons mentioned above)

},
},
);

export const selectorChartNormalizedYScales = createSelector(
Expand All @@ -435,6 +430,11 @@ export const selectorChartNormalizedYScales = createSelector(

return scales;
},
{
memoizeOptions: {
equalityCheck: isDeepEqual,
},
},
);

export const selectorChartXScales = createSelector(
Expand Down
Loading