Page MenuHomePhabricator

[native] Limit BackgroundIdentityLoginHandler to one try per startup
ClosedPublic

Authored by ashoat on Jul 6 2024, 2:41 PM.
Tags
None
Referenced Files
F3396460: D12679.diff
Sun, Dec 1, 12:31 PM
Unknown Object (File)
Tue, Nov 12, 4:56 AM
Unknown Object (File)
Tue, Nov 12, 12:16 AM
Unknown Object (File)
Tue, Nov 12, 12:10 AM
Unknown Object (File)
Mon, Nov 11, 7:00 PM
Unknown Object (File)
Oct 11 2024, 4:52 AM
Unknown Object (File)
Oct 11 2024, 4:52 AM
Unknown Object (File)
Oct 11 2024, 4:52 AM
Subscribers
None

Details

Summary

The effect can make it happen multiple times, which can be messy if they're simultaneous. Let's just stick with one try per app startup.

This addresses ENG-8775, the original context for which is here

Depends on D12678

Test Plan

Make sure we only ever log in once per app startup. I had a log for each Redux action and could configure it only appeared once

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ashoat requested review of this revision.Jul 6 2024, 2:57 PM

one try per startup

Why do we want to do this only once? Is there a downside to setting loginAttemptedRef.current to false after the login is done?

This revision is now accepted and ready to land.Jul 8 2024, 1:43 AM

one try per startup

Why do we want to do this only once? Is there a downside to setting loginAttemptedRef.current to false after the login is done?

If the login was successful, we certainly don't want to try it again. So the idea here is probably more about trying it again if the login fails.

I'm not sure about this idea. If for instance the network is down when the app opens, would we be trying in an infinite loop? We'd probably at least want a delay between retries, similar to how the keyserver <> client socket in lib/socket/socket.react.js works. But then the solution gets more complex: we would probably want something like a debounce here.

My assessment is that the current solution is probably fine for now. If we want to implement retries, we can add those later.