fix: script streaming #5895
fix: script streaming #5895
Conversation
|
View your CI Pipeline Execution ↗ for commit 76640fa
☁️ Nx Cloud last updated this comment at |
WalkthroughIntroduces a stream-barrier for SSR script ordering, wraps buffered SSR scripts as RouterManagedTag objects, renames preloadMeta→preloadLinks, renders empty client-side script tags to avoid hydration errors, and migrates route manifest handling to a local Manifest.routes shape. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App Render
participant Stream as TransformStream
participant Router as Router SSR
participant Client as Browser
App->>Stream: write HTML chunks (incremental)
Note right of Stream: Decoder runs with stream:true
Stream->>Stream: detect chunk containing TSR_SCRIPT_BARRIER_ID
Stream->>Router: call liftScriptBarrier()
Router->>Router: mark barrier lifted
Stream->>Client: flush buffered HTML + scripts (router buffer)
Client->>Client: execute serverBufferedScript (first)
Client->>Client: execute remaining scripts & hydrate
sequenceDiagram
autonumber
participant SSR as SSR Server
participant Buffer as Script Buffer
participant RouterAPI as Router ServerSsr API
SSR->>RouterAPI: takeBufferedScripts()
RouterAPI->>Buffer: wrap buffered markup into RouterManagedTag {id,nonce,className,innerHTML}
RouterAPI-->>SSR: return RouterManagedTag | undefined
SSR->>SSR: inject RouterManagedTag into emitted HTML (prepended)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/start-plugin-core/src/start-manifest-plugin/plugin.ts (1)
256-263: Critical bug: deleting from wrong object.Line 261 deletes from
routeTreeRoutesbut should delete frommanifest.routes. This inconsistency means routes without assets or preloads will still be included in the final manifest because the deletion targets the wrong object.Apply this diff to fix the bug:
Object.keys(manifest.routes).forEach((routeId) => { const route = manifest.routes[routeId]! const hasAssets = route.assets && route.assets.length > 0 const hasPreloads = route.preloads && route.preloads.length > 0 if (!hasAssets && !hasPreloads) { - delete routeTreeRoutes[routeId] + delete manifest.routes[routeId] } })
🧹 Nitpick comments (2)
packages/react-router/src/ScriptOnce.tsx (1)
17-17: Guarded TSR call looks good; consider tightening the guard slightlyThe new
typeof $_TSR !== "undefined" && $_TSR.c()guard is a clear improvement over the unguarded call and avoidsReferenceErrorwhen$_TSRis missing, while keeping the semicolon delimiter sochildrenand the TSR call stay separate statements.If you want this to be more defensive against unexpected shapes of
$_TSR, you could optionally also guard the method:children + ';typeof $_TSR !== "undefined" && typeof $_TSR.c === "function" && $_TSR.c()'Not required, but it would prevent a
TypeErrorif$_TSRexists without a callablec.packages/solid-router/src/ScriptOnce.tsx (1)
15-19: Guarded$_TSRcall is good; consider ES5-safe form inside the inline scriptThe new guard
typeof $_TSR !== "undefined" && $_TSR?.c()nicely avoids aReferenceErrorwhen$_TSRisn’t defined and keeps behavior no-op in that case. One nuance: because this is emitted as literal script text viainnerHTML, bundlers won’t transpile the optional chaining, so support for older browsers (without?.) could be affected.If you still care about non‑evergreen targets, consider an ES5‑style check instead:
- innerHTML={children + ';typeof $_TSR !== "undefined" && $_TSR?.c()'} + innerHTML={ + children + + ';typeof $_TSR !== "undefined" && $_TSR && $_TSR.c && $_TSR.c()' + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/react-router/src/Asset.tsx(1 hunks)packages/react-router/src/HeadContent.tsx(3 hunks)packages/react-router/src/ScriptOnce.tsx(1 hunks)packages/react-router/src/Scripts.tsx(1 hunks)packages/router-core/src/router.ts(2 hunks)packages/router-core/src/ssr/ssr-server.ts(3 hunks)packages/router-core/src/ssr/transformStreamWithRouter.ts(8 hunks)packages/solid-router/src/Asset.tsx(1 hunks)packages/solid-router/src/HeadContent.tsx(3 hunks)packages/solid-router/src/ScriptOnce.tsx(1 hunks)packages/solid-router/src/Scripts.tsx(1 hunks)packages/start-plugin-core/src/global.d.ts(1 hunks)packages/start-plugin-core/src/start-manifest-plugin/plugin.ts(8 hunks)packages/start-plugin-core/src/start-router-plugin/generator-plugins/routes-manifest-plugin.ts(2 hunks)packages/start-server-core/src/router-manifest.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/src/routes/non-nested/named/$baz_.bar.tsx:3-5
Timestamp: 2025-09-22T00:56:49.237Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments (e.g., `$baz_` becomes `baz` in generated types) but should be preserved in base path segments. This is the correct behavior as of the fix in PR #5182.
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts:167-172
Timestamp: 2025-09-22T00:56:53.426Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments during path parsing, but preserved in base path segments. This is the expected behavior implemented in PR #5182.
📚 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/solid-router/src/Asset.tsxpackages/start-server-core/src/router-manifest.tspackages/router-core/src/ssr/ssr-server.tspackages/react-router/src/Asset.tsx
📚 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-plugin-core/src/start-manifest-plugin/plugin.tspackages/start-plugin-core/src/global.d.tspackages/start-plugin-core/src/start-router-plugin/generator-plugins/routes-manifest-plugin.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-plugin-core/src/start-manifest-plugin/plugin.tspackages/start-server-core/src/router-manifest.tspackages/router-core/src/router.tspackages/start-plugin-core/src/start-router-plugin/generator-plugins/routes-manifest-plugin.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/router-core/src/router.ts
🔇 Additional comments (18)
packages/react-router/src/Asset.tsx (1)
146-154: Client-side empty<script>fallback aligns with hydration strategyRendering a no-op
<script>withsuppressHydrationWarningand emptydangerouslySetInnerHTMLis a reasonable way to avoid hydration errors while letting the effect handle real script injection and de‑duping. The control flow and DOM behavior look consistent with the rest of this component.If you see any remaining hydration mismatches around scripts in React apps, it might be worth double‑checking this against a minimal reproduction that exercises both
srcand inline script cases.packages/start-server-core/src/router-manifest.ts (1)
45-52: Preload/asset checks are clear and safeSwitching to explicit
v.preloads/v.assetschecks with length guards keeps the manifest skinny and avoids emitting empty arrays. This keeps the shape minimal without changing semantics.packages/react-router/src/Scripts.tsx (1)
65-76: PrependingserverBufferedScriptpreserves ordering and integrates SSR buffer cleanlyUsing
router.serverSsr.takeBufferedScripts()and unshifting the result intoallScriptsensures SSR-buffered scripts run before route/manifest scripts while keeping the existing selection logic intact. TheRouterManagedTag | undefinedhandling and conditional unshift look correct.If
takeBufferedScripts()is stateful (drains a queue), it’s worth confirming this component only runs in the scenarios you expect (initial SSR + first client pass vs. subsequent navigations) so buffered scripts aren’t accidentally consumed too early or too often.packages/solid-router/src/Asset.tsx (1)
126-129: Solid client-side placeholder<script />matches the hydration workaroundReturning an empty
<script />when!router.isServeris a minimal way to keep the node shape stable for hydration while letting theonMountblock handle actual script injection and de‑duping indocument.head. The flow is consistent with the React variant.Given Solid’s hydration semantics differ from React’s, it would be good to double‑check with an SSR+hydration example that inline and
srcscripts don’t end up executing twice (body vs head) when this placeholder is in play.packages/solid-router/src/Scripts.tsx (1)
54-67: SSR buffered script integration mirrors React implementation correctlyCapturing
serverBufferedScriptfromrouter.serverSsr.takeBufferedScripts()and unshifting it intoallScriptskeeps Solid’s script ordering aligned with the React router: SSR-buffered script first, then route/manifest scripts. The accessor usage (scripts().scripts,assetScripts()) remains intact.As with the React side, please confirm
takeBufferedScripts()’s consumption semantics (single vs repeated calls) match your expectations in streaming scenarios.packages/react-router/src/HeadContent.tsx (1)
116-138: Manifest-drivenpreloadLinkshook cleanly integrates modulepreload tagsThe new
preloadLinksselector that walksrouter.looseRoutesByIdandrouter.ssr?.manifest.routes[route.id]?.preloadsto emitlink rel="modulepreload"tags looks solid. Injecting these ahead oflinksin theuniqBycall gives modulepreload hints priority without changing existing meta/styles/headScripts behavior, and the global dedupe still ensures no duplicatelinktags escape.It’s worth sanity‑checking a couple of routes (root + nested) to confirm their
preloadsentries in the start manifest line up with the expected modulepreload URLs and that they appear in the head in the intended order.Also applies to: 176-183
packages/solid-router/src/HeadContent.tsx (1)
110-132: Solid headpreloadLinksimplementation matches React behavior and manifest usageThe added
preloadLinksselector that resolvesrouter.looseRoutesByIdandrouter.ssr?.manifest.routes[route.id]?.preloadsintolink rel="modulepreload"tags, then includes them via...preloadLinks()in the finaluniqBycall, keeps Solid’s head composition in sync with the React variant while relying on the shared manifest shape. The accessor usage and nonce propagation both look correct.After merging, it’d be useful to run a Solid SSR+hydration example and inspect the
<head>to confirm modulepreload links appear as expected for both leaf and parent routes.Also applies to: 169-177
packages/start-plugin-core/src/start-router-plugin/generator-plugins/routes-manifest-plugin.ts (1)
13-41: Fixchildrentyping to avoid assigningundefinedto a non-optional arrayHere
childrenis declared as always-presentArray<string>:const routes: Record< string, { filePath: string children: Array<string> } >but the per-route entries assign:
children: d.children?.map((childRoute) => childRoute.routePath),which is
string[] | undefined. This will either fail type-checking or silently diverge from the intended globalTSS_ROUTES_MANIFESTtype (which, per the surrounding changes, appears to allowchildrento be optional).I’d align the local type with the global manifest typing, e.g.:
- const routes: Record< - string, - { - filePath: string - children: Array<string> - } - > = { + const routes: Record< + string, + { + filePath: string + children?: Array<string> + } + > = { [rootRouteId]: { filePath: rootRouteNode.fullPath, children: allChildren, }, ...Object.fromEntries( routeNodes.map((d) => { const filePathId = d.routePath return [ filePathId, { filePath: d.fullPath, - children: d.children?.map((childRoute) => childRoute.routePath), + children: d.children?.map((childRoute) => childRoute.routePath), }, ] }), ), }This keeps
childrenpresent where meaningful while matching the updated global manifest contract.Please verify the declared type of
globalThis.TSS_ROUTES_MANIFESTin the shared typings so this plugin stays exactly in sync with that definition.packages/start-plugin-core/src/global.d.ts (1)
3-9: LGTM! Type restructuring is clear.The inline type definition for
TSS_ROUTES_MANIFESTis well-structured and self-documenting. The change from an externalManifesttype to an inlineRecordtype simplifies the global type declaration.packages/router-core/src/ssr/ssr-server.ts (2)
143-155: Good optimization: filtering manifest to only include current routes.The manifest filtering logic ensures that only the routes from the current navigation are sent to the client during dehydration, which reduces payload size and prevents leaking unnecessary route information.
207-222: Well-structured script barrier implementation.The
takeBufferedScriptsmethod now returns a properly structuredRouterManagedTagwith metadata (nonce, className, barrier ID) instead of a raw string. This provides better control over server-side script execution order and integrates cleanly with the stream barrier mechanism.packages/router-core/src/router.ts (1)
86-86: LGTM! Public API updated to support script barrier mechanism.The addition of
RouterManagedTagto the imports and the updated method signatures fortakeBufferedScriptsandliftScriptBarrierproperly expose the new SSR script streaming capabilities in the public API.Also applies to: 759-760
packages/start-plugin-core/src/start-manifest-plugin/plugin.ts (1)
160-202: Manifest refactoring looks good overall.The refactoring from directly using
routeTreeRoutesto a localmanifestobject is well-structured and improves code clarity. The pattern of building upmanifest.routeswith preloads and assets is consistent throughout.packages/router-core/src/ssr/transformStreamWithRouter.ts (5)
22-22: Good addition: exported barrier ID for cross-module coordination.Exporting
TSR_SCRIPT_BARRIER_IDenables consistent barrier identification across SSR modules, which is essential for the script streaming mechanism.
95-100: Excellent addition: timeout protection for injected HTML.The timeout mechanism (defaulting to 60 seconds) prevents the stream from hanging indefinitely if injected HTML promises don't resolve after app rendering finishes. This improves robustness and provides clear error messages.
Also applies to: 240-246
185-192: Cleaner barrier-based detection replaces pattern matching.The barrier-based approach using
TSR_SCRIPT_BARRIER_IDis more explicit and reliable than regex pattern matching for<head>tags. This improves the script lifting mechanism's clarity and maintainability.
53-57: Good enhancement: support for binary chunks.The updated
writemethod now handles both string and non-string (binary) chunks appropriately, providing better flexibility for streaming different content types.
120-120: Correct: incremental decoding enabled.The
{ stream: true }option enables proper incremental decoding of chunks, which is essential for streaming scenarios where chunks may contain partial multi-byte character sequences.
schiller-manuel
force-pushed
the
fix-script-streaming
branch
from
fdeeacb to
f830dff
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/react-router/src/Scripts.tsx (1)
65-69: Simplify with optional chaining and const.The code can be made more concise by using optional chaining and
constinstead ofletwith explicitundefinedinitialization.Apply this diff:
- let serverBufferedScript: RouterManagedTag | undefined = undefined - - if (router.serverSsr) { - serverBufferedScript = router.serverSsr.takeBufferedScripts() - } + const serverBufferedScript = router.serverSsr?.takeBufferedScripts()packages/start-plugin-core/src/start-manifest-plugin/plugin.ts (2)
100-101: Manifest construction fromTSS_ROUTES_MANIFESTis consistent, but consider a clearer failure modeBuilding a local
manifest: Manifest = { routes: {} }fromglobalThis.TSS_ROUTES_MANIFESTand enriching each route withpreloadsandassets(usingjoinURLfor base-path-aware URLs) matches the new manifest-driven flow described in the summary and keeps the manifest shape centralized.The only thing I’d consider is an explicit guard/error if
globalThis.TSS_ROUTES_MANIFESTis missing or malformed;Object.entries(routeTreeRoutes)will currently throw a generic TypeError, whereas a tailored error message would make misconfiguration (e.g. generator plugin not run) much easier to diagnose.Also applies to: 160-201
256-262: Pruning loop iteratesmanifest.routesbut mutatesrouteTreeRoutes— likely mismatchIn the pruning step:
Object.keys(manifest.routes).forEach((routeId) => { const route = manifest.routes[routeId]! const hasAssets = route.assets && route.assets.length > 0 const hasPreloads = route.preloads && route.preloads.length > 0 if (!hasAssets && !hasPreloads) { delete routeTreeRoutes[routeId] } })you iterate over
manifest.routes, but delete fromrouteTreeRouteseven though the finalstartManifestnow exportsroutes: manifest.routes. This means:
- The pruning decision is computed against
manifest.routes.- The actual routes returned in
startManifest.routesare not pruned; only the globalrouteTreeRoutesinput is mutated.If the intent is to prune the final manifest that powers script streaming, this should probably delete from
manifest.routesinstead. If, instead, you intentionally want to mutate the globalTSS_ROUTES_MANIFESTfor some other consumer, it’d be good to add a short comment clarifying that and/or avoid iteratingmanifest.routeshere.If the goal is to prune the emitted manifest, a minimal fix would be:
-Object.keys(manifest.routes).forEach((routeId) => { - const route = manifest.routes[routeId]! +Object.keys(manifest.routes).forEach((routeId) => { + const route = manifest.routes[routeId]! const hasAssets = route.assets && route.assets.length > 0 const hasPreloads = route.preloads && route.preloads.length > 0 if (!hasAssets && !hasPreloads) { - delete routeTreeRoutes[routeId] + delete manifest.routes[routeId] } })Also applies to: 266-266
packages/router-core/src/ssr/transformStreamWithRouter.ts (2)
33-37:ReadablePassthrough.writetyping could be narrowed to expected chunk shapesThe runtime logic to accept both strings and non‑string chunks (enqueuing raw chunks when they’re already binary) is good and makes the passthrough more flexible.
Right now the type is
(chunk: unknown) => void, which hides the actual expectations. If you only intend to support text and byte chunks, consider tightening the type for better call‑site safety:-type ReadablePassthrough = { - stream: ReadableStream - write: (chunk: unknown) => void +type ReadablePassthrough = { + stream: ReadableStream + write: (chunk: string | Uint8Array) => voidYou can always widen later if more shapes are legitimately needed.
Also applies to: 50-57, 60-66
104-111: Timeout wiring is solid; consider safer timeout typing and cleanup infinallyThe
timeoutMsoption and rejection ofinjectedHtmlDonePromiseon timeout/error are nice improvements. Two small polish points:
- Timeout handle type –
NodeJS.Timeoutcan be awkward when this code is type‑checked in environments that use the DOMsetTimeout(where the handle is anumber). UsingReturnType<typeof setTimeout>keeps it portable:- let timeoutHandle: NodeJS.Timeout + let timeoutHandle: ReturnType<typeof setTimeout> | undefined
- Always clear the timeout in
finally– currently the timeout is only cleared on the success path (then). IfinjectedHtmlDonePromiserejects early (e.g. injected HTML error) afteronEndhas scheduled a timeout, that timer will still fire later (harmless but unnecessary). You can centralize cleanup:- injectedHtmlDonePromise - .then(() => { - clearTimeout(timeoutHandle) + injectedHtmlDonePromise + .then(() => { const finalHtml = leftoverHtml + getBufferedRouterStream() + pendingClosingTags finalPassThrough.end(finalHtml) }) .catch((err) => { console.error('Error reading routerStream:', err) finalPassThrough.destroy(err) }) - .finally(stopListeningToInjectedHtml) + .finally(() => { + if (timeoutHandle !== undefined) { + clearTimeout(timeoutHandle) + } + stopListeningToInjectedHtml() + })The rest of the
onEnd/onError/ timeout coordination looks consistent.Also applies to: 163-176, 237-247, 249-253
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/react-router/src/Asset.tsx(1 hunks)packages/react-router/src/HeadContent.tsx(3 hunks)packages/react-router/src/ScriptOnce.tsx(1 hunks)packages/react-router/src/Scripts.tsx(1 hunks)packages/router-core/src/router.ts(2 hunks)packages/router-core/src/ssr/ssr-server.ts(3 hunks)packages/router-core/src/ssr/transformStreamWithRouter.ts(8 hunks)packages/solid-router/src/Asset.tsx(1 hunks)packages/solid-router/src/HeadContent.tsx(3 hunks)packages/solid-router/src/ScriptOnce.tsx(1 hunks)packages/solid-router/src/Scripts.tsx(1 hunks)packages/start-plugin-core/src/global.d.ts(1 hunks)packages/start-plugin-core/src/start-manifest-plugin/plugin.ts(8 hunks)packages/start-plugin-core/src/start-router-plugin/generator-plugins/routes-manifest-plugin.ts(2 hunks)packages/start-server-core/src/router-manifest.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/solid-router/src/Scripts.tsx
- packages/react-router/src/HeadContent.tsx
- packages/start-server-core/src/router-manifest.ts
- packages/start-plugin-core/src/start-router-plugin/generator-plugins/routes-manifest-plugin.ts
- packages/router-core/src/ssr/ssr-server.ts
- packages/start-plugin-core/src/global.d.ts
- packages/react-router/src/ScriptOnce.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 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/solid-router/src/Asset.tsxpackages/start-plugin-core/src/start-manifest-plugin/plugin.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/solid-router/src/Asset.tsxpackages/router-core/src/router.ts
📚 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/solid-router/src/Asset.tsxpackages/start-plugin-core/src/start-manifest-plugin/plugin.tspackages/react-router/src/Asset.tsx
📚 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-plugin-core/src/start-manifest-plugin/plugin.ts
🧬 Code graph analysis (1)
packages/router-core/src/router.ts (3)
packages/react-router/src/index.tsx (1)
RouterManagedTag(78-78)packages/router-core/src/index.ts (1)
RouterManagedTag(73-73)packages/solid-router/src/index.tsx (1)
RouterManagedTag(77-77)
🪛 ast-grep (0.40.0)
packages/react-router/src/Asset.tsx
[warning] 150-150: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
packages/react-router/src/Asset.tsx
[error] 151-151: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
🔇 Additional comments (11)
packages/react-router/src/Asset.tsx (1)
146-153: LGTM! Hydration fix is correct.The change from returning
nullto an empty<script>tag on the client correctly prevents hydration mismatches when the server renders script tags. The use ofdangerouslySetInnerHTMLwith an empty string is safe here—the static analysis warnings are false positives since there's no user-supplied content.Note on static analysis warnings: The
dangerouslySetInnerHTMLwarnings from ast-grep and Biome can be safely ignored in this case, as the content is an empty string literal with no XSS risk.packages/solid-router/src/Asset.tsx (1)
126-128: LGTM! Consistent hydration fix across frameworks.The empty
<script />on the client correctly mirrors the React Router implementation inpackages/react-router/src/Asset.tsx(lines 146-153), ensuring consistent hydration behavior across both React and Solid.js router implementations.packages/solid-router/src/ScriptOnce.tsx (1)
18-18: LGTM! Proper guard for SSR global.The defensive check correctly prevents
ReferenceErrorwhen$_TSRis not defined. Usingtypeofis the right approach for checking undeclared globals, and the optional chaining (?.) provides additional safety.packages/solid-router/src/HeadContent.tsx (1)
110-133: LGTM! Improved naming clarity.The rename from
preloadMetatopreloadLinksis semantically accurate since these are<link rel="modulepreload">elements, not meta tags. All usages are updated consistently throughout the function.Also applies to: 173-173
packages/react-router/src/Scripts.tsx (2)
67-69: Verify idempotency or add memoization fortakeBufferedScripts().Calling
takeBufferedScripts()directly in the component body means it will execute on every render. If this method is destructive (i.e., it clears a buffer after reading, as the "take" prefix suggests), subsequent re-renders could produce inconsistent results or hydration mismatches.Consider wrapping the call in
useMemoto ensure it's only called once:- if (router.serverSsr) { - serverBufferedScript = router.serverSsr.takeBufferedScripts() - } + const serverBufferedScript = useMemo(() => { + return router.serverSsr?.takeBufferedScripts() + }, [router.serverSsr])Alternatively, verify that
takeBufferedScripts()is idempotent (returns the same value on repeated calls) or thatrouter.serverSsronly exists during the initial server render and not during client hydration or re-renders.
73-76: Logic correctly prepends server-buffered scripts.The conditional check and
unshiftoperation correctly ensure that server-buffered scripts render before other scripts, aligning with the intended SSR script ordering.packages/router-core/src/router.ts (1)
86-86: LGTM: Type import added for ServerSsr interface.The addition of
RouterManagedTagto the import statement is necessary for the updatedtakeBufferedScriptsreturn type below.packages/start-plugin-core/src/start-manifest-plugin/plugin.ts (2)
9-26: Typed CSS asset handling viaRouterManagedTaglooks solidImporting
ManifestandRouterManagedTagand threadingRouterManagedTag[]throughgetCSSRecursively(cache/result) keeps this plugin aligned with router-core’s manifest shape and makes the CSS assets explicitly typed. The recursion guard withvisitedplus the per-chunk cache should also prevent the previously reported infinite-recursion issues ingetCSSRecursivelywithout changing behavior.
172-177: Preload and asset population plusrecurseRoutemigration look correct
- Using
joinURL(resolvedStartConfig.viteAppBase, ...)for both the entry chunk andchunk.importsensures preloads respect the app base path and avoids double-slash issues.- Computing
assetsviagetCSSRecursivelyand storing them on each route asassets: RouterManagedTag[]is consistent with how downstream code expects CSS tags to be represented.- Initializing
manifest.routes[rootRouteId](fallback{}) before assigningpreloadsand mergingassetsguarantees the root route always has a valid preloads list and CSS, while preserving any existing properties (likechildren) from the route manifest.- Updating
recurseRouteto look up children throughmanifest.routes[child]!and starting recursion frommanifest.routes[rootRouteId]keeps the preloads-deduplication behavior the same as before the refactor, just against the new manifest container.Overall this block hangs together well and aligns with the manifest-centric design.
Also applies to: 186-191, 193-201, 208-210, 225-227, 247-248, 253-253
packages/router-core/src/ssr/transformStreamWithRouter.ts (2)
142-161: Injected HTML buffering and script barrier lift look consistent with streaming flowSwitching
handleInjectedHtmlto buffer whileisAppRenderingistrueand write directly tofinalPassThroughonce rendering is done, combined with holdingpendingClosingTagsuntilinjectedHtmlDonePromisesettles, preserves the intended ordering of injected HTML before</body></html>.The
TSR_SCRIPT_BARRIER_IDdetection andstreamBarrierLiftedguard are also straightforward and low‑overhead (singleincludesscan until the barrier is seen), and callingrouter.serverSsr!.liftScriptBarrier()exactly once per stream matches the intended barrier semantics.No changes requested here.
Also applies to: 181-193
22-22: ExportedTSR_SCRIPT_BARRIER_IDconstant is a good centralization pointDefining the barrier marker as a single exported constant makes it easier to keep the server‑side tag emitter and this stream transformer in sync and avoids magic strings scattered across the SSR codepath.
| takeBufferedScripts: () => RouterManagedTag | undefined | ||
| liftScriptBarrier: () => void |
There was a problem hiding this comment.
Breaking API change: Return type updated to structured wrapper.
The takeBufferedScripts return type has changed from string | undefined to RouterManagedTag | undefined. This is a breaking change for consumers that expect a plain string, though it provides better type safety by wrapping script content with metadata (nonce, className, id).
The new liftScriptBarrier method extends the public API surface for SSR script barrier control.
Optional: Consider adding JSDoc documentation for the new method.
Adding brief JSDoc comments for liftScriptBarrier would help consumers understand when and how to use this new API:
dehydrate: () => Promise<void>
takeBufferedScripts: () => RouterManagedTag | undefined
+ /**
+ * Lifts the script barrier to allow subsequent scripts to be streamed.
+ * Used in SSR to control script ordering and streaming behavior.
+ */
liftScriptBarrier: () => voidCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
packages/router-core/src/router.ts lines 759-760: The change updates
takeBufferedScripts from returning string|undefined to
RouterManagedTag|undefined (breaking consumers) and introduces liftScriptBarrier
without documentation; to fix, restore backwards-compatible behavior by
providing either (a) an overload or adapter that preserves the original
signature (keep takeBufferedScripts returning string|undefined and add
takeBufferedScriptsManaged or similar that returns RouterManagedTag), or (b)
keep the new return type but add a helper method that extracts the string
content from RouterManagedTag for existing callers, and add a brief JSDoc
comment above liftScriptBarrier explaining its purpose, intended usage, and SSR
barrier semantics so consumers know when to call it.
| const textDecoder = new TextDecoder() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Avoid sharing a streaming TextDecoder instance across concurrent SSR streams
textDecoder is module-scoped and decodeChunk now calls decode(chunk, { stream: true }). With multiple transformStreamWithRouter invocations running concurrently, this shares the decoder’s streaming state across independent requests, so multi‑byte sequences split across chunks can get mixed between streams and corrupt the HTML output.
You should keep the { stream: true } behavior (it’s correct for cross‑chunk decoding) but instantiate a TextDecoder per transform (or pass it in), so each stream has its own decoder state.
A minimal fix could look like:
-const textDecoder = new TextDecoder()
-
-function decodeChunk(chunk: unknown): string {
- if (chunk instanceof Uint8Array) {
- return textDecoder.decode(chunk, { stream: true })
- }
- return String(chunk)
-}
+function decodeChunk(chunk: unknown, decoder: TextDecoder): string {
+ if (chunk instanceof Uint8Array) {
+ return decoder.decode(chunk, { stream: true })
+ }
+ return String(chunk)
+}
@@
-export function transformStreamWithRouter(
+export function transformStreamWithRouter(
router: AnyRouter,
appStream: ReadableStream,
opts?: {
timeoutMs?: number
},
) {
+ const textDecoder = new TextDecoder()
const finalPassThrough = createPassthrough()
@@
- onData: (chunk) => {
- const text = decodeChunk(chunk.value)
+ onData: (chunk) => {
+ const text = decodeChunk(chunk.value, textDecoder)Also applies to: 118-123, 178-181
🏁 Script executed:
#!/bin/bash
# View the target file to inspect current TextDecoder implementation
cat packages/router-core/src/ssr/transformStreamWithRouter.tsLength of output: 6566
🏁 Script executed:
#!/bin/bash
# Search for all invocations of transformStreamWithRouter to understand concurrency patterns
rg -n 'transformStreamWithRouter' --type ts --type tsx -C 3Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check if TextDecoder appears elsewhere or if there's any synchronization mechanism
rg -n 'TextDecoder' packages/router-core/src/ssr/ -C 2Length of output: 426
Module-scoped TextDecoder with { stream: true } causes data corruption across concurrent SSR requests
Verified: The issue is real and critical. Line 31 defines const textDecoder = new TextDecoder() at module scope, and the decodeChunk function at lines 119–123 calls textDecoder.decode(chunk, { stream: true }). When transformStreamWithRouter runs concurrently for multiple requests, they all share this single decoder instance. TextDecoder's { stream: true } mode maintains internal state for incomplete multi-byte UTF-8 sequences; concurrent streams will corrupt this state, causing mangled character decoding and HTML output corruption.
Each transformStreamWithRouter invocation must have its own TextDecoder instance. Move the instantiation inside the function so each stream maintains independent decoder state.
-const textDecoder = new TextDecoder()
-
type ReadablePassthrough = {
stream: ReadableStream
write: (chunk: unknown) => void
end: (chunk?: string) => void
destroy: (error: unknown) => void
destroyed: boolean
}
function createPassthrough() {
// ...
}
export function transformStreamWithRouter(
router: AnyRouter,
appStream: ReadableStream,
opts?: {
timeoutMs?: number
},
) {
+ const textDecoder = new TextDecoder()
const finalPassThrough = createPassthrough()📝 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.
| const textDecoder = new TextDecoder() | |
| type ReadablePassthrough = { | |
| stream: ReadableStream | |
| write: (chunk: unknown) => void | |
| end: (chunk?: string) => void | |
| destroy: (error: unknown) => void | |
| destroyed: boolean | |
| } | |
| function createPassthrough() { | |
| // ... | |
| } | |
| export function transformStreamWithRouter( | |
| router: AnyRouter, | |
| appStream: ReadableStream, | |
| opts?: { | |
| timeoutMs?: number | |
| }, | |
| ) { | |
| const textDecoder = new TextDecoder() | |
| const finalPassThrough = createPassthrough() |
🤖 Prompt for AI Agents
In packages/router-core/src/ssr/transformStreamWithRouter.ts around lines 31–32,
a module-scoped TextDecoder is created and later used with { stream: true },
which shares internal state across concurrent SSR requests and corrupts
multi-byte UTF-8 sequences; to fix, remove the module-level TextDecoder and
instead instantiate a new TextDecoder() inside transformStreamWithRouter (or in
the per-request scope where decodeChunk runs) so each invocation gets its own
decoder; update decodeChunk to reference the local decoder instance and ensure
no shared decoder remains at module scope.
697cd22 to
76640fa
Compare
Summary by CodeRabbit
Bug Fixes
Improvements