From bf682c8a96233f1e12b18e7d222665f12460fc92 Mon Sep 17 00:00:00 2001 From: Andrei Borza Date: Mon, 30 Mar 2026 15:50:25 +0900 Subject: [PATCH] feat(solid): Add route parametrization for Solid Router This PR adds route parametrization for the solidRouterBrowserTracingIntegration. It replaces raw URLs (e.g. /users/5) with parametrized routes (e.g. /users/:id) in transaction names. --- .../tests/performance.client.test.ts | 18 ++-- .../tests/performance.client.test.ts | 18 ++-- .../tests/performance.client.test.ts | 18 ++-- .../tests/performance.client.test.ts | 18 ++-- packages/solid/src/solidrouter.ts | 55 ++++++++++--- packages/solid/test/solidrouter.test.tsx | 82 +++++++++++-------- .../test/client/solidrouter.test.tsx | 82 +++++++++++-------- 7 files changed, 176 insertions(+), 115 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/solidstart-dynamic-import/tests/performance.client.test.ts b/dev-packages/e2e-tests/test-applications/solidstart-dynamic-import/tests/performance.client.test.ts index 63f97d519cf8..0cdc2465c065 100644 --- a/dev-packages/e2e-tests/test-applications/solidstart-dynamic-import/tests/performance.client.test.ts +++ b/dev-packages/e2e-tests/test-applications/solidstart-dynamic-import/tests/performance.client.test.ts @@ -18,14 +18,14 @@ test('sends a pageload transaction', async ({ page }) => { }, transaction: '/', transaction_info: { - source: 'url', + source: 'route', }, }); }); -test('sends a navigation transaction', async ({ page }) => { +test('sends a navigation transaction with parametrized route', async ({ page }) => { const transactionPromise = waitForTransaction('solidstart-dynamic-import', async transactionEvent => { - return transactionEvent?.transaction === '/users/5' && transactionEvent.contexts?.trace?.op === 'navigation'; + return transactionEvent?.transaction === '/users/:id' && transactionEvent.contexts?.trace?.op === 'navigation'; }); await page.goto(`/`); @@ -39,9 +39,9 @@ test('sends a navigation transaction', async ({ page }) => { origin: 'auto.navigation.solidstart.solidrouter', }, }, - transaction: '/users/5', + transaction: '/users/:id', transaction_info: { - source: 'url', + source: 'route', }, }); }); @@ -51,7 +51,7 @@ test('updates the transaction when using the back button', async ({ page }) => { // The sentry solidRouterBrowserTracingIntegration tries to update such // transactions with the proper name once the `useLocation` hook triggers. const navigationTxnPromise = waitForTransaction('solidstart-dynamic-import', async transactionEvent => { - return transactionEvent?.transaction === '/users/6' && transactionEvent.contexts?.trace?.op === 'navigation'; + return transactionEvent?.transaction === '/users/:id' && transactionEvent.contexts?.trace?.op === 'navigation'; }); await page.goto(`/back-navigation`); @@ -65,9 +65,9 @@ test('updates the transaction when using the back button', async ({ page }) => { origin: 'auto.navigation.solidstart.solidrouter', }, }, - transaction: '/users/6', + transaction: '/users/:id', transaction_info: { - source: 'url', + source: 'route', }, }); @@ -89,7 +89,7 @@ test('updates the transaction when using the back button', async ({ page }) => { }, transaction: '/back-navigation', transaction_info: { - source: 'url', + source: 'route', }, }); }); diff --git a/dev-packages/e2e-tests/test-applications/solidstart-spa/tests/performance.client.test.ts b/dev-packages/e2e-tests/test-applications/solidstart-spa/tests/performance.client.test.ts index c689bca22539..f54318bf171c 100644 --- a/dev-packages/e2e-tests/test-applications/solidstart-spa/tests/performance.client.test.ts +++ b/dev-packages/e2e-tests/test-applications/solidstart-spa/tests/performance.client.test.ts @@ -18,14 +18,14 @@ test('sends a pageload transaction', async ({ page }) => { }, transaction: '/', transaction_info: { - source: 'url', + source: 'route', }, }); }); -test('sends a navigation transaction', async ({ page }) => { +test('sends a navigation transaction with parametrized route', async ({ page }) => { const transactionPromise = waitForTransaction('solidstart-spa', async transactionEvent => { - return transactionEvent?.transaction === '/users/5' && transactionEvent.contexts?.trace?.op === 'navigation'; + return transactionEvent?.transaction === '/users/:id' && transactionEvent.contexts?.trace?.op === 'navigation'; }); await page.goto(`/`); @@ -39,9 +39,9 @@ test('sends a navigation transaction', async ({ page }) => { origin: 'auto.navigation.solidstart.solidrouter', }, }, - transaction: '/users/5', + transaction: '/users/:id', transaction_info: { - source: 'url', + source: 'route', }, }); }); @@ -51,7 +51,7 @@ test('updates the transaction when using the back button', async ({ page }) => { // The sentry solidRouterBrowserTracingIntegration tries to update such // transactions with the proper name once the `useLocation` hook triggers. const navigationTxnPromise = waitForTransaction('solidstart-spa', async transactionEvent => { - return transactionEvent?.transaction === '/users/6' && transactionEvent.contexts?.trace?.op === 'navigation'; + return transactionEvent?.transaction === '/users/:id' && transactionEvent.contexts?.trace?.op === 'navigation'; }); await page.goto(`/back-navigation`); @@ -65,9 +65,9 @@ test('updates the transaction when using the back button', async ({ page }) => { origin: 'auto.navigation.solidstart.solidrouter', }, }, - transaction: '/users/6', + transaction: '/users/:id', transaction_info: { - source: 'url', + source: 'route', }, }); @@ -89,7 +89,7 @@ test('updates the transaction when using the back button', async ({ page }) => { }, transaction: '/back-navigation', transaction_info: { - source: 'url', + source: 'route', }, }); }); diff --git a/dev-packages/e2e-tests/test-applications/solidstart-top-level-import/tests/performance.client.test.ts b/dev-packages/e2e-tests/test-applications/solidstart-top-level-import/tests/performance.client.test.ts index bd5dece39b33..55eeb5a5c757 100644 --- a/dev-packages/e2e-tests/test-applications/solidstart-top-level-import/tests/performance.client.test.ts +++ b/dev-packages/e2e-tests/test-applications/solidstart-top-level-import/tests/performance.client.test.ts @@ -18,14 +18,14 @@ test('sends a pageload transaction', async ({ page }) => { }, transaction: '/', transaction_info: { - source: 'url', + source: 'route', }, }); }); -test('sends a navigation transaction', async ({ page }) => { +test('sends a navigation transaction with parametrized route', async ({ page }) => { const transactionPromise = waitForTransaction('solidstart-top-level-import', async transactionEvent => { - return transactionEvent?.transaction === '/users/5' && transactionEvent.contexts?.trace?.op === 'navigation'; + return transactionEvent?.transaction === '/users/:id' && transactionEvent.contexts?.trace?.op === 'navigation'; }); await page.goto(`/`); @@ -39,9 +39,9 @@ test('sends a navigation transaction', async ({ page }) => { origin: 'auto.navigation.solidstart.solidrouter', }, }, - transaction: '/users/5', + transaction: '/users/:id', transaction_info: { - source: 'url', + source: 'route', }, }); }); @@ -51,7 +51,7 @@ test('updates the transaction when using the back button', async ({ page }) => { // The sentry solidRouterBrowserTracingIntegration tries to update such // transactions with the proper name once the `useLocation` hook triggers. const navigationTxnPromise = waitForTransaction('solidstart-top-level-import', async transactionEvent => { - return transactionEvent?.transaction === '/users/6' && transactionEvent.contexts?.trace?.op === 'navigation'; + return transactionEvent?.transaction === '/users/:id' && transactionEvent.contexts?.trace?.op === 'navigation'; }); await page.goto(`/back-navigation`); @@ -65,9 +65,9 @@ test('updates the transaction when using the back button', async ({ page }) => { origin: 'auto.navigation.solidstart.solidrouter', }, }, - transaction: '/users/6', + transaction: '/users/:id', transaction_info: { - source: 'url', + source: 'route', }, }); @@ -89,7 +89,7 @@ test('updates the transaction when using the back button', async ({ page }) => { }, transaction: '/back-navigation', transaction_info: { - source: 'url', + source: 'route', }, }); }); diff --git a/dev-packages/e2e-tests/test-applications/solidstart/tests/performance.client.test.ts b/dev-packages/e2e-tests/test-applications/solidstart/tests/performance.client.test.ts index 52d9cb219401..068fdc9b0cc2 100644 --- a/dev-packages/e2e-tests/test-applications/solidstart/tests/performance.client.test.ts +++ b/dev-packages/e2e-tests/test-applications/solidstart/tests/performance.client.test.ts @@ -18,14 +18,14 @@ test('sends a pageload transaction', async ({ page }) => { }, transaction: '/', transaction_info: { - source: 'url', + source: 'route', }, }); }); -test('sends a navigation transaction', async ({ page }) => { +test('sends a navigation transaction with parametrized route', async ({ page }) => { const transactionPromise = waitForTransaction('solidstart', async transactionEvent => { - return transactionEvent?.transaction === '/users/5' && transactionEvent.contexts?.trace?.op === 'navigation'; + return transactionEvent?.transaction === '/users/:id' && transactionEvent.contexts?.trace?.op === 'navigation'; }); await page.goto(`/`); @@ -39,9 +39,9 @@ test('sends a navigation transaction', async ({ page }) => { origin: 'auto.navigation.solidstart.solidrouter', }, }, - transaction: '/users/5', + transaction: '/users/:id', transaction_info: { - source: 'url', + source: 'route', }, }); }); @@ -51,7 +51,7 @@ test('updates the transaction when using the back button', async ({ page }) => { // The sentry solidRouterBrowserTracingIntegration tries to update such // transactions with the proper name once the `useLocation` hook triggers. const navigationTxnPromise = waitForTransaction('solidstart', async transactionEvent => { - return transactionEvent?.transaction === '/users/6' && transactionEvent.contexts?.trace?.op === 'navigation'; + return transactionEvent?.transaction === '/users/:id' && transactionEvent.contexts?.trace?.op === 'navigation'; }); await page.goto(`/back-navigation`); @@ -65,9 +65,9 @@ test('updates the transaction when using the back button', async ({ page }) => { origin: 'auto.navigation.solidstart.solidrouter', }, }, - transaction: '/users/6', + transaction: '/users/:id', transaction_info: { - source: 'url', + source: 'route', }, }); @@ -89,7 +89,7 @@ test('updates the transaction when using the back button', async ({ page }) => { }, transaction: '/back-navigation', transaction_info: { - source: 'url', + source: 'route', }, }); }); diff --git a/packages/solid/src/solidrouter.ts b/packages/solid/src/solidrouter.ts index 40af69caa2bc..0424183aea18 100644 --- a/packages/solid/src/solidrouter.ts +++ b/packages/solid/src/solidrouter.ts @@ -20,7 +20,7 @@ import type { RouteSectionProps, StaticRouter, } from '@solidjs/router'; -import { useBeforeLeave, useLocation } from '@solidjs/router'; +import { useBeforeLeave, useCurrentMatches, useLocation } from '@solidjs/router'; import type { Component, JSX, ParentProps } from 'solid-js'; import { createEffect, mergeProps, splitProps } from 'solid-js'; import { createComponent } from 'solid-js/web'; @@ -66,31 +66,60 @@ function SentryDefaultRoot(props: ParentProps): JSX.Element { */ function withSentryRouterRoot(Root: Component): Component { const SentryRouterRoot = (props: RouteSectionProps): JSX.Element => { - // TODO: This is a rudimentary first version of handling navigation spans - // It does not - // - use query params - // - parameterize the route + // Tracks the target of a pending navigation, so the effect can skip + // stale updates during redirects where the location signal + // hasn't caught up to the navigation span yet. + let pendingNavigationTarget: string | undefined; useBeforeLeave(({ to }: BeforeLeaveEventArgs) => { - // `to` could be `-1` if the browser back-button was used - handleNavigation(to.toString()); + const target = to.toString(); + pendingNavigationTarget = target; + handleNavigation(target); }); const location = useLocation(); + const matches = useCurrentMatches(); + createEffect(() => { const name = location.pathname; const rootSpan = getActiveRootSpan(); + if (!rootSpan) { + return; + } - if (rootSpan) { + // During redirects, the effect can fire before the router + // transition completes. In that case, location.pathname still points + // to the old route while the active span is already the navigation span. + // Skip the update to avoid overwriting the span with stale route data. + // `-1` is solid router's representation of a browser back-button + // navigation, where we don't know the target URL upfront. + if (pendingNavigationTarget && pendingNavigationTarget !== '-1' && name !== pendingNavigationTarget) { + return; + } + pendingNavigationTarget = undefined; + + const currentMatches = matches(); + const lastMatch = currentMatches[currentMatches.length - 1]; + + if (lastMatch) { + const parametrizedRoute = lastMatch.route.pattern || name; + rootSpan.updateName(parametrizedRoute); + rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); + + const params = lastMatch.params; + for (const [key, value] of Object.entries(params)) { + if (value !== undefined) { + rootSpan.setAttribute(`url.path.parameter.${key}`, value); + rootSpan.setAttribute(`params.${key}`, value); + } + } + } else { + // No matched route - update back-button navigations and set source to url const { op, description } = spanToJSON(rootSpan); - - // We only need to update navigation spans that have been created by - // a browser back-button navigation (stored as `-1` by solid router) - // everything else was already instrumented correctly in `useBeforeLeave` if (op === 'navigation' && description === '-1') { rootSpan.updateName(name); - rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url'); } + rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'url'); } }); diff --git a/packages/solid/test/solidrouter.test.tsx b/packages/solid/test/solidrouter.test.tsx index 5a5ab77e9f2c..f33b7e72daf4 100644 --- a/packages/solid/test/solidrouter.test.tsx +++ b/packages/solid/test/solidrouter.test.tsx @@ -1,4 +1,5 @@ import { spanToJSON } from '@sentry/browser'; +import type { Span } from '@sentry/core'; import { createTransport, getCurrentScope, @@ -9,7 +10,7 @@ import { } from '@sentry/core'; import type { MemoryHistory } from '@solidjs/router'; import { createMemoryHistory, MemoryRouter, Navigate, Route } from '@solidjs/router'; -import { render } from '@solidjs/testing-library'; +import { render, waitFor } from '@solidjs/testing-library'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { BrowserClient } from '../src'; import { solidRouterBrowserTracingIntegration, withSentryRouterRouting } from '../src/solidrouter'; @@ -114,39 +115,54 @@ describe('solidRouterBrowserTracingIntegration', () => { }); it.each([ - ['', '/navigate-to-about', '/about'], - ['for nested navigation', '/navigate-to-about-us', '/about/us'], - ['for navigation with param', '/navigate-to-user', '/user/5'], - ['for nested navigation with params', '/navigate-to-user-post', '/user/5/post/12'], - ])('starts a navigation span %s', (_itDescription, navigationPath, path) => { - const spanStartMock = vi.fn(); - - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.on('spanStart', span => { - spanStartMock(spanToJSON(span)); - }); - client.addIntegration(solidRouterBrowserTracingIntegration()); - const SentryRouter = withSentryRouterRouting(MemoryRouter); - - const history = createMemoryHistory(); - history.set({ value: navigationPath }); - - renderRouter(SentryRouter, history); - - expect(spanStartMock).toHaveBeenCalledWith( - expect.objectContaining({ - op: 'navigation', - description: path, - data: expect.objectContaining({ - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + ['', '/navigate-to-about', '/about', {}], + ['for nested navigation', '/navigate-to-about-us', '/about/us', {}], + ['for navigation with param', '/navigate-to-user', '/user/:id', { id: '5' }], + [ + 'for nested navigation with params', + '/navigate-to-user-post', + '/user/:id/post/:postId', + { id: '5', postId: '12' }, + ], + ])( + 'starts a parametrized navigation span %s', + async (_itDescription, navigationPath, parametrizedRoute, expectedParams) => { + const spans: Span[] = []; + + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.on('spanStart', span => { + spans.push(span); + }); + client.addIntegration(solidRouterBrowserTracingIntegration()); + const SentryRouter = withSentryRouterRouting(MemoryRouter); + + const history = createMemoryHistory(); + history.set({ value: navigationPath }); + + renderRouter(SentryRouter, history); + + // Wait for the router transition to complete (Navigate redirects are async) + await waitFor(() => { + const navSpan = spans.find(s => spanToJSON(s).op === 'navigation'); + expect(navSpan).toBeDefined(); + + const span = spanToJSON(navSpan!); + expect(span.description).toBe(parametrizedRoute); + expect(span.data).toMatchObject({ + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.solid.solidrouter', - }), - }), - ); - }); + }); + + for (const [key, value] of Object.entries(expectedParams as Record)) { + expect(span.data![`url.path.parameter.${key}`]).toBe(value); + expect(span.data![`params.${key}`]).toBe(value); + } + }); + }, + ); it('skips navigation span, with `instrumentNavigation: false`', () => { const spanStartMock = vi.fn(); @@ -172,7 +188,7 @@ describe('solidRouterBrowserTracingIntegration', () => { op: 'navigation', description: '/about', data: expect.objectContaining({ - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.solid.solidrouter', }), diff --git a/packages/solidstart/test/client/solidrouter.test.tsx b/packages/solidstart/test/client/solidrouter.test.tsx index 143a7340456e..1b9623cabc13 100644 --- a/packages/solidstart/test/client/solidrouter.test.tsx +++ b/packages/solidstart/test/client/solidrouter.test.tsx @@ -1,4 +1,5 @@ import { spanToJSON } from '@sentry/browser'; +import type { Span } from '@sentry/core'; import { createTransport, getCurrentScope, @@ -9,7 +10,7 @@ import { } from '@sentry/core'; import type { MemoryHistory } from '@solidjs/router'; import { createMemoryHistory, MemoryRouter, Navigate, Route } from '@solidjs/router'; -import { render } from '@solidjs/testing-library'; +import { render, waitFor } from '@solidjs/testing-library'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { BrowserClient } from '../../src/client'; import { solidRouterBrowserTracingIntegration, withSentryRouterRouting } from '../../src/client/solidrouter'; @@ -114,39 +115,54 @@ describe('solidRouterBrowserTracingIntegration', () => { }); it.each([ - ['', '/navigate-to-about', '/about'], - ['for nested navigation', '/navigate-to-about-us', '/about/us'], - ['for navigation with param', '/navigate-to-user', '/user/5'], - ['for nested navigation with params', '/navigate-to-user-post', '/user/5/post/12'], - ])('starts a navigation span %s', (_itDescription, navigationPath, path) => { - const spanStartMock = vi.fn(); - - const client = createMockBrowserClient(); - setCurrentClient(client); - - client.on('spanStart', span => { - spanStartMock(spanToJSON(span)); - }); - client.addIntegration(solidRouterBrowserTracingIntegration()); - const SentryRouter = withSentryRouterRouting(MemoryRouter); - - const history = createMemoryHistory(); - history.set({ value: navigationPath }); - - renderRouter(SentryRouter, history); - - expect(spanStartMock).toHaveBeenCalledWith( - expect.objectContaining({ - op: 'navigation', - description: path, - data: expect.objectContaining({ - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + ['', '/navigate-to-about', '/about', {}], + ['for nested navigation', '/navigate-to-about-us', '/about/us', {}], + ['for navigation with param', '/navigate-to-user', '/user/:id', { id: '5' }], + [ + 'for nested navigation with params', + '/navigate-to-user-post', + '/user/:id/post/:postId', + { id: '5', postId: '12' }, + ], + ])( + 'starts a parametrized navigation span %s', + async (_itDescription, navigationPath, parametrizedRoute, expectedParams) => { + const spans: Span[] = []; + + const client = createMockBrowserClient(); + setCurrentClient(client); + + client.on('spanStart', span => { + spans.push(span); + }); + client.addIntegration(solidRouterBrowserTracingIntegration()); + const SentryRouter = withSentryRouterRouting(MemoryRouter); + + const history = createMemoryHistory(); + history.set({ value: navigationPath }); + + renderRouter(SentryRouter, history); + + // Wait for the router transition to complete (Navigate redirects are async) + await waitFor(() => { + const navSpan = spans.find(s => spanToJSON(s).op === 'navigation'); + expect(navSpan).toBeDefined(); + + const span = spanToJSON(navSpan!); + expect(span.description).toBe(parametrizedRoute); + expect(span.data).toMatchObject({ + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.solidstart.solidrouter', - }), - }), - ); - }); + }); + + for (const [key, value] of Object.entries(expectedParams as Record)) { + expect(span.data![`url.path.parameter.${key}`]).toBe(value); + expect(span.data![`params.${key}`]).toBe(value); + } + }); + }, + ); it('skips navigation span, with `instrumentNavigation: false`', () => { const spanStartMock = vi.fn(); @@ -172,7 +188,7 @@ describe('solidRouterBrowserTracingIntegration', () => { op: 'navigation', description: '/about', data: expect.objectContaining({ - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'navigation', [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.solidstart.solidrouter', }),