Sync Logout across tabs #5190
Sync Logout across tabs #5190
Conversation
src/libs/actions/SignInRedirect.js
Outdated
| @@ -47,6 +47,10 @@ function redirectToSignIn(errorMessage) { | |||
| if (activeClients && activeClients.length > 0) { | |||
| Onyx.set(ONYXKEYS.ACTIVE_CLIENTS, activeClients); | |||
| } | |||
|
|
|||
| // We must set the authToken to null so that signOut action is triggered across other clients | |||
| // https://github.com/Expensify/App/issues/4971#issuecomment-916101493 | |||
There was a problem hiding this comment.
NAB, I wonder if we need the link to the issue. If someone is curious about this we can use git blame and hopefully learn why some decision was made.
src/libs/actions/SignInRedirect.js
Outdated
|
|
||
| // We must set the authToken to null so that signOut action is triggered across other clients | ||
| // https://github.com/Expensify/App/issues/4971#issuecomment-916101493 | ||
| Onyx.set(ONYXKEYS.SESSION, {authToken: null}); |
There was a problem hiding this comment.
Isn't this going to overwrite the error message above? Since we are calling Onyx.set() with ONYXKEYS.SESSION if we have an errorMessage and then again here.
There was a problem hiding this comment.
Yeah. I didn't notice. I think I can merge both together.
parasharrajat
force-pushed
the
synglogout
branch
from
c74cd6b to
1d3369d
Compare
| if (errorMessage) { | ||
| session.error = errorMessage; | ||
| } | ||
| Onyx.merge(ONYXKEYS.SESSION, session); |
There was a problem hiding this comment.
Why not keep Onyx.set here, now that we combined error and authToken keys into one object?
There was a problem hiding this comment.
After, Onyx.clear, Onyx is initialized with the initial data https://github.com/Expensify/react-native-onyx/blob/6eadd7f73fca87e6551e43607ec78f645e17ef50/lib/Onyx.js#L618.
set here, was replacing that. So I used merge
Line 30 in e8cdb02
There was a problem hiding this comment.
Aah nice, thanks for pointing that out! 👍
There was a problem hiding this comment.
NAB, it might be worth adding a brief comment here explaining why we're not using a set(). That explanation makes sense to me, but someone might have the innocent idea to change this from a merge() to a set() and break the intended behavior.
parasharrajat
dismissed stale reviews from marcaaron and Beamanator
via
fc4323c
|
🚀 Deployed to staging by @marcaaron in version: 1.0.97-3 🚀
|
|
We found this issue #5245 while testing this. Not sure if it fails the PR or not (I would say no, but I wanted to double check) @parasharrajat @marcaaron @Beamanator |
|
@Beamanator Please let us know your opinion here. I think it is fine until the user is not able to use the other tab to do some actions. |
|
I think it may be happening because the other tab is not logged out in Safari, see #5245 (comment) |
|
Yep, it is because of that. That's why I think this PR is still a pass. Also, not 100% sure if the issue is "real" since it's like a refresh to show the correct user and load the new information. |
Hmm, just did the same exercise again and now both tabs sync properly 🤷 . I just updated my macOS, I wonder if that updated Safari and now it works? 🤷 🤷 🤷 or maybe I was testing with a cached version of the newDot in Safari. |
|
I also tested in Safari today and it looks like this passed 👍 One small thing I noticed: The welcome message on the signin page is different on the 2 tabs: Screen.Recording.2021-09-15.at.1.50.33.PM.mov |
|
Oh I think it's because in one tab you landed in the enter email page and in the other it was the enter password 🤔 |
|
I just tested on my side and it was not reproducible, the sync was a LOT faster 🤔 Edit: I'm gonna check this PR off the list, please stop me if you disagree. |
|
Thanks, guys for proactively testing it. |
|
This has been deployed to production and is now subject to a 7-day regression period. |
|
🚀 Deployed to production by @francoisl in version: 1.0.98-1 🚀
|
Details
#4971 (comment)
Fixed Issues
$ #4971
Tests | QA Steps
Tested On
Screenshots
Web
logout-d.mp4
Mobile Web
Desktop
iOS
Android