feat: route intent preloading #102574
Merged
feat: route intent preloading #102574
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
Scope: Frontend
TkDodo
commented
Nov 3, 2025
❌ 17 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
so that a new render can re-start it
JonasBa
reviewed
Nov 3, 2025
JonasBa
reviewed
Nov 3, 2025
| } | ||
|
|
||
| // Add preload method that triggers the same shared promise as lazy() | ||
| RouteLazyLoad[PRELOAD_HANDLE] = getSharedPromise; |
Member
There was a problem hiding this comment.
Should we only set this if(!RouteLazyLoad[PRELOAD_HANDLE])?
Collaborator
Author
There was a problem hiding this comment.
the RouteLazyLoad function is created in line 43, so here in line 48, it can never have a PPRELOAD_HANDLE assigned.
JonasBa
reviewed
Nov 3, 2025
static/app/makeLazyloadComponent.tsx
Outdated
Comment on lines
25
to
31
| const getSharedPromise = async () => { | ||
| if (!sharedPromise) { | ||
| sharedPromise = retryableImport(resolve); | ||
| loadedComponent = (await sharedPromise).default; | ||
| } | ||
| return sharedPromise; | ||
| }; |
Member
There was a problem hiding this comment.
A little bike shedding, but we can just early return and flatten the flow
Suggested change
| const getSharedPromise = async () => { | |
| if (!sharedPromise) { | |
| sharedPromise = retryableImport(resolve); | |
| loadedComponent = (await sharedPromise).default; | |
| } | |
| return sharedPromise; | |
| }; | |
| const getSharedPromise = async () => { | |
| if(sharedPromise) return sharedPromise; | |
| sharedPromise = retryableImport(resolve); | |
| loadedComponent = (await sharedPromise).default; | |
| return sharedPromise; | |
| }; |
Collaborator
Author
There was a problem hiding this comment.
I had to refactor this to .then().catch() because an async function always returns a new Promise, so calling getSharedPromise() multiple times does in fact not return the same promise (which is necessary for react.lazy to work)
I should know because I wrote an article about that OVER 4 YEARS AGO on that 😂
https://tkdodo.eu/blog/about-async-functions#they-always-return-a-new-promise
regarding early return, I usually agree, but the situation here is a bit different - it’s the “initialize-cache-if-not-yet-initialized” pattern. An example with useRef would be:
const ref = useRef(null)
if (!ref.current) {
ref.current = myInitializer()
}
return ref.current
Here, we’re doing the same thing, and swapping the order and having two return sharedPromise calls hides that a bit.
const ref = useRef(null)
if (ref.current) return ref.current
ref.current = myInitializer()
return ref.current
this isn’t something we would write I guess (the react compiler also wouldn’t like that, while it’s fine with the lazy init pattern)
Member
There was a problem hiding this comment.
Interesting, but I didn't have the reaction to read it like that... Maybe it was because of the await side effect there too.
JonasBa
reviewed
Nov 3, 2025
| {...props} | ||
| LazyComponent={LazyComponent} | ||
| // If the component is already loaded, render it directly to avoid Suspense | ||
| LazyComponent={loadedComponent ?? LazyComponent} |
Member
There was a problem hiding this comment.
This is great, and now I wonder if the tiny loader flickers on the already loaded routes were happening because we were always returning a promise here, meaning this would always resolve in the next micro task
Collaborator
Author
There was a problem hiding this comment.
no, once react.lazy has loaded things, it returns the promise “synchronously”. React attaches a .status=“fulfilled” and .value property on the thenable and reads that. So, even in prod right now, once you’ve loaded all the assets, there shouldn’t be a flash.
Member
|
looks really good |
TkDodo
added a commit
that referenced
this pull request
Nov 4, 2025
This PR introduces a `LinkBehaviorContext` that allows us to decouple sentry specific code from our design-system `scraps`. In this first iteration, the only sentry specific things were: - `locationDescriptorToTo` - a call to `useLocation` to fall-back to anchor rendering if there is no location. Honestly, I don’t know why / if we need that because when would have no `useLocation` ? I do plan to add more sentry specific link behavior, like [intent prefetching](#102574), but this de-coupling is pre-requisite for that.
shashjar
pushed a commit
that referenced
this pull request
Nov 4, 2025
This PR introduces a `LinkBehaviorContext` that allows us to decouple sentry specific code from our design-system `scraps`. In this first iteration, the only sentry specific things were: - `locationDescriptorToTo` - a call to `useLocation` to fall-back to anchor rendering if there is no location. Honestly, I don’t know why / if we need that because when would have no `useLocation` ? I do plan to add more sentry specific link behavior, like [intent prefetching](#102574), but this de-coupling is pre-requisite for that.
priscilawebdev
pushed a commit
that referenced
this pull request
Nov 6, 2025
This PR introduces a `LinkBehaviorContext` that allows us to decouple sentry specific code from our design-system `scraps`. In this first iteration, the only sentry specific things were: - `locationDescriptorToTo` - a call to `useLocation` to fall-back to anchor rendering if there is no location. Honestly, I don’t know why / if we need that because when would have no `useLocation` ? I do plan to add more sentry specific link behavior, like [intent prefetching](#102574), but this de-coupling is pre-requisite for that.
# Conflicts: # static/app/components/core/link/link.tsx
# Conflicts: # static/app/utils/reactRouter6Compat/router.tsx
TkDodo
commented
Nov 6, 2025
Comment on lines
+33
to
+35
| <RouteConfigProvider value={router.routes}> | ||
| <RouterProvider router={router} /> | ||
| </RouteConfigProvider> |
Collaborator
Author
There was a problem hiding this comment.
note: react-router does not expose a hook that gives us access to all the routes; the recommended way is to just export your routs (“it’s just config”), but that ties it to a specific implementation and makes it hard to test.
instead, I’ve opted for another provider that allows access to the route config of the router that gets passed to RouterProvider.
This is the only place where we add this. linkBehavior reads the context to enable intent prefetching, and if there is no context, we simply don’t prefetch anything. That means we don’t need to add this provider everywhere.
Comment on lines
+18
to
+19
| const {organization} = useLegacyStore(OrganizationStore); | ||
| const intentPreloading = organization?.features?.includes('route-intent-preloading'); |
Collaborator
Author
There was a problem hiding this comment.
note: I had to useLegacyStore here because the OrganizationProvider is added further down the tree, so useOrganization({ allowNull: true}) always returns null :/
static/app/router/preload.ts
Outdated
Comment on lines
20
to
31
| Sentry.withScope(scope => { | ||
| scope.setLevel('warning'); | ||
| Sentry.captureException(error, { | ||
| tags: { | ||
| component: 'Link', | ||
| operation: 'preload', | ||
| }, | ||
| extra: { | ||
| to, | ||
| route: match.route.path, | ||
| }, | ||
| }); |
Collaborator
Author
There was a problem hiding this comment.
lmk if this should be logs instead
Member
There was a problem hiding this comment.
I would add logs instead. Async stack traces are poorly implemented in JS, so you won't see much of a gain here by using exceptions.
Collaborator
Author
|
@JonasBa I updated the tests |
JonasBa
approved these changes
Nov 7, 2025
static/app/router/preload.ts
Outdated
Comment on lines
20
to
31
| Sentry.withScope(scope => { | ||
| scope.setLevel('warning'); | ||
| Sentry.captureException(error, { | ||
| tags: { | ||
| component: 'Link', | ||
| operation: 'preload', | ||
| }, | ||
| extra: { | ||
| to, | ||
| route: match.route.path, | ||
| }, | ||
| }); |
Member
There was a problem hiding this comment.
I would add logs instead. Async stack traces are poorly implemented in JS, so you won't see much of a gain here by using exceptions.
Collaborator
Author
Comment on lines
222
to
229
| logger: { | ||
| warn: jest.fn(), | ||
| error: jest.fn(), | ||
| fatal: jest.fn(), | ||
| info: jest.fn(), | ||
| debug: jest.fn(), | ||
| trace: jest.fn(), | ||
| }, |
Member
There was a problem hiding this comment.
Why did we need this change?
Member
There was a problem hiding this comment.
probably haven't hit logs in a test before 🤷 wouldn't surprise me
Collaborator
Author
There was a problem hiding this comment.
Yeah this is generally missing. Could also extract to a separate PR?
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues:
|
TkDodo
added a commit
that referenced
this pull request
Nov 11, 2025
with [route intent preloading](#102574), we show way fewer loaders for lazy-loading assets. However, the loader still shows up when you hard reload a page, or navigate there for the first time. this still leads to cascading loaders, with the first loader (the vertically centered one) being from lazy-loading the static asset: https://github.com/user-attachments/assets/5a85bf86-74ee-4d1d-b2a9-10121bc7160f since lazy-loading static assets is usually fast, by deferring the loader by 300ms, we get a better experience for those cases, while still showing a loader should it really take some time: https://github.com/user-attachments/assets/35945187-d594-43b1-8f79-1b8bef4354c7
Jesse-Box
pushed a commit
that referenced
this pull request
Nov 12, 2025
This PR introduces a `LinkBehaviorContext` that allows us to decouple sentry specific code from our design-system `scraps`. In this first iteration, the only sentry specific things were: - `locationDescriptorToTo` - a call to `useLocation` to fall-back to anchor rendering if there is no location. Honestly, I don’t know why / if we need that because when would have no `useLocation` ? I do plan to add more sentry specific link behavior, like [intent prefetching](#102574), but this de-coupling is pre-requisite for that.
Jesse-Box
pushed a commit
that referenced
this pull request
Nov 12, 2025
This PR adds preloading to internal links. To do this, we: - expose a new `preload` function from `makeLazyLoadComponent` - attach this method to the route’s `handle` - invoke the `preload` method on hover / focus of our `<Link>` component if the Link would go to a route Note: - we attach the hover / focus event handlers to _all_ Links, but they will trigger the promise only once because it’s cached. - if a route was made without `makeLazyLoadComponent`, it won’t have a preload function so nothing happens. - only static assets are preloaded - not data calls This should ensure that navigations between routes, when going there the first time, are faster and we don’t have to show loading spinners that often. Combined with [deferring the fallback for lazy loaded files](#102031), we should get a smooth ux for first navigations. before (notice the big spinner in the middle of the page on each navigation): after (notice the big spinner in the middle of the page is gone):
Jesse-Box
pushed a commit
that referenced
this pull request
Nov 12, 2025
with [route intent preloading](#102574), we show way fewer loaders for lazy-loading assets. However, the loader still shows up when you hard reload a page, or navigate there for the first time. this still leads to cascading loaders, with the first loader (the vertically centered one) being from lazy-loading the static asset: https://github.com/user-attachments/assets/5a85bf86-74ee-4d1d-b2a9-10121bc7160f since lazy-loading static assets is usually fast, by deferring the loader by 300ms, we get a better experience for those cases, while still showing a loader should it really take some time: https://github.com/user-attachments/assets/35945187-d594-43b1-8f79-1b8bef4354c7
andrewshie-sentry
pushed a commit
that referenced
this pull request
Nov 13, 2025
This PR introduces a `LinkBehaviorContext` that allows us to decouple sentry specific code from our design-system `scraps`. In this first iteration, the only sentry specific things were: - `locationDescriptorToTo` - a call to `useLocation` to fall-back to anchor rendering if there is no location. Honestly, I don’t know why / if we need that because when would have no `useLocation` ? I do plan to add more sentry specific link behavior, like [intent prefetching](#102574), but this de-coupling is pre-requisite for that.
andrewshie-sentry
pushed a commit
that referenced
this pull request
Nov 13, 2025
This PR adds preloading to internal links. To do this, we: - expose a new `preload` function from `makeLazyLoadComponent` - attach this method to the route’s `handle` - invoke the `preload` method on hover / focus of our `<Link>` component if the Link would go to a route Note: - we attach the hover / focus event handlers to _all_ Links, but they will trigger the promise only once because it’s cached. - if a route was made without `makeLazyLoadComponent`, it won’t have a preload function so nothing happens. - only static assets are preloaded - not data calls This should ensure that navigations between routes, when going there the first time, are faster and we don’t have to show loading spinners that often. Combined with [deferring the fallback for lazy loaded files](#102031), we should get a smooth ux for first navigations. before (notice the big spinner in the middle of the page on each navigation): after (notice the big spinner in the middle of the page is gone):
andrewshie-sentry
pushed a commit
that referenced
this pull request
Nov 13, 2025
with [route intent preloading](#102574), we show way fewer loaders for lazy-loading assets. However, the loader still shows up when you hard reload a page, or navigate there for the first time. this still leads to cascading loaders, with the first loader (the vertically centered one) being from lazy-loading the static asset: https://github.com/user-attachments/assets/5a85bf86-74ee-4d1d-b2a9-10121bc7160f since lazy-loading static assets is usually fast, by deferring the loader by 300ms, we get a better experience for those cases, while still showing a loader should it really take some time: https://github.com/user-attachments/assets/35945187-d594-43b1-8f79-1b8bef4354c7
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds preloading to internal links. To do this, we:
preloadfunction frommakeLazyLoadComponenthandlepreloadmethod on hover / focus of our<Link>component if the Link would go to a routeNote:
makeLazyLoadComponent, it won’t have a preload function so nothing happens.This should ensure that navigations between routes, when going there the first time, are faster and we don’t have to show loading spinners that often. Combined with deferring the fallback for lazy loaded files, we should get a smooth ux for first navigations.
before (notice the big spinner in the middle of the page on each navigation):
after (notice the big spinner in the middle of the page is gone):