reimplement unsaved warning

[huss]

Describe the bug

Before RTK, OED used UnsavedWarning (searching for this will show where) to let the admin know when they might lose a change because it was unsaved. This was commented out to add in later after the basic RTK conversion was done.

Expected behavior

These warning should be re-implemented and enabled in appropriate files.

OED currently does not, in general, use this in the new modal pages with cards. Thus, it is possible this will go away at some point. However, OED should decide if it should be used in some cases.

Additional context

None

[gyordong]

Hi Steve, can @aduques and I work on this issue?

[huss]

Hi Steve, can @aduques and I work on this issue?

@gyordong & @aduques You are welcome to work on this. I'm not sure how easy/hard it will be with the transition to using the Redux Toolkit. If it can be made to work we can discuss how it might be used with the modals. For example, would the warning pop up if the modal is closed (such as clicking off the modal) and there are unsaved changes? Please let me know if you need anything.

[aduques]

As it has been a couple of weeks since looking at this issue, I would like to verify the intended behavior of this feature. There is currently an unsaved warning that appears whenever the user tries to leave the Admin Settings page with unsaved changes. The pop-up can be closed out when clicking anywhere on the screen or by clicking “Cancel,” “Leave,” or “Save All.” This is currently implemented using React. This issue requires us to revert back and implement the unsaved warning feature using the corresponding Redux reducers, which have been commented out. Once the unsaved warning feature in the Admin Settings page is implemented with Redux, should similar pages, such as MapCalibrationComponent and MapsDetailComponent, have the same functionality and implementation?

At the moment, my partner and I will look to see how converting back to Redux can be done by looking through previous commits that have the Redux implemented and figure out how it can be carried over to the current version.

[huss]

Overall, I think what you stated is correct. It seems the src/client/app/components/UnsavedWarningComponent.tsx has had some changes so I'm not sure what might be needed to make it RTK compatible. src/client/app/types/redux/unsavedWarning.ts seems old but I'm unsure where that is used. src/client/app/components/HeaderButtonsComponent.tsx is involved and uses the old style in src/client/app/types/redux/actions.ts. I don't know if this helps but it is what I readily found.

As for maps, PR #1314 upgrades this to modals and RTK usage. It would be the starting point for any other changes. I am behind in getting that reviewed.

Finally, OED could add warnings to the modal pages for all the admin edits to avoid losing edits. It might be nice to do if it isn't too hard.

Please let me know if you need any other info or I can help.

[aduques]

After going through the commit history of the UnsavedWarningComponent.tsx, unsavedWarning.ts, and PreferencesComponent, I have come to an understanding of the following:

• The early implementation of the unsaved warning was implemented using both React and Redux.

• From commit e30fb1d to 72f111b, the implementation of the UnsavedWarningComponent changes from Redux to React. Commit e30fb1d indicated that there was an issue with the implementation of Redux, but from what I could understand, the commit itself addressed the issue.

• In commit 72f111b, In PreferencesComponent.tsx, the PreferencesProps were removed, and its contents were added to the adminSlice. This would include the method that initially handled the unsaved warning, but this specific method was not added to the adminSlice. Instead, the unsaved warning was implemented using React.

It appears that the initial decision to have the unsaved warning method excluded from the admin slice and be implemented using React was assigned in these issues:
#726
#886

I think I still need to look into this more, but I think that it is possible for the unsaved warning functionality to be implemented in the same manner as the other methods that have been moved from PreferencesProps to the adminSlice, as done in commit 72f111b.

For this to be possible, I would need to start investigating how the current adminSlice functions to determine how the changes can be made and to also confirm if the adminSlice is implemented with Redux.

[ChrisMart21]

• The early implementation of the unsaved warning was implemented using both React and Redux.

• From commit e30fb1d to 72f111b, the implementation of the UnsavedWarningComponent changes from Redux to React. Commit e30fb1d indicated that there was an issue with the implementation of Redux, but from what I could understand, the commit itself addressed the issue.

• In commit 72f111b, In PreferencesComponent.tsx, the PreferencesProps were removed, and its contents were added to the adminSlice. This would include the method that initially handled the unsaved warning, but this specific method was not added to the adminSlice. Instead, the unsaved warning was implemented using React.

One of the primary reasons for the omission/rework of this feature, was due to the pre-rtk implementation requiring storage of function(s)/callback(s) in the store. Functions are not plain/serializable data which is a cardinal rule that the Redux apps should abide by if possible.
The original RTK migration attempted to squash many similar issues, but due to time constraints some features had to be left to a future commit.

UnsavedWarningComponent.tsx Is a solution I was experimenting with to utilize the some of the newer features from the react-router v6 bump that was also included in the inital rtk migration. IIRC I thought redux could be bypassed completely to fix/avoid the serializable issue, and simplify it a bit. It has some potential to be generic enough to be used anywhere a warning is needed, but it wasn't fleshed out completely due to time constraints. It should be a good enough starting point, or even inspiration for a better solution.

[huss]

Thanks to @ChrisMart21 for the insights. Those working on this should let us know if you need more information/help.

[aduques]

The reason as to why React and Redux are used together is because React simplifies the issue that Redux has when trying to store non-serializable data. Storing non-serializable data can cause problems later on, so using React as a temporary solution covers that.

My plan for making the change from React to Redux is the following. I want to reimplement the React code that compares the local state of the preferences with the initial state by moving this functionality as an action in the adminSlice's reducer. However, doing so would inherently cause issues due to Redux's ability of not being able to support storing non-serializable data. To bypass this problem, I have done research and found out it is possible to make use of Redux's serializableCheck option to allow non-serializable data to be stored.

I'm aware that changing the serializableCheck option in Redux presents a lot of risks if not handled correctly. With my current understanding, I think that the current implementation of having Redux handle storing non-serializable data and React handling the routing is a much more safer option. I would like to clarify and ask again if serializableCheck is a potential solution that might work.

About serializableCheck from the Redux ToolKit documentation: https://redux-toolkit.js.org/usage/usage-guide#working-with-non-serializable-data

My next steps, if plausible:

• I want to take the React code from PreferencesComponent.tsx and make it as an action that will be added to the adminSlice's reducers in adminSlice.ts.

• Once added to adminSlice, I want to update the store's serializableCheck to handle this new action in adminSlice's reducer. By doing so, it would address the storing non-serializable data issue that Redux has.

• Once this implementation works for PreferencesComponent.tsx, I can implement the same unsaved warning feature to the relevant Component pages by updating the HTML code to take in the UnsavedWarningComponent.

• For the UnsavedWarningComponent.tsx, its implementation is done in React, but I think it would be possible to transition to Redux by following the same plan. However, I would need to look further into this.

Please let me know if there is anything that I'm misunderstanding or other factors I should be considering.

[huss]

Thanks to @aduques for continued efforts on this issue. I'm not an expert here so we can see if @ChrisMart21 has any thoughts.

I tried to find out if using serializableCheck would have a performance impact. I did not find info but did not easily see anything that indicates it would. During this search I found this discussion on addressing the non-serializable issue: reduxjs/redux-toolkit#1292. You have have already seen it but I wanted to pass it along.

[aduques]

Thank you for sharing the discussion as I have not read this one before. I have seen this suggestion (Put the logic related to the non-serialized object in a custom Redux middleware) often and it would be helpful for me to look more into this. Relying on a Redux middleware seems to be a way to handle non-serializable data.

According to the discussion, the recommended solution (Custom hook + global state) seemed to work best, but from my understanding, this solution would use both React and Redux. The custom hook itself would be implemented using Redux, but for the reducers in the adminSlice to call the function, React is still required.

Both of these solutions seem to require modifying serializableCheck in some way, so I will need to look more into it.

[ChrisMart21]

Much work was done to maintain serializable state, and to be able to safely enable the serializable dev check. I think it would be a step back, & in the wrong direction to put unserializable data in the store. It's been a while since I've looked into this but typically there's a flaw in the approach if we need unserializable data in the store.