fix(router-ssr): ensure charset meta is placed before dehydration script #5038
Conversation
WalkthroughAdds SSR stream injection logic changes to defer router script insertion based on charset or head end. Introduces a full e2e “react-start/encoding” app with routes, components, client/server entries, configs, and Playwright tests verifying charset-related injection ordering. No public API signature changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SSR as SSR Transform
participant HTML as Incoming HTML Stream
participant Buf as Buffer
participant Out as Output Stream
rect rgba(230,240,255,0.5)
note over SSR,Out: Head/script injection control (updated)
HTML->>SSR: chunk
SSR->>Buf: append chunk
alt body not started
opt detect <head>
SSR->>SSR: headStarted = true
end
alt find <meta charset=...>
SSR->>Out: write prefix + bufferedRouterScript
SSR->>SSR: headScriptInjected = true
else find </head>
SSR->>Out: write prefix + bufferedRouterScript
SSR->>SSR: headScriptInjected = true
end
end
SSR->>Out: write remaining processed chunk
end
HTML-->>SSR: end
SSR->>Out: finalize and close
sequenceDiagram
autonumber
participant PW as Playwright
participant Web as WebServer (Vite-built)
participant App as React Start Server
participant Dummy as Dummy Server
Note over PW: Global setup
PW->>Dummy: start(packageName)
Note over PW,Web: Test run
PW->>Web: build with test env
PW->>App: start with ports (external/server)
PW->>App: navigate /no-charset or /charset
App->>PW: HTML response
PW->>PW: assert charset/script/head ordering
Note over PW: Global teardown
PW->>Dummy: stop(packageName)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/transformStreamWithRouter.ts (1)
243-247: Reset global regex state before each exec loop.
patternClosingTagis global; iflastIndexisn’t reset per chunk, matches can be skipped or the first match in a chunk can be missed.let result: RegExpExecArray | null let lastIndex = 0 + // Reset global regex state for this chunk + patternClosingTag.lastIndex = 0 while ((result = patternClosingTag.exec(chunkString)) !== null) { lastIndex = result.index + result[0].length }
🧹 Nitpick comments (1)
e2e/react-start/basic/tests/charset-encoding.spec.ts (1)
12-14: Remove unreachable return after failing assertion.
expect(html).toBeDefined()will fail the test ifhtmlis undefined; the subsequentreturnis dead code.- expect(html).toBeDefined() - if (!html) return + expect(html).toBeDefined()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
e2e/react-start/basic/tests/charset-encoding.spec.ts(1 hunks)packages/router-core/src/ssr/transformStreamWithRouter.ts(3 hunks)
🔇 Additional comments (2)
e2e/react-start/basic/tests/charset-encoding.spec.ts (1)
4-11: Good regression coverage for charset-before-dehydration ordering.Navigating, capturing SSR HTML, and asserting ordering is the right e2e signal for this bug.
packages/router-core/src/ssr/transformStreamWithRouter.ts (1)
103-103: Clarify injection intent and avoid flipping this flag without output.Only set
headScriptInjectedwhen buffered router HTML was actually written; otherwise, later head flushes get suppressed and content spills into<body>. The diff above guards withrouterStreamBuffer.Would you like me to add a targeted test that simulates delayed
injectedHtmlresolution so we can confirm the head flush occurs only once but still before<body>?
| // Case-insensitive search for charset meta and TSR script | ||
| const htmlLower = html.toLowerCase() | ||
| const charsetIndex = htmlLower.search(/<meta\s+charset=(["'])utf-8\1/) | ||
| const tsrScriptIndex = htmlLower.search(/<script\s+class=(["'])\$tsr\1/) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden selectors: support optional quotes/spacing and avoid lowercasing.
Match <meta charset> with optional quotes/whitespace and allow other attributes; match the $TSR script class among other attributes. This removes the need to lowercase the HTML.
- // Case-insensitive search for charset meta and TSR script
- const htmlLower = html.toLowerCase()
- const charsetIndex = htmlLower.search(/<meta\s+charset=(["'])utf-8\1/)
- const tsrScriptIndex = htmlLower.search(/<script\s+class=(["'])\$tsr\1/)
+ // Case-insensitive search for charset meta and TSR script (robust to spacing/attr order)
+ const charsetRe = /<meta[^>]*\bcharset\s*=\s*(["'])?utf-8\1[^>]*>/i
+ const tsrRe = /<script[^>]*\bclass\s*=\s*(["'])\$tsr\1[^>]*>/i
+ const charsetIndex = html.search(charsetRe)
+ const tsrScriptIndex = html.search(tsrRe)📝 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.
| // Case-insensitive search for charset meta and TSR script | |
| const htmlLower = html.toLowerCase() | |
| const charsetIndex = htmlLower.search(/<meta\s+charset=(["'])utf-8\1/) | |
| const tsrScriptIndex = htmlLower.search(/<script\s+class=(["'])\$tsr\1/) | |
| // Case-insensitive search for charset meta and TSR script (robust to spacing/attr order) | |
| const charsetRe = /<meta[^>]*\bcharset\s*=\s*(["'])?utf-8\1[^>]*>/i | |
| const tsrRe = /<script[^>]*\bclass\s*=\s*(["'])\$tsr\1[^>]*>/i | |
| const charsetIndex = html.search(charsetRe) | |
| const tsrScriptIndex = html.search(tsrRe) |
🤖 Prompt for AI Agents
In e2e/react-start/basic/tests/charset-encoding.spec.ts around lines 15-19, the
current code lowercases the HTML and uses brittle exact-match regexes; replace
that with case-insensitive, attribute-aware regexes that do not mutate the HTML.
Specifically, search the raw html string with patterns that locate a <meta ...
charset=utf-8 ...> allowing optional quotes, optional whitespace, and other
attributes, and similarly locate a <script ... class=...> that contains the
token "$tsr" among other classes/attributes; use the regex i-flag and \b/[^>]*
style matching so selectors are robust to attribute order and spacing and you
can remove the html.toLowerCase() step.
| const patternHeadEnd = /(<\/head>)/ | ||
| const patternCharset = /(<meta\s+charset=(["']?)[^"'>\s]+\2.*?>)/i | ||
| // regex pattern for matching closing tags |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broaden charset detection to handle spacing and additional attributes.
Current pattern requires charset= to appear immediately after <meta and without spaces around =. Make it tolerant.
-const patternHeadEnd = /(<\/head>)/
-const patternCharset = /(<meta\s+charset=(["']?)[^"'>\s]+\2.*?>)/i
+const patternHeadEnd = /(<\/head>)/
+const patternCharset = /(<meta[^>]*\bcharset\s*=\s*(["'])?[^"'>\s]+\2[^>]*>)/i📝 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.
| const patternHeadEnd = /(<\/head>)/ | |
| const patternCharset = /(<meta\s+charset=(["']?)[^"'>\s]+\2.*?>)/i | |
| // regex pattern for matching closing tags | |
| const patternHeadEnd = /(<\/head>)/ | |
| const patternCharset = /(<meta[^>]*\bcharset\s*=\s*(["'])?[^"'>\s]+\2[^>]*>)/i | |
| // regex pattern for matching closing tags |
🤖 Prompt for AI Agents
In packages/router-core/src/ssr/transformStreamWithRouter.ts around lines 27 to
29, the current patternCharset only matches when "charset=" immediately follows
"<meta " with no spaces or other attributes; update the regex to tolerate
arbitrary spacing around "=", allow other attributes before or after "charset",
and be case-insensitive so it matches tags like <meta http-equiv="X" charset =
"utf-8"> or <META charset='utf-8' data-x>. Use a pattern that finds a <meta ...>
element containing a charset attribute (e.g., allow any attributes, optional
quotes, \s* around =, word boundary for charset) and still captures the entire
meta tag for replacement. Ensure the new regex remains global/anchored as needed
and preserves existing capture groups used elsewhere.
|
what if the html does not contain that meta tag? |
@schiller-manuel good point. I added a test for this scenario, and a small fix. I moved the test to separate e2e folder (e2e/react-start/encoding/tests/charset-encoding.spec.ts) since I did not find a way to remove the meta tag for a single route. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/router-core/src/ssr/transformStreamWithRouter.ts (3)
22-26: Make tag detectors case-insensitive and stricterHTML tag names are case-insensitive; also add a word boundary for body start.
-const patternBodyStart = /(<body)/ -const patternBodyEnd = /(<\/body>)/ -const patternHtmlEnd = /(<\/html>)/ -const patternHeadStart = /(<head.*?>)/ +const patternBodyStart = /(<body\b)/i +const patternBodyEnd = /(<\/body>)/i +const patternHtmlEnd = /(<\/html>)/i +const patternHeadStart = /(<head\b[^>]*>)/i
176-179: Bug: stale RegExp indices after mutating chunkStringYou compute body/html end matches before you slice chunkString during injection, then use their indices on the sliced string. This can mis-slice or drop content.
- const bodyEndMatch = chunkString.match(patternBodyEnd) - const htmlEndMatch = chunkString.match(patternHtmlEnd) + // Defer computing these until after any head/charset injection updates chunkString + let bodyEndMatch: RegExpMatchArray | null = null + let htmlEndMatch: RegExpMatchArray | null = nullAnd right before Line 225:
+ // Recompute after any prior mutations to chunkString + bodyEndMatch = chunkString.match(patternBodyEnd) + htmlEndMatch = chunkString.match(patternHtmlEnd)
180-186: Set bodyStarted based on position, not just presence in chunkWhen a chunk contains both head content and , flipping bodyStarted immediately can force later injected HTML to bypass even if the injection point is before in the same chunk.
- if (!bodyStarted) { - const bodyStartMatch = chunkString.match(patternBodyStart) - if (bodyStartMatch) { - bodyStarted = true - } - } + const bodyStartPos = !bodyStarted ? chunkString.search(patternBodyStart) : -1 + if (!bodyStarted && bodyStartPos !== -1) { + // Defer flipping to true until after we handle potential head injection before this position + // bodyStarted = true (set later, after head handling) + }
♻️ Duplicate comments (2)
packages/router-core/src/ssr/transformStreamWithRouter.ts (2)
27-29: Broaden charset detection to handle attributes/spacing (matches more valid markup)Current regex misses cases like
<meta http-equiv="x" charset = "utf-8">. Use a tolerant pattern.-const patternHeadEnd = /(<\/head>)/ -const patternCharset = /(<meta\s+charset=(["']?)[^"'>\s]+\2.*?>)/i +const patternHeadEnd = /(<\/head>)/i +const patternCharset = /(<meta[^>]*\bcharset\s*=\s*(["'])?[^"'>\s]+\2[^>]*>)/i
187-217: Prevent head injection after ; allow an extra flush before
- Ensure the injection index is before when both appear in the same chunk.
- After the first head injection, flush any newly buffered head HTML before (without re-injecting the script).
- if (!headScriptInjected) { - if (!headStarted) { - const headStartMatch = chunkString.match(patternHeadStart) - if (headStartMatch) { - headStarted = true - } - } - - if (headStarted) { - const charsetMatch = chunkString.match(patternCharset) - - if (charsetMatch) { - headScriptInjected = true - const index = charsetMatch.index! + charsetMatch[0]!.length - finalPassThrough.write( - chunkString.slice(0, index) + getBufferedRouterStream(), - ) - chunkString = chunkString.slice(index) - } else { - const headEndMatch = chunkString.match(patternHeadEnd) - if (headEndMatch) { - headScriptInjected = true - const index = headEndMatch.index! - finalPassThrough.write( - chunkString.slice(0, index) + getBufferedRouterStream(), - ) - chunkString = chunkString.slice(index) - } - } - } - } + // Track <head> start + if (!headStarted) { + const headStartMatch = chunkString.match(patternHeadStart) + if (headStartMatch) headStarted = true + } + + if (headStarted) { + const beforeBody = (idx: number) => + bodyStartPos === -1 || idx < bodyStartPos + + const charsetMatch = !headScriptInjected + ? chunkString.match(patternCharset) + : null + const headEndMatch = chunkString.match(patternHeadEnd) + + // Choose first valid injection/flush point before <body> + let injectAt: number | undefined + if (charsetMatch && beforeBody(charsetMatch.index!)) { + injectAt = charsetMatch.index! + charsetMatch[0]!.length + } else if (headEndMatch && beforeBody(headEndMatch.index!)) { + injectAt = headEndMatch.index! + } + + if (typeof injectAt === 'number' && routerStreamBuffer) { + if (charsetMatch && injectAt === charsetMatch.index! + charsetMatch[0]!.length) { + headScriptInjected = true + } + finalPassThrough.write( + chunkString.slice(0, injectAt) + getBufferedRouterStream(), + ) + chunkString = chunkString.slice(injectAt) + } + } + + // Now that we've handled potential head injection, we can flip bodyStarted if needed + if (!bodyStarted && bodyStartPos !== -1) { + bodyStarted = true + }
🧹 Nitpick comments (22)
e2e/react-start/encoding/src/components/NotFound.tsx (1)
3-25: Tighten children typing and prevent implicit form submissionUse ReactNode instead of any, and set button type to avoid accidental submits if rendered inside a form.
-import { Link } from '@tanstack/react-router' +import { Link } from '@tanstack/react-router' +import type { ReactNode } from 'react' -export function NotFound({ children }: { children?: any }) { +export function NotFound({ children }: { children?: ReactNode }) { return ( <div className="space-y-2 p-2" data-testid="default-not-found-component"> <div className="text-gray-600 dark:text-gray-400"> {children || <p>The page you are looking for does not exist.</p>} </div> <p className="flex items-center gap-2 flex-wrap"> - <button + <button + type="button" onClick={() => window.history.back()} className="bg-emerald-500 text-white px-2 py-1 rounded uppercase font-black text-sm" > Go back </button>e2e/react-start/encoding/src/components/CustomMessage.tsx (1)
1-10: Remove unnecessary React importWith the modern JSX transform, the React import isn’t needed here.
-import * as React from 'react' export function CustomMessage({ message }: { message: string }) { return ( <div className="py-2"> <div className="italic">This is a custom message:</div> <p>{message}</p> </div> ) }e2e/react-start/encoding/vite.config.ts (1)
6-8: Avoid hardcoding the dev port; make it configurable to reduce CI collisions.
Use an env-driven port with non-strict binding for friendlier local/dev runs.Apply:
export default defineConfig({ server: { - port: 3000, + port: Number(process.env.PORT) || 3000, + strictPort: false, },e2e/react-start/encoding/package.json (1)
24-37: Assert Node version for JSON import assertions used in tests.
import ... with { type: 'json' }requires modern Node (NodeNext/ESM). Declare engines to prevent CI drift.Apply:
"devDependencies": { "@playwright/test": "^1.50.1", "@tanstack/router-e2e-utils": "workspace:^", "@types/node": "^22.10.2", "@types/react": "^19.0.8", "@types/react-dom": "^19.0.3", "@vitejs/plugin-react": "^4.3.4", "autoprefixer": "^10.4.20", "combinate": "^1.1.11", "postcss": "^8.5.1", "tailwindcss": "^3.4.17", "typescript": "^5.7.2", "vite-tsconfig-paths": "^5.1.4" - } + }, + "engines": { + "node": ">=20.6" + }e2e/react-start/encoding/src/components/DefaultCatchBoundary.tsx (2)
23-30: Prevent accidental form submission by setting button type.
Button defaults to submit; add type to avoid unintended submits when nested in forms.Apply:
- <button + <button + type="button" onClick={() => { router.invalidate() }}
17-17: Add context to error logging.
A short prefix helps triage logs.Apply:
- console.error(error) + console.error('DefaultCatchBoundary error:', error)e2e/react-start/encoding/tests/fixture.ts (2)
8-27: Capture page exceptions and clean up listeners to reduce flakiness.
Also assert after removing listeners so late events don’t leak.Apply:
export const test = base.extend<TestFixtureOptions>({ whitelistErrors: [[], { option: true }], page: async ({ page, whitelistErrors }, use) => { const errorMessages: Array<string> = [] - page.on('console', (m) => { + const onConsole = (m: import('@playwright/test').ConsoleMessage) => { if (m.type() === 'error') { const text = m.text() for (const whitelistError of whitelistErrors) { if ( (typeof whitelistError === 'string' && text.includes(whitelistError)) || (whitelistError instanceof RegExp && whitelistError.test(text)) ) { - return + return } } errorMessages.push(text) } - }) - await use(page) - expect(errorMessages).toEqual([]) + } + const onPageError = (err: Error) => { + const text = String(err && err.stack ? err.stack : err) + for (const whitelistError of whitelistErrors) { + if ( + (typeof whitelistError === 'string' && text.includes(whitelistError)) || + (whitelistError instanceof RegExp && whitelistError.test(text)) + ) { + return + } + } + errorMessages.push(text) + } + page.on('console', onConsole) + page.on('pageerror', onPageError) + try { + await use(page) + } finally { + page.off('console', onConsole) + page.off('pageerror', onPageError) + expect(errorMessages).toEqual([]) + } }, })
3-8: Terminology nit: considerallowedErrorsorignoredErrorsoverwhitelistErrors.
Purely naming; update if aligned with repo conventions.e2e/react-start/encoding/tests/charset-encoding.spec.ts (3)
15-24: Strengthen regexes and assertions to reduce flakiness
- Make charset regex tolerant to optional whitespace and attributes and allow UTF-8 case variations.
- Assert the TSR script appears exactly once.
-const charsetIndex = htmlLower.search(/<meta\s+charset=(["'])utf-8\1/) +const charsetIndex = htmlLower.search(/<meta[^>]*\bcharset\s*=\s*(["'])utf-8\1[^>]*>/) -const tsrScriptIndex = htmlLower.search(/<script\s+class=(["'])\$tsr\1/) +const tsrScriptIndex = htmlLower.search(/<script[^>]*\bclass\s*=\s*(["'])\$tsr\1[^>]*>/) +const tsrMatches = htmlLower.match(/<script[^>]*\bclass\s*=\s*(["'])\$tsr\1[^>]*>/g) || [] +expect(tsrMatches.length).toBe(1)
26-27: Also assert “before ” when no charsetGuard against a chunk where is missing but is present; ensure the TSR script is still before .
-// In the fallback case, the TSR dehydration script should appear BEFORE the closing </head> tag. -expect(tsrScriptIndex).toBeLessThan(headEndIndex) +// In the fallback case, the TSR dehydration script should appear BEFORE </head> (or, at worst, before <body>) +const bodyStartIndex = htmlLower.indexOf('<body') +if (headEndIndex > -1) { + expect(tsrScriptIndex).toBeLessThan(headEndIndex) +} else if (bodyStartIndex > -1) { + expect(tsrScriptIndex).toBeLessThan(bodyStartIndex) +}
40-50: Mirror regex hardening in the second testUse the same tolerant patterns as above to keep both tests consistent.
-const charsetIndex = htmlLower.search(/<meta\s+charset=(["'])utf-8\1/) -const tsrScriptIndex = htmlLower.search(/<script\s+class=(["'])\$tsr\1/) +const charsetIndex = htmlLower.search(/<meta[^>]*\bcharset\s*=\s*(["'])utf-8\1[^>]*>/) +const tsrScriptIndex = htmlLower.search(/<script[^>]*\bclass\s*=\s*(["'])\$tsr\1[^>]*>/)packages/router-core/src/ssr/transformStreamWithRouter.ts (2)
249-260: Avoid losing buffered head HTML when lastIndex == 0If there are no closing tags in the chunk, you append buffered HTML to leftoverHtml, which may end up after due to earlier premature bodyStarted. The fixes above mitigate this; also short-circuit when routerStreamBuffer is empty.
- if (lastIndex > 0) { + if (lastIndex > 0) { const processed = chunkString.slice(0, lastIndex) + getBufferedRouterStream() + leftoverHtml finalPassThrough.write(processed) leftover = chunkString.slice(lastIndex) - } else { + } else if (chunkString) { leftover = chunkString - leftoverHtml += getBufferedRouterStream() + if (routerStreamBuffer) leftoverHtml += getBufferedRouterStream() }
120-170: Minor: ensure injectedHtmlDonePromise settled once; guard multiple resolve/rejectIf multiple injectedHtml promises reject/resolve quickly after isAppRendering flips, double-settlement can occur. Use a local boolean guard.
Happy to provide a small guard patch if you’d like it included here.
e2e/react-start/encoding/src/routes/__root.tsx (1)
51-63: Consider html lang and default charset at the app level (kept out in tests)For production apps, add
<html lang="en">(or configurable) and a default<meta charSet="utf-8" />early in<HeadContent />. Leave the e2e “no-charset” route as-is for regression coverage.e2e/react-start/encoding/src/routes/index.tsx (1)
12-14: Nit: align title/content for clarityOptionally add a simple title to help manual inspection of prerendered output.
-function Page() { - return <h1>Charset</h1> -} +function Page() { + return <h1>Charset</h1> +}e2e/react-start/encoding/tests/setup/global.teardown.ts (1)
4-6: Harden teardown against cleanup failuresA thrown error here can mark the entire run as failed even when tests passed. Wrap in try/catch and warn instead.
-export default async function teardown() { - await e2eStopDummyServer(packageJson.name) -} +export default async function teardown(): Promise<void> { + try { + await e2eStopDummyServer(packageJson.name) + } catch (err) { + // Don't fail the suite on cleanup issues + console.warn('[teardown] Failed to stop dummy server:', err) + } +}e2e/react-start/encoding/src/server.ts (1)
9-9: Use console.info for server boot messageMinor severity level tweak; keeps logs semantically grouped.
-console.log("[server-entry]: using custom server entry in 'src/server.ts'") +console.info("[server-entry]: using custom server entry in 'src/server.ts'")e2e/react-start/encoding/playwright.config.ts (1)
29-34: Consider cross-platform env var handling across all e2e suites
The inline VAR=VALUE pattern appears in every e2e/*/playwright.config.ts; introducingpnpm dlx cross-env …here will diverge from the rest. If Windows compatibility is a priority, apply cross-env uniformly across all suites; otherwise retain the existing inline syntax for consistency.e2e/react-start/encoding/src/router.tsx (1)
10-13: Avoid recreating a component wrapper for NotFoundYou can pass the component directly; no need to wrap it in an inline function.
- defaultNotFoundComponent: () => <NotFound />, + defaultNotFoundComponent: NotFound,e2e/react-start/encoding/src/routeTree.gen.ts (3)
1-10: Mark as generated and ignore by Prettier to avoid churnAdd explicit markers so formatters don’t touch this file in local IDEs/CI.
+/* prettier-ignore-file */ +/* @generated - do not edit */ /* eslint-disable */ // @ts-nocheck
22-36: Generated.update({...} as any)is acceptable; consider tightening types only if touching the generatorNo action needed here for the PR; if you modify the generator later, prefer eliminating
as anyvia precise generics.
68-92: Module augmentation scope—avoid cross-app collisionsIf multiple e2e apps are compiled together, interface merging on
FileRoutesByPathcan collide (e.g., multiple'/'entries). Keep each e2e app isolated via separate TS project references or exclude others from the tsconfig used by the test runner.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (24)
e2e/react-start/encoding/.gitignore(1 hunks)e2e/react-start/encoding/.prettierignore(1 hunks)e2e/react-start/encoding/package.json(1 hunks)e2e/react-start/encoding/playwright.config.ts(1 hunks)e2e/react-start/encoding/postcss.config.mjs(1 hunks)e2e/react-start/encoding/src/client.tsx(1 hunks)e2e/react-start/encoding/src/components/CustomMessage.tsx(1 hunks)e2e/react-start/encoding/src/components/DefaultCatchBoundary.tsx(1 hunks)e2e/react-start/encoding/src/components/NotFound.tsx(1 hunks)e2e/react-start/encoding/src/routeTree.gen.ts(1 hunks)e2e/react-start/encoding/src/router.tsx(1 hunks)e2e/react-start/encoding/src/routes/__root.tsx(1 hunks)e2e/react-start/encoding/src/routes/charset.tsx(1 hunks)e2e/react-start/encoding/src/routes/index.tsx(1 hunks)e2e/react-start/encoding/src/routes/no-charset.tsx(1 hunks)e2e/react-start/encoding/src/server.ts(1 hunks)e2e/react-start/encoding/tailwind.config.mjs(1 hunks)e2e/react-start/encoding/tests/charset-encoding.spec.ts(1 hunks)e2e/react-start/encoding/tests/fixture.ts(1 hunks)e2e/react-start/encoding/tests/setup/global.setup.ts(1 hunks)e2e/react-start/encoding/tests/setup/global.teardown.ts(1 hunks)e2e/react-start/encoding/tsconfig.json(1 hunks)e2e/react-start/encoding/vite.config.ts(1 hunks)packages/router-core/src/ssr/transformStreamWithRouter.ts(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- e2e/react-start/encoding/.gitignore
- e2e/react-start/encoding/tsconfig.json
- e2e/react-start/encoding/.prettierignore
🧰 Additional context used
🧬 Code graph analysis (11)
e2e/react-start/encoding/src/components/NotFound.tsx (5)
examples/react/start-supabase-basic/app/components/NotFound.tsx (1)
NotFound(3-25)examples/react/start-basic-static/app/components/NotFound.tsx (1)
NotFound(3-25)examples/react/start-clerk-basic/app/components/NotFound.tsx (1)
NotFound(3-25)examples/react/start-convex-trellaux/app/components/NotFound.tsx (1)
NotFound(3-25)examples/react/start-basic-auth/app/components/NotFound.tsx (1)
NotFound(3-25)
e2e/react-start/encoding/src/routes/charset.tsx (2)
e2e/react-start/encoding/src/routes/__root.tsx (1)
Route(13-31)e2e/react-start/encoding/src/routes/no-charset.tsx (1)
Route(1-3)
e2e/react-start/encoding/src/routes/no-charset.tsx (3)
e2e/react-start/encoding/src/routes/__root.tsx (1)
Route(13-31)e2e/react-start/encoding/src/routes/charset.tsx (1)
Route(1-10)e2e/react-start/encoding/src/routes/index.tsx (1)
Route(1-10)
e2e/react-start/encoding/src/router.tsx (2)
e2e/react-start/encoding/src/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)e2e/react-start/encoding/src/components/NotFound.tsx (1)
NotFound(3-25)
e2e/react-start/encoding/src/client.tsx (1)
e2e/react-start/encoding/src/router.tsx (1)
createRouter(6-16)
e2e/react-start/encoding/src/components/DefaultCatchBoundary.tsx (6)
examples/react/start-clerk-basic/app/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)examples/react/start-basic-rsc/app/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)examples/react/start-basic-auth/app/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)examples/react/start-basic-react-query/app/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)examples/react/start-supabase-basic/app/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(11-54)examples/react/start-basic-static/app/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)
e2e/react-start/encoding/src/routes/index.tsx (3)
e2e/react-start/encoding/src/routes/__root.tsx (1)
Route(13-31)e2e/react-start/encoding/src/routes/charset.tsx (1)
Route(1-10)e2e/react-start/encoding/src/routes/no-charset.tsx (1)
Route(1-3)
e2e/react-start/encoding/tests/setup/global.setup.ts (1)
scripts/set-ts-version.js (1)
packageJson(33-33)
e2e/react-start/encoding/tests/charset-encoding.spec.ts (1)
e2e/react-start/encoding/tests/fixture.ts (1)
test(6-28)
e2e/react-start/encoding/playwright.config.ts (1)
scripts/set-ts-version.js (1)
packageJson(33-33)
e2e/react-start/encoding/src/routes/__root.tsx (2)
e2e/react-start/encoding/src/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)e2e/react-start/encoding/src/components/NotFound.tsx (1)
NotFound(3-25)
🔇 Additional comments (12)
e2e/react-start/encoding/postcss.config.mjs (1)
1-6: LGTMConfig is minimal and correct for Tailwind + Autoprefixer in an ESM PostCSS setup.
e2e/react-start/encoding/src/routes/charset.tsx (1)
1-10: No action required — the existing e2e test (charset-encoding.spec.ts) for the/no-charsetroute already asserts that the dehydration script is injected before</head>when no<meta charSet>tag is present.e2e/react-start/encoding/tailwind.config.mjs (1)
1-4: LGTMContent glob targets the TSX/JSX sources appropriately for this e2e fixture.
e2e/react-start/encoding/vite.config.ts (1)
9-14: Verify React plugin registration in Vite config
No explicit@vitejs/plugin-reactimport orreact()invocation exists in anyvite.config.*. Confirm thattanstackStartautomatically wires the React plugin; if it doesn’t, import and add it before other plugins to ensure proper HMR/JSX transforms:+import react from '@vitejs/plugin-react' plugins: [ + react(), tsConfigPaths({ projects: ['./tsconfig.json'] }), tanstackStart({ tsr: { verboseFileRoutes: false } }), ],File: e2e/react-start/encoding/vite.config.ts:9-14
e2e/react-start/encoding/package.json (1)
21-31: Compatibility confirmed: @vitejs/plugin-react@^4.3.4 supports Vite 6.3.5. No action required.e2e/react-start/encoding/tests/setup/global.setup.ts (1)
4-6: LGTM — concise and aligned with teardown pairing.e2e/react-start/encoding/src/client.tsx (1)
12-19: LGTMHydration entry looks correct; startTransition + hydrateRoot with StartClient is standard.
e2e/react-start/encoding/src/routeTree.gen.ts (5)
11-15: Type-only imports look correctImports align with Router/Start types and won’t pull runtime code.
94-144: Route module typings are consistentThe
declare module './routes/*'blocks correctly specializeCreateFileRoute/CreateServerFileRoutefor each path.
146-153: routeTree composition LGTMChildren map and
_addFileTypes<FileRouteTypes>()wiring look correct.
17-21: No action needed: route modules exist and exportRoute. All three files—index.tsx,charset.tsx, andno-charset.tsx—were verified to exportRoute.
38-66: Good: explicit entries for/,/charset, and/no-charsetVerified ine2e/react-start/encoding/tests/charset-encoding.spec.tsthat both/charset(line 34) and/no-charset(line 9) are exercised.
| "scripts": { | ||
| "dev": "vite dev --port 3000", | ||
| "dev:e2e": "vite dev", | ||
| "build": "vite build && tsc --noEmit", | ||
| "start": "node .output/server/index.mjs", | ||
| "test:e2e": "rm -rf port*.txt; playwright test --project=chromium" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make test cleanup cross‑platform.
rm -rf breaks on Windows runners. Use rimraf.
Apply:
"scripts": {
"dev": "vite dev --port 3000",
"dev:e2e": "vite dev",
"build": "vite build && tsc --noEmit",
"start": "node .output/server/index.mjs",
- "test:e2e": "rm -rf port*.txt; playwright test --project=chromium"
+ "test:e2e": "rimraf port*.txt && playwright test --project=chromium"
},And add devDependency:
"devDependencies": {
+ "rimraf": "^6.0.0",📝 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.
| "scripts": { | |
| "dev": "vite dev --port 3000", | |
| "dev:e2e": "vite dev", | |
| "build": "vite build && tsc --noEmit", | |
| "start": "node .output/server/index.mjs", | |
| "test:e2e": "rm -rf port*.txt; playwright test --project=chromium" | |
| }, | |
| "scripts": { | |
| "dev": "vite dev --port 3000", | |
| "dev:e2e": "vite dev", | |
| "build": "vite build && tsc --noEmit", | |
| "start": "node .output/server/index.mjs", | |
| "test:e2e": "rimraf port*.txt && playwright test --project=chromium" | |
| }, | |
| "devDependencies": { | |
| "rimraf": "^6.0.0" | |
| } |
🤖 Prompt for AI Agents
In e2e/react-start/encoding/package.json around lines 6 to 12, the test:e2e
script uses "rm -rf port*.txt" which fails on Windows; replace that removal with
a cross-platform tool: add rimraf as a devDependency (npm i -D rimraf or yarn
add -D rimraf) and update the "test:e2e" script to invoke rimraf to delete
port*.txt (e.g. use rimraf port*.txt && playwright test --project=chromium) so
cleanup works on all CI runners.
| export const Route = createFileRoute({ | ||
| head: () => ({ | ||
| meta: [ | ||
| { | ||
| charSet: 'utf-8', | ||
| }, | ||
| ], | ||
| }), | ||
| component: Page, | ||
| }) |
There was a problem hiding this comment.
Missing import for createFileRoute breaks build
This file uses createFileRoute but does not import it.
Apply:
+import { createFileRoute } from '@tanstack/react-router'
export const Route = createFileRoute({
head: () => ({
meta: [
{
charSet: 'utf-8',
},
],
}),
component: Page,
})📝 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.
| export const Route = createFileRoute({ | |
| head: () => ({ | |
| meta: [ | |
| { | |
| charSet: 'utf-8', | |
| }, | |
| ], | |
| }), | |
| component: Page, | |
| }) | |
| import { createFileRoute } from '@tanstack/react-router' | |
| export const Route = createFileRoute({ | |
| head: () => ({ | |
| meta: [ | |
| { | |
| charSet: 'utf-8', | |
| }, | |
| ], | |
| }), | |
| component: Page, | |
| }) |
🤖 Prompt for AI Agents
In e2e/react-start/encoding/src/routes/charset.tsx around lines 1 to 10, the
file calls createFileRoute but never imports it; add the missing import
statement at the top of the file importing createFileRoute from the correct
package used in this project (for example, import { createFileRoute } from
'@remix-run/react' or from the library where createFileRoute is exported),
ensuring the import appears before the export so the symbol is defined and the
build no longer fails.
| export const Route = createFileRoute({ | ||
| head: () => ({ | ||
| meta: [ | ||
| { | ||
| charSet: 'utf-8', | ||
| }, | ||
| ], | ||
| }), | ||
| component: Page, | ||
| }) |
There was a problem hiding this comment.
Missing import for createFileRoute
This file uses createFileRoute but doesn’t import it; build will fail.
+import { createFileRoute } from '@tanstack/react-router'
export const Route = createFileRoute({
head: () => ({
meta: [
{
charSet: 'utf-8',
},
],
}),
component: Page,
})📝 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.
| export const Route = createFileRoute({ | |
| head: () => ({ | |
| meta: [ | |
| { | |
| charSet: 'utf-8', | |
| }, | |
| ], | |
| }), | |
| component: Page, | |
| }) | |
| import { createFileRoute } from '@tanstack/react-router' | |
| export const Route = createFileRoute({ | |
| head: () => ({ | |
| meta: [ | |
| { | |
| charSet: 'utf-8', | |
| }, | |
| ], | |
| }), | |
| component: Page, | |
| }) |
🤖 Prompt for AI Agents
In e2e/react-start/encoding/src/routes/index.tsx around lines 1 to 10, the file
uses createFileRoute but doesn’t import it; add an import statement for
createFileRoute from its module (likely from the routing framework used in the
project, e.g., 'vite-plugin-ssr' or the app's route utilities) at the top of the
file. Ensure the import name matches the exported symbol and update any path to
the correct package/file; then save and run the build to confirm the
missing-import error is resolved.
| export const Route = createFileRoute({ | ||
| component: Page, | ||
| }) | ||
|
|
||
| function Page() { | ||
| return <h1>No charset</h1> | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Make the "no charset" intent explicit; optionally verify auto-import of createFileRoute
This route looks good for exercising the fallback path. Consider a short comment to prevent “fixers” from adding a charset later. Also, if the build relies on the router plugin to auto-inject imports, ignore; otherwise add the explicit import.
Add an intent comment:
+// Intentionally omits any <meta charSet> to exercise SSR dehydration injection fallback
export const Route = createFileRoute({
component: Page,
})To verify auto-imports are configured (no action if true), run:
🏁 Script executed:
#!/bin/bash
# Check if this package relies on plugin-based auto-imports or explicit imports
rg -nP "import\\s+\\{\\s*createFileRoute\\s*\\}\\s+from\\s+'@tanstack/react-router'" e2e/react-start/encoding/src/routes -C1 || true
rg -n "router-plugin|createFileRoute" e2e/react-start/encoding/vite.config.ts -C3 || trueLength of output: 223
Add intent comment and explicit import
+// Intentionally omits any <meta charSet> to exercise SSR dehydration injection fallback
+import { createFileRoute } from '@tanstack/react-router'
export const Route = createFileRoute({
component: Page,
})📝 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.
| export const Route = createFileRoute({ | |
| component: Page, | |
| }) | |
| function Page() { | |
| return <h1>No charset</h1> | |
| } | |
| // Intentionally omits any <meta charSet> to exercise SSR dehydration injection fallback | |
| import { createFileRoute } from '@tanstack/react-router' | |
| export const Route = createFileRoute({ | |
| component: Page, | |
| }) | |
| function Page() { | |
| return <h1>No charset</h1> | |
| } |
🤖 Prompt for AI Agents
e2e/react-start/encoding/src/routes/no-charset.tsx lines 1-7: this file defines
a route using createFileRoute but lacks an explicit import and an intent
comment; add a top-line intent comment (e.g. // intent:route or project-standard
intent annotation) and add an explicit import for createFileRoute from the
module in your app (insert the correct module path used in the repo) so the file
clearly declares its purpose and dependencies.
| import { test as base, expect } from '@playwright/test' | ||
|
|
||
| export interface TestFixtureOptions { | ||
| whitelistErrors: Array<RegExp | string> | ||
| } | ||
| export const test = base.extend<TestFixtureOptions>({ | ||
| whitelistErrors: [[], { option: true }], | ||
| page: async ({ page, whitelistErrors }, use) => { | ||
| const errorMessages: Array<string> = [] | ||
| page.on('console', (m) => { | ||
| if (m.type() === 'error') { | ||
| const text = m.text() | ||
| for (const whitelistError of whitelistErrors) { | ||
| if ( | ||
| (typeof whitelistError === 'string' && | ||
| text.includes(whitelistError)) || | ||
| (whitelistError instanceof RegExp && whitelistError.test(text)) | ||
| ) { | ||
| return | ||
| } | ||
| } | ||
| errorMessages.push(text) | ||
| } | ||
| }) | ||
| await use(page) | ||
| expect(errorMessages).toEqual([]) | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure there’s a test for “no meta charset” fallback.
Given the PR’s goal, validate injection order when the charset meta tag is absent.
Run:
If missing, I can draft a test variant mirroring the “charset” case.
🏁 Script executed:
#!/bin/bash
# Expect to find routes/tests referencing "no-charset"
rg -nP --glob '!**/node_modules/**' '(no-charset|no_charset|without charset)'Length of output: 2392
Add injection-order assertion for no-charset fallback
The existing test in e2e/react-start/encoding/tests/charset-encoding.spec.ts (lines 8–9) covers the “no-charset” route but doesn’t assert that the injected <meta charset> tag appears before other head elements. Add an assertion to verify its position in the document head.
🤖 Prompt for AI Agents
e2e/react-start/encoding/tests/fixture.ts around lines 1 to 27: update the
charset test (e2e/react-start/encoding/tests/charset-encoding.spec.ts, lines
~8–9) to assert the injected <meta charset> is the first/head-most element by
checking the head ordering (e.g., ensure document.head.firstElementChild is a
meta with a charset attribute or that the meta[charset] element appears before
other head children); add this assertion after the existing “no-charset” route
check so the test verifies both presence and correct injection order.
|
i think the better solution to this is let react render the dehydration script. will look into implementing this once we are done with the next alpha branch |
@schiller-manuel If you're talking about the approach to testing, I have created unit tests instead of e2e tests in this branch: jsedlacek@0dfe5b0 I can remove the e2e test, and use this unit test instead. |
|
no i mean instead of injecting the script into the stream , let react render it as bootstrap script |
Summary
When server-rendering with dehydration, the router currently injects its dehydration script immediately after the
<head>tag, before any<meta charSet="utf-8"/>declaration. This causes non-ASCII characters in loader data (e.g., Czech, Chinese, emoji) to be misinterpreted in some browsers since the charset is not yet established.Fix
Updated the SSR stream transformation logic so that the dehydration script is injected only after the charset meta tag has been written. This guarantees compliance with the HTML5 requirement that the charset declaration appears before content requiring specific encoding:
<meta charset>Test
Added a regression test at:
router/e2e/react-start/basic/tests/charset-encoding.spec.tsThe test verifies that the charset meta tag now appears before the dehydration script in prerendered output.
Impact
This resolves potential encoding issues for any TanStack Start app with non-ASCII characters in loader data and ensures correct HTML behavior across browsers.
Summary by CodeRabbit
Bug Fixes
Tests
Chores