mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
4.06k stars 1.25k forks source link

[data grid] Edited row does not update when update "rows" state for DataGrid "rows" prop in processRowUpdate #14379

Closed LANCHERBA closed 1 week ago

LANCHERBA commented 2 weeks ago

Steps to reproduce

Link to live example: (required) https://codesandbox.io/p/sandbox/great-fast-7pvd6d?file=%2Fsrc%2FDemo.tsx Steps:

  1. Click on "Age" column on row 4 (it is randomly chosen just for clear issue reproduction)
  2. Type in "xxx" and hit enter
  3. See both line 1 and 4 shows the same updated row 4's data.

Current behavior

Updated row 4's data shifts to the first row which is correct. However, current row 4 still shows updated 4's data which is incorrect.

Expected behavior

I want just-updated row 4 data shifted to row 1 as logic indicated (as data transfer from "tempPeople" state to "people" state and I concatenate two lists together to form "rows" provided to DataGrid). Current row 4 should show previous row 3's data, as row 4 shifted to front and cause row 1,2,3's data move one row below.

Context

I need to maintain two react states, one for valid data (people) and one for invalid data (tempPeople). As demo shown, the data-validation check is done in processRowUpdate. After the check, those two react states will be updated (based on whether some invalid data become valid after editing. This should trigger the "rows" change and re-render the DataGrid component). At the same time, as the return statement in processRowUpdate will also updateRows, I am wondering if it causes some racing conditions. One comment may be related to this issue: https://github.com/mui/mui-x/issues/12475#issuecomment-2020071204 Thanks for any help and suggestions.

Your environment

npx @mui/envinfo ``` System: OS: Windows 11 10.0.22621 Binaries: Node: 20.13.1 - C:\Program Files\nodejs\node.EXE npm: 10.5.2 - C:\Program Files\nodejs\npm.CMD pnpm: Not Found Browsers: Chrome: 128.0.6613.84 Edge: Chromium (127.0.2651.74) npmPackages: @emotion/react: ^11.13.3 => 11.12.0 @emotion/styled: ^11.13.0 => 11.12.0 @mui/core-downloads-tracker: 5.16.4 @mui/icons-material: ^5.16.7 => 5.16.4 @mui/material: ^5.16.7 => 5.16.4 @mui/private-theming: 5.16.4 @mui/styled-engine: 5.16.4 @mui/system: 5.16.4 @mui/types: 7.2.15 @mui/utils: 5.16.4 @mui/x-data-grid: ^7.14.0 => 7.9.0 @mui/x-internals: 7.9.0 @types/react: ^18.3.4 => 18.3.2 react: ^18.3.1 => 18.3.1 react-dom: ^18.3.1 => 18.3.1 typescript: ^5.5.4 => 5.4.5 ```

Search keywords: processRowUpdate

michelengelen commented 2 weeks ago

@LANCHERBA your guess was right. You are running into some weird race conditions. Mainly because of one thing: You should not update the state during a state update. The documentation clearly states that the processRowUpdate callback will call updateRows internally with the new row being returned. If you update the external state in between there will be problems.

Is there a specific reason why you need the two states handled separately?

LANCHERBA commented 2 weeks ago

@LANCHERBA your guess was right. You are running into some weird race conditions. Mainly because of one thing: You should not update the state during a state update. The documentation clearly states that the processRowUpdate callback will call updateRows internally with the new row being returned. If you update the external state in between there will be problems.

Is there a specific reason why you need the two states handled separately?

Hi, @michelengelen, thanks for the reply! I have the "valid-data" state which is used to sync with backend using websocket (I only want valid data go to backend and in another way, be replaced by incoming data from backend). The other "invalid-data" state is used for client-side storage to store temporary invalid data (for example, an just-added "People" row without "age" assigned as we evaluate if data is "valid" or not by checking if "age" exists).

And the reason why I I still keep states in react because in other components/pages, I need to use these data.

Could I ask is there any way to update those states after the processRowUpdate in logic? Thanks!

michelengelen commented 2 weeks ago

First of all its never a good idea to keep more than one state logic in place. You should rather keep the one state and act on it. This means you can build a function to determine the set of data that is invalid. By definition the other part of the data is invalid.

So, a function like this can be invoked and used wherever you need it:

function getValidRows(rows: readonly Person[]): Person[] {
  return rows
    .map((row) => (row.age && row.age > 0 && row.age < 120 && row.name.trim() !== '' ? row : null))
    .filter((row) => row !== null) as Person[];
}

This could be used in the processRowUpdate as well to determine the part of the rows that should be send to the API.

LANCHERBA commented 2 weeks ago

@michelengelen Thanks for the advise, but just one thing that the project is a multi-user project, multiple users can work on the same table on their local web client and bi-directional websocket is built between all clients with the backend server (and we want to only allow valid data go to backend+database).

In the actual solution, those two react states are stored in react context and injected into the component (then concatenated for "rows"). If we just keep one state (mix of valid and invalid data), then whenever websocket sends data updated by other users to frontend, it can update all rows marked as "valid" by direct replacement, but could mess up row indices in the DataGrid (rows might jump all around). Keep two states are for better UX and data usage in this case.

michelengelen commented 2 weeks ago

Well... I did look over your code and the whole logic around the id-setting is a bit too brittle. The id is the one property that needs to be absolutely stable during the component lifecycle.

I did update your example simplifying the logic a bit and trimming it down to the essentials. You can check it out here.

I hope this solves your problem!

github-actions[bot] commented 1 week ago

The issue has been inactive for 7 days and has been automatically closed.