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

[data grid] Grid crashes when switching from tree data to regular grid #15826

Open
Zitrooone opened this issue Dec 10, 2024 · 11 comments
Open

[data grid] Grid crashes when switching from tree data to regular grid #15826

Zitrooone opened this issue Dec 10, 2024 · 11 comments
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Tree data Related to the data grid Tree data feature plan: Pro Impact at least one Pro user support: pro standard Support request from a Pro standard plan user. https://mui.com/legal/technical-support-sla/

Comments

@Zitrooone
Copy link

Zitrooone commented Dec 10, 2024

Steps to reproduce

Steps:

  1. Open this link to live example: Link
  2. Select "Job title" as grouping in the select at the top
  3. Switch back to "None"
  4. Cannot read properties of undefined (reading 'depth')

Current behavior

The grid crashes when I "turn off" the treeData view.

Expected behavior

The grid does not crash and treeData is turned off, which results in a regular grid being displayed.

Context

This was working just fine with V6 of the DataGrid. I recently wanted to migrate to V7 and started encountering this issue. Before I crafted the example I encountered another error, which was Could not find row with ID #0. I can no longer replicate it though.

I tried to build an example that is as close as possible to what we are using. For some reason codesandbox has issues previewing the example Rendered fewer hooks than expected. This may be caused by an accidental early return statement.. Is this a problem with my setup or with codesandbox?

Your environment

npx @mui/envinfo
System:
    OS: Windows 11 10.0.22631
    @mui/private-theming:  6.1.9
    @mui/styled-engine:  6.1.9
    @mui/system:  6.1.9
    @mui/types:  7.2.19
    @mui/utils:  6.1.9
    @mui/x-data-grid:  7.23.0
    @mui/x-data-grid-pro: ^7.23.0 => 7.23.0
    @mui/x-date-pickers: ^7.23.0 => 7.23.0
    @mui/x-internals:  7.23.0
    @mui/x-license: ^7.23.0 => 7.23.0
    @types/react: ^18.3.12 => 18.3.12
    react: ^18.3.1 => 18.3.1
    react-dom: ^18.3.1 => 18.3.1
    typescript: ^5.7.2 => 5.7.2

Search keywords: treeData
Order ID: 102617

@Zitrooone Zitrooone added bug 🐛 Something doesn't work status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 10, 2024
@github-actions github-actions bot added component: data grid This is the name of the generic UI component, not the React module! support: pro standard Support request from a Pro standard plan user. https://mui.com/legal/technical-support-sla/ labels Dec 10, 2024
@arminmeh
Copy link
Contributor

Hey @Zitrooone thanks for reporting this.
I will look into it.

Regarding the Codesandbox, it is a known issue and we have opened an issue upstream.
Workaround has been merged and the issue should be gone with the next release.

More info here #15765

@arminmeh arminmeh added plan: Pro Impact at least one Pro user and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Dec 10, 2024
@arminmeh arminmeh changed the title [DataGridPro] V7 Grid crashes when switching from treeData to regular grid [data grid] Grid crashes when switching from tree data to regular grid Dec 10, 2024
@arminmeh
Copy link
Contributor

Is there a specific reason you are processing the rows the way you are in your example?

I found that this is the cause of the error
You can check updated example which uses the rows data (with hierarchy) directly and it works fine.

*I am also using the version of the grid that is not released yet but has the codesandbox issue resolved

@arminmeh arminmeh added status: waiting for author Issue with insufficient information and removed bug 🐛 Something doesn't work labels Dec 10, 2024
@Zitrooone
Copy link
Author

Hi @arminmeh, thanks for the heads up regarding the Codesandbox. And thank you for your example.

The reason we process the rows this way is that we have multiple groupings you could chose, which would dictate the way the hierarchy is built up. Essentially more cases in the getRows switch. Some groupings also have more depth than others. The original data is retained to be able to go back to a blank state, although I see that the hierarchy is ignored when not using treeData.

I was playing around with your suggestion, but kept stumbling across bits that make the current use of useMemo convenient.
We deploy many grids, all with different data structures and dependencies for the useMemo. Having a mostly unified way of keeping the rows in sync is very important. I will try to adapt your example to further showcase the situation.
While it's certainly possible, I would prefer to not rebuild all of those grids.

Any idea what causes the error in the first place?

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Dec 11, 2024
@Zitrooone
Copy link
Author

I tinkered with the example and made this, hoping it helps and does not cause more confusion. I will try to explain:
I've added two more columns and two more groupings, as well as a checkbox.
The checkbox will take effect when the grouping is set to "hobby". It will add all hobbies to the grid, even if no one is executing them. When unticking the checkbox I need to remove those rows, which can be done by either using the original data or filtering the current data with the key isHobbyOnly.
So now my dependencies are the grouping and the checkbox. I can add those to the useMemo along with any additional dependencies that might be added in the future. The other route I can think of would be to trigger a row update in every change handler of any of the dependencies. The latter would be easier to make mistakes with and less of an obvious pattern than the former, at least to me.

I hope this made sense. If not, I apologise! I will look into this topic further tomorrow with a fresh head.

PS: Knowing the cause of the error might help me figure out what to avoid, if it is indeed a fault in my architecture. I was considering it being some sort of out-of-sync data due to the useMemo, but I cannot make sense of it unfortunately.

@arminmeh
Copy link
Contributor

Thanks for all the info and additional example @Zitrooone

This is what I have observed last time

  • State is updated multiple times by changing the group value (without useMemo only once)
  • In one of those updates, there is a mismatch of the state parts which causes the error (accessing nonexistent record)
  • If a guarding condition is added to ignore this mismatch grid eventually ends up in the correct state.

I have to investigate why the mismatch happens. If it is on our side I can fix that, but you should still be aware that this setup re-renders grid multiple times.

Once I get more details I will get back to you.

@Zitrooone
Copy link
Author

Thanks for the feedback @arminmeh

Looking at the code I would think that isTree might be the bit that is not matching the data, but wrapping that in a useMemo does not appear to fix the issue either. Maybe it is time to give that sweet new compiler a chance 😄
Jokes aside, I will await your feedback. Thank you for your time.

@arminmeh
Copy link
Contributor

arminmeh commented Dec 12, 2024

Hi @Zitrooone

I have found the root cause of the error and found a way for your last example to work with the current data grid.

If you look at first warnings in Row definition and Column definition pages you can see that it is not expected to deal with rows and columns in the way you did.

But I think that the grid should still handle your case, only with the mentioned performance degradation.

Error is caused by the way the updates are being processed when both treeData and rows reference is updated.
I will discuss with the team how to adjust this, but it might take a bit longer until this is updated.

This is why I have updated the sandbox to keep the rows reference stable (when treeData is updated) and allow you to move forward.

Let me know if something else (not included in your last example) is blocking you to keep the reference stable.

@arminmeh arminmeh removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 12, 2024
@github-project-automation github-project-automation bot moved this to 🆕 Needs refinement in MUI X Data Grid Dec 13, 2024
@michelengelen michelengelen added the feature: Tree data Related to the data grid Tree data feature label Dec 13, 2024
@Zitrooone
Copy link
Author

Hi @arminmeh

Thank you for your update. I don't quite understand why the getRows was moved to be a useCallback. What I did notice when looking at your example is that you did not use structuredClone and instead modified gridData directly. I tried this in our code and it worked without further modifying the architecture. I see how this stabelises the reference for the rows, but in case we add the hobbies to the list, the reference would be different again? Surprisingly this does not cause issues.

Unfortunately my time to test is very limited, as I'm leaving for vacation very soon. I can do a deep dive and implement this fix in our various grids early next year. Let me know if you need anything from me.

@arminmeh
Copy link
Contributor

I don't quite understand why the getRows was moved to be a useCallback.

You can ignore this, I should have cleaned it up.
While I was doing some debugging I was moving things around. I have updated sandbox again to be aligned with your example.

I see how this stabelises the reference for the rows, but in case we add the hobbies to the list, the reference would be different again? Surprisingly this does not cause issues.

Issue only happens if treeData and rows are updated at the same time and if treeData is changing from true to false.
This is why it only happened while you were reverting to 'None'.
When you add all Hobbies, you are changing rows, but not treeData.

@arminmeh
Copy link
Contributor

Note for the team

The error happens because when both treeData and rows are updated, grid starts re-render and strategy processors availability is calculated again.

Problem is, for rowTreeCreation grid is not aware immediately that the strategy has changed.
It detects that the processor has changed for the tree-data strategy here
This triggers row and lookup recalculation, but with the new rows reference it clears rows cache

So, in this cycle it tries to build tree data lookups, but without any rows which crashes the grid.
If the rows reference is not updated, cache is used, which completes this cycle after which grid detects strategy change to default and does row creation again.

Fixing this problem will also get rid of this extra cycle which will help performance.

@cherniavskii
Copy link
Member

Another workaround for this issue is to change the key prop to reset the data grid state when switching to tree data and back: https://codesandbox.io/p/sandbox/datagrid-treedata-crash-forked-g6kdm4?file=%2Fsrc%2FDemo.tsx%3A135%2C1

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: Tree data Related to the data grid Tree data feature plan: Pro Impact at least one Pro user support: pro standard Support request from a Pro standard plan user. https://mui.com/legal/technical-support-sla/
Projects
Status: 🆕 Needs refinement
Development

No branches or pull requests

4 participants