Fix WarnWhenUnsavedChanges alert after deleting a record

packages/ra-core/src/controller/button/useDeleteController.tsx

@@ -1,4 +1,5 @@
 import { useCallback, useMemo } from 'react';
+import { useFormContext } from 'react-hook-form';
 
 import { useDelete, UseDeleteOptions } from '../../dataProvider/useDelete';
 import { useUnselect } from '../list/useUnselect';
@@ -75,6 +76,8 @@ export const useDeleteController = <
     const unselect = useUnselect(resource);
     const redirect = useRedirect();
     const translate = useTranslate();
+    // null when the delete button is used outside a form (e.g. in a list)
+    const form = useFormContext();
 
     const [deleteOne, { isPending }] = useDelete<RecordType, ErrorType>(
         resource,
@@ -96,6 +99,11 @@ export const useDeleteController = <
                     }
                 );
                 record && unselect([record.id], true);
+                // Clear the form dirty state so warnWhenUnsavedChanges doesn't
+                // block the redirect: the record is gone, so the unsaved
+                // changes are moot. keepValues avoids a flash of reverted
+                // fields before the redirect commits.
+                form?.reset(undefined, { keepValues: true });
                 redirect(redirectTo, resource);
             },
             onError: (error: any) => {

packages/ra-core/src/form/Form.spec.tsx

@@ -22,6 +22,7 @@ import {
     InNonDataRouter,
     ServerSideValidation,
     MultiRoutesForm,
+    WarnWhenUnsavedChangesWithDelete,
 } from './Form.stories';
 import { mergeTranslations } from '../i18n';
 
@@ -1008,4 +1009,37 @@ describe('Form', () => {
             ).toEqual(false);
         }
     );
+
+    describe('warnWhenUnsavedChanges with a delete button', () => {
+        it.each(['undoable', 'pessimistic', 'optimistic'] as const)(
+            'should not warn about unsaved changes after deleting a dirty record (%s)',
+            async mutationMode => {
+                // spy on "cancel": if the dialog were shown the navigation
+                // would be cancelled, failing the redirect assertion
+                const confirmSpy = jest
+                    .spyOn(window, 'confirm')
+                    .mockReturnValue(false);
+                render(
+                    <WarnWhenUnsavedChangesWithDelete
+                        mutationMode={mutationMode}
+                    />
+                );
+                // wait for the record to load
+                const input =
+                    await screen.findByDisplayValue<HTMLInputElement>('Hello');
+                // make the form dirty
+                fireEvent.change(input, {
+                    target: { value: 'Hello modified' },
+                });
+                fireEvent.blur(input);
+                // delete the record (the headless DeleteButton has no
+                // confirmation dialog and always uses the controller onSuccess)
+                fireEvent.click(screen.getByText('Delete'));
+                // the app should redirect to the list without warning
+                await screen.findByText('Post list');
+                expect(confirmSpy).not.toHaveBeenCalled();
+                confirmSpy.mockRestore();
+            }
+        );
+    });
 });

packages/ra-core/src/form/Form.stories.tsx

@@ -10,6 +10,7 @@ import { yupResolver } from '@hookform/resolvers/yup';
 import * as yup from 'yup';
 import polyglotI18nProvider from 'ra-i18n-polyglot';
 import englishMessages from 'ra-language-english';
+import fakeRestDataProvider from 'ra-data-fakerest';
 import {
     Route,
     Routes,
@@ -20,7 +21,12 @@ import {
 } from 'react-router-dom';
 
 import { CoreAdminContext } from '../core';
-import { RecordContextProvider, SaveContextProvider } from '../controller';
+import {
+    EditBase,
+    RecordContextProvider,
+    SaveContextProvider,
+} from '../controller';
+import { DeleteButton } from '../test-ui/DeleteButton';
 import { Form, FormProps } from './Form';
 import { useInput } from './useInput';
 import { required, ValidationError } from './validation';
@@ -415,6 +421,69 @@ export const InNonDataRouter = ({
     </HashRouter>
 );
 
+const PostEditWithDelete = ({
+    mutationMode,
+}: {
+    mutationMode: 'undoable' | 'optimistic' | 'pessimistic';
+}) => (
+    <EditBase resource="posts" id={1} mutationMode={mutationMode}>
+        <Form warnWhenUnsavedChanges>
+            <Input source="title" />
+            <Input source="body" />
+            <DeleteButton
+                label="Delete"
+                mutationMode={mutationMode}
+                redirect="/posts"
+            />
+            <button type="submit">Submit</button>
+        </Form>
+    </EditBase>
+);
+
+/**
+ * Edit a field (making the form dirty), then click Delete. The record is
+ * deleted and the app redirects to the list, but the unsaved-changes blocker
+ * pops a spurious "unsaved changes" confirm dialog even though the record is
+ * already gone.
+ */
+export const WarnWhenUnsavedChangesWithDelete = ({
+    i18nProvider = defaultI18nProvider,
+    mutationMode = 'undoable',
+}: {
+    i18nProvider?: I18nProvider;
+    mutationMode?: 'undoable' | 'optimistic' | 'pessimistic';
+}) => (
+    <TestMemoryRouter initialEntries={['/posts/1']}>
+        <CoreAdminContext
+            dataProvider={fakeRestDataProvider(
+                {
+                    posts: [
+                        { id: 1, title: 'Hello', body: 'world' },
+                        { id: 2, title: 'Foo', body: 'bar' },
+                    ],
+                },
+                process.env.NODE_ENV !== 'test'
+            )}
+            i18nProvider={i18nProvider}
+        >
+            <Routes>
+                <Route
+                    path="/posts/:id"
+                    element={<PostEditWithDelete mutationMode={mutationMode} />}
+                />
+                <Route path="/posts" element={<p>Post list</p>} />
+            </Routes>
+        </CoreAdminContext>
+    </TestMemoryRouter>
+);
+
+WarnWhenUnsavedChangesWithDelete.argTypes = {
+    mutationMode: {
+        options: ['undoable', 'optimistic', 'pessimistic'],
+        control: { type: 'select' },
+    },
+};
+
 const Notifications = () => {
     const { notifications } = useNotificationContext();
     return (

packages/ra-core/src/form/useWarnWhenUnsavedChanges.tsx

@@ -40,21 +40,24 @@ export const useWarnWhenUnsavedChanges = (
     });
 
     useEffect(() => {
-        if (blocker.state === 'blocked') {
-            // Corner case: the blocker might be triggered by a redirect in the onSuccess side effect,
-            // happening during the same tick the form is reset after a successful save.
-            // In that case, the blocker will block but shouldNotBlock will be true one tick after.
-            // If we are in that case, we can proceed immediately.
-            if (shouldNotBlock) {
-                blocker.proceed();
-                return;
-            }
+        if (blocker.state !== 'blocked') return;
 
-            setShouldNotify(true);
+        // Proceed automatically when the form becomes safe to leave while the
+        // navigation is blocked. This covers two cases where the redirect and
+        // the form reset happen during the same tick (and the reset may only be
+        // reflected one tick after the block):
+        // - a successful save resets the form;
+        // - a successful delete resets the form (the record no longer exists,
+        //   so the unsaved changes are moot).
+        if (shouldNotBlock) {
+            blocker.proceed && blocker.proceed();
+            return;
         }
-        // This effect should only run when the blocker state changes, not when shouldNotBlock changes.
+
+        setShouldNotify(true);
+        // Can't use blocker in the dependency array because it is not stable across rerenders
         // eslint-disable-next-line react-hooks/exhaustive-deps
-    }, [blocker.state]);
+    }, [blocker.state, shouldNotBlock]);
 
     useEffect(() => {
         if (shouldNotify) {

packages/ra-ui-materialui/src/button/DeleteWithConfirmButton.spec.tsx

@@ -11,6 +11,7 @@ import {
     CoreAdminContext,
     MutationMode,
     testDataProvider,
+    TestMemoryRouter,
     useNotificationContext,
 } from 'ra-core';
 import { createTheme, ThemeProvider } from '@mui/material/styles';
@@ -453,4 +454,109 @@ describe('<DeleteWithConfirmButton />', () => {
         const buttons = await screen.findAllByTestId('themed');
         expect(buttons[0].classList).toContain('MuiButton-outlined');
     });
+
+    describe('with warnWhenUnsavedChanges and a dirty form', () => {
+        // Use the router-agnostic TestMemoryRouter + locationCallback (instead of
+        // react-router's Routes) so the test does not depend on the router adapter.
+        const renderDirtyEditForm = (
+            mutationMode: MutationMode,
+            locationCallback: (l: any) => void
+        ) => {
+            const dataProvider = testDataProvider({
+                getOne: () =>
+                    // @ts-ignore
+                    Promise.resolve({ data: { id: 123, title: 'lorem' } }),
+                delete: jest.fn().mockResolvedValue({ data: { id: 123 } }),
+            });
+            const EditToolbar = props => (
+                <Toolbar {...props}>
+                    <DeleteWithConfirmButton mutationMode={mutationMode} />
+                </Toolbar>
+            );
+            render(
+                <TestMemoryRouter
+                    initialEntries={['/posts/123']}
+                    locationCallback={locationCallback}
+                >
+                    <ThemeProvider theme={theme}>
+                        <CoreAdminContext dataProvider={dataProvider}>
+                            <Edit resource="posts" id={123}>
+                                <SimpleForm
+                                    warnWhenUnsavedChanges
+                                    toolbar={<EditToolbar />}
+                                >
+                                    <TextInput source="title" />
+                                </SimpleForm>
+                            </Edit>
+                        </CoreAdminContext>
+                    </ThemeProvider>
+                </TestMemoryRouter>
+            );
+            return dataProvider;
+        };
+
+        const makeFormDirty = async () => {
+            const input =
+                await screen.findByDisplayValue<HTMLInputElement>('lorem');
+            fireEvent.change(input, { target: { value: 'lorem modified' } });
+            fireEvent.blur(input);
+        };
+
+        it('should delete the record and redirect without warning when confirming the dialog', async () => {
+            // spy on "cancel": if the unsaved-changes dialog were shown, the
+            // navigation would be cancelled and the redirect assertion would fail
+            const confirmSpy = jest
+                .spyOn(window, 'confirm')
+                .mockReturnValue(false);
+            let location;
+            const dataProvider = renderDirtyEditForm('pessimistic', l => {
+                location = l;
+            });
+            await makeFormDirty();
+            // open the confirmation dialog and confirm
+            fireEvent.click(
+                await screen.findByLabelText('resources.posts.action.delete')
+            );
+            fireEvent.click(screen.getByText('ra.action.confirm'));
+            // the record is deleted and the app redirects to the list
+            await waitFor(() => {
+                expect(location.pathname).toEqual('/posts');
+            });
+            expect(dataProvider.delete).toHaveBeenCalledWith('posts', {
+                id: 123,
+                previousData: { id: 123, title: 'lorem' },
+            });
+            // no spurious unsaved-changes warning
+            expect(confirmSpy).not.toHaveBeenCalled();
+            confirmSpy.mockRestore();
+        });
+
+        it('should not delete the record nor leave the form when cancelling the dialog (optimistic)', async () => {
+            const confirmSpy = jest
+                .spyOn(window, 'confirm')
+                .mockReturnValue(false);
+            let location;
+            const dataProvider = renderDirtyEditForm('optimistic', l => {
+                location = l;
+            });
+            await makeFormDirty();
+            // open the confirmation dialog and cancel
+            fireEvent.click(
+                await screen.findByLabelText('resources.posts.action.delete')
+            );
+            fireEvent.click(screen.getByText('ra.action.cancel'));
+            // the dialog closes
+            await waitFor(() => {
+                expect(screen.queryByText('ra.action.confirm')).toBeNull();
+            });
+            // the deletion never happened (no optimistic removal to roll back)
+            expect(dataProvider.delete).not.toHaveBeenCalled();
+            // the user stays on the dirty form, edits preserved
+            expect(location.pathname).toEqual('/posts/123');
+            expect(screen.getByDisplayValue('lorem modified')).not.toBeNull();
+            // no unsaved-changes warning was triggered either
+            expect(confirmSpy).not.toHaveBeenCalled();
+            confirmSpy.mockRestore();
+        });
+    });
 });

packages/ra-ui-materialui/src/button/DeleteWithConfirmButton.tsx

@@ -1,4 +1,5 @@
 import React, { Fragment, isValidElement, ReactEventHandler } from 'react';
+import { useFormContext } from 'react-hook-form';
 import ActionDelete from '@mui/icons-material/Delete';
 import {
     ComponentsOverrides,
@@ -59,6 +60,8 @@ export const DeleteWithConfirmButton = React.forwardRef(
         const notify = useNotify();
         const unselect = useUnselect(resource);
         const redirect = useRedirect();
+        // null when the delete button is used outside a form (e.g. in a list)
+        const form = useFormContext();
         const [open, setOpen] = React.useState(false);
         if (!resource) {
             throw new Error(
@@ -95,6 +98,10 @@ export const DeleteWithConfirmButton = React.forwardRef(
                             }
                         );
                         record && unselect([record.id]);
+                        // Clear the form dirty state so warnWhenUnsavedChanges
+                        // doesn't block the redirect: the record is gone, so the
+                        // unsaved changes are moot.
+                        form?.reset(undefined, { keepValues: true });
                         redirect(redirectTo, resource);
                     }
                 },