Skip to content

fix(router): hash history links with target="_blank" generate incorrect URLs #5242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
schiller-manuel merged 2 commits into from
Sep 29, 2025
Merged

fix(router): hash history links with target="_blank" generate incorrect URLs #5242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ea543e
fix(router): correct href generation for target=_blank links in hash …
leesb971204 Sep 26, 2025
2ef5384
test(router): add tests for target=_blank links in hash history mode
leesb971204 Sep 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/react-router/src/link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,13 +120,13 @@ export function useLinkProps<
let external = false
if (router.origin) {
if (href.startsWith(router.origin)) {
href = href.replace(router.origin, '') || '/'
href = router.history.createHref(href.replace(router.origin, '')) || '/'
Copy link
Contributor Author

@leesb971204 leesb971204 Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since routes with target="_blank" don’t go through the router.navigate logic, the href needs to include #.
But the hrefOption generation logic wasn't applying router.history.createHref() transformation, which is essential for hash history to format URLs correctly (adding the # prefix).

} else {
external = true
}
}
return { href, external }
}, [disabled, next.maskedLocation, next.url, router.origin])
}, [disabled, next.maskedLocation, next.url, router.origin, router.history])

const externalLink = React.useMemo(() => {
if (hrefOption?.external) {
Expand Down
53 changes: 53 additions & 0 deletions packages/react-router/tests/link.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
Outlet,
RouterProvider,
createBrowserHistory,
createHashHistory,
createLink,
createMemoryHistory,
createRootRoute,
Expand Down Expand Up @@ -6335,3 +6336,55 @@ describe('rewrite', () => {
expect(appLink).toHaveAttribute('href', 'http://app.example.com/')
})
})

describe('hash history with target="_blank" links', () => {
test('should generate correct href for target="_blank" links in hash history mode', async () => {
const hashHistory = createHashHistory()

const rootRoute = createRootRoute()
const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
component: () => {
return (
<>
<h1>Index</h1>
<Link data-testid="posts-link" to="/posts">
Posts (same tab)
</Link>
<Link data-testid="about-blank-link" to="/about" target="_blank">
About (new tab)
</Link>
</>
)
},
})

const postsRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/posts',
component: () => <h1>Posts</h1>,
})

const aboutRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/about',
component: () => <h1>About</h1>,
})

const router = createRouter({
routeTree: rootRoute.addChildren([indexRoute, postsRoute, aboutRoute]),
history: hashHistory,
})

render(<RouterProvider router={router} />)

const postsLink = await screen.findByTestId('posts-link')
expect(postsLink).toHaveAttribute('href', '/#/posts')
expect(postsLink).not.toHaveAttribute('target', '_blank')

const postsBlankLink = await screen.findByTestId('about-blank-link')
expect(postsBlankLink).toHaveAttribute('href', '/#/about')
expect(postsBlankLink).toHaveAttribute('target', '_blank')
})
Comment on lines +6342 to +6389
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Destroy the hash history instance after the test.

createHashHistory() hooks a window hashchange listener. Unlike the other tests, this block never tears it down (the global afterEach only destroys the createBrowserHistory from beforeEach). Leaving the hash history alive can bleed listeners and state into subsequent tests. Please ensure we destroy it—either reassign history = hashHistory so the existing cleanup handles it, or explicitly call hashHistory.destroy() in a finally block here.

-    const hashHistory = createHashHistory()
+    const hashHistory = createHashHistory()
+    try {
       …
-    expect(postsBlankLink).toHaveAttribute('target', '_blank')
+    expect(postsBlankLink).toHaveAttribute('target', '_blank')
+    } finally {
+      hashHistory.destroy()
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const hashHistory = createHashHistory()
const rootRoute = createRootRoute()
const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
component: () => {
return (
<>
<h1>Index</h1>
<Link data-testid="posts-link" to="/posts">
Posts (same tab)
</Link>
<Link data-testid="about-blank-link" to="/about" target="_blank">
About (new tab)
</Link>
</>
)
},
})
const postsRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/posts',
component: () => <h1>Posts</h1>,
})
const aboutRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/about',
component: () => <h1>About</h1>,
})
const router = createRouter({
routeTree: rootRoute.addChildren([indexRoute, postsRoute, aboutRoute]),
history: hashHistory,
})
render(<RouterProvider router={router} />)
const postsLink = await screen.findByTestId('posts-link')
expect(postsLink).toHaveAttribute('href', '/#/posts')
expect(postsLink).not.toHaveAttribute('target', '_blank')
const postsBlankLink = await screen.findByTestId('about-blank-link')
expect(postsBlankLink).toHaveAttribute('href', '/#/about')
expect(postsBlankLink).toHaveAttribute('target', '_blank')
})
const hashHistory = createHashHistory()
try {
const rootRoute = createRootRoute()
const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
component: () => {
return (
<>
<h1>Index</h1>
<Link data-testid="posts-link" to="/posts">
Posts (same tab)
</Link>
<Link data-testid="about-blank-link" to="/about" target="_blank">
About (new tab)
</Link>
</>
)
},
})
const postsRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/posts',
component: () => <h1>Posts</h1>,
})
const aboutRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/about',
component: () => <h1>About</h1>,
})
const router = createRouter({
routeTree: rootRoute.addChildren([indexRoute, postsRoute, aboutRoute]),
history: hashHistory,
})
render(<RouterProvider router={router} />)
const postsLink = await screen.findByTestId('posts-link')
expect(postsLink).toHaveAttribute('href', '/#/posts')
expect(postsLink).not.toHaveAttribute('target', '_blank')
const postsBlankLink = await screen.findByTestId('about-blank-link')
expect(postsBlankLink).toHaveAttribute('href', '/#/about')
expect(postsBlankLink).toHaveAttribute('target', '_blank')
} finally {
hashHistory.destroy()
}
})
🤖 Prompt for AI Agents 
packages/react-router/tests/link.test.tsx around lines 6342 to 6389: the test
creates a hash history with createHashHistory() but never destroys it, leaking a
window hashchange listener into later tests; fix by ensuring the instance is
cleaned up—either assign the created hashHistory to the test-level history
variable so the shared afterEach cleanup will destroy it, or wrap the test logic
in try/finally and call hashHistory.destroy() in the finally block to explicitly
remove the listener.

})
2 changes: 1 addition & 1 deletion packages/solid-router/src/link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export function useLinkProps<
let external = false
if (router.origin) {
if (href.startsWith(router.origin)) {
href = href.replace(router.origin, '')
href = router.history.createHref(href.replace(router.origin, ''))
} else {
external = true
}
Expand Down
53 changes: 53 additions & 0 deletions packages/solid-router/tests/link.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
Outlet,
RouterProvider,
createBrowserHistory,
createHashHistory,
createLink,
createMemoryHistory,
createRootRoute,
Expand Down Expand Up @@ -5674,3 +5675,55 @@ describe('relative links to from route', () => {
},
)
})

describe('hash history with target="_blank" links', () => {
test('should generate correct href for target="_blank" links in hash history mode', async () => {
const hashHistory = createHashHistory()

const rootRoute = createRootRoute()
const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
component: () => {
return (
<>
<h1>Index</h1>
<Link data-testid="posts-link" to="/posts">
Posts (same tab)
</Link>
<Link data-testid="about-blank-link" to="/about" target="_blank">
About (new tab)
</Link>
</>
)
},
})

const postsRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/posts',
component: () => <h1>Posts</h1>,
})

const aboutRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/about',
component: () => <h1>About</h1>,
})

const router = createRouter({
routeTree: rootRoute.addChildren([indexRoute, postsRoute, aboutRoute]),
history: hashHistory,
})

render(() => <RouterProvider router={router} />)

const postsLink = await screen.findByTestId('posts-link')
expect(postsLink).toHaveAttribute('href', '/#/posts')
expect(postsLink).not.toHaveAttribute('target', '_blank')

const postsBlankLink = await screen.findByTestId('about-blank-link')
expect(postsBlankLink).toHaveAttribute('href', '/#/about')
expect(postsBlankLink).toHaveAttribute('target', '_blank')
})
})
Loading