fix(sveltekit): Handle same origin and destination navigations correctly (#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.

Author: Lms24

packages/sveltekit/src/client/router.ts

@@ -42,6 +42,9 @@ function instrumentPageload(startTransactionFn: (context: TransactionContext) =>
     tags: {
       ...DEFAULT_TAGS,
     },
+    metadata: {
+      source: 'url',
+    },
   });
 
   page.subscribe(page => {
@@ -76,20 +79,30 @@ function instrumentNavigations(startTransactionFn: (context: TransactionContext)
       return;
     }
 
-    const routeDestination = navigation.to && navigation.to.route.id;
-    const routeOrigin = navigation.from && navigation.from.route.id;
+    const from = navigation.from;
+    const to = navigation.to;
+
+    // for the origin we can fall back to window.location.pathname because in this emission, it still is set to the origin path
+    const rawRouteOrigin = (from && from.url.pathname) || (WINDOW && WINDOW.location && WINDOW.location.pathname);
 
-    if (routeOrigin === routeDestination) {
+    const rawRouteDestination = to && to.url.pathname;
+
+    // We don't want to create transactions for navigations of same origin and destination.
+    // We need to look at the raw URL here because parameterized routes can still differ in their raw parameters.
+    if (rawRouteOrigin === rawRouteDestination) {
       return;
     }
 
+    const parameterizedRouteOrigin = from && from.route.id;
+    const parameterizedRouteDestination = to && to.route.id;
+
     activeTransaction = getActiveTransaction();
 
     if (!activeTransaction) {
       activeTransaction = startTransactionFn({
-        name: routeDestination || (WINDOW && WINDOW.location && WINDOW.location.pathname),
+        name: parameterizedRouteDestination || rawRouteDestination || 'unknown',
         op: 'navigation',
-        metadata: { source: 'route' },
+        metadata: { source: parameterizedRouteDestination ? 'route' : 'url' },
         tags: {
           ...DEFAULT_TAGS,
         },
@@ -105,7 +118,7 @@ function instrumentNavigations(startTransactionFn: (context: TransactionContext)
         op: 'ui.sveltekit.routing',
         description: 'SvelteKit Route Change',
       });
-      activeTransaction.setTag('from', routeOrigin);
+      activeTransaction.setTag('from', parameterizedRouteOrigin);
     }
   });
 }

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

@@ -57,6 +57,9 @@ describe('sveltekitRoutingInstrumentation', () => {
       tags: {
         'routing.instrumentation': '@sentry/sveltekit',
       },
+      metadata: {
+        source: 'url',
+      },
     });
 
     // We emit an update to the `page` store to simulate the SvelteKit router lifecycle
@@ -73,15 +76,15 @@ describe('sveltekitRoutingInstrumentation', () => {
     expect(mockedStartTransaction).toHaveBeenCalledTimes(0);
   });
 
-  it("doesn't starts a navigation transaction when `startTransactionOnLocationChange` is false", () => {
+  it("doesn't start a navigation transaction when `startTransactionOnLocationChange` is false", () => {
     svelteKitRoutingInstrumentation(mockedStartTransaction, false, false);
 
     // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle
     // @ts-ignore This is fine because we testUtils/stores.ts defines `navigating` as a writable store
-    navigating.set(
-      { from: { route: { id: 'testNavigationOrigin' } } },
-      { to: { route: { id: 'testNavigationDestination' } } },
-    );
+    navigating.set({
+      from: { route: { id: '/users' }, url: { pathname: '/users' } },
+      to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } },
+    });
 
     // This should update the transaction name with the parameterized route:
     expect(mockedStartTransaction).toHaveBeenCalledTimes(0);
@@ -93,14 +96,14 @@ describe('sveltekitRoutingInstrumentation', () => {
     // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle
     // @ts-ignore This is fine because we testUtils/stores.ts defines `navigating` as a writable store
     navigating.set({
-      from: { route: { id: 'testNavigationOrigin' } },
-      to: { route: { id: 'testNavigationDestination' } },
+      from: { route: { id: '/users' }, url: { pathname: '/users' } },
+      to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } },
     });
 
     // This should update the transaction name with the parameterized route:
     expect(mockedStartTransaction).toHaveBeenCalledTimes(1);
     expect(mockedStartTransaction).toHaveBeenCalledWith({
-      name: 'testNavigationDestination',
+      name: '/users/[id]',
       op: 'navigation',
       metadata: {
         source: 'route',
@@ -115,7 +118,7 @@ describe('sveltekitRoutingInstrumentation', () => {
       description: 'SvelteKit Route Change',
     });
 
-    expect(returnedTransaction?.setTag).toHaveBeenCalledWith('from', 'testNavigationOrigin');
+    expect(returnedTransaction?.setTag).toHaveBeenCalledWith('from', '/users');
 
     // We emit `null` here to simulate the end of the navigation lifecycle
     // @ts-ignore this is fine
@@ -124,16 +127,60 @@ describe('sveltekitRoutingInstrumentation', () => {
     expect(routingSpanFinishSpy).toHaveBeenCalledTimes(1);
   });
 
-  it("doesn't start a navigation transaction if navigation origin and destination are equal", () => {
-    svelteKitRoutingInstrumentation(mockedStartTransaction, false, true);
+  describe('handling same origin and destination navigations', () => {
+    it("doesn't start a navigation transaction if the raw navigation origin and destination are equal", () => {
+      svelteKitRoutingInstrumentation(mockedStartTransaction, false, true);
 
-    // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle
-    // @ts-ignore This is fine because we testUtils/stores.ts defines `navigating` as a writable store
-    navigating.set({
-      from: { route: { id: 'testRoute' } },
-      to: { route: { id: 'testRoute' } },
+      // We emit an update to the `navigating` store to simulate the SvelteKit navigation lifecycle
+      // @ts-ignore This is fine because we testUtils/stores.ts defines `navigating` as a writable store
+      navigating.set({
+        from: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } },
+        to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } },
+      });
+
+      expect(mockedStartTransaction).toHaveBeenCalledTimes(0);
     });
 
-    expect(mockedStartTransaction).toHaveBeenCalledTimes(0);
+    it('starts a navigation transaction if the raw navigation origin and destination are not equal', () => {
+      svelteKitRoutingInstrumentation(mockedStartTransaction, false, true);
+
+      // @ts-ignore This is fine
+      navigating.set({
+        from: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762' } },
+        to: { route: { id: '/users/[id]' }, url: { pathname: '/users/223412' } },
+      });
+
+      expect(mockedStartTransaction).toHaveBeenCalledTimes(1);
+      expect(mockedStartTransaction).toHaveBeenCalledWith({
+        name: '/users/[id]',
+        op: 'navigation',
+        metadata: {
+          source: 'route',
+        },
+        tags: {
+          'routing.instrumentation': '@sentry/sveltekit',
+        },
+      });
+
+      expect(returnedTransaction?.startChild).toHaveBeenCalledWith({
+        op: 'ui.sveltekit.routing',
+        description: 'SvelteKit Route Change',
+      });
+
+      expect(returnedTransaction?.setTag).toHaveBeenCalledWith('from', '/users/[id]');
+    });
+
+    it('falls back to `window.location.pathname` to determine the raw origin', () => {
+      svelteKitRoutingInstrumentation(mockedStartTransaction, false, true);
+
+      // window.location.pathame is "/" in tests
+
+      // @ts-ignore This is fine
+      navigating.set({
+        to: { route: {}, url: { pathname: '/' } },
+      });
+
+      expect(mockedStartTransaction).toHaveBeenCalledTimes(0);
+    });
   });
 });