Uh oh!
There was an error while loading. Please reload this page.
fix: dedupe repeated css assets in the start manifest - #7030
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces CSS asset deduplication to the Start manifest plugin by refactoring the preload deduplication logic to handle both preloads and stylesheets, preventing duplicate CSS from appearing in route entries or across parent-child route chains. Changes
Sequence DiagramsequenceDiagram
participant Builder as Start Manifest Builder
participant RouteTree as Route Tree
participant AssetCollector as Asset Collector
participant DedupeTracker as Dedupe Tracker
participant Output as Manifest Entry
Builder->>RouteTree: Traverse route tree starting at root
loop For each route in tree
RouteTree->>AssetCollector: Collect CSS assets from chunks
loop For each asset
AssetCollector->>DedupeTracker: Check asset identity
alt Asset not seen
DedupeTracker->>DedupeTracker: Add to seenAssets
AssetCollector->>Output: Include asset
else Asset already seen
AssetCollector->>AssetCollector: Skip duplicate
end
end
RouteTree->>Builder: Process child routes
end
Builder->>DedupeTracker: Clean up tracking state on unwind
DedupeTracker->>Builder: Complete deduplication
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
View your CI Pipeline Execution ↗ for commit 1867b8a
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview1 package(s) bumped directly, 3 bumped as dependents. 🟩 Patch bumps
|
Contributor
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts (1)
379-379: Avoid newanyescape hatches in the added tests.These fixtures are already structurally typed, so the new
anycasts turn off strict-mode checks around the exact manifest shapes this suite is supposed to pin down.asset.attrs?.hrefis enough here, and the route tree literals can be typed viaParameters<typeof buildStartManifest>[0]['routeTreeRoutes']instead ofas any.As per coding guidelines, "Use TypeScript strict mode with extensive type safety".
Also applies to: 455-455, 710-710, 827-827, 931-931, 1040-1040
🤖 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 379, Replace the newly added "any" casts in the tests so they don't bypass strict typing: change occurrences like assets.map((asset: any) => asset.attrs.href) to safely access the href via asset.attrs?.href, and remove "as any" on route tree literals by typing them using Parameters<typeof buildStartManifest>[0]['routeTreeRoutes']; update the test assertions to use the safe nullable access and strongly-typed routeTreeRoutes to preserve strict-mode checks (addresses the assets mapping and the route tree literals referenced in manifestBuilder.test.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts`:
- Around line 34-36: The dedupe pass currently collapses items in
DedupeRoute.assets using getAssetIdentity which only keys by href/src/rel/type
and thus incorrectly merges distinct tags that share a URL; update the dedupe to
operate only on stylesheet link tags (e.g., filter route.assets for
RouterManagedTag entries that are link elements with rel==='stylesheet' or
ManifestAssetLink-like items) before applying getAssetIdentity, leaving
non-stylesheet assets untouched, or alternatively replace the identity function
with one that includes full attributes (media, crossorigin, integrity, nonce)
and only apply that identity when processing generic route.assets; adjust the
logic in manifestBuilder.ts where DedupeRoute and route.assets are handled (also
mirror the same change at the other occurrences noted around the other blocks)
so only CSS link dedupe is scoped.
---
Nitpick comments:
In
`@packages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts`:
- Line 379: Replace the newly added "any" casts in the tests so they don't
bypass strict typing: change occurrences like assets.map((asset: any) =>
asset.attrs.href) to safely access the href via asset.attrs?.href, and remove
"as any" on route tree literals by typing them using Parameters<typeof
buildStartManifest>[0]['routeTreeRoutes']; update the test assertions to use the
safe nullable access and strongly-typed routeTreeRoutes to preserve strict-mode
checks (addresses the assets mapping and the route tree literals referenced in
manifestBuilder.test.ts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 948ab611-e516-42f3-8290-fac832bf286c
📒 Files selected for processing (3)
.changeset/fair-rivers-drum.mdpackages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.tspackages/start-plugin-core/tests/start-manifest-plugin/manifestBuilder.test.ts
Comment on lines
+34
to
+36
| type DedupeRoute = { | ||
| preloads?: Array<ManifestAssetLink> | ||
| assets?: Array<RouterManagedTag> |
Contributor
There was a problem hiding this comment.
Scope the nested asset dedupe to stylesheet links.
route.assets can already carry arbitrary RouterManagedTags, but this new pass dedupes the whole array with getAssetIdentity. That identity only keys link/script tags by href/src/rel/type, so distinct assets that share a URL but differ by media, crossorigin, integrity, nonce, etc. will collapse here. Since this PR is only trying to remove duplicate CSS, the safer fix is to dedupe stylesheet links only, or switch to a full-attrs identity before applying it to generic route assets.
💡 One way to keep the dedupe scoped to CSS
if (routeAssets && routeAssets.length > 0) {
let dedupedAssets: Array<RouterManagedTag> | undefined
for (let i = 0; i < routeAssets.length; i++) {
const asset = routeAssets[i]!
+ const isStylesheetLink =+ asset.tag === 'link' && asset.attrs?.rel === 'stylesheet'++ if (!isStylesheetLink) {+ if (dedupedAssets) {+ dedupedAssets.push(asset)+ }+ continue+ }+
const identity = getAssetIdentity(asset)
if (seenAssets.has(identity)) {
if (dedupedAssets === undefined) {
dedupedAssets = routeAssets.slice(0, i)
@@
if (routeAssets) {
for (let i = routeAssets.length - 1; i >= 0; i--) {
- seenAssets.delete(getAssetIdentity(routeAssets[i]!))+ const asset = routeAssets[i]!+ if (asset.tag === 'link' && asset.attrs?.rel === 'stylesheet') {+ seenAssets.delete(getAssetIdentity(asset))+ }
}
}Also applies to: 566-591, 619-623
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/src/start-manifest-plugin/manifestBuilder.ts`
around lines 34 - 36, The dedupe pass currently collapses items in
DedupeRoute.assets using getAssetIdentity which only keys by href/src/rel/type
and thus incorrectly merges distinct tags that share a URL; update the dedupe to
operate only on stylesheet link tags (e.g., filter route.assets for
RouterManagedTag entries that are link elements with rel==='stylesheet' or
ManifestAssetLink-like items) before applying getAssetIdentity, leaving
non-stylesheet assets untouched, or alternatively replace the identity function
with one that includes full attributes (media, crossorigin, integrity, nonce)
and only apply that identity when processing generic route.assets; adjust the
logic in manifestBuilder.ts where DedupeRoute and route.assets are handled (also
mirror the same change at the other occurrences noted around the other blocks)
so only CSS link dedupe is scoped.
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 freeto 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