feat(vue): Add TanStack Router integration #18547
Conversation
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
s1gr1d
left a comment
s1gr1d
left a comment
There was a problem hiding this comment.
Solid implementation, nice readme for the E2E test, well documented lines (e.g. in the config files), well-used span attributes and readable code 💯
|
|
||
| const initialWindowLocation = WINDOW.location; | ||
| if (instrumentPageLoad && initialWindowLocation) { | ||
| const matchedRoutes = router.matchRoutes( |
There was a problem hiding this comment.
Will this always return an array? Because you do matchedRoutes.length later.
There was a problem hiding this comment.
They don't really have the docs/types ready yet, but it does so in their react version: https://tanstack.com/router/latest/docs/framework/react/api/router/RouterType#matchroutes-method so I think this is probably going to be fine.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| ...routeMatchToParamSpanAttributes(lastMatch), | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: Pageload span starts at wrong time
startBrowserTracingPageLoadSpan is called without a startTime, so the pageload root span starts “now” (during afterAllSetup) instead of at the navigation time origin. This can skew pageload transaction timing and related performance interpretation compared to the default browser tracing behavior.
There was a problem hiding this comment.
Fair, same thing exists in the other tanstack routers. Will fix this on its own PR.
andreiborza
force-pushed
the
ab/tanstack-router-vue-integration
branch
from
9a0effe to
e577368
Compare
| navigationSpan.setAttributes(routeMatchToParamSpanAttributes(onResolvedLastMatch)); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Bug: Navigation listeners may leak on aborted navigations
Each onBeforeNavigate call registers an onResolved subscription that only unsubscribes inside the onResolved callback. If a navigation is cancelled/errors and onResolved never fires, the handler remains subscribed, potentially accumulating listeners over time and causing extra work on later navigations.
There was a problem hiding this comment.
Also exists in the other tanstack router integrations we have, will investigate in a separate issue.
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.vue.tanstack_router', | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: onBeforeNavigateLastMatch ? 'route' : 'url', | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Bug: Navigation spans omit URL snapshot at start
startBrowserTracingNavigationSpan is called without the url option. This prevents BrowserTracing from snapshotting the navigation URL in scope metadata at span creation time, which can lead to transactions being associated with an incorrect URL if WINDOW.location changes again before the span is sent.
There was a problem hiding this comment.
Also fair, also exists in the other. Will fix in followup.
andreiborza
force-pushed
the
ab/tanstack-router-vue-integration
branch
from
6ba0963 to
e6ebbc8
Compare
| : // In SSR/non-browser contexts, WINDOW.location may be undefined, so fall back to the router's location | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| navigationLocation?.pathname || onBeforeNavigateArgs.toLocation.pathname, | ||
| attributes: { |
There was a problem hiding this comment.
Bug: Navigation fallback uses previous pathname
When there is no route match during onBeforeNavigate, the navigation span name falls back to WINDOW.location.pathname, which is still the pre-navigation URL. This can mislabel navigation transactions for unmatched/404 routes (or transitional states) as the previous page instead of toLocation.pathname.
Closes: #18535