Skip to content

Comments

fix(react): Add POP guard for long-running pageload spans#17867

Merged
chargome merged 8 commits intodevelopfrom
onur/react-router-long-running-pageload-guard
Oct 15, 2025
Merged

fix(react): Add POP guard for long-running pageload spans#17867
chargome merged 8 commits intodevelopfrom
onur/react-router-long-running-pageload-guard

Conversation

@onurtemizkan
Copy link
Collaborator

@onurtemizkan onurtemizkan commented Oct 6, 2025

This resolves the issue that occurs when an extra navigation transaction is created after a prematurely ended pageload transaction in React Router lazy routes.

This apparently occurs when there's a long-running pageload with lazy-routes (after fetching assets, there are multiple potentially long-running API calls happening).

This causes the pageload transaction to prematurely end, even before the fully parameterized transaction name is resolved. The reason is that there can be a POP event emitted, which we subscribe to create a navigation transaction. This ends the ongoing pageload transaction before its name is updated with a resolved parameterized route path, and starts a navigation transaction, which contains the remaining spans that were supposed to be a part of the pageload transaction.

This fix makes sure the initial POP events are not necessarily treated as navigation pointers, which should fix both:

  • Duplicate / extra navigation transactions having a part of pageload spans.
  • Remaining wildcards in the pageload transaction names

@onurtemizkan onurtemizkan force-pushed the onur/react-router-long-running-pageload-guard branch from c5b63b9 to b3027fd Compare October 6, 2025 11:21
@onurtemizkan onurtemizkan marked this pull request as ready for review October 6, 2025 12:14
cursor[bot]

This comment was marked as outdated.

Copy link
Member

@chargome chargome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find! Any way we can test this in e2e?

cursor[bot]

This comment was marked as outdated.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2025

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.

Scenario Requests/s % of Baseline Prev. Requests/s Change %
GET Baseline 9,373 - 8,903 +5%
GET With Sentry 1,378 15% 1,367 +1%
GET With Sentry (error only) 6,233 66% 6,204 +0%
POST Baseline 1,193 - 1,208 -1%
POST With Sentry 514 43% 526 -2%
POST With Sentry (error only) 1,068 90% 1,069 -0%
MYSQL Baseline 3,403 - 3,350 +2%
MYSQL With Sentry 511 15% 438 +17%
MYSQL With Sentry (error only) 2,810 83% 2,715 +3%

View base workflow run

cursor[bot]

This comment was marked as outdated.

@onurtemizkan onurtemizkan force-pushed the onur/react-router-long-running-pageload-guard branch from 68e2702 to 5608b63 Compare October 9, 2025 13:14
@onurtemizkan
Copy link
Collaborator Author

@chargome - I updated the PR with edge case handling + E2E tests

@onurtemizkan onurtemizkan force-pushed the onur/react-router-long-running-pageload-guard branch from ec0a205 to 1fb8a6d Compare October 15, 2025 11:37
@chargome chargome self-assigned this Oct 15, 2025
@chargome chargome merged commit fc64c47 into develop Oct 15, 2025
91 checks passed
@chargome chargome deleted the onur/react-router-long-running-pageload-guard branch October 15, 2025 11:51
timfish pushed a commit that referenced this pull request Oct 15, 2025
This resolves the issue that occurs when an extra `navigation`
transaction is created after a prematurely ended `pageload` transaction
in React Router lazy routes.

This apparently occurs when there's a long-running pageload with
lazy-routes (after fetching assets, there are multiple potentially
long-running API calls happening).

This causes the `pageload` transaction to prematurely end, even before
the fully parameterized transaction name is resolved. The reason is that
there can be a `POP` event emitted, which we subscribe to create a
`navigation` transaction. This ends the ongoing `pageload` transaction
before its name is updated with a resolved parameterized route path, and
starts a `navigation` transaction, which contains the remaining spans
that were supposed to be a part of the `pageload` transaction.

This fix makes sure the initial `POP` events are not necessarily treated
as `navigation` pointers, which should fix both:

- Duplicate / extra `navigation` transactions having a part of
`pageload` spans.
- Remaining wildcards in the `pageload` transaction names
onurtemizkan added a commit that referenced this pull request Feb 10, 2026
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants