fix: cleanup streamed values #5870
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces server-side SSR script buffering (ScriptBuffer) and exposes router.serverSsr.takeBufferedScripts(); integrates buffered scripts into React and Solid HeadContent with nonce/class; extends global SSR typings and client hydration flow (hydrated/streamEnd) with cleanup; refines route manifest to omit routes without preload/assets. Changes
Sequence DiagramsequenceDiagram
participant Server as SSR Server
participant Buffer as ScriptBuffer
participant RouterSSR as router.serverSsr
participant Client as Client (Hydrate)
participant Head as HeadContent
Server->>Buffer: enqueue serialized scripts (onSerialize)
Server->>Buffer: enqueue streamEnd (onDone)
Note over Buffer: Barrier holds emission until liftBarrier()
Server->>Buffer: liftBarrier() when render finished
Client->>RouterSSR: call takeBufferedScripts()
RouterSSR->>Buffer: retrieve & clear buffered scripts
Buffer-->>RouterSSR: script string | undefined
RouterSSR-->>Client: return buffered scripts
Client->>Client: run hydrate hook
Client->>Client: set hydrated = true
Client->>Client: call c() cleanup
Note over Client: if hydrated && streamEnd → remove globals
Client->>Head: include serverHeadScript (nonce, class `$tsr`) if present
Head->>Client: render/inject script tag
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 7m 53s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 24s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-11-15 01:02:21 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/react-router/src/HeadContent.tsx(2 hunks)packages/react-router/src/ScriptOnce.tsx(0 hunks)packages/router-core/src/router.ts(1 hunks)packages/router-core/src/ssr/constants.ts(1 hunks)packages/router-core/src/ssr/ssr-client.ts(3 hunks)packages/router-core/src/ssr/ssr-server.ts(7 hunks)packages/router-core/src/ssr/tsrScript.ts(1 hunks)packages/solid-router/src/HeadContent.tsx(1 hunks)packages/start-server-core/src/router-manifest.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/react-router/src/ScriptOnce.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
packages/router-core/src/ssr/ssr-client.tspackages/start-server-core/src/router-manifest.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/start-server-core/src/router-manifest.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
packages/start-server-core/src/router-manifest.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/start-server-core/src/router-manifest.ts
🧬 Code graph analysis (3)
packages/react-router/src/HeadContent.tsx (1)
packages/react-router/src/useRouterState.tsx (1)
useRouterState(48-76)
packages/router-core/src/ssr/ssr-client.ts (1)
packages/router-core/src/ssr/constants.ts (1)
GLOBAL_TSR(1-1)
packages/router-core/src/ssr/ssr-server.ts (3)
packages/router-core/src/router.ts (1)
AnyRouter(768-768)packages/router-core/src/ssr/constants.ts (1)
GLOBAL_TSR(1-1)packages/router-core/src/ssr/tsrScript.ts (1)
p(11-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/start-server-core/src/router-manifest.ts (1)
46-57: LGTM! Clean filtering of routes without data.The
hasDataflag and conditional return pattern correctly filter out routes that have neither preloads nor assets. The explicit length checks handle both undefined and empty array cases properly. Since the root route is guaranteed to have at least one asset (the client entry script added at line 28), it won't be inadvertently filtered.
| @@ -1 +1,2 @@ | |||
| export const GLOBAL_TSR = '$_TSR' | |||
| export declare const GLOBAL_SEROVAL = '$R' | |||
There was a problem hiding this comment.
Fix the invalid constant declaration
Line 2 uses export declare const … = …, which is illegal in a .ts module (ambient declarations cannot have initializers). This will make TypeScript fail to compile. Please switch to a normal constant export.
-export declare const GLOBAL_SEROVAL = '$R'
+export const GLOBAL_SEROVAL = '$R'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export declare const GLOBAL_SEROVAL = '$R' | |
| export const GLOBAL_SEROVAL = '$R' |
🤖 Prompt for AI Agents
In packages/router-core/src/ssr/constants.ts around line 2, the file currently
uses an ambient declaration with an initializer ("export declare const ... =
...") which is illegal in a TS module; change it to a normal exported constant
by removing the "declare" keyword (e.g., use "export const GLOBAL_SEROVAL =
'$R'") so the initializer is allowed and the module compiles.
| import type { GLOBAL_SEROVAL, GLOBAL_TSR } from './constants' | ||
|
|
||
| declare global { | ||
| interface Window { | ||
| [GLOBAL_TSR]?: TsrSsrGlobal | ||
| [GLOBAL_SEROVAL]?: any | ||
| } |
There was a problem hiding this comment.
Import the constants as runtime values
GLOBAL_TSR (and the new GLOBAL_SEROVAL) are exported as runtime constants, so import type { … } cannot resolve them—TypeScript reports “has no exported member” and compilation stops. Bring them in as normal value imports instead.
-import type { GLOBAL_SEROVAL, GLOBAL_TSR } from './constants'
+import { GLOBAL_SEROVAL, GLOBAL_TSR } from './constants'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { GLOBAL_SEROVAL, GLOBAL_TSR } from './constants' | |
| declare global { | |
| interface Window { | |
| [GLOBAL_TSR]?: TsrSsrGlobal | |
| [GLOBAL_SEROVAL]?: any | |
| } | |
| import { GLOBAL_SEROVAL, GLOBAL_TSR } from './constants' | |
| declare global { | |
| interface Window { | |
| [GLOBAL_TSR]?: TsrSsrGlobal | |
| [GLOBAL_SEROVAL]?: any | |
| } |
🤖 Prompt for AI Agents
In packages/router-core/src/ssr/ssr-client.ts around lines 9 to 15, the file
imports GLOBAL_TSR and GLOBAL_SEROVAL using "import type" but those are
runtime-exported constants and must be imported as actual values; change the
import to a normal value import (remove "type") so the constants are available
at runtime, keep any type-only imports as "import type" if needed, and update
any references to use the imported runtime values; rebuild to ensure TypeScript
no longer reports the missing exported member error.
Summary by CodeRabbit
Refactor
Bug Fixes
Chores