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.07k stars 1.26k forks source link

[DataGrid] `processRowUpdate` misbehaves when row is removed right before it fires #13707

Open Janpot opened 2 months ago

Janpot commented 2 months ago

Steps to reproduce

Link to live example: https://stackblitz.com/edit/react-xqph3o?file=Demo.tsx

Steps:

  1. Open the example
  2. open the console
  3. Click "ADD"
  4. Change "baz" to something else
  5. press enter

Current behavior

it prints

{username: "quux", age: 123} null

The original row is lost, as well as the id in the updated row. Note that this null is also inconsistent with the type signature.

Expected behavior

It should print

{id: 3, username: "quux", age: 123} {id: 3, username: "baz", age: 123}

Alternatively one could argue the types should be updated, but that effectively results into a loss of information in the API. i.e. you lose the start value to compare with as well as the assigned id.

Context

Trying to build a more reliable version of the CRUD example I've ran into this several times.

Your environment

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

Search keywords: processRowUpdate null

arminmeh commented 2 months ago

@Janpot not sure if I am missing something here besides the console output, what is the expected handling for the state change?

Once the row is out of the draft, it is removed from rows

const rows = React.useMemo(
  () => (draft ? [draft, ...rowsFromExternalApi] : rowsFromExternalApi),
  [draft]
);

This is why row data cannot be fetched here and we end up with the error (in dev mode) and some missing data in the callback args

Having a separate rows state like in the CRUD example that you have shared, solves the issue

https://stackblitz.com/edit/react-xqph3o-j4obgd?file=Demo.tsx

Janpot commented 2 months ago

besides the console output, what is the expected handling for the state change?

Having a separate rows state like in the CRUD example that you have shared, solves the issue

IMO the example is too simplistic:

  1. In many cases, rows will be fetched from an external API. Libraries like react-query will potentially even invalidate and refetch them in the background while the create flow is active. The code in the example feels a bit anti-react to me as it requires careful curating of the rows state instead of deriving it from "am I in row creation mode or not" state.
  2. In many use-cases the backend only assigns an id during the creation API call and it's not quite intuitive how the example needs to be updated to account for that.
  3. There are also several UX issues with this demo:
    • It's possible to open multiple rows in edit mode at the same time. Potential solution: Make only one row editable at the same time. Close the currently editable one if a new one is opened if it doesn't have edits, otherwise ask to commit/discard/cancel.
    • It's possible to scroll away from those rows with no visual indication they are in edit mode. As well as switch pages or filter. The user may have something in draft on a different page without realizing it. Potential solution: Add a floating button when the edit row is out of the viewport which scrolls or moves page to it after clicking
    • It's possible to open the create flow at the same time, and scroll away from it. Potential solution: disable create button when already editing a row.
    • It's possible to open multiple rows in create mode
    • When there are multiple pages, the "add record" button creates a row at the end of the last page, regardless of which page you are on. Potential solution: pin created row to the top, regardless of which page you are on.
michelengelen commented 2 months ago

Lets add this to the board for further discussion!