ref(react): Add more guarding against wildcards in lazy route transactions #18155
Conversation
onurtemizkan
force-pushed
the
onur/more-guarding-against-wildcard-transactions
branch
from
7a6e14e to
46df23c
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.
|
size-limit report 📦
|
onurtemizkan
force-pushed
the
onur/more-guarding-against-wildcard-transactions
branch
from
8468d79 to
179f040
Compare
onurtemizkan
force-pushed
the
onur/more-guarding-against-wildcard-transactions
branch
from
179f040 to
c055e02
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
packages/react/src/reactrouter-compat-utils/instrumentation.tsx
Outdated
Show resolved
Hide resolved
s1gr1d
requested review from
Lms24 and
Copilot
and removed request for
Lms24
There was a problem hiding this comment.
Pull Request Overview
This PR adds additional guards against wildcard routes in lazy route transactions for React Router integrations. The main goal is to prevent premature reporting of transactions with unresolved (wildcard) route names by introducing a configurable wait timeout for lazy route resolution.
Key Changes:
- Added
transactionNameHasWildcard()utility function to detect wildcard characters in route names - Introduced
maxLazyRouteWaitMsconfiguration option (defaults toidleTimeout * 3) to delay span finalization until lazy routes resolve - Enhanced
updateNavigationSpan,handleNavigation, andpatchSpanEndwith wildcard detection and route upgrade logic
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/reactrouter-compat-utils/utils.ts | Added transactionNameHasWildcard() utility function to detect wildcards in transaction names |
| packages/react/src/reactrouter-compat-utils/instrumentation.tsx | Core implementation changes including wildcard checking, timeout handling, lazy route tracking, and span name upgrade logic |
| packages/react/src/reactrouter-compat-utils/index.ts | Exported new transactionNameHasWildcard utility function |
| packages/react/test/reactrouter-compat-utils/utils.test.ts | Added comprehensive test coverage for transactionNameHasWildcard() function |
| packages/react/test/reactrouter-compat-utils/instrumentation.test.tsx | Added tests for wildcard detection and source upgrade/downgrade scenarios in navigation spans |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Defaults to 3× the configured `idleTimeout`. Set to `0` to not wait at all. | ||
| * | ||
| * Default: idleTimeout * 3 |
There was a problem hiding this comment.
The documentation states "Defaults to 3× the configured idleTimeout. Set to 0 to not wait at all." However, setting it to 0 has special behavior (line 931), but the validation logic (line 524) would allow negative values like -1 through to be set if they're not exactly Infinity.
The documentation should clarify:
- What happens with negative values (they fall back to default)
- That
Infinitymeans "wait forever" - That
0means "don't wait"
Example:
/**
* Maximum time (in milliseconds) to wait for lazy routes to load before finalizing span names.
*
* - Set to `0` to not wait at all (immediate finalization)
* - Set to `Infinity` to wait indefinitely
* - Negative values will fall back to the default
*
* Defaults to 3× the configured `idleTimeout` (default: 3000ms).
*/| * Defaults to 3× the configured `idleTimeout`. Set to `0` to not wait at all. | |
| * | |
| * Default: idleTimeout * 3 | |
| * - Set to `0` to not wait at all (immediate finalization) | |
| * - Set to `Infinity` to wait indefinitely | |
| * - Negative values will fall back to the default | |
| * | |
| * Defaults to 3× the configured `idleTimeout` (default: 3000ms). |
| tryUpdateSpanNameBeforeEnd( | ||
| span, | ||
| spanToJSON(span), | ||
| spanToJSON(span).description, | ||
| location, | ||
| routes, | ||
| basename, | ||
| ) as unknown as RouteMatch[]; | ||
| spanType, | ||
| allRoutes, | ||
| ); |
There was a problem hiding this comment.
There's a potential issue where spanToJSON(span) is called three times in quick succession (lines 935-936, and then again at 964). This is inefficient and could lead to inconsistent data if the span changes between calls.
Consider capturing the result once and reusing it:
const currentSpanJson = spanToJSON(span);
tryUpdateSpanNameBeforeEnd(
span,
currentSpanJson,
currentSpanJson.description,
location,
routes,
basename,
spanType,
allRoutes,
);| // 2. The current name has wildcards (incomplete parameterization) | ||
| const hasBeenNamed = (activeRootSpan as { __sentry_navigation_name_set__?: boolean })?.__sentry_navigation_name_set__; | ||
| const currentNameHasWildcard = currentName && transactionNameHasWildcard(currentName); | ||
| const shouldUpdate = forceUpdate || !hasBeenNamed || currentNameHasWildcard; |
There was a problem hiding this comment.
[nitpick] The condition for determining if an update should occur is complex and might allow unintended updates. Specifically, on line 211, shouldUpdate will be true if currentNameHasWildcard is true, regardless of whether hasBeenNamed is true or forceUpdate is false.
This could cause issues because even if a span has been marked as finalized (hasBeenNamed = true) and forceUpdate = false, the function will still attempt updates if the name has wildcards. While this may be intentional for wildcard resolution, the logic on line 213 checks !spanJson.timestamp to prevent updates on ended spans, but doesn't check hasBeenNamed again in the context where currentNameHasWildcard triggered the update.
Consider refactoring for clarity:
const shouldUpdate = !hasBeenNamed || forceUpdate || currentNameHasWildcard;This makes it clearer that wildcards allow re-updating even after being named.
| const shouldUpdate = forceUpdate || !hasBeenNamed || currentNameHasWildcard; | |
| const shouldUpdate = !hasBeenNamed || forceUpdate || currentNameHasWildcard; |
| (!hasBeenNamed || // Span not finalized - accept any name | ||
| !currentName || // No current name - always set | ||
| (currentNameHasWildcard && source === 'route') || // Wildcard route → better route (MUST stay in route source) | ||
| (currentSource !== 'route' && source === 'route')); // URL → route upgrade |
There was a problem hiding this comment.
The isImprovement logic has a subtle issue on line 229. The condition !hasBeenNamed means "span not finalized", but this doesn't necessarily mean we should accept any name.
The comment says "accept any name" if not finalized, but consider this scenario:
- Span starts with name
/users/:id(route source, not finalized yet) - New resolution returns
/users/123(url source) - Since
!hasBeenNamedis true, it would accept this URL, downgrading from a parameterized route to a URL
This contradicts the comment on line 225 that says "Never downgrade from route source to url source". The condition should verify that we're not downgrading even when the span isn't finalized:
const isImprovement =
name &&
(!currentName || // No current name - always set
(currentNameHasWildcard && source === 'route') || // Wildcard route → better route
(currentSource !== 'route' && source === 'route') || // URL → route upgrade
(!hasBeenNamed && currentSource !== 'route')); // Not finalized, but only if not already a route| (!hasBeenNamed || // Span not finalized - accept any name | |
| !currentName || // No current name - always set | |
| (currentNameHasWildcard && source === 'route') || // Wildcard route → better route (MUST stay in route source) | |
| (currentSource !== 'route' && source === 'route')); // URL → route upgrade | |
| ( | |
| !currentName || // No current name - always set | |
| (currentNameHasWildcard && source === 'route') || // Wildcard route → better route (MUST stay in route source) | |
| (currentSource !== 'route' && source === 'route') || // URL → route upgrade | |
| (!hasBeenNamed && currentSource !== 'route') // Not finalized, but only if not already a route | |
| ); |
| // If we're already in a navigation span, check if we should update its name | ||
| if (isAlreadyInNavigationSpan && activeSpan) { | ||
| // Only update if the new name is better (doesn't have wildcards or is more complete) | ||
| const shouldUpdate = currentName && transactionNameHasWildcard(currentName) && !transactionNameHasWildcard(name); |
There was a problem hiding this comment.
[nitpick] The logic on line 752 only updates the span if the current name has wildcards AND the new name doesn't. However, this misses a valid upgrade scenario:
If currentName = '/users/*' (wildcard) and name = '/users/*/profile/*' (still has wildcards but more specific), this condition would be false and no update occurs, even though the new name provides more specificity.
Consider updating the condition to allow upgrades between wildcard routes if the new route is more specific, or document why such updates are intentionally skipped.
| const shouldUpdate = currentName && transactionNameHasWildcard(currentName) && !transactionNameHasWildcard(name); | |
| const shouldUpdate = | |
| currentName && | |
| ( | |
| // Upgrade from wildcard to non-wildcard | |
| (transactionNameHasWildcard(currentName) && !transactionNameHasWildcard(name)) | |
| // Or upgrade to a more specific route, even if both have wildcards | |
| || isMoreSpecificRouteName(currentName, name) | |
| ); |
| const isImprovement = | ||
| name && | ||
| (!currentName || // No current name - always set | ||
| (hasWildcard && source === 'route') || // Wildcard route → better route (MUST stay in route source) |
There was a problem hiding this comment.
[nitpick] Similar to the issue in updateNavigationSpan (lines 227-232), the isImprovement logic here has the same problem. Line 878 with condition !currentName means "always set if no current name", but this could allow a downgrade scenario:
- Current span has no description yet
- Initial resolution returns URL source
/users/123 - Since
!currentNameis true, it accepts the URL - Later, a better route source
/users/:idbecomes available but might not update
While this specific scenario might be less common, the logic should be consistent and prevent downgrades. Consider:
const isImprovement =
name &&
(!currentName || // No current name - always set
(hasWildcard && source === 'route' && !transactionNameHasWildcard(name)) || // Wildcard → non-wildcard route
(currentSource !== 'route' && source === 'route')); // URL → route upgradeThis ensures wildcard routes only upgrade to non-wildcard routes, making the improvement criteria more strict and predictable.
| (hasWildcard && source === 'route') || // Wildcard route → better route (MUST stay in route source) | |
| (hasWildcard && source === 'route' && !transactionNameHasWildcard(name)) || // Wildcard → non-wildcard route |
|
|
||
| /** Checks if transaction name has wildcard (/* or * or ends with *). */ | ||
| export function transactionNameHasWildcard(name: string): boolean { | ||
| return name.includes('/*') || name === '*' || name.endsWith('*'); |
There was a problem hiding this comment.
The condition name.endsWith('*') is redundant because any string ending with * is already covered by name.includes('/*') (if it's /something*) or name === '*' (if it's just *). However, this logic fails to catch edge cases like /path*something or paths ending with * but not preceded by /.
Consider either:
- Removing the redundant
name.endsWith('*')check since it's already covered - Or if you want to catch all wildcards more broadly, use:
name.includes('*')which will catch any wildcard character anywhere in the path
Based on the test cases, it seems the intent is to detect any wildcard (*) character in paths, so name.includes('*') would be the most accurate implementation.
| return name.includes('/*') || name === '*' || name.endsWith('*'); | |
| return name.includes('*'); |
| getGlobalPathname: vi.fn(() => '/test'), | ||
| routeIsDescendant: vi.fn(() => false), | ||
| transactionNameHasWildcard: vi.fn((name: string) => { | ||
| return name.includes('/*') || name === '*' || name.endsWith('*'); |
There was a problem hiding this comment.
[nitpick] The mock implementation duplicates the same potentially problematic logic from the actual implementation. The condition name.includes('/*') || name === '*' || name.endsWith('*') doesn't comprehensively catch all wildcards.
For consistency and maintainability, consider using the actual implementation instead of mocking it, or if mocking is necessary, simplify to: name.includes('*') to match any wildcard character in the path.
| return name.includes('/*') || name === '*' || name.endsWith('*'); | |
| return name.includes('*'); |
| DEBUG_BUILD && | ||
| debug.warn('[React Router] maxLazyRouteWaitMs must be a number, falling back to default:', defaultMaxWait); | ||
| _maxLazyRouteWaitMs = defaultMaxWait; | ||
| } else if (configuredMaxWait < 0 && configuredMaxWait !== Infinity) { |
There was a problem hiding this comment.
The validation condition configuredMaxWait < 0 && configuredMaxWait !== Infinity is problematic. This condition will never be true because:
Infinityis positive (not less than 0)-Infinitywould pass this check but should probably be rejected
The correct condition should be: configuredMaxWait < 0 (without the && configuredMaxWait !== Infinity part), since Infinity is never less than 0 anyway.
| } else if (configuredMaxWait < 0 && configuredMaxWait !== Infinity) { | |
| } else if (configuredMaxWait < 0) { |
| tryUpdateSpanNameBeforeEnd( | ||
| span, | ||
| spanToJSON(span), | ||
| spanToJSON(span).description, | ||
| location, | ||
| routes, | ||
| currentAllRoutes.length > 0 ? currentAllRoutes : routes, | ||
| branches, | ||
| basename, | ||
| spanType, | ||
| allRoutes, | ||
| ); |
There was a problem hiding this comment.
Similar to the issue at lines 933-942, spanToJSON(span) is called twice here (lines 963 and 964). This is inefficient and could lead to inconsistent data.
Capture the result once and reuse it:
const currentSpanJson = spanToJSON(span);
tryUpdateSpanNameBeforeEnd(
span,
currentSpanJson,
currentSpanJson.description,
location,
routes,
basename,
spanType,
allRoutes,
);
| * | ||
| * Default: idleTimeout * 3 | ||
| */ | ||
| maxLazyRouteWaitMs?: number; |
There was a problem hiding this comment.
Naming suggestions here: lazyRouteTimeoutMs or even just lazyRouteTimeout as the JSDoc says it's milliseconds. That would align with the naming of idleTimeout
packages/react/src/reactrouter-compat-utils/instrumentation.tsx
Outdated
Show resolved
Hide resolved
| if (endCalled) { | ||
| return; | ||
| } | ||
| endCalled = true; |
There was a problem hiding this comment.
I am just wondering why this might be called multiple times 🤔
There was a problem hiding this comment.
As we're deferring a manually-triggered span.end when the lazy route resolution promise is resolved (or this new lazy-timeout is fired), if an idle timeout or a trigger like document.hidden triggers this beforehand, this saves us from running matchRoutes etc. again, before trying to run originalEnd which won't work anyway. I updated the comments for it.
|
|
||
| if (branches) { | ||
| const [name, source] = resolveRouteNameAndSource( | ||
| // Take snapshot of current promises to wait for (prevents race conditions with new navigations) |
There was a problem hiding this comment.
This is an interesting approach 😅 I guess if it works with existing setups, it's fine but it's a bit hard to grasp
There was a problem hiding this comment.
This is not that required actually, we're mapping the pending promises by their respective transaction anyway. Removed it 👍
| activeSpan.updateName(name); | ||
| activeSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source as 'route' | 'url' | 'custom'); | ||
| DEBUG_BUILD && debug.log(`[Tracing] Updated navigation span name from "${currentName}" to "${name}"`); | ||
| } |
There was a problem hiding this comment.
Bug: Navigation Source Downgrade Violates Principle
In handleNavigation, the wildcard update logic allows downgrading from route source to URL source. When an existing navigation span has a wildcard route name like /users/*, it can be updated to a URL source name like /users/123. This violates the "never downgrade from route source to URL source" principle mentioned in updateNavigationSpan. The condition should check that source === 'route' before allowing the update, similar to the logic in updateNavigationSpan.
Building on top of #17962
Added a few more checks to make sure non-resolved (wildcard) routes are not reported in lazy route pageloads / navigations.
patchSpanEndwith a user-configurable wait timeout for potentially slow route resolution. Named this option asmaxLazyRouteWaitMsand it's defaulted asidleTimeout* 3. It may conditionally delay reporting (if the route resolution is still not done by the end of the timeout), but will prevent prematurely sent lazy-route transactions inside that window.updateNavigationSpanandhandleNavigationfor whether any wildcard still exists in a lazy-route, so they are still marked as open to full resolution. We keep track of pending lazy-route resolutions insidependingLazyRouteLoadsAny of these should not affect the behaviour of non-lazy route usage