fix(react): Prevent navigation span leaks for consecutive navigations #18098
Conversation
onurtemizkan
force-pushed
the
onur/fix-react-router-navigation-leak
branch
from
63ebcd0 to
d720cad
Compare
size-limit report 📦
|
onurtemizkan
force-pushed
the
onur/fix-react-router-navigation-leak
branch
4 times, most recently
from
cb20156 to
f703ae9
Compare
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.
|
onurtemizkan
force-pushed
the
onur/fix-react-router-navigation-leak
branch
from
f703ae9 to
23f63b6
Compare
onurtemizkan
force-pushed
the
onur/fix-react-router-navigation-leak
branch
from
59e8f76 to
6dbf27e
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| source, | ||
| version, | ||
| location, | ||
| routes, | ||
| basename, | ||
| allRoutes, | ||
| navigationKey: currentNavigationKey, | ||
| }); | ||
|
|
||
| // Patch navigation span to handle early cancellation (e.g., document.hidden) | ||
| if (navigationSpan) { | ||
| patchNavigationSpanEnd(navigationSpan, location, routes, basename, allRoutes); | ||
| } | ||
| } else if (isNavDuplicate && isAlreadyInNavigationSpan && activeSpan) { | ||
| tryUpdateSpanName(activeSpan, spanJson?.description, name, source); | ||
| } |
There was a problem hiding this comment.
Bug: Stale LAST_NAVIGATION_PER_CLIENT entry can cause subsequent navigations to be silently ignored if span creation fails.
Severity: HIGH | Confidence: 0.90
🔍 Detailed Analysis
If startBrowserTracingNavigationSpan() returns undefined (e.g., if no idle span is created/set), the LAST_NAVIGATION_PER_CLIENT entry is still set, but the cleanup listener is not registered. This leaves a stale timestamp entry in LAST_NAVIGATION_PER_CLIENT. Consequently, a subsequent navigation to the same location within 100ms will be incorrectly identified as a duplicate by isDuplicateNavigation() and silently ignored, leading to missing navigation spans.
💡 Suggested Fix
Ensure LAST_NAVIGATION_PER_CLIENT entries are always cleaned up, even if startBrowserTracingNavigationSpan() returns undefined, or modify isDuplicateNavigation() to account for cases where no span was successfully created for the initial navigation.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/react/src/reactrouter-compat-utils/instrumentation.tsx#L717-L731
Potential issue: If `startBrowserTracingNavigationSpan()` returns `undefined` (e.g., if
no idle span is created/set), the `LAST_NAVIGATION_PER_CLIENT` entry is still set, but
the cleanup listener is not registered. This leaves a stale timestamp entry in
`LAST_NAVIGATION_PER_CLIENT`. Consequently, a subsequent navigation to the same location
within 100ms will be incorrectly identified as a duplicate by `isDuplicateNavigation()`
and silently ignored, leading to missing navigation spans.
Did we get this right? 👍 / 👎 to inform future reviews.
onurtemizkan
force-pushed
the
onur/fix-react-router-navigation-leak
branch
from
eedce19 to
11d1a6a
Compare
s1gr1d
left a comment
s1gr1d
left a comment
There was a problem hiding this comment.
I think this is good to go, I just have some minor comments
| transactionEvent.contexts?.trace?.op === 'navigation' && | ||
| transactionEvent.transaction === '/another-lazy/sub/:id/:subId' | ||
| ); | ||
| }); |
There was a problem hiding this comment.
You could move the waitForTransaction calls to the beginning of the test and then do the .locator and .click calls for the navigations right after one another.
I am not sure how long the test takes in between the calls right now, so you can also leave it like it is if you think it's already very fast.
| const shouldHandleNavigation = | ||
| state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete); | ||
| (state.historyAction === 'PUSH' || (state.historyAction === 'POP' && isInitialPageloadComplete)) && | ||
| state.navigation.state === 'idle'; | ||
|
|
||
| if (shouldHandleNavigation) { |
There was a problem hiding this comment.
Maybe this value can be gathered from a function if(shouldHandleNavigation(state, isInitialPageLoadComplete))
As this code is already used two times
| newName: string, | ||
| newSource: string, | ||
| ): void { | ||
| const isNewNameBetter = newName !== currentSpanName && newName.includes(':'); |
There was a problem hiding this comment.
Maybe we can name this differently to indicate what "better" means :D
Something like isNewNameParametrized or similar
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical bug where consecutive navigations to different routes in React Router v6/v7 applications failed to create separate navigation spans, causing span leaks and missing transaction data. The fix introduces a navigation key tracking mechanism to distinguish between cross-usage scenarios (multiple wrappers instrumenting the same navigation) and legitimate consecutive navigations to different routes.
Key Changes:
- Introduced
LAST_NAVIGATION_PER_CLIENTWeakMap to track active navigation keys per client - Modified navigation span creation logic to compare navigation keys (pathname + search + hash) instead of just checking for active navigation spans
- Refactored
getNormalizedNamefunction in utils.ts for better readability using early returns/continues - Added condition to only handle navigation when router state is 'idle' to prevent creating spans for incomplete navigations
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
packages/react/src/reactrouter-compat-utils/instrumentation.tsx |
Core implementation: added LAST_NAVIGATION_PER_CLIENT tracking, navigation key comparison logic, and helper functions for duplicate detection and span name updates |
packages/react/src/reactrouter-compat-utils/utils.ts |
Refactored getNormalizedName function to use early continue/return patterns for improved readability and added getFallbackTransactionName helper |
packages/react/test/reactrouterv6.test.tsx |
Added allRoutes.clear() in beforeEach for test isolation |
packages/react/test/reactrouter-descendant-routes.test.tsx |
Added allRoutes.clear() in beforeEach for test isolation |
packages/react/test/reactrouter-cross-usage.test.tsx |
Updated cross-usage test expectations to verify span updates instead of duplicate span creation, and added comprehensive tests for consecutive navigations scenarios |
dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts |
Added e2e test verifying separate transactions are created for rapid consecutive navigations with unique trace and span IDs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| newName: string, | ||
| newSource: string, | ||
| ): void { | ||
| const isNewNameBetter = newName !== currentSpanName && newName.includes(':'); |
There was a problem hiding this comment.
The condition newName.includes(':') may not correctly identify all parameterized routes. For example:
- Routes with regex patterns like
/user/[0-9]+won't be detected - Routes with wildcard patterns like
/posts/*contain:but aren't necessarily "better" parameterized names - Non-parameterized routes like
/api/v1:stablethat legitimately contain:would trigger false positives
Consider a more robust check, such as verifying the route contains React Router parameter syntax specifically (e.g., checking for patterns like /:param/ or ending with /:param).
| const isNewNameBetter = newName !== currentSpanName && newName.includes(':'); | |
| const isReactRouterParam = /\/:[a-zA-Z0-9_]+/.test(newName); | |
| const isNewNameBetter = newName !== currentSpanName && isReactRouterParam; |
| * Adds resolved routes as children to the parent route. | ||
| * Prevents duplicate routes by checking if they already exist. | ||
| * Tracks last navigation per client to prevent duplicate spans in cross-usage scenarios. | ||
| * Entry persists until next different navigation, handling delayed wrapper execution. |
There was a problem hiding this comment.
The comment "Entry persists until next different navigation" is slightly misleading. Looking at the implementation in createNavigationSpan (lines 663-672), the entry is cleared when the navigation span ends (via the spanEnd event), not when the "next different navigation" occurs. Consider rephrasing to: "Entry persists until the navigation span ends, allowing cross-usage detection during delayed wrapper execution."
| * Entry persists until next different navigation, handling delayed wrapper execution. | |
| * Entry persists until the navigation span ends, allowing cross-usage detection during delayed wrapper execution. |
Fixes an issue where consecutive navigations to different routes fail to create separate navigation spans, causing span leaks and missing transaction data.
This came up in a React Router v6/v7 application where the pageload / navigation transactions take longer and there is a high
finalTimeoutset in config. When users navigate between different routes (e.g.,/users/:id→/projects/:projectId→/settings). The SDK was incorrectly preventing new navigation spans from being created whenever an ongoing navigation span was active, regardless of whether the navigation was to a different route. This resulted in only the first navigation being tracked, with subsequent navigations being silently ignored. Also, the spans that needed to be a part of the subsequent navigation were recorded as a part of the previous one.The root cause was the
if (!isAlreadyInNavigationSpan)check that we used to prevent cross-usage scenarios (multiple wrappers instrumenting the same navigation), which incorrectly blocked legitimate consecutive navigations to different routes.So, this fix changes the logic to check both navigation span state and the route name:
isSpanForSameRoute = isAlreadyInNavigationSpan && spanJson?.description === name. This allows consecutive navigations to different routes while preventing duplicate spans for the same route.Also added tracking using
LAST_NAVIGATION_PER_CLIENT. When multiple wrappers (e.g.,wrapCreateBrowserRouter+wrapUseRoutes) instrument the same application, they may each trigger span creation for the same navigation event. We store the navigation key${location.pathname}${location.search}${location.hash}while the span is active and clear it when that span ends.If the same navigation key shows up again before the original span finishes, the second wrapper updates that span’s name if it has better parameterization instead of creating a duplicate, which keeps cross-usage covered.