fix(tanstack-router): remove Suspense gate around RouterProvider during hydration#3213
fix(tanstack-router): remove Suspense gate around RouterProvider during hydration#3213Seifeldin7 wants to merge 9 commits intomainfrom
Conversation
…ng hydration The RouteChunkPreloadGate wrapped RouterProvider in a Suspense boundary that the server render does not emit. serverRender's buildAppElement returns `AppWrapper > RouterProvider` directly, so the extra boundary on the client produces a Suspense-shape mismatch and React bails to a full client-side re-render. Render RouterProvider directly so the client tree mirrors the server output. The route-chunk preload promise is still awaited in the post-mount effect before router.load(), so navigation behavior is unchanged. Ported from printivity/printivity#2843.
|
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:
WalkthroughRemoves the Suspense-based route-chunk preload gate and its settled tracking. Hydration now renders Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AppWrapper as AppWrapper/Render
participant RouterProvider as Router
participant ChunkPreloader as RouteChunkPreload
Client->>AppWrapper: render (if provided)
AppWrapper->>Router: render RouterProvider
Note right of Router: no Suspense/gate present
Client->>ChunkPreloader: ensure routeChunkPreloadPromiseRef
Client->>Client: runPostHydrationLoad()
Client->>ChunkPreloader: await routeChunkPreloadPromiseRef
ChunkPreloader-->>Client: preload promise settles
Client->>Router: call router.load()
Router-->>Client: navigation proceeds
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 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 |
Greptile SummaryThis PR removes the The removal is safe: Confidence Score: 5/5Safe to merge — the fix correctly aligns client and server render trees with no regressions to chunk preloading logic. Single-file change with a clear, well-documented rationale. The server tree shape is confirmed in serverRender.ts (line 22). The removed gate code was the source of the mismatch; the only ref it depended on (routeChunkPreloadSettledRef) is cleanly removed. The remaining routeChunkPreloadPromiseRef is still awaited in the post-hydration effect, preserving the chunk preload sequencing contract. No logic errors or regressions found. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Server as serverRender.ts
participant Client as clientHydrate.ts
participant React as React Hydration
Note over Server: SSR render
Server->>React: AppWrapper > RouterProvider
Note over Client: Before fix (mismatch)
Client-->>React: AppWrapper > Suspense > RouteChunkPreloadGate > RouterProvider
React--xReact: Tree shape mismatch → full client re-render
Note over Client: After fix (matches server)
Client->>React: AppWrapper > RouterProvider
React->>React: Hydration succeeds
Note over Client: Post-hydration effect
Client->>Client: await routeChunkPreloadPromiseRef (chunks)
Client->>Client: router.load() → client nav enabled
Reviews (1): Last reviewed commit: "fix(tanstack-router): remove Suspense ga..." | Re-trigger Greptile |
Code ReviewOverviewThis PR removes the What's correct
Concerns1. No regression test for the Suspense-shape hydration mismatch (minor) The existing test suite verifies that 2. Lost Suspense-gate semantics for post-hydration re-renders (acknowledged trade-off) The old gate blocked React from committing any re-render triggered between mount and chunk-preload completion. With the gate gone, if something triggers a synchronous re-render in that window, route components could render before their chunks are fully loaded. In practice this window is negligible (the effect fires immediately after mount, and 3. Test plan gap The test plan verifies that existing tests pass but does not include a test for "chunk preloads are awaited before VerdictApprove with minor suggestions. The fix is correct, the removed code was doing more harm than good (the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // client tree mirrors what serverRender.ts emits (AppWrapper > RouterProvider). | ||
| // Any extra boundary here produces a Suspense-shape mismatch during hydration | ||
| // and React bails to full client-side rendering. | ||
| let app: ReactElement = createElement(RouterProvider, { router }); |
There was a problem hiding this comment.
This is the right fix. Even with the old isHydrating guard skipping the throw, the Suspense boundary itself was still present in the component tree, so React's reconciler saw Suspense > RouteChunkPreloadGate > RouterProvider on the client vs. RouterProvider on the server — a structural type-mismatch that causes a hydration bail-out regardless of whether the boundary ever suspends.
One thing worth adding to the comment: routeChunkPreloadPromiseRef is still awaited in runPostHydrationLoad (line ~321) before router.load(), so chunk-preload timing for post-hydration navigation is unchanged. The only semantic difference is that React can no longer suspend intermediate re-renders during the preload window — which is acceptable given how brief that window is in practice.
| @@ -184,7 +158,6 @@ function TanStackHydrationApp({ | |||
| const didTriggerPostHydrationLoadRef = useRef(false); | |||
| const didSetSsrFlagRef = useRef(false); | |||
| const routeChunkPreloadPromiseRef = useRef<Promise<void> | null>(null); | |||
There was a problem hiding this comment.
This ref is still used correctly — it is populated at line ~231 and awaited in runPostHydrationLoad before router.load(). Consider a short inline comment here (or a JSDoc on runPostHydrationLoad) clarifying that this is the only remaining consumer, so a future reader doesn't assume it is dead code after the gate removal.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
size-limit report 📦
|
…loadPromiseRef Per review feedback, document that the preload promise ref is still consumed by runPostHydrationLoad after the Suspense gate removal, so a future reader does not assume it is dead code.
Code ReviewOverviewSolid fix for a genuine hydration bug. Removing the IssuesMissing CHANGELOG entryPer the repo's changelog guidelines, user-visible bug fixes must be recorded in Behavioral change lacks dedicated test coverageThe comment at lines 376–380 correctly documents the semantic difference: React can no longer suspend intermediate re-renders during the brief window between when A straightforward test would: mock Positive notes
|
| // router.load(), so chunk-preload timing for post-hydration navigation is | ||
| // unchanged. The only semantic difference is that React can no longer suspend | ||
| // intermediate re-renders during the brief preload window. | ||
| let app: ReactElement = createElement(RouterProvider, { router }); |
There was a problem hiding this comment.
This is the correct fix. One follow-up worth considering: there is no test that asserts this path never suspends. If a future change re-adds a Suspense wrapper here (e.g. thinking it only affects post-hydration), the hydration regression will be silent in tests but loud in production. A renderToString-based test that sets loadRouteChunk to a pending promise and verifies the component renders without suspending would lock this in.
| let app: ReactElement = createElement(RouterProvider, { router }); | |
| let app: ReactElement = createElement(RouterProvider, { router }); |
| // Awaited in runPostHydrationLoad before router.load() so post-hydration | ||
| // navigation waits for matched lazy chunks. Since the Suspense/gate wrapper | ||
| // around RouterProvider was removed, runPostHydrationLoad is the only | ||
| // remaining consumer of this ref. |
There was a problem hiding this comment.
The comment is accurate, but it might help to make the write-site vs. read-site split explicit for future readers: this ref is written here (render phase, line ~235) and read only in the runPostHydrationLoad effect (~line 327). Calling it "the only remaining consumer" is precise, but "consumer" could be read as "writer". Something like "Read only in runPostHydrationLoad..." would remove the ambiguity.
…PreloadPromiseRef Per review feedback, distinguish where the ref is written (render-phase SSR block) from where it is read (runPostHydrationLoad effect) so future readers do not conflate the two.
| // waits for matched lazy chunks to settle. Since the Suspense/gate wrapper | ||
| // around RouterProvider was removed, runPostHydrationLoad is the sole | ||
| // read-site for this ref. | ||
| const routeChunkPreloadPromiseRef = useRef<Promise<void> | null>(null); |
There was a problem hiding this comment.
The comment explains the history of what was removed rather than explaining the current invariant. Since future readers won't know what was removed, the backward-looking framing ("since the Suspense/gate wrapper was removed") is confusing. Consider simplifying to just the forward-looking contract:
| const routeChunkPreloadPromiseRef = useRef<Promise<void> | null>(null); | |
| // Set during render-phase SSR init; awaited in runPostHydrationLoad before | |
| // router.load() so post-hydration navigation waits for matched lazy chunks. | |
| const routeChunkPreloadPromiseRef = useRef<Promise<void> | null>(null); |
| // The SSR flag prevented auto-loading, so we do it manually here. | ||
| // This is the sole read-site for routeChunkPreloadPromiseRef: it awaits the | ||
| // preload promise before router.load() so navigation waits for matched | ||
| // lazy chunks to settle. |
There was a problem hiding this comment.
Same backward-looking framing. The "sole read-site" comment is better placed on the ref declaration (above) where a reader naturally looks when tracing where something is read. Suggest dropping these lines — the ref comment and the await call on line 330 already tell the full story without redundancy.
| // Render RouterProvider directly without any Suspense/gate wrapper so the | ||
| // client tree mirrors what serverRender.ts emits (AppWrapper > RouterProvider). | ||
| // Any extra boundary here produces a Suspense-shape mismatch during hydration | ||
| // and React bails to full client-side rendering. | ||
| // | ||
| // RouteChunkPreloadGate blocks re-renders until matched lazy chunks finish | ||
| // preloading (when preload support is available). During the initial SSR | ||
| // hydration render, the gate is skipped to match the server-rendered tree | ||
| // and avoid a hydration mismatch. After the post-mount effect runs | ||
| // (didTriggerPostHydrationLoadRef becomes true), the gate activates normally. | ||
| let app: ReactElement = createElement( | ||
| Suspense, | ||
| { fallback: null }, | ||
| createElement( | ||
| RouteChunkPreloadGate, | ||
| { | ||
| preloadPromise: routeChunkPreloadPromiseRef.current, | ||
| preloadSettledRef: routeChunkPreloadSettledRef, | ||
| isHydrating: hasSsrPayload && !didTriggerPostHydrationLoadRef.current, | ||
| }, | ||
| createElement(RouterProvider, { router }), | ||
| ), | ||
| ); | ||
| // routeChunkPreloadPromiseRef is still awaited in runPostHydrationLoad before | ||
| // router.load(), so chunk-preload timing for post-hydration navigation is | ||
| // unchanged. The only semantic difference is that React can no longer suspend | ||
| // intermediate re-renders during the brief preload window. | ||
| let app: ReactElement = createElement(RouterProvider, { router }); |
There was a problem hiding this comment.
The last two sentences of this block explain the old behavior that was removed, which is fine for the PR description but adds noise to the code. The first two sentences are the essential invariant readers need. The routeChunkPreloadPromiseRef paragraph is worth keeping only if the comment is colocated with the await on line 330 — here it's two screens away. Suggest trimming to:
| // Render RouterProvider directly without any Suspense/gate wrapper so the | |
| // client tree mirrors what serverRender.ts emits (AppWrapper > RouterProvider). | |
| // Any extra boundary here produces a Suspense-shape mismatch during hydration | |
| // and React bails to full client-side rendering. | |
| // | |
| // RouteChunkPreloadGate blocks re-renders until matched lazy chunks finish | |
| // preloading (when preload support is available). During the initial SSR | |
| // hydration render, the gate is skipped to match the server-rendered tree | |
| // and avoid a hydration mismatch. After the post-mount effect runs | |
| // (didTriggerPostHydrationLoadRef becomes true), the gate activates normally. | |
| let app: ReactElement = createElement( | |
| Suspense, | |
| { fallback: null }, | |
| createElement( | |
| RouteChunkPreloadGate, | |
| { | |
| preloadPromise: routeChunkPreloadPromiseRef.current, | |
| preloadSettledRef: routeChunkPreloadSettledRef, | |
| isHydrating: hasSsrPayload && !didTriggerPostHydrationLoadRef.current, | |
| }, | |
| createElement(RouterProvider, { router }), | |
| ), | |
| ); | |
| // routeChunkPreloadPromiseRef is still awaited in runPostHydrationLoad before | |
| // router.load(), so chunk-preload timing for post-hydration navigation is | |
| // unchanged. The only semantic difference is that React can no longer suspend | |
| // intermediate re-renders during the brief preload window. | |
| let app: ReactElement = createElement(RouterProvider, { router }); | |
| // Render RouterProvider directly — matching the server-rendered tree (AppWrapper > RouterProvider). | |
| // Any extra Suspense boundary here produces a shape mismatch during hydration | |
| // and React bails to full client-side rendering. | |
| let app: ReactElement = createElement(RouterProvider, { router }); |
Code ReviewOverviewThis PR correctly fixes a React hydration mismatch in The core logic is sound:
IssuesMissing test: chunk preload awaited before There is a test verifying that it('waits for chunk preload promise before triggering post-hydration router.load', async () => {
const router = buildRouter();
let resolveChunkLoad: (() => void) | undefined;
router.loadRouteChunk = jest.fn().mockReturnValue(
new Promise<void>((resolve) => { resolveChunkLoad = resolve; }),
);
router.looseRoutesById = { '/products': { id: '/products' } };
router.load = jest.fn().mockResolvedValue(undefined);
// ... render, assert router.load not called, resolve, assert router.load called once
});Inline commentsThree inline notes on the new comments added — they're written in a backward-looking style (explaining what was removed) that reads awkwardly once the PR context is gone. Suggestions in the thread to trim them to the forward-looking contract only. SummaryThe fix is correct and addresses a real SSR regression. The two items above are non-blocking but worth addressing before merge: adding the missing async test and tidying the comments. |
…ariants Per review feedback, drop backward-looking phrasing about the removed Suspense wrapper. Comments should explain current behavior, not historical context that PR descriptions cover. - routeChunkPreloadPromiseRef: state the contract concisely (set during init, awaited before router.load()). - useEffect comment: drop the redundant "sole read-site" sentence; the ref declaration covers it. - JSX comment: trim to the essential invariant — RouterProvider must render directly to match the server tree.
Code ReviewOverviewThis PR fixes a React hydration mismatch in Correctness ✅The fix is correct. Even though Rendering Behavior change: Suspense fallback → deferred
|
| // (AppWrapper > RouterProvider). Any extra Suspense boundary here produces | ||
| // a shape mismatch during hydration and React bails to full client-side | ||
| // rendering. | ||
| let app: ReactElement = createElement(RouterProvider, { router }); |
There was a problem hiding this comment.
This is the right fix. Worth noting the behavioral shift: the old RouteChunkPreloadGate would re-render-block via Suspense (null fallback) after hydration if lazy chunks were still loading. With that gone, the deferred await routeChunkPreloadPromiseRef.current inside runPostHydrationLoad is now the only thing preventing router.load() from firing early. The test suite covers that loadRouteChunk is called, but there's no async test verifying the ordering (router.load is not called until the preload promise settles). A test similar to waits for async router.options.hydrate before triggering post-hydration router.load but for the chunk-preload path would lock in that invariant against future refactors.
…tion Two regression tests for the Suspense-gate removal: - "renders RouterProvider directly without an enclosing Suspense boundary during hydration" fails against the pre-fix code with <!--$-->/<!--/$--> markers in the rendered HTML, locking in that TanStackHydrationApp emits RouterProvider with no wrapping boundary (matching serverRender.ts's buildAppElement output). - "waits for the matched route chunk preload promise before triggering post-hydration router.load" pins the explicitly preserved invariant from the PR description: routeChunkPreloadPromiseRef is awaited in runPostHydrationLoad before router.load(), so post-hydration navigation does not race ahead of matched lazy chunks.
![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: c835ce72ea
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| lastMatchId: ' products', | ||
| matches: [{ i: ' products', l: { products: ['hammer'] }, s: 'success', ssr: true, u: 123 }], |
There was a problem hiding this comment.
This line embeds a raw NUL byte in the TypeScript source ('\u0000products' was expected), which makes the file behave like a binary file for common text tooling and can cause search/lint/review workflows to skip or mis-handle it. The runtime value is the same if written as an escape sequence, so keeping a literal 0x00 byte here adds avoidable maintenance risk.
Useful? React with 👍 / 👎.
Code ReviewOverviewThis PR removes the What's Good
IssuesMissing CHANGELOG entryPer the project's Fragile microtask-counting in the chunk-preload timing test
Test data typo: leading space in dehydrated match IDIn the Minor Observations
|
| // One tick to settle the preload promise's .then(() => undefined), | ||
| // a second to advance through the awaits in runPostHydrationLoad. | ||
| await Promise.resolve(); | ||
| await Promise.resolve(); |
There was a problem hiding this comment.
Counting microtask hops like this is fragile — if the implementation ever adds or removes one await the assertion below fires before router.load() has been called, silently breaking the test.
A safer pattern is to flush all pending microtasks generically (e.g. await new Promise(setImmediate) or a small jest.runAllTimersAsync() call if you switch to fake timers), or — even better — spy on the resolved promise directly:
| // One tick to settle the preload promise's .then(() => undefined), | |
| // a second to advance through the awaits in runPostHydrationLoad. | |
| await Promise.resolve(); | |
| await Promise.resolve(); | |
| resolveChunk?.(); | |
| // Flush all pending microtasks so runPostHydrationLoad can fully drain. | |
| await new Promise((r) => setTimeout(r, 0)); |
| ssrRouter: { | ||
| manifest: undefined, | ||
| lastMatchId: ' products', | ||
| matches: [{ i: ' products', l: { products: ['hammer'] }, s: 'success', ssr: true, u: 123 }], |
There was a problem hiding this comment.
Looks like a typo — ' products' (leading space) is neither the raw route ID ('/products') nor its dehydrated form ('\0products', which is what dehydrateSsrMatchId produces by replacing / with \0).
Because rehydrateMatchId(' products') returns ' products' unchanged, it never matches the client match id '/products', so applyDehydratedMatchData always takes the "no server match found" → override-to-success fallback. The ssrRouter block in this test is therefore effectively inert and the SSR-data hydration code path is never exercised.
The test still validates the Suspense-wrapper assertion correctly, but it would be clearer to either use the correct dehydrated ID so the SSR data path is exercised, or remove the ssrRouter block entirely to make the fallback path explicit.
| matches: [{ i: ' | |
| matches: [{ i: '\0products', l: { products: ['hammer'] }, s: 'success', ssr: true, u: 123 }], |
- Use escaped Unicode (backslash u 0000) instead of a raw NUL byte in the ssrRouter.matches dehydrated match id. Raw NUL bytes make the file behave like binary for text tooling and rendered as a leading space in GitHub's diff UI, which led one reviewer to mis-read the value as a typo. The runtime value is unchanged. - Replace microtask-counting (await Promise.resolve() x2) in the chunk-preload timing test with a setTimeout(0) macrotask boundary so all queued microtasks drain before we assert. Counting hops is brittle: adding or removing a single await in runPostHydrationLoad would silently break the assertion.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-on-rails-pro/tests/tanstackRouter.test.ts (1)
1376-1382: Optional: dangling pending promise on early-failure paths.
loadRouteChunkreturns aPromisewhoseresolveis captured intoresolveChunk; if an assertion fails before line 1438, the promise never resolves andrunPostHydrationLoadstays pending until the test finishes. Jest will still pass/fail correctly, but you may want a smallafterEachthat callsresolveChunk?.()(or wrap the whole body in try/finally that resolves it) to avoid noisy unhandled-promise warnings in CI on regressions. Non-blocking nit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-on-rails-pro/tests/tanstackRouter.test.ts` around lines 1376 - 1382, The test creates a Promise whose resolver is captured in resolveChunk by the mocked loadRouteChunk and may remain unresolved on early assertion failures, leaving runPostHydrationLoad pending; ensure the resolver is always invoked on tear-down by adding an afterEach that calls resolveChunk?.() (or wrap the test body in try/finally and call resolveChunk?.() in finally) so the pending promise is resolved and avoids unhandled-promise warnings; refer to resolveChunk, loadRouteChunk and runPostHydrationLoad when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-on-rails-pro/tests/tanstackRouter.test.ts`:
- Around line 1376-1382: The test creates a Promise whose resolver is captured
in resolveChunk by the mocked loadRouteChunk and may remain unresolved on early
assertion failures, leaving runPostHydrationLoad pending; ensure the resolver is
always invoked on tear-down by adding an afterEach that calls resolveChunk?.()
(or wrap the test body in try/finally and call resolveChunk?.() in finally) so
the pending promise is resolved and avoids unhandled-promise warnings; refer to
resolveChunk, loadRouteChunk and runPostHydrationLoad when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1490e1ed-6ccc-4f62-9415-47af2780aa47
📒 Files selected for processing (1)
packages/react-on-rails-pro/tests/tanstackRouter.test.ts
Code ReviewOverviewThis PR fixes a React hydration mismatch in The key insight is also preserved: the route-chunk preload behavior is not lost — ✅ What the PR gets right
|
| ]); | ||
| router.looseRoutesById = { '/products': productsRoute }; | ||
| router.loadRouteChunk = loadRouteChunk; | ||
| router.load = jest.fn().mockResolvedValue(undefined); |
There was a problem hiding this comment.
buildRouter() already sets load: jest.fn().mockResolvedValue(undefined) (line 33 of the same file), so this reassignment is a no-op. You can remove it to reduce noise — or, if the intent is to make the mock explicitly visible at the call site for readability, a brief comment saying so would help.
| router.load = jest.fn().mockResolvedValue(undefined); | |
| router.options = {}; |
| } | ||
|
|
||
| await compatAct(async () => { | ||
| resolveChunk?.(); |
There was a problem hiding this comment.
The ?. optional chain is redundant here — the if (!resolveChunk) { throw … } guard at line 1433 already narrows the type to () => void, so TypeScript (and any reader) knows it's defined at this point. Using plain call is cleaner and avoids the false impression that it might be undefined.
| resolveChunk?.(); | |
| resolveChunk(); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77b1845944
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Add an [Unreleased] > Fixed entry for PR 3213 explaining the Suspense-shape mismatch that caused full client re-renders during hydration and noting that chunk-preload sequencing is preserved.
|
Code Review - see inline comments for details. Overall verdict: Approve. The Suspense removal is the correct fix for the hydration mismatch. Chunk-preload sequencing is preserved via routeChunkPreloadPromiseRef awaited in runPostHydrationLoad. Two focused regression tests cover both behaviors. One concern worth a comment: RouteChunkPreloadGate also protected subsequent re-renders (after hydration, before router.load()) from rendering while chunks are in-flight. That protection is now gone. The router.ssr flag mitigates this for navigation, but a parent-triggered re-render in that window could reach RouterProvider before chunks settle. The window is short and risk is low, but worth documenting inline so a future developer does not re-introduce the gate for the wrong reason. |
| // Render RouterProvider directly — matching the server-rendered tree | ||
| // (AppWrapper > RouterProvider). Any extra Suspense boundary here produces | ||
| // a shape mismatch during hydration and React bails to full client-side | ||
| // rendering. |
There was a problem hiding this comment.
The old RouteChunkPreloadGate also suspended post-hydration re-renders while chunk promises were in-flight (the gate only skipped suspension on the initial hydration render). That protection is intentionally dropped here — the router.ssr flag prevents Transitioner-initiated navigation during this window, and the window itself is very short.
Worth leaving a note in the comment so a future reader does not assume the gate needs to be re-introduced:
| // rendering. | |
| // Render RouterProvider directly — matching the server-rendered tree | |
| // (AppWrapper > RouterProvider). Any extra Suspense boundary here produces | |
| // a shape mismatch during hydration and React bails to full client-side | |
| // rendering. | |
| // | |
| // Note: the old RouteChunkPreloadGate also suspended post-hydration | |
| // re-renders while chunk promises were in-flight. That protection is | |
| // intentionally absent here: router.ssr prevents Transitioner navigation | |
| // during the window, and the window is short (mount effect → router.load() | |
| // completion). Do not re-introduce a Suspense wrapper to restore it. | |
| let app: ReactElement = createElement(RouterProvider, { router }); |
- Remove router.load reassignment in the chunk-preload timing test; buildRouter() already sets load to a resolved jest.fn at line 33. - Replace resolveChunk?.() with a narrowed local const after the guard, removing the stale optional-chain that read as 'this might be undefined' even though the throw above proves it isn't.
The markdown-link-check workflow currently fails on three pre-existing links that return 403 due to bot protection: - https://invite.reactrails.com (Slack invite endpoint, three uses in README.md and docs/oss/) - https://www.110grill.com (Popmenu-powered restaurant referenced as a production example in docs/oss/migrating/migrating-from-vite-rails.md) Both fit the established 'sites that block bots' exclusion pattern alongside reactrails.slack.com, careersatdoordash.com, etc.
Code ReviewOverviewThis PR fixes a real and significant bug: What works well
Points to addressSubtle behavior change worth documenting — With
Lychee / CHANGELOG changesThe two new SummaryThe fix is correct, the dead-code removal is complete, and the regression tests cover both the ordering invariant and the no-Suspense structural guarantee. The points above are minor. Approving with a suggestion to add a short note about the re-render behavior change alongside the existing comment block near the |
| // (AppWrapper > RouterProvider). Any extra Suspense boundary here produces | ||
| // a shape mismatch during hydration and React bails to full client-side | ||
| // rendering. | ||
| let app: ReactElement = createElement(RouterProvider, { router }); |
There was a problem hiding this comment.
The comment explains the hydration-mismatch side of this decision well. Worth adding one sentence about the re-render behavior change:
| let app: ReactElement = createElement(RouterProvider, { router }); | |
| // Render RouterProvider directly — matching the server-rendered tree | |
| // (AppWrapper > RouterProvider). Any extra Suspense boundary here produces | |
| // a shape mismatch during hydration and React bails to full client-side | |
| // rendering. Note: the former RouteChunkPreloadGate that suspended re-renders | |
| // during chunk preload is intentionally not replicated here — chunk-preload | |
| // sequencing is now enforced by awaiting routeChunkPreloadPromiseRef in | |
| // runPostHydrationLoad before router.load(), so free re-renders during that | |
| // window are safe and avoid the null-fallback flash the gate caused. |
| router.looseRoutesById = { '/products': productsRoute }; | ||
| router.loadRouteChunk = loadRouteChunk; | ||
| // No user-defined hydrate callback — isolate the chunk-preload await path. | ||
| router.options = {}; |
There was a problem hiding this comment.
This silently replaces the entire options object (including the hydrate: jest.fn() set by buildRouter()). The intent is clear from the comment, but being explicit about what's being suppressed reads better:
| router.options = {}; | |
| // No user-defined hydrate callback — isolate the chunk-preload await path. | |
| router.options = { hydrate: undefined }; |
2 participants
Summary
TanStackHydrationAppwas wrappingRouterProviderinSuspense+RouteChunkPreloadGate, butserverRender.ts'sbuildAppElementemitsAppWrapper > RouterProviderdirectly with no Suspense boundary. The extra client-side boundary produces a Suspense-shape mismatch during hydration and React bails to a full client-side re-render — defeating SSR.This change renders
RouterProviderdirectly so the client tree mirrors the server output exactly.What changed
packages/react-on-rails-pro/src/tanstack-router/clientHydrate.tscreateElement(RouterProvider, { router })directly (noSuspense/gate wrapper).Suspenseimport,RouteChunkPreloadGatecomponent,RouteChunkPreloadGatePropsinterface, androuteChunkPreloadSettledRef(only the gate read it).routeChunkPreloadPromiseRefis preserved —runPostHydrationLoadstill awaits it beforerouter.load(), so chunk-preload behavior for post-hydration navigation is unchanged.Test plan
packages/react-on-rails-pro/tests/tanstackRouter.test.tspass<Link>navigation still works after hydration (verifies post-mountrouter.load()still runs)Origin
Originally landed in
printivity/printivityas a yarn patch on the publishedreact-on-rails-pro16.6.0 npm package (see.yarn/patches/react-on-rails-pro-npm-16.6.0-183d3c1ef3.patchin #2843). Porting it upstream so the patch can be removed.https://claude.ai/code/session_01NzWXywhF3M8h8zU8n8u3qX
Generated by Claude Code
Summary by CodeRabbit