-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(ui): always render tabs in insights/frontend to avoid flash during first navigation #100915
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
base: master
Are you sure you want to change the base?
Conversation
…g first navigation
|
||
return ( | ||
<Fragment> | ||
{handle && 'module' in handle ? <FrontendHeader module={handle.module} /> : null} |
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.
note: there are sub-routes that do not get the module
passed, and in those cases, we don’t want to render the tabs at all. This basically applies to all detail pages you can get to from the top landing pages.
The check is done with ’module’ in handle
because for the Overview
page, we need to explicitly pass handle: undefined
because there is no explicit ModuleName
for the Overview page.
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.
Massive improvement, thank you for tackling this! Curious about naming but only if this is a pattern that's going to start proliferating. If this is intended as more of a temporary/migration step, I think we should add a comment to that effect.
handle is the official name in react router for this: reactrouter.com/how-to/using-handle
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.
Would love someone from @getsentry/insights to weigh in before you merge, but thrilled to see this 🙌
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.
👍🏻 excellent, thank you! No notes!
In the current UI, wherever we use top-level tabs, we can see a full flash of an empty page when doing the first navigation, which is pretty annoying and not great UX:
Screen.Recording.2025-10-03.at.22.18.43.mov
The technical problem is that we lazy-load each route, but each route is also responsible for rendering its own tabs. That means when we navigate through the tabs, the tabs unmount.
This PR introduces a solution where we use nested layouts to render the tabs instead of having the tabs be part of each separate page.
That means the tabs will stay mounted while only its content lazy-loads, which leads to a lot better UX:
Screen.Recording.2025-10-03.at.22.17.17.mov
Technically, we render a
layout.tsx
file at the root of/frontend
, which will render the<FrontendHeader>
for all sub-routes plus an<Outlet>
for the children.The problem is that the layout must know which “child” it is going to render in order to select the current tab.
With useMatches, we can take a look at what renders at runtime, and then “inspect” the last match to see what child it is. We could just look at the url path segment but that’s a bit brittle, so I opted for using route handles.
Route handles are often used to build dynamic UI elements like breadcrumbs based on the route hierarchy. Here, we just use it to pass custom information from the route tree (where we define our routes) to the layout, so that it knows what to do. In this case, we pass the
ModuleName
of the rendered page so that we can pass that to the<FrontendHeader>
component. This sadly isn’t type-safe (react-router doesn’t do type-safety like that), but I think this is a fine trade-off.