mui-x icon indicating copy to clipboard operation
mui-x copied to clipboard

[DataGridPro] Fix crash on rows change in tree data with pagination

Open Copilot opened this issue 2 months ago • 2 comments

Fixes #20202

gridPaginationRowRangeSelector crashes when switching between tree data modes with pagination enabled, attempting to read .id from undefined when visible row arrays are temporarily empty during tree rebuilding.

Changes

  • gridPaginationSelector.ts: Added three defensive checks that return null instead of throwing:

    • Empty visibleSortedRowEntries array (line 115)
    • Undefined topLevelFirstRow when accessing by index (line 145)
    • -1 firstRowIndex when row not found in visible entries (line 155)
  • treeData.DataGridPro.test.tsx: Added integration test that toggles treeData prop with pagination enabled to verify no crashes occur during transitions.

Example

// Before: crashes with "Cannot read property 'id' of undefined"
const topLevelFirstRow = visibleSortedTopLevelRowEntries[topLevelFirstRowIndex];
const firstRowIndex = visibleSortedRowEntries.findIndex(
  (row) => row.id === topLevelFirstRow.id,
);

// After: safely handles edge cases
if (!visibleSortedRowEntries || visibleSortedRowEntries.length === 0) {
  return null;
}
// ... existing logic ...
if (!topLevelFirstRow) {
  return null;
}

[!WARNING]

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • iojs.org
    • Triggering command: curl -q --fail --compressed -L -s REDACTED -o - (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Original prompt

Problem

When switching treeData + pagination, gridPaginationRowRangeSelector can attempt to read properties of undefined because visibleSortedRowEntries (or visibleSortedTopLevelRowEntries) can be an empty array while the internal tree is rebuilding. This causes runtime exceptions (reading .id of undefined) and crashes.

Goal

Add defensive checks in packages/x-data-grid/src/hooks/features/pagination/gridPaginationSelector.ts so the selector returns null (or a safe default) and does not throw when there are no visible rows. Add unit tests that reproduce the empty-visible-rows case and assert the selector does not throw and returns null, plus a positive case.

Files to change

  1. Modify packages/x-data-grid/src/hooks/features/pagination/gridPaginationSelector.ts to add guards for empty visibleSortedRowEntries and for a missing firstRowIndex.

Proposed patch (replace the selector block in that file):

// (unchanged imports remain)

export const gridPaginationRowRangeSelector = createSelectorMemoized(
  gridPaginationEnabledClientSideSelector,
  gridPaginationModelSelector,
  gridRowTreeSelector,
  gridRowMaximumTreeDepthSelector,
  gridExpandedSortedRowEntriesSelector,
  gridFilteredSortedTopLevelRowEntriesSelector,
  (
    clientSidePaginationEnabled,
    paginationModel,
    rowTree,
    rowTreeDepth,
    visibleSortedRowEntries,
    visibleSortedTopLevelRowEntries,
  ) => {
    if (!clientSidePaginationEnabled) {
      return null;
    }

    // Defensive: if there are no visible rows at all, don't attempt to read ids
    if (!visibleSortedRowEntries || visibleSortedRowEntries.length === 0) {
      return null;
    }

    const visibleTopLevelRowCount = visibleSortedTopLevelRowEntries.length;
    const topLevelFirstRowIndex = Math.min(
      paginationModel.pageSize * paginationModel.page,
      visibleTopLevelRowCount - 1,
    );
    const topLevelLastRowIndex =
      paginationModel.pageSize === ALL_RESULTS_PAGE_VALUE
        ? visibleTopLevelRowCount - 1
        : Math.min(
            topLevelFirstRowIndex + paginationModel.pageSize - 1,
            visibleTopLevelRowCount - 1,
          );

    // The range contains no element
    if (topLevelFirstRowIndex === -1 || topLevelLastRowIndex === -1) {
      return null;
    }

    // The tree is flat, there is no need to look for children
    if (rowTreeDepth < 2) {
      return { firstRowIndex: topLevelFirstRowIndex, lastRowIndex: topLevelLastRowIndex };
    }

    const topLevelFirstRow = visibleSortedTopLevelRowEntries[topLevelFirstRowIndex];

    // Defensive: if the top level first row can't be found (should be rare), bail out
    if (!topLevelFirstRow) {
      return null;
    }

    const topLevelRowsInCurrentPageCount = topLevelLastRowIndex - topLevelFirstRowIndex + 1;
    const firstRowIndex = visibleSortedRowEntries.findIndex(
      (row) => row.id === topLevelFirstRow.id,
    );

    // Defensive: if the matching first row wasn't found in the flattened visible entries, bail out
    if (firstRowIndex === -1) {
      return null;
    }

    let lastRowIndex = firstRowIndex;
    let topLevelRowAdded = 0;

    while (
      lastRowIndex < visibleSortedRowEntries.length &&
      topLevelRowAdded <= topLevelRowsInCurrentPageCount
    ) {
      const row = visibleSortedRowEntries[lastRowIndex];
      const depth = rowTree[row.id]?.depth;

      if (depth === undefined) {
        lastRowIndex += 1;
      } else {
        if (topLevelRowAdded < topLevelRowsInCurrentPageCount || depth > 0) {
          lastRowIndex += 1;
        }

        if (depth === 0) {
          topLevelRowAdded += 1;
        }
      }
    }

    return { firstRowIndex, lastRowIndex: lastRowIndex - 1 };
  },
);
  1. Add unit tests: packages/x-data-grid/src/hooks/features/pagination/tests/gridPaginationSelector.test.ts
import { gridPaginationRowRangeSelector } from '../gridPaginationSelector';

describe('gridPaginationRowRangeSelector', () => {
  it('does not throw and returns null when visibleSortedRowEntries is empty', () => {
    const state: any = {
      pagination: { enabled: true, paginationModel: { page: 0, pageSize: 10 }, meta: {} },
      rows: {
        // selectors expect expandedSortedRowEntries and filteredSortedTopLevelRowEntries
        // but the selector under test receives the arrays directly via the selector pipeline.
      },
      // provide the selectors inputs by shaping the state the same way the selector expects
      // depending on the createSelector implementation, the selector will be invoked with the full state
      // In our unit test we only need to ensure the arguments passed to the...

</details>

*This pull request was created as a result of the following prompt from Copilot chat.*
> Problem
> 
> When switching treeData + pagination, gridPaginationRowRangeSelector can attempt to read properties of undefined because visibleSortedRowEntries (or visibleSortedTopLevelRowEntries) can be an empty array while the internal tree is rebuilding. This causes runtime exceptions (reading .id of undefined) and crashes.
> 
> Goal
> 
> Add defensive checks in packages/x-data-grid/src/hooks/features/pagination/gridPaginationSelector.ts so the selector returns null (or a safe default) and does not throw when there are no visible rows. Add unit tests that reproduce the empty-visible-rows case and assert the selector does not throw and returns null, plus a positive case.
> 
> Files to change
> 
> 1) Modify packages/x-data-grid/src/hooks/features/pagination/gridPaginationSelector.ts to add guards for empty visibleSortedRowEntries and for a missing firstRowIndex.
> 
> Proposed patch (replace the selector block in that file):
> 
> ```typescript name=packages/x-data-grid/src/hooks/features/pagination/gridPaginationSelector.ts url=https://github.com/mui/mui-x/blob/2423e2fd491c837ce5d30fbf6c34913c15ac2cb9/packages/x-data-grid/src/hooks/features/pagination/gridPaginationSelector.ts
> // (unchanged imports remain)
> 
> export const gridPaginationRowRangeSelector = createSelectorMemoized(
>   gridPaginationEnabledClientSideSelector,
>   gridPaginationModelSelector,
>   gridRowTreeSelector,
>   gridRowMaximumTreeDepthSelector,
>   gridExpandedSortedRowEntriesSelector,
>   gridFilteredSortedTopLevelRowEntriesSelector,
>   (
>     clientSidePaginationEnabled,
>     paginationModel,
>     rowTree,
>     rowTreeDepth,
>     visibleSortedRowEntries,
>     visibleSortedTopLevelRowEntries,
>   ) => {
>     if (!clientSidePaginationEnabled) {
>       return null;
>     }
> 
>     // Defensive: if there are no visible rows at all, don't attempt to read ids
>     if (!visibleSortedRowEntries || visibleSortedRowEntries.length === 0) {
>       return null;
>     }
> 
>     const visibleTopLevelRowCount = visibleSortedTopLevelRowEntries.length;
>     const topLevelFirstRowIndex = Math.min(
>       paginationModel.pageSize * paginationModel.page,
>       visibleTopLevelRowCount - 1,
>     );
>     const topLevelLastRowIndex =
>       paginationModel.pageSize === ALL_RESULTS_PAGE_VALUE
>         ? visibleTopLevelRowCount - 1
>         : Math.min(
>             topLevelFirstRowIndex + paginationModel.pageSize - 1,
>             visibleTopLevelRowCount - 1,
>           );
> 
>     // The range contains no element
>     if (topLevelFirstRowIndex === -1 || topLevelLastRowIndex === -1) {
>       return null;
>     }
> 
>     // The tree is flat, there is no need to look for children
>     if (rowTreeDepth < 2) {
>       return { firstRowIndex: topLevelFirstRowIndex, lastRowIndex: topLevelLastRowIndex };
>     }
> 
>     const topLevelFirstRow = visibleSortedTopLevelRowEntries[topLevelFirstRowIndex];
> 
>     // Defensive: if the top level first row can't be found (should be rare), bail out
>     if (!topLevelFirstRow) {
>       return null;
>     }
> 
>     const topLevelRowsInCurrentPageCount = topLevelLastRowIndex - topLevelFirstRowIndex + 1;
>     const firstRowIndex = visibleSortedRowEntries.findIndex(
>       (row) => row.id === topLevelFirstRow.id,
>     );
> 
>     // Defensive: if the matching first row wasn't found in the flattened visible entries, bail out
>     if (firstRowIndex === -1) {
>       return null;
>     }
> 
>     let lastRowIndex = firstRowIndex;
>     let topLevelRowAdded = 0;
> 
>     while (
>       lastRowIndex < visibleSortedRowEntries.length &&
>       topLevelRowAdded <= topLevelRowsInCurrentPageCount
>     ) {
>       const row = visibleSortedRowEntries[lastRowIndex];
>       const depth = rowTree[row.id]?.depth;
> 
>       if (depth === undefined) {
>         lastRowIndex += 1;
>       } else {
>         if (topLevelRowAdded < topLevelRowsInCurrentPageCount || depth > 0) {
>           lastRowIndex += 1;
>         }
> 
>         if (depth === 0) {
>           topLevelRowAdded += 1;
>         }
>       }
>     }
> 
>     return { firstRowIndex, lastRowIndex: lastRowIndex - 1 };
>   },
> );
> ```
> 
> 2) Add unit tests: packages/x-data-grid/src/hooks/features/pagination/__tests__/gridPaginationSelector.test.ts
> 
> ```typescript name=packages/x-data-grid/src/hooks/features/pagination/__tests__/gridPaginationSelector.test.ts
> import { gridPaginationRowRangeSelector } from '../gridPaginationSelector';
> 
> describe('gridPaginationRowRangeSelector', () => {
>   it('does not throw and returns null when visibleSortedRowEntries is empty', () => {
>     const state: any = {
>       pagination: { enabled: true, paginationModel: { page: 0, pageSize: 10 }, meta: {} },
>       rows: {
>         // selectors expect expandedSortedRowEntries and filteredSortedTopLevelRowEntries
>         // but the selector under test receives the arrays directly via the selector pipeline.
>       },
>       // provide the selectors inputs by shaping the state the same way the selector expects
>       // depending on the createSelector implementation, the selector will be invoked with the full state
>       // In our unit test we only need to ensure the arguments passed to the final projector match.
>     };
> 
>     // Call the selector with a shaped state that results in empty visibleSortedRowEntries and top-level entries
>     const shapedState: any = {
>       pagination: { enabled: true, paginationModel: { page: 0, pageSize: 10 } },
>       // These are the pieces consumed by the selector pipeline (the test harness will depend on createSelector implementation)
>       _private_test_inputs: true,
>     };
> 
>     // The selector is a memoized selector that will accept a full state. Because wiring the full selector
>     // pipeline in unit tests can be brittle, keep this test focused on calling the exported selector
>     // with a minimal object that corresponds to the createSelector inputs via their root selectors.
> 
>     const stateForSelector: any = {
>       pagination: { enabled: true, paginationModel: { page: 0, pageSize: 10 } },
>       // Provide the arrays via the selectors' root accessors. The exact keys used by the selector utilities
>       // in the project map the inputs differently. To keep this test simple and robust, we'll call the
>       // inner projector by calling the selector and verify it doesn't throw when visible arrays are empty.
>     };
> 
>     // When invoked with a state that ends up providing empty arrays to the final projector, the selector must not throw.
>     expect(() => gridPaginationRowRangeSelector({
>       pagination: { enabled: true, paginationModel: { page: 0, pageSize: 10 } },
>       // emulate selectors: these top-level keys will be used by the selector pipeline during test
>       // keep them empty to simulate the crash scenario
>       rows: { visibleSortedRowEntries: [], visibleSortedTopLevelRowEntries: [], tree: {}, treeDepth: 0 },
>     } as any)).not.toThrow();
> 
>     const result = gridPaginationRowRangeSelector({
>       pagination: { enabled: true, paginationModel: { page: 0, pageSize: 10 } },
>       rows: { visibleSortedRowEntries: [], visibleSortedTopLevelRowEntries: [], tree: {}, treeDepth: 0 },
>     } as any);
> 
>     expect(result).toBeNull();
>   });
> });
> ```
> 
> Notes
> 
> - The key changes are small and add early returns to avoid reading .id on undefined when arrays are empty or indices are -1.
> - The tests exercise the empty-array path and assert the selector doesn't throw and returns null.
> 
> Please create a branch (e.g. fix/pagination-empty-visible-rows), apply the changes, add the test file, and open a PR with the title above. Include the unit test so CI can validate.
> 

<!-- START COPILOT CODING AGENT TIPS -->
---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our [2 minute survey](https://gh.io/copilot-coding-agent-survey).

Copilot avatar Nov 05 '25 21:11 Copilot

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

Bundle size report

Bundle Parsed size Gzip size
@mui/x-data-grid 🔺+40B(+0.01%) 🔺+10B(+0.01%)
@mui/x-data-grid-pro 🔺+40B(+0.01%) 🔺+9B(+0.01%)
@mui/x-data-grid-premium 🔺+40B(+0.01%) 🔺+9B(0.00%)
@mui/x-charts 0B(0.00%) 0B(0.00%)
@mui/x-charts-pro 0B(0.00%) 0B(0.00%)
@mui/x-charts-premium 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 :no_entry_sign: dangerJS against e10e4c1c3d98a4818639c5657f1d2ec6b11acce8

mui-bot avatar Nov 05 '25 21:11 mui-bot

This pull request has been inactive for 30 days. Please remove the stale label or leave a comment to keep it open. Otherwise, it will be closed in 15 days.

github-actions[bot] avatar Dec 06 '25 15:12 github-actions[bot]