Uh oh!
There was an error while loading. Please reload this page.
fix(react): Add transaction name guards for rapid lazy-route navigations - #18346
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds transaction name guards to prevent corruption when users rapidly navigate between lazy routes before lazy handlers complete resolution. The fix captures the location at the time a lazy handler is invoked and verifies it still matches the current location before updating transaction names.
- Captures window location when lazy route handlers are invoked
- Adds guard logic to skip span updates if user has navigated away
- Includes comprehensive e2e tests for various rapid navigation scenarios
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/reactrouter-compat-utils/lazy-routes.tsx | Added captureCurrentLocation() function and modified proxy to capture location at invocation time, passing it through to processResolvedRoutes |
| packages/react/src/reactrouter-compat-utils/instrumentation.tsx | Added guard logic to compare captured location with current window location, skipping span updates when they don't match |
| packages/react/test/reactrouter-compat-utils/lazy-routes.test.ts | Updated test expectations to include new currentLocation parameter and added test for null-to-undefined conversion |
| dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts | Added seven new e2e tests covering rapid navigation, zero-wait navigation, browser back, and multiple overlapping handlers |
| dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/SlowFetchLazyRoutes.tsx | New test page with 500ms delay and top-level await to simulate slow lazy route loading |
| dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/Index.tsx | Added navigation link to slow-fetch route for testing |
| dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/DelayedLazyRoute.tsx | Added navigation links for testing rapid navigation between routes |
| dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/index.tsx | Added slow-fetch route configuration with lazy handler |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| const windowLocation = WINDOW.location; | ||
| if (windowLocation) { | ||
| const capturedKey = computeLocationKey(currentLocation); | ||
| const currentKey = `${windowLocation.pathname}${windowLocation.search || ''}${windowLocation.hash || ''}`; |
There was a problem hiding this comment.
The location key is computed twice: once using computeLocationKey() for currentLocation, and once manually for windowLocation. This duplicates the logic and increases the risk of inconsistency. Consider using computeLocationKey() for both, or extracting the manual computation into a reusable helper that accepts a window.location-like object.
| constcurrentKey=`${windowLocation.pathname}${windowLocation.search||''}${windowLocation.hash||''}`; | |
| constcurrentKey=computeLocationKey(windowLocation); |
| expect(mockProcessResolvedRoutes).toHaveBeenCalledWith(routes, route, mockLocation); | ||
| }); | ||
| it('should pass null location when currentLocation is null', () => { |
There was a problem hiding this comment.
The test name "should pass null location when currentLocation is null" is misleading because the test actually verifies that null is converted to undefined when passed to processResolvedRoutes. Consider renaming to "should convert null location to undefined for processResolvedRoutes" to better reflect what is being tested.
| it('should pass null location when currentLocation is null',()=>{ | |
| it('should convert null location to undefined for processResolvedRoutes',()=>{ |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
4f05cf5 to
1deccb8CompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Uh oh!
There was an error while loading. Please reload this page.
29a8700 to
1deccb8CompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
9d875a8 to
b270752Compareb270752 to
8ae6a96CompareUh oh!
There was an error while loading. Please reload this page.
s1gr1d
left a comment
There was a problem hiding this comment.
Looks good, also thanks for adding those tests.
Uh oh!
There was an error while loading. Please reload this page.
…ad (#18881) Following up on #18155 and #18346 Fixes an issue where pageload transactions have incorrect names (URL-based or wildcard-based) when lazy routes load after the span ends due to idle timeout. This occurs when using patchRoutesOnNavigation for lazy route loading. The idle timeout can fire before lazy routes finish loading, causing the span to end with a wildcard like `/slow-fetch/*` instead of the parameterized `/slow-fetch/:id`. The root cause was that the active span was captured after router creation, making it inaccessible when `patchRoutesOnNavigation` was called later (span already ended). Also, `patchSpanEnd` was taking a snapshot of `allRoutes`, so lazy routes added later wouldn't be visible. This fix: - Captures the active span before router creation and passes it to `wrapPatchRoutesOnNavigation` - Adds a deferred promise mechanism that blocks span finalization until `patchRoutesOnNavigation` completes - Uses the global `allRoutes` Set directly instead of a snapshot - Handles pageload spans in `patchRoutesOnNavigation` (was only handling navigation before)
…n span (#18898) Following up on #18346 Fixes a remaining race condition where the `args.patch` callback and completion handler in `wrapPatchRoutesOnNavigation` were still calling `getActiveRootSpan()` at resolution time instead of using the captured span. This caused wrong span updates when users navigated away during lazy route loading. The fix uses the captured `activeRootSpan` consistently, adds a `!spanJson.timestamp` check to skip updates on ended spans, and removes the `WINDOW.location` fallback. In `captureCurrentLocation`, returns `null` when navigation context exists but `targetPath` is `undefined`, instead of falling back to stale `WINDOW.location`. Also added E2E tests that simulate rapid navigation scenarios where a slow lazy route handler completes after the user navigated elsewhere.
…es (#19086) This PR will resolve the core reason for the series of fixes / handling for automatic lazy-route resolution for a while. Related: #18898, #18881, #18346, #18155, #18098, #17962, #17867, #17438, #17277 The core issue we have been trying to tackle is not having access to the complete route hierarchy when asynchronously loaded lazy routes are used. React Router provides a route manifest that we can use while matching parameterized transaction names with routes in all cases except this lazy-routes pattern. This problem has been discussed on React Router: - remix-run/react-router#11113 While this has been [addressed](remix-run/react-router#11626) for Remix / React Router (Framework Mode), it's still not available in Library Mode. The manifest contains the lazily-loaded route, only when it's navigated to. While waiting for navigation, our transactions can be dropped for several reasons, such as user behaviour like switching tabs (`document.hidden` guard), hitting timeouts like `idleTimeout`, and potentially other reasons. This results in incomplete transaction naming with leftover wildcards, which caused broken aggregation on the Sentry dashboard. The series of attempts to fix this while keeping automatic route discovery has been prone to race conditions and required special-case handling of each edge case scenario, also requiring a considerable amount of internal logic, affecting our readability and performance. At the end, all failed in giving completely robust and deterministic results on the customers' side. This PR proposes a new option: `lazyRouteManifest` specifically for lazy routes. This will let us have initial information about the route hierarchy. So we can assign correct parameterized transaction names without needing to wait for navigated state. It's a static array of routes in parameterized format (needs to be maintained by the users on route hierarchy updates) like: ```ts Sentry.reactRouterV7BrowserTracingIntegration({ // ... enableAsyncRouteHandlers: true lazyRouteManifest: [ '/', '/pricing', '/features', '/login', '/signup', '/forgot-password', '/reset-password/:token', '/org/:orgSlug', '/org/:orgSlug/dashboard', '/org/:orgSlug/projects', '/org/:orgSlug/projects/:projectId', '/org/:orgSlug/projects/:projectId/settings', '/org/:orgSlug/projects/:projectId/issues', '/org/:orgSlug/projects/:projectId/issues/:issueId', '/org/:orgSlug/team', '/org/:orgSlug/team/:memberId', '/org/:orgSlug/settings', '/org/:orgSlug/billing', '/admin', '/admin/users', '/admin/users/:userId', '/admin/orgs', '/admin/orgs/:orgId', ], }) ``` - This will only be active when `enableAsyncRouteHandlers` is set to `true` - To match URLs with given routes, we mimic React Router's own implementation. - When this is not provided or fails, it falls back to the current behaviour - This manifest is primarily for lazy routes, but the users can also add their non-lazy routes here for convenience or consistency. - Also added E2E tests that will fail when (if at some point) React Router manifests include the lazy routes before navigation, so we'll be aware and plan depending on that manifest instead. - We can do a cleanup for the race-condition / edge-case handling part of the code in a follow-up PR. Closes#19090 (added automatically)
Following up on #18098 and #18155
Fixes a race condition where rapid navigations between lazy routes cause transaction names to be incorrectly assigned or corrupted.
This occurs when async lazy handlers resolve after the user already navigated to a different route. The root cause was
captureCurrentLocation()was readingwindow.locationat the time of handler invocation, but duringpatchRoutesOnNavigation, the browser URL hasn't actually updated yet. So when handlers invoked during navigation A resolve later, they would capture navigation B's location (or whatever the current URL is), leading to incorrect span updates.The fix introduces a navigation context mechanism that captures both the correct
targetPathand the active span at the start ofpatchRoutesOnNavigation, making them available to async handler proxies during invocation. This ensures handlers always use the navigation context they were invoked with, not the current browser state.The navigation context uses a stack to properly handle overlapping/concurrent
patchRoutesOnNavigationcalls. If a second navigation starts before the first one's handlers complete, each navigation maintains its own captured context.