Page MenuHomePhabricator

[lib][web][native] Stop logging out from the Socket
ClosedPublic

Authored by inka on Dec 14 2023, 4:31 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 4:54 PM
Unknown Object (File)
Sat, Nov 23, 3:39 PM
Unknown Object (File)
Sat, Nov 23, 1:43 PM
Unknown Object (File)
Mon, Nov 18, 8:08 PM
Unknown Object (File)
Sun, Nov 17, 6:19 PM
Unknown Object (File)
Sun, Nov 17, 2:57 PM
Unknown Object (File)
Thu, Nov 7, 11:28 PM
Unknown Object (File)
Oct 17 2024, 3:45 AM
Subscribers

Details

Summary

issue: https://linear.app/comm/issue/ENG-5914/stop-calling-logout-from-socket
We want to stop logging out from the Socket component. Temporarily, after this action is dispatched we do call logout from KeyserverConnectionHandler (see D10331). So the behaviour from users perspective stays the same

Test Plan

Forced the dispatch from all of those three places, checked that the user is logged out and no errors show up

Diff Detail

Repository
rCOMM Comm
Branch
inka/connectionIssue2
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka requested review of this revision.Dec 14 2023, 4:48 AM
This revision is now accepted and ready to land.Dec 14 2023, 9:43 AM
ashoat requested changes to this revision.Dec 15 2023, 10:12 AM

I'm not sure @ginsu makes sense as a reviewer for this stack... he really doesn't have context. I'm not sure the other reviewers do either.

How about @tomek? He's been looking at this closely with you, right?

lib/socket/socket.react.js
696 ↗(On Diff #34626)

Shouldn't this be more specific? This doesn't happen on any socket crash loop... it only happens when we're seeing a socket crash loop after policy acknowledgement

native/socket.react.js
124 ↗(On Diff #34626)

I'm concerned that we're using socket_crash_loop in two places. Can you explain the differences between these two callsites, and why it makes sense to treat them the same?

This revision now requires changes to proceed.Dec 15 2023, 10:12 AM
inka added inline comments.
lib/socket/socket.react.js
696 ↗(On Diff #34626)

I will amend D10287

native/socket.react.js
124 ↗(On Diff #34626)

Both of those places are called in case of a socket crash loop connected to noDataAfterPolicyAcknowledgment. And in both those cases we were not able to recover from the socket crash loop. Either because the user is an ENS user and we just have no way of recovering for them (this call site here), or the user is a password user, but the recovery failed for some other reason (the call site in lib/socket/socket.react.js).

It's worth noting that after @varun creates the new auth endpoint on the keyserver, the ENS users will be able to retry, like password users can now. So this whole if (!accountHasPassword(currentUserInfo)) will be just removed, and only the call site in lib will remain.

I think there was a task tracking this, but I can't find it. @varun do you know which task it was?

native/socket.react.js
124 ↗(On Diff #34626)

Thanks for explaining! I think the task is ENG-5940, which has been passed to me

Rename connection issue case

tomek added inline comments.
lib/socket/socket.react.js
41–65 ↗(On Diff #34873)

These imports can be merged

This revision is now accepted and ready to land.Dec 20 2023, 6:59 AM