fix(tanstack-router): dedupe loadRouteChunk under React 18 StrictMode (#3405)#3410
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR prevents React 18 StrictMode remounts from re-running TanStack Router render-phase hydration work by caching per-router preload and hydration promises plus SSR-flag ownership in a module-level WeakMap and reusing them on subsequent mounts. ChangesStrictMode Hydration Deduplication
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/react-on-rails-pro/src/tanstack-router/clientHydrate.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/react-on-rails-pro/tests/tanstackRouter.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b588414b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
/run-skipped-ci |
Greptile SummaryThis PR fixes a React 18 StrictMode bug where
Confidence Score: 4/5Safe to merge; the WeakMap deduplication correctly targets the React 18 StrictMode double-render and leaves the production (fresh-router) path untouched. The core fix is well-reasoned and the test matrix covers both React 18 and 19. The one edge case worth watching is that the WeakMap entry stores Only Important Files Changed
Sequence DiagramsequenceDiagram
participant R18 as React 18 StrictMode
participant Comp as TanStackHydrationApp
participant WM as WeakMap (sharedHydrationInitStates)
participant Router as TanStackRouter
Note over R18,Router: Render pass 1 (fresh hooks, routerRef.current = null)
R18->>Comp: render (pass 1)
Comp->>Router: options.createRouter()
Comp->>WM: get(router) → miss
Comp->>Router: "router.update({ history })"
Comp->>Router: __store.setState(matches)
Comp->>Router: "router.ssr = { manifest: undefined }"
Comp->>WM: "set(router, { preloadPromise, hydrationPromise, didSetSsrFlag: true })"
Note over R18,Router: Render pass 2 (fresh hooks, routerRef.current = null again)
R18->>Comp: render (pass 2)
Comp->>Router: options.createRouter() → same instance
Comp->>WM: get(router) → HIT
Comp-->>Comp: restore refs from cache (skip re-init)
Note over R18,Router: Mount — effects fire once
R18->>Comp: useEffect fires
Comp->>Router: await routeChunkPreloadPromise
Comp->>Router: router.load()
Comp->>Router: "router.ssr = undefined (clear flag)"
Note over R18,Router: StrictMode simulated unmount/remount
R18->>Comp: "cleanup (cancelled=true, didTriggerPostHydrationLoad=false)"
R18->>Comp: re-render (routerRef.current preserved → render-phase init skipped)
R18->>Comp: useEffect fires again
Comp->>Router: router.load() (second call)
Reviews (1): Last reviewed commit: "fix(tanstack-router): dedupe render-phas..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| sharedHydrationInitStates.set(router, { | ||
| routeChunkPreloadPromise: routeChunkPreloadPromiseRef.current, | ||
| hydrationCallbackPromise: hydrationCallbackPromiseRef.current, | ||
| didSetSsrFlag: didSetSsrFlagRef.current, | ||
| }); |
There was a problem hiding this comment.
didSetSsrFlag in WeakMap after first successful mount
The WeakMap entry persists with didSetSsrFlag: true after the first mount's finally block clears router.ssr and sets didSetSsrFlagRef.current = false. In a "cached router, full unmount → remount" scenario (e.g. test suites that cache a router and re-render after cleanup), a second mount hits the WeakMap, restores didSetSsrFlagRef.current = true, then fires the dev sanity check at line 361: didSetSsrFlagRef.current && router.ssr == null — triggering the "unexpectedly cleared" warning even though router.ssr was correctly cleared by the previous mount's effect, not by a TanStack Router version change.
A minimal mitigation would be to delete the WeakMap entry in the effect's cleanup when the mount fully tears down, or to cap the cached didSetSsrFlag at the actual current value of router.ssr at hit time rather than at write time.
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-on-rails-pro/src/tanstack-router/clientHydrate.ts`:
- Around line 217-333: The cache reuse logic on sharedHydrationInitStates is
only keyed by router and can return stale init state when the SSR payload
changes; compute a small payload discriminator from the current hydration
payload (e.g., from dehydratedState or dehydratedState.ssrRouter identifiers)
and compare it against a discriminator stored on the cached value in
sharedHydrationInitStates; if they differ (or if hasSsrPayload changed), treat
it as a cache miss (do not reuse cachedInit), proceed with router.update(),
match injection, __store.setState and user hydrate() as normal, and when storing
the init into sharedHydrationInitStates include the payload discriminator so
future mounts only reuse the cached init for the same (router, payload) pair.
🪄 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: 7ecc7634-2532-455a-9ca6-ae848a563eb9
📒 Files selected for processing (2)
CHANGELOG.mdpackages/react-on-rails-pro/src/tanstack-router/clientHydrate.ts
|
Thanks @greptile-apps — good catch on the stale
|
|
/run-skipped-ci |
|
The fix looks correct. The two
Both paths already set One minor observation worth confirming: after The regression test (mount → unmount → remount of the same cached router, asserting no unexpected-cleared warning) directly exercises the previously broken path. LGTM. |
|
🚀 Full CI Mode Enabled ✅ Triggered all workflows for full CI coverage Note: Added the To disable full CI mode, use the View progress in the Actions tab. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd0577ac98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…Mode mounts React 18's StrictMode double-renders components with fresh hook state on each pass, so the `routerRef.current === null` guard in `clientHydrate.ts` fired twice when `options.createRouter` returned the same router instance, re-running `loadRouteChunk`, `__store.setState`, and the user-defined `hydrate` callback. Add a module-level `WeakMap<TanStackRouter, SharedHydrationInitState>` that memoizes the render-phase init per router instance. On the second render the cached preload/hydrate promises and `didSetSsrFlag` are reattached to the current mount's refs so the post-hydration effect still awaits the original work and cleanup correctly clears `router.ssr`. Production behavior is unchanged because each mount creates a fresh router (WeakMap miss → full init). Fixes #3405.
Addresses Greptile's review: after the post-hydration effect cleared
`router.ssr` and set `didSetSsrFlagRef.current = false`, the cached
`didSetSsrFlag: true` in `sharedHydrationInitStates` went stale. A
subsequent full mount/remount of the same router would restore the stale
true and trigger the dev sanity-check warning at clientHydrate.ts:361
on a router whose ssr flag had been cleared correctly.
Mutate `cached.didSetSsrFlag = false` alongside both clear paths (effect
finally + cleanup deferred microtask) so the shared cache always
reflects the current `didSetSsrFlag` state.
Adds a regression test ("does not warn about cleared router.ssr after
full remount of a cached router") that mounts → unmounts → remounts the
same router and asserts the unexpected-cleared warning is not logged.
justin808
force-pushed
the
jg/3405-tanstack-strictmode-dedup
branch
from
cd0577a to
67a22f8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67a22f807f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (cachedInit) { | ||
| // Same router instance was already initialized by a discarded render | ||
| // (or prior mount). Reattach the pending preload/hydrate promises to | ||
| // this mount's refs so the post-hydration effect awaits the original | ||
| // work; restore didSetSsrFlag so cleanup correctly clears router.ssr. | ||
| routeChunkPreloadPromiseRef.current = cachedInit.routeChunkPreloadPromise; | ||
| hydrationCallbackPromiseRef.current = cachedInit.hydrationCallbackPromise; | ||
| didSetSsrFlagRef.current = cachedInit.didSetSsrFlag; |
There was a problem hiding this comment.
When sharedHydrationInitStates hits, this branch only restores cached refs and skips the SSR-flag setup path, so a real unmount/remount that reuses the same router can start a hydration mount with router.ssr already cleared. In this file’s own contract, that flag is what prevents Transitioner from auto-calling router.load() before post-hydration sequencing; skipping it on remount allows an early load (and then another from runPostHydrationLoad), which can race hydration state/chunk preloads and reintroduce hydration mismatch behavior for cached-router setups.
Useful? React with 👍 / 👎.
Code ReviewOverviewThis PR fixes a React 18 StrictMode bug where The fix adds a module-level Correctness ✅The fix is correct. Key aspects that hold up under scrutiny:
Design Trade-off Worth DocumentingThe WeakMap entry is never deleted after a full unmount/remount cycle. This means that on a genuine remount of the same router (non-StrictMode), the
This is safe in practice because the router already carries the browser history and injected match state from the first mount. But it is an implicit behavior: a developer who expects a full remount to re-run hydration init against the same router instance will be surprised. The safety invariant comment at line 205 could call this out explicitly as an intentional consequence of the WeakMap approach. Test Coverage ✅The new regression test ( Minor style note: the CHANGELOGThe entry is accurate but noticeably longer than surrounding entries. A shorter version would be more consistent with the file's style:
SummaryApprove with minor notes. The fix is minimal, targeted, and correct. The WeakMap approach is the right tool — it naturally avoids memory leaks and leaves production paths untouched. The test coverage adequately guards the specific regression. The full-remount implicit behavior and the verbose changelog entry are worth a quick look, but neither blocks merging. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // work; restore didSetSsrFlag so cleanup correctly clears router.ssr. | ||
| routeChunkPreloadPromiseRef.current = cachedInit.routeChunkPreloadPromise; | ||
| hydrationCallbackPromiseRef.current = cachedInit.hydrationCallbackPromise; | ||
| didSetSsrFlagRef.current = cachedInit.didSetSsrFlag; |
There was a problem hiding this comment.
The comment says "discarded render (or prior mount)" which covers StrictMode, but it's worth making explicit that this path also fires on a genuine full unmount → remount of the same router instance (non-StrictMode). On that path createBrowserHistory(), __store.setState(), and options.hydrate are all skipped because they already ran during the first mount. That's safe — the router already carries the correct state — but it's a non-obvious invariant. A short note like:
// Also fires on full unmount → remount of the same router (non-StrictMode).
// Safe to skip re-init because the router already has the browser history
// and injected match state from the first mount.
would help future readers who may only see the StrictMode framing.
| routeChunkPreloadPromise: routeChunkPreloadPromiseRef.current, | ||
| hydrationCallbackPromise: hydrationCallbackPromiseRef.current, | ||
| didSetSsrFlag: didSetSsrFlagRef.current, | ||
| }); |
There was a problem hiding this comment.
The set() call being outside (after) the try/catch block is intentionally correct: if options.hydrate or router.hydrate() throws, the catch block clears router.ssr, re-throws, and the WeakMap is never populated. The next render (StrictMode's second pass or a React retry) therefore gets a WeakMap miss and runs a full, clean init rather than reattaching state from a partially-failed mount. Worth a one-liner to preserve this intent:
// Outside the try/catch intentionally: a throw leaves the WeakMap unpopulated
// so the next render gets a full retry rather than reusing partial state.
sharedHydrationInitStates.set(router, {
size-limit report 📦
|
Brings in #3408 (Pro Gemfile.lock sync, fixes benchmark CI), #3410 (tanstack-router StrictMode dedupe), and #3395 (changelog docs). Conflict in packages/react-on-rails-pro/src/tanstack-router/clientHydrate.ts resolved by taking origin/main's version: #3410 is a more recent reimplementation by the same author of the StrictMode-replay dedup done on this branch (commits 4eee58e, 1b6ef61, 9309155). The branch's per-route preloadRouteChunk cache is superseded by main's unified sharedHydrationInitStates per-router state.
1 participant
Summary
tanstack-router integration (Pro) › does not double-call loadRouteChunk when StrictMode replays hydration effectstest that has been failing on every push tomainsince PR #3213.WeakMap<TanStackRouter, SharedHydrationInitState>inclientHydrate.tsthat memoizes the render-phase init per router instance, dedup'ingloadRouteChunk,__store.setState, and the user-definedhydratecallback across React 18 StrictMode's double-render-with-fresh-hooks behavior.options.createRouter()and gets a fresh router, so theWeakMapalways misses and the existing init runs.Fixes #3405.
Root cause
React 18's StrictMode renders components twice in dev with separate hook state per pass (verified empirically against
react@18.3.1):The
routerRef.current === nullguard relied on React 19's behavior of preservinguseRefacross the two passes, so on React 18 it fired twice when the user (or test) returned the same router fromoptions.createRouter.The previous "Safety invariant" comment at
clientHydrate.ts:186-196accounted for discarded renders, where the discarded router instance is dropped. That's still correct — but it didn't cover the case where the same router instance survives across passes (cached by the user's factory) and the discarded render's side effects on it leak.Fix
sharedHydrationInitStates: WeakMap<router, {routeChunkPreloadPromise, hydrationCallbackPromise, didSetSsrFlag}>is checked at the top of the init block. On a cache hit:routeChunkPreloadPromiseRef/hydrationCallbackPromiseRefso the post-hydration effect still awaits the original work before callingrouter.load()(otherwiserouter.loadwould race the in-flight preload).didSetSsrFlagRefso the cleanup path correctly clearsrouter.ssr.router.update({ history }),__store.setState,router.ssrwrites, and the user-definedhydratecallback.Per-router-instance memoization means:
Test plan
script/convert+pnpm install --no-frozen-lockfile, confirmedExpected: 1, Received: 2before the fix.tanstackRouter.test.tstests pass on React 18.0.0 (the minimum-supported Node 20 CI matrix).pnpm --filter react-on-rails-pro type-checkclean.pnpm lintclean.packages/react-on-rails-pro/src/tanstack-router/clientHydrate.tsandCHANGELOG.md.What's still in scope to watch
initializes hydration store injection once per created router in StrictModetest creates a new router percreateRoutercall — that's still a WeakMap miss for each, so each router'ssetStatefires once. Verified passing on both React 18 and 19.waits for the matched route chunk preload promise before triggering post-hydration router.loadtest (single mount, no StrictMode) still fires the preload exactly once. Verified.Note
Low Risk
Scoped to Pro client hydration in dev/StrictMode and cached-router scenarios; production paths with fresh routers per mount are unchanged.
Overview
Fixes Pro TanStack Router client hydration under React 18 StrictMode when
createRouterreturns a reused router instance (common in tests or user caching).Render-phase hydration in
clientHydrate.tsstill runs only whenrouterRef.current === null, but StrictMode resets refs between passes, so that guard could run twice and repeatloadRouteChunk,__store.setState, andoptions.hydrate. A module-levelWeakMapkeyed by the router instance now runs that init once per router; cache hits only reattach preload/hydrate promises anddidSetSsrFlagfor the post-hydration effect and cleanup.router.ssrclearing updates the WeakMap so a full remount of the same router does not trigger a false dev warning.Production behavior is unchanged when each mount gets a new router. CHANGELOG documents the fix for issue #3405; a regression test covers remount of a cached router.
Reviewed by Cursor Bugbot for commit 67a22f8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests