-
Notifications
You must be signed in to change notification settings - Fork 345
feat(website): Sync state changes across tabs #4174
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
base: master
Are you sure you want to change the base?
feat(website): Sync state changes across tabs #4174
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@TheMythologist is attempting to deploy a commit to the modsbot's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this! The change looks good to me with a minor nit.
Do let one of the other active maintainers @ravern, @jloh02, or @leslieyip02 review it too before merging.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4174 +/- ##
==========================================
- Coverage 54.52% 52.89% -1.64%
==========================================
Files 274 290 +16
Lines 6076 6693 +617
Branches 1455 1633 +178
==========================================
+ Hits 3313 3540 +227
- Misses 2763 3153 +390 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @zwliew I finally got to working on this again, could you help me take a look at the changes and review this PR once more? Thanks! |
jloh02
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi i noticed a bug. steps to reproduce
- Open 2 tabs on https://nusmods-website-mmnouxt0t-modsbots-projects.vercel.app/timetable/sem-1
- Add a course on one tab
- Other tab has the screenshot as below
| } | ||
|
|
||
| // `FETCH_` request actions should not be synced to other tabs | ||
| if (action.type.toString().startsWith('FETCH_')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hook.js:608 TypeError: Cannot read properties of undefined (reading 'find')
at u (modules.ts:30:30)
at d (modules.ts:35:16)
at timetables.ts:168:21
at mapValues.js:38:34
at _createBaseFor.js:17:11
at e.exports (_baseForOwn.js:13:20)
at e.exports (mapValues.js:37:3)
at ue (timetables.ts:167:10)
at timetables.ts:127:14
The problem is that in getModuleSemesterData() of modules.ts, the semesterData field of module is undefined for a module that was synced over from another tab.
Perhaps a fix for this is to whitelist the FETCH_MODULE_REQUEST and FETCH_MODULE_SUCCESS actions, which are there to add module info (like semesterData) for newly added modules.
As an aside, I would be more comfortable with switching over to an action whitelist rather than a blacklist.
Context
Previously, changes to a redux state were not reflected in other tabs. This causes 2 main issues:
Implementation
Uses
redux-state-syncto sync redux states across tabs, which uses a polyfilledBroadcastChannelunder the hood.Other Information
Another option I looked at was
redux-persist-crosstab, which is unmaintained and incompatible with the current version ofredux-persist.Additionally, I had to forcefully resolve
broadcast-channelto version5.3.0.