-
Notifications
You must be signed in to change notification settings - Fork 584
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
fix: avoid prefetching when following deeplinks #11097
Changes from all commits
5f37160
c500a53
216ddb0
c028406
aeb8d74
8bdb4f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
import { renderHook } from "@testing-library/react-hooks" | ||
import { matchRoute } from "app/routes" | ||
import { useIsDeepLink } from "app/utils/hooks/useIsDeepLink" | ||
import { Linking } from "react-native" | ||
|
||
const mockUseIsFocusedMock = jest.fn() | ||
jest.mock("@react-navigation/native", () => ({ | ||
useIsFocused: () => mockUseIsFocusedMock(), | ||
})) | ||
|
||
jest.mock("react-native", () => ({ | ||
Linking: { | ||
getInitialURL: jest.fn(), | ||
}, | ||
})) | ||
|
||
jest.mock("app/routes", () => ({ | ||
matchRoute: jest.fn(), | ||
})) | ||
|
||
describe("useIsDeepLink", () => { | ||
const mockLinkingGetInitialURL = Linking.getInitialURL as jest.Mock | ||
const mockMatchRoute = matchRoute as jest.Mock | ||
|
||
it("should return true if opened from a deep link", async () => { | ||
// Setup the mock to return the specific URL | ||
mockLinkingGetInitialURL.mockResolvedValue("artsy:///artwork/foo") | ||
mockMatchRoute.mockReturnValue({ type: "match", module: "Artwork" }) | ||
mockUseIsFocusedMock.mockReturnValue(true) | ||
|
||
// Render the hook under test | ||
const { result, waitForNextUpdate } = renderHook(() => useIsDeepLink()) | ||
|
||
// Wait for async effects to resolve | ||
await waitForNextUpdate() | ||
|
||
expect(result.current).toEqual({ | ||
isDeepLink: true, | ||
}) | ||
expect(mockUseIsFocusedMock).toHaveBeenCalled() | ||
expect(mockLinkingGetInitialURL).toHaveBeenCalled() | ||
expect(mockMatchRoute).toHaveBeenCalled() | ||
}) | ||
|
||
it("should return false if not opened from a deep link", async () => { | ||
// Setup the mock to return null | ||
mockLinkingGetInitialURL.mockResolvedValue(null) | ||
mockUseIsFocusedMock.mockReturnValue(true) | ||
|
||
// Render the hook under test | ||
const { result, waitForNextUpdate } = renderHook(() => useIsDeepLink()) | ||
|
||
// Wait for async effects to resolve | ||
await waitForNextUpdate() | ||
|
||
expect(result.current.isDeepLink).toEqual(false) | ||
expect(mockUseIsFocusedMock).toHaveBeenCalled() | ||
expect(mockLinkingGetInitialURL).toHaveBeenCalled() | ||
expect(mockMatchRoute).toHaveBeenCalled() | ||
}) | ||
|
||
it("should return false if opened from a link to /", async () => { | ||
// Setup the mock to return null | ||
mockLinkingGetInitialURL.mockResolvedValue("artsy:///") | ||
mockMatchRoute.mockReturnValue({ type: "match", module: "Home" }) | ||
mockUseIsFocusedMock.mockReturnValue(true) | ||
|
||
// Render the hook under test | ||
const { result, waitForNextUpdate } = renderHook(() => useIsDeepLink()) | ||
|
||
// Wait for async effects to resolve | ||
await waitForNextUpdate() | ||
|
||
expect(result.current.isDeepLink).toEqual(false) | ||
expect(mockUseIsFocusedMock).toHaveBeenCalled() | ||
expect(mockLinkingGetInitialURL).toHaveBeenCalled() | ||
expect(mockMatchRoute).toHaveBeenCalled() | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import { useIsFocused } from "@react-navigation/native" | ||
import { matchRoute } from "app/routes" | ||
import { useEffect, useState } from "react" | ||
import { Linking } from "react-native" | ||
|
||
/** | ||
* This is a hook that returns whether or not the user came from a deep link | ||
* (defined as a direct navigation to a route other than "/"). | ||
* | ||
* This can be used to avoid rendering content in previous screens in react-navigation history | ||
* | ||
* @returns {isDeepLink: boolean | null}` isDeepLink` is true if the user came from a deep link. Initially, it is set to `null` while retrieving the deep link URL asynchronously and will be set to `false` if retrieving the `URL` fails. | ||
*/ | ||
export const useIsDeepLink = () => { | ||
const [isDeepLink, setIsDeepLink] = useState<boolean | null>(null) | ||
|
||
const isFocused = useIsFocused() | ||
|
||
useEffect(() => { | ||
// When the user comes back from a deep link, | ||
// we want to show content again | ||
if (isFocused && isDeepLink === true) { | ||
setIsDeepLink(false) | ||
} | ||
}, [isFocused]) | ||
|
||
useEffect(() => { | ||
Linking.getInitialURL() | ||
.then((url) => { | ||
if (!url) { | ||
setIsDeepLink(false) | ||
return | ||
} | ||
|
||
const result = matchRoute(url) | ||
const isExternalUrl = result.type === "external_url" | ||
const isHomeLink = result.type === "match" && result.module === "Home" | ||
const shouldTreatAsDeepLink = !isHomeLink && !isExternalUrl | ||
|
||
setIsDeepLink(shouldTreatAsDeepLink) | ||
}) | ||
.catch((error) => { | ||
console.error("Error getting initial URL", error) | ||
setIsDeepLink(false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also very minor: Setting the value to |
||
}) | ||
}, []) | ||
|
||
return { | ||
isDeepLink, | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ import { homeViewScreenQueryVariables } from "app/Scenes/HomeView/HomeView" | |
import { GlobalStore } from "app/store/GlobalStore" | ||
import { usePrefetch } from "app/utils/queryPrefetching" | ||
import { useEffect } from "react" | ||
import { Linking } from "react-native" | ||
import RNBootSplash from "react-native-bootsplash" | ||
|
||
const HOME_VIEW_SPLASH_SCREEN_DELAY = 500 | ||
|
@@ -20,14 +21,18 @@ export const useHideSplashScreen = () => { | |
|
||
if (isHydrated) { | ||
if (isLoggedIn && isNavigationReady) { | ||
prefetchUrl("/", homeViewScreenQueryVariables(), { | ||
force: false, | ||
Linking.getInitialURL().then((url) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't we use the new hook There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had the same thought and tried this out briefly, but got some errors that suggested this wouldn't work. Do you want to give it a try @olerichter00? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like But I wonder whether we need the deep link check here at all. Even when following a deep link, prefetching the home view query would make sense because it's likely that the user will navigate to the home screen within the same session. Removing the check could have performance implications, but I don't think they are significant (which we should check :)). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the motivation here is that eigen is very chatty when first launched and this has led to incidents in the past: https://artsy.slack.com/archives/CA8SANW3W/p1731531969773739 but yeah it is a balance between performance for users and not overloading our systems. It does occur to me though that the concern is more around push notifications and in app messages specifically as these will result in large groups of users launching the app in a small time frame and causing spikes on our servers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could possibly distinguish that case specifically 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in app messages I would need to look at, there should be some braze callbacks we can get hopefully in js land. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll timebox an attempt at this^ (happy to pair also) — but if that doesn't pan out quickly how would we feel about merging this as-is, and then following up with this further refinement? If I'm understanding correctly, the only risk right now is a non-optimized home screen load for people who following an email deeplink, then navigate back to home. If that's right I think we stand to gain more than we lose with this current version, since we are still open to this stampede problem, even as we speak. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am fine with that personally, I think the more risky thing is the check in the HomeContainer, if we get that wrong we get dead home screens. I will create a ticket to follow up so if you want to timebox fine but fine also to leave it for the time being. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 merging this PR as is 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, let's go with that. (My timebox didn't really yield anything). One more rebase and I'll merge this. |
||
const isDeepLink = !!url | ||
if (!isDeepLink) { | ||
prefetchUrl("/", homeViewScreenQueryVariables(), { | ||
force: false, | ||
}) | ||
} | ||
setTimeout(() => { | ||
hideSplashScreen() | ||
}, HOME_VIEW_SPLASH_SCREEN_DELAY) | ||
}) | ||
|
||
setTimeout(() => { | ||
hideSplashScreen() | ||
}, HOME_VIEW_SPLASH_SCREEN_DELAY) | ||
|
||
return | ||
} | ||
|
||
|
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.
very minor: Shouldn't the initial value be
undefined
instead ofnull
because it's not yet defined (null vs undefined)?