feat: deep linking set up#1878
Conversation
… app is not ready by storing the url in context until the app is ready.
|
| Name | Status | Previous | Current |
|---|---|---|---|
expo-linking |
- | 7.0.4 |
|
@ErikSin I know you suggested moving the DeepLinkListener (new name) into the AppScreens file but I could not figure out how to do it because
so if you could provide a bit of guidance about what you were thinking I would appreciate it. And, should I try to add an integration test to this somehow once our testing is updated? I will need a few tips about the approach there too, I am afraid. |
ErikSin
left a comment
ErikSin
left a comment
There was a problem hiding this comment.
Thanks for doing this work. I think your possibly dispatching the navigation event for 1 invite up to 3 times:
- config property of the deeplinking
- the DeepLinkListener
- the PendingInviteListener (this one im not sure about)
Also, I think that we can simplify the listener a bit if it is aware of the nav route.
| const [pendingInviteId, setPendingInviteId] = React.useState<string | null>( | ||
| null, | ||
| ); |
There was a problem hiding this comment.
I don't think I fully understand the architecture, but would the PendingInvitesListener also catch the invite? And if thats the case, would there be 2 navigation events?
There was a problem hiding this comment.
No — PendingInvitesListener watches useManyInvites() which returns local peer-to-peer invites not the deep link invites, which is a completely different path — it never appears in that list. So there's no double navigation from that source.
| const isNotReadyForInvite = | ||
| security.authState === 'unauthenticated' || | ||
| !deviceInfo.name || | ||
| !activeProjectId; | ||
|
|
||
| React.useEffect(() => { | ||
| setDeepLinkReady(!isNotReadyForInvite); | ||
| }, [isNotReadyForInvite]); | ||
|
|
There was a problem hiding this comment.
You can do this without a useEffect:
const isNotReadyForInvite =
security.authState === 'unauthenticated' ||
!deviceInfo.name ||
!activeProjectId
//directly inline with no useEffecr
setDeepLinkReady(!isNotReadyForInvite);|
|
||
| export const DeepLinkListener = () => { | ||
| const navigation = useNavigationFromRoot(); | ||
| const url = ExpoLinking.useURL(); |
There was a problem hiding this comment.
This is deprecated and you should use useLinkingURL()
| config: { | ||
| screens: { | ||
| InviteReceived: 'invite/:inviteId', | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
the config property here is used for matching screens with the url. ie, allows you to have a 1 to 1 matching with the url, so as long as your url matches your screen pattern you can open that screen directly. But we are not doing that. We are always intercepting the url, and, grabbing the invite id, and then dispatching a navigation based on that invite id. This would cause 2 navigation events, so I think we need to get rid of it
| React.useEffect(() => { | ||
| if (!url) return; | ||
| const inviteId = parseInviteUrl(url); | ||
| if (inviteId) { | ||
| setPendingInviteId(inviteId); | ||
| } | ||
| }, [url]); |
There was a problem hiding this comment.
i think we can get rid of this useEffect entirely.
eg
const url = ExpoLinking.useURL();
const pendingInviteId = url ? parseInviteUrl(url) : undefined
React.useEffect(() => {
if (!pendingInviteId) return;
const waitUntilMount = requestAnimationFrame(() => {
navigation.navigate('InviteReceived', {inviteId: pendingInviteId});
setPendingInviteId(null);
});
return () => cancelAnimationFrame(waitUntilMount);
}, [pendingInviteId, navigation]);| const waitUntilMount = requestAnimationFrame(() => { | ||
| navigation.navigate('InviteReceived', {inviteId: pendingInviteId}); |
There was a problem hiding this comment.
I think the better thing to do is follow the architecture of the other listeners, where they are aware of the nav route and uses isInviteScreen and isEditingScreen. This is also related to this comment:
@ErikSin I know you suggested moving the DeepLinkListener (new name) into the AppScreens file but I could not figure out how to do it because
ERROR [Error: A navigator can only contain 'Screen', 'Group' or 'React.Fragment' as its direct children (found 'DeepLinkListener'). To render this component in the navigator, pass it in the 'component' prop to 'Screen'.]
so if you could provide a bit of guidance about what you were thinking I would appreciate it. And, should I try to add an integration test to this somehow once our testing is updated? I will need a few tips about the approach there too, I am afraid.
What i was originally suggesting was just an in passing comment when we were looking at the conditional navigation, it wasnt a fully formed suggestion, just something to explore.
But i think also we can avoid using requestAnimationFrame if the listener is aware of the nav route. The nav route is set after the navigation has fully finished. So if we are passing the navigation route to this listener we know the navigation has fully finished before we try and dispatch the navigation action
…ows same pattern as pending invites listener to check current route name. Updated to non deprecated useLinkingUrl. Eliminates unneeded use effect.
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
ErikSin
left a comment
ErikSin
left a comment
There was a problem hiding this comment.
Sorry for the delay, i just wanted to have the meeting about the invites before i fully did a review.
to answer your question:
assetlinks.json: Am I responsible for setting this up on app.comapeo.org? Without it, Android will fall back to the custom scheme only and won't intercept https:// links.
No, ill create an issue for this and we can do it later.
Just one non blocking comment that answers your question about obscure mode.
| const activeProjectId = useActiveProjectId(); | ||
| const {formatMessage} = useIntl(); | ||
| const isNotReadyForInvite = | ||
| security.authState === 'unauthenticated' || |
There was a problem hiding this comment.
Obscured mode: Currently, if the app is in obscured mode (passcode screen showing), an invite link is saved and shown after unlock — same as the passcode scenario. Should invite links be suppressed entirely in obscured mode, or is showing them after unlock the right behavior?
I don't think we should show the invite link if the app is obscured. So this logic should be
cons isNotReadyForInvite = security.authstate !== authenticated ....rest
2 participants
closes #1766
What this adds
Sets up the deep linking infrastructure needed for invite-over-the-internet. When a user taps an invite link (comapeo://invite/ or https://app.comapeo.org/invite/), the app opens and navigates to the InviteReceived screen (for now).
Changes:
RootStackNavigatorfor two situations where the app isn't ready to navigate immediately: mid-onboarding and passcode locked. In both cases the invite ID is captured and held, then navigation happens once the app is ready. A requestAnimationFrame is used so that navigation only happens after the app navigator has finished mounting its screens.deepLinkReady) indeepLinkConfig.tstracks whether the app is ready. AppNavigator reads this flag to suppress React Navigation's own automatic Link handling while the app is not ready — otherwise React Navigation would try to navigate before the right screens exist.Questions:
Obscured mode: Currently, if the app is in obscured mode (passcode screen showing), an invite link is saved and shown after unlock — same as the passcode scenario. Should invite links be suppressed entirely in obscured mode, or is showing them after unlock the right behavior?
Has the domain for invite links been confirmed? The current implementation assumes app.comapeo.org based on the invite-over-internet in notion, but if that's not finalized the host will need to change in both
app.jsonanddeepLinkConfig.ts.assetlinks.json: Am I responsible for setting this up on app.comapeo.org? Without it, Android will fall back to the custom scheme only and won't intercept https:// links.
To do next:
InviteReceivedto handle internet invites (the current screen is built for local wifi invites) or make new UI for it and it should handle expired and/ or invalid invite idsInviteReceived(or whatever we create) will need to parse window.location.hash or the raw URL to retrieve the secretTesting:
Integration tests would not really work for this because it is all about timing and navigation which is something we would have to mock so much it wouldn't actually test what we want to test.
Instead, you can test manually.
Requires a dev build on a physical Android device.
Scenario 1 — Happy path:
adb shell am start -W -a android.intent.action.VIEW -d "comapeo://invite/test-invite-id-123" com.comapeo.devExpected: app opens but there will be an error message since that invite id is not handled by
InviteReceived. I mocked out the response to it in InviteReceived just to check, which you can do if you wish.Scenario 2 — App backgrounded with passcode:
Set a passcode in Settings
Background the app and wait for it to lock
Run the adb command above
Enter the passcode Expected: same as above.