From 7b92f0820c0277c8224a831ec09196f121db985d Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 4 Sep 2026 13:03:23 +0200 Subject: [PATCH 1/3] fix(nextjs): Keep router.back/forward navigation type under span streaming In router-patch mode, router.back() and router.forward() started a navigation span with a placeholder name that the popstate listener renamed later. With span streaming, ignoreSpans is applied when a span starts, so the placeholder span was non-recording from the start and popstate fell through to creating a second span tagged browser.popstate. Instead of the placeholder span, remember the router method and its timestamp, and start the navigation span from the popstate event with that navigation type and start time. This keeps the span starting at the router call, works for both trace lifecycles, and no longer records an ignored-span client outcome per traversal. A pending traversal expires after one second so a back()/forward() with no matching history entry cannot be attributed to a later unrelated popstate. The placeholder name and its ignoreSpans entry are removed since nothing produces such spans anymore. Fixes #23909 Refs JS-3527 Co-Authored-By: Claude Fable 5.1 --- ...client-app-routing-instrumentation.test.ts | 10 +- packages/nextjs/src/client/index.ts | 7 +- .../appRouterRoutingInstrumentation.ts | 83 +++++---- .../appRouterRoutingInstrumentation.test.ts | 161 ++++++++++++++++++ packages/nextjs/test/clientSdk.test.ts | 26 --- 5 files changed, 212 insertions(+), 75 deletions(-) create mode 100644 packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts index a967ebd7eaef..4d4c89316408 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/client-app-routing-instrumentation.test.ts @@ -81,10 +81,7 @@ test('Creates a navigation span for `router.replace()`', async ({ page }) => { expect(await navigationSpanPromise).toBeDefined(); }); -// Skipped rather than relaxed to `browser.popstate`: under span streaming these navigations lose the -// back/forward distinction, which looks like a regression rather than intended behaviour. -// See https://github.com/getsentry/sentry-javascript/issues/23909 -test.skip('Creates a navigation span for `router.back()`', async ({ page }) => { +test('Creates a navigation span for `router.back()`', async ({ page }) => { const navigationSpanPromise = waitForStreamedSpan('nextjs-app-dir', span => { return span.name === `/navigation/:param/router-back` && getSpanOp(span) === 'navigation'; }); @@ -101,10 +98,7 @@ test.skip('Creates a navigation span for `router.back()`', async ({ page }) => { expect(navigationSpan.attributes['navigation.type']?.value).toMatch(/router\.(back|traverse)/); }); -// Skipped rather than relaxed to `browser.popstate`: under span streaming these navigations lose the -// back/forward distinction, which looks like a regression rather than intended behaviour. -// See https://github.com/getsentry/sentry-javascript/issues/23909 -test.skip('Creates a navigation span for `router.forward()`', async ({ page }) => { +test('Creates a navigation span for `router.forward()`', async ({ page }) => { const navigationSpanPromise = waitForStreamedSpan('nextjs-app-dir', span => { return ( span.name === `/navigation/:param/router-push` && diff --git a/packages/nextjs/src/client/index.ts b/packages/nextjs/src/client/index.ts index 5c5d3ffc2c85..3c8c091d2b91 100644 --- a/packages/nextjs/src/client/index.ts +++ b/packages/nextjs/src/client/index.ts @@ -11,7 +11,6 @@ import { getVercelEnv } from '../common/getVercelEnv'; import { isRedirectNavigationError } from '../common/nextNavigationErrorUtils'; import { browserTracingIntegration } from './browserTracingIntegration'; import { nextjsClientStackFrameNormalizationIntegration } from './clientNormalizationIntegration'; -import { INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME } from './routing/appRouterRoutingInstrumentation'; import { removeIsrSsgTraceMetaTags } from './routing/isrRoutingTracing'; import { applyTunnelRouteOption } from './tunnelRoute'; @@ -74,12 +73,8 @@ export function init(options: BrowserOptions): Client | undefined { opts.ignoreSpans = [ ...(opts.ignoreSpans || []), - // we filter out segment spans for /404 pages + // we filter out segment spans for /404 pages (exact match, so a string match isn't safe) /^\/404$/, - // segment spans where we didn't get a reasonable transaction name - // in this case, constructing a dynamic RegExp is fine because the variable is a constant - // we need to ensure to exact-match, so a string match isn't safe (same for /404 above) - new RegExp(`^${INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME}$`), ]; const client = reactInit(opts); diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index aff86d2c2e37..55b46ba42a84 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -6,6 +6,7 @@ import { PAGELOAD_SPAN_NAME_FALLBACK, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, filterCollectedUrl, + timestampInSeconds, } from '@sentry/core'; import { startBrowserTracingNavigationSpan, @@ -38,7 +39,34 @@ function setNavigationSpanUrlAttributes(span: Span, urlPath: string, urlOrPath: }); } -export const INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME = 'incomplete-app-router-transaction'; +/** + * `router.back()` and `router.forward()` carry no destination, so their navigation span can only be + * started once the resulting `popstate` event tells us where we ended up. Until then, this remembers + * which router method triggered the traversal and when, so the span still gets the router's + * navigation type and starts at the router call rather than at the `popstate`. + */ +interface PendingHistoryTraversal { + navigationType: 'router.back' | 'router.forward'; + startTime: number; +} + +let pendingHistoryTraversal: PendingHistoryTraversal | undefined; + +/** + * A `back()`/`forward()` without a matching history entry never fires `popstate`. Without an expiry, + * a later unrelated `popstate` (e.g. the browser's back button) would be attributed to that stale + * router call. Browsers dispatch the `popstate` of a same-document traversal within a few + * milliseconds, so anything older than this is not the traversal we are waiting for. + */ +const PENDING_HISTORY_TRAVERSAL_MAX_AGE_S = 1; + +function takePendingHistoryTraversal(): PendingHistoryTraversal | undefined { + const traversal = pendingHistoryTraversal; + pendingHistoryTraversal = undefined; + return traversal && timestampInSeconds() - traversal.startTime <= PENDING_HISTORY_TRAVERSAL_MAX_AGE_S + ? traversal + : undefined; +} /** * This mutable keeps track of what router navigation instrumentation mechanism we are using. @@ -164,7 +192,10 @@ export function appRouterInstrumentNavigation(client: Client): void { // With span streaming, span names have to be low cardinality, so we can't fall back to the URL. const spanName = parameterizedPathname ?? (hasSpanStreamingEnabled(client) ? NAVIGATION_SPAN_NAME_FALLBACK : pathname); - if (currentRouterPatchingNavigationSpanRef.current?.isRecording()) { + const traversal = takePendingHistoryTraversal(); + // A traversal triggered through the router always gets its own span: an open router-patch span + // here would be a `push()`/`replace()` that the user navigated away from again. + if (!traversal && currentRouterPatchingNavigationSpanRef.current?.isRecording()) { currentRouterPatchingNavigationSpanRef.current.updateName(spanName); currentRouterPatchingNavigationSpanRef.current.setAttribute( SENTRY_SEGMENT_NAME_SOURCE, @@ -179,10 +210,11 @@ export function appRouterInstrumentNavigation(client: Client): void { client, { name: spanName, + startTime: traversal?.startTime, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', [SENTRY_SEGMENT_NAME_SOURCE]: parameterizedPathname ? 'route' : 'url', - 'navigation.type': 'browser.popstate', + 'navigation.type': traversal?.navigationType ?? 'browser.popstate', ...(parameterizedPathname && { [URL_TEMPLATE]: parameterizedPathname }), }, }, @@ -252,56 +284,37 @@ function patchRouter(client: Client, router: NextRouter, currentNavigationSpanRe return target.apply(thisArg, argArray); } - let transactionName = INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME; - const transactionAttributes: Record = { - [SENTRY_OP]: NAVIGATION, - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', - [SENTRY_SEGMENT_NAME_SOURCE]: 'url', - }; + if (routerFunctionName === 'back' || routerFunctionName === 'forward') { + pendingHistoryTraversal = { + navigationType: `router.${routerFunctionName}`, + startTime: timestampInSeconds(), + }; + return target.apply(thisArg, argArray); + } const href = argArray[0]; const basePath = process.env._sentryBasePath ?? globalWithInjectedBasePath._sentryBasePath; const normalizedHref = basePath && typeof href === 'string' && !href.startsWith(basePath) ? `${basePath}${href}` : href; - if (routerFunctionName === 'push') { - transactionName = stripTrailingSlash(transactionNameifyRouterArgument(normalizedHref)); - transactionAttributes['navigation.type'] = 'router.push'; - } else if (routerFunctionName === 'replace') { - transactionName = stripTrailingSlash(transactionNameifyRouterArgument(normalizedHref)); - transactionAttributes['navigation.type'] = 'router.replace'; - } else if (routerFunctionName === 'back') { - transactionAttributes['navigation.type'] = 'router.back'; - } else if (routerFunctionName === 'forward') { - transactionAttributes['navigation.type'] = 'router.forward'; - } - + const transactionName = stripTrailingSlash(transactionNameifyRouterArgument(normalizedHref)); const parameterizedPathname = maybeParameterizeRoute(transactionName); - const navigationUrl = - routerFunctionName === 'back' || routerFunctionName === 'forward' - ? undefined - : getAbsoluteUrl(normalizedHref); - - // The incomplete-instrumentation placeholder is a static name, so it is low cardinality - // already, and keeping it is what makes the `ignoreSpans` entry filtering those spans match. - const isPlaceholderName = transactionName === INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME; - currentNavigationSpanRef.current = startBrowserTracingNavigationSpan( client, { // With span streaming, span names have to be low cardinality, so we can't fall back to the URL. name: parameterizedPathname ?? - (isPlaceholderName || !hasSpanStreamingEnabled(client) - ? transactionName - : NAVIGATION_SPAN_NAME_FALLBACK), + (hasSpanStreamingEnabled(client) ? NAVIGATION_SPAN_NAME_FALLBACK : transactionName), attributes: { - ...transactionAttributes, + [SENTRY_OP]: NAVIGATION, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.navigation.nextjs.app_router_instrumentation', [SENTRY_SEGMENT_NAME_SOURCE]: parameterizedPathname ? 'route' : 'url', + 'navigation.type': `router.${routerFunctionName}`, ...(parameterizedPathname && { [URL_TEMPLATE]: parameterizedPathname }), }, }, - navigationUrl ? { url: navigationUrl } : undefined, + { url: getAbsoluteUrl(normalizedHref) }, ); return target.apply(thisArg, argArray); diff --git a/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts b/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts new file mode 100644 index 000000000000..99051f66f4fb --- /dev/null +++ b/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts @@ -0,0 +1,161 @@ +// @vitest-environment jsdom +import type { Client } from '@sentry/core'; +import type * as SentryCore from '@sentry/core'; +import type * as SentryReact from '@sentry/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type * as AppRouterInstrumentation from '../../src/client/routing/appRouterRoutingInstrumentation'; +import type { RouteManifest } from '../../src/config/manifest/types'; + +type Core = typeof SentryCore; +type React = typeof SentryReact; +type Instrumentation = typeof AppRouterInstrumentation; + +interface NextRouter { + back: () => void; + forward: () => void; + push: (target: string) => void; + replace: (target: string) => void; +} + +const globalWithNext = globalThis as typeof globalThis & { + next?: { router?: NextRouter }; + _sentryRouteManifest?: string; +}; + +const manifest: RouteManifest = { + staticRoutes: [{ path: '/navigation' }], + dynamicRoutes: [ + { + path: '/navigation/:param/router-back', + regex: '^/navigation/([^/]+)/router-back$', + paramNames: ['param'], + hasOptionalPrefix: false, + }, + ], + isrRoutes: [], +}; + +function sleep(ms: number): Promise { + return new Promise(resolve => setTimeout(resolve, ms)); +} + +/** + * The instrumentation module keeps its routing state (patched routers, the current navigation span, + * the popstate listener) at module level, so every test gets fresh copies of it and of the SDK + * packages it imports. + */ +async function setup(traceLifecycle: 'stream' | 'static'): Promise<{ + core: Core; + router: NextRouter; + client: Client; +}> { + vi.resetModules(); + const core: Core = await import('@sentry/core'); + const react: React = await import('@sentry/react'); + const instrumentation: Instrumentation = await import('../../src/client/routing/appRouterRoutingInstrumentation'); + + const client = new react.BrowserClient({ + dsn: 'http://examplePublicKey@localhost/0', + transport: () => core.createTransport({ recordDroppedEvent: () => undefined }, () => core.resolvedSyncPromise({})), + stackParser: () => [], + tracesSampleRate: 1, + traceLifecycle, + integrations: [react.browserTracingIntegration({ instrumentPageLoad: false, instrumentNavigation: false })], + }); + core.setCurrentClient(client); + client.init(); + + const router: NextRouter = { back: vi.fn(), forward: vi.fn(), push: vi.fn(), replace: vi.fn() }; + const originalBack = router.back; + globalWithNext.next = { router }; + + instrumentation.appRouterInstrumentNavigation(client); + await vi.waitFor(() => expect(router.back).not.toBe(originalBack)); + + return { core, router, client }; +} + +describe('appRouterInstrumentNavigation (router-patch mode)', () => { + beforeEach(() => { + globalWithNext._sentryRouteManifest = JSON.stringify(manifest); + window.history.replaceState({}, '', '/navigation'); + }); + + afterEach(() => { + delete globalWithNext.next; + delete globalWithNext._sentryRouteManifest; + }); + + describe.each(['stream', 'static'] as const)('with traceLifecycle %s', traceLifecycle => { + it('tags the navigation span of `router.back()` with `router.back` and starts it at the call', async () => { + const { core, router } = await setup(traceLifecycle); + + const beforeCall = core.timestampInSeconds(); + router.back(); + const afterCall = core.timestampInSeconds(); + + await sleep(30); + window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + const spanJson = core.spanToJSON(span!); + expect(spanJson.name).toBe('/navigation/:param/router-back'); + expect(spanJson.attributes).toEqual( + expect.objectContaining({ + 'sentry.op': 'navigation', + 'navigation.type': 'router.back', + 'url.template': '/navigation/:param/router-back', + 'url.path': '/navigation/1337/router-back', + }), + ); + expect(spanJson.start_timestamp).toBeGreaterThanOrEqual(beforeCall); + expect(spanJson.start_timestamp).toBeLessThanOrEqual(afterCall); + }); + + it('tags the navigation span of `router.forward()` with `router.forward`', async () => { + const { core, router } = await setup(traceLifecycle); + + router.forward(); + window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + expect(core.spanToJSON(span!).attributes).toEqual( + expect.objectContaining({ 'navigation.type': 'router.forward' }), + ); + }); + + it('tags a popstate without a preceding router call with `browser.popstate`', async () => { + const { core } = await setup(traceLifecycle); + + window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + const spanJson = core.spanToJSON(span!); + expect(spanJson.name).toBe('/navigation/:param/router-back'); + expect(spanJson.attributes).toEqual(expect.objectContaining({ 'navigation.type': 'browser.popstate' })); + }); + + it('does not carry a router call over to a later, unrelated popstate', async () => { + const { core, router } = await setup(traceLifecycle); + + router.forward(); + // A `forward()` without a forward history entry never fires `popstate`. + await sleep(1100); + + window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + expect(core.spanToJSON(span!).attributes).toEqual( + expect.objectContaining({ 'navigation.type': 'browser.popstate' }), + ); + }); + }); +}); diff --git a/packages/nextjs/test/clientSdk.test.ts b/packages/nextjs/test/clientSdk.test.ts index 3aa69f92f8b3..fc59741506d4 100644 --- a/packages/nextjs/test/clientSdk.test.ts +++ b/packages/nextjs/test/clientSdk.test.ts @@ -5,7 +5,6 @@ import { getClient, WINDOW } from '@sentry/react'; import { JSDOM } from 'jsdom'; import { afterAll, afterEach, describe, expect, it, vi } from 'vitest'; import { breadcrumbsIntegration, browserTracingIntegration, init } from '../src/client'; -import { INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME } from '../src/client/routing/appRouterRoutingInstrumentation'; const reactInit = vi.spyOn(SentryReact, 'init'); const debugLogSpy = vi.spyOn(debug, 'log'); @@ -101,19 +100,6 @@ describe('Client init()', () => { expect(debugLogSpy).toHaveBeenCalledWith(expect.stringContaining('matches `ignoreSpans`')); }); - it('drops incomplete navigation transactions', () => { - init({ dsn: TEST_DSN_404, tracesSampleRate: 1.0 }); - const transportSend = vi.spyOn(getClient()!.getTransport()!, 'send'); - - // Ensure we have no current span, so our next span is a transaction - SentryReact.withActiveSpan(null, () => { - SentryReact.startInactiveSpan({ name: INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME })?.end(); - }); - - expect(transportSend).not.toHaveBeenCalled(); - expect(debugLogSpy).toHaveBeenCalledWith(expect.stringContaining('matches `ignoreSpans`')); - }); - describe('span streaming', () => { it('drops /404 segment spans', () => { init({ dsn: TEST_DSN_404, tracesSampleRate: 1.0, traceLifecycle: 'stream' }); @@ -125,18 +111,6 @@ describe('Client init()', () => { expect(debugLogSpy).toHaveBeenCalledWith(expect.stringContaining('matches `ignoreSpans`')); }); - it('drops incomplete navigation segment spans', () => { - init({ dsn: TEST_DSN_404, tracesSampleRate: 1.0, traceLifecycle: 'stream' }); - - // Ensure we have no current span, so our next span is a segment span - const span = SentryReact.withActiveSpan(null, () => - SentryReact.startInactiveSpan({ name: INCOMPLETE_APP_ROUTER_INSTRUMENTATION_TRANSACTION_NAME }), - ); - - expect(span).toBeInstanceOf(SentryNonRecordingSpan); - expect(debugLogSpy).toHaveBeenCalledWith(expect.stringContaining('matches `ignoreSpans`')); - }); - it('drops /404 non-segment spans', () => { init({ dsn: TEST_DSN_404, tracesSampleRate: 1.0, traceLifecycle: 'stream' }); From 659c92197d88b21df98c97327c649022de2a7abc Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 4 Sep 2026 13:40:08 +0200 Subject: [PATCH 2/3] fix(nextjs): Expire pending history traversal with a timer A timestamp comparison misattributed the popstate when the main thread was blocked between router.back() and the event, since the delay counts against wall-clock age. A timer is delayed by the same block, which is how the previous placeholder span's idle timeout behaved. Also cover the branch that starts a fresh span when back() is called while a push() span is still open. Refs JS-3527 Co-Authored-By: Claude Fable 5.1 --- .../appRouterRoutingInstrumentation.ts | 25 +++++++----- .../appRouterRoutingInstrumentation.test.ts | 40 ++++++++++++++++++- 2 files changed, 55 insertions(+), 10 deletions(-) diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index 55b46ba42a84..f2603ecc90aa 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -51,21 +51,31 @@ interface PendingHistoryTraversal { } let pendingHistoryTraversal: PendingHistoryTraversal | undefined; +let pendingHistoryTraversalTimeout: ReturnType | undefined; /** * A `back()`/`forward()` without a matching history entry never fires `popstate`. Without an expiry, * a later unrelated `popstate` (e.g. the browser's back button) would be attributed to that stale * router call. Browsers dispatch the `popstate` of a same-document traversal within a few - * milliseconds, so anything older than this is not the traversal we are waiting for. + * milliseconds, so anything older than this is not the traversal we are waiting for. A timer rather + * than a timestamp comparison keeps this tolerant of a blocked main thread, which delays the + * `popstate` and the timer alike. */ -const PENDING_HISTORY_TRAVERSAL_MAX_AGE_S = 1; +const PENDING_HISTORY_TRAVERSAL_TIMEOUT_MS = 1000; + +function setPendingHistoryTraversal(navigationType: PendingHistoryTraversal['navigationType']): void { + clearTimeout(pendingHistoryTraversalTimeout); + pendingHistoryTraversal = { navigationType, startTime: timestampInSeconds() }; + pendingHistoryTraversalTimeout = setTimeout(() => { + pendingHistoryTraversal = undefined; + }, PENDING_HISTORY_TRAVERSAL_TIMEOUT_MS); +} function takePendingHistoryTraversal(): PendingHistoryTraversal | undefined { + clearTimeout(pendingHistoryTraversalTimeout); const traversal = pendingHistoryTraversal; pendingHistoryTraversal = undefined; - return traversal && timestampInSeconds() - traversal.startTime <= PENDING_HISTORY_TRAVERSAL_MAX_AGE_S - ? traversal - : undefined; + return traversal; } /** @@ -285,10 +295,7 @@ function patchRouter(client: Client, router: NextRouter, currentNavigationSpanRe } if (routerFunctionName === 'back' || routerFunctionName === 'forward') { - pendingHistoryTraversal = { - navigationType: `router.${routerFunctionName}`, - startTime: timestampInSeconds(), - }; + setPendingHistoryTraversal(`router.${routerFunctionName}`); return target.apply(thisArg, argArray); } diff --git a/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts b/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts index 99051f66f4fb..a6a523320bd9 100644 --- a/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts +++ b/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts @@ -42,7 +42,8 @@ function sleep(ms: number): Promise { /** * The instrumentation module keeps its routing state (patched routers, the current navigation span, * the popstate listener) at module level, so every test gets fresh copies of it and of the SDK - * packages it imports. + * packages it imports. The `popstate` listeners of earlier tests stay registered on `window`, but + * they only reach clients that are no longer current, whose navigation handlers bail out early. */ async function setup(traceLifecycle: 'stream' | 'static'): Promise<{ core: Core; @@ -128,6 +129,43 @@ describe('appRouterInstrumentNavigation (router-patch mode)', () => { ); }); + it('keeps the router call when the main thread is blocked until the popstate', async () => { + const { core, router } = await setup(traceLifecycle); + + router.back(); + const blockedUntil = Date.now() + 1100; + while (Date.now() < blockedUntil) { + // busy-wait + } + window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + expect(core.spanToJSON(span!).attributes).toEqual(expect.objectContaining({ 'navigation.type': 'router.back' })); + }); + + it('starts a new span for `router.back()` while a `router.push()` span is still open', async () => { + const { core, router } = await setup(traceLifecycle); + + router.push('/navigation'); + const pushSpan = core.getActiveSpan(); + expect(pushSpan).toBeDefined(); + + router.back(); + window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.dispatchEvent(new PopStateEvent('popstate')); + + const span = core.getActiveSpan(); + expect(span).toBeDefined(); + expect(span).not.toBe(pushSpan); + expect(core.spanToJSON(span!).attributes).toEqual(expect.objectContaining({ 'navigation.type': 'router.back' })); + expect(core.spanToJSON(pushSpan!).attributes).toEqual( + expect.objectContaining({ 'navigation.type': 'router.push' }), + ); + expect(core.spanToJSON(pushSpan!).end_timestamp).toBeDefined(); + }); + it('tags a popstate without a preceding router call with `browser.popstate`', async () => { const { core } = await setup(traceLifecycle); From 25cf59a6c55fc22037bf5d6f7f7f573c4c4e597b Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 4 Sep 2026 14:10:15 +0200 Subject: [PATCH 3/3] fix(nextjs): Keep the query string on popstate navigation spans Spans started from popstate built their URL from the pathname alone, so router.back()/forward() lost the query string in url.full that the previous update path preserved from location.href. Pass the full location instead, which also aligns plain browser.popstate spans. Drive the traversal expiry test with fake timers instead of a real sleep. Refs JS-3527 Co-Authored-By: Claude Fable 5.1 --- .../src/client/routing/appRouterRoutingInstrumentation.ts | 4 +++- .../test/client/appRouterRoutingInstrumentation.test.ts | 7 +++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts index f2603ecc90aa..eb3edbf3eced 100644 --- a/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts +++ b/packages/nextjs/src/client/routing/appRouterRoutingInstrumentation.ts @@ -228,7 +228,9 @@ export function appRouterInstrumentNavigation(client: Client): void { ...(parameterizedPathname && { [URL_TEMPLATE]: parameterizedPathname }), }, }, - { url: getAbsoluteUrl(pathname) }, + // The full location rather than just the pathname, so the span's `url.full` keeps the + // (filtered) query string like the update path above does. + { url: WINDOW.location.href }, ); } }); diff --git a/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts b/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts index a6a523320bd9..098f54581b58 100644 --- a/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts +++ b/packages/nextjs/test/client/appRouterRoutingInstrumentation.test.ts @@ -83,6 +83,7 @@ describe('appRouterInstrumentNavigation (router-patch mode)', () => { }); afterEach(() => { + vi.useRealTimers(); delete globalWithNext.next; delete globalWithNext._sentryRouteManifest; }); @@ -96,7 +97,7 @@ describe('appRouterInstrumentNavigation (router-patch mode)', () => { const afterCall = core.timestampInSeconds(); await sleep(30); - window.history.replaceState({}, '', '/navigation/1337/router-back'); + window.history.replaceState({}, '', '/navigation/1337/router-back?foo=bar'); window.dispatchEvent(new PopStateEvent('popstate')); const span = core.getActiveSpan(); @@ -109,6 +110,7 @@ describe('appRouterInstrumentNavigation (router-patch mode)', () => { 'navigation.type': 'router.back', 'url.template': '/navigation/:param/router-back', 'url.path': '/navigation/1337/router-back', + 'url.full': 'http://localhost:3000/navigation/1337/router-back?foo=bar', }), ); expect(spanJson.start_timestamp).toBeGreaterThanOrEqual(beforeCall); @@ -182,9 +184,10 @@ describe('appRouterInstrumentNavigation (router-patch mode)', () => { it('does not carry a router call over to a later, unrelated popstate', async () => { const { core, router } = await setup(traceLifecycle); + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }); router.forward(); // A `forward()` without a forward history entry never fires `popstate`. - await sleep(1100); + vi.advanceTimersByTime(1000); window.history.replaceState({}, '', '/navigation/1337/router-back'); window.dispatchEvent(new PopStateEvent('popstate'));