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] RowIndexes are not stable, breaking memoization #10373

Closed
2 tasks done
lauri865 opened this issue Sep 17, 2023 · 2 comments · Fixed by #10674
Closed
2 tasks done

[Datagrid] RowIndexes are not stable, breaking memoization #10373

lauri865 opened this issue Sep 17, 2023 · 2 comments · Fixed by #10674
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module!

Comments

@lauri865
Copy link
Contributor

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 🕹

Link to live example:

Steps:

  1. https://mui.com/x/react-data-grid/virtualization/
  2. Focus first cell
  3. Open inspector and scroll down, and notice how id and rowIndex get out of sync, and rowIndex for a particular rowId changes while you scroll.

Current behavior 😯

When you scroll back up quickly, there's a whole bunch of rows that are forced to re-render, breaking memoization of them (essentially whatever hits the scrollable area gets reindex re-assigned on every single row being added back by virtulisation).

Caused by this PR: #7357

Expected behavior 🤔

RowIndexes should be kept intact regardless of whether the rows are kept in the DOM or not, the underlying data didn't change.

For inspiration, this is a good design for "sticky rows" virtualisation that doesn't manipulate indexes: https://tanstack.com/virtual/v3/docs/api/virtualizer#rangeextractor

Context 🔦

No response

Your environment 🌎

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Order ID or Support key 💳 (optional)

No response

@lauri865 lauri865 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 17, 2023
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Sep 18, 2023
@cherniavskii
Copy link
Member

No focused row in the DOM:

With focused row in the DOM:

Notice the offset between data-id and data-rowindex.

@lauri865 Is this what you mean?

When you scroll back up quickly, there's a whole bunch of rows that are forced to re-render,

I believe this only applies to a single row (the one that is focused). So it shouldn't impact performance much.

But yeah, there's no need to change the data-rowindex attribute for that row 👍🏻

@cherniavskii cherniavskii added bug 🐛 Something doesn't work status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 20, 2023
@github-actions
Copy link

The issue has been inactive for 7 days and has been automatically closed. If you think that it has been incorrectly closed, please reopen it and provide missing information (if any) or continue the last discussion.

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!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants