fix: dont use script tag for OnRendered#7054
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefactors OnRendered from a DOM script-sentinel to a hook-based emission via useLayoutEffect, adds a resetKey prop to control emission timing, adds a server-side guard, updates tests to remove an empty SSR script, and updates a comment in the Solid router variant. (43 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 2021b7f
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 3 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-router/src/Match.tsx`:
- Around line 221-243: The server check and href-based dedupe are wrong: move
the isServer check inside the existing useLayoutEffect so the hook is called
unconditionally, and replace the href comparison with a dedupe based on the
location key (router.latestLocation.state.__TSR_key) so new history entries with
the same href still emit; specifically, keep the
useLayoutEffect([...router.latestLocation.state.__TSR_key, resetKey, router])
but change prevHrefRef to a prevKeyRef (or compare prevHrefRef.current against
router.latestLocation.state.__TSR_key) and only return early from the effect
when isServer is true, otherwise call router.emit({ type: 'onRendered',
...getLocationChangeInfo(router.stores.location.state,
router.stores.resolvedLocation.state) }) and update the prevKeyRef to the
current __TSR_key.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d91d736-fff0-4a86-b3e3-fdf744ec664e
📒 Files selected for processing (4)
.changeset/calm-suns-thank.mdpackages/react-router/src/Match.tsxpackages/react-router/tests/Scripts.test.tsxpackages/solid-router/src/Match.tsx
| if (isServer ?? router.isServer) { | ||
| return null | ||
| } | ||
|
|
||
| return ( | ||
| <script | ||
| key={router.latestLocation.state.__TSR_key} | ||
| suppressHydrationWarning | ||
| ref={(el) => { | ||
| if ( | ||
| el && | ||
| (prevLocationRef.current === undefined || | ||
| prevLocationRef.current.href !== router.latestLocation.href) | ||
| ) { | ||
| router.emit({ | ||
| type: 'onRendered', | ||
| ...getLocationChangeInfo( | ||
| router.stores.location.state, | ||
| router.stores.resolvedLocation.state, | ||
| ), | ||
| }) | ||
| prevLocationRef.current = router.latestLocation | ||
| } | ||
| }} | ||
| /> | ||
| ) | ||
| const prevHrefRef = React.useRef<string | undefined>(undefined) | ||
|
|
||
| useLayoutEffect(() => { | ||
| const currentHref = router.latestLocation.href | ||
|
|
||
| if ( | ||
| prevHrefRef.current === undefined || | ||
| prevHrefRef.current !== currentHref | ||
| ) { | ||
| router.emit({ | ||
| type: 'onRendered', | ||
| ...getLocationChangeInfo( | ||
| router.stores.location.state, | ||
| router.stores.resolvedLocation.state, | ||
| ), | ||
| }) | ||
| prevHrefRef.current = currentHref | ||
| } | ||
| }, [router.latestLocation.state.__TSR_key, resetKey, router]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that OnRendered currently has both:
# 1) conditional return before hook call
# 2) dedupe by href while effect depends on __TSR_key
rg -n -C4 'function OnRendered|if \(isServer \?\? router\.isServer\)|useLayoutEffect\(|prevHrefRef|latestLocation\.href|__TSR_key' packages/react-router/src/Match.tsxRepository: TanStack/router
Length of output: 2601
🏁 Script executed:
# Search for __TSR_key definition and usage to understand state changes
rg -n '__TSR_key' packages/react-router/src/ -A 2 -B 2 | head -60Repository: TanStack/router
Length of output: 383
🏁 Script executed:
# Check the router's latestLocation and state structure
rg -n 'latestLocation' packages/react-router/src/Match.tsx -B 2 -A 2Repository: TanStack/router
Length of output: 314
🏁 Script executed:
# Look for tests of the OnRendered component
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts | xargs rg -l 'OnRendered' 2>/dev/null | head -5Repository: TanStack/router
Length of output: 188
🏁 Script executed:
# Verify the hooks rule concern by checking if there are any linter configs or comments
rg -n 'react-hooks' packages/react-router/ -t ts -t tsx | head -20Repository: TanStack/router
Length of output: 86
🏁 Script executed:
# Search for __TSR_key definition in router core
rg -n '__TSR_key\s*[=:]' packages/ -t ts -t tsx -B 3 -A 3 | head -80Repository: TanStack/router
Length of output: 86
🏁 Script executed:
# Look for tests related to onRendered
rg -n 'onRendered' packages/react-router/tests/ -B 2 -A 5 | head -100Repository: TanStack/router
Length of output: 1307
🏁 Script executed:
# Check router state definition and latestLocation type
rg -n 'latestLocation.*=' packages/react-router/src/ -t ts -A 2 | head -40Repository: TanStack/router
Length of output: 307
🏁 Script executed:
# Search for comments or context around the OnRendered function
sed -n '200,250p' packages/react-router/src/Match.tsxRepository: TanStack/router
Length of output: 1497
🏁 Script executed:
# Search for __TSR_key definition with simpler pattern
rg '__TSR_key' packages/router-core/src/ -A 3 -B 1 | head -100Repository: TanStack/router
Length of output: 2213
🏁 Script executed:
# Search in all files for __TSR_key definition
rg '__TSR_key\s*=' packages/ -A 2 -B 2 | head -80Repository: TanStack/router
Length of output: 519
🏁 Script executed:
# Check the location state type definition
rg -n 'state:.*__TSR_key|__TSR_key.*:' packages/ -B 3 -A 1 | head -60Repository: TanStack/router
Length of output: 2710
🏁 Script executed:
# Look for any related issues or comments about href vs __TSR_key
rg -n 'href.*__TSR_key|__TSR_key.*href' packages/ -B 2 -A 2Repository: TanStack/router
Length of output: 445
Move server check inside the effect and dedupe by __TSR_key instead of href.
The onRendered event fails to emit when navigating to the same URL as a new history entry (different __TSR_key). The dependency array correctly tracks __TSR_key, but the dedupe logic compares href, suppressing the emit when URLs match despite the history entry being new. Additionally, the conditional return before useLayoutEffect violates React Hooks rules (hooks must be called unconditionally on every render).
Suggested fix
function OnRendered({ resetKey }: { resetKey: number }) {
const router = useRouter()
-
- if (isServer ?? router.isServer) {
- return null
- }
-
- const prevHrefRef = React.useRef<string | undefined>(undefined)
+ const prevRenderKeyRef = React.useRef<string | undefined>(undefined)
useLayoutEffect(() => {
- const currentHref = router.latestLocation.href
-
- if (
- prevHrefRef.current === undefined ||
- prevHrefRef.current !== currentHref
- ) {
+ if (isServer ?? router.isServer) return
+ const currentRenderKey = router.latestLocation.state.__TSR_key
+ if (prevRenderKeyRef.current !== currentRenderKey) {
router.emit({
type: 'onRendered',
...getLocationChangeInfo(
router.stores.location.state,
router.stores.resolvedLocation.state,
),
})
- prevHrefRef.current = currentHref
+ prevRenderKeyRef.current = currentRenderKey
}
}, [router.latestLocation.state.__TSR_key, resetKey, router])
return null
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-router/src/Match.tsx` around lines 221 - 243, The server check
and href-based dedupe are wrong: move the isServer check inside the existing
useLayoutEffect so the hook is called unconditionally, and replace the href
comparison with a dedupe based on the location key
(router.latestLocation.state.__TSR_key) so new history entries with the same
href still emit; specifically, keep the
useLayoutEffect([...router.latestLocation.state.__TSR_key, resetKey, router])
but change prevHrefRef to a prevKeyRef (or compare prevHrefRef.current against
router.latestLocation.state.__TSR_key) and only return early from the effect
when isServer is true, otherwise call router.emit({ type: 'onRendered',
...getLocationChangeInfo(router.stores.location.state,
router.stores.resolvedLocation.state) }) and update the prevKeyRef to the
current __TSR_key.
There was a problem hiding this comment.
what was the behavior before when it comes to same url?
this does not violate rules of hooks as the condition is constant
There was a problem hiding this comment.
@schiller-manuel, you're right on both points — my review comment was mistaken.
Same-URL behavior before: The old <script> approach also used an href guard — even though the key={__TSR_key} change caused React to remount the element and fire the ref callback, the prevLocationRef.current.href !== router.latestLocation.href check would still suppress the onRendered emit when the href was the same. So the new code preserves the exact same behavior: same-URL navigation (even with a different __TSR_key) does not fire onRendered again, same as before.
Rules of Hooks: You're correct — isServer is a module-level constant and router.isServer is set once at router construction and never mutates. The expression isServer ?? router.isServer evaluates to the same value on every render, so the early return path never changes between renders and does not violate the Rules of Hooks in practice.
I apologize for the noise.
(´･ω･`)
🧠 Learnings used
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Learnt from: nlynzaad
Repo: TanStack/router PR: 6389
File: e2e/solid-start/basic/src/routes/specialChars/malformed/route.tsx:11-24
Timestamp: 2026-01-18T17:36:24.388Z
Learning: In TanStack Router e2e malformed test routes (e.g., `e2e/solid-start/basic/src/routes/specialChars/malformed/route.tsx`), intentional use of `href` prop instead of `to` prop on Link components may be used for testing purposes to validate specific encoding/decoding behaviors or edge cases, even though the standard `tanstack/solid-router` Link API uses the `to` prop.
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Merging this PR will improve performance by 5.04%
Performance Changes
Comparing 
Footnotes |
schiller-manuel
force-pushed
the
no-script-onrendered
branch
from
83aac8a to
d34a94e
Compare
1 participant
Summary by CodeRabbit