Page MenuHomePhabricator

[native] fix farcaster prompt being displayed to users with fid on login
ClosedPublic

Authored by ginsu on Jul 10 2024, 3:03 AM.
Tags
None
Referenced Files
F3546330: D12718.diff
Thu, Dec 26, 2:34 PM
Unknown Object (File)
Tue, Dec 24, 5:00 AM
Unknown Object (File)
Sun, Dec 22, 4:04 PM
Unknown Object (File)
Sun, Dec 22, 4:04 PM
Unknown Object (File)
Sun, Dec 22, 4:04 PM
Unknown Object (File)
Sun, Dec 22, 4:04 PM
Unknown Object (File)
Sun, Dec 22, 4:03 PM
Unknown Object (File)
Sun, Dec 22, 4:03 PM
Subscribers

Details

Summary

Was doing some final testing, and ran into an issue where a user who had already created a farcaster association and had an fid linked to their comm account was getting shown the connect to farcaster prompt whenever they logged back in.

Did some digging, and it looks like the cause of this issue is that when a user logs back the fid is not set (since the user logged out and the data in the synced metadata table was deleted) until the identity service returns the fid stored from the service. This means that initially the ConnectFarcasterAlertHandler thinks that the user does not have an fid and will show the prompt.

To fix this I added an additional check where if the user does not have an fid set in redux, then we should also query and check the identity service and see if the user has an fid there

Eventually when the synced metadata table store can sync + share the data to other clients through the backup service and tunnelbroker this check won't be necessary

Test Plan

Logged out and back in of the user with the fid and confirmed that this issue was no longer occuring

In this demo Bruce has a fid in the identity service and should not get the farcaster prompt and dennis does not have an fid set in the identity service and should get the farcaster prompt

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu added reviewers: ashoat, inka.
ginsu requested review of this revision.Jul 10 2024, 3:22 AM
ashoat requested changes to this revision.Jul 10 2024, 10:42 AM

Thanks for finding this bug!! Really appreciate you trying to make sure your work is 100% before finishing off.

I'm a bit concerned here that we have two places that are going to be querying findUserIdentities for the same reason.

What if we instead changed where ConnectFarcasterAlertHandler was rendered, to make sure it isn't rendered until FarcasterDataHandler has a chance to run?

Here's my concrete proposal:

  1. Update FarcasterDataHandler to take a children property. It will render the children once it has a chance to run its effect once and query for findUserIdentities.
  2. Update the native code to render ConnectFarcasterAlertHandler as a child of FarcasterDataHandler
  3. Revert the changes to ConnectFarcasterAlertHandler here

What do you think?

This revision now requires changes to proceed.Jul 10 2024, 10:42 AM
lib/components/farcaster-data-handler.react.js
23 ↗(On Diff #42215)

Made this optional since we also use FarcasterDataHandler on web too, but we don't need to render ConnectFarcasterAlertHandler as a child in web

native/root.react.js
311 ↗(On Diff #42215)

PersistedStateGate simply just checks that the persisted redux state has been loaded/rehydrated.

https://github.com/CommE2E/comm/blob/master/native/components/persisted-state-gate.js

When I moved FarcasterDataHandler to be outside PersistedStateGate (we need to do this since ConnectFarcasterAlertHandler uses navigation logic and has to be in this NavigationContainer), I was getting the following errors in my logs:

Error: Identity service client is not initialized

Since PersistedStateGate is just a check whether to render the children or not, it seemed reasonable to bring this gate also into inside the NavigationContainer as well

This revision is now accepted and ready to land.Jul 11 2024, 5:27 PM
This revision was landed with ongoing or failed builds.Jul 11 2024, 11:58 PM
This revision was automatically updated to reflect the committed changes.