refinedev / refine

A React Framework for building internal tools, admin panels, dashboards & B2B apps with unmatched flexibility.
https://refine.dev
MIT License
25.92k stars 1.96k forks source link

[BUG] UnsavedChangesNotifier not triggered on back/forward navigation #6060

Open tlkv opened 1 week ago

tlkv commented 1 week ago

Describe the bug

UnsavedChangesNotifier doesn't trigger warning message if user came from list view and clicks browser back button after making changes to any field. You can see on the screenshot below which actions trigger warning message (green) and which ones don't work as expected (red): chrome_1CknMgfLqp

Steps To Reproduce

  1. Create refine project with Vite and warnWhenUnsavedChanges set to true, e. g. npm create refine-app@latest -- --example data-provider-strapi-v4.
  2. Navigate to list view for posts http://localhost:5173/posts.
  3. Open any content from this list in edit mode.
  4. Change any field content.
  5. Navigate to list view using browser back button - you don't get warning about unsaved changes.

Expected behavior

Warning about unsaved changes should be triggered before navigating back to list view using browser back button.

Packages

"@refinedev/antd": "^5.40.0", "@refinedev/cli": "^2.16.33", "@refinedev/core": "^4.51.0", "@refinedev/react-router-v6": "^4.5.11", "@refinedev/strapi-v4": "^6.0.8",

Additional Context

I took a brief glance at source code and it looks like such behavior is mostly related to routing. All other cases (clicking on elements, home or refresh buttons) are covered. It also works as expected when you navigate directly to edit page (e.g. pasting a link to it), not from list view. That's because in the latter case beforeunload is triggered not just for home and refresh buttons, but for back/forward as well.

aliemir commented 1 week ago

Hey @tlkv thank you for the detailed explanation! I think this can be handled by adding an event listener for popstate and also by using a hacky way to manipulate window.history. Doing this will break the forward states and disable the forward button if unsaved changes notifier is triggered due to the nature of window.history API πŸ˜…

Something like this:

  React.useEffect(() => {
    if (warnWhen) {
      // push the current state (this will break the forward actions)
      window.history.pushState(null, document.title, location.href);

      const popstateListener = (event: PopStateEvent) => {
        event.preventDefault();
        // prompt user for the navigation
        if (window.confirm(warnMessage)) {
          setWarnWhen?.(false);
          // go back if confirmed
          window.history.back();
        } else {
          // restore duplicate state
          window.history.pushState(null, document.title, location.href);
        }
      };

      addEventListener("popstate", popstateListener);

      return () => {
        removeEventListener("popstate", popstateListener);
      };
    }

    return () => 0;
  }, [pathname, warnMessage, warnWhen]);

While this may work, we probably need to remove the override of navigator.go from the usePrompt hook to make sure they're not conflicting with each other.

I'm not sure if this is worth breaking the forward buttons πŸ€” Maybe there can be a prop in <UnsavedChangesNotifier /> to introduce this behavior. Adding this without keeping the current behavior is kinda a breaking change πŸ€–

Do you have any suggestions or alternative ideas for the implementation?

tlkv commented 5 days ago

Hey @aliemir πŸ‘‹πŸ» Thank you for the solution! I've tried it as global one (by adding to UnsavedChangesNotifier) and also on component level. While I haven't encountered any conflicts with navigator.go from usePrompt, there seems to be a problem with back button on the page, not browser one. If you edit any field and then click on this button you get redirected to the same url since pushState made a temporary duplicate. So we fix one thing and break another πŸ˜… Header props can be overridden e.g. headerProps={{ onBack: () => ... }}, but TBH it seems as too much hassle for such a feature.

I think this solution might be satisfactory to use only for certain components, but not as a global one. So I wouldn't introduce this behavior as breaking change, unless it depends on some prop, just as you mentioned.

Another suggestion would be to postpone this issue (maybe convert to discussion for now) until the new data router from React Router V6 is fully supported and then just rely on their features like https://reactrouter.com/en/main/hooks/use-prompt instead of current usePrompt workaround.