fix: reduce start SSR manifest asset duplication#7157
Merged
schiller-manuel merged 3 commits intomainfrom Apr 11, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughExcludes unmatched route assets from SSR-dehydrated router state, adds seroval-based start-manifest serialization to preserve shared asset identity, changes server to disable unmatched-route assets by default, and introduces an E2E suite validating CSS-module hydration, navigation, and manifest asset reuse. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Server as Server (Nitro/Vite SSR)
participant Router as Router SSR
participant ManifestBuilder as StartManifest Builder
participant Store as Serialized Manifest (seroval)
Client->>Server: HTTP SSR request (route)
Server->>Router: load router, run server SSR
Router->>ManifestBuilder: request start-manifest for matched routes
ManifestBuilder->>Store: serializeStartManifest(startManifest)
Store-->>Router: serialized manifest (embedded)
Router-->>Server: dehydrate router state (exclude unmatched assets per flag)
Server-->>Client: SSR HTML + serialized manifest payload
Client->>Client: hydrate, fetch clientEntry, reuse shared assets (identity preserved)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
🚀 Changeset Version Preview3 package(s) bumped directly, 20 bumped as dependents. 🟩 Patch bumps
|
Contributor
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/ssr/ssr-server.ts (1)
245-247:⚠️ Potential issue | 🟠 MajorInclude the new option in the manifest cache key.
Line 246 caches by route IDs only. In production, if two routers share the same
manifestbut use differentincludeUnmatchedRouteAssetsvalues, one can reuse the other’s cached filtered routes and return the wrong manifest shape.Suggested fix
- const manifestCacheKey = currentRouteIdsList.join('\0') + const manifestCacheKey = `${includeUnmatchedRouteAssets ? '1' : '0'}\0${currentRouteIdsList.join('\0')}`Also applies to: 250-275
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/ssr/ssr-server.ts` around lines 245 - 247, Cache key currently built from currentRouteIdsList (manifestCacheKey) ignores the includeUnmatchedRouteAssets option, so different includeUnmatchedRouteAssets settings can collide; update the cache key construction (where manifestCacheKey is created/used in ssr-server.ts and the related cache logic around matchesToDehydrate) to incorporate the includeUnmatchedRouteAssets boolean (or its string/serialised form) into the key (e.g., append `\0includeUnmatchedRouteAssets=${includeUnmatchedRouteAssets}`) so cached filtered manifests are segregated by that option.
🧹 Nitpick comments (2)
packages/router-core/src/ssr/ssr-server.ts (1)
241-243: Update the filtering comment to match the new conditional behavior.The comment says unmatched routes always include assets, but this now depends on
includeUnmatchedRouteAssets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/ssr/ssr-server.ts` around lines 241 - 243, Update the misleading comment above the manifest conditional to reflect the new conditional behavior: instead of stating "For all other routes, only send assets (no preloads...)", mention that unmatched routes will include assets only when includeUnmatchedRouteAssets is true; reference the existing manifest conditional and the includeUnmatchedRouteAssets flag in the comment so readers know the behavior depends on includeUnmatchedRouteAssets.packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts (1)
1-1: Import ordering and type specifier style.ESLint flags import ordering and suggests using top-level type-only imports. These are style preferences that could be addressed for consistency.
♻️ Optional: Fix import ordering and type specifier style
+import type { StartManifest } from '../../src/start-manifest-plugin/manifestBuilder' import { deserialize } from 'seroval' import { describe, expect, test } from 'vitest' import { appendUniqueAssets, appendUniqueStrings, buildStartManifest, collectDynamicImportCss, createChunkCssAssetCollector, createManifestAssetResolvers, getRouteFilePathsFromModuleIds, normalizeViteClientBuild, normalizeViteClientChunk, - serializeStartManifest, scanClientChunks, - type StartManifest, + serializeStartManifest, } from '../../src/start-manifest-plugin/manifestBuilder'Also applies to: 13-15
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts` at line 1, Reorder and style the imports so runtime imports like "deserialize" from 'seroval' are grouped before/after other groups per the project's import-order rule, and convert any type-only imports used in this test (e.g., interfaces or types imported on lines around the test file where types are imported) to top-level "import type" specifiers; update the "import { deserialize } from 'seroval'" import grouping and change any purely type imports referenced in this file to use "import type" to satisfy ESLint import-order and type-only import rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/router-core/src/ssr/ssr-server.ts`:
- Around line 245-247: Cache key currently built from currentRouteIdsList
(manifestCacheKey) ignores the includeUnmatchedRouteAssets option, so different
includeUnmatchedRouteAssets settings can collide; update the cache key
construction (where manifestCacheKey is created/used in ssr-server.ts and the
related cache logic around matchesToDehydrate) to incorporate the
includeUnmatchedRouteAssets boolean (or its string/serialised form) into the key
(e.g., append `\0includeUnmatchedRouteAssets=${includeUnmatchedRouteAssets}`) so
cached filtered manifests are segregated by that option.
---
Nitpick comments:
In `@packages/router-core/src/ssr/ssr-server.ts`:
- Around line 241-243: Update the misleading comment above the manifest
conditional to reflect the new conditional behavior: instead of stating "For all
other routes, only send assets (no preloads...)", mention that unmatched routes
will include assets only when includeUnmatchedRouteAssets is true; reference the
existing manifest conditional and the includeUnmatchedRouteAssets flag in the
comment so readers know the behavior depends on includeUnmatchedRouteAssets.
In
`@packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts`:
- Line 1: Reorder and style the imports so runtime imports like "deserialize"
from 'seroval' are grouped before/after other groups per the project's
import-order rule, and convert any type-only imports used in this test (e.g.,
interfaces or types imported on lines around the test file where types are
imported) to top-level "import type" specifiers; update the "import {
deserialize } from 'seroval'" import grouping and change any purely type imports
referenced in this file to use "import type" to satisfy ESLint import-order and
type-only import rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f949c0cf-e87b-428d-919d-074555766fee
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
.changeset/cyan-camels-dance.mde2e/react-start/start-manifest/package.jsone2e/react-start/start-manifest/playwright.config.tse2e/react-start/start-manifest/src/components/AppShell.tsxe2e/react-start/start-manifest/src/components/SharedNestedLayout.tsxe2e/react-start/start-manifest/src/routeTree.gen.tse2e/react-start/start-manifest/src/router.tsxe2e/react-start/start-manifest/src/routes/__root.tsxe2e/react-start/start-manifest/src/routes/index.tsxe2e/react-start/start-manifest/src/routes/r1.tsxe2e/react-start/start-manifest/src/routes/r2.tsxe2e/react-start/start-manifest/src/routes/r3.tsxe2e/react-start/start-manifest/src/routes/r4.tsxe2e/react-start/start-manifest/src/routes/r5.tsxe2e/react-start/start-manifest/src/routes/r6.tsxe2e/react-start/start-manifest/src/routes/shared-a.tsxe2e/react-start/start-manifest/src/routes/shared-b.tsxe2e/react-start/start-manifest/src/routes/shared-c.tsxe2e/react-start/start-manifest/src/styles/root-shell.module.csse2e/react-start/start-manifest/src/styles/route-one.module.csse2e/react-start/start-manifest/src/styles/route-two.module.csse2e/react-start/start-manifest/src/styles/shared-layout.module.csse2e/react-start/start-manifest/tests/start-manifest.spec.tse2e/react-start/start-manifest/tsconfig.jsone2e/react-start/start-manifest/vite.config.tspackages/router-core/src/ssr/ssr-server.tspackages/router-core/tests/ssr-server-manifest.test.tspackages/start-plugin-core/package.jsonpackages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.tspackages/start-plugin-core/src/vite/start-manifest-plugin/plugin.tspackages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.tspackages/start-server-core/src/createStartHandler.ts
Contributor
|
View your CI Pipeline Execution ↗ for commit 8f5b695
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
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/tests/start-manifest-plugin/manifestBuilder.test.ts (1)
3-16:⚠️ Potential issue | 🟡 MinorFix import style/order to clear current ESLint failures.
This block currently violates
sort-importsandimport/consistent-type-specifier-style(inlinetype StartManifest+ member ordering).Suggested diff
import { appendUniqueAssets, appendUniqueStrings, buildStartManifest, collectDynamicImportCss, createChunkCssAssetCollector, createManifestAssetResolvers, getRouteFilePathsFromModuleIds, normalizeViteClientBuild, normalizeViteClientChunk, - serializeStartManifest, scanClientChunks, - type StartManifest, + serializeStartManifest, } from '../../src/start-manifest-plugin/manifestBuilder' +import type { StartManifest } from '../../src/start-manifest-plugin/manifestBuilder'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts` around lines 3 - 16, The import list violates sort-imports and import/consistent-type-specifier-style; fix by splitting the type import into a dedicated type-only import and alphabetizing the non-type imports from the module. For example, keep value exports (appendUniqueAssets, appendUniqueStrings, buildStartManifest, collectDynamicImportCss, createChunkCssAssetCollector, createManifestAssetResolvers, getRouteFilePathsFromModuleIds, normalizeViteClientBuild, normalizeViteClientChunk, scanClientChunks, serializeStartManifest) in one alphabetized import, and add a separate "import type { StartManifest }" line for the type; ensure the member order matches project import sorting rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts`:
- Around line 3-16: The import list violates sort-imports and
import/consistent-type-specifier-style; fix by splitting the type import into a
dedicated type-only import and alphabetizing the non-type imports from the
module. For example, keep value exports (appendUniqueAssets,
appendUniqueStrings, buildStartManifest, collectDynamicImportCss,
createChunkCssAssetCollector, createManifestAssetResolvers,
getRouteFilePathsFromModuleIds, normalizeViteClientBuild,
normalizeViteClientChunk, scanClientChunks, serializeStartManifest) in one
alphabetized import, and add a separate "import type { StartManifest }" line for
the type; ensure the member order matches project import sorting rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6c7dc4a-a377-4258-baf8-fd9567c72be1
📒 Files selected for processing (2)
packages/router-core/src/ssr/ssr-server.tspackages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts
Co-authored-by: schiller-manuel <schiller-manuel@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We add non-null assertions (!) to the assets property accesses in the new serializeStartManifest identity test, fixing the TS2532 "Object is possibly 'undefined'" errors raised by TypeScript 5.6's stricter indexed-access checks. The assets field is optional on RouteTreeRoute, but the test explicitly constructs routes with assets populated, so the assertions are safe and accurately reflect the test's intent.
Tip
✅ We verified this fix by re-running @tanstack/start-plugin-core:test:types.
diff --git a/packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts b/packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts
index 9cdb00d8e2..c11b1f6726 100644
--- a/packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts
+++ b/packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts
@@ -703,11 +703,11 @@ describe('buildStartManifest', () => {
serializeStartManifest(manifest),
)
- expect(evaluated.routes['/a'].assets[0]).toBe(
- evaluated.routes['/b'].assets[0],
+ expect(evaluated.routes['/a']!.assets![0]).toBe(
+ evaluated.routes['/b']!.assets![0],
)
- expect(evaluated.routes['/b'].assets[0]).toBe(
- evaluated.routes['/c'].assets[0],
+ expect(evaluated.routes['/b']!.assets![0]).toBe(
+ evaluated.routes['/c']!.assets![0],
)
})
Or Apply changes locally with:
npx nx-cloud apply-locally 65lt-TxmR
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
Contributor
There was a problem hiding this comment.
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/tests/start-manifest-plugin/manifestBuilder.test.ts (1)
3-16:⚠️ Potential issue | 🟡 MinorFix import ordering and split the type-only import.
This block currently violates the configured lint rules (
sort-importsandimport/consistent-type-specifier-style).Proposed fix
import { appendUniqueAssets, appendUniqueStrings, buildStartManifest, collectDynamicImportCss, createChunkCssAssetCollector, createManifestAssetResolvers, getRouteFilePathsFromModuleIds, normalizeViteClientBuild, normalizeViteClientChunk, - serializeStartManifest, scanClientChunks, - type StartManifest, + serializeStartManifest, } from '../../src/start-manifest-plugin/manifestBuilder' +import type { StartManifest } from '../../src/start-manifest-plugin/manifestBuilder'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts` around lines 3 - 16, Reorder and split the import list to satisfy the lint rules: alphabetically sort the imported bindings (appendUniqueAssets, appendUniqueStrings, buildStartManifest, collectDynamicImportCss, createChunkCssAssetCollector, createManifestAssetResolvers, getRouteFilePathsFromModuleIds, normalizeViteClientBuild, normalizeViteClientChunk, scanClientChunks, serializeStartManifest) and move the type-only import into its own statement using the `import type { StartManifest } from '../../src/start-manifest-plugin/manifestBuilder'` form so the type specifier style is consistent with linting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts`:
- Around line 3-16: Reorder and split the import list to satisfy the lint rules:
alphabetically sort the imported bindings (appendUniqueAssets,
appendUniqueStrings, buildStartManifest, collectDynamicImportCss,
createChunkCssAssetCollector, createManifestAssetResolvers,
getRouteFilePathsFromModuleIds, normalizeViteClientBuild,
normalizeViteClientChunk, scanClientChunks, serializeStartManifest) and move the
type-only import into its own statement using the `import type { StartManifest }
from '../../src/start-manifest-plugin/manifestBuilder'` form so the type
specifier style is consistent with linting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be4725c9-5fe0-43b6-b580-f03da2212b6e
📒 Files selected for processing (1)
packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts
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.
Summary by CodeRabbit