Migrate to use redux-remember instead of redux-persist#4379
Conversation
|
@zehata is attempting to deploy a commit to the modsbot's projects Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4379 +/- ##
==========================================
+ Coverage 54.52% 58.34% +3.81%
==========================================
Files 274 319 +45
Lines 6076 7312 +1236
Branches 1455 1803 +348
==========================================
+ Hits 3313 4266 +953
- Misses 2763 3046 +283 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- This was squashed from a number of commits from https://github.com/zehata/nusmods/tree/redux-remember-test
- Previous state during rehydration is not the default state, but rather the rehydrated state itself
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
|
@greptileai review |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@greptileai review |
|
Sorry, it's been a while since I worked on this, but if I remember correctly there was some issue related to #4383 that is blocking this. If I remember correctly, there was an edge case where the rehydrate actions can contain a function in the object which causes it serialization to fail. I should also mention, this was written awhile back when the repo first moved to using vitest, and a lint message had made me think that the repo moved to React Compiler, which is the reason why I left out the memo and callback. |
|
Thanks, sorry for walking all over your PR - I thought it was about ready to merge + it is the end of semester. Usually for these kinds of PRs I just fix the small issues then merge. If you prefer I can reset this to before I made any changes. Given that this PR is blocked, you prefer to re-do this migration in another PR or leave this here till we confirm a solution in #4383? |
|
The behavioral changes look fine actually, and I really should have changed it before but I was working on something else. I was planning to take a look through again. There were some refactoring that I was thinking might need to be done, including moving the rehydrate gate component to maybe a file together with the rest of the store logic. Let me quickly check through it, I actually meant for this to be done before #4387 so that we can use remigrate's migration logic, so it's actually better to get this done first. |
|
|
||
| const App: FC<PropsWithChildren<Props>> = ({ store }) => { | ||
| const onBeforeLift = () => { | ||
| const onBeforeLift = React.useCallback(() => { |
There was a problem hiding this comment.
There is nothing wrong with making this a callback but doing so is actually redundant. App never gets rerendered, I think. I added a console log and it only gets logged on initial page load.
- With changing `composeEnhancers` to `compose` in `configureStore.ts` which enables the RTK extension by default, there is no longer a need for this flag.
- I was thinking about whether we should document the function of rehydrategate to point to https://redux-remember.js.org/usage/rehydration-gate/ but I think it should be obvious enough? Let me know whether to add it
@ravern Yea, let's. I don't mind resolving the merge conflicts once #4383 is fixed, do you prefer if I rebase this onto the fixed trunk? I won't delete the branch just yet, of course, do you want to close this PR now or KIV it? Don't worry, it's not blocking me. I'm actually working on #4387 (reply in thread) so this can wait |
|
I am resolving the conflicts |
|
T-Rex pricing update — T-Rex was free through June 2026. Effective July 1, 2026, T-Rex adds 2 credits on top of the standard 1-credit review (3 total). T-Rex settings |
| const persistor = persistStore(store, undefined, () => { | ||
| initStateWithPrevTab(store); | ||
| }); | ||
| initStateWithPrevTab(store); |
There was a problem hiding this comment.
initStateWithPrevTab is called immediately after store creation, before REMEMBER_REHYDRATED has fired. The old code guarded against this with an explicit comment:
This must run from the redux-persist bootstrapped callback rather than immediately: [...] adopting a peer's full snapshot (RECEIVE_INIT_STATE) before local REHYDRATE settles would let a later REHYDRATE overwrite persisted slices (timetables/theme) while leaving the peer's non-persisted undoHistory in place — pairing the peer undo stack with stale local state.
That race is fully reintroduced here. If a peer tab responds to initStateWithPrevTab with RECEIVE_INIT_STATE before REMEMBER_REHYDRATED fires on this tab, the local rehydration will subsequently overwrite the peer's persisted slices (timetables, settings, theme) while leaving the peer's undoHistory in place — exactly the broken mixed state the comment described. The fix is to defer the call until state.reduxRemember.isRehydrated is true, e.g. via a one-shot store.subscribe unsubscriber.
Prompt To Fix With AI
This is a comment left during a code review.
Path: website/src/bootstrapping/configure-store.ts
Line: 73
Comment:
**Race condition: cross-tab state sync before rehydration**
`initStateWithPrevTab` is called immediately after store creation, before `REMEMBER_REHYDRATED` has fired. The old code guarded against this with an explicit comment:
> *This must run from the redux-persist bootstrapped callback rather than immediately: [...] adopting a peer's full snapshot (RECEIVE_INIT_STATE) before local REHYDRATE settles would let a later REHYDRATE overwrite persisted slices (timetables/theme) while leaving the peer's non-persisted undoHistory in place — pairing the peer undo stack with stale local state.*
That race is fully reintroduced here. If a peer tab responds to `initStateWithPrevTab` with `RECEIVE_INIT_STATE` before `REMEMBER_REHYDRATED` fires on this tab, the local rehydration will subsequently overwrite the peer's persisted slices (timetables, settings, theme) while leaving the peer's `undoHistory` in place — exactly the broken mixed state the comment described. The fix is to defer the call until `state.reduxRemember.isRehydrated` is `true`, e.g. via a one-shot `store.subscribe` unsubscriber.
How can I resolve this? If you propose a fix, please make it concise.
…d `initStateWithPrevTab` by calling `initStateWithPrevTab` during unserialization
|
@ravern I'm a bit confused about the call chain of the actions because I am not exactly sure how nusmods/website/src/bootstrapping/configure-store.ts Lines 69 to 71 in db1e724 the persistStore accepts the callback () => initStateWithPrevTab(store) which then gets ran in redux-persist/persistStore as boostrappedCb() after rehydration
export default function persistStore(
store: Object,
options?: ?PersistorOptions,
cb?: BoostrappedCb
): Persistor {
// ...
// redacted for brevity
// ...
// dispatch to `store` to rehydrate and `persistor` to track result
store.dispatch(rehydrateAction)
_pStore.dispatch(rehydrateAction)
if (boostrappedCb && persistor.getState().bootstrapped) {
boostrappedCb()
boostrappedCb = false
}With the change to |
|
Also, I will be closing this PR, rebasing this branch and opening this as a separate PR to just clean up the history |
2 participants
Context
redux-persist is last committed to in 2021.
Objectively speaking, this is adding technical debt dating back to 2020: 4b484cf.
nusmods/website/src/reducers/timetables.ts
Lines 38 to 41 in 9e0e84f
nusmods/website/src/reducers/timetables.ts
Lines 47 to 50 in 9e0e84f
nusmods/website/src/reducers/moduleBank.ts
Lines 137 to 140 in 9e0e84f
With the push to move towards agentic involvement, #4314, there may be a need to migrate over to a more recent solution.
To be quite honest, I didn't spend too long looking for an alternative, and redux-remember is the only one I have found. You should try looking for alternatives to redux-persist and redux-remember to decide if this is a prudent replacement.
Implementation
WIP
Note that I am not removing the redux-persist key-values in localStorage, just in case something goes wrong and we need to rollback. It will be trivial to remove in a future PR, perhaps along with the migration code.
Other Information
Obviously, with such a fundamental change to how we are storing user data, I think it is only prudent if we wait until the end of the semester.