fix: avoid prefetching when following deeplinks #11097

anandaroop · 2024-11-14T22:42:29Z

This PR resolves ONYX-1392

Co-authored-by: @brainbicycle
Co-authored-by: @MounirDhahri
Co-authored-by: @olerichter00

Description

The goal here is to avoid prefetching when following a deep link, in order to avoid an unnecessary stampede that results in API latency.

We observe that prefetching happens in a few ways:

A lighter home view prefetch is initiated from the splash screen
- can be avoided by checking if we are following a deep link
A heavier prefetch for a subset of home view sections’ content
- can also be avoided by declining to render the home view at all when launching via deep link
Content for other tabs is initiated from the home view
- can be avoided thanks to the previous point

After this change

Case	Behavior	Screencap
Launching the app normally	Performs prefetching, as before	regular-launch1080.out.mov
Launching via deep link	Avoids prefetching	deep-link1080.out.mov
Launching via deep link, then returning to home	Fetches the content just-in-time rather than prefetch. This is the necessary de-optimization that allows us to avoid the stampede of API requests	deep-then-home1080.out.mov
Launching via link to `/`	Performs prefetching, as before	home-link1080.out.mov

PR Checklist

I have tested my changes on iOS and Android.
I hid my changes behind a feature flag, or they don't need one.
I have included screenshots or videos, or I have not changed the UI.
I have added tests, or my changes don't require any.
I added an app state migration, or my changes do not require one.
I have documented any follow-up work that this PR will require, or it does not require any.
I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

I would like at least one of the reviewers to run this PR on the simulator or device.

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

Avoids prefetching home view content when opening a deep link

Need help with something? Have a look at our docs, or get in touch with us.

MounirDhahri

Nice! thanks for the fix

src/app/Scenes/Home/HomeContainer.tsx

src/app/utils/hooks/useIsDeepLink.tests.ts

src/app/Scenes/Home/HomeContainer.tsx

ArtsyOpenSource · 2024-11-19T18:54:04Z

This PR contains the following changes:

Dev changes (Avoids prefetching home view content when opening a deep link - anandaroop)

Generated by 🚫 dangerJS against 8bdb4f8

src/app/utils/hooks/useIsDeepLink.ts

brainbicycle

i think we need a little more nuance in our home deeplink detection, happy to pair tomorrow if it would be helpful!

anandaroop · 2024-11-20T23:34:47Z

Brian and I paired on switching the detection back to matchRoute in 4bf5896 since it will be more reliable in the edge cases noted above. (h/t @MounirDhahri for suggesting that originally)

I confirmed that the screengrabs above still work the same after these changes, and we also ran through the various cases in this gist, which may also come in handy at Mobile QA time:

https://gist.github.com/anandaroop/8903e16ced8f962ff1fea60e13266584

brainbicycle

thank you!

src/app/utils/hooks/useIsDeepLink.ts

olerichter00 · 2024-11-21T11:17:35Z

src/app/utils/hooks/useIsDeepLink.ts

+ * @returns {isDeepLink: boolean | null} isDeepLink is true if the user came from a deep link
+ */
+export const useIsDeepLink = () => {
+  const [isDeepLink, setIsDeepLink] = useState<boolean | null>(null)


very minor: Shouldn't the initial value be undefined instead of null because it's not yet defined (null vs undefined)?

olerichter00 · 2024-11-21T11:20:45Z

src/app/utils/hooks/useIsDeepLink.ts

+      })
+      .catch((error) => {
+        console.error("Error getting initial URL", error)
+        setIsDeepLink(false)


also very minor: Setting the value to false when retrieving the URL fails does not seem 100% correct. However, setting it to another value (undefined or null) or adding an error state will probably make this hook very complicated. 🤷

olerichter00 · 2024-11-21T11:22:57Z

src/app/utils/useHideSplashScreen.ts

@@ -20,14 +21,18 @@ export const useHideSplashScreen = () => {

    if (isHydrated) {
      if (isLoggedIn && isNavigationReady) {
-        prefetchUrl("/", homeViewScreenQueryVariables(), {
-          force: false,
+        Linking.getInitialURL().then((url) => {


Couldn't we use the new hook useIsDeepLink here as well?

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?

It looks like useIsFocused (which is used by useIsDeepLink) is causing the error because it cannot be used outside of a NavigationContainer.

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 :)).

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

we could possibly distinguish that case specifically 🤔

in app messages I would need to look at, there should be some braze callbacks we can get hopefully in js land.

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.

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.

+1 merging this PR as is 👍

Ok, let's go with that. (My timebox didn't really yield anything).

One more rebase and I'll merge this.

src/app/utils/hooks/useIsDeepLink.ts

Co-authored-by: Ole <[email protected]> Co-authored-by: Anandaroop Roy <[email protected]>

Prior to this change we were rendering a blank home screen in the event of navigating directly to /

This will be more reliable in the case of various edges cases that might bring the user to the home screen

Co-authored-by: Ole <[email protected]> Co-authored-by: Mounir Dhahri <[email protected]>

anandaroop · 2024-11-25T22:33:08Z

The rebase was a bit trickier due to #11184 getting merged this morning — but all good now, and I've run through all the above cases again on iOS and Android.

Going for the merge on green…

anandaroop requested review from MounirDhahri and brainbicycle November 14, 2024 22:42

anandaroop self-assigned this Nov 14, 2024

artsy-peril bot added the Jira Synced label Nov 14, 2024

MounirDhahri previously approved these changes Nov 15, 2024

View reviewed changes

MounirDhahri dismissed their stale review via 4ea8724 November 15, 2024 16:56

MounirDhahri force-pushed the anandaroop-brainbicycle/ONYX-1392-conditional-prefetching branch from ca7936d to 4ea8724 Compare November 15, 2024 16:56

This comment was marked as outdated.

Sign in to view

brainbicycle reviewed Nov 15, 2024

View reviewed changes

src/app/Scenes/Home/HomeContainer.tsx Outdated Show resolved Hide resolved

olerichter00 reviewed Nov 18, 2024

View reviewed changes

src/app/utils/hooks/useIsDeepLink.tests.ts Outdated Show resolved Hide resolved

src/app/utils/hooks/useIsDeepLink.tests.ts Outdated Show resolved Hide resolved

src/app/Scenes/Home/HomeContainer.tsx Outdated Show resolved Hide resolved

anandaroop force-pushed the anandaroop-brainbicycle/ONYX-1392-conditional-prefetching branch from e7f4dc4 to d151919 Compare November 19, 2024 18:05

anandaroop marked this pull request as ready for review November 19, 2024 18:51

anandaroop requested review from brainbicycle, olerichter00 and MounirDhahri November 19, 2024 18:53

brainbicycle reviewed Nov 19, 2024

View reviewed changes

src/app/utils/hooks/useIsDeepLink.ts Outdated Show resolved Hide resolved

brainbicycle requested changes Nov 19, 2024

View reviewed changes

anandaroop force-pushed the anandaroop-brainbicycle/ONYX-1392-conditional-prefetching branch from d151919 to 4bf5896 Compare November 20, 2024 23:31

brainbicycle previously approved these changes Nov 21, 2024

View reviewed changes

olerichter00 reviewed Nov 21, 2024

View reviewed changes

MounirDhahri reviewed Nov 21, 2024

View reviewed changes

src/app/utils/hooks/useIsDeepLink.ts Outdated Show resolved Hide resolved

anandaroop dismissed brainbicycle’s stale review via 716ed41 November 21, 2024 22:57

anandaroop force-pushed the anandaroop-brainbicycle/ONYX-1392-conditional-prefetching branch from 716ed41 to eff1785 Compare November 25, 2024 19:01

anandaroop and others added 5 commits November 25, 2024 14:40

fix: avoid prefetching when following deeplinks

5f37160

fix: avoid making home view query requests when opening deep links

c500a53

Co-authored-by: Ole <[email protected]> Co-authored-by: Anandaroop Roy <[email protected]>

fix: don't treat / as a deep link

216ddb0

Prior to this change we were rendering a blank home screen in the event of navigating directly to /

fix: switch back to matching route to module

c028406

This will be more reliable in the case of various edges cases that might bring the user to the home screen

fix: code review suggestions

aeb8d74

Co-authored-by: Ole <[email protected]> Co-authored-by: Mounir Dhahri <[email protected]>

anandaroop force-pushed the anandaroop-brainbicycle/ONYX-1392-conditional-prefetching branch from eff1785 to aeb8d74 Compare November 25, 2024 22:12

fix(test): mock the deep link hook

8bdb4f8

anandaroop added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: avoid prefetching when following deeplinks #11097

fix: avoid prefetching when following deeplinks #11097

anandaroop commented Nov 14, 2024 •

edited

Loading

MounirDhahri left a comment

This comment was marked as outdated.

ArtsyOpenSource commented Nov 19, 2024 •

edited

Loading

brainbicycle left a comment

anandaroop commented Nov 20, 2024 •

edited

Loading

brainbicycle left a comment

olerichter00 Nov 21, 2024

olerichter00 Nov 21, 2024

olerichter00 Nov 21, 2024

anandaroop Nov 21, 2024

olerichter00 Nov 21, 2024

brainbicycle Nov 21, 2024 •

edited

Loading

brainbicycle Nov 21, 2024

brainbicycle Nov 22, 2024

anandaroop Nov 22, 2024

brainbicycle Nov 22, 2024

olerichter00 Nov 25, 2024

anandaroop Nov 25, 2024

anandaroop commented Nov 25, 2024

fix: avoid prefetching when following deeplinks #11097

Are you sure you want to change the base?

fix: avoid prefetching when following deeplinks #11097

Conversation

anandaroop commented Nov 14, 2024 • edited Loading

Description

After this change

PR Checklist

To the reviewers 👀

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

MounirDhahri left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

ArtsyOpenSource commented Nov 19, 2024 • edited Loading

This PR contains the following changes:

brainbicycle left a comment

Choose a reason for hiding this comment

anandaroop commented Nov 20, 2024 • edited Loading

brainbicycle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brainbicycle Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandaroop commented Nov 25, 2024

anandaroop commented Nov 14, 2024 •

edited

Loading

ArtsyOpenSource commented Nov 19, 2024 •

edited

Loading

anandaroop commented Nov 20, 2024 •

edited

Loading

brainbicycle Nov 21, 2024 •

edited

Loading