Page MenuHomePhabricator

[lib] launch connect farcaster prompt
Needs RevisionPublic

Authored by varun on Sun, Mar 16, 10:05 AM.
Tags
None
Referenced Files
F4944471: D14444.diff
Tue, Mar 18, 6:37 PM
Unknown Object (File)
Tue, Mar 18, 11:36 AM
Subscribers

Details

Reviewers
ashoat
Summary

This work has been complete for 9 months, but looks like it was never launched. Flipped DISABLE_CONNECT_FARCASTER_ALERT to false and removed the isDev check in shouldSkipConnectFarcasterAlert. Doesn't seem that annoying to see the bottom sheet when devs log in on mobile, but happy to add the isDev check back if we think that's better.

Test Plan

confirmed that the bottom sheet appears on login now

NOTE: didn't test the actual connect farcaster flow. i assume this has been tested already

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

need to think about how to sequence things a little more

lib/utils/push-alerts.js
24–25

I'd probably prefer to keep this

Ended up testing the connect farcaster flow and it works. A couple things worth noting:

  1. The connect farcaster bottom sheet appears at the same time as the NUX tips if the user installs the app and logs in. Not sure if this edge case is worth addressing
  1. The bottom sheet appears at the same time as the SIWE backup message screen when a SIWE user who doesn't have a backup message yet logs in. This seems unlikely to happen since the backup message generation is now part of the registration flow
ashoat requested changes to this revision.Sun, Mar 16, 5:07 PM

Feels like there are probably ways to dismiss the modal without setting the fid to null here. Ideally we can catch all methods of closing the modal, but if that's really hard, one alternative would just be to set the fid to null as soon as we show the modal.

The connect farcaster bottom sheet appears at the same time as the NUX tips if the user installs the app and logs in. Not sure if this edge case is worth addressing

Can you clarify:

  1. Does this occur for every login?
  2. Alternately, does this occur for every fresh install login?
  3. Or does it only occur for a user that has not previously responded to the prompt?

If it's 1 or 2 I feel like it's probably worth addressing...

The bottom sheet appears at the same time as the SIWE backup message screen when a SIWE user who doesn't have a backup message yet logs in. This seems unlikely to happen since the backup message generation is now part of the registration flow

That makes sense to me.

native/components/connect-farcaster-bottom-sheet.react.js
81 ↗(On Diff #47409)

Is this different than goBack() in some way?

96 ↗(On Diff #47409)

Are there any other ways to dismiss the modal? For instance, the hardware back button on Android? How can we make sure we catch those?

This revision now requires changes to proceed.Sun, Mar 16, 5:07 PM

Alternately, does this occur for every fresh install login?

Yes, it occurs the first time a user logs in after fresh install

Does it not occur on every single login because the NUX doesn't display on subsequent logins?

Does it not occur on every single login because the NUX doesn't display on subsequent logins?

That's right. When the NUX is displayed, we write to AsyncStorage so that it doesn't appear again for that install

varun added inline comments.
native/components/connect-farcaster-bottom-sheet.react.js
81 ↗(On Diff #47409)

@ginsu explains here why we use close() instead of goBack(): https://phab.comm.dev/D11598?id=38949#inline-69634

native/components/connect-farcaster-bottom-sheet.react.js
81 ↗(On Diff #47409)

How does that explain why we use close() instead of goBack()? He only explains why we have an effect for it, I don't see any discussion of close() vs. goBack()

native/components/connect-farcaster-bottom-sheet.react.js
81 ↗(On Diff #47409)

Oh yeah... not sure why we need this ref

native/components/connect-farcaster-bottom-sheet.react.js
81 ↗(On Diff #47409)

goBack() seems to work just fine

96 ↗(On Diff #47409)

there are many ways to dismiss the modal. i'm not sure i understand what the concern is here

Feels like there are probably ways to dismiss the modal without setting the fid to null here. Ideally we can catch all methods of closing the modal, but if that's really hard, one alternative would just be to set the fid to null as soon as we show the modal.

the onClosed callback gets called by BottomSheet's onChange callback:

const onChange = React.useCallback(
  (index: number) => {
    if (index === -1) {
      onClosed();
    }
  },
  [onClosed],
);

i think this should handle all methods of closing the modal

native/components/connect-farcaster-bottom-sheet.react.js
96 ↗(On Diff #47409)

hmm thinking about this more... maybe we don't need this goBack at all?

native/components/connect-farcaster-bottom-sheet.react.js
96 ↗(On Diff #47409)

oh we need the goBack to get rid of the backdrop

Feels like there are probably ways to dismiss the modal without setting the fid to null here. Ideally we can catch all methods of closing the modal, but if that's really hard, one alternative would just be to set the fid to null as soon as we show the modal.

the onClosed callback gets called by BottomSheet's onChange callback:

const onChange = React.useCallback(
  (index: number) => {
    if (index === -1) {
      onClosed();
    }
  },
  [onClosed],
);

i think this should handle all methods of closing the modal

Nice find!! Worth confirming that it works for the hardware back button on Android.

The connect farcaster bottom sheet appears at the same time as the NUX tips if the user installs the app and logs in. Not sure if this edge case is worth addressing

Could probably be addressed in a separate diff. If you remove the DISABLE_CONNECT_FARCASTER_ALERT change from this one, we could land this diff sooner and address the NUX tips appearing at the same time later, and then flip DISABLE_CONNECT_FARCASTER_ALERT after that.

native/components/connect-farcaster-bottom-sheet.react.js
81 ↗(On Diff #47409)

Cool let's just swap to goBack() then for simplicity

96 ↗(On Diff #47409)

there are many ways to dismiss the modal. i'm not sure i understand what the concern is here

We need to call setLocalFID(null) when the modal is dismissed – but I think you got the concern because you responded separately