Skip to content
Draft
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 115 additions & 33 deletions pnpm-lock.yaml

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -504,10 +504,7 @@ THEN

it('parses omitted course counts in separate clauses as one-of groups', () => {
const result: PrereqTree = {
and: [
{ or: ['EE2012:D', 'ST2334:D'] },
{ or: ['EE2023:D', 'CG2023:D'] },
],
and: [{ or: ['EE2012:D', 'ST2334:D'] }, { or: ['EE2023:D', 'CG2023:D'] }],
};
expect(
parse(
Expand Down
4 changes: 3 additions & 1 deletion website/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"@material/button": "14.0.0",
"@material/fab": "14.0.0",
"@material/snackbar": "14.0.0",
"@reduxjs/toolkit": "^2.11.2",
"@sentry/browser": "7.119.1",
"@sentry/node": "6.19.7",
"@sentry/tracing": "6.19.7",
Expand Down Expand Up @@ -69,7 +70,8 @@
"react-router-dom": "5.3.4",
"react-scrollspy": "3.4.3",
"redux": "4.2.1",
"redux-persist": "6.0.0",
"redux-remember": "^6.0.2",
"redux-remigrate": "^6.0.6",
"redux-state-sync": "3.1.4",
"redux-thunk": "2.4.2",
"reselect": "4.1.8",
Expand Down
7 changes: 7 additions & 0 deletions website/remigrate.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { defineRemigrateConfig } from 'redux-remigrate';

export default defineRemigrateConfig({
storagePath: './src/remigrate',
stateFilePath: './src/types/state.ts',
stateTypeExpression: 'State',
});
3 changes: 1 addition & 2 deletions website/scripts/vitest-setup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ configureTestingLibrary({ asyncUtilTimeout: 5000 });

configure({ adapter: new Adapter() });

// immer uses Object.freeze on returned state objects, which is incompatible with
// redux-persist. See https://github.com/rt2zz/redux-persist/issues/747
// immer uses Object.freeze on returned state objects, which breaks undo history functionality
setAutoFreeze(false);

// Prevent causing errors during test runs due to unclosed BroadcastChannel
Expand Down
60 changes: 31 additions & 29 deletions website/src/bootstrapping/configure-store.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { createStore, applyMiddleware, compose, PreloadedState } from 'redux';
import { persistStore } from 'redux-persist';
import { applyMiddleware, compose } from 'redux';
import thunk from 'redux-thunk';
import { setAutoFreeze } from 'immer';

Expand All @@ -12,16 +11,15 @@ import getLocalStorage from 'storage/localStorage';
import type { GetState } from 'types/redux';
import type { State } from 'types/state';
import type { Actions } from 'types/actions';
import { configureStore as RTKConfigureStore, StoreEnhancer } from '@reduxjs/toolkit';
import { rememberEnhancer } from 'redux-remember';
import { migrate } from 'remigrate';
import storage from 'storage';

// For redux-devtools-extensions - see
// https://github.com/zalmoxisus/redux-devtools-extension
const composeEnhancers: typeof compose = window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__ || compose;

// immer uses Object.freeze on returned state objects, which is incompatible with
// redux-persist. See https://github.com/rt2zz/redux-persist/issues/747
// immer uses Object.freeze on returned state objects, which breaks undo history functionality
setAutoFreeze(false);

export default function configureStore(defaultState?: State) {
export default function configureStore(defaultState?: State, usePersistence: boolean = false) {
// Clear legacy reduxState deprecated by https://github.com/nusmodifications/nusmods/pull/669
// to reduce the amount of data NUSMods is using
getLocalStorage().removeItem('reduxState');
Expand All @@ -38,37 +36,41 @@ export default function configureStore(defaultState?: State) {
diff: true,
// Avoid diffing actions that insert a lot of stuff into the state to prevent console from lagging
diffPredicate: (_getState: GetState, action: Actions) =>
!action.type.startsWith('FETCH_MODULE_LIST') && !action.type.startsWith('persist/'),
!action.type.startsWith('FETCH_MODULE_LIST') && !action.type.startsWith('@@REMEMBER_'),
});
middlewares.push(logger);
}

const storeEnhancer = applyMiddleware(...middlewares);

const store = createStore(
rootReducer,
// Redux typings does not seem to allow non-JSON serialized values in PreloadedState so this needs to be casted
defaultState as PreloadedState<State> | undefined,
composeEnhancers(storeEnhancer),
);
const store = RTKConfigureStore({
reducer: rootReducer,
preloadedState: defaultState,
enhancers: (getDefaultEnhancers) =>
getDefaultEnhancers().concat(
(usePersistence
? compose(
rememberEnhancer(
storage,
['moduleBank', 'venueBank', 'timetables', 'theme', 'settings', 'planner'],
{
migrate,
serialize: (state, _key) => state,
unserialize: (state, _key) => state,
},
),
Comment thread
greptile-apps[bot] marked this conversation as resolved.
storeEnhancer,
)
: storeEnhancer) as StoreEnhancer,
),
});

if (module.hot) {
// Enable webpack hot module replacement for reducers
module.hot.accept('../reducers', () => store.replaceReducer(rootReducer));
}

// Ask any already-open tab to hand over its current state (including
// non-persisted slices like undoHistory) so cross-tab undo stays in sync.
// This must run from the redux-persist bootstrapped callback rather than
// immediately: persistStore rehydrates asynchronously, and 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. Syncing after bootstrap keeps the
// adopted snapshot internally consistent.
const persistor = persistStore(store, undefined, () => {
initStateWithPrevTab(store);
});
initStateWithPrevTab(store);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

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.


return { persistor, store };
return store;
}
19 changes: 19 additions & 0 deletions website/src/bootstrapping/migrate-persist-to-remember.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { mapValues, omit } from 'lodash';
import storage from 'storage';

test('redux-persist JSON members should be parsed and _persist should be removed', () => {
const mockData = {
maps: {},
arrays: [],
number: 0,
string: '',
_persist: true,
};

const mockDataWithStringifiedMembers = mapValues(mockData, JSON.stringify);

storage.setItem('persist:test_key', mockDataWithStringifiedMembers);

const recoveredData = storage.getItem('@@remember-test_key');
expect(recoveredData).toStrictEqual(omit(mockData, '_persist'));
});
22 changes: 22 additions & 0 deletions website/src/bootstrapping/migrate-persist-to-remember.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import { mapValues, omit } from 'lodash-es';
import { captureException } from 'utils/error';

/**
* Each member in the redux-persist data is stringified, and the entire map is stringified\
* Redux-remember format stringifies the data without stringifying each member\
* This function takes the redux-persist JSON string and converts it to the redux-remember data format\
* @param persistJsonString
* @returns parsed data
*/
const migratePersistToRemember = (persistJsonString: string): any => {
try {
const parsedValue = JSON.parse(persistJsonString);
const data = omit(parsedValue, '_persist');
return mapValues(data, JSON.parse);
} catch (error) {
captureException(error);
return null;
}
};

export default migratePersistToRemember;
10 changes: 4 additions & 6 deletions website/src/entry/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,20 @@ import * as React from 'react';
import type { FC, PropsWithChildren } from 'react';
import { BrowserRouter as Router } from 'react-router-dom';
import { Provider } from 'react-redux';
import { PersistGate } from 'redux-persist/integration/react';
import { Persistor } from 'storage/persistReducer';
import { State } from 'types/state';

import AppShell from 'views/AppShell';
import Routes from 'views/routes/Routes';
import { DIMENSIONS, setCustomDimensions } from 'bootstrapping/matomo';
import ErrorBoundary from 'views/errors/ErrorBoundary';
import ErrorPage from 'views/errors/ErrorPage';
import RehydrateGate from 'storage/RehydrateGate';

type Props = {
store: Store<State>;
persistor: Persistor;
};

const App: FC<PropsWithChildren<Props>> = ({ store, persistor }) => {
const App: FC<PropsWithChildren<Props>> = ({ store }) => {
const onBeforeLift = () => {
const { theme, settings } = store.getState();

Expand All @@ -32,13 +30,13 @@ const App: FC<PropsWithChildren<Props>> = ({ store, persistor }) => {
return (
<ErrorBoundary errorPage={() => <ErrorPage showReportDialog />}>
<Provider store={store}>
<PersistGate persistor={persistor} onBeforeLift={onBeforeLift}>
<RehydrateGate onBeforeLift={onBeforeLift}>
<Router>
<AppShell>
<Routes />
</AppShell>
</Router>
</PersistGate>
</RehydrateGate>
</Provider>
</ErrorBoundary>
);
Expand Down
2 changes: 1 addition & 1 deletion website/src/entry/export/main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ declare global {
}

// Set up Redux store
const { store } = configureStore();
const store = configureStore(undefined, true);
window.store = store;

// For Puppeteer to import data
Expand Down
7 changes: 2 additions & 5 deletions website/src/entry/main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
import 'bootstrapping/sentry';
// core-js has issues with Promise feature detection on Edge, and hence
// polyfills Promise incorrectly. Importing this polyfill directly resolves that.
// This is necessary as PersistGate used in ./App uses `Promise.prototype.finally`.
// See: https://github.com/zloirock/core-js/issues/579#issuecomment-504325213
import 'core-js/es/promise/finally';

import { createRoot } from 'react-dom/client';
import ReactModal from 'react-modal';
Expand All @@ -18,7 +15,7 @@ import 'styles/main.scss';

import App from './App';

const { store, persistor } = configureStore();
const store = configureStore(undefined, true);

subscribeOnlineEvents(store);

Expand All @@ -30,7 +27,7 @@ if (!container) {
throw new Error('#app element not found');
}
const root = createRoot(container);
root.render(<App store={store} persistor={persistor} />);
root.render(<App store={store} />);

if (
((NUSMODS_ENV === 'preview' || NUSMODS_ENV === 'staging' || NUSMODS_ENV === 'production') &&
Expand Down
8 changes: 4 additions & 4 deletions website/src/middlewares/state-sync-middleware.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { OPEN_NOTIFICATION, POP_NOTIFICATION } from 'actions/app';
import type { AnyAction } from 'redux';
import { PERSIST, PURGE, REHYDRATE } from 'redux-persist';
import { REMEMBER_REHYDRATED, REMEMBER_PERSISTED } from 'redux-remember';
import {
createStateSyncMiddleware,
initStateWithPrevTab,
Expand Down Expand Up @@ -28,6 +28,7 @@ export const receiveState = (prevState: State, nextState: State): State => ({
...nextState,
requests: prevState.requests,
app: { ...nextState.app, notifications: prevState.app.notifications },
reduxRemember: prevState.reduxRemember,
});

const reduxStateSyncConfig = {
Expand All @@ -38,9 +39,8 @@ const reduxStateSyncConfig = {
predicate: (action: AnyAction) => {
// Reference: https://github.com/aohua/redux-state-sync/issues/53
const blacklist = [
PERSIST,
PURGE,
REHYDRATE,
REMEMBER_PERSISTED,
REMEMBER_REHYDRATED,
OPEN_NOTIFICATION,
POP_NOTIFICATION,
RECEIVE_INIT_STATE,
Expand Down
2 changes: 1 addition & 1 deletion website/src/reducers/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
TOGGLE_FEEDBACK_MODAL,
} from 'actions/app';

const defaultAppState = (): AppState => ({
export const defaultAppState = (): AppState => ({
// Default to the current semester from config.
activeSemester: config.semester,
// The lesson being modified on the timetable.
Expand Down
66 changes: 33 additions & 33 deletions website/src/reducers/index.ts
Original file line number Diff line number Diff line change
@@ -1,32 +1,27 @@
import { Reducer } from 'redux';
import { AnyAction } from 'redux';

import { REMOVE_MODULE, SET_TIMETABLE } from 'actions/timetables';

import persistReducer from 'storage/persistReducer';
import { withReduxStateSync, receiveState } from 'middlewares/state-sync-middleware';
import { State } from 'types/state';
import { Actions } from 'types/actions';

// Non-persisted reducers
import requests from './requests';
import app from './app';
import createUndoReducer from './undoHistory';
import createUndoReducer, { defaultUndoHistoryState } from './undoHistory';

// Persisted reducers
import moduleBankReducer, { persistConfig as moduleBankPersistConfig } from './moduleBank';
import venueBankReducer, { persistConfig as venueBankPersistConfig } from './venueBank';
import timetablesReducer, { persistConfig as timetablesPersistConfig } from './timetables';
import moduleBankReducer from './moduleBank';
import venueBankReducer from './venueBank';
import timetablesReducer from './timetables';
import themeReducer from './theme';
import settingsReducer, { persistConfig as settingsPersistConfig } from './settings';
import plannerReducer, { persistConfig as plannerPersistConfig } from './planner';

// Persist reducers
const moduleBank = persistReducer('moduleBank', moduleBankReducer, moduleBankPersistConfig);
const venueBank = persistReducer('venueBank', venueBankReducer, venueBankPersistConfig);
const timetables = persistReducer('timetables', timetablesReducer, timetablesPersistConfig);
const theme = persistReducer('theme', themeReducer);
const settings = persistReducer('settings', settingsReducer, settingsPersistConfig);
const planner = persistReducer('planner', plannerReducer, plannerPersistConfig);
import settingsReducer from './settings';
import plannerReducer from './planner';
import { rememberReducer } from 'redux-remember';
import reduxRemember from './reduxRemember';
import { UndoHistoryState } from 'types/reducers';
import { combineReducers } from 'redux';

// State default is delegated to its child reducers.
const defaultState = {} as unknown as State;
Expand All @@ -36,26 +31,31 @@ const undoReducer = createUndoReducer<State>({
storedKeyPaths: ['timetables', 'theme.colors'],
});

function reducers(state: State = defaultState, action: Actions): State {
// Update every reducer except the undo reducer
const newState: State = {
moduleBank: moduleBank(state.moduleBank, action),
venueBank: venueBank(state.venueBank, action),
requests: requests(state.requests, action),
timetables: timetables(state.timetables, action),
app: app(state.app, action),
theme: theme(state.theme, action),
settings: settings(state.settings, action),
planner: planner(state.planner, action),
undoHistory: state.undoHistory,
};
const reducers = {
moduleBank: moduleBankReducer,
venueBank: venueBankReducer,
requests,
timetables: timetablesReducer,
app,
theme: themeReducer,
settings: settingsReducer,
planner: plannerReducer,
reduxRemember: reduxRemember.reducer,
// State members are required to have a reducer
// The reducer is required to return a state, but the history reducer runs after state reducer
// Thus we initialize undo history state if it was uninitialized
undoHistory: (state: UndoHistoryState<State> = defaultUndoHistoryState, _action: Actions) =>
state,
};

const reducer = rememberReducer(combineReducers(reducers));

export function rootReducer(state: State = defaultState, action: AnyAction): State {
const newState = reducer(state, action);
return undoReducer(state, newState, action);
}

// withReduxStateSync intercepts RECEIVE_INIT_STATE (from initStateWithPrevTab)
// and adopts the peer tab's state, including non-persisted slices like
// undoHistory so undo/redo stay in sync across tabs.
export default withReduxStateSync(
reducers as unknown as Reducer<State>,
receiveState,
) as unknown as Reducer<State, Actions>;
export default withReduxStateSync(rootReducer, receiveState);
Loading