Fix useMutationWithMutationMode in optimistic and undoable mode may pass invalid parameters to the mutation

packages/ra-core/src/dataProvider/useMutationWithMutationMode.spec.tsx

@@ -1,7 +1,10 @@
 import * as React from 'react';
-import { render, waitFor } from '@testing-library/react';
+import { fireEvent, render, screen, waitFor } from '@testing-library/react';
 import expect from 'expect';
-import { useMutationWithMutationMode } from './useMutationWithMutationMode';
+import {
+    useMutationWithMutationMode,
+    UseMutationWithMutationModeOptions,
+} from './useMutationWithMutationMode';
 import { CoreAdminContext } from '../core/CoreAdminContext';
 import { useDataProvider } from './useDataProvider';
 import { DataProvider } from '../types';
@@ -12,7 +15,17 @@ describe('useMutationWithMutationMode', () => {
         updateUserProfile: ({ data }: { data: any }) => Promise<{ data: any }>;
     };
 
-    const useUpdateUserProfile = (args?: { data?: any }) => {
+    const useUpdateUserProfile = (
+        args?: { data?: any },
+        options?: Pick<
+            UseMutationWithMutationModeOptions<
+                Error,
+                { data: any },
+                { data?: any }
+            >,
+            'mutationMode'
+        >
+    ) => {
         const dataProvider = useDataProvider<MyDataProvider>();
         return useMutationWithMutationMode<
             Error,
@@ -33,6 +46,7 @@ describe('useMutationWithMutationMode', () => {
             getSnapshot: () => {
                 return [];
             },
+            ...options,
         });
     };
 
@@ -91,4 +105,51 @@ describe('useMutationWithMutationMode', () => {
             });
         });
     });
+
+    it('uses the latest params at execution time in optimistic mode', async () => {
+        const dataProvider = testDataProvider({
+            updateUserProfile: jest.fn(({ data }) =>
+                Promise.resolve({ data: { id: 1, ...data } } as any)
+            ),
+        }) as MyDataProvider;
+        const Dummy = () => {
+            const [data, setData] = React.useState({ value: 'value1' });
+            const [update] = useUpdateUserProfile(
+                {
+                    data,
+                },
+                { mutationMode: 'optimistic' }
+            );
+            return (
+                <>
+                    <button onClick={() => setData({ value: 'value2' })}>
+                        Update data
+                    </button>
+                    <button onClick={() => update()}>Update</button>
+                </>
+            );
+        };
+
+        render(
+            <CoreAdminContext dataProvider={dataProvider}>
+                <Dummy />
+            </CoreAdminContext>
+        );
+        fireEvent.click(screen.getByText('Update'));
+        // In case of undoable, clicking the _Update data_ button would trigger the mutation
+        fireEvent.click(screen.getByText('Update data'));
+        await waitFor(() => {
+            expect(dataProvider.updateUserProfile).toHaveBeenCalledWith({
+                data: { value: 'value1' },
+            });
+        });
+
+        // Ensure the next call uses the latest data
+        fireEvent.click(screen.getByText('Update'));
+        await waitFor(() => {
+            expect(dataProvider.updateUserProfile).toHaveBeenCalledWith({
+                data: { value: 'value2' },
+            });
+        });
+    });
 });

packages/ra-core/src/dataProvider/useMutationWithMutationMode.ts

@@ -59,6 +59,10 @@ export const useMutationWithMutationMode = <
     useEffect(() => {
         paramsRef.current = params;
     }, [params]);
+    // This ref won't be updated when params change in an effect, only when the mutate callback is called (See L247)
+    // This ensures that for undoable and optimistic mutations, the params are not changed by side effects (unselectAll for instance)
+    // _after_ the mutate function has been called, while keeping the ability to change declaration time params _until_ the mutation is called.
+    const paramsAtExecutionTimeRef = useRef<Partial<TVariables>>(params);
 
     // Ref that stores the snapshot of the state before the mutation to allow reverting it
     const snapshot = useRef<Snapshot>([]);
@@ -96,7 +100,6 @@ export const useMutationWithMutationMode = <
         {
             mutationKey,
             mutationFn: async params => {
-                const callTimeParams = { ...paramsRef.current, ...params };
                 if (params == null) {
                     throw new Error(
                         'useMutationWithMutationMode mutation requires parameters'
@@ -105,7 +108,7 @@ export const useMutationWithMutationMode = <
 
                 return (
                     mutateWithMiddlewares
-                        .current(callTimeParams as TVariables)
+                        .current(params as TVariables)
                         // Middlewares expect the data property of the dataProvider response
                         .then(({ data }) => data)
                 );
@@ -216,7 +219,10 @@ export const useMutationWithMutationMode = <
                 (params: TVariables) => {
                     // Store the final parameters which might have been changed by middlewares
                     paramsRef.current = params;
-                    return mutationFnEvent(params);
+                    return mutationFnEvent({
+                        ...params,
+                        ...callTimeParams,
+                    });
                 }
             );
         } else {
@@ -234,6 +240,9 @@ export const useMutationWithMutationMode = <
         // because they may change afterwards, which would break the undoable mode
         // as the previousData would be overwritten by the optimistic update
         paramsRef.current = params;
+        // Also store them in a ref that will always be used for optimistic and undoable updates
+        // as their actual mutation happens after their side effects which may change the params (unselectAll for instance)
+        paramsAtExecutionTimeRef.current = params;
 
         if (mutationMode) {
             mode.current = mutationMode;
@@ -246,7 +255,7 @@ export const useMutationWithMutationMode = <
         }
 
         snapshot.current = getSnapshotEvent(
-            { ...paramsRef.current, ...callTimeParams },
+            { ...paramsAtExecutionTimeRef.current, ...callTimeParams },
             {
                 mutationMode: mode.current,
             }
@@ -255,13 +264,13 @@ export const useMutationWithMutationMode = <
         if (mode.current === 'pessimistic') {
             if (returnPromise) {
                 return mutation.mutateAsync(
-                    { ...paramsRef.current, ...callTimeParams },
+                    { ...paramsAtExecutionTimeRef.current, ...callTimeParams },
                     // We don't pass onError and onSettled here as we will call them in the useMutation hook side effects
                     { onSuccess, ...otherCallTimeOptions }
                 );
             }
             return mutation.mutate(
-                { ...paramsRef.current, ...callTimeParams },
+                { ...paramsAtExecutionTimeRef.current, ...callTimeParams },
                 // We don't pass onError and onSettled here as we will call them in the useMutation hook side effects
                 { onSuccess, ...otherCallTimeOptions }
             );
@@ -276,7 +285,7 @@ export const useMutationWithMutationMode = <
 
         // Optimistically update to the new value
         const optimisticResult = updateCacheEvent(
-            { ...paramsRef.current, ...callTimeParams },
+            { ...paramsAtExecutionTimeRef.current, ...callTimeParams },
             {
                 mutationMode: mode.current,
             },
@@ -288,7 +297,7 @@ export const useMutationWithMutationMode = <
             if (onSuccess) {
                 onSuccess(
                     optimisticResult,
-                    callTimeParams,
+                    { ...paramsAtExecutionTimeRef.current, ...callTimeParams },
                     {
                         snapshot: snapshot.current,
                     },
@@ -304,7 +313,7 @@ export const useMutationWithMutationMode = <
             ) {
                 mutationOptions.onSuccess(
                     optimisticResult,
-                    callTimeParams,
+                    { ...paramsAtExecutionTimeRef.current, ...callTimeParams },
                     {
                         snapshot: snapshot.current,
                     },
@@ -319,24 +328,36 @@ export const useMutationWithMutationMode = <
 
         if (mode.current === 'optimistic') {
             // call the mutate method without success side effects
-            return mutation.mutate(callTimeParams);
+            return mutation.mutate({
+                ...paramsAtExecutionTimeRef.current,
+                ...callTimeParams,
+            });
         } else {
             // Undoable mutation: add the mutation to the undoable queue.
             // The Notification component will dequeue it when the user confirms or cancels the message.
             addUndoableMutation(({ isUndo }) => {
                 if (isUndo) {
                     if (onUndo) {
-                        onUndoEvent(callTimeParams, {
-                            mutationMode: mode.current,
-                        });
+                        onUndoEvent(
+                            {
+                                ...paramsAtExecutionTimeRef.current,
+                                ...callTimeParams,
+                            },
+                            {
+                                mutationMode: mode.current,
+                            }
+                        );
                     }
                     // rollback
                     snapshot.current.forEach(([key, value]) => {
                         queryClient.setQueryData(key, value);
                     });
                 } else {
                     // call the mutate method without success side effects
-                    mutation.mutate(callTimeParams);
+                    mutation.mutate({
+                        ...paramsAtExecutionTimeRef.current,
+                        ...callTimeParams,
+                    });
                 }
             });
         }