feat(start): support rsbuild iife client output#7477
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors manifest to ServerManifest (preloads, scripts, css), adds ScriptFormat (module|iife), changes transformAsset kinds to 'script'/'stylesheet'/'css-url', implements rsbuild/Rspack per-environment server-function metadata persistence/replay, and updates routers, server SSR, tests, docs, and e2e suites. ChangesServerManifest model + rsbuild server-fn persistence
Estimated code review effort Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
github-actions
Bot
added
documentation
Contributor
|
View your CI Pipeline Execution ↗ for commit 20dde7f
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview9 package(s) bumped directly, 14 bumped as dependents. 🟩 Patch bumps
|
Contributor
Bundle Size Benchmarks
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/solid-router/src/Scripts.tsx (1)
11-11: ⚡ Quick winReplace
Array<any>with typed match inputs.Using
anyhere weakens safety exactly where manifest route/script fields are accessed.As per coding guidelines, "
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety".Also applies to: 38-38
🤖 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 `@packages/solid-router/src/Scripts.tsx` at line 11, Replace the loose Array<any> on the getAssetScripts parameter with a proper typed Match[] (or the exact route match interface used across the router) so callers and property accesses (e.g., manifest route/script fields) get compile-time safety; update the function signature getAssetScripts(matches: Match[]) and any other similar signatures in this file to use the shared Match/RouteManifest type instead of any, and adjust imports/types where needed to reference the correct match/interface.packages/vue-router/src/HeadContent.tsx (1)
12-38: ⚡ Quick winTighten the attribute matcher type.
Record<string, any>opts this hydration-matching path out of strict checking. UsingRouterManagedTag['attrs']or a narrow attribute-value union here will keep bad attr shapes from silently flowing into reconciliation. As per coding guidelines**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.🤖 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 `@packages/vue-router/src/HeadContent.tsx` around lines 12 - 38, The attrsMatch function currently accepts a permissive Record<string, any> which bypasses TS strict checks; change its parameter type to the specific RouterManagedTag['attrs'] (or a narrow union of allowed attribute value types) so invalid attr shapes are caught at compile time, update any callers that pass loose objects to satisfy the new type, and ensure the logic within attrsMatch still handles undefined/false/null/true and string conversions as before.packages/vue-router/src/Scripts.tsx (1)
22-67: ⚡ Quick winStop threading
anythrough script collection.These helpers sit on the manifest-to-DOM boundary, so
Array<any>drops exactly the type information strict mode should preserve. Reusing the match type fromrouter.stores.matcheshere would keep route access and script normalization checked end-to-end. As per coding guidelines**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.🤖 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 `@packages/vue-router/src/Scripts.tsx` around lines 22 - 67, The helpers getAssetScripts and getScripts currently use Array<any> for matches; change their signatures and callers to use the concrete match type from router.stores.matches (e.g. the array item type: typeof router.stores.matches[number] or a named Match type exported from the store) so script access is statically checked; update both getAssetScripts(matches: ...) and getScripts(matches: ...) and the computed calls that pass matches.value to accept that match type, and ensure any internal usages (routeId, scripts, etc.) conform to the actual match shape so TypeScript strict mode verifies manifest-to-DOM boundaries.
🤖 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 `@packages/react-router/src/headContentUtils.tsx`:
- Around line 105-153: The manifest-derived tag arrays manifestCssTags and
preloadLinks can contain duplicates when multiple matches reference the same
chunk; dedupe before appending to the final head list by keying on a stable
identifier (e.g. attrs.href for link tags or a stringified attrs object for
preload/link entries). Update the logic that builds manifestCssTags (using
resolveManifestCssLink) and preloadLinks (using getScriptPreloadAttrs) to record
seen keys in a Set and only push a RouterManagedTag when its key is not already
present; apply the same dedup approach to the other mentioned blocks (around the
other ranges that push manifest-derived tags) so SSR and client head lists never
emit duplicate stylesheet/preload tags.
In `@packages/router-core/src/ssr/ssr-server.ts`:
- Around line 462-478: The code calls stripInlinedStylesheetAssets(manifest,
nextFilteredRoutes) after populating nextFilteredRoutes with unmatched routes
when includeUnmatchedRouteAssets is true, which causes inlinable CSS for
unmatched routes to be stripped; fix by ensuring stripInlinedStylesheetAssets is
only applied to the subset of routes that are current matches (or otherwise
known to be inlined) — e.g., compute a routesToStrip object from
nextFilteredRoutes that contains only matched route IDs (using the current match
list/variable used elsewhere in this flow) and pass routesToStrip to
stripInlinedStylesheetAssets(manifest, routesToStrip), leaving unmatched route
entries in nextFilteredRoutes untouched before assignment to filteredRoutes.
In `@packages/solid-router/src/headContentUtils.tsx`:
- Around line 213-216: The code currently pushes preloadLinks() and
manifestCssTags() directly into next, which bypasses deduplication; replace
those direct pushes with calls to appendUniqueUserTags to dedupe against
existing tags (i.e., use appendUniqueUserTags(next, preloadLinks()) instead of
next.push(...preloadLinks()) and appendUniqueUserTags(next, manifestCssTags())
instead of next.push(...manifestCssTags())), keeping the existing calls that
already dedupe links() and styles().
In `@packages/solid-router/src/Scripts.tsx`:
- Line 70: Keep the nullish coalescing in the server check (do not change
isServer ?? router.isServer) and replace the overly broad Array<any> in the
getAssetScripts and getScripts signatures with the concrete match type used by
the router stores (a type/interface that includes at least routeId and scripts);
update the function params for getAssetScripts(matches: /* router match type */)
and getScripts(matches: /* router match type */) and adjust any internal
property accesses to use that typed shape so the TypeScript strict checks pass
while preserving the isServer ?? router.isServer server gate.
In `@packages/vue-router/src/Scripts.tsx`:
- Line 91: The checks using the nullish coalescing (isServer ?? router.isServer)
short-circuit when imported isServer is false and thus ignore router.isServer;
update both occurrences in Scripts.tsx to use a boolean OR so router.isServer is
honored when isServer is false (replace (isServer ?? router.isServer) with
(isServer || router.isServer) in the if conditions that reference isServer,
including the one combined with mounted).
---
Nitpick comments:
In `@packages/solid-router/src/Scripts.tsx`:
- Line 11: Replace the loose Array<any> on the getAssetScripts parameter with a
proper typed Match[] (or the exact route match interface used across the router)
so callers and property accesses (e.g., manifest route/script fields) get
compile-time safety; update the function signature getAssetScripts(matches:
Match[]) and any other similar signatures in this file to use the shared
Match/RouteManifest type instead of any, and adjust imports/types where needed
to reference the correct match/interface.
In `@packages/vue-router/src/HeadContent.tsx`:
- Around line 12-38: The attrsMatch function currently accepts a permissive
Record<string, any> which bypasses TS strict checks; change its parameter type
to the specific RouterManagedTag['attrs'] (or a narrow union of allowed
attribute value types) so invalid attr shapes are caught at compile time, update
any callers that pass loose objects to satisfy the new type, and ensure the
logic within attrsMatch still handles undefined/false/null/true and string
conversions as before.
In `@packages/vue-router/src/Scripts.tsx`:
- Around line 22-67: The helpers getAssetScripts and getScripts currently use
Array<any> for matches; change their signatures and callers to use the concrete
match type from router.stores.matches (e.g. the array item type: typeof
router.stores.matches[number] or a named Match type exported from the store) so
script access is statically checked; update both getAssetScripts(matches: ...)
and getScripts(matches: ...) and the computed calls that pass matches.value to
accept that match type, and ensure any internal usages (routeId, scripts, etc.)
conform to the actual match shape so TypeScript strict mode verifies
manifest-to-DOM boundaries.
🪄 Autofix (Beta)
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
Run ID: e1c7592f-439e-44d6-82b6-0d4c5714d33d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (68)
.changeset/gentle-pandas-clap.md.changeset/slow-badgers-remember.mddocs/router/guide/document-head-management.mddocs/start/framework/react/guide/cdn-asset-urls.mddocs/start/framework/react/guide/early-hints.mde2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-start/basic/package.jsone2e/react-start/basic/playwright.config.tse2e/react-start/basic/start-mode-config.tse2e/react-start/basic/tests/client-output.spec.tse2e/react-start/dev-ssr-styles/package.jsone2e/react-start/dev-ssr-styles/tests/app.spec.tse2e/react-start/server-functions/package.jsone2e/react-start/server-functions/playwright.rsbuild-cache.config.tse2e/react-start/server-functions/rsbuild.config.tse2e/react-start/server-functions/tests/rsbuild-cache.spec.tse2e/react-start/start-manifest/tests/start-manifest.spec.tse2e/react-start/transform-asset-urls/src/routes/__root.tsxe2e/react-start/transform-asset-urls/src/server.tse2e/react-start/transform-asset-urls/tests/transform-asset-urls.spec.tse2e/solid-start/start-manifest/tests/start-manifest.spec.tse2e/vue-start/start-manifest/tests/start-manifest.spec.tspackages/react-router/src/Asset.tsxpackages/react-router/src/HeadContent.dev.tsxpackages/react-router/src/Scripts.tsxpackages/react-router/src/headContentUtils.tsxpackages/react-router/tests/Scripts.test.tsxpackages/react-start-rsc/src/serialization.server.tspackages/router-core/src/index.tspackages/router-core/src/manifest.tspackages/router-core/src/router.tspackages/router-core/src/ssr/createRequestHandler.tspackages/router-core/src/ssr/ssr-server.tspackages/router-core/tests/hydrate.test.tspackages/router-core/tests/manifest.test.tspackages/router-core/tests/ssr-server-manifest.test.tspackages/solid-router/src/HeadContent.dev.tsxpackages/solid-router/src/Scripts.tsxpackages/solid-router/src/headContentUtils.tsxpackages/solid-router/tests/Scripts.test.tsxpackages/start-plugin-core/src/rsbuild/COMPILER_ARCHITECTURE.mdpackages/start-plugin-core/src/rsbuild/planning.tspackages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/src/rsbuild/schema.tspackages/start-plugin-core/src/rsbuild/start-compiler-host.tspackages/start-plugin-core/src/rsbuild/start-compiler-metadata-loader.tspackages/start-plugin-core/src/rsbuild/start-compiler-metadata.tspackages/start-plugin-core/src/rsbuild/virtual-modules.tspackages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.tspackages/start-plugin-core/src/vite/start-manifest-plugin/plugin.tspackages/start-plugin-core/tests/rsbuild-output-directory.test.tspackages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.tspackages/start-plugin-core/vite.config.tspackages/start-server-core/src/early-hints.tspackages/start-server-core/src/finalManifest.tspackages/start-server-core/src/request-handler.tspackages/start-server-core/src/router-manifest.tspackages/start-server-core/src/tanstack-start.d.tspackages/start-server-core/src/transformAssetUrls.tspackages/start-server-core/tests/early-hints.test.tspackages/start-server-core/tests/finalManifest.test.tspackages/start-server-core/tests/transformAssets.test.tspackages/start-storage-context/src/async-local-storage.tspackages/vue-router/src/HeadContent.dev.tsxpackages/vue-router/src/HeadContent.tsxpackages/vue-router/src/Scripts.tsxpackages/vue-router/src/headContentUtils.tsxpackages/vue-router/tests/Scripts.test.tsx
Comment on lines
105
to
+153
| const manifest = router.ssr?.manifest | ||
| const assetLinks = matches | ||
| .map((match) => manifest?.routes[match.routeId]?.assets ?? []) | ||
| .filter(Boolean) | ||
| .flat(1) | ||
| .flatMap((asset): Array<RouterManagedTag> => { | ||
| if (asset.tag === 'link') { | ||
| if (isInlinableStylesheet(manifest, asset)) { | ||
| return [] | ||
| } | ||
|
|
||
| return [ | ||
| { | ||
| tag: 'link', | ||
| attrs: { | ||
| ...asset.attrs, | ||
| crossOrigin: | ||
| getAssetCrossOrigin(assetCrossOrigin, 'stylesheet') ?? | ||
| asset.attrs?.crossOrigin, | ||
| suppressHydrationWarning: true, | ||
| nonce, | ||
| }, | ||
| const manifestCssTags: Array<RouterManagedTag> = [] | ||
| if (manifest) { | ||
| matches.forEach((match) => { | ||
| const css = manifest.routes[match.routeId]?.css | ||
| css?.forEach((link) => { | ||
| const resolvedLink = resolveManifestCssLink(link) | ||
| manifestCssTags.push({ | ||
| tag: 'link', | ||
| attrs: { | ||
| rel: 'stylesheet', | ||
| ...resolvedLink, | ||
| crossOrigin: | ||
| getAssetCrossOrigin(assetCrossOrigin, 'stylesheet') ?? | ||
| resolvedLink.crossOrigin, | ||
| suppressHydrationWarning: true, | ||
| nonce, | ||
| }, | ||
| ] | ||
| } | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| if (asset.tag === 'style') { | ||
| return [ | ||
| { | ||
| tag: 'style', | ||
| attrs: { | ||
| ...asset.attrs, | ||
| nonce, | ||
| }, | ||
| children: asset.children, | ||
| ...(asset.inlineCss ? { inlineCss: true as const } : {}), | ||
| }, | ||
| ] | ||
| } | ||
| if (manifest.inlineStyle) { | ||
| manifestCssTags.push({ | ||
| tag: 'style', | ||
| attrs: { | ||
| ...manifest.inlineStyle.attrs, | ||
| nonce, | ||
| }, | ||
| children: manifest.inlineStyle.children, | ||
| inlineCss: true, | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| return [] | ||
| const preloadLinks: Array<RouterManagedTag> = [] | ||
| if (manifest) { | ||
| matches.forEach((match) => { | ||
| manifest.routes[match.routeId]?.preloads?.forEach((preload) => { | ||
| preloadLinks.push({ | ||
| tag: 'link', | ||
| attrs: { | ||
| ...getScriptPreloadAttrs(manifest, preload, assetCrossOrigin), | ||
| nonce, | ||
| }, | ||
| }) | ||
| }) | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Restore deduplication for manifest-derived head tags.
The previous implementation deduped the combined head tag list, but these paths now append preloadLinks and manifestCssTags directly. If the same chunk is referenced by multiple active matches, we'll emit duplicate preload/stylesheet tags on both SSR and the client.
Suggested direction
+function appendUniqueTags(
+ target: Array<RouterManagedTag>,
+ tags: Array<RouterManagedTag>,
+) {
+ const seen = new Set(target.map((tag) => JSON.stringify(tag)))
+ for (const tag of tags) {
+ const key = JSON.stringify(tag)
+ if (seen.has(key)) continue
+ seen.add(key)
+ target.push(tag)
+ }
+}
+
const tags: Array<RouterManagedTag> = []
appendUniqueUserTags(tags, resultMeta)
- tags.push(...preloadLinks)
+ appendUniqueTags(tags, preloadLinks)
appendUniqueUserTags(tags, constructedLinks)
- tags.push(...manifestCssTags)
+ appendUniqueTags(tags, manifestCssTags)
appendUniqueUserTags(tags, styles)
appendUniqueUserTags(tags, headScripts)
return tagsAlso applies to: 179-186, 310-378, 421-428
🤖 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 `@packages/react-router/src/headContentUtils.tsx` around lines 105 - 153, The
manifest-derived tag arrays manifestCssTags and preloadLinks can contain
duplicates when multiple matches reference the same chunk; dedupe before
appending to the final head list by keying on a stable identifier (e.g.
attrs.href for link tags or a stringified attrs object for preload/link
entries). Update the logic that builds manifestCssTags (using
resolveManifestCssLink) and preloadLinks (using getScriptPreloadAttrs) to record
seen keys in a Set and only push a RouterManagedTag when its key is not already
present; apply the same dedup approach to the other mentioned blocks (around the
other ranges that push manifest-derived tags) so SSR and client head lists never
emit duplicate stylesheet/preload tags.
Collaborator
Author
There was a problem hiding this comment.
this deduplication is supposed to happen at build time already. check this!
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Good question — let me check whether the manifest builder already deduplicates cross-route entries at build time before concluding.
🏁 Script executed:
#!/bin/bash
# Look at the deduplication logic in manifestBuilder.ts
rg -n "dedupe\|dedup\|unique\|preloads\|css" packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts | head -80
echo "---"
# Check the nested/ancestor dedup logic
ast-grep --pattern 'route.preloads' packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts
echo "---"
# Look for where preloads and css are deduplicated in build routes
rg -n "preloads\|\.css" packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts | head -60
Length of output: 380
🏁 Script executed:
#!/bin/bash
# Get full context around the dedup logic for preloads and css in manifestBuilder.ts
sed -n '620,720p' packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts
echo "=== CSS dedup ==="
rg -n "css\|dedupe\|dedup" packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts | head -60
Length of output: 2641
🏁 Script executed:
#!/bin/bash
# See more of dedupeNestedRouteManifestEntries - the recursive part and how it's called
sed -n '720,800p' packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts
echo "=== How dedupeNestedRouteManifestEntries is called ==="
rg -n "dedupeNestedRouteManifestEntries" packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts
Length of output: 1742
@schiller-manuel, you're right — the build-time deduplication fully covers this. dedupeNestedRouteManifestEntries (called from rootRouteId at line 237 of manifestBuilder.ts) does a parent-first recursive traversal of the route tree with shared seenPreloads, seenScripts, and seenStylesheets sets. Any entry already present in an ancestor route is stripped from child routes before the manifest is written. So at runtime, iterating over all active matches (root → layout → leaf) will never yield the same preload or CSS entry twice.
My concern was unfounded — no runtime deduplication guard is needed here. Apologies for the noise!
🐇 (`ﾟ▽ﾟ)
✏️ Learnings added
Learnt from: schiller-manuel
Repo: TanStack/router PR: 7477
File: packages/react-router/src/headContentUtils.tsx:105-153
Timestamp: 2026-05-23T22:09:30.386Z
Learning: In TanStack Router's `packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts`, `dedupeNestedRouteManifestEntries` performs a parent-first recursive traversal of the route tree at build time using shared `seenPreloads`, `seenScripts`, and `seenStylesheets` Sets. It strips any preload/CSS/script entry from a child route that is already present in an ancestor route. This means that at runtime, iterating `manifest.routes[match.routeId]?.preloads` / `.css` / `.scripts` over all active matches will never produce cross-match duplicates — no additional runtime deduplication is needed for these manifest-derived head tags.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+213
to
+216
| next.push(...preloadLinks()) | ||
| appendUniqueUserTags(next, links()) | ||
| next.push(...manifestCssTags()) | ||
| appendUniqueUserTags(next, styles()) |
Contributor
There was a problem hiding this comment.
Preload and manifest CSS tags are no longer deduped.
next.push(...preloadLinks()) and next.push(...manifestCssTags()) can emit duplicates across active matches (e.g., shared root+child CSS), which regresses previous global dedupe behavior.
Suggested fix
- next.push(...preloadLinks())
+ appendUniqueUserTags(next, preloadLinks())
appendUniqueUserTags(next, links())
- next.push(...manifestCssTags())
+ appendUniqueUserTags(next, manifestCssTags())📝 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.
Suggested change
| next.push(...preloadLinks()) | |
| appendUniqueUserTags(next, links()) | |
| next.push(...manifestCssTags()) | |
| appendUniqueUserTags(next, styles()) | |
| appendUniqueUserTags(next, preloadLinks()) | |
| appendUniqueUserTags(next, links()) | |
| appendUniqueUserTags(next, manifestCssTags()) | |
| appendUniqueUserTags(next, styles()) |
🤖 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 `@packages/solid-router/src/headContentUtils.tsx` around lines 213 - 216, The
code currently pushes preloadLinks() and manifestCssTags() directly into next,
which bypasses deduplication; replace those direct pushes with calls to
appendUniqueUserTags to dedupe against existing tags (i.e., use
appendUniqueUserTags(next, preloadLinks()) instead of
next.push(...preloadLinks()) and appendUniqueUserTags(next, manifestCssTags())
instead of next.push(...manifestCssTags())), keeping the existing calls that
already dedupe links() and styles().
| allScripts.push(asset) | ||
| } | ||
| } | ||
| if ((isServer ?? router.isServer) && router.serverSsr) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ls -la
rg -n "isServer" packages/vue-router/src/Scripts.tsx
sed -n '1,220p' packages/vue-router/src/Scripts.tsxRepository: TanStack/router
Length of output: 7157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '70,160p' packages/vue-router/src/Scripts.tsxRepository: TanStack/router
Length of output: 1677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '80,150p' packages/vue-router/src/Scripts.tsxRepository: TanStack/router
Length of output: 1428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# show around both referenced lines, and also find exact line numbers for the snippet
nl -ba packages/vue-router/src/Scripts.tsx | sed -n '80,140p'Repository: TanStack/router
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# search for other occurrences of "?? router.isServer" and "router.isServer"
rg -n "\?\?\s*router\.isServer" packages/vue-router/src/Scripts.tsx
rg -n "router\.isServer" packages/vue-router/src/Scripts.tsxRepository: TanStack/router
Length of output: 335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# locate how isServer is computed/declared in this file
nl -ba packages/vue-router/src/Scripts.tsx | sed -n '1,110p'Repository: TanStack/router
Length of output: 102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# also inspect the rest for context after line 140
nl -ba packages/vue-router/src/Scripts.tsx | sed -n '140,240p'Repository: TanStack/router
Length of output: 102
Honor router.isServer when imported isServer is false (avoid ?? short-circuiting).
In packages/vue-router/src/Scripts.tsx, both checks use if ((isServer ?? router.isServer) && router.serverSsr) and if (mounted || ((isServer ?? router.isServer) && router.serverSsr)). With ??, when imported isServer is false, the fallback never runs, so router.isServer is ignored—sending script handling down the client path (skipping server buffered scripts and deferring assetScripts) even in SSR configurations where router.isServer is true.
Suggested fix
- if ((isServer ?? router.isServer) && router.serverSsr) {
+ if ((isServer || router.isServer) && router.serverSsr) {
@@
- if (mounted || ((isServer ?? router.isServer) && router.serverSsr)) {
+ if (mounted || ((isServer || router.isServer) && router.serverSsr)) {🤖 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 `@packages/vue-router/src/Scripts.tsx` at line 91, The checks using the nullish
coalescing (isServer ?? router.isServer) short-circuit when imported isServer is
false and thus ignore router.isServer; update both occurrences in Scripts.tsx to
use a boolean OR so router.isServer is honored when isServer is false (replace
(isServer ?? router.isServer) with (isServer || router.isServer) in the if
conditions that reference isServer, including the one combined with mounted).
Merging this PR will not alter performance
Comparing 
Footnotes
|
schiller-manuel
force-pushed
the
rsbuild-iife-cache
branch
from
9461e80 to
a9bf7f3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/vue-router/src/Scripts.tsx (1)
22-22: 💤 Low valueConsider using proper match type instead of
Array<any>.Both
getAssetScriptsandgetScriptsuseArray<any>for thematchesparameter. For better type safety, consider importing and usingAnyRouteMatchfrom@tanstack/router-core, similar to how it's done in the Solid router implementation.Suggested type improvement
+import type { AnyRouteMatch, RouterManagedTag } from '`@tanstack/router-core`' -import type { RouterManagedTag } from '`@tanstack/router-core`' - const getAssetScripts = (matches: Array<any>) => { + const getAssetScripts = (matches: Array<AnyRouteMatch>) => { - const getScripts = (matches: Array<any>): Array<RouterManagedTag> => + const getScripts = (matches: Array<AnyRouteMatch>): Array<RouterManagedTag> =>🤖 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 `@packages/vue-router/src/Scripts.tsx` at line 22, Replace the loose Array<any> parameter types with the proper route-match type: import AnyRouteMatch from `@tanstack/router-core` and change the signatures of getAssetScripts and getScripts to use Array<AnyRouteMatch> (or AnyRouteMatch[]); update any internal uses that assume the shape of match objects to the stronger type so TypeScript can validate fields accessed on match elements.packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts (1)
149-160: ⚡ Quick winAvoid
anyinnormalizeAttrsto preserve strict typing.
Record<string, any>weakens compile-time guarantees for manifest script attrs. A narrow union (orunknown) keeps this strict-safe without changing behavior.Suggested change
-function normalizeAttrs(attrs: Record<string, any> | undefined) { +type ScriptAttrValue = string | number | boolean | null | undefined + +function normalizeAttrs( + attrs: Record<string, ScriptAttrValue> | undefined, +) { if (!attrs) { return null }As per coding guidelines: "
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety".🤖 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 `@packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts` around lines 149 - 160, normalizeAttrs currently accepts Record<string, any>, weakening type safety; change its signature to accept Record<string, unknown> | undefined and return Record<string, unknown> | null, then update internal handling to preserve runtime behavior: use Object.entries(attrs as Record<string, unknown>) so entries are typed [string, unknown], sort by key the same way, and return Object.fromEntries(entries) typed as Record<string, unknown>. Ensure any places calling normalizeAttrs are compatible with the new narrower type (or cast at call sites) and add any minimal runtime checks/casts for values if callers expect specific shapes.
🤖 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/transform-asset-urls/tests/transform-asset-urls.spec.ts`:
- Around line 48-53: The current regex that builds scriptPreloads assumes
rel="modulepreload" appears before href, making the test order-sensitive; update
the match logic in the test (the scriptPreloads extraction and the subsequent
href extraction) to be attribute-order-agnostic — e.g. match elements that
contain rel="modulepreload" regardless of attribute order (use a single regex
with a lookahead for rel="modulepreload" and then capture href, or a regex that
alternates both orderings) and then extract the href from the correct capture
group (update the loop that uses match.match(...) accordingly).
---
Nitpick comments:
In `@packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts`:
- Around line 149-160: normalizeAttrs currently accepts Record<string, any>,
weakening type safety; change its signature to accept Record<string, unknown> |
undefined and return Record<string, unknown> | null, then update internal
handling to preserve runtime behavior: use Object.entries(attrs as
Record<string, unknown>) so entries are typed [string, unknown], sort by key the
same way, and return Object.fromEntries(entries) typed as Record<string,
unknown>. Ensure any places calling normalizeAttrs are compatible with the new
narrower type (or cast at call sites) and add any minimal runtime checks/casts
for values if callers expect specific shapes.
In `@packages/vue-router/src/Scripts.tsx`:
- Line 22: Replace the loose Array<any> parameter types with the proper
route-match type: import AnyRouteMatch from `@tanstack/router-core` and change the
signatures of getAssetScripts and getScripts to use Array<AnyRouteMatch> (or
AnyRouteMatch[]); update any internal uses that assume the shape of match
objects to the stronger type so TypeScript can validate fields accessed on match
elements.
🪄 Autofix (Beta)
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
Run ID: f9e95558-0f0d-4cb8-9b81-d742657331f2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (74)
.changeset/gentle-pandas-clap.md.changeset/slow-badgers-remember.mddocs/router/guide/document-head-management.mddocs/start/framework/react/guide/cdn-asset-urls.mddocs/start/framework/react/guide/early-hints.mde2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-start/basic/package.jsone2e/react-start/basic/playwright.config.tse2e/react-start/basic/start-mode-config.tse2e/react-start/basic/tests/client-output.spec.tse2e/react-start/csp/tests/csp.spec.tse2e/react-start/dev-ssr-styles/package.jsone2e/react-start/dev-ssr-styles/tests/app.spec.tse2e/react-start/rsc-deferred-hydration/tests/hydration.spec.tse2e/react-start/scroll-restoration/tests/ssr-scroll-key.spec.tse2e/react-start/server-functions/package.jsone2e/react-start/server-functions/playwright.rsbuild-cache.config.tse2e/react-start/server-functions/rsbuild.config.tse2e/react-start/server-functions/tests/rsbuild-cache.spec.tse2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/start-manifest/tests/start-manifest.spec.tse2e/react-start/transform-asset-urls/src/routes/__root.tsxe2e/react-start/transform-asset-urls/src/server.tse2e/react-start/transform-asset-urls/tests/transform-asset-urls.spec.tse2e/solid-start/csp/tests/csp.spec.tse2e/solid-start/start-manifest/tests/start-manifest.spec.tse2e/vue-start/start-manifest/tests/start-manifest.spec.tspackages/react-router/src/Asset.tsxpackages/react-router/src/HeadContent.dev.tsxpackages/react-router/src/Scripts.tsxpackages/react-router/src/headContentUtils.tsxpackages/react-router/tests/Scripts.test.tsxpackages/react-start-rsc/src/serialization.server.tspackages/router-core/src/index.tspackages/router-core/src/manifest.tspackages/router-core/src/router.tspackages/router-core/src/ssr/createRequestHandler.tspackages/router-core/src/ssr/ssr-server.tspackages/router-core/tests/hydrate.test.tspackages/router-core/tests/manifest.test.tspackages/router-core/tests/ssr-server-manifest.test.tspackages/solid-router/src/HeadContent.dev.tsxpackages/solid-router/src/Scripts.tsxpackages/solid-router/src/headContentUtils.tsxpackages/solid-router/tests/Scripts.test.tsxpackages/start-plugin-core/src/rsbuild/COMPILER_ARCHITECTURE.mdpackages/start-plugin-core/src/rsbuild/planning.tspackages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/src/rsbuild/schema.tspackages/start-plugin-core/src/rsbuild/start-compiler-host.tspackages/start-plugin-core/src/rsbuild/start-compiler-metadata-loader.tspackages/start-plugin-core/src/rsbuild/start-compiler-metadata.tspackages/start-plugin-core/src/rsbuild/virtual-modules.tspackages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.tspackages/start-plugin-core/src/vite/start-manifest-plugin/plugin.tspackages/start-plugin-core/tests/rsbuild-output-directory.test.tspackages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.tspackages/start-plugin-core/vite.config.tspackages/start-server-core/src/createStartHandler.tspackages/start-server-core/src/early-hints.tspackages/start-server-core/src/finalManifest.tspackages/start-server-core/src/request-handler.tspackages/start-server-core/src/router-manifest.tspackages/start-server-core/src/tanstack-start.d.tspackages/start-server-core/src/transformAssetUrls.tspackages/start-server-core/tests/early-hints.test.tspackages/start-server-core/tests/finalManifest.test.tspackages/start-server-core/tests/transformAssets.test.tspackages/start-storage-context/src/async-local-storage.tspackages/vue-router/src/HeadContent.dev.tsxpackages/vue-router/src/HeadContent.tsxpackages/vue-router/src/Scripts.tsxpackages/vue-router/src/headContentUtils.tsxpackages/vue-router/tests/Scripts.test.tsx
💤 Files with no reviewable changes (1)
- packages/start-server-core/src/createStartHandler.ts
✅ Files skipped from review due to trivial changes (11)
- e2e/react-start/server-functions/playwright.rsbuild-cache.config.ts
- e2e/react-start/server-functions/tests/server-functions.spec.ts
- docs/router/guide/document-head-management.md
- docs/start/framework/react/guide/early-hints.md
- e2e/react-router/basic-file-based/src/routeTree.gen.ts
- .changeset/slow-badgers-remember.md
- e2e/react-start/basic/package.json
- packages/start-server-core/src/request-handler.ts
- docs/start/framework/react/guide/cdn-asset-urls.md
- packages/start-server-core/tests/finalManifest.test.ts
- packages/router-core/tests/hydrate.test.ts
schiller-manuel
force-pushed
the
rsbuild-iife-cache
branch
from
5783c38 to
a2ca058
Compare
schiller-manuel
force-pushed
the
rsbuild-iife-cache
branch
from
a2ca058 to
53498e8
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/react-start/server-functions/tests/rsbuild-cache.spec.ts (1)
66-79: 💤 Low valueProcess kill error handling may silently fail on non-existent processes.
The
killDevServerfunction catches all errors when killing the process group but only falls back to direct child kill if the process group kill throws. On some systems,process.kill(-pid, signal)may fail for reasons other than "process already exited" (e.g., permission issues), and swallowing these silently could mask debugging information.Consider logging the error in test environments:
} catch (e) { - // Fall back to killing the direct child below. + // Fall back to killing the direct child below. + // Log in case it's not just "process doesn't exist" + if (process.env.DEBUG) console.warn('process.kill(-pid) failed:', e) }🤖 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/server-functions/tests/rsbuild-cache.spec.ts` around lines 66 - 79, The catch-all in killDevServer swallows all errors from process.kill(-pid, signal) which can hide real failures; update killDevServer to catch the error into a variable, check for "process not found" (e.g. err.code === 'ESRCH' or equivalent) and only silently fallback to devServer.child.kill(signal) in that case, otherwise surface the error (rethrow or log via console.error/processLogger) so permission or other failures are not hidden; reference the killDevServer function, the devServer.child/ pid variables and the process.kill(-pid, signal) call when making the change.
🤖 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.
Nitpick comments:
In `@e2e/react-start/server-functions/tests/rsbuild-cache.spec.ts`:
- Around line 66-79: The catch-all in killDevServer swallows all errors from
process.kill(-pid, signal) which can hide real failures; update killDevServer to
catch the error into a variable, check for "process not found" (e.g. err.code
=== 'ESRCH' or equivalent) and only silently fallback to
devServer.child.kill(signal) in that case, otherwise surface the error (rethrow
or log via console.error/processLogger) so permission or other failures are not
hidden; reference the killDevServer function, the devServer.child/ pid variables
and the process.kill(-pid, signal) call when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb86e12e-646a-4e30-ab06-beac50c82443
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (76)
.changeset/gentle-pandas-clap.md.changeset/slow-badgers-remember.mddocs/router/guide/document-head-management.mddocs/start/framework/react/guide/cdn-asset-urls.mddocs/start/framework/react/guide/early-hints.mde2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-start/basic/package.jsone2e/react-start/basic/playwright.config.tse2e/react-start/basic/start-mode-config.tse2e/react-start/basic/tests/client-output.spec.tse2e/react-start/csp/tests/csp.spec.tse2e/react-start/dev-ssr-styles/package.jsone2e/react-start/dev-ssr-styles/tests/app.spec.tse2e/react-start/rsc-deferred-hydration/tests/hydration.spec.tse2e/react-start/scroll-restoration/tests/ssr-scroll-key.spec.tse2e/react-start/server-functions/package.jsone2e/react-start/server-functions/playwright.rsbuild-cache.config.tse2e/react-start/server-functions/rsbuild.config.tse2e/react-start/server-functions/tests/rsbuild-cache.spec.tse2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/start-manifest/tests/start-manifest.spec.tse2e/react-start/transform-asset-urls/src/routes/__root.tsxe2e/react-start/transform-asset-urls/src/server.tse2e/react-start/transform-asset-urls/tests/transform-asset-urls.spec.tse2e/solid-start/csp/tests/csp.spec.tse2e/solid-start/start-manifest/tests/start-manifest.spec.tse2e/vue-start/start-manifest/tests/start-manifest.spec.tspackages/react-router/src/Asset.tsxpackages/react-router/src/HeadContent.dev.tsxpackages/react-router/src/Scripts.tsxpackages/react-router/src/headContentUtils.tsxpackages/react-router/tests/Scripts.test.tsxpackages/react-start-rsc/src/serialization.server.tspackages/router-core/src/index.tspackages/router-core/src/manifest.tspackages/router-core/src/router.tspackages/router-core/src/ssr/createRequestHandler.tspackages/router-core/src/ssr/ssr-server.tspackages/router-core/tests/hydrate.test.tspackages/router-core/tests/manifest.test.tspackages/router-core/tests/ssr-server-manifest.test.tspackages/solid-router/src/HeadContent.dev.tsxpackages/solid-router/src/Scripts.tsxpackages/solid-router/src/headContentUtils.tsxpackages/solid-router/tests/Scripts.test.tsxpackages/start-plugin-core/src/import-protection/virtualModules.tspackages/start-plugin-core/src/rsbuild/COMPILER_ARCHITECTURE.mdpackages/start-plugin-core/src/rsbuild/planning.tspackages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/src/rsbuild/schema.tspackages/start-plugin-core/src/rsbuild/start-compiler-host.tspackages/start-plugin-core/src/rsbuild/start-compiler-metadata-loader.tspackages/start-plugin-core/src/rsbuild/start-compiler-metadata.tspackages/start-plugin-core/src/rsbuild/virtual-modules.tspackages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.tspackages/start-plugin-core/src/vite/start-manifest-plugin/plugin.tspackages/start-plugin-core/tests/importProtection/virtualModules.test.tspackages/start-plugin-core/tests/rsbuild-output-directory.test.tspackages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.tspackages/start-plugin-core/vite.config.tspackages/start-server-core/src/createStartHandler.tspackages/start-server-core/src/early-hints.tspackages/start-server-core/src/finalManifest.tspackages/start-server-core/src/request-handler.tspackages/start-server-core/src/router-manifest.tspackages/start-server-core/src/tanstack-start.d.tspackages/start-server-core/src/transformAssetUrls.tspackages/start-server-core/tests/early-hints.test.tspackages/start-server-core/tests/finalManifest.test.tspackages/start-server-core/tests/transformAssets.test.tspackages/start-storage-context/src/async-local-storage.tspackages/vue-router/src/HeadContent.dev.tsxpackages/vue-router/src/HeadContent.tsxpackages/vue-router/src/Scripts.tsxpackages/vue-router/src/headContentUtils.tsxpackages/vue-router/tests/Scripts.test.tsx
💤 Files with no reviewable changes (1)
- packages/start-server-core/src/createStartHandler.ts
✅ Files skipped from review due to trivial changes (8)
- packages/start-server-core/src/request-handler.ts
- e2e/react-start/server-functions/rsbuild.config.ts
- .changeset/gentle-pandas-clap.md
- docs/start/framework/react/guide/early-hints.md
- .changeset/slow-badgers-remember.md
- e2e/react-start/basic/package.json
- docs/router/guide/document-head-management.md
- docs/start/framework/react/guide/cdn-asset-urls.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
also fix serverfns with rsbuild cache
Summary by CodeRabbit
New Features
Bug Fixes
Behavior Changes
script/stylesheet.Documentation