[data grid] invalid gridState in local storage causes crash on page load

[hdale94]

Steps to reproduce

The issue

Steps:

1. Open this link to live example: https://codesandbox.io/p/sandbox/dry-microservice-forked-927pcx?workspaceId=ws_SzhXwxFRYxciKVso4R9bwN
2. Copy code to local project to allow local storage use
3. Group name column, which should cause crash
4. Refresh page, and it will forever crash because the invalid gridState was saved in local storage

Current behavior

Invalid grid state in local storage crashes application when initializing grid state.

Expected behavior

Allow us to catch DataGrid errors, e.g. with ErrorBoundary so we can handle errors gracefully.

Context

No response

Your environment

npx @mui/envinfo

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

Search keywords: DataGridPremium, Crash, Grouping

Order ID: 83335

[michelengelen]

The main problem here is the crash that is caused by an incorrect block of code.
valueGetter should either check if the row in question is autogenerated: Autogenerated rows

example:

valueGetter: (_, row: RowType) => {
  if (isAutogeneratedRow(row)) {
    return '[this is an autogenerated row]';
  }
  ...
},

Or you can adapt to something else and fix the error in your function:

valueGetter: (_, row: RowType) => {
  // Extract fieldNames from all questions based on the questionIds in the row
  const questionNames = questions
-   ?.filter((question) => row.questionIds.includes(question.fieldId))
+   ?.filter((question) => row.questionIds?.includes(question.fieldId))
    .map((question) => question.fieldName)
    .join(', ');
  return questionNames ?? '';
},

This should solve your issue

[hdale94]

The main problem here is the crash that is caused by an incorrect block of code. valueGetter should either check if the row in question is autogenerated: Autogenerated rows

example:

valueGetter: (_, row: RowType) => {
  if (isAutogeneratedRow(row)) {
    return '[this is an autogenerated row]';
  }
  ...
},

Or you can adapt to something else and fix the error in your function:

valueGetter: (_, row: RowType) => {
  // Extract fieldNames from all questions based on the questionIds in the row
  const questionNames = questions
-   ?.filter((question) => row.questionIds.includes(question.fieldId))
+   ?.filter((question) => row.questionIds?.includes(question.fieldId))
    .map((question) => question.fieldName)
    .join(', ');
  return questionNames ?? '';
},

This should solve your issue

Thanks, I didn't know about the autogenerated rows.

While this solves the issue in this particular case, it doesn't solve the issue at its root, which is that the data grid will store an invalid state to local storage.

[michelengelen]

The main problem here is the crash that is caused by an incorrect block of code. valueGetter should either check if the row in question is autogenerated: Autogenerated rows

example:

valueGetter: (_, row: RowType) => {

if (isAutogeneratedRow(row)) {

return '[this is an autogenerated row]';

}

...

},

Or you can adapt to something else and fix the error in your function:

valueGetter: (_, row: RowType) => {

// Extract fieldNames from all questions based on the questionIds in the row

const questionNames = questions

• ?.filter((question) => row.questionIds.includes(question.fieldId))

• ?.filter((question) => row.questionIds?.includes(question.fieldId))

.map((question) => question.fieldName)

.join(', ');

return questionNames ?? '';

},

This should solve your issue

Thanks, I didn't know about the autogenerated rows.

While this solves the issue in this particular case, it doesn't solve the issue at its root, which is that the data grid will store an invalid state to local storage.

Why should the state be invalid? Once you resolve that problem with the row property not being available the error will go away. The state that is being stored to the local storage is not related to that.

[hdale94]

The main problem here is the crash that is caused by an incorrect block of code. valueGetter should either check if the row in question is autogenerated: Autogenerated rows

example:

valueGetter: (_, row: RowType) => {

if (isAutogeneratedRow(row)) {

return '[this is an autogenerated row]';

}

...

},

Or you can adapt to something else and fix the error in your function:

valueGetter: (_, row: RowType) => {

// Extract fieldNames from all questions based on the questionIds in the row

const questionNames = questions

• ?.filter((question) => row.questionIds.includes(question.fieldId))

• ?.filter((question) => row.questionIds?.includes(question.fieldId))

.map((question) => question.fieldName)

.join(', ');

return questionNames ?? '';

},

This should solve your issue

Thanks, I didn't know about the autogenerated rows.
While this solves the issue in this particular case, it doesn't solve the issue at its root, which is that the data grid will store an invalid state to local storage.

Why should the state be invalid? Once you resolve that problem with the row property not being available the error will go away. The state that is being stored to the local storage is not related to that.

The fix you provided does solve the issue in this particular case, but when having large data-grids, 10-20 columns with some valueGetters, catching these potential errors is difficult.

It's really about the order of the operations, which should ensure validation before setting the grid-state to local storage.

This is how I experience it currently:

1. Click group column
2. New data-grid state is set to local storage
3. Row grouping occurs and fails

Now you're stuck with a state in local storage that crashes the data-grid because the state was stored before the row grouping occurred.

It would IMO make more sense this way:

1. Click group column
2. Row grouping occurs and fails
3. New data-grid state is not set to local storage because the application has already crashed

[cherniavskii]

Correct me if I'm wrong, but localStorage and state export are unrelated to the reported issue.
I've created this minimal demo that doesn't use the initial state at all and I can still reproduce the issue you reported: https://codesandbox.io/p/sandbox/dry-microservice-forked-dh6mwt

The Autogenerated rows section is indeed the current solution to avoid this.

Hope this helps!

[hdale94]

Correct me if I'm wrong, but localStorage and state export are unrelated to the reported issue. I've created this minimal demo that doesn't use the initial state at all and I can still reproduce the issue you reported: https://codesandbox.io/p/sandbox/dry-microservice-forked-dh6mwt

The Autogenerated rows section is indeed the current solution to avoid this.

Hope this helps!

I agree partly, but the issue is really that there is seemingly no graceful handling in place before updating the grid-state, or when loading the grid state.

Does it make sense to push a complex object to local storage if any errors are thrown in the grid? In my opinion no, especially if I can't validate the incoming object when reading back from local storage.

If there was a function that could validate the grid-state to the rows, it would atleast allow us to handle these errors gracefully when reading the state.

[michelengelen]

As mentioned already, the grid state is unrelated to the error. We could validate it, but it would still run into the error, since the state in itself is correct. The valueGetter implementation in this case is the source of the problem.

Additionally the local storage recipe is not a feature of the grid, but a recipe to get you started on a potential implementation. So if you want you can adjust the recipe and remove the object on error, or don't even store it in the first place.

IMHO adding a method to "test" the state against the current set of rows would mean an immense performance impact.

[hdale94]

Yeah, I understand the performance impact.

Additionally the local storage recipe is not a feature of the grid, but a recipe to get you started on a potential implementation. So if you want you can adjust the recipe and remove the object on error, or don't even store it in the first place.

Do you have a suggestion how I can remove the object on error? A Try...Catch?

[michelengelen]

Yeah, I understand the performance impact.

Additionally the local storage recipe is not a feature of the grid, but a recipe to get you started on a potential implementation. So if you want you can adjust the recipe and remove the object on error, or don't even store it in the first place.

Do you have a suggestion how I can remove the object on error? A Try...Catch?

Typically you would do this with an ErrorBoundary. But it's been quite some time since i built something like that. Maybe @cherniavskii knows a good way to do this. I am not opposed to add this to the docs if we find a good enough solution for this as well to prevent problems like this beforehand! 👍🏼

[hdale94]

Yeah, I understand the performance impact.

Additionally the local storage recipe is not a feature of the grid, but a recipe to get you started on a potential implementation. So if you want you can adjust the recipe and remove the object on error, or don't even store it in the first place.

Do you have a suggestion how I can remove the object on error? A Try...Catch?

Typically you would do this with an ErrorBoundary. But it's been quite some time since i built something like that. Maybe @cherniavskii knows a good way to do this. I am not opposed to add this to the docs if we find a good enough solution for this as well to prevent problems like this beforehand! 👍🏼

I know atleast 1 other person using DataGridPremium who has faced the same issue. I think an example using ErrorBoundary in the docs would be useful for a lot of people.

[michelengelen]

@mui/xgrid should we add this to the board?

[MBilalShafi]

@mui/xgrid should we add this to the board?

Sure, let's keep it as a recipe waiting for upvotes, if we have demand for it in the community, we could make update the existing one or add a new recipe.

@hdale94 Feel free to upvote the issue description.

[hdale94]

@mui/xgrid should we add this to the board?

Sure, let's keep it as a recipe waiting for upvotes, if we have demand for it in the community, we could make update the existing one or add a new recipe.

@hdale94 Feel free to upvote the issue description.

Great, updated the expected behaviour in the issue.