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

Authored by ginsu on Jul 10 2024, 3:03 AM.

Details

Reviewers

ashoat
inka

Commits

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

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 created this revision.Jul 10 2024, 3:03 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 10 2024, 3:03 AM

ginsu edited the summary of this revision. (Show Details)Jul 10 2024, 3:18 AM

ginsu added reviewers: ashoat, inka.

Harbormaster completed remote builds in B30264: Diff 42205.Jul 10 2024, 3:22 AM

ginsu requested review of this revision.Jul 10 2024, 3:22 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:

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.
Update the native code to render ConnectFarcasterAlertHandler as a child of FarcasterDataHandler
Revert the changes to ConnectFarcasterAlertHandler here

What do you think?

This revision now requires changes to proceed.Jul 10 2024, 10:42 AM

address comments

ginsu added inline comments.Jul 10 2024, 11:49 PM

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