This repository has been archived by the owner on Nov 30, 2022. It is now read-only.
1401 admin UI persist redux store to localstorage #1409
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -56,27 +52,6 @@ export const selectToken = (state: RootState) => selectAuth(state).token; | |||
|
|
|||
| export const { login, logout } = authSlice.actions; | |||
|
|
|||
| export const credentialStorage = createListenerMiddleware(); | |||
There was a problem hiding this comment.
Removed the following event listeners given the redux-perist localStorage functionality. Redux-persist localStorage root key is persist-root.
|
|
||
| const { connectionOption, step } = useAppSelector(selectConnectionTypeState); | ||
|
|
||
| /** | ||
| * NOTE: If the user reloads the web page via F5, the react redux store state is lost. |
There was a problem hiding this comment.
The following code snippet was no longer needed given the Redux store is persisted in localStorage.
| @@ -65,7 +66,14 @@ const RequestTable: React.FC<RequestTableProps> = () => { | |||
| isFetching, | |||
| } = useRequestTable(); | |||
|
|
|||
| return ( | |||
| useEffect(() => { | |||
There was a problem hiding this comment.
This resolved a React memory leak at runtime when a user clicks the User icon to logout. Memory leak warning was visible via browser console.
chriscalhoun1974
requested review from
LKCSmith and
ssangervasi
and removed request for
TheAndrewJackson
ssangervasi
suggested changes
Sep 30, 2022
LKCSmith
suggested changes
Sep 30, 2022
There was a problem hiding this comment.
I'm not personally super familiar with this pattern, but maybe @ssangervasi can provide some more useful feedback. Otherwise, I think this looks okay, just one small thing.
LKCSmith
approved these changes
Sep 30, 2022
chriscalhoun1974
deleted the
1401-admin-ui-persist-redux-store-to-localstorage
branch
ssangervasi
added a commit
to ethyca/fides
that referenced
this pull request
Oct 14, 2022
This was driving me crazy debugging while working on the config wizard. The persist code merged in, but the necessity of excluding these reducers was missed: ethyca/fidesops#1409 (comment) I'm actually a bit surprised ctl stuff has been working okay at all. Maye there are some bug issues I haven't seen yet.
chriscalhoun1974
pushed a commit
to ethyca/fides
that referenced
this pull request
Oct 14, 2022
This was driving me crazy debugging while working on the config wizard. The persist code merged in, but the necessity of excluding these reducers was missed: ethyca/fidesops#1409 (comment) I'm actually a bit surprised ctl stuff has been working okay at all. Maye there are some bug issues I haven't seen yet.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Currently, the Admin UI Redux store is persisted in memory by default. This storage mechanism is problematic when a user reloads a browser page. Maintaining state via URL query string parameters is not a scalable solution. In order to provide a better overall user experience, the Redux store must maintain state between user page reloads.
Changes
Checklist
CHANGELOG.mdfileCHANGELOG.mdfile is being appended toUnreleasedsection in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #1401