refactor: react link ssr perf #6482
Conversation
📝 WalkthroughWalkthroughAdds an SSR early-return branch in Changes
Sequence Diagram(s)sequenceDiagram
participant Server as Server (SSR)
participant Router as useLinkProps
participant History as RouterState/History
participant Client as Client (Hydration)
Server->>Router: call useLinkProps (SSR)
Router->>History: read current location (no subscription)
History-->>Router: return location
Router->>Router: run getHrefOption, isSafeInternal, compute isActive/external
Router-->Server: return static anchor props (no handlers)
Client->>Router: call useLinkProps (client/hydration)
Router->>History: subscribe to router state
History-->>Router: updates (location, transitioning)
Router->>Router: reuse SSR decisions, recompute client-only props
alt isHydrated
Router->>Client: include transition props, preload, intersection observer
else not hydrated
Router->>Client: return stable props without transition handlers
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit fe11b45
☁️ Nx Cloud last updated this comment at |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-router/src/link.tsx (1)
496-499: Potential flash of inactive state whenincludeHash: true.When
activeOptions.includeHashistrue, the check at line 497 returnsfalseuntil hydration completes (isHydrated && ...). This means links with hash-based active state will briefly appear inactive during the hydration window, potentially causing a visual flash.Consider returning
trueon server/pre-hydration for hash matches to maintain consistency, or document this as expected behavior.
🧹 Nitpick comments (3)
packages/react-router/src/link.tsx (3)
109-115: Minor inconsistency between server and clientisSafeInternalchecks.The server version considers relative paths starting with
.(e.g.,./,../) as safe internal, while the client version (lines 435-438) only checks for absolute paths starting with/. Although this doesn't cause functional issues (relative paths throw innew URL()anyway), the divergence makes the code harder to maintain.Consider aligning the logic for consistency:
♻️ Suggested alignment
const isSafeInternal = typeof to === 'string' && - (to.charCodeAt(0) === 47 - ? // '/' - to.charCodeAt(1) !== 47 // but not '//' - : // '.', '..', './', '../' - to.charCodeAt(0) === 46) + to.charCodeAt(0) === 47 && // '/' + to.charCodeAt(1) !== 47 // but not '//'
195-207: Potentially redundant external URL check.The code at lines 119-156 already handles absolute URLs with
new URL(to)parsing and returns early. The check at lines 196-206 appears to duplicate this logic. If the earlier block returns for valid absolute URLs, this code path may be unreachable.Consider consolidating or adding a comment explaining when this branch is reached (e.g., specific edge cases where the first check doesn't return).
248-270: Server search comparison is more defensive than client version.The server-side search comparison (lines 250-269) includes explicit empty object checks before calling
deepEqual, while the client version (lines 486-494) delegates directly todeepEqual. While functionally correct, this creates maintenance overhead as changes need to be synchronized.Consider extracting a shared helper function for search comparison logic if this pattern is stable.
There was a problem hiding this comment.
should this (can this) be applied to solid and vue too?
There was a problem hiding this comment.
I think so yes, needs @birkskyum probably
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react-router/src/link.tsx (1)
365-583: Biome'suseHookAtTopLevelrule will flag hooks after the early return.Biome's
correctness/useHookAtTopLevelrule is enabled by default and will report an error for the hooks placed after theif (_isServer)early return, even though the pattern is safe due to compile-time dead code elimination. Addbiome-ignore lint/correctness/useHookAtTopLevelsuppressions to each hook call:🔧 Biome suppression for hook sites
- // eslint-disable-next-line react-hooks/rules-of-hooks - const isHydrated = useHydrated() + // eslint-disable-next-line react-hooks/rules-of-hooks + // biome-ignore lint/correctness/useHookAtTopLevel + const isHydrated = useHydrated()Alternatively, disable the rule in
biome.json:{ "linter": { "rules": { "correctness": { "useHookAtTopLevel": "off" } } } }
🤖 Fix all issues with AI agents
In `@packages/react-router/src/link.tsx`:
- Around line 245-271: The current active-check always returns true at the end,
ignoring includeHash on SSR; update the closure in Link (the active computation
that references activeOptions) to perform a hash comparison similar to the
search logic: when activeOptions?.includeHash (default true) is set, if running
on the server (typeof window === 'undefined') then treat a non-empty next.hash
as non-active (return false) to avoid marking links active when the client hash
may differ; on the client compare currentLocation.hash and next.hash (and use
deepEqual/strict equality as appropriate) to determine activeness, mirroring the
search comparison behavior and preserving exact/explicitUndefined semantics.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/react-router/src/link.tsx`:
- Around line 376-381: Add Biome suppression comments alongside the existing
ESLint disables for the conditional hooks so Biome won't flag them;
specifically, for the useHydrated call that assigns isHydrated and the
useRouterState call that assigns currentSearch, keep the existing "//
eslint-disable-next-line react-hooks/rules-of-hooks" and add the corresponding
Biome ignore (e.g., a "// biome-ignore lint: react-hooks/rules-of-hooks" or the
project's Biome lint suppression syntax) immediately above each hook use so both
linters are suppressed while preserving the current comments and behavior.
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| const isHydrated = useHydrated() | ||
|
|
||
| // subscribe to search params to re-build location if it changes | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| const currentSearch = useRouterState({ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file at the specified lines with context
cat -n packages/react-router/src/link.tsx | sed -n '365,390p'Repository: TanStack/router
Length of output: 1373
🏁 Script executed:
# Check if there's an early return before line 376
cat -n packages/react-router/src/link.tsx | sed -n '1,381p' | tail -100Repository: TanStack/router
Length of output: 3910
🏁 Script executed:
# Check for package.json to see if eslint-plugin-react-hooks is installed
cat package.json | grep -A 30 "dependencies\|devDependencies"Repository: TanStack/router
Length of output: 1095
🏁 Script executed:
# Look for eslint and biome configuration
find . -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" -o -name "biome.json" -o -name "biome.jsonc" \) 2>/dev/nullRepository: TanStack/router
Length of output: 90
🏁 Script executed:
# Find the early return before line 376
cat -n packages/react-router/src/link.tsx | sed -n '350,375p'Repository: TanStack/router
Length of output: 1424
🏁 Script executed:
# Check the eslint config to see if react-hooks is properly configured
cat eslint.config.js | head -100Repository: TanStack/router
Length of output: 770
🏁 Script executed:
# Check biome.json configuration
cat biome.jsonRepository: TanStack/router
Length of output: 1833
🏁 Script executed:
# Search for the isServer check and early return before line 363
cat -n packages/react-router/src/link.tsx | sed -n '200,365p' | grep -A 5 -B 5 "isServer"Repository: TanStack/router
Length of output: 292
🏁 Script executed:
# Check the tanstack config to see what ESLint rules are extended
find . -path ./node_modules -prune -o -type f -name "eslint*" -print | head -20Repository: TanStack/router
Length of output: 970
🏁 Script executed:
# Look for tanstack config in node_modules or elsewhere
find . -maxdepth 3 -type f -name "package.json" | xargs grep -l "@tanstack/config" 2>/dev/null | head -3Repository: TanStack/router
Length of output: 73
🏁 Script executed:
# Look for the isServer check and early return statement
cat -n packages/react-router/src/link.tsx | grep -B 5 -A 10 "isServer" | head -40Repository: TanStack/router
Length of output: 1432
🏁 Script executed:
# Check the tanstack config eslint package
cat package.json | grep -A 5 '"@tanstack/config"'Repository: TanStack/router
Length of output: 268
🏁 Script executed:
# Look at the react-router eslint config
cat packages/react-router/eslint.config.tsRepository: TanStack/router
Length of output: 786
Add Biome suppression for conditional hooks.
Biome flags hooks after the early return at line 108. The eslint-plugin-react-hooks rule is already properly installed and configured, so the ESLint disable comments are valid. Add a Biome-specific ignore to suppress the lint error while preserving the existing ESLint directive.
🔧 Add Biome suppression (apply to all affected hooks)
+ // biome-ignore lint/correctness/useHookAtTopLevel: server branch is tree-shaken
// eslint-disable-next-line react-hooks/rules-of-hooks
const isHydrated = useHydrated()📝 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.
| // eslint-disable-next-line react-hooks/rules-of-hooks | |
| const isHydrated = useHydrated() | |
| // subscribe to search params to re-build location if it changes | |
| // eslint-disable-next-line react-hooks/rules-of-hooks | |
| const currentSearch = useRouterState({ | |
| // biome-ignore lint/correctness/useHookAtTopLevel: server branch is tree-shaken | |
| // eslint-disable-next-line react-hooks/rules-of-hooks | |
| const isHydrated = useHydrated() | |
| // subscribe to search params to re-build location if it changes | |
| // biome-ignore lint/correctness/useHookAtTopLevel: server branch is tree-shaken | |
| // eslint-disable-next-line react-hooks/rules-of-hooks | |
| const currentSearch = useRouterState({ |
🧰 Tools
🪛 Biome (2.1.2)
[error] 377-377: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 381-381: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 ESLint
[error] 376-376: Definition for rule 'react-hooks/rules-of-hooks' was not found.
(react-hooks/rules-of-hooks)
[error] 380-380: Definition for rule 'react-hooks/rules-of-hooks' was not found.
(react-hooks/rules-of-hooks)
🤖 Prompt for AI Agents
In `@packages/react-router/src/link.tsx` around lines 376 - 381, Add Biome
suppression comments alongside the existing ESLint disables for the conditional
hooks so Biome won't flag them; specifically, for the useHydrated call that
assigns isHydrated and the useRouterState call that assigns currentSearch, keep
the existing "// eslint-disable-next-line react-hooks/rules-of-hooks" and add
the corresponding Biome ignore (e.g., a "// biome-ignore lint:
react-hooks/rules-of-hooks" or the project's Biome lint suppression syntax)
immediately above each hook use so both linters are suppressed while preserving
the current comments and behavior.
schiller-manuel
force-pushed
the
react-link-perf
branch
from
9db966c to
fe11b45
Compare
Summary by CodeRabbit
Bug Fixes
Improvements
Note
✏️ Tip: You can customize this high-level summary in your review settings.