Skip to content

[DataGridPro] Fix duplicate nested rows for dynamically updated row IDs #18526

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

Jump to bottom
Merged
merged 16 commits into from
Jul 30, 2025
Merged

[DataGridPro] Fix duplicate nested rows for dynamically updated row IDs #18526

merged 16 commits into from
Jul 30, 2025

Conversation

MBilalShafi
Copy link
Member

Reported in https://mui.zendesk.com/agent/tickets/28655

Reproduction Tree Data: https://stackblitz.com/edit/vwnrw8kg?file=src%2FDemo.tsx
Reproduction Row Grouping: https://stackblitz.com/edit/cqu1xgfv?file=src%2FDemo.tsx

For the nested Data Source rows (tree data and row grouping), if the IDs of rows are constantly updating the Data Grid treats them as new rows and doesn't remove the previous ones. This PR aims to fix that.

@MBilalShafi MBilalShafi added type: bug It doesn't behave as expected. scope: data grid Changes related to the data grid. plan: Pro Impact at least one Pro user. feature: Tree data Related to the data grid Tree data feature feature: Row grouping Related to the data grid Row grouping feature feature: Server integration Better integration with backends, e.g. data source labels Jun 25, 2025
@mui-bot
Copy link

mui-bot commented Jun 25, 2025 •
edited
Loading

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

Bundle size report

Bundle Parsed Size Gzip Size
@mui/x-data-grid 0B(0.00%) 0B(0.00%)
@mui/x-data-grid/DataGrid 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-pro 🔺+648B(+0.14%) 🔺+140B(+0.10%)
@mui/x-data-grid-pro/DataGridPro 🔺+650B(+0.14%) 🔺+144B(+0.11%)
@mui/x-data-grid-premium 🔺+559B(+0.10%) 🔺+121B(+0.07%)
@mui/x-data-grid-premium/DataGridPremium 🔺+555B(+0.10%) 🔺+108B(+0.07%)
@mui/x-charts 0B(0.00%) 0B(0.00%)
@mui/x-charts-pro 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers-pro 0B(0.00%) 0B(0.00%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against 0fcc04c

@MBilalShafi MBilalShafi marked this pull request as ready for review June 25, 2025 15:20
@MBilalShafi MBilalShafi requested a review from a team June 25, 2025 15:20
romgrk
romgrk reviewed Jun 25, 2025
View reviewed changes
cherniavskii
cherniavskii reviewed Jun 26, 2025
View reviewed changes
MBilalShafi and others added 2 commits June 28, 2025 01:16
@MBilalShafi @cherniavskii
Use concat
5e7502b 
Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
Signed-off-by: Bilal Shafi <bilalshafidev@gmail.com>
@MBilalShafi
Prettier
1063c36
@MBilalShafi MBilalShafi requested a review from cherniavskii July 14, 2025 16:42
@MBilalShafi MBilalShafi requested a review from arminmeh July 25, 2025 12:12
@arminmeh
Copy link
Contributor

Why can't we handle this within the data source logic in a similar way lazy loading is done?
Whenever children are collapsed you can remove those rows from the grid and on a next expand they could either be fetched from the cache or from a new request

@cherniavskii
Copy link
Member

Whenever children are collapsed you can remove those rows from the grid and on a next expand they could either be fetched from the cache or from a new request

That's a good point. Combined with a null cache, this should solve the issue for this scenario.
For other scenarios, it will unnecessarily manipulate the row tree, but I think it's OK.

@MBilalShafi
Copy link
Member Author

MBilalShafi commented Jul 29, 2025 •
edited
Loading

Why can't we handle this within the data source logic in a similar way lazy loading is done? Whenever children are collapsed you can remove those rows from the grid and on a next expand they could either be fetched from the cache or from a new request

This approach looks cleaner to me too. Thanks for the suggestion @arminmeh.

@arminmeh
Copy link
Contributor

I see an error on a second open of a group in the first demo on
https://deploy-preview-18526--material-ui-x.netlify.app/x/react-data-grid/server-side-data/row-grouping/

Cannot read properties of undefined (reading 'id')

arminmeh
arminmeh reviewed Jul 30, 2025
View reviewed changes
@MBilalShafi MBilalShafi enabled auto-merge (squash) July 30, 2025 15:13
@MBilalShafi MBilalShafi merged commit a1c7026 into mui:master Jul 30, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@romgrk romgrk romgrk left review comments

@arminmeh arminmeh arminmeh approved these changes

@cherniavskii cherniavskii Awaiting requested review from cherniavskii

Assignees
No one assigned
Labels
feature: Row grouping Related to the data grid Row grouping feature feature: Server integration Better integration with backends, e.g. data source feature: Tree data Related to the data grid Tree data feature plan: Pro Impact at least one Pro user. scope: data grid Changes related to the data grid. type: bug It doesn't behave as expected.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MBilalShafi @mui-bot @arminmeh @cherniavskii @romgrk