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

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

Closed
tlkv opened this issue Jun 20, 2024 · 3 comments
Closed

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

tlkv opened this issue Jun 20, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@tlkv
Copy link

tlkv commented Jun 20, 2024

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.

@tlkv tlkv added the bug Something isn't working label Jun 20, 2024
@aliemir
Copy link
Member

aliemir commented Jun 20, 2024

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
Copy link
Author

tlkv commented Jun 25, 2024

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.

@aliemir
Copy link
Member

aliemir commented Jul 22, 2024

Hey @tlkv, thank you for your comment! Discussed further with the Refine team and we've decided to postpone until we've settled with the new features of React Router, then we'll be able to fix this behavior without using some hacky workarounds.

While acknowledging the issue, closing this for now. We'll re-open and update the issue when we have it cleared in our roadmap.

@aliemir aliemir closed this as not planned Won't fix, can't repro, duplicate, stale Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants