Page MenuHomePhabricator

[native] Have BackgroundIdentityLoginHandler wait until LogInHandler
ClosedPublic

Authored by ashoat on Jul 8 2024, 10:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 21, 12:18 PM
Unknown Object (File)
Fri, Oct 11, 2:58 AM
Unknown Object (File)
Fri, Oct 11, 2:58 AM
Unknown Object (File)
Fri, Oct 11, 2:58 AM
Unknown Object (File)
Fri, Oct 11, 2:58 AM
Unknown Object (File)
Sep 21 2024, 6:27 AM
Unknown Object (File)
Sep 19 2024, 10:44 AM
Unknown Object (File)
Sep 19 2024, 10:34 AM
Subscribers
None

Details

Summary

This addresses a race condition described in ENG-8785, first investigated in ENG-8772.

Depends on D12697

Test Plan

I reproduced the issue by following these instructions, but making sure to log in with SIWE. Before this diff the issue reproduces; after the diff, it no longer reproduced

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added inline comments.
native/components/background-identity-login-handler.react.js
31–37 ↗(On Diff #42123)

This sounds confusing: we're checking if we are logged in to identity to know if we are ready to try the identity login.

This revision is now accepted and ready to land.Jul 9 2024, 5:04 AM
native/components/background-identity-login-handler.react.js
31–37 ↗(On Diff #42123)

Hmm... I see your point. I think it all stems from the naming of isLoggedInToIdentityAndAuthoritativeKeyserver.

That name is based on a post-identity world, where currentUserInfo is set by identity (mostly), and the authoritative keyserver cookie is set in keyserverStore by the authoritative keyserver.

However, in a pre-identity world, both of these are set by the authoritative keyserver.

I'm going to land this for now, but I'm curious if you have a better suggested name for isLoggedInToIdentityAndAuthoritativeKeyserver that could improve the readability here.

native/components/background-identity-login-handler.react.js
31–37 ↗(On Diff #42123)

Not sure, we can consider using a different layer of abstraction and say hasCurrentUserInfoAndIsLoggedInAuthoritativeKeyserver - but it is really long. We can also consider mentioning that the user was logged in to identity. But overall, I don't see a name that is significantly better than this.