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] Warn unsaved changes when there are no changes #6691

Open
jpenna opened this issue Feb 25, 2025 · 10 comments · May be fixed by #6702
Open

[BUG] Warn unsaved changes when there are no changes #6691

jpenna opened this issue Feb 25, 2025 · 10 comments · May be fixed by #6702
Labels
bug Something isn't working

Comments

@jpenna
Copy link

jpenna commented Feb 25, 2025

Describe the bug

When setting warnWhenUnsavedChanges: true, if the user changes a field but then undo the change and try to navigate to another page, the warn modal pops up even if there aren't no real data changes.

Steps To Reproduce

  1. Set warnWhenUnsavedChanges: true in <Refine />
  2. Create a resource and go to edit it
  3. Change a field
  4. Undo the change
  5. Try to navigate to another page
  6. The warn will pop up

Expected behavior

Should check the actual values for changes and show the modal only if there are actual changes to the data.

Packages

  • "@refinedev/cli": "^2.16.37",
  • "@refinedev/core": "^4.54.0",
  • "@refinedev/kbar": "1.3.12",
  • "@refinedev/mui": "^5.20.0",
  • "@refinedev/react-hook-form": "^4.9.0",
  • "@refinedev/react-router-v6": "^4.6.0",
  • "@refinedev/react-table": "^5.6.13",
  • "@refinedev/simple-rest": "^5.0.8",
  • "@tanstack/react-table": "^8.2.6",

Additional Context

No response

@jpenna jpenna added the bug Something isn't working label Feb 25, 2025
@Varun789-mx
Copy link

Varun789-mx commented Feb 28, 2025

i tried to find the issue for about 2 days but could recreate it and the app is working fine maybe i could't recreated the issue here is the proof
video link

@jpenna
Copy link
Author

jpenna commented Feb 28, 2025

Hey @Varun789-mx , you are showing the bug in the video. You removed a space and added it back, so there are no changes, but the warning is popping up saying you have unsaved changes.

@alicanerdurmaz
Copy link
Member

Hello @jpenna, thanks for the issue.

I think we can check React Hook Form's isDirty state and use it to handle warnWhenUnsavedChanges.

@mohit-2003
Copy link

Hello @alicanerdurmaz, Can i work on this?

@alicanerdurmaz
Copy link
Member

Yes @mohit-2003, thanks for improving Refine 🙌

@mohit-2003
Copy link

Hello @jpenna, thanks for the issue.

I think we can check React Hook Form's isDirty state and use it to handle warnWhenUnsavedChanges.

Hi @alicanerdurmaz, I tested isDirty, but it doesn't work as expected. The issue is that isDirty remains true even after reverting changes back to the original values. I think we have to compare old values and new values for this, do you any other idea?

@alicanerdurmaz
Copy link
Member

Hello @jpenna, thanks for the issue.
I think we can check React Hook Form's isDirty state and use it to handle warnWhenUnsavedChanges.

Hi @alicanerdurmaz, I tested isDirty, but it doesn't work as expected. The issue is that isDirty remains true even after reverting changes back to the original values. I think we have to compare old values and new values for this, do you any other idea?

Did you provide defaultValues? react-hook-form says it checks based on defaultValues.

We can solve the issue by checking the old and new values, but I don’t prefer this. react-hook-form already does this, so doing it again would mean extra work and unnecessary performance loss.

@mohit-2003
Copy link

mohit-2003 commented Mar 6, 2025

Hello @alicanerdurmaz,

You were absolutely right! After providing default values, isDirty now works as expected. However, I’m facing an issue where the isDirty state updates after watch().

In onValuesChange, I’m still getting the previous state of isDirty.

As shown in the attached image, I've added A in Content. The watch useEffect is triggered first, where isDirty is false. However, in the UserView example, isDirty is true, as expected.

I’ve done extensive debugging but still haven’t found a way to fix this. Do you have any ideas on this?

Image

mohit-2003 added a commit to mohit-2003/refine that referenced this issue Mar 8, 2025
…edev#6691)

Used useEffect to track isDirty and update setWarnWhen accordingly.
@alicanerdurmaz
Copy link
Member

Hello @alicanerdurmaz,

You were absolutely right! After providing default values, isDirty now works as expected. However, I’m facing an issue where the isDirty state updates after watch().

In onValuesChange, I’m still getting the previous state of isDirty.

As shown in the attached image, I've added A in Content. The watch useEffect is triggered first, where isDirty is false. However, in the UserView example, isDirty is true, as expected.

I’ve done extensive debugging but still haven’t found a way to fix this. Do you have any ideas on this?

Image

@mohit-2003 Is this isDirty state doesn't sync issue still ongoing?

@mohit-2003
Copy link

Hello @alicanerdurmaz,
You were absolutely right! After providing default values, isDirty now works as expected. However, I’m facing an issue where the isDirty state updates after watch().
In onValuesChange, I’m still getting the previous state of isDirty.
As shown in the attached image, I've added A in Content. The watch useEffect is triggered first, where isDirty is false. However, in the UserView example, isDirty is true, as expected.
I’ve done extensive debugging but still haven’t found a way to fix this. Do you have any ideas on this?
Image

@mohit-2003 Is this isDirty state doesn't sync issue still ongoing?

Yes @alicanerdurmaz, I did hours of debugging but didn't find any way to fix this. In my PR #6702, I am watching the isDirty state in a separate useEffect and updating setWarn there. This approach solves the issue.

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

Successfully merging a pull request may close this issue.

4 participants