Page MenuHomePhabricator
Differential D12683

[native] update connect farcaster prompt to be less annyoying
ClosedPublic
Actions

Authored by ginsu on Jul 7 2024, 10:37 PM.
Tags
None
Referenced Files
F3346753: D12683.id42164.diff
Fri, Nov 22, 9:45 AM
F3345116: D12683.diff
Fri, Nov 22, 5:32 AM
Unknown Object (File)
Sun, Nov 10, 3:09 PM
Unknown Object (File)
Sun, Nov 10, 2:38 PM
Unknown Object (File)
Sun, Nov 10, 1:10 PM
Unknown Object (File)
Sun, Nov 10, 7:44 AM
Unknown Object (File)
Sun, Nov 10, 6:47 AM
Unknown Object (File)
Sun, Nov 10, 1:25 AM
View All 71 Files
Subscribers
ashoat
tomek

Details

Reviewers
ashoat
inka
Commits
rCOMMf56e3117de03: [native] update connect farcaster prompt to be less annyoying
Summary

We want to update the logic for when we show the farcaster prompt so that it is less annoying. This means we only want to show this prompt to the user just once if the user has an existing account that does not have an fid yet. We will also avoid showing the prompt to users who created new accounts and chose not to connect their farcaster account during the registration process

Linear task: https://linear.app/comm/issue/ENG-8449/rethink-farcaster-prompt-to-make-sure-its-not-annoying

Test Plan

Confirmed that the following 3 scenarios work as expected

  1. Existing user with fid => No prompt
  2. Existing user with no fid => Prompt
  3. New user who declined to connect to farcaster => No prompot

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu created this revision.Jul 7 2024, 10:37 PM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 7 2024, 10:37 PM
ginsu edited the summary of this revision. (Show Details)Jul 7 2024, 10:44 PM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: ashoat, inka.
Harbormaster completed remote builds in B30182: Diff 42094.Jul 7 2024, 10:53 PM
ginsu requested review of this revision.Jul 7 2024, 10:53 PM
inka added a comment.Jul 8 2024, 4:43 AM

shouldSkipPushPermissionAlert should probably be updated - if I understand correctly this diff makes it impossible for totalAlerts to exceed 1, is that correct?

native/account/registration/registration-server-call.js
369–375 ↗(On Diff #42094)

Why do we do this? Is this later checked somehow?

ginsu added a comment.Jul 8 2024, 11:02 AM

shouldSkipPushPermissionAlert should probably be updated - if I understand correctly this diff makes it impossible for totalAlerts to exceed 1, is that correct?

No everything should continue to work the same as before with shouldSkipPushPermissionAlert. We only want to update the connect farcaster alert logic. This diff will only make it impossible for the totalAlerts of the connect farcaster alert to exceed 1.

native/account/registration/registration-server-call.js
369–375 ↗(On Diff #42094)

We do this to guarantee that a new user who just registered for a new comm account won't be shown the connect farcaster prompt. In the ConnectFarcasterAlertHandler we have logic that will check if the user has a value for an fid in the synced metadata table store and if they have a value we won't show them the alert/prompt

https://github.com/CommE2E/comm/blob/master/native/components/connect-farcaster-alert-handler.react.js#L39-L47

ashoat requested changes to this revision.Jul 8 2024, 12:41 PM

Don't we now need to consider NO_FID_METADATA in all places we fetch the FID? It seems like it would need to be checked at every place the FID is fetched.

Let me know if I'm missing something.

native/account/registration/registration-server-call.js
360–375 ↗(On Diff #42094)

We can significantly cut down on the duplicated lines here:

const fidToSave = farcasterID ?? NO_FID_METADATA;
dispatch({
  type: setSyncedMetadataEntryActionType,
  payload: {
    name: syncedMetadataNames.CURRENT_USER_FID,
    data: fidToSave,
  },
});
This revision now requires changes to proceed.Jul 8 2024, 12:41 PM
ginsu added a child revision: D12569: [lib/web] introduce handleCurrentUserFID to FarcasterDataHandler.Jul 8 2024, 4:25 PM
ginsu added a comment.Jul 8 2024, 4:41 PM

Don't we now need to consider NO_FID_METADATA in all places we fetch the FID? It seems like it would need to be checked at every place the FID is fetched.

In the linear task we said:

Don't prompt if the FID in Redux is null NONE

Based off this I was under the impression that we only want to keep NO_FID_METADATA something that is only in redux/sqlite and not in the identity service. By keeping NO_FID_METADATA only in redux/sqlite, we would re-show the farcaster prompt to the user if they ever logged out + logged back in on native, and my thinking was that this would act as another one time nudge to the user that they should connect their farcaster account to comm.

It seems like I may have misinterpreted this point. If I did I should be able to easily update this diff so that we also have NO_FID_METADATA stored in the identity service if the user decides not to connect their farcaster account

ashoat added a comment.EditedJul 9 2024, 10:33 AM
In D12683#359547, @ginsu wrote:

Don't we now need to consider NO_FID_METADATA in all places we fetch the FID? It seems like it would need to be checked at every place the FID is fetched.

In the linear task we said:

Don't prompt if the FID in Redux is null NONE

Based off this I was under the impression that we only want to keep NO_FID_METADATA something that is only in redux/sqlite and not in the identity service. By keeping NO_FID_METADATA only in redux/sqlite, we would re-show the farcaster prompt to the user if they ever logged out + logged back in on native, and my thinking was that this would act as another one time nudge to the user that they should connect their farcaster account to comm.

It seems like I may have misinterpreted this point. If I did I should be able to easily update this diff so that we also have NO_FID_METADATA stored in the identity service if the user decides not to connect their farcaster account

I think there's some confusion here. I'm not suggesting storing NO_FID_METADATA on identity.

Rather, I'm concerned that we don't have any updates to code that fetches/checks syncedMetadataNames.CURRENT_USER_FID here.

We are storing a new special value in that field, that should be treated equivalently to null/undefined. Don't we need to update the code that fetches/checks syncedMetadataNames.CURRENT_USER_FID so that it knows to treat this special value as null/undefined?

Let me know if I'm missing something!

ginsu added a comment.Jul 9 2024, 11:58 AM
In D12683#359698, @ashoat wrote:
In D12683#359547, @ginsu wrote:

Don't we now need to consider NO_FID_METADATA in all places we fetch the FID? It seems like it would need to be checked at every place the FID is fetched.

In the linear task we said:

Don't prompt if the FID in Redux is null NONE

Based off this I was under the impression that we only want to keep NO_FID_METADATA something that is only in redux/sqlite and not in the identity service. By keeping NO_FID_METADATA only in redux/sqlite, we would re-show the farcaster prompt to the user if they ever logged out + logged back in on native, and my thinking was that this would act as another one time nudge to the user that they should connect their farcaster account to comm.

It seems like I may have misinterpreted this point. If I did I should be able to easily update this diff so that we also have NO_FID_METADATA stored in the identity service if the user decides not to connect their farcaster account

I think there's some confusion here. I'm not suggesting storing NO_FID_METADATA on identity.

Rather, I'm concerned that we don't have any updates to code that fetches/checks syncedMetadataNames.CURRENT_USER_FID here.

We are storing a new special value in that field, that should be treated equivalently to null/undefined. Don't we need to update the code that fetches/checks syncedMetadataNames.CURRENT_USER_FID so that it knows to treat this special value as null/undefined?

Let me know if I'm missing something!

Thank you for further explaining and apologies for the confusion on my end. I will need to update useCurrentUserFID so that it knows to treat NO_FID_METADATA as null. Updating the diff right now

ginsu updated this revision to Diff 42164.Jul 9 2024, 11:58 AM

address comments

ginsu added inline comments.Jul 9 2024, 12:03 PM
lib/utils/farcaster-utils.js
17 ↗(On Diff #42164)

if it's null then that means the user has decided NOT to set a Farcaster association. If it's undefined, then this is a case where a user has not been prompted yet, either during registration or via the later prompt.

https://linear.app/comm/issue/ENG-8449/rethink-farcaster-prompt-to-make-sure-its-not-annoying#comment-d068720f

native/components/connect-farcaster-alert-handler.react.js
43 ↗(On Diff #42164)

Moved this check into shouldSkipConnectFarcasterAlert

Harbormaster completed remote builds in B30235: Diff 42164.Jul 9 2024, 12:15 PM
ginsu added inline comments.Jul 9 2024, 12:20 PM
lib/utils/farcaster-utils.js
17 ↗(On Diff #42164)

Was thinking it would be good to leave a code comment here so other devs have context about this distinction between null & undefined.

I can do this before landing if this is wanted

ginsu mentioned this in D12685: [lib] check if the user fid is still valid.Jul 9 2024, 1:48 PM
ashoat accepted this revision.Jul 9 2024, 1:51 PM
ashoat added inline comments.
lib/utils/farcaster-utils.js
17 ↗(On Diff #42164)

Yeah seems like a good idea

lib/utils/push-alerts.js
29–30 ↗(On Diff #42164)

Can we replace these with fid !== undefined?

This revision is now accepted and ready to land.Jul 9 2024, 1:51 PM
ginsu updated this revision to Diff 42180.Jul 9 2024, 3:29 PM

address comments + rebase before landing

Harbormaster completed remote builds in B30245: Diff 42180.Jul 9 2024, 3:45 PM
Closed by commit rCOMMf56e3117de03: [native] update connect farcaster prompt to be less annyoying (authored by ginsu). · Explain WhyJul 9 2024, 3:57 PM
This revision was automatically updated to reflect the committed changes.
ginsu added a commit: rCOMMf56e3117de03: [native] update connect farcaster prompt to be less annyoying.

Revision Contents
Changeset List

PathSize
lib/
utils/
16 lines
8 lines
native/
account/
registration/
18 lines
components/
3 lines

Diff 42184

View Options

lib/utils/farcaster-utils.js

Loading...
View Options

lib/utils/push-alerts.js

Loading...
View Options

native/account/registration/registration-server-call.js

Loading...
View Options

native/components/connect-farcaster-alert-handler.react.js

Loading...