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.

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

[data grid] processRowUpdate misbehaves when row is removed right before it fires #13707

Open
Janpot opened this issue Jul 2, 2024 · 3 comments · May be fixed by #16741
Open

[data grid] processRowUpdate misbehaves when row is removed right before it fires #13707

Janpot opened this issue Jul 2, 2024 · 3 comments · May be fixed by #16741
Assignees
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Editing Related to the data grid Editing feature

Comments

@Janpot
Copy link
Member

Janpot commented Jul 2, 2024

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

@Janpot Janpot added component: data grid This is the name of the generic UI component, not the React module! status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 2, 2024
@KenanYusuf KenanYusuf added the feature: Editing Related to the data grid Editing feature label Jul 2, 2024
@arminmeh
Copy link
Contributor

arminmeh commented Jul 9, 2024

@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

@arminmeh arminmeh 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 Jul 9, 2024
@Janpot
Copy link
Member Author

Janpot commented Jul 15, 2024

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

  • The function has a type signature and that doesn't include null for its second parameter. My expectation is that it never gets called with arguments that don't satisfy their types.
  • The row switches from edit to view in the same render as it disappears. I think the behavior of switching edit to view of a row that doesn't exist anymore needs to be defined.
    • If processRowUpdate gets called, I expect it to get passed the original row and the updated row. i.e. store the row values when edit starts and use that value when row edit stops instead of trying to read it from the grid state.
    • If it doesn't get called I expect to see an error in the console. i.e. "a row that is in edit mode has been removed,..."

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.

@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 Jul 15, 2024
@michelengelen
Copy link
Member

Lets add this to the board for further discussion!

@michelengelen michelengelen removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 15, 2024
@github-project-automation github-project-automation bot moved this to 🆕 Needs refinement in MUI X Data Grid Jul 15, 2024
@cherniavskii cherniavskii added the bug 🐛 Something doesn't work label Aug 27, 2024
@cherniavskii cherniavskii moved this from 🆕 Needs refinement to 🔖 Ready in MUI X Data Grid Aug 27, 2024
@arminmeh arminmeh self-assigned this Feb 17, 2025
@arminmeh arminmeh moved this from 🔖 Ready to 🏗 In progress in MUI X Data Grid Feb 17, 2025
@arminmeh arminmeh moved this from 🏗 In progress to 👀 In review in MUI X Data Grid Feb 26, 2025
@oliviertassinari oliviertassinari changed the title [DataGrid] processRowUpdate misbehaves when row is removed right before it fires [data grid] processRowUpdate misbehaves when row is removed right before it fires Mar 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Editing Related to the data grid Editing feature
Projects
Status: 👀 In review
Development

Successfully merging a pull request may close this issue.

5 participants