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.

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

[data grid] Removing column with turned on aggregation leads to crash #16091

Open
rettoua opened this issue Jan 7, 2025 · 4 comments · May be fixed by #16181
Open

[data grid] Removing column with turned on aggregation leads to crash #16091

rettoua opened this issue Jan 7, 2025 · 4 comments · May be fixed by #16181
Assignees
@cherniavskii
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! customization: logic Logic customizability feature: Aggregation Related to the data grid Aggregation feature support: premium standard Support request from a Premium standard plan user. https://mui.com/legal/technical-support-sla/

Comments

@rettoua
Copy link
Contributor

rettoua commented Jan 7, 2025 •
edited by github-actions bot
Loading

Steps to reproduce

Steps:

  1. Open this link to live example: https://codesandbox.io/p/sandbox/dazzling-jasper-9h3xhf
  2. After the grid is shown, change the comment or add a new one in the source code: it's important to make any changes in the source file
  3. Press Remove Gross Column button

Current behavior

After removing the column from the datagrid following error in thrown:
image

Expected behavior

No error

Context

In my project, this error occurs without the need to modify the source code – simply removing a column causes a crash. I couldn’t reproduce it in the sandbox, as the datagrid in my project is highly customized.
The issue started occurring after updating @mui/x-data-grid-premium to version 7.23. Version 7.22 works fine.

Two places can 'help' to fix the error:

  1. Removing groupingValueGetter eliminates error
    image
  2. Changing rows (just new array would be enough) also eliminates error
    image

I suppose there is probably a problem with the cache in the internals of the Datagrid.

Your environment

npx @mui/envinfo 
  System:
    OS: macOS 15.2
  Binaries:
    Node: 22.12.0 - /usr/local/bin/node
    npm: 10.9.0 - /usr/local/bin/npm
    pnpm: Not Found
  Browsers:
    Chrome: 123.0.6312.86
  npmPackages:
    @emotion/react: ~11.14.0 => 11.14.0 
    @emotion/styled: ~11.14.0 => 11.14.0 
    @mui/base:  5.0.0-beta.40-0 
    @mui/core-downloads-tracker:  5.16.13 
    @mui/icons-material: ~5.16.13 => 5.16.13 
    @mui/lab: 5.0.0-alpha.175 => 5.0.0-alpha.175 
    @mui/material: ~5.16.13 => 5.16.13 
    @mui/private-theming:  5.16.13 
    @mui/styled-engine:  5.16.13 
    @mui/styles: ~5.16.13 => 5.16.13 
    @mui/system: ~5.16.13 => 5.16.13 
    @mui/types:  7.2.20 
    @mui/utils:  6.3.0 
    @mui/x-charts: ~7.23.2 => 7.23.2 
    @mui/x-charts-vendor:  7.20.0 
    @mui/x-data-grid:  7.23.5 
    @mui/x-data-grid-premium: ~7.23.0 => 7.23.5 
    @mui/x-data-grid-pro:  7.23.5 
    @mui/x-date-pickers:  7.23.3 
    @mui/x-date-pickers-pro: ~7.23.3 => 7.23.3 
    @mui/x-internals:  7.23.0 
    @mui/x-license:  7.23.5 
    @mui/x-tree-view: ~7.23.2 => 7.23.2 
    @types/react: ~18.3.18 => 18.3.18 
    react: ~18.3.1 => 18.3.1 
    react-dom: ~18.3.1 => 18.3.1 
    typescript: ~5.7.2 => 5.7.2

Search keywords: datagrid

Order ID: 100790
@rettoua rettoua added bug 🐛 Something doesn't work status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 7, 2025
@github-actions github-actions bot added component: data grid This is the name of the generic UI component, not the React module! support: premium standard Support request from a Premium standard plan user. https://mui.com/legal/technical-support-sla/ labels Jan 7, 2025
@michelengelen
Copy link
Member

Hey @rettoua and sry for the late answer.

The problem seems to stem from this part of the code, where it cannot find the auto generated aggregation row to process.

mui-x/packages/x-data-grid/src/hooks/features/rows/useGridParamsApi.ts

Lines 57 to 70 in 5999d29

const getCellParams = React.useCallback<GridParamsApi['getCellParams']>(
(id, field) => {
const colDef = (
props.unstable_listView
? gridListColumnSelector(apiRef.current.state)
: apiRef.current.getColumn(field)
) as GridStateColDef;
const row = apiRef.current.getRow(id);
const rowNode = apiRef.current.getRowNode(id);
if (!row || !rowNode) {
throw new MissingRowIdError(`No row with id #${id} found`);
}

That being said we do not recommend using a columns array that has no stable reference. For the grid to work as expected you should never alter the columns array.

Wouldn't it be enough to hide the column instead of plain-removing it: column visibility?

@michelengelen michelengelen added 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 Jan 10, 2025
@michelengelen michelengelen changed the title Removing column with turned on aggregation leads to crash [data grid] Removing column with turned on aggregation leads to crash Jan 10, 2025
@michelengelen michelengelen added customization: logic Logic customizability feature: Aggregation Related to the data grid Aggregation feature labels Jan 10, 2025
@rettoua
Copy link
Contributor Author

rettoua commented Jan 13, 2025

hi @michelengelen,
I don’t think the statement For the grid to work as expected, you should never alter the columns array sounds entirely right. It contradicts the whole idea of props.

What approach would you recommend for using the DataGrid to meet the following requirements:
1. There are hundreds of potential fields that could be used in the DataGrid.
2. The user configures the grid on the fly: adding columns, setting up grouping, aggregation, using column bands, and so on.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Jan 13, 2025
@michelengelen
Copy link
Member

Hey @rettoua ... maybe I picked too extreme wording there. The columns prop should ideally never change. There is definitely the possibility to work with dynamic columns, but it is not the recommended approach. Either way you should make sure that the columns array contains a stable reference during rerenders. This can be done by either define it outside of the component as a constant or by memoizing it.

The issue presented here is still valid and should be handled, so I'll add it to the board.

@michelengelen michelengelen removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 13, 2025
@github-project-automation github-project-automation bot moved this to 🆕 Needs refinement in MUI X Data Grid Jan 13, 2025
@rettoua
Copy link
Contributor Author

rettoua commented Jan 13, 2025

@michelengelen,
got it, thank you.

@cherniavskii cherniavskii self-assigned this Jan 14, 2025
@cherniavskii cherniavskii linked a pull request Jan 20, 2025 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cherniavskii cherniavskii

Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! customization: logic Logic customizability feature: Aggregation Related to the data grid Aggregation feature support: premium standard Support request from a Premium standard plan user. https://mui.com/legal/technical-support-sla/
Projects
MUI X Data Grid
Status: 🆕 Needs refinement
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[DataGridPremium] Fix error when aggregated column is removed cherniavskii/mui-x
3 participants
@cherniavskii @michelengelen @rettoua