[DataGridPro] remove then add with throttleRowsMs causes missing rows

[wildcat-cs]

Duplicates

• [X] I have searched the existing issues

Latest version

• [X] I have tested the latest version

Steps to reproduce 🕹

This is a simplified example of the problem. Click Remove and Add to show not all rows are reflected and removing them and re-adding them

https://codesandbox.io/s/data-grid-community-forked-5rl7pg?file=/src/App.tsx

Current behavior 😯

If throttleRowsMs is not 0 and you remove more than one item and then add the items back rapidly not all of the items will be added back.

apiRef.current.updateRows([{ id: 1, _action: "delete" }]); apiRef.current.updateRows([{ id: 2, _action: "delete" }]); apiRef.current.updateRows([{ id: 1, col1: "Hello Update", col2: "World" }]); apiRef.current.updateRows([ { id: 2, col1: "XGrid Update", col2: "is Awesome" }]);

With the example on codesandbox only item 1 will be visible after the code is executed

Expected behavior 🤔

Expect all items added via updateRows to be visible in the grid.

Context 🔦

We are evaluating the Premium version and testing rapidly updating, removing and adding of data. We have a scenario where a few thousand items can be removed from the grid and a subset of them may be added back base on logic elsewhere in the system. We could set throttleRowsMs to 0 but in some cases this data can be updated in sub second intervals and we wanted to take advantage of the the throttling.

Your environment 🌎

npx @mui/envinfo

System: OS: macOS 13.5 Binaries: Node: 16.14.0 - ~/Applications/node-v16.14.0-darwin-x64/bin/node Yarn: 1.22.17 - ~/node_modules/.bin/yarn npm: 8.3.1 - ~/Applications/node-v16.14.0-darwin-x64/bin/npm Browsers: Chrome: 115.0.5790.170 Edge: 115.0.1901.200 Safari: 16.6

Order ID or Support key 💳 (optional)

No response

[DanailH]

@wildcat-cs thanks for raising this! Looking at the example the problem might be related to the implementation of the functionality you want to achieve. Aren't you able to batch updates so that you utilize the updateRows at its fullest? It takes an array of rows so you don't have to call it multiple times. In the example you gave with the few thousand rows, I imagine that these updates will be batch anyway. Regarding the sub-second interval updates, I guess those will also be batched. Out of curiosity what type of application are you building so we can understand the use case more correctly?

[wildcat-cs]

@DanailH thanks for the reply.
I did test with putting the two deletes in a single call to updateRows which made the issue go away, but it doesn't fix our case. The sample code is just a very simplistic way to show the error we are having with the grid.

Our data is streamed to us and we don't know what we are going to be adding or removing so to solve this problem we will need to implement some queuing to allow us to batch update the grid, but my impression from the documentation is the non-community versions include that capability with throttleRowsMs making me not have to write it :).

I'm concerned that with throttleRowsMs enabled and I create our own queue to throttle the updates that we could still arrive at the same bug (just way more complex to debug) if we do batch updates where one does some set of adds/update and the next does some adds/updates which conflict with the first and we happen to fall within one rendering loop for the grid.

If we have our own batching/throttling and turn off throttleRowsMs does this give us the same benefit of throttleRowsMs or would we incur a performance hit just because throttleRowsMs is turned off?

[romgrk]

@wildcat-cs After discussion, we have concluded that the behavior is implemented correctly according to the definition of "throttling", and that the feature you're looking for is batching and that is not implemented in the datagrid.

If we have our own batching/throttling and turn off throttleRowsMs does this give us the same benefit of throttleRowsMs or would we incur a performance hit just because throttleRowsMs is turned off?

You can safely ignore throttleRowsMs, it doesn't have a cost if it's not used.

Considering your use case, I would recommend to implement batching+throttling on your side, you'll have more control over what goes on, and you get the same benefits. There is not much complexity to throttling so I'm sure it will be fine to implement on your side.

If you want to request batching to be implemented natively in the datagrid, let us know and we will discuss the feature further, but it might take some time.

[wildcat-cs]

@romgrk Thanks for the quick reply, I do think something should be done for this or documentation should be updated as batching is specifically called out in the documentation.

Whenever the rows are updated, the data grid has to apply the sorting and filters. This can be a problem if you have high frequency updates. To maintain good performances, the data grid allows to batch the updates and only apply them after a period of time....

As written it seems like the throttleRowsMs only works with updateRows that do not have delete actions? I guess i'm not sure what specifically makes my example not fall under the documentation for high frequency updates.

So yes, I think the feature would be useful to the Pro/Premium and should be added as part of the throttling that is there today.

[yaredtsy]

i think the batching process has already been put in place. However, the issue lies in the execution as updates are not being performed in the correct order. The current sequence runs inserts first, followed by deletes, and then updates. updates should be prioritized according to their order of arrival to the system - first come, first served.

https://github.com/mui/mui-x/blob/68b4da1495774669138213fdf681f8a9ae6e712a/packages/grid/x-data-grid-pro/src/utils/tree/updateRowTree.ts#L34-L84

[DanailH]

In that case, it is a bug indeed but it still needs to be further investigated. If that is the case then the batching logic is incorrect - it should not care if something is inserted, deleted, or updated but rather when it is being called.

[yaredtsy]

i have tried to fix it it seems to work. In the update action array, if the same ID exists in both the insert and remove lists, I remove it from both and add it to the modify list.

--- a/packages/grid/x-data-grid/src/hooks/features/rows/gridRowsUtils.ts
+++ b/packages/grid/x-data-grid/src/hooks/features/rows/gridRowsUtils.ts
@@ -333,7 +333,12 @@ export const updateCacheWithNewRows = ({
     // Then add it to the "insert" update.
     // `actionAlreadyAppliedToRow` can't be equal to "modify", otherwise we would have an `oldRow` above.
     else if (actionAlreadyAppliedToRow == null) {
-      partialUpdates.actions.insert.push(id);
+      if (partialUpdates.actions.remove.includes(id)) {
+        partialUpdates.actions.remove = partialUpdates.actions.remove.filter(removeId => removeId !== id)
+        partialUpdates.actions.modify.push(id);
+      } else {
+        partialUpdates.actions.insert.push(id);
+      }
     }

demo: https://codesandbox.io/s/data-grid-community-forked-sr79ct?file=/src/App.tsx