Use token-based LIFO for navigation context cleanup

Author: onurtemizkan

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

@@ -26,6 +26,11 @@ export {
   pathIsWildcardAndHasChildren,
   getNumberOfUrlSegments,
   transactionNameHasWildcard,
+  getActiveRootSpan,
+  // Navigation context functions (internal use and testing)
+  setNavigationContext,
+  clearNavigationContext,
+  getNavigationContext,
 } from './utils';
 
 // Lazy route exports

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

@@ -747,12 +747,12 @@ function wrapPatchRoutesOnNavigation(
 
       const lazyLoadPromise = (async () => {
         // Set context so async handlers can access correct targetPath and span
-        setNavigationContext(targetPath, activeRootSpan);
+        const contextToken = setNavigationContext(targetPath, activeRootSpan);
         let result;
         try {
           result = await originalPatchRoutes(args);
         } finally {
-          clearNavigationContext(activeRootSpan);
+          clearNavigationContext(contextToken);
         }
 
         const currentActiveRootSpan = getActiveRootSpan();

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

@@ -1,5 +1,6 @@
 import type { Span, TransactionSource } from '@sentry/core';
-import { getActiveSpan, getRootSpan, spanToJSON } from '@sentry/core';
+import { debug, getActiveSpan, getRootSpan, spanToJSON } from '@sentry/core';
+import { DEBUG_BUILD } from '../debug-build';
 import type { Location, MatchRoutes, RouteMatch, RouteObject } from '../types';
 
 // Global variables that these utilities depend on
@@ -8,29 +9,38 @@ let _stripBasename: boolean = false;
 
 // Navigation context stack for nested/concurrent patchRoutesOnNavigation calls.
 // Required because window.location hasn't updated yet when handlers are invoked.
-// Uses a stack to handle overlapping navigations correctly (LIFO semantics).
 interface NavigationContext {
+  token: object;
   targetPath: string | undefined;
   span: Span | undefined;
 }
 
 const _navigationContextStack: NavigationContext[] = [];
 const MAX_CONTEXT_STACK_SIZE = 10;
 
-/** Pushes a navigation context before invoking patchRoutesOnNavigation. */
-export function setNavigationContext(targetPath: string | undefined, span: Span | undefined): void {
-  // Prevent unbounded stack growth from cleanup failures or rapid navigations
+/**
+ * Pushes a navigation context and returns a unique token for cleanup.
+ * The token uses object identity for uniqueness (no counter needed).
+ */
+export function setNavigationContext(targetPath: string | undefined, span: Span | undefined): object {
+  const token = {};
+  // Prevent unbounded stack growth - oldest (likely stale) contexts are evicted first
   if (_navigationContextStack.length >= MAX_CONTEXT_STACK_SIZE) {
-    _navigationContextStack.shift(); // Remove oldest
+    DEBUG_BUILD && debug.warn('[React Router] Navigation context stack overflow - removing oldest context');
+    _navigationContextStack.shift();
   }
-  _navigationContextStack.push({ targetPath, span });
+  _navigationContextStack.push({ token, targetPath, span });
+  return token;
 }
 
-/** Pops the navigation context for the given span after patchRoutesOnNavigation completes. */
-export function clearNavigationContext(span: Span | undefined): void {
-  // Only pop if top of stack matches this span to prevent corruption from mismatched calls
+/**
+ * Clears the navigation context if it's on top of the stack (LIFO).
+ * If our context is not on top (out-of-order completion), we leave it -
+ * it will be cleaned up by overflow protection when the stack fills up.
+ */
+export function clearNavigationContext(token: object): void {
   const top = _navigationContextStack[_navigationContextStack.length - 1];
-  if (top?.span === span) {
+  if (top?.token === token) {
     _navigationContextStack.pop();
   }
 }

packages/react/test/reactrouter-compat-utils/utils.test.ts

@@ -1,5 +1,7 @@
-import { beforeEach, describe, expect, it, vi } from 'vitest';
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
 import {
+  clearNavigationContext,
+  getNavigationContext,
   getNormalizedName,
   getNumberOfUrlSegments,
   initializeRouterUtils,
@@ -9,6 +11,7 @@ import {
   prefixWithSlash,
   rebuildRoutePathFromAllRoutes,
   resolveRouteNameAndSource,
+  setNavigationContext,
   transactionNameHasWildcard,
 } from '../../src/reactrouter-compat-utils';
 import type { Location, MatchRoutes, RouteMatch, RouteObject } from '../../src/types';
@@ -664,4 +667,137 @@ describe('reactrouter-compat-utils/utils', () => {
       expect(transactionNameHasWildcard('/path/to/asterisk')).toBe(false); // 'asterisk' contains 'isk' but not '*'
     });
   });
+
+  describe('navigation context management', () => {
+    // Clean up navigation context after each test by popping until empty
+    afterEach(() => {
+      // Pop all remaining contexts
+      while (getNavigationContext() !== null) {
+        const ctx = getNavigationContext();
+        if (ctx) {
+          clearNavigationContext((ctx as any).token);
+        }
+      }
+    });
+
+    describe('setNavigationContext', () => {
+      it('should return unique tokens (object identity)', () => {
+        const token1 = setNavigationContext('/path1', undefined);
+        const token2 = setNavigationContext('/path2', undefined);
+        const token3 = setNavigationContext('/path3', undefined);
+
+        // Each token should be a unique object
+        expect(token1).not.toBe(token2);
+        expect(token2).not.toBe(token3);
+        expect(token1).not.toBe(token3);
+      });
+
+      it('should store targetPath and span in context', () => {
+        const mockSpan = { name: 'test-span' } as any;
+        setNavigationContext('/test-path', mockSpan);
+
+        const context = getNavigationContext();
+        expect(context).not.toBeNull();
+        expect(context?.targetPath).toBe('/test-path');
+        expect(context?.span).toBe(mockSpan);
+      });
+
+      it('should handle undefined targetPath', () => {
+        setNavigationContext(undefined, undefined);
+
+        const context = getNavigationContext();
+        expect(context).not.toBeNull();
+        expect(context?.targetPath).toBeUndefined();
+      });
+    });
+
+    describe('clearNavigationContext', () => {
+      it('should remove context when token matches top of stack (LIFO)', () => {
+        const token = setNavigationContext('/test', undefined);
+
+        expect(getNavigationContext()).not.toBeNull();
+
+        clearNavigationContext(token);
+
+        expect(getNavigationContext()).toBeNull();
+      });
+
+      it('should NOT remove context when token is not on top (out-of-order completion)', () => {
+        // Simulate: Nav1 starts, Nav2 starts, Nav1 tries to complete first
+        const token1 = setNavigationContext('/nav1', undefined);
+        const token2 = setNavigationContext('/nav2', undefined);
+
+        // Most recent should be nav2
+        expect(getNavigationContext()?.targetPath).toBe('/nav2');
+
+        // Nav1 tries to complete first (out of order) - should NOT pop because nav1 is not on top
+        clearNavigationContext(token1);
+
+        // Nav2 should still be the current context (nav1's context is still buried)
+        expect(getNavigationContext()?.targetPath).toBe('/nav2');
+
+        // Nav2 completes - should pop because nav2 IS on top
+        clearNavigationContext(token2);
+
+        // Now nav1's stale context is on top (will be cleaned by overflow protection)
+        expect(getNavigationContext()?.targetPath).toBe('/nav1');
+      });
+
+      it('should not throw when clearing with unknown token', () => {
+        const unknownToken = {};
+        expect(() => clearNavigationContext(unknownToken)).not.toThrow();
+      });
+
+      it('should correctly handle LIFO cleanup order', () => {
+        const token1 = setNavigationContext('/path1', undefined);
+        const token2 = setNavigationContext('/path2', undefined);
+        const token3 = setNavigationContext('/path3', undefined);
+
+        // Clear in LIFO order
+        clearNavigationContext(token3);
+        expect(getNavigationContext()?.targetPath).toBe('/path2');
+
+        clearNavigationContext(token2);
+        expect(getNavigationContext()?.targetPath).toBe('/path1');
+
+        clearNavigationContext(token1);
+        expect(getNavigationContext()).toBeNull();
+      });
+    });
+
+    describe('getNavigationContext', () => {
+      it('should return null when stack is empty', () => {
+        expect(getNavigationContext()).toBeNull();
+      });
+
+      it('should return the most recent context', () => {
+        setNavigationContext('/first', undefined);
+        setNavigationContext('/second', undefined);
+        setNavigationContext('/third', undefined);
+
+        expect(getNavigationContext()?.targetPath).toBe('/third');
+      });
+    });
+
+    describe('stack overflow protection', () => {
+      it('should remove oldest context when stack exceeds limit', () => {
+        // Push 12 contexts (limit is 10)
+        const tokens: object[] = [];
+        for (let i = 0; i < 12; i++) {
+          tokens.push(setNavigationContext(`/path${i}`, undefined));
+        }
+
+        // Most recent should be /path11
+        expect(getNavigationContext()?.targetPath).toBe('/path11');
+
+        // The oldest contexts (path0, path1) were evicted due to overflow
+        // Trying to clear them does nothing (their tokens no longer match anything)
+        clearNavigationContext(tokens[0]!);
+        clearNavigationContext(tokens[1]!);
+
+        // /path11 should still be current
+        expect(getNavigationContext()?.targetPath).toBe('/path11');
+      });
+    });
+  });
 });