Details

Reviewers

tomek
inka
• przemek
ashoat

Commits

rCOMM5fb26991e4cf: [native] track socket crashes and fetch data/logout user when needed

Summary

Follow up to: https://phab.comm.dev/D6093#182947.
Tracking socket crashes, and when there are 2 in a row, when policy was acknowledged previously (updatesCurrentAsOf === 0), and there is a stable connection (connectivity) we fix this by:

calling fetchNewCookieFromNativeCredentials for regular users
logout and showing modal for SIWE users

Test Plan

Make sure user doesn't have accepted policies
Log in (modal should be visible)
Crash socket connection (e.g close socket on keyserver)
Accept policy
Modal should disappear, and there should be empty app with disconnected bar
For username/password user after a while data should be fetched
SIWE user should be logged-out with Alert explaining reason

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Jan 12 2023, 6:05 AM

kamil held this revision as a draft.

Herald added subscribers: atul, ashoat. · View Herald TranscriptJan 12 2023, 6:05 AM

kamil added a parent revision: D6094: [keyserver] disable injecting redux state when policies are not accepted.Jan 12 2023, 6:06 AM

Harbormaster completed remote builds in B15296: Diff 20858.Jan 12 2023, 6:19 AM

kamil published this revision for review.Jan 12 2023, 6:41 AM

ashoat requested changes to this revision.Jan 12 2023, 9:56 AM

ashoat added inline comments.

lib/socket/socket.react.js
125 ↗	(On Diff #20858)	This only counts failures after policy acknowledgment, yeah? Can we use a more specific name?
644 ↗	(On Diff #20858)	Does `await undefined` cause any problems if it's not set? What happens if `refetchUserData` fails? What happens if it throws an exception? What happens if it fails silently?
native/selectors/account-selectors.js
37 ↗	(On Diff #20858)	Don't need this return statement, just use a shorthand
native/socket.react.js
95 ↗	(On Diff #20858)	Can we use `accountHasPassword` instead? You seem to have reimplemented it here
106 ↗	(On Diff #20858)	No reason for this
112 ↗	(On Diff #20858)	Wonder if we should"dedup" this with the other time it's called in `lib/socket/socket.react.js`, eg. maybe we should have a function `attemptToResolveDeauthorization` that just takes `LogInActionSource`, and then maybe we don't need to pass `urlPrefix` into `Socket` anymore
116 ↗	(On Diff #20858)	This is not an accurate source for this action We should never have the same source twice. The point of the sources is to distinguish which line triggered the log-in. If we reuse them, they are basically pointless
147–148 ↗	(On Diff #20858)	Do we need this on `web` as well?

This revision now requires changes to proceed.Jan 12 2023, 9:56 AM

update naming, add source, handle refetchUserData exceptions, improve code

kamil added inline comments.Jan 20 2023, 6:07 AM

lib/socket/socket.react.js
125 ↗	(On Diff #20858)	sure, updated to `failuresAfterPolicyAcknowledgment`
644 ↗	(On Diff #20858)	Does await undefined cause any problems if it's not set? I don't think so, but for clarity, I updated code to call this only when `refetchUserData` is defined. What happens if refetchUserData fails? What happens if it throws an exception? Good call, Added `try {} catch() {}` to logout user where this function will throw an exception. The different solution might be resetting the counter and if opening the socket will keep failing trying to call this again but I think logging out user is safer than waiting and hoping it will work the next time. What happens if it fails silently? What do you mean by this? The only thing I can imagine is `fetchNewCookieFromNativeCredentials` returning `null` when `resolveInvalidatedCookie` is not defined but it's web case and here we use it only on native, but maybe I am missing something.
native/socket.react.js
95 ↗	(On Diff #20858)	I missed `accountHasPassword` while searching for sth like this, thanks!
112 ↗	(On Diff #20858)	This sounds reasonable to me, can I create a task to address this later in a separate diff? This one is already pretty complicated
116 ↗	(On Diff #20858)	Added `refetchUserDataAfterAcknowledgment` source
147–148 ↗	(On Diff #20858)	On web we discussed to perform fetching missing data via reloading website by user, that's why this is not needed. I've typed those fields as optional to not pass values instead of `false`/empty-function.

Harbormaster completed remote builds in B15501: Diff 21145.Jan 20 2023, 6:20 AM

Nits inline

lib/socket/socket.react.js
105 ↗	(On Diff #21145)	This prop is listed under "async functions that hit server APIs", which it is not... let's move it up to the bottom of the "Redux state" list
native/socket.react.js
92 ↗	(On Diff #21145)	Can we make this name more clear that it's only used to reconsider from a socket crash loop? eg. `refetchUserDataForSocketCrashLoopRecovery`? Or if you prefer shorter, I think `socketCrashLoopRecovery` would be fine. Open to alternatives