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

[DataGrid] Fix error caused by trying to render rows that are not in the state anymore #17057

Merged
merged 2 commits into from
Mar 24, 2025

Conversation

arminmeh
Copy link
Contributor

Fixes #16638 (still have to verify since there is no reproduction code)
Fixes #17022

Removes the need for #16672

The root cause for this is dealing with state from different update cycles.
Rows are rendered from getRows which uses rows data obtained through useGridSelector here.

Once those rows are rendered, they use getRowParam API which throws if the row is not there, but it gets row information from the state directly.

If the updates are too fast, it can happen that the rendering lags behind and tries to get params for the row that is not there anymore.

Worth mentioning is that I was able to reproduce this only on React 18, which might explain this.

Using selector directly here solves the linked issues, but introduces a lot of other issue where we need reactivity.

So, my solution is to check the row tree once more before setting a row to be rendered. Instead of doing this in the row component I have moved it up in the chain to prevent all other unnecessary row processing.

@arminmeh arminmeh added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Rendering layout Related to the data grid Rendering engine needs cherry-pick The PR should be cherry-picked to master after merge v7.x labels Mar 20, 2025
@arminmeh arminmeh requested a review from a team March 20, 2025 15:45
Copy link

Thanks for adding a type label to the PR! 👍

@@ -98,7 +98,6 @@ export function useGridColumns(

apiRef.current.setState(mergeColumnsState(columnsState));
apiRef.current.publishEvent('columnsChange', columnsState.orderedFields);
apiRef.current.updateRenderContext?.();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@arminmeh arminmeh changed the title [DataGrid] Fix error while trying to render rows that are not in the state anymore [DataGrid] Fix error caused by trying to render rows that are not in the state anymore Mar 20, 2025
@mui-bot
Copy link

mui-bot commented Mar 20, 2025

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

Generated by 🚫 dangerJS against 997fa5b

@cherniavskii cherniavskii self-requested a review March 24, 2025 14:24
@@ -125,7 +128,7 @@ const GridRow = forwardRef<HTMLDivElement, GridRowProps>(function GridRow(props,
rowReordering,
);
const handleRef = useForkRef(ref, refProp);
const rowNode = apiRef.current.getRowNode(rowId);
const rowNode = gridRowNodeSelector(apiRef, rowId);
Copy link
Member

Choose a reason for hiding this comment

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

Is this change here just for the non-nullable return type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that and also because getRowNode is deprecated

@arminmeh arminmeh merged commit 372991b into mui:master Mar 24, 2025
19 checks passed
Copy link

Cherry-pick PRs will be created targeting branches: v7.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Rendering layout Related to the data grid Rendering engine needs cherry-pick The PR should be cherry-picked to master after merge v7.x
Projects
None yet
3 participants