-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Farcaster Improvements #2462
base: main
Are you sure you want to change the base?
Farcaster Improvements #2462
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Bundle SizesCompared against 8a7866b Route: No significant changes found Dynamic import: No significant changes found |
const [isFarcasterLoading, setIsFarcasterLoading] = useState(false); | ||
const [attemptReconnect, setAttemptReconnect] = useState(false); | ||
|
||
const { | ||
open: handleConnectFarcaster, | ||
isSuccess, | ||
isConnected, | ||
isPolling, | ||
} = useLoginWithFarcaster({ | ||
setIsFarcasterLoading, | ||
}); | ||
|
||
const [appState, setAppState] = useState(AppState.currentState); | ||
|
||
useEffect(() => { | ||
const subscription = AppState.addEventListener('change', (nextAppState) => { | ||
if (appState.match(/inactive|background/) && nextAppState === 'active') { | ||
if (isPolling || (!isConnected && !isSuccess)) { | ||
setAttemptReconnect(true); | ||
} | ||
setIsFarcasterLoading(false); | ||
} | ||
setAppState(nextAppState); | ||
}); | ||
|
||
return () => { | ||
subscription.remove(); | ||
}; | ||
}, [appState, isPolling, isConnected, isSuccess]); | ||
|
||
const handleConnectFarcasterPress = useCallback(() => { | ||
handleConnectFarcaster(attemptReconnect); | ||
}, [attemptReconnect, handleConnectFarcaster]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the only thing i'm concerned about with this PR is farcaster-related-logic being split in this component (FarcasterBottomSheetRow
) as well as the main hook useLoginWithFarcaster
from what i can tell, the appState / useState
and useEffect
can both live inside useLoginWithFarcaster
. the only variable used by this component is isFarcasterLoading
, but even that i think can be a state that's defined inside useLoginWithFarcaster
and exported out for this component to use
if (isPolling || (!isConnected && !isSuccess)) { | ||
setAttemptReconnect(true); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic makes sense in that if the connection did not go through, we attempt to reconnect from scratch.
is there any chance the connection is actually successful, but our app doesn't pick up on it yet by the time the user arrives back on our app? just wondering if we need to wait at all before immediately going into reconnection mode
const { nonce } = await createNonce(); | ||
setNonce(nonce); | ||
signIn(); | ||
Linking.openURL(url); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we're attempting to reconnect i don't think we need to create a new nonce! so we can probably remove L201-202
Summary of Changes
https://linear.app/galleryso/issue/GAL-5603/various-auth-polish
Demo or Before/After Pics
Screen.Recording.2024-05-07.at.14.48.15.mov
Edge Cases
List any common edge cases that you have considered and tested.
Testing Steps
Provide steps on how the reviewer can test the changes.
Checklist
Please make sure to review and check all of the following: