feat: allow to configure dev ssr style injection#6797
Merged
schiller-manuel merged 1 commit intomainfrom Mar 1, 2026
Merged
Conversation
|
View your CI Pipeline Execution ↗ for commit f7623fb
☁️ Nx Cloud last updated this comment at |
Contributor
📝 WalkthroughWalkthroughThis PR introduces configurable dev SSR styles support with a new e2e test workspace, schema extensions, and conditional middleware registration. Changes enable toggling dev styles injection via configuration flags and custom basepath support during development. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 unit tests (beta)
Comment |
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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/start-plugin-core/src/plugin.ts (1)
423-426: Use parsed config as the single source fordevSsrStylesEnabled.Line 425 reads from
startPluginOpts, while Lines 313-314 already use parsedstartConfig. Keeping both on parsed config prevents future drift if parsing/defaulting behavior changes.♻️ Suggested single-source wiring
// packages/start-plugin-core/src/plugin.ts devServerPlugin({ getConfig, - devSsrStylesEnabled: startPluginOpts?.dev?.ssrStyles?.enabled ?? true, + getDevSsrStylesEnabled: () => getConfig().startConfig.dev.ssrStyles.enabled, }),// packages/start-plugin-core/src/dev-server-plugin/plugin.ts export function devServerPlugin({ getConfig, - devSsrStylesEnabled, + getDevSsrStylesEnabled, }: { getConfig: GetConfigFn - devSsrStylesEnabled: boolean + getDevSsrStylesEnabled: () => boolean }): PluginOption { - if (devSsrStylesEnabled) { + if (getDevSsrStylesEnabled()) { // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/plugin.ts` around lines 423 - 426, The devServerPlugin call currently reads devSsrStylesEnabled from startPluginOpts causing divergence from the parsed startConfig used elsewhere; change the argument to source devSsrStylesEnabled from the parsed startConfig (the same object used at lines where startConfig is referenced) so devServerPlugin({ getConfig, devSsrStylesEnabled: /* from startConfig */ }) uses startConfig's value instead of startPluginOpts, keeping startConfig as the single source of truth for SSR styles defaults; update the devServerPlugin invocation to reference startConfig and remove reliance on startPluginOpts for this flag.e2e/react-start/dev-ssr-styles/playwright.config.ts (1)
12-21: Consider extractinggetPortKeyto a shared helper.This key builder is duplicated with
e2e/react-start/dev-ssr-styles/tests/setup/global.setup.ts(Lines 9-18). A shared helper reduces drift between config/setup/teardown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/dev-ssr-styles/playwright.config.ts` around lines 12 - 21, The getPortKey function is duplicated; extract it into a shared helper (e.g., export function getPortKey(packageJson, ssrStylesMode, useNitro) in a new/shared module) and replace the local implementations in both getPortKey (playwright.config.ts) and the duplicate in global.setup.ts to import and call that helper; ensure the helper accepts the same inputs (packageJson.name, ssrStylesMode, useNitro) and preserves the existing key-building logic and export it for reuse so both files import the single source of truth.packages/start-plugin-core/src/dev-server-plugin/plugin.ts (1)
87-89: Remove theanycast for routes manifest access.Line 87 bypasses strict typing with
(globalThis as any), which makes this path easier to break silently.♻️ Suggested type-safe replacement
- const routesManifest = (globalThis as any).TSS_ROUTES_MANIFEST as - | Record<string, { filePath: string; children?: Array<string> }> - | undefined + const routesManifest = globalThis.TSS_ROUTES_MANIFEST// Add an ambient declaration (preferred in a shared types file) declare global { // eslint-disable-next-line no-var var TSS_ROUTES_MANIFEST: | Record<string, { filePath: string; children?: Array<string> }> | undefined }As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/dev-server-plugin/plugin.ts` around lines 87 - 89, Replace the unsafe cast by declaring a proper ambient global type for TSS_ROUTES_MANIFEST and then access it without (globalThis as any); add a declaration like "declare global { var TSS_ROUTES_MANIFEST: Record<string, { filePath: string; children?: string[] }> | undefined }" in a shared types file (or project-wide d.ts), remove the "(globalThis as any)" cast and read the manifest via "const routesManifest = globalThis.TSS_ROUTES_MANIFEST" so the code using routesManifest has correct compile-time types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/react-start/dev-ssr-styles/vite.config.ts`:
- Around line 7-16: Add an explicit return type for getSsrStylesConfig (e.g., a
union type representing the three possible config shapes) and implement a
never-branch guard that throws if ssrStylesMode is an unhandled SsrStylesMode
value; specifically update function getSsrStylesConfig to declare its return
type, keep the existing cases for 'disabled', 'custom-basepath', and 'default',
then add a final default case that calls a helper like
assertUnreachable(ssrStylesMode) or throws with the unknown value to ensure
future SsrStylesMode additions do not result in undefined returns.
In `@e2e/react-start/split-base-and-basepath/tests/setup/global.setup.ts`:
- Around line 30-65: The try/finally currently begins after creating context and
page so if chromium.launch(), browser.newContext(), or context.newPage() throws
those resources may leak; move the browser/newContext/newPage calls inside the
try and use nested try/finally blocks (one surrounding context creation to
ensure context.close() runs, and an inner one surrounding page creation to
ensure page-related waits/actions and then page.close()/context.close()), i.e.,
wrap chromium.launch()/browser in the outer try, create context inside that try
and ensure context.close() in its finally, then create page inside a nested try
and ensure page cleanup in its finally; reference the variables browser,
context, page and the warmup logic so the cleanup always runs even on
construction failure.
---
Nitpick comments:
In `@e2e/react-start/dev-ssr-styles/playwright.config.ts`:
- Around line 12-21: The getPortKey function is duplicated; extract it into a
shared helper (e.g., export function getPortKey(packageJson, ssrStylesMode,
useNitro) in a new/shared module) and replace the local implementations in both
getPortKey (playwright.config.ts) and the duplicate in global.setup.ts to import
and call that helper; ensure the helper accepts the same inputs
(packageJson.name, ssrStylesMode, useNitro) and preserves the existing
key-building logic and export it for reuse so both files import the single
source of truth.
In `@packages/start-plugin-core/src/dev-server-plugin/plugin.ts`:
- Around line 87-89: Replace the unsafe cast by declaring a proper ambient
global type for TSS_ROUTES_MANIFEST and then access it without (globalThis as
any); add a declaration like "declare global { var TSS_ROUTES_MANIFEST:
Record<string, { filePath: string; children?: string[] }> | undefined }" in a
shared types file (or project-wide d.ts), remove the "(globalThis as any)" cast
and read the manifest via "const routesManifest =
globalThis.TSS_ROUTES_MANIFEST" so the code using routesManifest has correct
compile-time types.
In `@packages/start-plugin-core/src/plugin.ts`:
- Around line 423-426: The devServerPlugin call currently reads
devSsrStylesEnabled from startPluginOpts causing divergence from the parsed
startConfig used elsewhere; change the argument to source devSsrStylesEnabled
from the parsed startConfig (the same object used at lines where startConfig is
referenced) so devServerPlugin({ getConfig, devSsrStylesEnabled: /* from
startConfig */ }) uses startConfig's value instead of startPluginOpts, keeping
startConfig as the single source of truth for SSR styles defaults; update the
devServerPlugin invocation to reference startConfig and remove reliance on
startPluginOpts for this flag.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
e2e/react-start/dev-ssr-styles/.gitignoree2e/react-start/dev-ssr-styles/.prettierignoree2e/react-start/dev-ssr-styles/env.tse2e/react-start/dev-ssr-styles/package.jsone2e/react-start/dev-ssr-styles/playwright.config.tse2e/react-start/dev-ssr-styles/src/routeTree.gen.tse2e/react-start/dev-ssr-styles/src/router.tsxe2e/react-start/dev-ssr-styles/src/routes/__root.tsxe2e/react-start/dev-ssr-styles/src/routes/index.tsxe2e/react-start/dev-ssr-styles/src/styles/app.csse2e/react-start/dev-ssr-styles/tests/app.spec.tse2e/react-start/dev-ssr-styles/tests/setup/global.setup.tse2e/react-start/dev-ssr-styles/tests/setup/global.teardown.tse2e/react-start/dev-ssr-styles/tsconfig.jsone2e/react-start/dev-ssr-styles/vite.config.tse2e/react-start/split-base-and-basepath/playwright.config.tse2e/react-start/split-base-and-basepath/src/routes/__root.tsxe2e/react-start/split-base-and-basepath/tests/app.spec.tse2e/react-start/split-base-and-basepath/tests/setup/global.setup.tse2e/react-start/split-base-and-basepath/tests/setup/global.teardown.tspackages/start-plugin-core/src/dev-server-plugin/plugin.tspackages/start-plugin-core/src/plugin.tspackages/start-plugin-core/src/schema.tspackages/start-server-core/src/router-manifest.ts
Comment on lines
+7
to
+16
| function getSsrStylesConfig() { | ||
| switch (ssrStylesMode) { | ||
| case 'disabled': | ||
| return { enabled: false } | ||
| case 'custom-basepath': | ||
| return { enabled: true, basepath: '/custom-styles/' } | ||
| case 'default': | ||
| return {} // use defaults (enabled=true, basepath=vite base) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current switch coverage vs SsrStylesMode definition
rg -n -C3 'SsrStylesMode|ssrStylesModeSchema|function getSsrStylesConfig|switch \(ssrStylesMode\)|case ' \
e2e/react-start/dev-ssr-styles/env.ts \
e2e/react-start/dev-ssr-styles/vite.config.tsRepository: TanStack/router
Length of output: 1857
🏁 Script executed:
# Also check TypeScript configuration to verify strict mode
find . -name "tsconfig.json" -o -name "tsconfig*.json" | head -5 | xargs grep -l "strict"Repository: TanStack/router
Length of output: 266
🏁 Script executed:
# Check the full content of the vite.config.ts file
cat e2e/react-start/dev-ssr-styles/vite.config.tsRepository: TanStack/router
Length of output: 1244
Make getSsrStylesConfig exhaustively typed.
Add an explicit return type + never guard so future SsrStylesMode additions can't silently return undefined.
♻️ Proposed refactor
+function assertNever(value: never): never {
+ throw new Error(`Unhandled ssrStylesMode: ${String(value)}`)
+}
+
-function getSsrStylesConfig() {
+function getSsrStylesConfig(): { enabled?: boolean; basepath?: string } {
switch (ssrStylesMode) {
case 'disabled':
return { enabled: false }
case 'custom-basepath':
return { enabled: true, basepath: '/custom-styles/' }
case 'default':
return {} // use defaults (enabled=true, basepath=vite base)
+ default:
+ return assertNever(ssrStylesMode)
}
}📝 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
| function getSsrStylesConfig() { | |
| switch (ssrStylesMode) { | |
| case 'disabled': | |
| return { enabled: false } | |
| case 'custom-basepath': | |
| return { enabled: true, basepath: '/custom-styles/' } | |
| case 'default': | |
| return {} // use defaults (enabled=true, basepath=vite base) | |
| } | |
| } | |
| function assertNever(value: never): never { | |
| throw new Error(`Unhandled ssrStylesMode: ${String(value)}`) | |
| } | |
| function getSsrStylesConfig(): { enabled?: boolean; basepath?: string } { | |
| switch (ssrStylesMode) { | |
| case 'disabled': | |
| return { enabled: false } | |
| case 'custom-basepath': | |
| return { enabled: true, basepath: '/custom-styles/' } | |
| case 'default': | |
| return {} // use defaults (enabled=true, basepath=vite base) | |
| default: | |
| return assertNever(ssrStylesMode) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/dev-ssr-styles/vite.config.ts` around lines 7 - 16, Add an
explicit return type for getSsrStylesConfig (e.g., a union type representing the
three possible config shapes) and implement a never-branch guard that throws if
ssrStylesMode is an unhandled SsrStylesMode value; specifically update function
getSsrStylesConfig to declare its return type, keep the existing cases for
'disabled', 'custom-basepath', and 'default', then add a final default case that
calls a helper like assertUnreachable(ssrStylesMode) or throws with the unknown
value to ensure future SsrStylesMode additions do not result in undefined
returns.
Comment on lines
+30
to
+65
| const browser = await chromium.launch() | ||
| const context = await browser.newContext() | ||
| const page = await context.newPage() | ||
|
|
||
| try { | ||
| await page.goto(`${baseURL}/`, { waitUntil: 'domcontentloaded' }) | ||
| await page.getByTestId('home-heading').waitFor({ state: 'visible' }) | ||
| await page.waitForLoadState('networkidle') | ||
|
|
||
| // Exercise client-side navigation so Vite discovers all deps | ||
| await page.getByTestId('link-about').click() | ||
| await page.waitForURL('**/about') | ||
| await page.getByTestId('about-heading').waitFor({ state: 'visible' }) | ||
| await page.waitForLoadState('networkidle') | ||
|
|
||
| await page.getByTestId('link-home').click() | ||
| await page.waitForURL(/\/([^/]*)(\/)?($|\?)/) | ||
| await page.getByTestId('home-heading').waitFor({ state: 'visible' }) | ||
| await page.waitForLoadState('networkidle') | ||
|
|
||
| // Ensure we end in a stable state. Vite's optimize step triggers a reload; | ||
| // this waits until no further navigations happen for a short window. | ||
| for (let i = 0; i < 40; i++) { | ||
| const currentUrl = page.url() | ||
| await page.waitForTimeout(250) | ||
| if (page.url() === currentUrl) { | ||
| await page.waitForTimeout(250) | ||
| if (page.url() === currentUrl) return | ||
| } | ||
| } | ||
|
|
||
| throw new Error('Dev server did not reach a stable URL after warmup') | ||
| } finally { | ||
| await context.close() | ||
| await browser.close() | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n e2e/react-start/split-base-and-basepath/tests/setup/global.setup.ts | head -80Repository: TanStack/router
Length of output: 3145
Move cleanup boundaries to cover resource creation.
try/finally starts after newContext() and newPage(). If either throws, cleanup is skipped, causing resource leaks. Move both creation calls inside the try block with nested try/finally blocks to ensure each resource is cleaned up at the appropriate level.
🔧 Proposed fix
async function preOptimizeDevServer(baseURL: string) {
const browser = await chromium.launch()
- const context = await browser.newContext()
- const page = await context.newPage()
-
try {
+ const context = await browser.newContext()
+ try {
+ const page = await context.newPage()
+
+ await page.goto(`${baseURL}/`, { waitUntil: 'domcontentloaded' })
+ await page.getByTestId('home-heading').waitFor({ state: 'visible' })
+ await page.waitForLoadState('networkidle')
+
+ // Exercise client-side navigation so Vite discovers all deps
+ await page.getByTestId('link-about').click()
+ await page.waitForURL('**/about')
+ await page.getByTestId('about-heading').waitFor({ state: 'visible' })
+ await page.waitForLoadState('networkidle')
+
+ await page.getByTestId('link-home').click()
+ await page.waitForURL(/\/([^/]*)(\/)?($|\?)/)
+ await page.getByTestId('home-heading').waitFor({ state: 'visible' })
+ await page.waitForLoadState('networkidle')
+
+ // Ensure we end in a stable state. Vite's optimize step triggers a reload;
+ // this waits until no further navigations happen for a short window.
+ for (let i = 0; i < 40; i++) {
+ const currentUrl = page.url()
+ await page.waitForTimeout(250)
+ if (page.url() === currentUrl) {
+ await page.waitForTimeout(250)
+ if (page.url() === currentUrl) return
+ }
+ }
+
+ throw new Error('Dev server did not reach a stable URL after warmup')
+ } finally {
+ await context.close()
+ }
- await page.goto(`${baseURL}/`, { waitUntil: 'domcontentloaded' })
- await page.getByTestId('home-heading').waitFor({ state: 'visible' })
- await page.waitForLoadState('networkidle')
-
- // Exercise client-side navigation so Vite discovers all deps
- await page.getByTestId('link-about').click()
- await page.waitForURL('**/about')
- await page.getByTestId('about-heading').waitFor({ state: 'visible' })
- await page.waitForLoadState('networkidle')
-
- await page.getByTestId('link-home').click()
- await page.waitForURL(/\/([^/]*)(\/)?($|\?)/)
- await page.getByTestId('home-heading').waitFor({ state: 'visible' })
- await page.waitForLoadState('networkidle')
-
- // Ensure we end in a stable state. Vite's optimize step triggers a reload;
- // this waits until no further navigations happen for a short window.
- for (let i = 0; i < 40; i++) {
- const currentUrl = page.url()
- await page.waitForTimeout(250)
- if (page.url() === currentUrl) {
- await page.waitForTimeout(250)
- if (page.url() === currentUrl) return
- }
- }
-
- throw new Error('Dev server did not reach a stable URL after warmup')
} finally {
- await context.close()
await browser.close()
}
}📝 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 browser = await chromium.launch() | |
| const context = await browser.newContext() | |
| const page = await context.newPage() | |
| try { | |
| await page.goto(`${baseURL}/`, { waitUntil: 'domcontentloaded' }) | |
| await page.getByTestId('home-heading').waitFor({ state: 'visible' }) | |
| await page.waitForLoadState('networkidle') | |
| // Exercise client-side navigation so Vite discovers all deps | |
| await page.getByTestId('link-about').click() | |
| await page.waitForURL('**/about') | |
| await page.getByTestId('about-heading').waitFor({ state: 'visible' }) | |
| await page.waitForLoadState('networkidle') | |
| await page.getByTestId('link-home').click() | |
| await page.waitForURL(/\/([^/]*)(\/)?($|\?)/) | |
| await page.getByTestId('home-heading').waitFor({ state: 'visible' }) | |
| await page.waitForLoadState('networkidle') | |
| // Ensure we end in a stable state. Vite's optimize step triggers a reload; | |
| // this waits until no further navigations happen for a short window. | |
| for (let i = 0; i < 40; i++) { | |
| const currentUrl = page.url() | |
| await page.waitForTimeout(250) | |
| if (page.url() === currentUrl) { | |
| await page.waitForTimeout(250) | |
| if (page.url() === currentUrl) return | |
| } | |
| } | |
| throw new Error('Dev server did not reach a stable URL after warmup') | |
| } finally { | |
| await context.close() | |
| await browser.close() | |
| } | |
| const browser = await chromium.launch() | |
| try { | |
| const context = await browser.newContext() | |
| try { | |
| const page = await context.newPage() | |
| await page.goto(`${baseURL}/`, { waitUntil: 'domcontentloaded' }) | |
| await page.getByTestId('home-heading').waitFor({ state: 'visible' }) | |
| await page.waitForLoadState('networkidle') | |
| // Exercise client-side navigation so Vite discovers all deps | |
| await page.getByTestId('link-about').click() | |
| await page.waitForURL('**/about') | |
| await page.getByTestId('about-heading').waitFor({ state: 'visible' }) | |
| await page.waitForLoadState('networkidle') | |
| await page.getByTestId('link-home').click() | |
| await page.waitForURL(/\/([^/]*)(\/)?($|\?)/) | |
| await page.getByTestId('home-heading').waitFor({ state: 'visible' }) | |
| await page.waitForLoadState('networkidle') | |
| // Ensure we end in a stable state. Vite's optimize step triggers a reload; | |
| // this waits until no further navigations happen for a short window. | |
| for (let i = 0; i < 40; i++) { | |
| const currentUrl = page.url() | |
| await page.waitForTimeout(250) | |
| if (page.url() === currentUrl) { | |
| await page.waitForTimeout(250) | |
| if (page.url() === currentUrl) return | |
| } | |
| } | |
| throw new Error('Dev server did not reach a stable URL after warmup') | |
| } finally { | |
| await context.close() | |
| } | |
| } finally { | |
| await browser.close() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/split-base-and-basepath/tests/setup/global.setup.ts` around
lines 30 - 65, The try/finally currently begins after creating context and page
so if chromium.launch(), browser.newContext(), or context.newPage() throws those
resources may leak; move the browser/newContext/newPage calls inside the try and
use nested try/finally blocks (one surrounding context creation to ensure
context.close() runs, and an inner one surrounding page creation to ensure
page-related waits/actions and then page.close()/context.close()), i.e., wrap
chromium.launch()/browser in the outer try, create context inside that try and
ensure context.close() in its finally, then create page inside a nested try and
ensure page cleanup in its finally; reference the variables browser, context,
page and the warmup logic so the cleanup always runs even on construction
failure.
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
Release Notes
New Features
Tests