TanStack: Render real link hrefs in the Link mock - #35505
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe TanStack React Router ChangesTanStack Router Link mock
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant L as Link
participant S as StoryOnClick
participant N as onNavigate
participant R as Router
L->>S: invoke click handler with event
S-->>L: optionally prevent default
L->>N: report to and from when not prevented
L-->>R: retain current pathname
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/frameworks/tanstack-react/src/export-mocks/react-router.ts`:
- Around line 80-84: Update the Link onClick flow around _useLinkProps to
preserve and invoke the user-provided props.onClick handler before navigation.
Skip navigation when the event is defaultPrevented, and only prevent the browser
default for ordinary unmodified clicks without an explicit target, allowing
modifier-key clicks and target-based behavior to remain native.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a10f4c44-d05c-4c67-af92-26af963bf985
📒 Files selected for processing (2)
code/frameworks/tanstack-react/src/export-mocks/react-router-link.test.tsxcode/frameworks/tanstack-react/src/export-mocks/react-router.ts
Run the caller's onClick before the navigation spy and honor preventDefault, so custom click handlers fire and can cancel navigation like the real router. Add explicit afterEach(cleanup) since globals are disabled in this repo.
Add static-route href, search serialization, click-event passthrough, and repeat-click cases alongside the existing param/onClick tests.
42f48c6 to
6ae28eb
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/frameworks/tanstack-react/src/export-mocks/react-router.ts (1)
90-91: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve browser modifier keys and target behavior.
The current implementation unconditionally prevents the default browser action and spies on navigation. This breaks standard behaviors like
Cmd+Click(to open a link in a new tab) or clicks with an explicittarget="_blank".As noted in a previous review, allow the browser to handle modifier keys and non-primary clicks natively by conditionally calling
preventDefault()andonNavigate().🛠️ Proposed fix
- e.preventDefault(); - onNavigate({ to, from: location.href }); + if ( + e.button === 0 && + !e.ctrlKey && + !e.metaKey && + !e.altKey && + !e.shiftKey && + (!props.target || props.target === '_self') + ) { + e.preventDefault(); + onNavigate({ to, from: location.href }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/frameworks/tanstack-react/src/export-mocks/react-router.ts` around lines 90 - 91, Update the click handling around onNavigate to intercept only unmodified primary-button clicks without an explicit non-default target; allow modifier-key clicks, non-primary clicks, and target="_blank" navigation to proceed natively. Call preventDefault() and onNavigate({ to, from: location.href }) only for eligible navigations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@code/frameworks/tanstack-react/src/export-mocks/react-router.ts`:
- Around line 90-91: Update the click handling around onNavigate to intercept
only unmodified primary-button clicks without an explicit non-default target;
allow modifier-key clicks, non-primary clicks, and target="_blank" navigation to
proceed natively. Call preventDefault() and onNavigate({ to, from: location.href
}) only for eligible navigations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c840c0c4-f6e7-47c9-94ee-fd2b6bfc2b94
📒 Files selected for processing (2)
code/frameworks/tanstack-react/src/export-mocks/react-router-link.test.tsxcode/frameworks/tanstack-react/src/export-mocks/react-router.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/frameworks/tanstack-react/src/export-mocks/react-router-link.test.tsx
Co-authored-by: Julien Huang <julien.h.dev@gmail.com>
Drop the story onClick passthrough per review; the mock only spies navigation. Remove the related tests and trim comments.
|
Confirming this also affects <Link to="/catalog" viewTransition>
Catalog
</Link>With The proposed |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 11 | 13 | 🚨 +2 🚨 |
| Self size | 106 KB | 191 KB | 🚨 +85 KB 🚨 |
| Dependency size | 2.74 MB | 2.89 MB | 🚨 +145 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook-addon-pseudo-states
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 0 | 0 | 0 |
| Self size | 24 KB | 21 KB | 🎉 -3 KB 🎉 |
| Dependency size | 689 B | 689 B | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/addon-vitest
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 467 KB | 429 KB | 🎉 -37 KB 🎉 |
| Dependency size | 350 KB | 350 KB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/builder-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 186 | 186 | 0 |
| Self size | 92 KB | 79 KB | 🎉 -13 KB 🎉 |
| Dependency size | 35.81 MB | 35.81 MB | 🚨 +102 B 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 73 | 73 | 0 |
| Self size | 21.85 MB | 21.48 MB | 🎉 -364 KB 🎉 |
| Dependency size | 30.98 MB | 30.98 MB | 🚨 +692 B 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 185 | 0 |
| Self size | 266 KB | 255 KB | 🎉 -11 KB 🎉 |
| Dependency size | 30.19 MB | 30.18 MB | 🎉 -13 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 20 | 20 | 0 |
| Self size | 23.12 MB | 23.06 MB | 🎉 -52 KB 🎉 |
| Dependency size | 11.49 MB | 11.49 MB | 🎉 -909 B 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 185 | 0 |
| Self size | 13 KB | 13 KB | 🚨 +18 B 🚨 |
| Dependency size | 31.17 MB | 31.16 MB | 🎉 -13 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 540 | 531 | 🎉 -9 🎉 |
| Self size | 641 KB | 641 KB | 🎉 -149 B 🎉 |
| Dependency size | 63.07 MB | 62.50 MB | 🎉 -571 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 101 | 90 | 🎉 -11 🎉 |
| Self size | 1.42 MB | 1.42 MB | 🎉 -971 B 🎉 |
| Dependency size | 23.31 MB | 23.11 MB | 🎉 -195 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 272 | 272 | 0 |
| Self size | 23 KB | 23 KB | 0 B |
| Dependency size | 48.26 MB | 48.24 MB | 🎉 -18 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 198 | 0 |
| Self size | 15 KB | 15 KB | 0 B |
| Dependency size | 37.09 MB | 37.08 MB | 🎉 -13 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/tanstack-react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 80 | 80 | 0 |
| Self size | 132 KB | 118 KB | 🎉 -14 KB 🎉 |
| Dependency size | 20.46 MB | 20.45 MB | 🎉 -7 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 205 | 205 | 0 |
| Self size | 852 KB | 833 KB | 🎉 -19 KB 🎉 |
| Dependency size | 86.49 MB | 86.12 MB | 🎉 -374 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 198 | 0 |
| Self size | 44 KB | 32 KB | 🎉 -12 KB 🎉 |
| Dependency size | 84.96 MB | 84.59 MB | 🎉 -364 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 74 | 74 | 0 |
| Self size | 1.09 MB | 1.09 MB | 🚨 +1 KB 🚨 |
| Dependency size | 52.83 MB | 52.47 MB | 🎉 -364 KB 🎉 |
| Bundle Size Analyzer | node | node |
@storybook/mcp
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 11 | 11 | 0 |
| Self size | 147 KB | 107 KB | 🎉 -39 KB 🎉 |
| Dependency size | 2.74 MB | 2.74 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
@storybook/vue3
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 90 | 91 | 🚨 +1 🚨 |
| Self size | 142 KB | 102 KB | 🎉 -39 KB 🎉 |
| Dependency size | 18.10 MB | 18.14 MB | 🚨 +41 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
Follows #3550
What I did
The
Linkmock renderedhref={to}verbatim and spread every prop onto the anchor element, soproduced
The href was never interpolated and router-only props leaked onto the DOM.
The mock now derives the anchor's props from the real
useLinkProps, sohrefis exactly what the app would render. The path params are interpolated, search is serialized, and router-only props are consumed by the router instead of reaching the DOM.Navigation stays replaced by the
onNavigatespy, clicking still does not navigate.Manual testing
yarn vitest run code/frameworks/tanstack-react(57 tests, three newLink tests: param interpolation, splat interpolation, navigation spy).
LinkHrefsstory fails without thisfix and passes with it.
AI disclosure
Root-cause analysis, fix, and tests developed with Claude (Claude Code), reviewed and submitted by me.
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Linkrendering so path parameters, splat segments, and search queries are correctly reflected in the generatedhref.onClickruns first, and router navigation spying is suppressed when the click prevents default.Tests
Linkmock test coverage to verifyhref/query serialization and correct click + navigation behavior across dynamic routes, splats, and search params.