Skip to content

[DataGrid] Fix lazy loading crash with isRowSelectable prop #17629

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

Merged
merged 6 commits into from
May 5, 2025

Conversation

MBilalShafi
Copy link
Member

@MBilalShafi MBilalShafi commented Apr 30, 2025

Fixes #17608

Follow-up tasks:

  • Add test coverage

@MBilalShafi MBilalShafi added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Server integration Better integration with backends, e.g. data source labels Apr 30, 2025
@mui-bot
Copy link

mui-bot commented Apr 30, 2025

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

Generated by 🚫 dangerJS against b783dd5

@MBilalShafi MBilalShafi marked this pull request as ready for review May 1, 2025 14:45
@MBilalShafi MBilalShafi requested a review from a team May 1, 2025 14:45
@philipburleigh
Copy link

When will this be approved?

@arminmeh
Copy link
Contributor

arminmeh commented May 5, 2025

The issue can also be fixed by using direct selectors to get the selection candidates

--- a/packages/x-data-grid/src/components/columnSelection/GridHeaderCheckbox.tsx
+++ b/packages/x-data-grid/src/components/columnSelection/GridHeaderCheckbox.tsx
@@ -40,11 +40,6 @@ const GridHeaderCheckbox = forwardRef<HTMLButtonElement, GridColumnHeaderParams>
     const classes = useUtilityClasses(ownerState);
     const tabIndexState = useGridSelector(apiRef, gridTabIndexColumnHeaderSelector);
     const selection = useGridSelector(apiRef, gridRowSelectionStateSelector);
-    const visibleRowIds = useGridSelector(apiRef, gridExpandedSortedRowIdsSelector);
-    const paginatedVisibleRowIds = useGridSelector(
-      apiRef,
-      gridPaginatedVisibleSortedGridRowIdsSelector,
-    );

     const filteredSelection = React.useMemo(() => {
       const isRowSelectable = rootProps.isRowSelectable;
@@ -80,8 +75,8 @@ const GridHeaderCheckbox = forwardRef<HTMLButtonElement, GridColumnHeaderParams>
         !rootProps.pagination ||
         !rootProps.checkboxSelectionVisibleOnly ||
         rootProps.paginationMode === 'server'
-          ? visibleRowIds
-          : paginatedVisibleRowIds;
+          ? gridExpandedSortedRowIdsSelector(apiRef)
+          : gridPaginatedVisibleSortedGridRowIdsSelector(apiRef);

       // Convert to a Set to make O(1) checking if a row exists or not
       const candidates = new Set<GridRowId>();
@@ -97,8 +92,6 @@ const GridHeaderCheckbox = forwardRef<HTMLButtonElement, GridColumnHeaderParams>
       rootProps.pagination,
       rootProps.paginationMode,
       rootProps.checkboxSelectionVisibleOnly,
-      paginatedVisibleRowIds,
-      visibleRowIds,
     ]);

@MBilalShafi
Copy link
Member Author

The issue can also be fixed by using direct selectors to get the selection candidates

--- a/packages/x-data-grid/src/components/columnSelection/GridHeaderCheckbox.tsx
+++ b/packages/x-data-grid/src/components/columnSelection/GridHeaderCheckbox.tsx
@@ -40,11 +40,6 @@ const GridHeaderCheckbox = forwardRef<HTMLButtonElement, GridColumnHeaderParams>
     const classes = useUtilityClasses(ownerState);
     const tabIndexState = useGridSelector(apiRef, gridTabIndexColumnHeaderSelector);
     const selection = useGridSelector(apiRef, gridRowSelectionStateSelector);
-    const visibleRowIds = useGridSelector(apiRef, gridExpandedSortedRowIdsSelector);
-    const paginatedVisibleRowIds = useGridSelector(
-      apiRef,
-      gridPaginatedVisibleSortedGridRowIdsSelector,
-    );

     const filteredSelection = React.useMemo(() => {
       const isRowSelectable = rootProps.isRowSelectable;
@@ -80,8 +75,8 @@ const GridHeaderCheckbox = forwardRef<HTMLButtonElement, GridColumnHeaderParams>
         !rootProps.pagination ||
         !rootProps.checkboxSelectionVisibleOnly ||
         rootProps.paginationMode === 'server'
-          ? visibleRowIds
-          : paginatedVisibleRowIds;
+          ? gridExpandedSortedRowIdsSelector(apiRef)
+          : gridPaginatedVisibleSortedGridRowIdsSelector(apiRef);

       // Convert to a Set to make O(1) checking if a row exists or not
       const candidates = new Set<GridRowId>();
@@ -97,8 +92,6 @@ const GridHeaderCheckbox = forwardRef<HTMLButtonElement, GridColumnHeaderParams>
       rootProps.pagination,
       rootProps.paginationMode,
       rootProps.checkboxSelectionVisibleOnly,
-      paginatedVisibleRowIds,
-      visibleRowIds,
     ]);

I'd not do it, as it would disrupt the reactive behavior established through the store binding via useGridSelector. Consequently, selectionCandidates would not recompute unless one of the remaining dependencies changes, resulting in stale data.

@arminmeh
Copy link
Contributor

arminmeh commented May 5, 2025

I'd not do it, as it would disrupt the reactive behavior established through the store binding via useGridSelector.

Right. Ignore
I had an impression that this is inside the feature hook.
Component needs useGridSelector.

@MBilalShafi MBilalShafi merged commit 0d4270c into mui:master May 5, 2025
21 checks passed
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: Server integration Better integration with backends, e.g. data source
Projects
None yet
4 participants