8000 fix(sveltekit): Handle same origin and destination navigations correc… · lapa182/sentry-javascript@1121507 · GitHub
[go: up one dir, main page]
Skip to content < 8000 div data-target="react-partial.reactRoot">

Commit 1121507

Browse files
Lms24Lms24
authored
fix(sveltekit): Handle same origin and destination navigations correctly (getsentry#7584)
Previously our Kit routing instrumentation used the parameterized route id to determine if the origin and destination of the navigation were identical (in which case we don't want to create a navigation transaction). This caused navigations from e.g. `/users/123` to `users/456` to not create a transaction because their route id was identical. This PR fixes this bug by using the raw URL which we also get from the `navigating` store emissions. Furthermore, this PR fixes the transaction source which previously was always set to `route`, even if no route id was available.
1 parent f1128bd commit 1121507

File tree

2 files changed

+83
-23
lines changed

2 files changed

+83
-23
lines changed

‎packages/sveltekit/src/client/router.ts

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,9 @@ function instrumentPageload(startTransactionFn: (context: TransactionContext) =>
4242
tags: {
4343
...DEFAULT_TAGS,
4444
},
45+
metadata: {
46+
source: 'url',
47+
},
4548
});
4649

4750
page.subscribe(page => {
@@ -76,20 +79,30 @@ function instrumentNavigations(startTransactionFn: (context: TransactionContext)
7679
return;
7780
}
7881

79-
const routeDestination = navigation.to && navigation.to.route.id;
80-
const routeOrigin = navigation.from && navigation.from.route.id;
82+
const from = navigation.from;
83+
const to = navigation.to;
84+
85+
// for the origin we can fall back to window.location.pathname because in this emission, it still is set to the origin path
86+
const rawRouteOrigin = (from && from.url.pathname) || (WINDOW && WINDOW.location && WINDOW.location.pathname);
8187

82-
if (routeOrigin === routeDestination) {
88+
const rawRouteDestination = to && to.url.pathname;
89+
90+
// We don't want to create transactions for navigations of same origin and destination.
91+
// We need to look at the raw URL here because parameterized routes can still differ in their raw parameters.
92+
if (rawRouteOrigin === rawRouteDestination) {
8393
return;
8494
}
8595

96+
const parameterizedRouteOrigin = from && from.route.id;
97+
const parameterizedRouteDestination = to && to.route.id;
98+
8699
activeTransaction = getActiveTransaction();
87100

88101
if (!activeTransaction) {
89102
activeTransaction = startTransactionFn({
90-
name: routeDestination || (WINDOW && WINDOW.location && WINDOW.location.pathname),
103+
name: parameterizedRouteDestination || rawRouteDestination || 'unknown',
91104
op: 'navigation',
92-
metadata: { source: 'route' },
105+
metadata: { source: parameterizedRouteDestination ? 'route' : 'url' },
93106
tags: {
94107
...DEFAULT_TAGS,
95108
},
@@ -105,7 +118,7 @@ function instrumentNavigations(startTransactionFn: (context: TransactionContext)
105118
op: 'ui.sveltekit.routing',
106119
description: 'SvelteKit Route Change',
107120
});
108-
activeTransaction.setTag('from', routeOrigin);
121+
activeTransaction.setTag('from', parameterizedRouteOrigin);
109122
}
110123
});
111124
}

‎packages/sveltekit/test/client/router.test.ts

Lines changed: 64 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,9 @@ describe('sveltekitRoutingInstrumentation', () => {
5757
tags: {
5858
'routing.instrumentation': '@sentry/sveltekit',
5959
},
60+
metadata: {
61+
source: 'url',
62+
},
6063
});
6164

6265
// We emit an update to the `page` store to simulate the SvelteKit router lifecycle
@@ -73,15 +76,15 @@ describe('sveltekitRoutingInstrumentation', () => {
7376
expect(mockedStartTransaction).toHaveBeenCalledTimes(0);
7477
});
7578

76-
it("doesn't starts a navigation transaction when `startTransactionOnLocationChange` is false", () => {
79+
it("doesn't start a navigation transaction when `startTransactionOnLocationChange` is false", () => {
7780
svelteKitRoutingInstrumentation(mockedStartTransaction, false, false);
7881

7982
// We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle
8083
// @ts-ignore This is fine because we testUtils/stores.ts defines `navigating` as a writable store
81-
navigating.set(
82-
{ from: { route: { id: 'testNavigationOrigin' } } },
83-
{ to: { route: { id: 'testNavigationDestination' } } },
84-
);
84+
navigating.set({
85+
from: { route: { id: '/users' }, url: { pathname: '/users' } },
86+
to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } },
87+
});
8588

8689
// This should update the transaction name with the parameterized route:
8790
expect(mockedStartTransaction).toHaveBeenCalledTimes(0);
@@ -93,14 +96,14 @@ describe('sveltekitRoutingInstrumentation', () => {
9396
// We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle
9497
// @ts-ignore This is fine because we testUtils/stores.ts defines `navigating` as a writable store
9598
navigating.set({
96-
from: { route: { id: 'testNavigationOrigin' } },
97-
to: { route: { id: 'testNavigationDestination' } },
99+
from: { route: { id: '/users' }, url: { pathname: '/users' } },
100+
to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } },
98101
});
99102

100103
// This should update the transaction name with the parameterized route:
101104
expect(mockedStartTransaction).toHaveBeenCalledTimes(1);
102105
expect(mockedStartTransaction).toHaveBeenCalledWith({
103-
name: 'testNavigationDestination',
106+
name: '/users/[id]',
104107
op: 'navigation',
105108
metadata: {
106109
source: 'route',
@@ -115,7 +118,7 @@ describe('sveltekitRoutingInstrumentation', () => {
115118
description: 'SvelteKit Route Change',
116119
});
117120

118-
expect(returnedTransaction?.setTag).toHaveBeenCalledWith('from', 'testNavigationOrigin');
121+
expect(returnedTransaction?.setTag).toHaveBeenCalledWith('from', '/users');
119122

120123
// We emit `null` here to simulate the end of the navigation lifecycle
121124
// @ts-ignore this is fine
@@ -124,16 +127,60 @@ describe('sveltekitRoutingInstrumentation', () => {
124127
expect(routingSpanFinishSpy).toHaveBeenCalledTimes(1);
125128
});
126129

127-
it("doesn't start a navigation transaction if navigation origin and destination are equal", () => {
128-
svelteKitRoutingInstrumentation(mockedStartTransaction, false, true);
130+
describe('handling same origin and destination navigations', () => {
131+
it("doesn't start a navigation transaction if the raw navigation origin and destination are equal", () => {
132+
svelteKitRoutingInstrumentation(mockedStartTransaction, false, true);
129133

130-
// We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle
131-
// @ts-ignore This is fine because we testUtils/stores.ts defines `navigating` as a writable store
132-
navigating.set({
133-
from: { route: { id: 'testRoute' } },
134-
to: { route: { id: 'testRoute' } },
134+
// We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle
135+
// @ts-ignore This is fine because we testUtils/stores.ts defines `navigating` as a writable store
136+
navigating.set({
137+
from: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } },
138+
to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } },
139+
});
140+
141+
expect(mockedStartTransaction).toHaveBeenCalledTimes(0);
135142
});
136143

137-
expect(mockedStartTransaction).toHaveBeenCalledTimes(0);
144+
it('starts a navigation transaction if the raw navigation origin and destination are not equal', () => {
145+
svelteKitRoutingInstrumentation(mockedStartTransaction, false, true);
146+
147+
// @ts-ignore This is fine
148+
navigating.set({
149+
from: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } },
150+
to: { route: { id: '/users/[id]' }, url: { pathname: '/users/223412' } },
151+
});
152+
153+
expect(mockedStartTransaction).toHaveBeenCalledTimes(1);
154+
expect(mockedStartTransaction).toHaveBeenCalledWith({
155+
name: '/users/[id]',
156+
op: 'navigation',
157+
metadata: {
158+
source: 'route',
159+
},
160+
tags: {
161+
'routing.instrumentation': '@sentry/sveltekit',
162+
},
163+
});
164+
165+
expect(returnedTransaction?.startChild).toHaveBeenCalledWith({
166+
op: 'ui.sveltekit.routing',
167+
description: 'SvelteKit Route Change',
168+
});
169+
170+
expect(returnedTransaction?.setTag).toHaveBeenCalledWith('from', '/users/[id]');
171+
});
172+
173+
it('falls back to `window.location.pathname` to determine the raw origin', () => {
174+
svelteKitRoutingInstrumentation(mockedStartTransaction, false, true);
175+
176+
// window.location.pathame is "/" in tests
177+
178+
// @ts-ignore This is fine
179+
navigating.set({
180+
to: { route: {}, url: { pathname: '/' } },
181+
});
182+
183+
expect(mockedStartTransaction).toHaveBeenCalledTimes(0);
184+
});
138185
});
139186
});

0 commit comments

Comments
 (0)
0