-
Notifications
You must be signed in to change notification settings - Fork 10
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
CHI-2311: POC - Do not merge - persist redux locally #1936
base: master
Are you sure you want to change the base?
Conversation
… load on initialise. Remove use of Set types from redux state
…in localStorage is invalidated on upgrade / rollback
In my experience, persisting certain parts of Redux state to local storage can be beneficial in specific scenarios, such as storing user preferences that rarely change and are not ideal for database storage. However, it's important to approach this method with caution. Persisting complex state to local storage leads to challenges like client-side application lockups that aren't resolved by a simple page refresh, potential security risks on shared computers and around local storage in general, unexpected behavior when opening multiple tabs, and the complexity of managing cache invalidation. If you're considering this approach, here are some strategies that may help mitigate some of the potential issues:
|
Oh...
|
Thanks for this - whilst I had mitigations in mind for the other things you mentioned, I hadn't considered the security implications. One thing we could do is only persist the routing & the drafts / metadata of case & contacts - which would generally only hold the stuff that's rendered to the screen anyway, so could be scraped by an XSS And sorry for the naïve question - but why is session / locally stored data any more vulnerable to being read via an XSS attack than the redux state already held in memory? The persisted state would never hold more than what's already loaded As for the wierd multi tab behaviour -I deliberately only load state at startup, then only save after that - which means you could never be sure which tab's state you would 'resume' if you closed them both at the same time, but at least you wouldn't see weird random updates in multiple tabs. Also - the multi tab experience in Flex is already far from great :-) |
The redux state is susceptible to XSS attack in the context of the memory of the currently running page/window. The session store and even more so local storage since it persists forever are susceptible to XSS attack from code that is served from anywhere in the current domain. Since we are twilio hosting flex and all of our code is served from https://flex.twilio.com/, any other twilio hosted flex application can access local storage. It can't even really be considered XSS vulnerability in our case since an attacker could spin up their own twilio hosted flex application, use phishing or other social engineering techniques to get users to open their app, and then use that app to harvest whatever they want out of local storage. If we were self hosting flex at a domain that we control and were totally sure that there is no avenue for attack (are we using clickable user generated links anywhere? We are currently using |
I like this idea a lot |
Saving anything that might include PII or other sensitive data to local storage scares the hell out of me. Let's please not do that. Is the problem that we're trying to solve that people can lose data across a refresh? I suppose I'd be a little worried that adding on local storage and the potential for "glitches" that could appear with that might be equal in pain to the current situation of losing context/routing on refresh. I've seen apps in previous companies get borked due to local storage issues and one of the web devs had to come over and manually fix them, and I would be wary about introducing something similar. Though I don't really have a good sense of the tradeoffs... it might be that capturing routing info/metadata in local/session storage might be worth the added complexity if it solves a lot of user pain. |
The one thing we would have to do if we were to go down a partial loading route would be to move to a move to a more 'lazy load' type pattern for resources. Currently there are places in the code that assume certain resources must be loaded up in redux for you to have gotten there. This holds true if we persist the whole state, but not if we only persist parts of it (or if we move to a standard react routing model where we can describe any location in the UI with a URL). In those cases we cannot assume any state is present in any part of the UI. In that scenario, it makes more sense to have lazy load type actions, where the component just requests resources and the redux layer can serve them from the local store if present or request them from the backend if not, that way it can always load the data for a given part of the UI regardless of the state of the redux store. It's a model I'd like to move to in the medium term now we have the redux store configured to support it (the resources state already uses this approach), but these sort of 'restore any state' enhancements would make it a necessity |
There are also options like https://www.npmjs.com/package/redux-persist-transform-encrypt when we have the ability to customise our middlewares |
@annaliseirby090 - the other thing to bear in mind is that we now have the reminder dialog when you try to close / refresh a browser window with unsaved data - might that make this feature less important? |
Yes! I started down this path for profiles. This is generally the responsibility of a hook that can useEffect in one of the more common patterns in modern react. That hook can live in the state domain (like they do right now for profiles), but it is probably best not to try to create redux getter functions that trigger actions based on the current state. (I'm making some assumptions here about what you have planned, forgive me if I'm off-base). The other side of the coin is that you need to start dealing with "partial loaded" resources when list response from the backend only contain part of the needed data, but they stick the partial data into the same resource entry in state that the detail view uses. I would suggest that emulating the pattern that SWR and react-query both use using redux can be useful. The component that needs data shows the data that it currently has (if any partial data was cached during the list view or if the the resource has been loaded before) and regardless of the state it fires requests to the backend to update on mount and rehydrates based on the response. This makes navigation around the site feel fast (no giant loading spinners... the data fills in as it loads) and keeps the displayed data eventually consistent with what is stored on the backend. It does require keeping your components tightly coupled to the data that they display (see the division of the profile components for an example of how fine this divisions needs to be) |
Description
Checklist
Related Issues
CHI-2311
Verification steps