-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: include assets in route manifest for all routes #6058
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request refines SSR dehydration logic to selectively include route manifests based on active state. Active routes receive full manifests, inactive routes with assets receive asset-only entries, and routes without assets are omitted entirely. Additionally, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 0d6870f
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/src/ssr/ssr-server.ts (1)
159-160: Consider enhancing the comment to explain the rationale.The comment accurately describes what the code does, but could be more explicit about why this behavior is necessary—specifically, to prevent shared CSS modules from being unloaded during navigation (fixes #6028).
Apply this diff to enhance the comment:
- // For currently matched routes, send full manifest (preloads + assets) - // For all other routes, only send assets (no preloads as they are handled via dynamic imports) + // For currently matched routes, send full manifest (preloads + assets) + // For all other routes, only send assets (no preloads as they are handled via dynamic imports) + // This ensures shared assets (e.g., CSS modules) remain available during navigation (fixes #6028)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/ssr/ssr-server.ts(1 hunks)packages/start-server-core/src/router-manifest.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/ssr/ssr-server.tspackages/start-server-core/src/router-manifest.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/ssr/ssr-server.tspackages/start-server-core/src/router-manifest.ts
🧠 Learnings (2)
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/start-server-core/src/router-manifest.ts
📚 Learning: 2025-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/start-server-core/src/router-manifest.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (3)
packages/start-server-core/src/router-manifest.ts (1)
39-39: LGTM! Clean refactor to enable filtering.The change from
maptoflatMapcombined with wrapping the return value in[[k, result]]enables proper filtering of routes without data while maintaining the correct structure forObject.fromEntries.Also applies to: 56-56
packages/router-core/src/ssr/ssr-server.ts (2)
162-164: LGTM! Efficient active route detection.Using a Set for O(1) lookup of active routes is the right approach. The variable naming is clear and the logic correctly handles all cases.
166-186: Excellent fix for the CSS unloading bug!The conditional logic correctly addresses issue #6028 by ensuring:
- Active routes receive full manifests (preloads + assets)
- Inactive routes with assets receive asset-only manifests, preventing CSS modules from being unloaded during navigation
- Routes without assets are filtered out to minimize payload size
The use of
flatMapfor conditional filtering is appropriate, and the improved parameter naming enhances readability.
4fc4646 to
0d6870f
Compare
fixes #6028
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.