fix(router): respect target attribute on inner component#5115
Merged
schiller-manuel merged 5 commits intoTanStack:mainfrom Sep 18, 2025
Merged
Conversation
Signed-off-by: leesb971204 <leesb971204@gmail.com>
Signed-off-by: leesb971204 <leesb971204@gmail.com>
Contributor
WalkthroughUpdates modify Link click interception in React and Solid routers to compute an effective target from either the prop or the rendered anchor’s target attribute. Clicks with effective target not set or set to "_self" are intercepted; others (e.g., "_blank") follow browser default. Tests added to validate custom createLink components. No API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant L as Link Component
participant A as <a> Element
participant R as Router
participant B as Browser
U->>L: Click
L->>A: Read DOM target (elementTarget)
L->>L: effectiveTarget = propTarget ?? elementTarget
alt Intercept (effectiveTarget absent or "_self")
L->>U: preventDefault()
L->>R: navigate(to)
R-->>L: navigation complete
else Let browser handle (e.g., "_blank")
L-->>U: no preventDefault
L->>B: default navigation
Note over B: Opens per target (e.g., new tab)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit abf2149
☁️ Nx Cloud last updated this comment at |
leesb971204
commented
Sep 10, 2025
| !disabled && | ||
| !isCtrlEvent(e) && | ||
| !e.defaultPrevented && | ||
| (!target || target === '_self') && |
Contributor
Author
There was a problem hiding this comment.
If the target prop is undefined, the condition treats it as _self, causing the router to intercept the click and prevent target="_blank" links from opening in a new tab.
Comment on lines
+221
to
+224
| // Check actual element's target attribute as fallback | ||
| const elementTarget = (e.currentTarget as HTMLAnchorElement).target | ||
| const effectiveTarget = target !== undefined ? target : elementTarget | ||
|
|
Contributor
Author
There was a problem hiding this comment.
If no target prop is provided, check the rendered element’s target attribute to ensure correct behavior.
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-plugin
@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-start
@tanstack/solid-start-client
@tanstack/solid-start-plugin
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-server-functions-client
@tanstack/start-server-functions-fetcher
@tanstack/start-server-functions-server
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react-router/src/link.tsx (1)
221-224: Make target handling robust (case-insensitive + generic Element) and don’t rely on anchor-only property.Reading
.targetassumes an anchor and is case-sensitive for_self. UsinggetAttribute('target')and normalizing improves resilience (e.g.,_SELF) and works with any element rendered viacreateLink.Apply within this hunk:
- // Check actual element's target attribute as fallback - const elementTarget = (e.currentTarget as HTMLAnchorElement).target - const effectiveTarget = target !== undefined ? target : elementTarget + // Check actual element's target attribute as fallback + const rawTarget = + (e.currentTarget as Element).getAttribute?.('target') ?? undefined + const effectiveTarget = target !== undefined ? target : rawTarget + const keywordTarget = + typeof effectiveTarget === 'string' + ? effectiveTarget.toLowerCase() + : undefined @@ - (!effectiveTarget || effectiveTarget === '_self') && + (!keywordTarget || keywordTarget === '_self') &&Additionally, to avoid clobbering a child component’s default
targetwhen the prop is undefined, consider returningtargetconditionally in the props spread later in this file:// ...inside the return object ...(target !== undefined && { target }),Also applies to: 229-229
packages/react-router/tests/link.test.tsx (1)
4686-4746: Remove unused window.open mock or assert on it.The test doesn’t assert
openMockcalls, so the manual override adds noise and potential flakiness. Either drop it or usevi.spyOn(window, 'open')and assert.Minimal cleanup within this hunk:
- const originalOpen = window.open - const openMock = vi.fn() - window.open = openMock @@ - window.open = originalOpenIf you prefer to assert:
+ const openSpy = vi.spyOn(window, 'open').mockImplementation(() => null as any) @@ + await waitFor(() => expect(openSpy).toHaveBeenCalled()) + openSpy.mockRestore()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-router/src/link.tsx(1 hunks)packages/react-router/tests/link.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-router/tests/link.test.tsx (2)
packages/react-router/src/index.tsx (6)
createLink(151-151)createRootRoute(262-262)createRoute(259-259)createRouter(277-277)createMemoryHistory(121-121)RouterProvider(287-287)packages/react-router/src/router.ts (1)
createRouter(80-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/react-router/tests/link.test.tsx (1)
4748-4796: LGTM: override semantics validated.This cleanly proves
targetprop overrides a custom component’s default, and the router intercepts navigation when_selfis effective.
Contributor
|
other than making use of test-ids for the tests this looks good to me |
Signed-off-by: leesb971204 <leesb971204@gmail.com>
Contributor
Author
Thank you for the feedback. |
Contributor
|
one last thing please apply the same tests and update to solid-router |
Signed-off-by: leesb971204 <leesb971204@gmail.com>
Signed-off-by: leesb971204 <leesb971204@gmail.com>
leesb971204
changed the title
target attribute on inner componenttarget attribute on inner component
Contributor
Author
I’ve added the related logic and tests to solid-router. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/solid-router/src/link.tsx (1)
419-419: Style gating likely never appliesObject.keys is called on the accessor function, not its value, so the style prop is never added. Prefer checking keys of resolvedStyle() while still passing the accessor.
Apply this diff:
- ...(Object.keys(resolvedStyle).length && { style: resolvedStyle }), + ...(Object.keys(resolvedStyle()).length && { style: resolvedStyle }),packages/solid-router/tests/link.test.tsx (1)
4249-4307: Trim unused window.open stubThe test doesn’t assert on window.open; the stub can be removed to reduce globals churn.
- const originalOpen = window.open - const openMock = vi.fn() - window.open = openMock ... - window.open = originalOpen
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/solid-router/src/link.tsx(1 hunks)packages/solid-router/tests/link.test.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (2)
packages/solid-router/src/link.tsx (1)
267-271: Target fallback logic LGTMDeriving effectiveTarget from either prop or element attribute and gating interception on it fixes the custom link/new‑tab case.
Also applies to: 276-276
packages/solid-router/tests/link.test.tsx (1)
4310-4360: Override behavior test LGTMConfirms that an explicit target prop overrides a component’s default.
naoya7076
pushed a commit
to naoya7076/router
that referenced
this pull request
Feb 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #5011
Summary by CodeRabbit