Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit 38cc574

Browse files
onurtemizkans1gr1d
andauthored
fix(react): Remove handleExistingNavigation (#17534)
Removes recently introduced special-casing lazy-route -> lazy-route transaction name updates. This completes the fix in #17438. E2E tests are updated to replicate a similar case in the reproduction here: #17417 Also manually tested on the reproduction. --------- Co-authored-by: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com>
1 parent f3f0ba3 commit 38cc574

File tree

7 files changed

+195
-433
lines changed

7 files changed

+195
-433
lines changed

‎dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/src/pages/InnerLazyRoutes.tsx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@ export const someMoreNestedRoutes = [
3232
<Link to="/another-lazy/sub/888/999" id="navigate-to-another-from-inner">
3333
Navigate to Another Lazy Route
3434
</Link>
35+
<Link to="/lazy/inner/1/2/" id="navigate-to-upper">
36+
Navigate to Upper Lazy Route
37+
</Link>
3538
</div>
3639
),
3740
},

‎dev-packages/e2e-tests/test-applications/react-router-7-lazy-routes/tests/transactions.test.ts‎

Lines changed: 112 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,15 @@ test('Creates navigation transactions between two different lazy routes', async
107107
expect(secondEvent.contexts?.trace?.op).toBe('navigation');
108108
});
109109

110-
test('Creates navigation transactions from inner lazy route to another lazy route', async ({ page }) => {
110+
test('Creates navigation transactions from inner lazy route to another lazy route with history navigation', async ({
111+
page,
112+
}) => {
113+
await page.goto('/');
114+
115+
// Navigate to inner lazy route first
116+
const navigationToInner = page.locator('id=navigation');
117+
await expect(navigationToInner).toBeVisible();
118+
111119
// First, navigate to the inner lazy route
112120
const firstTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
113121
return (
@@ -117,11 +125,6 @@ test('Creates navigation transactions from inner lazy route to another lazy rout
117125
);
118126
});
119127

120-
await page.goto('/');
121-
122-
// Navigate to inner lazy route first
123-
const navigationToInner = page.locator('id=navigation');
124-
await expect(navigationToInner).toBeVisible();
125128
await navigationToInner.click();
126129

127130
const firstEvent = await firstTransactionPromise;
@@ -135,6 +138,10 @@ test('Creates navigation transactions from inner lazy route to another lazy rout
135138
expect(firstEvent.type).toBe('transaction');
136139
expect(firstEvent.contexts?.trace?.op).toBe('navigation');
137140

141+
// Click the navigation link from within the inner lazy route to another lazy route
142+
const navigationToAnotherFromInner = page.locator('id=navigate-to-another-from-inner');
143+
await expect(navigationToAnotherFromInner).toBeVisible();
144+
138145
// Now navigate from the inner lazy route to another lazy route
139146
const secondTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
140147
return (
@@ -144,9 +151,6 @@ test('Creates navigation transactions from inner lazy route to another lazy rout
144151
);
145152
});
146153

147-
// Click the navigation link from within the inner lazy route to another lazy route
148-
const navigationToAnotherFromInner = page.locator('id=navigate-to-another-from-inner');
149-
await expect(navigationToAnotherFromInner).toBeVisible();
150154
await navigationToAnotherFromInner.click();
151155

152156
const secondEvent = await secondTransactionPromise;
@@ -159,4 +163,103 @@ test('Creates navigation transactions from inner lazy route to another lazy rout
159163
expect(secondEvent.transaction).toBe('/another-lazy/sub/:id/:subId');
160164
expect(secondEvent.type).toBe('transaction');
161165
expect(secondEvent.contexts?.trace?.op).toBe('navigation');
166+
167+
// Go back to the previous page to ensure history navigation works as expected
168+
const goBackTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
169+
return (
170+
!!transactionEvent?.transaction &&
171+
transactionEvent.contexts?.trace?.op === 'navigation' &&
172+
transactionEvent.transaction === '/lazy/inner/:id/:anotherId/:someAnotherId'
173+
);
174+
});
175+
176+
await page.goBack();
177+
178+
const goBackEvent = await goBackTransactionPromise;
179+
180+
// Validate the second go back transaction event
181+
expect(goBackEvent.transaction).toBe('/lazy/inner/:id/:anotherId/:someAnotherId');
182+
expect(goBackEvent.type).toBe('transaction');
183+
expect(goBackEvent.contexts?.trace?.op).toBe('navigation');
184+
185+
// Navigate to the upper route
186+
const goUpperRouteTransactionPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
187+
return (
188+
!!transactionEvent?.transaction &&
189+
transactionEvent.contexts?.trace?.op === 'navigation' &&
190+
transactionEvent.transaction === '/lazy/inner/:id/:anotherId'
191+
);
192+
});
193+
194+
const navigationToUpper = page.locator('id=navigate-to-upper');
195+
196+
await navigationToUpper.click();
197+
198+
const goUpperRouteEvent = await goUpperRouteTransactionPromise;
199+
200+
// Validate the go upper route transaction event
201+
expect(goUpperRouteEvent.transaction).toBe('/lazy/inner/:id/:anotherId');
202+
expect(goUpperRouteEvent.type).toBe('transaction');
203+
expect(goUpperRouteEvent.contexts?.trace?.op).toBe('navigation');
204+
});
205+
206+
test('Does not send any duplicate navigation transaction names browsing between different routes', async ({ page }) => {
207+
const transactionNamesList: string[] = [];
208+
209+
// Monitor and add all transaction names sent to Sentry for the navigations
210+
const allTransactionsPromise = waitForTransaction('react-router-7-lazy-routes', async transactionEvent => {
211+
if (transactionEvent?.transaction) {
212+
transactionNamesList.push(transactionEvent.transaction);
213+
}
214+
215+
if (transactionNamesList.length >= 5) {
216+
// Stop monitoring once we have enough transaction names
217+
return true;
218+
}
219+
220+
return false;
221+
});
222+
223+
// Go to root page
224+
await page.goto('/');
225+
page.waitForTimeout(1000);
226+
227+
// Navigate to inner lazy route
228+
const navigationToInner = page.locator('id=navigation');
229+
await expect(navigationToInner).toBeVisible();
230+
await navigationToInner.click();
231+
232+
// Navigate to another lazy route
233+
const navigationToAnother = page.locator('id=navigate-to-another-from-inner');
234+
await expect(navigationToAnother).toBeVisible();
235+
await page.waitForTimeout(1000);
236+
237+
// Click to navigate to another lazy route
238+
await navigationToAnother.click();
239+
const anotherLazyRouteContent = page.locator('id=another-lazy-route-deep');
240+
await expect(anotherLazyRouteContent).toBeVisible();
241+
await page.waitForTimeout(1000);
242+
243+
// Navigate back to inner lazy route
244+
await page.goBack();
245+
await expect(page.locator('id=innermost-lazy-route')).toBeVisible();
246+
await page.waitForTimeout(1000);
247+
248+
// Navigate to upper inner lazy route
249+
const navigationToUpper = page.locator('id=navigate-to-upper');
250+
await expect(navigationToUpper).toBeVisible();
251+
await navigationToUpper.click();
252+
253+
await page.waitForTimeout(1000);
254+
255+
await allTransactionsPromise;
256+
257+
expect(transactionNamesList.length).toBe(5);
258+
expect(transactionNamesList).toEqual([
259+
'/',
260+
'/lazy/inner/:id/:anotherId/:someAnotherId',
261+
'/another-lazy/sub/:id/:subId',
262+
'/lazy/inner/:id/:anotherId/:someAnotherId',
263+
'/lazy/inner/:id/:anotherId',
264+
]);
162265
});

‎packages/react/src/reactrouter-compat-utils/index.ts‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,6 @@ export {
99
createV6CompatibleWrapCreateMemoryRouter,
1010
createV6CompatibleWrapUseRoutes,
1111
handleNavigation,
12-
handleExistingNavigationSpan,
13-
createNewNavigationSpan,
1412
addResolvedRoutesToParent,
1513
processResolvedRoutes,
1614
updateNavigationSpan,
@@ -21,7 +19,6 @@ export {
2119
resolveRouteNameAndSource,
2220
getNormalizedName,
2321
initializeRouterUtils,
24-
isLikelyLazyRouteContext,
2522
locationIsInsideDescendantRoute,
2623
prefixWithSlash,
2724
rebuildRoutePathFromAllRoutes,

‎packages/react/src/reactrouter-compat-utils/instrumentation.tsx‎

Lines changed: 15 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ import { checkRouteForAsyncHandler } from './lazy-routes';
4444
import {
4545
getNormalizedName,
4646
initializeRouterUtils,
47-
isLikelyLazyRouteContext,
4847
locationIsInsideDescendantRoute,
4948
prefixWithSlash,
5049
rebuildRoutePathFromAllRoutes,
@@ -176,12 +175,7 @@ export function updateNavigationSpan(
176175
// Check if this span has already been named to avoid multiple updates
177176
// But allow updates if this is a forced update (e.g., when lazy routes are loaded)
178177
const hasBeenNamed =
179-
!forceUpdate &&
180-
(
181-
activeRootSpan as {
182-
__sentry_navigation_name_set__?: boolean;
183-
}
184-
)?.__sentry_navigation_name_set__;
178+
!forceUpdate && (activeRootSpan as { __sentry_navigation_name_set__?: boolean })?.__sentry_navigation_name_set__;
185179

186180
if (!hasBeenNamed) {
187181
// Get fresh branches for the current location with all loaded routes
@@ -355,13 +349,7 @@ export function createV6CompatibleWrapCreateMemoryRouter<
355349
: router.state.location;
356350

357351
if (router.state.historyAction === 'POP' && activeRootSpan) {
358-
updatePageloadTransaction({
359-
activeRootSpan,
360-
location,
361-
routes,
362-
basename,
363-
allRoutes: Array.from(allRoutes),
364-
});
352+
updatePageloadTransaction({ activeRootSpan, location, routes, basename, allRoutes: Array.from(allRoutes) });
365353
}
366354

367355
router.subscribe((state: RouterState) => {
@@ -389,11 +377,7 @@ export function createReactRouterV6CompatibleTracingIntegration(
389377
options: Parameters<typeof browserTracingIntegration>[0] & ReactRouterOptions,
390378
version: V6CompatibleVersion,
391379
): Integration {
392-
const integration = browserTracingIntegration({
393-
...options,
394-
instrumentPageLoad: false,
395-
instrumentNavigation: false,
396-
});
380+
const integration = browserTracingIntegration({ ...options, instrumentPageLoad: false, instrumentNavigation: false });
397381

398382
const {
399383
useEffect,
@@ -532,13 +516,7 @@ function wrapPatchRoutesOnNavigation(
532516
if (activeRootSpan && (spanToJSON(activeRootSpan) as { op?: string }).op === 'navigation') {
533517
updateNavigationSpan(
534518
activeRootSpan,
535-
{
536-
pathname: targetPath,
537-
search: '',
538-
hash: '',
539-
state: null,
540-
key: 'default',
541-
},
519+
{ pathname: targetPath, search: '', hash: '', state: null, key: 'default' },
542520
Array.from(allRoutes),
543521
true, // forceUpdate = true since we're loading lazy routes
544522
_matchRoutes,
@@ -559,13 +537,7 @@ function wrapPatchRoutesOnNavigation(
559537
if (pathname) {
560538
updateNavigationSpan(
561539
activeRootSpan,
562-
{
563-
pathname,
564-
search: '',
565-
hash: '',
566-
state: null,
567-
key: 'default',
568-
},
540+
{ pathname, search: '', hash: '', state: null, key: 'default' },
569541
Array.from(allRoutes),
570542
false, // forceUpdate = false since this is after lazy routes are loaded
571543
_matchRoutes,
@@ -604,18 +576,20 @@ export function handleNavigation(opts: {
604576
basename,
605577
);
606578

607-
// Check if this might be a lazy route context
608-
const isLazyRouteContext = isLikelyLazyRouteContext(allRoutes || routes, location);
609-
610579
const activeSpan = getActiveSpan();
611580
const spanJson = activeSpan && spanToJSON(activeSpan);
612581
const isAlreadyInNavigationSpan = spanJson?.op === 'navigation';
613582

614583
// Cross usage can result in multiple navigation spans being created without this check
615-
if (isAlreadyInNavigationSpan && activeSpan && spanJson) {
616-
handleExistingNavigationSpan(activeSpan, spanJson, name, source, isLazyRouteContext);
617-
} else {
618-
createNewNavigationSpan(client, name, source, version, isLazyRouteContext);
584+
if (!isAlreadyInNavigationSpan) {
585+
startBrowserTracingNavigationSpan(client, {
586+
name,
587+
attributes: {
588+
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
589+
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
590+
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`,
591+
},
592+
});
619593
}
620594
}
621595
}
@@ -726,13 +700,7 @@ export function createV6CompatibleWithSentryReactRouterRouting<P extends Record<
726700
});
727701
isMountRenderPass.current = false;
728702
} else {
729-
handleNavigation({
730-
location,
731-
routes,
732-
navigationType,
733-
version,
734-
allRoutes: Array.from(allRoutes),
735-
});
703+
handleNavigation({ location, routes, navigationType, version, allRoutes: Array.from(allRoutes) });
736704
}
737705
},
738706
// `props.children` is purposely not included in the dependency array, because we do not want to re-run this effect
@@ -765,69 +733,3 @@ function getActiveRootSpan(): Span | undefined {
765733
// Only use this root span if it is a pageload or navigation span
766734
return op === 'navigation' || op === 'pageload' ? rootSpan : undefined;
767735
}
768-
769-
/**
770-
* Handles updating an existing navigation span
771-
*/
772-
export function handleExistingNavigationSpan(
773-
activeSpan: Span,
774-
spanJson: ReturnType<typeof spanToJSON>,
775-
name: string,
776-
source: TransactionSource,
777-
isLikelyLazyRoute: boolean,
778-
): void {
779-
// Check if we've already set the name for this span using a custom property
780-
const hasBeenNamed = (
781-
activeSpan as {
782-
__sentry_navigation_name_set__?: boolean;
783-
}
784-
)?.__sentry_navigation_name_set__;
785-
786-
if (!hasBeenNamed) {
787-
// This is the first time we're setting the name for this span
788-
if (!spanJson.timestamp) {
789-
activeSpan?.updateName(name);
790-
}
791-
792-
// For lazy routes, don't mark as named yet so it can be updated later
793-
if (!isLikelyLazyRoute) {
794-
addNonEnumerableProperty(
795-
activeSpan as { __sentry_navigation_name_set__?: boolean },
796-
'__sentry_navigation_name_set__',
797-
true,
798-
);
799-
}
800-
}
801-
802-
// Always set the source attribute to keep it consistent with the current route
803-
activeSpan?.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, source);
804-
}
805-
806-
/**
807-
* Creates a new navigation span
808-
*/
809-
export function createNewNavigationSpan(
810-
client: Client,
811-
name: string,
812-
source: TransactionSource,
813-
version: string,
814-
isLikelyLazyRoute: boolean,
815-
): void {
816-
const newSpan = startBrowserTracingNavigationSpan(client, {
817-
name,
818-
attributes: {
819-
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
820-
[SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation',
821-
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: `auto.navigation.react.reactrouter_v${version}`,
822-
},
823-
});
824-
825-
// For lazy routes, don't mark as named yet so it can be updated later when the route loads
826-
if (!isLikelyLazyRoute && newSpan) {
827-
addNonEnumerableProperty(
828-
newSpan as { __sentry_navigation_name_set__?: boolean },
829-
'__sentry_navigation_name_set__',
830-
true,
831-
);
832-
}
833-
}

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /