Skip to content
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.

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

[DataGridPremium] Fix expanded groups being collapsed after calling updateRows #8823

Merged
merged 3 commits into from
May 4, 2023

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented May 2, 2023

Fixes #8580
Before: https://codesandbox.io/s/kind-lamarr-ofjvim
After: https://codesandbox.io/s/wild-http-h2gfjq

Fixes #8853
Before: https://codesandbox.io/s/cocky-sunset-f7qkjg
After: https://codesandbox.io/s/frosty-bassi-1hwexf


The issue comes down to this part where we update the row tree:

const { id, path } = params.nodes.modified[i];
const pathInPreviousTree = getNodePathInTree({ tree, id });
const isInSameGroup = isDeepEqual(pathInPreviousTree, path);

Here we compare path and pathInPreviousTree. But isInSameGroup always equals false, even if the groups have not been changed. Here are example values that are being compared:

[{ field: undefined, key: "1" }, { field: 14, key: 14 }] // pathInPreviousTree
[{ key: 14, field: "age" }, { key: "1", field: null }] // path

There are two differences between pathInPreviousTree and path:

  1. In path, the null value is used when the field is missing, but in pathInPreviousTree, undefined is used instead.
    This issue comes from a bug in getNodePathInTree
    field: (node as GridGroupNode).groupingField,

    Leaf nodes do not have groupingField property, therefore field should be set to null here. TS didn't warn about this because of the as GridGroupNode assertion.
    -field: (node as GridGroupNode).groupingField,
    +field: node.type === 'leaf' ? null : node.groupingField,
  2. The order of items in the array is different, therefore deepEqual returns false.
    The order comes from getNodePathInTree as well, where Array.push is being used:

    The fix is to use Array.unshift to reverse the order of items.

TODO:

  • Explain the changes

@cherniavskii cherniavskii added component: data grid This is the name of the generic UI component, not the React module! regression A bug, but worse plan: Premium Impact at least one Premium user feature: Row grouping Related to the data grid Row grouping feature labels May 2, 2023
@mui-bot
Copy link

mui-bot commented May 2, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8823--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 647 1,160.3 1,160.3 930.7 206.104
Sort 100k rows ms 735.5 1,325.1 1,284.1 1,103.8 210.556
Select 100k rows ms 278.8 570.1 310.2 372.46 106.109
Deselect 100k rows ms 241.1 385.9 294.2 297.6 53.407

Generated by 🚫 dangerJS against 7838728

@cherniavskii cherniavskii marked this pull request as ready for review May 2, 2023 16:06
@@ -34,8 +34,8 @@ export const getNodePathInTree = ({
let node = tree[id] as GridGroupNode | GridLeafNode;

while (node.id !== GRID_ROOT_GROUP_ID) {
path.push({
field: (node as GridGroupNode).groupingField,
path.unshift({
Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, I'm going to keep .push and just reverse the array later, because it looks like unshift is ~70% slower even for short arrays: https://jsperf.app/wisoqe

@cherniavskii
Copy link
Member Author

Copy link
Member

@MBilalShafi MBilalShafi left a comment

Choose a reason for hiding this comment

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

Thank you, it is a nice stability improvement. 🙏

@@ -2634,4 +2634,24 @@ describe('<DataGridPremium /> - Row Grouping', () => {

await waitFor(() => expect(getCell(1, 3).textContent).to.equal('username 2'));
});

// See https://github.com/mui/mui-x/issues/8580
Copy link
Member

Choose a reason for hiding this comment

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

Optional Nice to have (possibly in a follow-up): It'd be nice to have a small test case covering #8853 too

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I'll add a regression test in a follow-up PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in #8870

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Row grouping Related to the data grid Row grouping feature plan: Premium Impact at least one Premium user regression A bug, but worse
Projects
None yet
3 participants