fix(start): preserve dev SSR CSS order#7830
Open
schiller-manuel wants to merge 3 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughDevelopment SSR CSS collection now uses ordered module-graph traversal, client-side CSS loading, and cached extraction. The change avoids duplicated imports, preserves code-split styles, updates plugin wiring, and adds framework-specific tests for ordering and SSR behavior. ChangesDevelopment SSR CSS collection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant DevStylesMiddleware
participant collectDevStyles
participant ViteModuleGraph
participant ClientDevEnvironment
Browser->>DevStylesMiddleware: request development styles
DevStylesMiddleware->>collectDevStyles: pass route entries and CSS loader
collectDevStyles->>ViteModuleGraph: resolve ordered CSS dependencies
ViteModuleGraph-->>collectDevStyles: return CSS module nodes
collectDevStyles->>ClientDevEnvironment: load transformed CSS
ClientDevEnvironment-->>collectDevStyles: return CSS contents
collectDevStyles-->>DevStylesMiddleware: return concatenated CSS
DevStylesMiddleware-->>Browser: serve styles.css
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit 77035e4
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview1 package(s) bumped directly, 4 bumped as dependents. 🟩 Patch bumps
|
Contributor
Bundle Size BenchmarksThis pull request does not affect bundle size in any measured scenario. |
![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: 3
🤖 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/dev-ssr-styles/src/styles/css-import-base.css`:
- Line 3: In the stylesheet declaration block, add an empty line between the
custom property and the background-color declaration to satisfy Stylelint’s
declaration-empty-line-before rule.
In `@e2e/react-start/dev-ssr-styles/tests/app.spec.ts`:
- Around line 28-47: Update the CSS response collection in the test around the
page response listener and navigation steps so each navigation records its own
stylesheet response. Capture the response associated with the initial page.goto
separately from the response observed after page.reload, then compare those two
navigation-specific promises instead of taking cssBodies.slice(0, 2).
In `@packages/start-plugin-core/src/vite/dev-server-plugin/dev-styles.ts`:
- Around line 161-162: Update the module classification around isCssFile and
resolveModuleDeps to use node.url rather than the fallback fileOrUrl value,
ensuring virtual Vue style-block URLs with CSS query parameters are recognized
as CSS modules. Add a regression test covering a node whose file is the host
.vue path while its url contains the CSS style query.
🪄 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: d7b3be4d-0a24-46c3-a998-c1e7c0fb0b9b
📒 Files selected for processing (13)
.changeset/fix-dev-ssr-css-order.mde2e/react-start/dev-ssr-styles/src/components/CodeSplitStyledBox.tsxe2e/react-start/dev-ssr-styles/src/routeTree.gen.tse2e/react-start/dev-ssr-styles/src/routes/css-import-order.tsxe2e/react-start/dev-ssr-styles/src/routes/index.tsxe2e/react-start/dev-ssr-styles/src/styles/app.csse2e/react-start/dev-ssr-styles/src/styles/code-split-route.module.csse2e/react-start/dev-ssr-styles/src/styles/css-import-base.csse2e/react-start/dev-ssr-styles/src/styles/css-import-order.csse2e/react-start/dev-ssr-styles/tests/app.spec.tspackages/start-plugin-core/src/vite/dev-server-plugin/dev-styles.tspackages/start-plugin-core/src/vite/dev-server-plugin/plugin.tspackages/start-plugin-core/tests/vite/dev-styles.test.ts
💤 Files with no reviewable changes (1)
- e2e/react-start/dev-ssr-styles/src/styles/app.css
| @@ -0,0 +1,5 @@ | |||
| .css-import-order { | |||
| --css-import-base-marker: issue-7794; | |||
| background-color: #ffffff; | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Stylelint error before merging.
Stylelint reports declaration-empty-line-before on Line 3. Add an empty line between the custom property and background-color.
Proposed fix
.css-import-order {
--css-import-base-marker: issue-7794;
+
background-color: `#ffffff`;📝 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
| background-color: #ffffff; | |
| .css-import-order { | |
| --css-import-base-marker: issue-7794; | |
| background-color: `#ffffff`; | |
| } |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 3-3: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 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/dev-ssr-styles/src/styles/css-import-base.css` at line 3, In
the stylesheet declaration block, add an empty line between the custom property
and the background-color declaration to satisfy Stylelint’s
declaration-empty-line-before rule.
Source: Linters/SAST tools
Comment on lines
+28
to
+47
| const cssBodies: Array<Promise<string>> = [] | ||
| page.on('response', (response) => { | ||
| if ( | ||
| new URL(response.url()).pathname.endsWith( | ||
| '/@tanstack-start/styles.css', | ||
| ) | ||
| ) { | ||
| cssBodies.push(response.text()) | ||
| } | ||
| }) | ||
|
|
||
| await page.goto('/css-import-order') | ||
| await expect(page.getByTestId('css-import-order')).toBeVisible() | ||
| await expect.poll(() => cssBodies.length).toBeGreaterThanOrEqual(1) | ||
|
|
||
| await page.reload() | ||
| await expect(page.getByTestId('css-import-order')).toBeVisible() | ||
| await expect.poll(() => cssBodies.length).toBeGreaterThanOrEqual(2) | ||
|
|
||
| const [initialCss, reloadedCss] = await Promise.all(cssBodies.slice(0, 2)) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Associate each CSS response with its navigation.
The listener may collect multiple stylesheet responses before reload(), so slice(0, 2) can compare two responses from the first navigation and never validate the reload.
Proposed fix
- const cssBodies: Array<Promise<string>> = []
- page.on('response', (response) => {
- if (
- new URL(response.url()).pathname.endsWith(
- '/@tanstack-start/styles.css',
- )
- ) {
- cssBodies.push(response.text())
- }
- })
+ const isDevStylesResponse = (response: { url(): string }) =>
+ new URL(response.url()).pathname.endsWith(
+ '/@tanstack-start/styles.css',
+ )
+ const initialResponsePromise =
+ page.waitForResponse(isDevStylesResponse)
await page.goto('/css-import-order')
await expect(page.getByTestId('css-import-order')).toBeVisible()
- await expect.poll(() => cssBodies.length).toBeGreaterThanOrEqual(1)
+ const initialCss = await (await initialResponsePromise).text()
+ const reloadedResponsePromise =
+ page.waitForResponse(isDevStylesResponse)
await page.reload()
await expect(page.getByTestId('css-import-order')).toBeVisible()
- await expect.poll(() => cssBodies.length).toBeGreaterThanOrEqual(2)
-
- const [initialCss, reloadedCss] = await Promise.all(cssBodies.slice(0, 2))
+ const reloadedCss = await (await reloadedResponsePromise).text()📝 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
| const cssBodies: Array<Promise<string>> = [] | |
| page.on('response', (response) => { | |
| if ( | |
| new URL(response.url()).pathname.endsWith( | |
| '/@tanstack-start/styles.css', | |
| ) | |
| ) { | |
| cssBodies.push(response.text()) | |
| } | |
| }) | |
| await page.goto('/css-import-order') | |
| await expect(page.getByTestId('css-import-order')).toBeVisible() | |
| await expect.poll(() => cssBodies.length).toBeGreaterThanOrEqual(1) | |
| await page.reload() | |
| await expect(page.getByTestId('css-import-order')).toBeVisible() | |
| await expect.poll(() => cssBodies.length).toBeGreaterThanOrEqual(2) | |
| const [initialCss, reloadedCss] = await Promise.all(cssBodies.slice(0, 2)) | |
| const isDevStylesResponse = (response: { url(): string }) => | |
| new URL(response.url()).pathname.endsWith( | |
| '/@tanstack-start/styles.css', | |
| ) | |
| const initialResponsePromise = | |
| page.waitForResponse(isDevStylesResponse) | |
| await page.goto('/css-import-order') | |
| await expect(page.getByTestId('css-import-order')).toBeVisible() | |
| const initialCss = await (await initialResponsePromise).text() | |
| const reloadedResponsePromise = | |
| page.waitForResponse(isDevStylesResponse) | |
| await page.reload() | |
| await expect(page.getByTestId('css-import-order')).toBeVisible() | |
| const reloadedCss = await (await reloadedResponsePromise).text() |
🤖 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/dev-ssr-styles/tests/app.spec.ts` around lines 28 - 47,
Update the CSS response collection in the test around the page response listener
and navigation steps so each navigation records its own stylesheet response.
Capture the response associated with the initial page.goto separately from the
response observed after page.reload, then compare those two navigation-specific
promises instead of taking cssBodies.slice(0, 2).
Merging this PR will regress 5 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | mem server serialization-payload (vue) |
3.5 MB | 6.1 MB | -43.19% |
| ❌ | Memory | mem server error-paths unmatched (react) |
567.3 KB | 824.4 KB | -31.18% |
| ❌ | Memory | mem server server-fn-churn (solid) |
263 KB | 273.8 KB | -3.96% |
| ❌ | Memory | mem server error-paths not-found (react) |
267 KB | 277.2 KB | -3.66% |
| ❌ | Simulation | ssr control-flow error 500 (react) |
62.2 ms | 64.3 ms | -3.26% |
| ⚡ | Memory | mem server error-paths redirect (vue) |
979.9 KB | 304.5 KB | ×3.2 |
| ⚡ | Memory | mem server error-paths not-found (solid) |
1,146.2 KB | 421.2 KB | ×2.7 |
| ⚡ | Memory | mem server peak-large-page (react) |
2,216.6 KB | 954.4 KB | ×2.3 |
| ⚡ | Memory | mem server serialization-payload (react) |
5 MB | 3.4 MB | +44.64% |
| ⚡ | Memory | mem server streaming-peak chunked (vue) |
12.1 MB | 11.1 MB | +8.86% |
| ⚡ | Simulation | ssr streaming deferred (react) |
73.1 ms | 70.9 ms | +3.03% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix-7794 (77035e4) with main (edf5575)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/solid-start/dev-ssr-styles/src/styles/code-split-route.module.css`:
- Around line 2-3: In the stylesheet rule containing
--solid-code-split-style-marker, insert an empty line before the
background-color declaration to satisfy the declaration-empty-line-before
Stylelint rule.
In `@e2e/solid-start/dev-ssr-styles/tsconfig.json`:
- Line 21: Update the compilerOptions.types setting in the dev-ssr-styles
tsconfig to include Node ambient types alongside vite/client, or remove the
explicit types allowlist so tests/setup/global.setup.ts receives the process
type from `@types/node`.
🪄 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: 14108bd2-a34a-4920-8377-03211f98c330
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
e2e/solid-start/dev-ssr-styles/.gitignoree2e/solid-start/dev-ssr-styles/package.jsone2e/solid-start/dev-ssr-styles/playwright.config.tse2e/solid-start/dev-ssr-styles/src/components/CodeSplitStyledBox.tsxe2e/solid-start/dev-ssr-styles/src/router.tsxe2e/solid-start/dev-ssr-styles/src/routes/__root.tsxe2e/solid-start/dev-ssr-styles/src/routes/index.tsxe2e/solid-start/dev-ssr-styles/src/styles/app.csse2e/solid-start/dev-ssr-styles/src/styles/code-split-route.module.csse2e/solid-start/dev-ssr-styles/tests/app.spec.tse2e/solid-start/dev-ssr-styles/tests/setup/global.setup.tse2e/solid-start/dev-ssr-styles/tests/setup/global.teardown.tse2e/solid-start/dev-ssr-styles/tsconfig.jsone2e/solid-start/dev-ssr-styles/vite.config.tse2e/vue-start/dev-ssr-styles/.gitignoree2e/vue-start/dev-ssr-styles/package.jsone2e/vue-start/dev-ssr-styles/playwright.config.tse2e/vue-start/dev-ssr-styles/src/components/SfcStyledBox.vuee2e/vue-start/dev-ssr-styles/src/router.tsxe2e/vue-start/dev-ssr-styles/src/routes/__root.tsxe2e/vue-start/dev-ssr-styles/src/routes/index.tsxe2e/vue-start/dev-ssr-styles/src/styles/app.csse2e/vue-start/dev-ssr-styles/src/vue-shim.d.tse2e/vue-start/dev-ssr-styles/tests/app.spec.tse2e/vue-start/dev-ssr-styles/tests/setup/global.setup.tse2e/vue-start/dev-ssr-styles/tests/setup/global.teardown.tse2e/vue-start/dev-ssr-styles/tsconfig.jsone2e/vue-start/dev-ssr-styles/vite.config.tspackages/start-plugin-core/src/vite/dev-server-plugin/dev-styles.tspackages/start-plugin-core/tests/vite/dev-styles.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/start-plugin-core/src/vite/dev-server-plugin/dev-styles.ts
Comment on lines
+2
to
+3
| --solid-code-split-style-marker: present; | ||
| background-color: #3b82f6; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the Stylelint violation.
Insert an empty line between the custom property and background-color to satisfy declaration-empty-line-before.
Suggested fix
.styledBox {
--solid-code-split-style-marker: present;
+
background-color: `#3b82f6`;📝 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
| --solid-code-split-style-marker: present; | |
| background-color: #3b82f6; | |
| --solid-code-split-style-marker: present; | |
| background-color: `#3b82f6`; |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 3-3: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 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/solid-start/dev-ssr-styles/src/styles/code-split-route.module.css` around
lines 2 - 3, In the stylesheet rule containing --solid-code-split-style-marker,
insert an empty line before the background-color declaration to satisfy the
declaration-empty-line-before Stylelint rule.
Source: Linters/SAST tools
| "~/*": ["./src/*"] | ||
| }, | ||
| "noEmit": true, | ||
| "types": ["vite/client"] |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n '"types"|"`@types/node`"|process\.env' \
e2e/solid-start/dev-ssr-styles/tsconfig.json \
e2e/solid-start/dev-ssr-styles/package.json \
e2e/solid-start/dev-ssr-styles/tests/setup/global.setup.tsRepository: TanStack/router
Length of output: 406
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== tsconfig ==\n'
cat -n e2e/solid-start/dev-ssr-styles/tsconfig.json
printf '\n== package.json ==\n'
cat -n e2e/solid-start/dev-ssr-styles/package.json
printf '\n== setup file ==\n'
cat -n e2e/solid-start/dev-ssr-styles/tests/setup/global.setup.ts
printf '\n== config inheritance search ==\n'
rg -n '"extends"|compilerOptions|types' e2e/solid-start/dev-ssr-styles -g 'tsconfig*.json'Repository: TanStack/router
Length of output: 2907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, pathlib
p = pathlib.Path('e2e/solid-start/dev-ssr-styles/tsconfig.json')
data = json.loads(p.read_text())
print('extends:', data.get('extends'))
print('compilerOptions.types:', data.get('compilerOptions', {}).get('types'))
PYRepository: TanStack/router
Length of output: 206
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on any tsconfig chain that may add node types.
find e2e/solid-start/dev-ssr-styles -name 'tsconfig*.json' -printRepository: TanStack/router
Length of output: 198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, pathlib
p = pathlib.Path('e2e/solid-start/dev-ssr-styles/package.json')
data = json.loads(p.read_text())
print('scripts:')
for k,v in data.get('scripts', {}).items():
print(f' {k}: {v}')
PYRepository: TanStack/router
Length of output: 432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, pathlib
p = pathlib.Path('e2e/solid-start/dev-ssr-styles/tsconfig.json')
data = json.loads(p.read_text())
print(json.dumps(data, indent=2))
PYRepository: TanStack/router
Length of output: 771
Include Node ambient types here
e2e/solid-start/dev-ssr-styles/tsconfig.json:21 only allows vite/client, so tests/setup/global.setup.ts won’t get process from @types/node even though it’s used at line 12. Add "node" to compilerOptions.types, or remove the explicit allowlist.
🤖 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/solid-start/dev-ssr-styles/tsconfig.json` at line 21, Update the
compilerOptions.types setting in the dev-ssr-styles tsconfig to include Node
ambient types alongside vite/client, or remove the explicit types allowlist so
tests/setup/global.setup.ts receives the process type from `@types/node`.
Collaborator
Author
|
depends on vitejs/vite#22968 |
Member
|
@schiller-manuel Gentle ping when you have a chance: I refreshed this onto current main (77035e4). CI is green aside from the bundled-dev import-protection failure tied to vitejs/vite#22968. We’ll leave the upstream follow-up with you—no rush, just wanted to put it back on your radar. Thanks! |
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
2 participants
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.
fixes #7794
Summary by CodeRabbit
Bug Fixes
Tests
@importdependency behavior.