fix(tanstack-router): remove Suspense gate around RouterProvider during hydration

[Seifeldin7]

Summary

TanStackHydrationApp was wrapping RouterProvider in Suspense + RouteChunkPreloadGate, but serverRender.ts's buildAppElement emits AppWrapper > RouterProvider directly 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 RouterProvider directly so the client tree mirrors the server output exactly.

What changed

• packages/react-on-rails-pro/src/tanstack-router/clientHydrate.ts
  • Render createElement(RouterProvider, { router }) directly (no Suspense/gate wrapper).
  • Remove now-unused Suspense import, RouteChunkPreloadGate component, RouteChunkPreloadGateProps interface, and routeChunkPreloadSettledRef (only the gate read it).
  • routeChunkPreloadPromiseRef is preserved — runPostHydrationLoad still awaits it before router.load(), so chunk-preload behavior for post-hydration navigation is unchanged.

Test plan

• CI: unit tests in packages/react-on-rails-pro/tests/tanstackRouter.test.ts pass
• Lint and type checks pass on the package
• Manual: SSR a TanStack Router page with code-split routes and confirm no React hydration warnings in the console; the server-rendered HTML stays mounted (no full client re-render flash)
• Manual: client-side <Link> navigation still works after hydration (verifies post-mount router.load() still runs)

Origin

Originally landed in printivity/printivity as a yarn patch on the published react-on-rails-pro 16.6.0 npm package (see .yarn/patches/react-on-rails-pro-npm-16.6.0-183d3c1ef3.patch in #2843). Porting it upstream so the patch can be removed.

https://claude.ai/code/session_01NzWXywhF3M8h8zU8n8u3qX

---

Generated by Claude Code

Summary by CodeRabbit

• Refactor
  • Removed an intermediate Suspense wrapper during client hydration to simplify the render tree.
  • Post-hydration now awaits lazy route preloads before enabling navigation, reducing transient loading glitches and navigation races.
• Tests
  • Added integration tests verifying deferred navigation until route preload settles and absence of a Suspense wrapper in the hydrated output.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

Removes the Suspense-based route-chunk preload gate and its settled tracking. Hydration now renders RouterProvider (optionally wrapped by options.AppWrapper) directly and awaits routeChunkPreloadPromiseRef in post-mount flow before calling router.load().

Changes

Cohort / File(s)	Summary
Router Hydration Simplification packages/react-on-rails-pro/src/tanstack-router/clientHydrate.ts	Removes RouteChunkPreloadGate and surrounding Suspense; deletes routeChunkPreloadSettledRef and its finally() updates; keeps routeChunkPreloadPromiseRef and awaits it in runPostHydrationLoad before router.load(); render tree is RouterProvider (optionally options.AppWrapper) without Suspense.
Hydration Integration Tests packages/react-on-rails-pro/tests/tanstackRouter.test.ts	Adds two regression tests: one ensures router.load is deferred until matched route chunk preload promise settles (mocks route matching and chunk load), the other verifies hydration output renders RouterProvider directly with no enclosing Suspense markup.

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

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I found the gate unlatched and lightened up my pace,
No Suspense to startle me, just promises in place.
I waited on the quiet hum until the chunks were done,
The router woke and hopped away beneath the sun.
A carrot cheer — the hydrate race is run 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(tanstack-router): remove Suspense gate around RouterProvider during hydration' directly and accurately describes the main change: removing a Suspense-based gate layer (RouteChunkPreloadGate) that wrapped RouterProvider during hydration. It is concise, specific, and matches the primary objective of the changeset.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/port-printivity-patch-DrhDk

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Summary

This PR removes the Suspense + RouteChunkPreloadGate wrapper around RouterProvider in TanStackHydrationApp, making the client render tree match the server-emitted tree (AppWrapper > RouterProvider) and eliminating the React hydration mismatch that caused full client-side re-renders during SSR.

The removal is safe: routeChunkPreloadPromiseRef is preserved and still awaited in runPostHydrationLoad before router.load(), so chunk-preload sequencing for post-hydration navigation remains intact.

Confidence Score: 5/5

Safe 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

Filename	Overview
packages/react-on-rails-pro/src/tanstack-router/clientHydrate.ts	Removes Suspense/RouteChunkPreloadGate wrapper around RouterProvider to match server render tree; preserves routeChunkPreloadPromiseRef for post-hydration chunk sequencing. Change is correct and well-justified.

Sequence Diagram

sequenceDiagram
    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

Loading

_{Reviews (1): Last reviewed commit: "fix(tanstack-router): remove Suspense ga..." | Re-trigger Greptile}

[claude]

Code Review

Overview

This PR removes the Suspense/RouteChunkPreloadGate wrapper from the client-side hydration render path so that the client component tree exactly mirrors what serverRender.ts's buildAppElement() emits: AppWrapper > RouterProvider. The fix is correct and well-motivated — even a Suspense boundary that never suspends (because isHydrating skipped the throw) still produces a different React component tree shape than the server, causing React's hydration reconciler to bail out to a full client re-render.

---

What's correct

• Root cause analysis is accurate. React hydration requires the component type tree to match exactly, not just the rendered DOM. A Suspense > RouteChunkPreloadGate > RouterProvider chain on the client vs. a bare RouterProvider on the server is a structural mismatch regardless of whether the gate ever throws.
• Chunk preload behavior is preserved. routeChunkPreloadPromiseRef is still populated by preloadMatchedRouteChunks() and still awaited in runPostHydrationLoad before router.load() is called (lines 320–325). Lazy route chunks still finish loading before the router enables client-side navigation.
• Dead code cleanly removed. routeChunkPreloadSettledRef, its finally tracking block, RouteChunkPreloadGate, RouteChunkPreloadGateProps, and the Suspense import are all gone without residue.

---

Concerns

1. No regression test for the Suspense-shape hydration mismatch (minor)

The existing test suite verifies that RouterProvider is rendered (not RouterClient), but there is no test that would catch reintroduction of a Suspense wrapper around RouterProvider. A hydrateRoot-based test that asserts no hydration-error console output would make this guarantee durable.

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 runPostHydrationLoad awaits chunks before router.load()), but it is a subtle behavioral difference worth acknowledging in a comment or the PR description.

3. Test plan gap

The test plan verifies that existing tests pass but does not include a test for "chunk preloads are awaited before router.load()". Adding a test that holds the preload promise pending and asserts router.load has not been called until it resolves would close this gap.

---

Verdict

Approve with minor suggestions. The fix is correct, the removed code was doing more harm than good (the isHydrating escape hatch was always a fragile workaround for the structural mismatch), and the simplification is a net positive. The concerns above are documentation and test gaps rather than correctness issues.

[github-actions]

size-limit report 📦

Path	Size
react-on-rails/client bundled (gzip)	62.83 KB (0%)
react-on-rails/client bundled (gzip) (time)	62.83 KB (0%)
react-on-rails/client bundled (brotli)	53.9 KB (0%)
react-on-rails/client bundled (brotli) (time)	53.9 KB (0%)
react-on-rails-pro/client bundled (gzip)	63.77 KB (0%)
react-on-rails-pro/client bundled (gzip) (time)	63.77 KB (0%)
react-on-rails-pro/client bundled (brotli)	54.81 KB (0%)
react-on-rails-pro/client bundled (brotli) (time)	54.81 KB (0%)
registerServerComponent/client bundled (gzip)	127.95 KB (0%)
registerServerComponent/client bundled (gzip) (time)	127.95 KB (0%)
registerServerComponent/client bundled (brotli)	62.06 KB (0%)
registerServerComponent/client bundled (brotli) (time)	62.06 KB (0%)
wrapServerComponentRenderer/client bundled (gzip)	122.37 KB (0%)
wrapServerComponentRenderer/client bundled (gzip) (time)	122.37 KB (0%)
wrapServerComponentRenderer/client bundled (brotli)	57.09 KB (0%)
wrapServerComponentRenderer/client bundled (brotli) (time)	57.09 KB (0%)

[claude]

Code Review

Overview

Solid fix for a genuine hydration bug. Removing the Suspense/RouteChunkPreloadGate wrapper is the correct solution — it directly addresses the root cause (client tree diverging from the server tree) rather than trying to paper over it with the isHydrating guard that was already in RouteChunkPreloadGate. The change is minimal, well-commented, and preserves the chunk-preload timing for post-hydration navigation via routeChunkPreloadPromiseRef.

---

Issues

Missing CHANGELOG entry

Per the repo's changelog guidelines, user-visible bug fixes must be recorded in /CHANGELOG.md. This fixes SSR hydration failures (full client re-render instead of hydration) for any TanStack Router app with code-split routes — clearly user-visible and important enough to call out. Please add an entry under the current unreleased section, e.g.:

- **[Pro]** **TanStack Router hydration fix**: Remove Suspense/RouteChunkPreloadGate wrapper around RouterProvider to prevent React hydration mismatch when using code-split routes. [PR 3213](https://github.com/shakacode/react_on_rails/pull/3213) by [Seifeldin7](https://github.com/Seifeldin7)

Behavioral change lacks dedicated test coverage

The comment at lines 376–380 correctly documents the semantic difference: React can no longer suspend intermediate re-renders during the brief window between when routeChunkPreloadPromiseRef.current is set and when runPostHydrationLoad awaits it. In practice this window is milliseconds and almost certainly harmless, but there is no test locking in "no Suspense thrown from this component during chunk preload". Without one, a future contributor could re-introduce a Suspense wrapper believing it is safe, hit the same hydration bug in production, and have no test failure to guide them.

A straightforward test would: mock loadRouteChunk to return a never-resolving promise, render TanStackHydrationApp via renderToString, and assert the output does not contain a Suspense fallback node (or simply that it does not throw).

---

Positive notes

• routeChunkPreloadSettledRef removal is clean: The ref was only ever read by RouteChunkPreloadGate; removing both together is correct.
• Backward compatibility: The deprecated RouterClient parameter is retained with a one-time warning. Good.
• The existing test suite is thorough: StrictMode double-invoke, async hydrate rejection, cancellation-after-unmount, user-provided router.ssr preservation — these are exactly the scenarios that matter here.
• Comment quality: The inline comments explaining why RouterProvider is used directly (and what would break if a Suspense boundary were added) are genuinely useful for future maintainers.

[claude]

Code Review

Overview

This PR correctly fixes a React hydration mismatch in TanStackHydrationApp. The server renders AppWrapper > RouterProvider directly; the client was wrapping that in Suspense > RouteChunkPreloadGate > RouterProvider, producing a Suspense-shape mismatch that caused React to abandon SSR and fall back to a full client render. Removing the extra boundary makes the client tree match the server output exactly.

The core logic is sound:

• routeChunkPreloadSettledRef is correctly removed — its only consumer was RouteChunkPreloadGate.
• routeChunkPreloadPromiseRef is correctly preserved and awaited in runPostHydrationLoad before router.load(), so post-hydration navigation still waits for matched lazy chunks to settle.
• The old RouteChunkPreloadGate suspension only applied to re-renders after hydration and before chunks settled (it was bypassed during the hydration render itself via the isHydrating guard). Since the fallback was null and the window is brief, removing it is a safe trade-off.

Issues

Missing test: chunk preload awaited before router.load()

There is a test verifying that router.options.hydrate is awaited before router.load() (the async hydrate test), but there is no equivalent test for routeChunkPreloadPromiseRef. Given that this is now the sole mechanism ensuring lazy chunks settle before navigation starts, a parallel async test would close the gap:

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 comments

Three 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.

Summary

The 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.

[claude]

Code Review

Overview

This PR fixes a React hydration mismatch in TanStackHydrationApp by removing the Suspense + RouteChunkPreloadGate wrapper around RouterProvider. The root cause is exactly right: the server renders AppWrapper > RouterProvider (see serverRender.ts:buildAppElement), but the client was producing AppWrapper > Suspense > RouteChunkPreloadGate > RouterProvider — an extra Suspense node that React can't reconcile against the server HTML, forcing a full client re-render.

Correctness ✅

The fix is correct. Even though RouteChunkPreloadGate was engineered to not throw during the initial hydration render (isHydrating=true short-circuits the throw), the Suspense boundary itself was still present as an extra tree node with no server counterpart. React sees the structural mismatch regardless of whether the boundary activates.

Rendering RouterProvider directly mirrors the server tree exactly.

Behavior change: Suspense fallback → deferred router.load()

The old RouteChunkPreloadGate had a second job beyond hydration: after the post-mount effect set didTriggerPostHydrationLoadRef.current = true, it would re-render-block via Suspense (showing a null fallback) until matched lazy chunks finished loading. That behavior is now replaced by awaiting routeChunkPreloadPromiseRef inside runPostHydrationLoad before calling router.load().

The new approach is strictly better from a UX perspective: instead of flashing a blank screen (null fallback) during the chunk-load window, the server-rendered HTML stays mounted. Navigation is still deferred until chunks are ready because router.load() hasn't been called yet and router.ssr blocks the Transitioner's auto-load.

Removed code

• RouteChunkPreloadGate + RouteChunkPreloadGateProps — no longer needed.
• routeChunkPreloadSettledRef — was only read by the gate; gone with it.
• The finally block that updated routeChunkPreloadSettledRef — note this was also an uncancelled background microtask; the new runPostHydrationLoad path handles cancellation properly via the cancelled flag.

Test coverage

Existing tests cover all the moved pieces well:

• "renders RouterProvider (not RouterClient) on client hydration with SSR match data" — directly validates the fix.
• "preloads matched lazy route chunks before the first hydration render" — confirms loadRouteChunk is called for all matched routes.
• Async lifecycle tests (hydrate callback ordering, cancellation on unmount, StrictMode) remain unchanged and still pass.

One minor gap (see inline comment): there is no async test verifying that router.load() waits for the chunk preload promise to settle before being called. This is the post-hydration analogue of the existing waits for async router.options.hydrate before triggering post-hydration router.load test. Without it, a future accidental reordering in runPostHydrationLoad would go undetected.

Summary

Clean, focused fix with a clear root cause explanation and solid test coverage. The behavioral trade-off (Suspense null-flash → deferred load) is an improvement. The one suggested addition is a test for the chunk-preload await ordering. Approve with that suggestion.

[chatgpt-codex-connector]

💡 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".

[claude]

Code Review

Overview

This PR removes the Suspense + RouteChunkPreloadGate wrapper around RouterProvider in TanStackHydrationApp. The root cause is correctly diagnosed: serverRender.ts's buildAppElement emits AppWrapper > RouterProvider with no Suspense boundary, but the client was rendering Suspense > RouteChunkPreloadGate > RouterProvider. React detects the tree-shape mismatch during hydration and bails to a full client re-render. The fix is exactly right — mirror the server tree by rendering RouterProvider directly.

---

What's Good

• Correct diagnosis and minimal fix. The patch removes exactly the code that caused the mismatch and nothing else. The isHydrating guard in the old RouteChunkPreloadGate was already an attempt to paper over this, but having a Suspense node in the tree at all is enough to trigger the mismatch.
• Chunk-preload behavior preserved. routeChunkPreloadPromiseRef is still set in the render phase and awaited in runPostHydrationLoad before router.load(), so post-hydration navigation still waits for matched lazy chunks.
• routeChunkPreloadSettledRef correctly removed. That ref only ever served the gate's suspension condition. Nothing else read it.
• Two focused regression tests. One verifies the timing invariant (chunk preload awaited before router.load()), the other verifies no Suspense wrapper in the rendered HTML.

---

Issues

Missing CHANGELOG entry

Per the project's changelog-guidelines.md, user-visible bug fixes need an entry under [Unreleased] > #### Fixed in /CHANGELOG.md. This is a Pro-specific fix, so it should be tagged **[Pro]**. Example:

- **[Pro]** **TanStack Router hydration no longer bails to full client re-render**: Removed the `Suspense`/`RouteChunkPreloadGate` wrapper around `RouterProvider` in `TanStackHydrationApp`. The extra boundary produced a tree-shape mismatch against the server-rendered output, causing React to discard SSR HTML and re-render from scratch. [PR 3213](https://github.com/shakacode/react_on_rails/pull/3213) by [Seifeldin7](https://github.com/Seifeldin7).

Fragile microtask-counting in the chunk-preload timing test

await Promise.resolve(); await Promise.resolve(); advances through the async chain by counting microtask hops. If the implementation ever adds or removes a single await (e.g. adding error handling, changing Promise.all shape), the test silently breaks — router.load would not yet have been called when the assertion runs. See inline comment.

Test data typo: leading space in dehydrated match ID

In the renderToString regression test, i: ' products' has a leading space (line 1482). The correct dehydrated form of /products is \0products (per dehydrateSsrMatchId in serverRender.ts, which replaces / with \0). Because ' products' never matches the client match '/products', the SSR data path is never exercised — applyDehydratedMatchData takes the "no server match found" fallback for every match. The test still validates the Suspense-wrapper assertion correctly, but the ssrRouter block is effectively inert, which is misleading. See inline comment.

---

Minor Observations

• The renderToString approach for detecting Suspense wrappers is clever and correct for React 18 (<!--$--> / <!--/$--> markers are emitted even for non-suspending Suspense boundaries). Worth noting this may differ in React 19's renderToString behaviour if the supported React range ever expands.
• The routeChunkPreloadPromiseRef comment added at line 160–162 is appropriately terse and explains the non-obvious invariant.

[coderabbitai]

🧹 Nitpick comments (1)

packages/react-on-rails-pro/tests/tanstackRouter.test.ts (1)

1376-1382: Optional: dangling pending promise on early-failure paths.

loadRouteChunk returns a Promise whose resolve is captured into resolveChunk; if an assertion fails before line 1438, the promise never resolves and runPostHydrationLoad stays pending until the test finishes. Jest will still pass/fail correctly, but you may want a small afterEach that calls resolveChunk?.() (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

📥 Commits

Reviewing files that changed from the base of the PR and between 2ee58c9 and 77b1845.

📒 Files selected for processing (1)

• packages/react-on-rails-pro/tests/tanstackRouter.test.ts

[claude]

Code Review

Overview

This PR fixes a React hydration mismatch in TanStackHydrationApp by removing the Suspense > RouteChunkPreloadGate > RouterProvider render tree in favour of rendering RouterProvider directly. The root cause is correctly identified: serverRender.ts emits AppWrapper > RouterProvider with no Suspense boundary, so any extra boundary on the client produces a shape mismatch and React bails to a full client-side re-render, defeating SSR.

The key insight is also preserved: the route-chunk preload behavior is not lost — runPostHydrationLoad already awaits routeChunkPreloadPromiseRef before calling router.load(), so post-hydration navigation still blocks until matched lazy chunks are ready.

✅ What the PR gets right

• Correct root-cause fix. The Suspense shape mismatch is eliminated cleanly, and the comment at line 367–370 explains why this is the right approach.
• No behavior regression on chunk preload. routeChunkPreloadPromiseRef is retained and runPostHydrationLoad awaits it; the gate's only meaningful behavior (blocking router.load() until preload settles) is replicated without the Suspense machinery.
• Net UX improvement. With the old gate, any external re-render in the window between didTriggerPostHydrationLoadRef.current = true and preload settling would flash a null Suspense fallback. The new code keeps the UI stable in that window.
• Two targeted regression tests. The structural test (no Suspense markers in renderToString output) and the async sequencing test (gate of router.load() on preload) are exactly the right bets to lock in this fix.

⚠️ Issues / suggestions

1. Missing CHANGELOG entry

There is no entry in CHANGELOG.md under [Unreleased] for this fix. Per project convention, bug fixes in the Pro package should appear there tagged with [Pro]. For example:

#### Fixed
- **[Pro]** Fixed TanStack Router SSR hydration mismatch caused by a stray `Suspense` boundary around `RouterProvider` in `TanStackHydrationApp`. React now hydrates without bailing to a full client-side re-render. [PR 3213](https://github.com/shakacode/react_on_rails/pull/3213) by [Seifeldin7](https://github.com/Seifeldin7)

2. routeChunkPreloadSettledRef tracking block removed but its .finally() was redundant anyway

The removed block:

if (routeChunkPreloadPromiseRef.current) {
  routeChunkPreloadSettledRef.current = false;
  void routeChunkPreloadPromiseRef.current.finally(() => {
    routeChunkPreloadSettledRef.current = true;
  });
}

…was the only writer to routeChunkPreloadSettledRef, and that ref was only ever read inside RouteChunkPreloadGate (which is also gone). The removal is correct — noting this for reviewers who might wonder if the finally() served another purpose.

3. Minor: redundant router.load mock reassignment in new test

buildRouter() already sets load: jest.fn().mockResolvedValue(undefined) at line 33. The reassignment at test line 1388 is a no-op and can be removed for clarity (see inline comment).

4. Minor: redundant optional chaining after throw guard

resolveChunk?.() at test line 1438 uses optional chaining, but the if (!resolveChunk) { throw … } guard at line 1433 already narrows the type to () => void at that point. Either the guard or the ?. is unnecessary (see inline comment).

Test coverage assessment

The two new tests cover the most important invariants:

• Structural: renderToString output equals exactly <div data-testid="provider"></div> — any Suspense wrapper injects <!--$--> / <!--/$--> markers that break equality.
• Behavioral: router.load() is not called until after resolveChunk() settles and a macrotask drains — confirms the await ordering.

One gap worth noting but not blocking: neither test uses hydrateRoot (they use createRoot / renderToString). A true hydration test would give stronger confidence that React doesn't emit a reconciliation warning in the real scenario. This is appropriate for a unit-test suite; the manual test plan items cover it.

Verdict

The fix is correct, well-motivated, and well-tested. The two minor test nits are cleanup-level and don't affect correctness. The only meaningful ask is the CHANGELOG entry.

[chatgpt-codex-connector]

💡 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".

[claude]

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.

[claude]

Code Review

Overview

This PR fixes a real and significant bug: TanStackHydrationApp was wrapping RouterProvider in Suspense > RouteChunkPreloadGate, but the server-render path (serverRender.ts's buildAppElement) emits AppWrapper > RouterProvider with no Suspense boundary. The shape mismatch caused React to discard the server HTML and re-render entirely on the client, defeating SSR. The fix is correct and minimal.

---

What works well

• Root cause is accurate. Confirmed against serverRender.ts: buildAppElement renders createElement(RouterProvider, { router }) directly, optionally wrapped by AppWrapper. The removed Suspense boundary was the only source of mismatch.
• Chunk-preload sequencing is preserved. routeChunkPreloadPromiseRef is still set during render-phase init and is awaited in runPostHydrationLoad before router.load(). Post-hydration navigation cannot race ahead of matched lazy chunks.
• Dead code is fully cleaned up. RouteChunkPreloadGate, RouteChunkPreloadGateProps, routeChunkPreloadSettledRef, and the unused Suspense import are all gone with no orphaned references.
• Error handling is intact. preloadMatchedRouteChunks already catches chunk-load errors internally, so a chunk failure will log and still allow router.load() to proceed rather than hanging the post-hydration flow.
• Regression tests are well-designed. The renderToString HTML equality assertion is a tight guarantee — Suspense boundaries emit <!--$--> / <!--/$--> markers even when they don't suspend, so this check would catch any accidental re-introduction of a wrapping Suspense.

---

Points to address

Subtle behavior change worth documenting — With RouteChunkPreloadGate in place, any re-render triggered during the chunk-preload window (e.g. a context value update from a parent) would be suspended and show the null fallback until preloads settled. That gate is now gone: re-renders proceed freely through RouterProvider during that window. In practice this is actually better UX (no spurious loading flash), but it is a behavior change. The existing comment at the render call explains the hydration-mismatch side; a note about the re-render behavior change would help future readers understand why the gate was deliberately not moved to a post-hydration position.

router.options = {} in the new preload test — The assignment silently replaces buildRouter()'s options.hydrate: jest.fn() mock to isolate the chunk-preload await path. The intent is clear from the adjacent comment, but this form of wholesale mutation is easy to misread. A minor improvement: router.options = { hydrate: undefined } makes the isolation explicit rather than replacing the entire options object.

setTimeout(r, 0) macrotask drain — The comment in the test correctly explains why a macrotask boundary is used rather than counting await hops. The approach is reasonable. If runPostHydrationLoad ever gains a genuine async I/O step (not just microtask-depth awaits), setTimeout(0) would still pass spuriously. The current approach is acceptable; just worth watching if the test becomes flaky.

---

Lychee / CHANGELOG changes

The two new lychee.toml exclusions (invite.reactrails.com, 110grill.com) are unrelated housekeeping; no issues. The CHANGELOG entry is accurate and well-written.

---

Summary

The 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 RouterProvider render call.

[claude]

Code Review

Summary

This is a well-targeted fix for a genuine React hydration mismatch bug. The root cause analysis is correct: the server emits AppWrapper > RouterProvider with no Suspense boundary, so the old client tree (Suspense > RouteChunkPreloadGate > RouterProvider) forces React to bail to a full client re-render. Rendering RouterProvider directly is the right fix.

The chunk-preload sequencing is preserved correctly by awaiting routeChunkPreloadPromiseRef inside runPostHydrationLoad before router.load(), and the new tests verify both the happy path and the error/reject path.

---

Issues

1. Non-standard use of useInsertionEffect (medium risk — flagged inline)

useInsertionEffect is used to get synchronous, non-replaying unmount cleanup for clearing router.ssr. The mechanism works today, but the React docs explicitly limit this hook to CSS-in-JS style injection, and the "not replayed by StrictMode" property is an implementation detail, not a documented contract. If React ever extends StrictMode replay to insertion effects, the two-path cleanup invariant (useSynchronousRealUnmountEffect vs. the finally block) breaks silently.

The inline comment on line 28 sketches a simpler alternative: consolidate SSR-flag clearing into the useEffect cleanup (which already handles the cancellation path), removing the need for useInsertionEffect entirely.

2. latestEffectRunIdRef guard relies on undocumented React scheduling invariant (low risk — flagged inline)

The guard prevents stale StrictMode passive-effect finally blocks from racing a remount. It works because React's flushPassiveEffects() is synchronous, so cleanup + re-setup complete before any queued microtasks. This invariant has been stable throughout React 18 but is not part of the public API contract. The inline comment on lines 394–405 suggests naming the specific mechanism to make any future breakage easier to diagnose.

---

Correctness — things checked and found correct

• Render-phase router init guard (routerRef.current === null): correctly handles both StrictMode's discarded renders and concurrent mode re-renders; no double-init.
• Chunk-preload promise preserved: routeChunkPreloadPromiseRef is set in the render phase and awaited in runPostHydrationLoad; navigation cannot race ahead of matched lazy chunks.
• No final cancelled check before router.load(): correct by single-threaded invariant — if neither await was reached, no yield occurred and unmount cannot have fired between the last guard and this call.
• Dev-mode warning for unexpected router.ssr clearing: narrow condition (didSetSsrFlagRef.current && router.ssr == null) correctly fires only when we set the flag but something else cleared it.
• didTriggerPostHydrationLoadRef reset in cleanup: correctly allows the post-hydration effect to re-run on StrictMode remount.
• renderToString regression test: using raw HTML equality to assert no Suspense boundary is a strong, minimal assertion for the core bug.

---

Test gap worth noting

There is no test that exercises the useEffect fallback path (React < 18, where useInsertionEffect is unavailable). The module-level typeof useInsertionEffect === 'function' branch is untested. This is a pre-existing gap if the package genuinely targets React 16/17, though in practice most CI environments will run React 18+.

---

Minor nit

The local type alias type ReactElement = React.ReactElement on line 14 is redundant; React.ReactElement can be used directly at the call sites. Not a blocker.

[claude]

Code Review — PR #3213

Summary

Correct and well-motivated fix. The root cause (Suspense shape mismatch between server and client renders) is accurately diagnosed, and rendering RouterProvider directly to mirror buildAppElement's output (serverRender.ts:22) is the right solution. Chunk-preload sequencing is correctly preserved by awaiting routeChunkPreloadPromiseRef inside runPostHydrationLoad rather than through a Suspense suspension gate.

What's good

• Core fix is correct. The regression test 'renders RouterProvider directly without an enclosing Suspense boundary' makes the server/client tree contract machine-checkable via renderToString — any future Suspense wrapper reintroduction will fail this test immediately.
• StrictMode handling is thorough. The latestEffectRunIdRef counter + deferred Promise.resolve().then() in cleanup correctly handles React 18 StrictMode's cleanup-then-remount replay. The 'does not double-call loadRouteChunk when StrictMode replays hydration effects' test pins the key invariant.
• Test coverage is comprehensive. The four new tests cover the dev warning, chunk-preload sequencing, rejection fallthrough, and StrictMode idempotency — all the behaviorally significant regressions.
• Changelog entry follows project format.

Observations (non-blocking)

1. Import style — The hybrid import * as React + module-level const { createElement, useEffect, useRef } = React is unconventional. Named imports with a separate import type { ReactElement } from 'react' would be equally expressive and closer to the original style.

2. React scheduler assumption — The deferred Promise.resolve().then() in the effect cleanup works because React 18's flushPassiveEffects() runs passive cleanup + re-setup in one synchronous call stack, meaning the microtask queue cannot drain between them. This lets the re-setup increment latestEffectRunIdRef before the stale continuation fires. The comment documents this well, but it's a load-bearing assumption about React's scheduler that future React versions could change.

3. resolveChunk?.() in test finally blocks — In the happy-path chunk-preload test, settleChunk and resolveChunk reference the same resolver function. resolveChunk?.() in finally is harmless after first settlement (Promise resolvers are no-ops), but a reader may wonder why it's called twice. Renaming the finally call to e.g. pendingResolve?.() with a separate variable, or adding a brief comment, would make the intent clearer.

Verdict

Approve. The fix is correct, tested, and well-documented. Observations are cosmetic/informational and should not block merge.

[justin808]

Address-review checkpoint for feedback after 2026-05-24T01:15:28Z.

Mattered:

• Addressed the useInsertionEffect concern in cbd16518d by removing the non-standard hook path entirely. Cleanup now uses the passive effect cleanup's deferred continuation with latestEffectRunIdRef and didSetSsrFlagRef.
• Addressed the StrictMode scheduling comment in cbd16518d; the code comment now names flushPassiveEffects() and explains the synchronous cleanup/setup invariant.

Optional:

• Addressed the test-name feedback in cbd16518d; the test now focuses on preventing duplicate loadRouteChunk calls while preserving the assertions for delayed router.load() and SSR-flag cleanup.

Skipped:

• The no-final-cancellation-check thread was confirmation of the current logic, so no code change was needed.

Validation:

• pnpm --filter react-on-rails-pro exec jest tests/tanstackRouter.test.ts --runInBand

Future full-PR scans should start after this comment unless check all reviews is requested.

[justin808]

Address-review checkpoint for feedback after 2026-05-24T01:15:28Z.

Mattered:

• Addressed the useInsertionEffect concern in cbd16518d by removing the non-standard hook path entirely. Cleanup now uses the passive effect cleanup's deferred continuation with latestEffectRunIdRef and didSetSsrFlagRef.
• Addressed the StrictMode scheduling comment in cbd16518d; the code comment now names flushPassiveEffects() and explains the synchronous cleanup/setup invariant.

Optional:

• Addressed the test-name feedback in cbd16518d; the test now focuses on preventing duplicate loadRouteChunk calls while preserving the assertions for delayed router.load() and SSR-flag cleanup.

Skipped:

• The no-final-cancellation-check thread was confirmation of the current logic, so no code change was needed.

Validation:

• pnpm --filter react-on-rails-pro exec jest tests/tanstackRouter.test.ts --runInBand

Additional cleanup after the first checkpoint:

• Resolved the already-rationalized React namespace import compatibility thread without code changes.
• Resolved three non-blocking follow-up nits as rationale-only: import style, cleanup comment placement, and defensive promise resolver cleanup.

No additional code changes were made after cbd16518d.

Future full-PR scans should start after this comment unless check all reviews is requested.