Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(react): Patch spanEnd for potentially cancelled lazy-route transactions#17962
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
fa46b4abd6da33657256ee7270bcbf7633a30860c88f6671bFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -8,7 +8,7 @@ import { | ||
| startBrowserTracingPageLoadSpan, | ||
| WINDOW, | ||
| } from '@sentry/browser'; | ||
| import type { Client, Integration, Span, TransactionSource } from '@sentry/core'; | ||
| import type { Client, Integration, Span } from '@sentry/core'; | ||
| import { | ||
| addNonEnumerableProperty, | ||
| debug, | ||
| @@ -41,14 +41,7 @@ import type { | ||
| UseRoutes, | ||
| } from '../types'; | ||
| import { checkRouteForAsyncHandler } from './lazy-routes'; | ||
| import { | ||
| getNormalizedName, | ||
| initializeRouterUtils, | ||
| locationIsInsideDescendantRoute, | ||
| prefixWithSlash, | ||
| rebuildRoutePathFromAllRoutes, | ||
| resolveRouteNameAndSource, | ||
| } from './utils'; | ||
| import { initializeRouterUtils, resolveRouteNameAndSource } from './utils'; | ||
| let _useEffect: UseEffect; | ||
| let _useLocation: UseLocation; | ||
| @@ -667,14 +660,19 @@ export function handleNavigation(opts: { | ||
| // Cross usage can result in multiple navigation spans being created without this check | ||
| if (!isAlreadyInNavigationSpan) { | ||
| startBrowserTracingNavigationSpan(client, { | ||
| const navigationSpan = startBrowserTracingNavigationSpan(client, { | ||
| name, | ||
| attributes: { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`, | ||
| }, | ||
| }); | ||
| // Patch navigation span to handle early cancellation (e.g., document.hidden) | ||
| if (navigationSpan) { | ||
| patchNavigationSpanEnd(navigationSpan, location, routes, basename, allRoutes); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| @@ -727,29 +725,104 @@ function updatePageloadTransaction({ | ||
| : (_matchRoutes(allRoutes || routes, location, basename) as unknown as RouteMatch[]); | ||
| if (branches) { | ||
| let name, | ||
| source: TransactionSource = 'url'; | ||
| const isInDescendantRoute = locationIsInsideDescendantRoute(location, allRoutes || routes); | ||
| if (isInDescendantRoute) { | ||
| name = prefixWithSlash(rebuildRoutePathFromAllRoutes(allRoutes || routes, location)); | ||
| source = 'route'; | ||
| } | ||
| if (!isInDescendantRoute || !name) { | ||
| [name, source] = getNormalizedName(routes, location, branches, basename); | ||
| } | ||
| const [name, source] = resolveRouteNameAndSource(location, routes, allRoutes || routes, branches, basename); | ||
| getCurrentScope().setTransactionName(name || '/'); | ||
| if (activeRootSpan) { | ||
| activeRootSpan.updateName(name); | ||
| activeRootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); | ||
| // Patch span.end() to ensure we update the name one last time before the span is sent | ||
| patchPageloadSpanEnd(activeRootSpan, location, routes, basename, allRoutes); | ||
| } | ||
| } | ||
| } | ||
| /** | ||
| * Patches the span.end() method to update the transaction name one last time before the span is sent. | ||
| * This handles cases where the span is cancelled early (e.g., document.hidden) before lazy routes have finished loading. | ||
| */ | ||
| function patchSpanEnd( | ||
| span: Span, | ||
| location: Location, | ||
| routes: RouteObject[], | ||
| basename: string | undefined, | ||
| _allRoutes: RouteObject[] | undefined, | ||
| spanType: 'pageload' | 'navigation', | ||
| ): void { | ||
| const patchedPropertyName = `__sentry_${spanType}_end_patched__` as const; | ||
| const hasEndBeenPatched = (span as unknown as Record<string, boolean | undefined>)?.[patchedPropertyName]; | ||
| if (hasEndBeenPatched || !span.end) { | ||
| return; | ||
| } | ||
| const originalEnd = span.end.bind(span); | ||
| span.end = function patchedEnd(...args) { | ||
| try { | ||
| // Only update if the span source is not already 'route' (i.e., it hasn't been parameterized yet) | ||
| const spanJson = spanToJSON(span); | ||
| const currentSource = spanJson.data?.[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]; | ||
| if (currentSource !== 'route') { | ||
| // Last chance to update the transaction name with the latest route info | ||
| // Use the live global allRoutes Set to include any lazy routes loaded after patching | ||
| const currentAllRoutes = Array.from(allRoutes); | ||
| const branches = _matchRoutes( | ||
| currentAllRoutes.length > 0 ? currentAllRoutes : routes, | ||
| location, | ||
| basename, | ||
| ) as unknown as RouteMatch[]; | ||
| if (branches) { | ||
| const [name, source] = resolveRouteNameAndSource( | ||
| location, | ||
| routes, | ||
| currentAllRoutes.length > 0 ? currentAllRoutes : routes, | ||
| branches, | ||
| basename, | ||
| ); | ||
| // Only update if we have a valid name | ||
| if (name && (spanType === 'pageload' || !spanJson.timestamp)) { | ||
| span.updateName(name); | ||
| span.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source); | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // Silently catch errors to ensure span.end() is always called | ||
| DEBUG_BUILD && debug.warn(`Error updating span details before ending: ${error}`); | ||
| } | ||
| return originalEnd(...args); | ||
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. | ||
| }; | ||
| // Mark this span as having its end() method patched to prevent duplicate patching | ||
| addNonEnumerableProperty(span as unknown as Record<string, boolean>, patchedPropertyName, true); | ||
| } | ||
| function patchPageloadSpanEnd( | ||
| span: Span, | ||
| location: Location, | ||
| routes: RouteObject[], | ||
| basename: string | undefined, | ||
| _allRoutes: RouteObject[] | undefined, | ||
| ): void { | ||
| patchSpanEnd(span, location, routes, basename, _allRoutes, 'pageload'); | ||
| } | ||
| function patchNavigationSpanEnd( | ||
| span: Span, | ||
| location: Location, | ||
| routes: RouteObject[], | ||
| basename: string | undefined, | ||
| _allRoutes: RouteObject[] | undefined, | ||
| ): void { | ||
| patchSpanEnd(span, location, routes, basename, _allRoutes, 'navigation'); | ||
| } | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export function createV6CompatibleWithSentryReactRouterRouting<P extends Record<string, any>, R extends React.FC<P>>( | ||
| Routes: R, | ||
Uh oh!
There was an error while loading. Please reload this 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.
We have
client.on("spanEnd"). Is it possible to use it here? Then it would be more transparent and aligned with the other implementations.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.
I think that's not usable in this case, as we need to update the span name/source right before it ends. Looks like
client.on('spanEnd')runs when the span is not mutable anymore.