fix(react-router): avoid Suspense above root documents - #8055
Conversation
|
View your CI Pipeline Execution ↗ for commit e7fcb00
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview3 package(s) bumped directly, 10 bumped as dependents. 🟩 Patch bumps
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRoot route rendering no longer wraps document-level SSR or hydrated output in unsafe Suspense boundaries. Pending-state tests and a production Playwright test verify content retention, document preservation, and hydration without errors. ChangesRoot Suspense handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MatchView
participant SuspenseGuard as canWrapRouteInSuspense
participant MatchInner
participant SSRDocument as SSR document
MatchView->>SuspenseGuard: Check root route and SSR state
SuspenseGuard-->>MatchView: Allow or disallow Suspense wrapping
MatchView->>MatchInner: Render pending match
MatchInner->>SSRDocument: Retain document output for SSR or hydration
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Merging this PR will degrade performance by 4.76%
|
![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
`@e2e/react-start/dev-ssr-styles/tests/issue-8053-root-document-hydration.spec.ts`:
- Around line 9-12: Declare __issue8053Hydration and __issue8053SsrNode on the
Window interface with their appropriate types, then update the hydration-state
assignment and MutationObserver access to use window.__issue8053Hydration and
window.__issue8053SsrNode directly. Remove both window as any casts while
preserving the existing SSR-node and hydration-state behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2822edd2-1749-4472-92b6-172274e50207
📒 Files selected for processing (3)
e2e/react-start/dev-ssr-styles/package.jsone2e/react-start/dev-ssr-styles/src/routes/__root.tsxe2e/react-start/dev-ssr-styles/tests/issue-8053-root-document-hydration.spec.ts
| ;(window as any).__issue8053Hydration = state | ||
|
|
||
| new MutationObserver((records) => { | ||
| const ssrNode = (window as any).__issue8053SsrNode as Node | undefined |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace any with typed Window properties.
window as any removes type checks for the SSR-node and hydration-state contract. Declare these test properties on Window and access them directly.
Proposed fix
+declare global {
+ interface Window {
+ __issue8053Hydration?: { removed: boolean }
+ __issue8053SsrNode?: Node
+ }
+}
+
- ;(window as any).__issue8053Hydration = state
+ window.__issue8053Hydration = state
...
- const ssrNode = (window as any).__issue8053SsrNode as Node | undefined
+ const ssrNode = window.__issue8053SsrNode
...
- captured: !!(window as any).__issue8053SsrNode,
- removed: (window as any).__issue8053Hydration.removed,
+ captured: !!window.__issue8053SsrNode,
+ removed: window.__issue8053Hydration!.removed,As per coding guidelines: **/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.
Also applies to: 45-50
🤖 Prompt for 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.
In
`@e2e/react-start/dev-ssr-styles/tests/issue-8053-root-document-hydration.spec.ts`
around lines 9 - 12, Declare __issue8053Hydration and __issue8053SsrNode on the
Window interface with their appropriate types, then update the hydration-state
assignment and MutationObserver access to use window.__issue8053Hydration and
window.__issue8053SsrNode directly. Remove both window as any casts while
preserving the existing SSR-node and hydration-state behavior.
Source: Coding guidelines
|
|
||
| function RootComponent() { | ||
| useEffect(() => { | ||
| document.documentElement.dataset.hydrated = 'true' |
There was a problem hiding this comment.
why are we using this particular e2e test as a regression test? this doesnt have to do anything with dev styles, right?
maybe just add a separate e2e project?
| }).observe(document, { childList: true, subtree: true }) | ||
| }) | ||
|
|
||
| const hydrationErrors: Array<string> = [] |
There was a problem hiding this comment.
do we really need to manually check errors? doesnt our custom test fixture from @tanstack/router-e2e-utils handle that already?
|
|
||
| expect(await screen.findByTestId('pending')).toBeVisible() | ||
| expect(screen.getByTestId('content')).not.toBeVisible() | ||
| expect(screen.queryByTestId('content')).not.toBeInTheDocument() |
There was a problem hiding this comment.
why was this changed? same question for the other similar changes down below
2 participants
Fixes #8053
Summary
shellComponent, explicitwrapInSuspense, and selective SSR (ssr: falseorssr: data-only)<html>with its pending UIRegression origin: #7805
Before #7805,
MatchViewdeliberately prevented a root route from getting an implicit route-level Suspense boundary merely because it defined apendingComponent:For a normal SSR root,
route.isRootwas true,wrapInSuspensewas not enabled, andresolvedNoSsrwas false. The first condition therefore selectedSafeFragment, leaving a document-owning root shaped like this during hydration:This was an intentional exception, documented by the adjacent comment about only allowing the root to be forcefully wrapped. The separate boundary in
Matches.tsxwas also disabled during SSR and hydration, while the first child route could still suspend safely through the boundary rendered byOutletinside the root document.The
Match.tsxrewrite in #7805 removed the root-route condition and reduced the selection to:Changing
PendingComponenttopendingElementwas not itself the problem: both are truthy when a pending component exists. The regression came from dropping the!route.isRoot || ...gate. After that change, adding evenpendingComponent: () => nullimplicitly produced this tree:React treats
<html>,<head>, and<body>as document singletons and hoists them into the server-rendered document preamble. The Suspense markers are emitted lower in<body>, but the client fiber says that Suspense is above<html>. React cannot associate that marker with the client boundary, reports a hydration mismatch, clears the boundary, and client-renders the entire SSR document. The page can still look correct afterward, which makes the lost SSR DOM identity easy to miss.The lane-loader architecture did not inherently require removing this rule; the guard was lost during the broad renderer simplification. The current fix restores the document-ownership invariant while accounting for the newer architecture:
shellComponentis safe because the shell owns<html>/<head>/<body>and the boundary is placed inside itwrapInSuspenseremains a force opt-inpendingComponentThe fix also handles the modern lane renderer's pending branch. When an unwrappable hydrated root becomes pending, it keeps rendering its retained SSR content rather than replacing
<html>with pending UI. Pure CSR roots continue to render their pending component normally. Before #7805, the older loading/presentation flow did not expose this direct pending-root replacement path, so restoring only the old boundary expression would not cover all behavior under the new architecture.Reproduction
The hydration issue is now reproduced directly in
e2e/react-start/dev-ssr-stylesusing React 19.2.3; upgrading to React 19.2.8 is not required.The Playwright regression:
MutationObserverbefore application code runsThis uses DOM removal and node identity as the behavioral oracle rather than React internal stream comment markers.
Main versus this fix
The production test was also run in an isolated worktree based on
origin/mainat38485038c5, with only the E2E fixture commit cherry-picked and all workspace artifacts rebuilt from that checkout.main: fails withcaptured: true,removed: true, andreplaced: trueThe destructive hydration recovery reproduces in both Vite development and a built production application, so the issue is not dev-only. In development React also reports the hydration mismatch; in production the DOM identity check directly detects the destructive recovery.
Coverage
pendingComponentwraps<html>in an unhydratable Suspense since 1.170.19, discarding the whole SSR document #8053 regression and runs in the existing dev matrix plus a focused built-production run<!--html--><!--head--><!--body-->was removed because it was an indirect, React-internal proxy and did not perform hydrationTests
CI=1 NX_DAEMON=false pnpm nx run @tanstack/react-router:test:unit --outputStyle=stream --skipRemoteCache(1,010 passed, 1 skipped)CI=1 NX_DAEMON=false pnpm nx run @tanstack/react-router:test:types --outputStyle=stream --skipRemoteCache(TypeScript 5.6 through 7.0)CI=1 NX_DAEMON=false pnpm nx run @tanstack/react-router:test:eslint --outputStyle=stream --skipRemoteCache(0 errors)CI=1 pnpm run test:e2eine2e/react-start/dev-ssr-styles(six dev configurations plus production)CI=1 pnpm run test:e2e:prodine2e/react-start/dev-ssr-stylesgit diff --checkSummary by CodeRabbit
Bug Fixes
Tests