Fix React router Link component in SVG elements #5590
Conversation
WalkthroughLink click handling now reads the element's Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Element as Link element (a or svg)
participant ClickHandler as Link.onClick
participant Router as Router/navigation
User->>Element: click
Element->>ClickHandler: event dispatched
ClickHandler->>Element: element.getAttribute("target")
alt target empty / same-origin / not _blank
ClickHandler->>Router: intercept -> client-side navigate
Router-->>User: SPA navigation (no full reload)
else target="_blank" or external
ClickHandler-->>User: allow default (open new tab/navigation)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-router/src/link.tsx (1)
246-248: Fix correctly addresses the SVG navigation bug.The change from
e.currentTarget.targettogetAttribute('target')properly handles both HTML and SVG anchor elements. This resolves the issue whereelement.targetreturns anSVGAnimatedStringfor SVG elements, which was preventing navigation.Optional: The type assertion can be removed.
Since
React.MouseEventdefaults toReact.MouseEvent<Element>,e.currentTargetis already typed asEventTarget & Element, andElement.prototype.getAttributeexists. The cast toHTMLAnchorElement | SVGAElementis unnecessary:- const elementTarget = ( - e.currentTarget as HTMLAnchorElement | SVGAElement - ).getAttribute('target') + const elementTarget = e.currentTarget.getAttribute('target')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-router/src/link.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/react-router/src/link.tsx
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/react-router/src/link.tsx
|
View your CI Pipeline Execution ↗ for commit 729b191
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-router-ssr-query
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
|
can you please also add an e2e test? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/react-router/basic/tests/app.spec.ts (1)
53-64: Consider simplifying to match existing test patterns.The test correctly addresses the reviewer's request and the logic is sound. However, it's inconsistent with other tests in this file:
- Manual URL construction (line 60): Other tests rely on implicit navigation without
waitForURL. Consider removing the manual URL construction and following the pattern of existing tests (e.g., lines 11-15).- Explicit URL wait: The
waitForURLwith a full URL is unnecessary if Playwright'sbaseURLis configured. You could either navigate and assert content on the/postspage, or use a relative URL:await page.waitForURL('/posts').Apply this diff to align with existing patterns:
test('Link in SVG does not trigger a full page reload', async ({ page }) => { let fullPageLoad = false page.on('domcontentloaded', () => { fullPageLoad = true }) await page.getByRole('link', { name: 'Open posts from SVG' }).click() - const url = `http://localhost:${PORT}/posts` - await page.waitForURL(url) + await page.waitForURL('/posts') expect(fullPageLoad).toBeFalsy() })e2e/react-router/basic/src/main.tsx (1)
73-88: Remove unusedidattribute from title element.The SVG structure correctly demonstrates the fix for Link components in SVG. However, the
id="rectTitle"on line 75 is unused—it's not referenced by anyaria-labelledbyor other attribute. Since the Link already has an appropriatearia-label, the id serves no purpose and should be removed to avoid confusion.Apply this diff:
- <svg width="20" height="20" viewBox="0 0 20 20" role="img"> - <title id="rectTitle">Link in SVG</title> + <svg width="20" height="20" viewBox="0 0 20 20" role="img" aria-label="Link in SVG"> + <title>Link in SVG</title> <Link to="/posts" aria-label="Open posts from SVG">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/react-router/basic/src/main.tsx(1 hunks)e2e/react-router/basic/tests/app.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-router/basic/src/main.tsxe2e/react-router/basic/tests/app.spec.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-router/basic/src/main.tsxe2e/react-router/basic/tests/app.spec.ts
🧬 Code graph analysis (1)
e2e/react-router/basic/tests/app.spec.ts (1)
scripts/set-ts-version.js (1)
packageJson(33-33)
🔇 Additional comments (1)
e2e/react-router/basic/tests/app.spec.ts (1)
2-5: LGTM! Clean test setup.The JSON import with import attributes and top-level await for PORT is well-structured and follows modern JavaScript patterns.
ReactI’ve added an e2e test for the React version to the basic example. For detecting the page reload I used the same method as in redirect.spec.ts. SolidI’ve also looked into Solid and this turned out to be more tricky than the React version, mainly because of two quirks in Solid:
As you can see in the examples, these are not specific to Tanstack router. I have prepared a fix for the original issue, but it required a bit of a workaround in the e2e test because of the mentioned issues. For now I have kept this separate so you can decide if you want to include it or not: https://github.com/dsdeur/tanstack-router/pull/1/files Just let me know, and I can include it in this PR or open a new one. |
|
@brenelz @birkskyum can you please have a look at the solid part here? |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/solid-router/src/link.tsx (1)
1-627: Remove boolean_asChildusage in e2e test and clarify component requirement.The e2e test at
e2e/solid-router/basic/src/main.tsx:77uses a bare_asChildattribute, which in JSX evaluates totrue(boolean). However, theLinkcomponent's runtime code (line 592-594 inpackages/solid-router/src/link.tsx) passeslocal._asChilddirectly to<Dynamic component={...}>, which expects a component or tag name, not a boolean. This will cause a runtime error.Additionally, the type definition at line 481 incorrectly allows
boolean | Solid.ValidComponent, but the implementation cannot handle boolean values. The type should be updated to only acceptSolid.ValidComponent(or remove the boolean union entirely).Changes needed:
- e2e/solid-router/basic/src/main.tsx:77 — Remove the bare
_asChildattribute or pass an actual component/element reference- packages/solid-router/src/link.tsx:481 — Update type definition to remove the
booleanoption from_asChild
🧹 Nitpick comments (4)
e2e/solid-router/basic/tests/app.spec.ts (2)
2-6: Avoid hard-coding origin/port in tests.Rely on a relative URL or regex to reduce flakiness and remove the PORT dependency.
Apply:
- const url = `http://localhost:${PORT}/posts` - await page.waitForURL(url) + await page.waitForURL(/\/posts$/)
53-64: Make reload detection precise and avoid races.Use once instead of on, and await URL + click concurrently.
Apply:
- let fullPageLoad = false - page.on('domcontentloaded', () => { - fullPageLoad = true - }) - - await page.getByRole('link', { name: 'Open posts from SVG' }).click() - const url = `http://localhost:${PORT}/posts` - await page.waitForURL(url) + let fullPageLoad = false + page.once('domcontentloaded', () => { + fullPageLoad = true + }) + + await Promise.all([ + page.waitForURL(/\/posts$/), + page.getByRole('link', { name: 'Open posts from SVG' }).click(), + ])packages/solid-router/src/link.tsx (2)
577-589: Expose real transitioning state to render-prop children.You’re returning
isTransitioning: false. Wire the actual signal.Apply:
- return ch({ + return ch({ get isActive() { return (linkProps as any)['data-status'] === 'active' }, - isTransitioning: false, + get isTransitioning() { + // hoist the signal from useLinkProps scope if needed + return (linkProps as any)['data-transitioning'] === 'transitioning' + }, })Or, if available in scope, pass
isTransitioning()directly.
600-604: Comment suggests SVG anchors won’t work by default; add note about_asChildusage.Clarify in the comment that
_asChild="a"inside<svg>is the supported path, and default<a>only works in HTML.Proposed:
- // Default rendering - just use regular anchor element - // This works for HTML contexts; SVG links inside SVGs won't work properly - // due to Solid's compile-time namespace determination + // Default rendering - regular anchor element (HTML context only). + // For SVG, prefer `<Link _asChild="a">...</Link>` inside `<svg>`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/solid-router/basic/src/main.tsx(1 hunks)e2e/solid-router/basic/tests/app.spec.ts(2 hunks)packages/solid-router/src/link.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/solid-router/src/link.tsxe2e/solid-router/basic/src/main.tsxe2e/solid-router/basic/tests/app.spec.ts
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/solid-router/src/link.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-router/basic/src/main.tsxe2e/solid-router/basic/tests/app.spec.ts
🔇 Additional comments (1)
packages/solid-router/src/link.tsx (1)
284-287: Good fix: read target via getAttribute for SVG.This avoids SVGAnimatedString pitfalls and matches browser behavior. LGTM.
birkskyum
force-pushed
the
fix-svg-a-link-react-component
branch
from
b451f10 to
54f23e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/react-router/basic/tests/app.spec.ts (1)
2-5: Consider using relative URL to avoid PORT import.The
PORTconstant is only used in the new test'swaitForURLcall. Since other tests use relative URLs (e.g.,page.goto('/')inbeforeEach), Playwright appears to be configured with a base URL. You can simplify by usingpage.waitForURL('/posts')instead of the absolute URL, eliminating the need for these imports.Apply this diff to use a relative URL:
-import { getTestServerPort } from '@tanstack/router-e2e-utils' -import packageJson from '../package.json' with { type: 'json' } - -const PORT = await getTestServerPort(packageJson.name) - test.beforeEach(async ({ page }) => {And update the test:
await page.getByRole('link', { name: 'Open posts from SVG' }).click() - const url = `http://localhost:${PORT}/posts` - await page.waitForURL(url) + await page.waitForURL('/posts')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/react-router/basic/src/main.tsx(1 hunks)e2e/react-router/basic/tests/app.spec.ts(2 hunks)e2e/solid-router/basic/src/main.tsx(2 hunks)e2e/solid-router/basic/tests/app.spec.ts(2 hunks)packages/react-router/src/link.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- e2e/solid-router/basic/tests/app.spec.ts
- e2e/solid-router/basic/src/main.tsx
- packages/react-router/src/link.tsx
- e2e/react-router/basic/src/main.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-router/basic/tests/app.spec.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-router/basic/tests/app.spec.ts
🔇 Additional comments (1)
e2e/react-router/basic/tests/app.spec.ts (1)
53-64: Excellent test coverage for the SVG link fix!The test correctly verifies that clicking a link inside an SVG element uses client-side routing instead of triggering a full page reload. The
domcontentloadedevent listener pattern effectively detects whether a full page reload occurred.
birkskyum
force-pushed
the
fix-svg-a-link-react-component
branch
from
54f23e6 to
efcfb61
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-router/src/link.tsx (1)
246-249: Excellent fix for the SVGAnimatedString issue!The use of
getAttribute('target')correctly handles bothHTMLAnchorElementandSVGAElement, returningnull | stringinstead of the problematicSVGAnimatedStringobject. This ensures consistent behavior for SVG-based links.The implementation is sound and maintains backward compatibility.
Optional: Add a clarifying comment.
Consider adding a brief comment explaining why
getAttributeis used instead of direct property access to help future maintainers:- // Check actual element's target attribute as fallback + // Use getAttribute to avoid SVGAnimatedString on SVGAElement.target const elementTarget = ( e.currentTarget as HTMLAnchorElement | SVGAElement ).getAttribute('target')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e/react-router/basic/src/main.tsx(1 hunks)e2e/react-router/basic/tests/app.spec.ts(2 hunks)e2e/solid-router/basic/src/main.tsx(2 hunks)e2e/solid-router/basic/tests/app.spec.ts(2 hunks)packages/react-router/src/link.tsx(1 hunks)packages/solid-router/src/link.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/react-router/basic/src/main.tsx
- e2e/react-router/basic/tests/app.spec.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/solid-router/basic/tests/app.spec.tspackages/react-router/src/link.tsxe2e/solid-router/basic/src/main.tsxpackages/solid-router/src/link.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/solid-router/basic/tests/app.spec.tse2e/solid-router/basic/src/main.tsx
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/react-router/src/link.tsxpackages/solid-router/src/link.tsx
🧬 Code graph analysis (3)
e2e/solid-router/basic/tests/app.spec.ts (1)
scripts/set-ts-version.js (1)
packageJson(33-33)
e2e/solid-router/basic/src/main.tsx (2)
packages/solid-router/src/index.tsx (2)
Link(229-229)useLinkProps(229-229)packages/solid-router/src/link.tsx (1)
useLinkProps(31-444)
packages/solid-router/src/link.tsx (1)
packages/react-router/src/link.tsx (3)
useLinkProps(40-385)Link(567-598)LinkComponent(498-509)
🔇 Additional comments (8)
packages/solid-router/src/link.tsx (4)
2-2: LGTM!The
Dynamicimport is required for the newcreateLinkimplementation and is correctly imported fromsolid-js/web.
284-286: Excellent fix for SVG link navigation.Using
getAttribute('target')correctly avoids theSVGAnimatedStringissue onSVGAElementwhile maintaining compatibility withHTMLAnchorElement. The type union properly covers both cases, and this approach aligns with the React implementation.
563-574: LGTM!The
createLinkimplementation correctly usesDynamicto support custom components. The prop splitting and type removal are handled properly.
577-603: Link simplified; SVG usage requiresuseLinkPropsworkaround.The removal of
_asChildsupport resolves the runtime errors flagged in past reviews. The plain<a>rendering is correct for HTML contexts.The comments accurately document that SVG links won't work due to Solid's compile-time namespace determination—the
<a>JSX insideLinkis compiled in the HTML namespace regardless of whereLinkis used. The e2e test demonstrates the correct workaround: useuseLinkPropsdirectly and render the<a>element in your own JSX inside the<svg>, ensuring proper SVG namespace compilation.e2e/solid-router/basic/tests/app.spec.ts (2)
2-5: LGTM!The test setup correctly uses the e2e utilities to dynamically resolve the test server port, enabling proper URL construction in the test.
53-64: Effective test coverage for the SVG link fix.The test correctly validates that clicking the SVG-embedded link navigates via the router without triggering a full page reload. The
domcontentloadedevent tracking is a reliable indicator of full page loads, and the aria-label selector properly targets the test element.e2e/solid-router/basic/src/main.tsx (2)
12-12: LGTM!The
useLinkPropsimport is required for the SVG link implementation and is correctly imported from@tanstack/solid-router.
75-96: Excellent demonstration of the SVG link workaround.This implementation correctly addresses Solid's compile-time namespace constraint by:
- Using
useLinkPropsto obtain navigation props (including the fixedonClickhandler)- Creating the
<a>element directly in your JSX inside the<svg>, ensuring Solid compiles it with the SVG namespace- Spreading the link props onto the
<a>elementThe accessibility attributes (
title,aria-label) are properly included, and this pattern serves as the recommended approach for SVG links when theLinkcomponent's HTML-namespace limitation applies.
birkskyum
force-pushed
the
fix-svg-a-link-react-component
branch
from
ee74dfc to
729b191
Compare
|
This is apparently much harder to do in solid than react - I've opened a ticket for it, but I think we can fix it for react now, we should just go ahead, and we'll add solid tests when we have a solution for it. |
4 participants
Problem
When using the React Link component in an SVG it causes a full refresh, unless the
targetprop is explicitly set.Cause
This is caused by retrieving the element target with
.target:router/packages/react-router/src/link.tsx
Line 246 in 91d9940
In an SVG,
element.targetreturns an SVGAnimatedString instead of a string, making the target check evaluate to false:router/packages/react-router/src/link.tsx
Line 253 in 91d9940
Resulting in the navigate function to not be triggered and the default link behavior to cause a full refresh.
Example/demo
https://stackblitz.com/edit/github-9fsuvtph?file=src%2Froutes%2Findex.tsx
Solution
The easiest solution seems to be to use
.getAttribute("target")instead of.targetas that returns null or the value instead of the SVGAnimatedString.(This is the same approach as Next.js link uses)
Summary by CodeRabbit
New Features
Bug Fixes
Tests