Page MenuHomePhabricator
Differential D5571

[native] Alert staff/developers if `REHYDRATE` action payload isn't as expected
ClosedPublic
Actions

Authored by atul on Nov 8 2022, 2:27 PM.
Tags
None
Referenced Files
F3361425: D5571.id18283.diff
Sun, Nov 24, 5:25 PM
F3361108: D5571.diff
Sun, Nov 24, 3:23 PM
F3358883: D5571.id18283.diff
Sun, Nov 24, 6:31 AM
F3358882: D5571.id18276.diff
Sun, Nov 24, 6:31 AM
F3358881: D5571.id18275.diff
Sun, Nov 24, 6:31 AM
F3358880: D5571.id18238.diff
Sun, Nov 24, 6:31 AM
F3358879: D5571.id.diff
Sun, Nov 24, 6:31 AM
Unknown Object (File)
Thu, Nov 21, 5:15 AM
View All Files
Subscribers
• abosh

Details

Reviewers
ashoat
tomek
marcin
varun
ginsu
rohan
Commits
rCOMM3201ebe45589: [native] Alert staff/developers if `REHYDRATE` action payload isn't as expected
Summary

Context: https://linear.app/comm/issue/ENG-2127/

We want to make sure that the payload we get from the REHYDRATE action has all the keys that we expect.

In this diff we add a check to redux-setup:reducer(...) for staff/developers that pulls the keys out of the REHYDRATE payload and makes sure that it matches what we expect (keys in defaultState MINUS those in persistConfig.blacklist)

Test Plan

Ensured that the alerts displayed as expected by modifying logic. Also logged values and checked that they were as expected.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

atul created this revision.Nov 8 2022, 2:27 PM
Herald added a subscriber: • abosh. · View Herald TranscriptNov 8 2022, 2:27 PM
atul published this revision for review.Nov 8 2022, 2:29 PM
atul edited the summary of this revision. (Show Details)
ashoat added inline comments.Nov 8 2022, 2:39 PM
native/redux/redux-setup.js
142–146 ↗(On Diff #18238)

We should probably factor this out... do we have a task to track?

Also, realizing my idea of storing the state of "has the user ever been logged in?" in a React context is a bad idea if we ever want to use it in a reducer... probably should just keep it in Redux somewhere

156 ↗(On Diff #18238)

Hmmmmm this worries me. Is there some rehydrate-failed state where action.payload isn't set? Would rather pull the user up to the login screen than eg. put the app in a crash loop or something

177–181 ↗(On Diff #18238)

Do both OSs handle double-Alerts gracefully?

Harbormaster completed remote builds in B13294: Diff 18238.Nov 8 2022, 2:41 PM
atul added inline comments.Nov 8 2022, 2:48 PM
native/redux/redux-setup.js
142–146 ↗(On Diff #18238)

We use the useStaffCanSee(...) hook elsewhere, but I don't think we can use that here?

156 ↗(On Diff #18238)

Could I instead do Object.keys(action.payload ?? {});?

I'll take a look through the codebase

177–181 ↗(On Diff #18238)

Works fine on iOS, I'll flip conditions so they both alway trigger and check on Android Emulator

ashoat added inline comments.Nov 8 2022, 2:49 PM
native/redux/redux-setup.js
142–146 ↗(On Diff #18238)

If we kept it in Redux we wouldn't need a hook

atul added inline comments.Nov 8 2022, 2:53 PM
native/redux/redux-setup.js
142–146 ↗(On Diff #18238)

I can create a task to move staffUserHasBeenLoggedIn from a React.Context to Redux, but is there anything actionable for this diff?

ashoat added inline comments.Nov 8 2022, 3:12 PM
native/redux/redux-setup.js
142–146 ↗(On Diff #18238)

No, just the task would be great. Let's mention:

  1. We should move the state from React Context to Redux
  2. We should update the hook to check from Redux
  3. We should update this location and any others that couldn't previously use the hook to use the new state in Redux
ashoat requested changes to this revision.Nov 8 2022, 3:13 PM

Back to you

This revision now requires changes to proceed.Nov 8 2022, 3:13 PM
atul added inline comments.Nov 9 2022, 7:35 AM
native/redux/redux-setup.js
142–146 ↗(On Diff #18238)

Created the Linear task: https://linear.app/comm/issue/ENG-2208/move-staffuserhasbeenloggedin-from-reactcontext-to-redux

atul updated this revision to Diff 18271.Nov 9 2022, 7:42 AM

remove invariant... about to test multiple alerts on Android

Harbormaster failed remote builds in B13321: Diff 18271!Nov 9 2022, 7:43 AM
Harbormaster completed remote builds in B13321: Diff 18271.Nov 9 2022, 7:56 AM
ashoat accepted this revision.Nov 9 2022, 8:13 AM

Looks good pending testing

This revision is now accepted and ready to land.Nov 9 2022, 8:13 AM
atul added a comment.EditedNov 9 2022, 9:08 AM
In D5571#165428, @atul wrote:

remove invariant... about to test multiple alerts on Android

an hour and 26 minutes later, can confirm that things work as expected on Android:

atul attached a referenced file: F229436: Screen Recording 2022-11-09 at 12.09.13 PM.mov. (Show Details)Nov 9 2022, 9:11 AM
atul updated this revision to Diff 18275.Nov 9 2022, 9:12 AM

rebase before landing

Harbormaster completed remote builds in B13324: Diff 18275.Nov 9 2022, 9:27 AM
atul updated this revision to Diff 18276.Nov 9 2022, 10:01 AM

isStaffRelease export in staff-utils

Harbormaster completed remote builds in B13325: Diff 18276.Nov 9 2022, 10:16 AM
Closed by commit rCOMM3201ebe45589: [native] Alert staff/developers if `REHYDRATE` action payload isn't as expected (authored by atul). · Explain WhyNov 9 2022, 10:19 AM
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM3201ebe45589: [native] Alert staff/developers if `REHYDRATE` action payload isn't as expected.

Revision Contents
Changeset List

PathSize
native/
redux/
50 lines
utils/
2 lines

Diff 18283

View Options

native/redux/redux-setup.js

Loading...
View Options

native/utils/staff-utils.js

Loading...