Fix: Prevent Flicker When Navigation Happens During Render #5905
Conversation
WalkthroughNavigation calls in React and Solid examples and the React Router core were changed to run inside lifecycle hooks (React: layout effect; Solid: onMount) instead of invoking navigation synchronously during render. Changes
Sequence DiagramsequenceDiagram
participant Component
participant LifecycleHook as Lifecycle Hook (useLayoutEffect/onMount)
participant Navigator as navigate(options)
participant RouterState as Router / Browser
Note over Component,RouterState: Deferred navigation via lifecycle hooks
Component->>LifecycleHook: Register effect (deps: navigate, options)
alt On layout/mount
LifecycleHook->>Navigator: invoke navigate(options)
Navigator->>RouterState: update route / URL
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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: 1
🧹 Nitpick comments (2)
packages/react-router/src/useNavigate.tsx (1)
70-71: Consider extracting to module level.The
useIsomorphicLayoutEffecthook is redefined on every render. For better performance, consider extracting it to the module level:+const useIsomorphicLayoutEffect = + typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect + export function Navigate< TRouter extends AnyRouter = RegisteredRouter, const TFrom extends string = string, const TTo extends string | undefined = undefined, const TMaskFrom extends string = TFrom, const TMaskTo extends string = '', >(props: NavigateOptions<TRouter, TFrom, TTo, TMaskFrom, TMaskTo>): null { const router = useRouter() const navigate = useNavigate() const previousPropsRef = React.useRef<NavigateOptions< TRouter, TFrom, TTo, TMaskFrom, TMaskTo > | null>(null) - const useIsomorphicLayoutEffect = - typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffectexamples/react/start-large/src/typePrimitives.tsx (1)
25-26: Consider extracting isomorphic layout effect to a shared utility.This pattern is duplicated in
packages/react-router/src/useNavigate.tsx(lines 70-71). Consider creating a shared utility function or importing from the router package to avoid duplication:// In a shared utilities file or exported from @tanstack/react-router export const useIsomorphicLayoutEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffectThen import and use it:
import * as React from 'react' -import { Link, redirect, useNavigate } from '@tanstack/react-router' +import { Link, redirect, useNavigate, useIsomorphicLayoutEffect } from '@tanstack/react-router'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
examples/react/start-large/src/typePrimitives.tsx(2 hunks)examples/solid/start-large/src/typePrimitives.tsx(2 hunks)packages/react-router/src/useNavigate.tsx(1 hunks)
🔇 Additional comments (2)
examples/react/start-large/src/typePrimitives.tsx (1)
28-30: LGTM!The isomorphic layout effect correctly ensures navigation timing across client and server environments, and the dependencies ensure navigation updates when options change.
packages/react-router/src/useNavigate.tsx (1)
73-78: Code is correct;Navigateis intentionally client-only.The isomorphic layout effect pattern is properly implemented. The
Navigatecomponent is designed for client-side rendering only—its JSDoc explicitly states "Navigation executes in an effect after mount/update." For server-side navigation during SSR, use theredirect()function from route loaders orbeforeLoadhooks, which is the framework's server-side counterpart.
| onMount(() => { | ||
| navigate(options) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify intended navigation behavior difference.
The Solid implementation navigates once on mount via onMount, while the React implementation (in examples/react/start-large/src/typePrimitives.tsx lines 28-30) re-navigates whenever options changes using useIsomorphicLayoutEffect with [navigate, options] dependencies.
This means:
- Solid: Navigation occurs once after mount, regardless of subsequent
optionschanges - React: Navigation re-executes whenever
optionsreference changes
Is this behavioral difference intentional, or should the Solid version also react to options changes using createEffect?
🏁 Script executed:
#!/bin/bash
# Search for usage patterns of useCustomNavigate to understand expected behavior
# Find all usages in Solid example
rg -n "useCustomNavigate" examples/solid/
# Find all usages in React example
rg -n "useCustomNavigate" examples/react/
# Check if there are any tests that document the expected behavior
fd -e test -e spec | xargs rg -l "useCustomNavigate|CustomNavigate" | head -10Length of output: 1029
🏁 Script executed:
#!/bin/bash
# Examine the actual implementations in both React and Solid
echo "=== Solid typePrimitives.tsx (lines 20-30) ==="
sed -n '20,30p' examples/solid/start-large/src/typePrimitives.tsx
echo -e "\n=== React typePrimitives.tsx (lines 19-30) ==="
sed -n '19,30p' examples/react/start-large/src/typePrimitives.tsx
echo -e "\n=== Solid linkProps.tsx (lines 20-35) ==="
sed -n '20,35p' examples/solid/start-large/src/routes/linkProps.tsx
echo -e "\n=== React linkProps.tsx (lines 20-35) ==="
sed -n '20,35p' examples/react/start-large/src/routes/linkProps.tsxLength of output: 1458
🏁 Script executed:
#!/bin/bash
# Check for any comments or documentation about this difference
# and look at the full context around these functions
echo "=== Full Solid typePrimitives.tsx function context (lines 15-35) ==="
sed -n '15,35p' examples/solid/start-large/src/typePrimitives.tsx
echo -e "\n=== Full React typePrimitives.tsx function context (lines 14-35) ==="
sed -n '14,35p' examples/react/start-large/src/typePrimitives.tsx
# Check if there's a comment/discussion in git history or PR
echo -e "\n=== Check for Solid createEffect imports ==="
grep -n "createEffect\|onMount" examples/solid/start-large/src/typePrimitives.tsxLength of output: 1572
🏁 Script executed:
#!/bin/bash
# Check for any comments or documentation about this difference
# and look at the full context around these functions
echo "=== Full Solid typePrimitives.tsx context with imports (lines 1-40) ==="
sed -n '1,40p' examples/solid/start-large/src/typePrimitives.tsx
echo -e "\n=== Full React typePrimitives.tsx context with imports (lines 1-40) ==="
sed -n '1,40p' examples/react/start-large/src/typePrimitives.tsx
# Check if createEffect is used elsewhere in Solid examples
echo -e "\n=== Check for createEffect usage in Solid examples ==="
rg -n "createEffect" examples/solid/ | head -20Length of output: 4531
Use createEffect with dependency tracking instead of onMount for consistency with React behavior.
The review comment accurately identifies a behavioral difference: Solid uses onMount (runs once) while React uses useIsomorphicLayoutEffect with [navigate, options] dependencies (runs when dependencies change).
In Solid, when options changes, navigation will not re-execute because onMount only runs on component initialization. This differs from React, which will re-navigate if the options reference changes. For parity with the React implementation and to follow Solid reactive patterns, use createEffect with proper dependency tracking:
const navigate = useNavigate()
createEffect(() => {
navigate(options)
})This ensures both frameworks have consistent behavior when options change.
🤖 Prompt for AI Agents
In examples/solid/start-large/src/typePrimitives.tsx around lines 26 to 28, the
component uses onMount to call navigate(options) which only runs once; change
this to use Solid's reactive createEffect that depends on the navigate/options
so navigation re-runs when options change. Replace the onMount call with a
createEffect that calls navigate(options) inside it (ensuring navigate and
options are captured) so behavior matches React's dependency-driven
re-navigation.
vsh1h
changed the title
|
Hi @birkskyum! |
|
@vsh1h , for solid it seems like the only change is in an example? so this pr will effectively only impact the react-router. Is that because the solid-router already behaves as expected, if used correctly? |
|
@birkskyum , |
|
View your CI Pipeline Execution ↗ for commit ac105f2
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-router/src/useNavigate.tsx (1)
45-47: Consider clarifying the timing in documentation.The JSDoc comment states "Navigation executes in an effect after mount/update." While accurate, it could be more precise to mention that navigation executes in a layout effect, which runs synchronously before paint. This helps developers understand the timing guarantees.
Apply this diff to enhance clarity:
/** * Component that triggers a navigation when rendered. Navigation executes - * in an effect after mount/update. + * in a layout effect after mount/update, before the browser paints. * * Props are the same as `NavigateOptions` used by `navigate()`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-router/src/useNavigate.tsx(2 hunks)
🔇 Additional comments (2)
packages/react-router/src/useNavigate.tsx (2)
2-2: LGTM: Using the utility version as requested.The import now uses
useLayoutEffectfrom the local utils module, which addresses the previous review feedback. This utility likely provides SSR-safe behavior by falling back touseEffecton the server while usinguseLayoutEffecton the client.
71-76: LGTM: Layout effect prevents flicker as intended.Switching to
useLayoutEffectensures navigation executes synchronously after DOM mutations but before the browser paints. This eliminates the brief flicker where nested<Outlet />components would render null during navigation, directly addressing the PR objective.The effect logic is correct:
- Reference equality check on
propstriggers navigation when the props object identity changespreviousPropsReftracks whether navigation has already occurred for current props- Dependencies are complete and appropriate
3 participants
This PR prevents a brief flicker that occurred when navigation happened during render, causing nested to temporarily render null. Navigation is now triggered before paint (React) or on mount (Solid), so the UI transitions smoothly without flashing.
Bug Fix
Summary by CodeRabbit