fix: ensure that trailingSlashes don't break path matching (#1711)

* fix: ensure that trailingSlashes don't break path matching

* chore: this wasn't on purpose

* fix: make `removeTrailingSlash` account for basepath

* test: tests for basepath

* refactor: consolidate how we handle basepath replacing

* test: matrix tests with different basepaths for utils

* refactor: revert basereplace

packages/react-router/src/link.tsx

@@ -3,7 +3,8 @@ import { flushSync } from 'react-dom'
 import { useMatch } from './useMatch'
 import { useRouterState } from './useRouterState'
 import { useRouter } from './useRouter'
-import { deepEqual, exactPathTest, functionalUpdate } from './utils'
+import { deepEqual, functionalUpdate } from './utils'
+import { exactPathTest, removeTrailingSlash } from './path'
 import type { AnyRouter, ParsedLocation } from '.'
 import type { HistoryState } from '@tanstack/history'
 import type { AnyRoute, RootSearchSchema } from './route'
@@ -615,14 +616,20 @@ export function useLinkProps<
   const isActive = useRouterState({
     select: (s) => {
       // Compare path/hash for matches
-      const currentPathSplit = s.location.pathname.split('/')
-      const nextPathSplit = next.pathname.split('/')
+      const currentPathSplit = removeTrailingSlash(
+        s.location.pathname,
+        router.basepath,
+      ).split('/')
+      const nextPathSplit = removeTrailingSlash(
+        next.pathname,
+        router.basepath,
+      ).split('/')
       const pathIsFuzzyEqual = nextPathSplit.every(
         (d, i) => d === currentPathSplit[i],
       )
       // Combine the matches based on user router.options
       const pathTest = activeOptions?.exact
-        ? exactPathTest(s.location.pathname, next.pathname)
+        ? exactPathTest(s.location.pathname, next.pathname, router.basepath)
         : pathIsFuzzyEqual
       const hashTest = activeOptions?.includeHash
         ? s.location.hash === next.hash

packages/react-router/src/path.ts

@@ -28,6 +28,28 @@ export function trimPath(path: string) {
   return trimPathRight(trimPathLeft(path))
 }
 
+export function removeTrailingSlash(value: string, basepath: string): string {
+  if (value.endsWith('/') && value !== '/' && value !== `${basepath}/`) {
+    return value.slice(0, -1)
+  }
+  return value
+}
+
+// intended to only compare path name
+// see the usage in the isActive under useLinkProps
+// /sample/path1 = /sample/path1/
+// /sample/path1/some <> /sample/path1
+export function exactPathTest(
+  pathName1: string,
+  pathName2: string,
+  basepath: string,
+): boolean {
+  return (
+    removeTrailingSlash(pathName1, basepath) ===
+    removeTrailingSlash(pathName2, basepath)
+  )
+}
+
 // When resolving relative paths, we treat all paths as if they are trailing slash
 // documents. All trailing slashes are removed after the path is resolved.
 // Here are a few examples:

packages/react-router/src/utils.ts

@@ -309,21 +309,6 @@ export function escapeJSON(jsonString: string) {
     .replace(/"/g, '\\"') // Escape double quotes
 }
 
-export function removeTrailingSlash(value: string): string {
-  if (value.endsWith('/') && value !== '/') {
-    return value.slice(0, -1)
-  }
-  return value
-}
-
-// intended to only compare path name
-// see the usage in the isActive under useLinkProps
-// /sample/path1 = /sample/path1/
-// /sample/path1/some <> /sample/path1
-export function exactPathTest(pathName1: string, pathName2: string): boolean {
-  return removeTrailingSlash(pathName1) === removeTrailingSlash(pathName2)
-}
-
 export type ControlledPromise<T> = Promise<T> & {
   resolve: (value: T) => void
   reject: (value: any) => void

packages/react-router/tests/link.test.tsx

@@ -10,20 +10,20 @@ import {
 } from '@testing-library/react'
 
 import {
+  Link,
+  Outlet,
+  RouterProvider,
   createLink,
   createMemoryHistory,
   createRootRoute,
+  createRootRouteWithContext,
   createRoute,
   createRouter,
-  Link,
-  RouterProvider,
-  useLoaderData,
-  useSearch,
   redirect,
-  useRouteContext,
+  useLoaderData,
   useParams,
-  Outlet,
-  createRootRouteWithContext,
+  useRouteContext,
+  useSearch,
 } from '../src'
 
 afterEach(() => {
@@ -1635,6 +1635,146 @@ describe('Link', () => {
       ),
     ).toBeInTheDocument()
   })
+
+  describe('active links', () => {
+    const makeRouter = ({
+      trailingSlash,
+      basepath,
+    }: {
+      trailingSlash: boolean
+      basepath: string | undefined
+    }) => {
+      const rootRoute = createRootRoute({
+        component: () => {
+          return (
+            <React.Fragment>
+              <h1>Root</h1>
+              <Link to={`/posts${trailingSlash ? '/' : ''}`}>Posts</Link>
+              <Link
+                to={`/posts/$postId${trailingSlash ? '/' : ''}`}
+                params={{ postId: 'id1' }}
+              >
+                To first post
+              </Link>
+              <Outlet />
+            </React.Fragment>
+          )
+        },
+      })
+
+      const indexRoute = createRoute({
+        getParentRoute: () => rootRoute,
+        path: '/',
+        component: () => {
+          return (
+            <React.Fragment>
+              <h1>Index</h1>
+            </React.Fragment>
+          )
+        },
+      })
+
+      const postsRoute = createRoute({
+        getParentRoute: () => rootRoute,
+        path: 'posts',
+        component: () => {
+          return (
+            <React.Fragment>
+              <h1>Posts</h1>
+              <Outlet />
+            </React.Fragment>
+          )
+        },
+      })
+
+      const PostComponent = () => {
+        const params = useParams({ strict: false })
+        return (
+          <React.Fragment>
+            <span>Params: {params.postId}</span>
+          </React.Fragment>
+        )
+      }
+
+      const postRoute = createRoute({
+        getParentRoute: () => postsRoute,
+        path: '$postId',
+        component: PostComponent,
+      })
+
+      return createRouter({
+        basepath,
+        trailingSlash: trailingSlash ? 'always' : undefined,
+        routeTree: rootRoute.addChildren([
+          indexRoute,
+          postsRoute.addChildren([postRoute]),
+        ]),
+      })
+    }
+
+    describe('no basepath', () => {
+      test('when on /posts/$postId, Links to /posts should be active', async () => {
+        const router = makeRouter({ trailingSlash: false, basepath: undefined })
+        render(<RouterProvider router={router} />)
+
+        fireEvent.click(
+          await screen.findByRole('link', { name: 'To first post' }),
+        )
+
+        const postsLink = await screen.findByRole('link', { name: 'Posts' })
+
+        expect(router.state.location.pathname).toBe('/posts/id1')
+        expect(postsLink).toHaveAttribute('data-status', 'active')
+        expect(postsLink).toHaveAttribute('aria-current', 'page')
+      })
+      test('when on /posts/$postId/, Links to /posts/ should be active (trailingSlash: always)', async () => {
+        const router = makeRouter({ trailingSlash: true, basepath: undefined })
+
+        render(<RouterProvider router={router} />)
+
+        fireEvent.click(
+          await screen.findByRole('link', { name: 'To first post' }),
+        )
+
+        const postsLink = await screen.findByRole('link', { name: 'Posts' })
+
+        expect(router.state.location.pathname).toBe('/posts/id1/')
+        expect(postsLink).toHaveAttribute('data-status', 'active')
+        expect(postsLink).toHaveAttribute('aria-current', 'page')
+      })
+    })
+    describe('with basepath', () => {
+      test('when on /app/posts/$postId, Links to /posts should be active', async () => {
+        const router = makeRouter({ trailingSlash: false, basepath: '/app' })
+        render(<RouterProvider router={router} />)
+
+        fireEvent.click(
+          await screen.findByRole('link', { name: 'To first post' }),
+        )
+
+        const postsLink = await screen.findByRole('link', { name: 'Posts' })
+
+        expect(router.state.location.pathname).toBe('/app/posts/id1')
+        expect(postsLink).toHaveAttribute('data-status', 'active')
+        expect(postsLink).toHaveAttribute('aria-current', 'page')
+      })
+      test('when on /app/posts/$postId/, Links to /posts/ should be active (trailingSlash: always)', async () => {
+        const router = makeRouter({ trailingSlash: true, basepath: '/app' })
+
+        render(<RouterProvider router={router} />)
+
+        fireEvent.click(
+          await screen.findByRole('link', { name: 'To first post' }),
+        )
+
+        const postsLink = await screen.findByRole('link', { name: 'Posts' })
+
+        expect(router.state.location.pathname).toBe('/app/posts/id1/')
+        expect(postsLink).toHaveAttribute('data-status', 'active')
+        expect(postsLink).toHaveAttribute('aria-current', 'page')
+      })
+    })
+  })
 })
 
 describe('createLink', () => {

packages/react-router/tests/path.test.ts

@@ -0,0 +1,71 @@
+import { describe, expect, it } from 'vitest'
+import { exactPathTest, removeTrailingSlash } from '../src/path'
+
+describe.each([{ basepath: '/' }, { basepath: '/app' }, { basepath: '/app/' }])(
+  'removeTrailingSlash with basepath $basepath',
+  ({ basepath }) => {
+    it('should remove trailing slash if present', () => {
+      const input = 'https://example.com/'
+      const expectedOutput = 'https://example.com'
+      const result = removeTrailingSlash(input, basepath)
+      expect(result).toBe(expectedOutput)
+    })
+    it('should not modify the string if no trailing slash present', () => {
+      const input = 'https://example.com'
+      const result = removeTrailingSlash(input, basepath)
+      expect(result).toBe(input)
+    })
+    it('should handle empty string', () => {
+      const input = ''
+      const result = removeTrailingSlash(input, basepath)
+      expect(result).toBe(input)
+    })
+    it('should handle strings with only a slash', () => {
+      const input = '/'
+      const result = removeTrailingSlash(input, basepath)
+      expect(result).toBe(input)
+    })
+    it('should handle strings with multiple slashes', () => {
+      const input = 'https://example.com/path/to/resource/'
+      const expectedOutput = 'https://example.com/path/to/resource'
+      const result = removeTrailingSlash(input, basepath)
+      expect(result).toBe(expectedOutput)
+    })
+  },
+)
+
+describe.each([{ basepath: '/' }, { basepath: '/app' }, { basepath: '/app/' }])(
+  'exactPathTest with basepath $basepath',
+  ({ basepath }) => {
+    it('should return true when two paths are exactly the same', () => {
+      const path1 = 'some-path/additional-path'
+      const path2 = 'some-path/additional-path'
+      const result = exactPathTest(path1, path2, basepath)
+      expect(result).toBe(true)
+    })
+    it('should return true when two paths are the same with or without trailing slash', () => {
+      const path1 = 'some-path/additional-path'
+      const path2 = 'some-path/additional-path/'
+      const result = exactPathTest(path1, path2, basepath)
+      expect(result).toBe(true)
+    })
+    it('should return true when two paths are the same with or without trailing slash 2', () => {
+      const path1 = 'some-path/additional-path'
+      const path2 = 'some-path/additional-path/'
+      const result = exactPathTest(path1, path2, basepath)
+      expect(result).toBe(true)
+    })
+    it('should return false when two paths are different', () => {
+      const path1 = 'some-path/additional-path/'
+      const path2 = 'some-path2/additional-path/'
+      const result = exactPathTest(path1, path2, basepath)
+      expect(result).toBe(false)
+    })
+    it('should return true when both paths are just a slash', () => {
+      const path1 = '/'
+      const path2 = '/'
+      const result = exactPathTest(path1, path2, basepath)
+      expect(result).toBe(true)
+    })
+  },
+)