nitro with vite preview #5983
nitro with vite preview #5983
Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive end-to-end test infrastructure for four project variants: React Start with Nitro (SPA and full-stack) and Solid Start with Nitro (SPA and full-stack). It scaffolds complete projects with routing, components, configuration, and Playwright tests, and modifies the start-plugin-core to integrate Nitro prerender workflows alongside existing Vite-based prerendering. Changes
Sequence Diagram(s)sequenceDiagram
participant Orchestrator as Build/Prerender<br/>Orchestrator
participant PluginCore as start-plugin-core
participant ServerSelector as Server Selection
participant NitroServer as Nitro Server
participant ViteServer as Vite Preview
participant PrerenderEngine as Prerender<br/>Engine
participant Cleanup as Cleanup
Orchestrator->>PluginCore: postServerBuild() or postServerBuildForNitro()
PluginCore->>PluginCore: setupPrerenderConfig(startConfig)
alt Nitro Path Provided
PluginCore->>ServerSelector: nitroServerPath exists
ServerSelector->>NitroServer: startNitroServer(nitroServerPath)
NitroServer->>NitroServer: spawn child process
NitroServer->>NitroServer: discover free port, poll ready
NitroServer-->>ServerSelector: {url, close()}
else Vite Path
PluginCore->>ServerSelector: no nitroServerPath
ServerSelector->>ViteServer: startPreviewServer(configFile)
ViteServer->>ViteServer: build & start preview
ViteServer-->>ServerSelector: preview server instance
end
ServerSelector-->>PrerenderEngine: server URL
PrerenderEngine->>PrerenderEngine: waitForServerReady()
PrerenderEngine->>PrerenderEngine: prerender pages via fetch
PrerenderEngine-->>PluginCore: prerender complete
alt Error During Prerender
PrerenderEngine->>Cleanup: error occurred
Cleanup->>Cleanup: log error
else Success
PrerenderEngine->>Cleanup: prerender done
end
Cleanup->>Cleanup: call cleanup/close handler
alt Nitro Active
Cleanup->>NitroServer: close Nitro child
else Vite Active
Cleanup->>ViteServer: close preview server
end
Cleanup-->>Orchestrator: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit 99e6322
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (17)
packages/start-plugin-core/src/prerender-nitro.ts (4)
28-82: Consider extracting shared prerender configuration logic.The prerender configuration setup (lines 28-61) duplicates logic from
post-server-build.ts. Consider extracting a shared helper to reduce maintenance burden:// In a shared utility file: export function setupPrerenderConfig(startConfig: TanStackStartOutputConfig): void { // Shared prerender.enabled and SPA page logic }This would centralize the prerender setup logic and prevent divergence between the standard and Nitro prerender paths.
86-101: Hardcoded Nitro version may become stale.The
versions: { nitro: '3.0.0' }on line 94 is hardcoded. Consider deriving this from the actual Nitro package version or making it configurable to prevent the metadata from becoming misleading over time.
151-162: Hardcoded shutdown timeout may be insufficient or excessive.The 3500ms wait on line 157 is arbitrary. Consider:
- Making this configurable via
startConfig.prerender.shutdownTimeout- Or implementing a more robust shutdown detection (e.g., polling for process termination)
The current approach may fail on slower systems or waste time on faster ones.
165-190: Consider failing instead of proceeding when server isn't ready.On line 187-189, the code logs a warning but proceeds when the server doesn't become ready. This could lead to confusing downstream failures during prerendering. Consider making this a hard failure or at least configurable:
- logger.warn( - `Server may not be fully ready after ${maxAttempts * delay}ms, proceeding anyway...`, - ) + throw new Error( + `Server did not become ready after ${maxAttempts * delay}ms. Check that the Nitro build completed successfully.`, + )e2e/solid-start/basic-nitro/src/styles/app.css (1)
27-29: Confirm.using-mousefocus handling to avoid a11y regressionsRemoving outlines with
.using-mouse * { outline: none !important; }can hide focus indicators if that class is ever present for keyboard users. Make sure the.using-mouseclass is only applied for true pointer/mouse interaction, or consider narrowing the selector (e.g., to specific components) so keyboard focus remains clearly visible.e2e/solid-start/basic-nitro-spa/src/components/NotFound.tsx (1)
1-25: Tightenchildrentyping instead of usinganyTo keep TS strictness and avoid
any, you can typechildrenusing Solid’s JSX types:-import { Link } from '@tanstack/solid-router' +import type { JSX } from 'solid-js' +import { Link } from '@tanstack/solid-router' -export function NotFound({ children }: { children?: any }) { +export function NotFound({ children }: { children?: JSX.Element }) {This keeps the component strongly typed without changing behavior.
e2e/react-start/basic-nitro/src/components/NotFound.tsx (1)
1-25: UseReactNodeinstead ofanyforchildrenFor stricter typing, you can avoid
anyby typingchildrenas a React node:-import { Link } from '@tanstack/react-router' +import type { ReactNode } from 'react' +import { Link } from '@tanstack/react-router' -export function NotFound({ children }: { children?: any }) { +export function NotFound({ children }: { children?: ReactNode }) {This keeps the component API the same but aligns better with TypeScript strict mode.
e2e/solid-start/basic-nitro-spa/src/utils/seo.ts (1)
1-33: Consider filtering out metas with empty/undefined content
descriptionandkeywordsare optional but their metas are always emitted, so you may end up with tags like<meta name="description" content="undefined">if callers omit them. A small filter pass overtags(e.g. dropping entries withcontent == nullor empty string) would keep the head cleaner, especially in tests that validate meta tags.e2e/solid-start/basic-nitro/src/routes/__root.tsx (1)
16-94: MoveHeadContentinto<head>and consider addinglangon<html>Right now
<HeadContent />is rendered inside<body>, while it’s intended to manage<head>tags. Moving it into the<head>block (alongsideHydrationScript) will produce more correct HTML and better reflect recommended usage. You may also want to add alangattribute on<html>(e.g.<html lang="en">) for accessibility/SEO completeness in this reference fixture.e2e/solid-start/basic-nitro-spa/src/styles/app.css (1)
1-30: Be mindful of focus outlines with.using-mousehelperThe
.using-mouse * { outline: none !important; }rule is fine if you reliably toggle.using-mouseonly for true mouse interaction. Just ensure keyboard users never land in a state where focus outlines are suppressed, or consider scoping this more narrowly if you reuse the pattern beyond tests.e2e/react-start/basic-nitro-spa/src/utils/seo.ts (1)
12-30: Consider filtering tags with undefined content values.When optional parameters (
description,keywords) are not provided, the tags array will include entries withcontent: undefined. Depending on how the meta tags are rendered, this could result in attributes likecontent="undefined"in the HTML.Apply this refactor to filter out undefined values:
export const seo = ({ title, description, keywords, image, }: { title: string description?: string image?: string keywords?: string }) => { const tags = [ { title }, - { name: 'description', content: description }, - { name: 'keywords', content: keywords }, + ...(description ? [{ name: 'description', content: description }] : []), + ...(keywords ? [{ name: 'keywords', content: keywords }] : []), { name: 'twitter:title', content: title }, - { name: 'twitter:description', content: description }, + ...(description ? [{ name: 'twitter:description', content: description }] : []), { name: 'twitter:creator', content: '@tannerlinsley' }, { name: 'twitter:site', content: '@tannerlinsley' }, { name: 'og:type', content: 'website' }, { name: 'og:title', content: title }, - { name: 'og:description', content: description }, + ...(description ? [{ name: 'og:description', content: description }] : []), ...(image ? [ { name: 'twitter:image', content: image }, { name: 'twitter:card', content: 'summary_large_image' }, { name: 'og:image', content: image }, ] : []), ] return tags }e2e/react-start/basic-nitro/src/utils/seo.ts (1)
1-33: Note: Code duplication across e2e fixtures.This SEO utility is duplicated across multiple e2e test directories (
basic-nitroandbasic-nitro-spa). While this keeps each test self-contained, it creates maintenance overhead if changes are needed.The same undefined content value issue noted in
e2e/react-start/basic-nitro-spa/src/utils/seo.tsapplies here.e2e/solid-start/basic-nitro-spa/src/routes/__root.tsx (1)
31-34: Consider adding manifest link.The project includes a
site.webmanifestfile, but the root route'slinksarray doesn't reference it. While this won't break functionality, including the manifest link would complete the PWA setup.links: [ { rel: 'stylesheet', href: appCss }, { rel: 'icon', href: '/favicon.ico' }, + { rel: 'manifest', href: '/site.webmanifest' }, ],e2e/react-start/basic-nitro-spa/src/routes/__root.tsx (1)
30-33: Consider adding manifest link.Similar to the Solid Start version, this root route could reference the
site.webmanifestfile to complete the PWA configuration.links: [ { rel: 'stylesheet', href: appCss }, { rel: 'icon', href: '/favicon.ico' }, + { rel: 'manifest', href: '/site.webmanifest' }, ],e2e/solid-start/basic-nitro/src/routes/index.tsx (1)
4-26: Index route + server function are wired correctlyThe file-based route,
createServerFnusage, andRoute.useLoaderData()pattern all look correct for Solid Start + Nitro. If you want to trim a tiny bit of boilerplate, you could change the route definition to:export const Route = createFileRoute('/')({ loader: getData, component: Home, })but the current version is perfectly fine.
e2e/solid-start/basic-nitro-spa/src/routes/static.tsx (1)
4-26: Static route is well-structured for prerender testsThe
/staticroute cleanly separates the server function (getStaticData) from the component and exposes stable selectors viadata-testidfor Playwright. As with the index route, you could inlineloader: getStaticDatainstead of() => getStaticData(), but the current implementation is already clear.e2e/react-start/basic-nitro/src/routes/__root.tsx (1)
15-92: Root route + shell are correctly wired; optional hex color tweakThe root route setup (head metadata, error/not-found components, and
RootDocumentshell) looks correct and consistent with other React Start examples. One tiny nit: if thatcolorfield on the manifest link is meant to be a hex color, you probably want'#ffffff'instead of'#fffff', though it won’t affect the tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (29)
e2e/react-start/basic-nitro-spa/public/android-chrome-192x192.pngis excluded by!**/*.pnge2e/react-start/basic-nitro-spa/public/android-chrome-512x512.pngis excluded by!**/*.pnge2e/react-start/basic-nitro-spa/public/apple-touch-icon.pngis excluded by!**/*.pnge2e/react-start/basic-nitro-spa/public/favicon-16x16.pngis excluded by!**/*.pnge2e/react-start/basic-nitro-spa/public/favicon-32x32.pngis excluded by!**/*.pnge2e/react-start/basic-nitro-spa/public/favicon.icois excluded by!**/*.icoe2e/react-start/basic-nitro-spa/public/favicon.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/android-chrome-192x192.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/android-chrome-512x512.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/apple-touch-icon.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/favicon-16x16.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/favicon-32x32.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/favicon.icois excluded by!**/*.icoe2e/react-start/basic-nitro/public/favicon.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro-spa/public/android-chrome-192x192.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro-spa/public/android-chrome-512x512.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro-spa/public/apple-touch-icon.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro-spa/public/favicon-16x16.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro-spa/public/favicon-32x32.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro-spa/public/favicon.icois excluded by!**/*.icoe2e/solid-start/basic-nitro-spa/public/favicon.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro/public/android-chrome-192x192.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro/public/android-chrome-512x512.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro/public/apple-touch-icon.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro/public/favicon-16x16.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro/public/favicon-32x32.pngis excluded by!**/*.pnge2e/solid-start/basic-nitro/public/favicon.icois excluded by!**/*.icoe2e/solid-start/basic-nitro/public/favicon.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (75)
e2e/react-start/basic-nitro-spa/.gitignore(1 hunks)e2e/react-start/basic-nitro-spa/package.json(1 hunks)e2e/react-start/basic-nitro-spa/playwright.config.ts(1 hunks)e2e/react-start/basic-nitro-spa/postcss.config.mjs(1 hunks)e2e/react-start/basic-nitro-spa/public/site.webmanifest(1 hunks)e2e/react-start/basic-nitro-spa/src/components/DefaultCatchBoundary.tsx(1 hunks)e2e/react-start/basic-nitro-spa/src/components/NotFound.tsx(1 hunks)e2e/react-start/basic-nitro-spa/src/routeTree.gen.ts(1 hunks)e2e/react-start/basic-nitro-spa/src/router.tsx(1 hunks)e2e/react-start/basic-nitro-spa/src/routes/__root.tsx(1 hunks)e2e/react-start/basic-nitro-spa/src/routes/index.tsx(1 hunks)e2e/react-start/basic-nitro-spa/src/routes/static.tsx(1 hunks)e2e/react-start/basic-nitro-spa/src/styles/app.css(1 hunks)e2e/react-start/basic-nitro-spa/src/utils/seo.ts(1 hunks)e2e/react-start/basic-nitro-spa/tests/app.spec.ts(1 hunks)e2e/react-start/basic-nitro-spa/tsconfig.json(1 hunks)e2e/react-start/basic-nitro-spa/vite.config.ts(1 hunks)e2e/react-start/basic-nitro/.gitignore(1 hunks)e2e/react-start/basic-nitro/.prettierignore(1 hunks)e2e/react-start/basic-nitro/package.json(1 hunks)e2e/react-start/basic-nitro/playwright.config.ts(1 hunks)e2e/react-start/basic-nitro/postcss.config.mjs(1 hunks)e2e/react-start/basic-nitro/public/site.webmanifest(1 hunks)e2e/react-start/basic-nitro/src/components/DefaultCatchBoundary.tsx(1 hunks)e2e/react-start/basic-nitro/src/components/NotFound.tsx(1 hunks)e2e/react-start/basic-nitro/src/routeTree.gen.ts(1 hunks)e2e/react-start/basic-nitro/src/router.tsx(1 hunks)e2e/react-start/basic-nitro/src/routes/__root.tsx(1 hunks)e2e/react-start/basic-nitro/src/routes/index.tsx(1 hunks)e2e/react-start/basic-nitro/src/routes/static.tsx(1 hunks)e2e/react-start/basic-nitro/src/styles/app.css(1 hunks)e2e/react-start/basic-nitro/src/utils/seo.ts(1 hunks)e2e/react-start/basic-nitro/tests/app.spec.ts(1 hunks)e2e/react-start/basic-nitro/tests/setup/global.setup.ts(1 hunks)e2e/react-start/basic-nitro/tests/setup/global.teardown.ts(1 hunks)e2e/react-start/basic-nitro/tsconfig.json(1 hunks)e2e/react-start/basic-nitro/vite.config.ts(1 hunks)e2e/solid-start/basic-nitro-spa/.gitignore(1 hunks)e2e/solid-start/basic-nitro-spa/package.json(1 hunks)e2e/solid-start/basic-nitro-spa/playwright.config.ts(1 hunks)e2e/solid-start/basic-nitro-spa/postcss.config.mjs(1 hunks)e2e/solid-start/basic-nitro-spa/public/site.webmanifest(1 hunks)e2e/solid-start/basic-nitro-spa/src/components/DefaultCatchBoundary.tsx(1 hunks)e2e/solid-start/basic-nitro-spa/src/components/NotFound.tsx(1 hunks)e2e/solid-start/basic-nitro-spa/src/routeTree.gen.ts(1 hunks)e2e/solid-start/basic-nitro-spa/src/router.tsx(1 hunks)e2e/solid-start/basic-nitro-spa/src/routes/__root.tsx(1 hunks)e2e/solid-start/basic-nitro-spa/src/routes/index.tsx(1 hunks)e2e/solid-start/basic-nitro-spa/src/routes/static.tsx(1 hunks)e2e/solid-start/basic-nitro-spa/src/styles/app.css(1 hunks)e2e/solid-start/basic-nitro-spa/src/utils/seo.ts(1 hunks)e2e/solid-start/basic-nitro-spa/tests/app.spec.ts(1 hunks)e2e/solid-start/basic-nitro-spa/tsconfig.json(1 hunks)e2e/solid-start/basic-nitro-spa/vite.config.ts(1 hunks)e2e/solid-start/basic-nitro/.gitignore(1 hunks)e2e/solid-start/basic-nitro/.prettierignore(1 hunks)e2e/solid-start/basic-nitro/package.json(1 hunks)e2e/solid-start/basic-nitro/playwright.config.ts(1 hunks)e2e/solid-start/basic-nitro/postcss.config.mjs(1 hunks)e2e/solid-start/basic-nitro/public/site.webmanifest(1 hunks)e2e/solid-start/basic-nitro/src/components/DefaultCatchBoundary.tsx(1 hunks)e2e/solid-start/basic-nitro/src/components/NotFound.tsx(1 hunks)e2e/solid-start/basic-nitro/src/routeTree.gen.ts(1 hunks)e2e/solid-start/basic-nitro/src/router.tsx(1 hunks)e2e/solid-start/basic-nitro/src/routes/__root.tsx(1 hunks)e2e/solid-start/basic-nitro/src/routes/index.tsx(1 hunks)e2e/solid-start/basic-nitro/src/routes/static.tsx(1 hunks)e2e/solid-start/basic-nitro/src/styles/app.css(1 hunks)e2e/solid-start/basic-nitro/src/utils/seo.ts(1 hunks)e2e/solid-start/basic-nitro/tests/app.spec.ts(1 hunks)e2e/solid-start/basic-nitro/tsconfig.json(1 hunks)e2e/solid-start/basic-nitro/vite.config.ts(1 hunks)packages/start-plugin-core/src/plugin.ts(1 hunks)packages/start-plugin-core/src/post-server-build.ts(2 hunks)packages/start-plugin-core/src/prerender-nitro.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety throughout the codebase
Files:
e2e/react-start/basic-nitro/tests/setup/global.teardown.tse2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro/tests/app.spec.tse2e/react-start/basic-nitro-spa/src/components/NotFound.tsxe2e/react-start/basic-nitro/playwright.config.tse2e/solid-start/basic-nitro/src/components/NotFound.tsxe2e/react-start/basic-nitro-spa/src/components/DefaultCatchBoundary.tsxe2e/react-start/basic-nitro/src/components/NotFound.tsxe2e/react-start/basic-nitro-spa/playwright.config.tse2e/react-start/basic-nitro/src/router.tsxe2e/react-start/basic-nitro/src/routes/static.tsxe2e/react-start/basic-nitro-spa/src/router.tsxe2e/react-start/basic-nitro/src/components/DefaultCatchBoundary.tsxe2e/react-start/basic-nitro-spa/vite.config.tse2e/solid-start/basic-nitro-spa/src/components/DefaultCatchBoundary.tsxe2e/solid-start/basic-nitro-spa/src/utils/seo.tspackages/start-plugin-core/src/post-server-build.tse2e/solid-start/basic-nitro-spa/vite.config.tse2e/solid-start/basic-nitro/tests/app.spec.tse2e/solid-start/basic-nitro/src/components/DefaultCatchBoundary.tsxe2e/solid-start/basic-nitro-spa/src/routes/index.tsxe2e/solid-start/basic-nitro-spa/src/routes/__root.tsxe2e/solid-start/basic-nitro/src/utils/seo.tse2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/static.tsxe2e/react-start/basic-nitro/src/utils/seo.tse2e/solid-start/basic-nitro-spa/src/routeTree.gen.tse2e/solid-start/basic-nitro-spa/src/router.tsxe2e/react-start/basic-nitro-spa/tests/app.spec.tse2e/solid-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro/src/routes/static.tsxe2e/solid-start/basic-nitro/playwright.config.tse2e/solid-start/basic-nitro/src/router.tsxpackages/start-plugin-core/src/prerender-nitro.tse2e/solid-start/basic-nitro-spa/src/routes/static.tsxe2e/solid-start/basic-nitro-spa/src/components/NotFound.tsxe2e/react-start/basic-nitro-spa/src/routes/index.tsxe2e/solid-start/basic-nitro-spa/playwright.config.tse2e/solid-start/basic-nitro/vite.config.tse2e/react-start/basic-nitro/src/routes/index.tsxe2e/react-start/basic-nitro-spa/src/utils/seo.tse2e/solid-start/basic-nitro-spa/tests/app.spec.tse2e/solid-start/basic-nitro/src/routeTree.gen.tse2e/react-start/basic-nitro/vite.config.tse2e/solid-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/tests/setup/global.setup.tspackages/start-plugin-core/src/plugin.tse2e/react-start/basic-nitro/src/routeTree.gen.ts
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace protocol for internal dependencies (workspace:*)
Files:
e2e/react-start/basic-nitro/package.jsone2e/react-start/basic-nitro-spa/package.jsone2e/solid-start/basic-nitro/package.jsone2e/solid-start/basic-nitro-spa/package.json
**/src/routes/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use file-based routing in src/routes/ directories or code-based routing with route definitions
Files:
e2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routes/static.tsxe2e/solid-start/basic-nitro-spa/src/routes/index.tsxe2e/solid-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/src/routes/static.tsxe2e/solid-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro/src/routes/static.tsxe2e/solid-start/basic-nitro-spa/src/routes/static.tsxe2e/react-start/basic-nitro-spa/src/routes/index.tsxe2e/react-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routes/__root.tsx
🧠 Learnings (11)
📚 Learning: 2025-11-25T00:18:21.282Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.282Z
Learning: Applies to packages/react-router/**/*.{ts,tsx} : React Router components and hooks should use the tanstack/react-router package
Applied to files:
e2e/react-start/basic-nitro/package.jsone2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/src/components/NotFound.tsxe2e/react-start/basic-nitro-spa/src/components/DefaultCatchBoundary.tsxe2e/react-start/basic-nitro-spa/package.jsone2e/react-start/basic-nitro/src/components/NotFound.tsxe2e/react-start/basic-nitro/src/router.tsxe2e/react-start/basic-nitro/src/routes/static.tsxe2e/react-start/basic-nitro-spa/src/router.tsxe2e/react-start/basic-nitro/src/components/DefaultCatchBoundary.tsxe2e/solid-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/static.tsxe2e/solid-start/basic-nitro-spa/src/routeTree.gen.tse2e/solid-start/basic-nitro-spa/src/router.tsxe2e/solid-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro/src/router.tsxe2e/react-start/basic-nitro-spa/src/routes/index.tsxe2e/react-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro/src/routeTree.gen.tse2e/solid-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routeTree.gen.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:
e2e/react-start/basic-nitro/package.jsone2e/react-start/basic-nitro/.prettierignoree2e/react-start/basic-nitro-spa/package.jsone2e/react-start/basic-nitro-spa/tsconfig.jsone2e/react-start/basic-nitro/tsconfig.jsone2e/solid-start/basic-nitro-spa/src/routes/index.tsxe2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/static.tsxe2e/solid-start/basic-nitro-spa/src/routeTree.gen.tse2e/solid-start/basic-nitro/tsconfig.jsone2e/solid-start/basic-nitro-spa/tsconfig.jsone2e/solid-start/basic-nitro/package.jsone2e/solid-start/basic-nitro/.prettierignoree2e/react-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro-spa/package.jsone2e/solid-start/basic-nitro/src/routeTree.gen.tse2e/react-start/basic-nitro/src/routeTree.gen.ts
📚 Learning: 2025-11-25T00:18:21.282Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.282Z
Learning: Applies to packages/solid-router/**/*.{ts,tsx} : Solid Router components and primitives should use the tanstack/solid-router package
Applied to files:
e2e/react-start/basic-nitro/package.jsone2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/package.jsone2e/react-start/basic-nitro/src/router.tsxe2e/react-start/basic-nitro/src/routes/static.tsxe2e/react-start/basic-nitro-spa/src/router.tsxe2e/react-start/basic-nitro/src/components/DefaultCatchBoundary.tsxe2e/solid-start/basic-nitro-spa/vite.config.tse2e/solid-start/basic-nitro-spa/src/routes/index.tsxe2e/solid-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/static.tsxe2e/solid-start/basic-nitro-spa/src/routeTree.gen.tse2e/solid-start/basic-nitro-spa/src/router.tsxe2e/solid-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro/src/routes/static.tsxe2e/solid-start/basic-nitro/tsconfig.jsone2e/solid-start/basic-nitro/src/router.tsxe2e/solid-start/basic-nitro-spa/src/routes/static.tsxe2e/solid-start/basic-nitro-spa/tsconfig.jsone2e/solid-start/basic-nitro/package.jsone2e/solid-start/basic-nitro/.prettierignoree2e/react-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro-spa/package.jsone2e/solid-start/basic-nitro/src/routeTree.gen.tse2e/solid-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routeTree.gen.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/react-start/basic-nitro/.prettierignoree2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routes/static.tsxe2e/react-start/basic-nitro-spa/src/router.tsxe2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/static.tsxe2e/solid-start/basic-nitro-spa/src/routeTree.gen.tse2e/solid-start/basic-nitro/src/router.tsxe2e/solid-start/basic-nitro/.prettierignoree2e/react-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro/src/routeTree.gen.tse2e/solid-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routeTree.gen.ts
📚 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:
e2e/react-start/basic-nitro/.prettierignoree2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro/tests/app.spec.tse2e/react-start/basic-nitro/src/routes/static.tsxe2e/react-start/basic-nitro-spa/src/router.tsxe2e/solid-start/basic-nitro/tests/app.spec.tse2e/solid-start/basic-nitro-spa/src/routes/index.tsxe2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/static.tsxe2e/solid-start/basic-nitro-spa/src/routeTree.gen.tse2e/solid-start/basic-nitro-spa/src/router.tsxe2e/react-start/basic-nitro-spa/tests/app.spec.tse2e/solid-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro/src/routes/static.tsxe2e/solid-start/basic-nitro/src/router.tsxe2e/solid-start/basic-nitro-spa/src/routes/static.tsxe2e/solid-start/basic-nitro/.prettierignoree2e/react-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro-spa/tests/app.spec.tse2e/solid-start/basic-nitro/src/routeTree.gen.tse2e/solid-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routeTree.gen.ts
📚 Learning: 2025-11-25T00:18:21.282Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.282Z
Learning: Applies to **/src/routes/**/*.{ts,tsx} : Use file-based routing in src/routes/ directories or code-based routing with route definitions
Applied to files:
e2e/react-start/basic-nitro/.prettierignoree2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro/src/router.tsxe2e/react-start/basic-nitro/src/routes/static.tsxe2e/react-start/basic-nitro-spa/src/router.tsxe2e/solid-start/basic-nitro-spa/src/routes/index.tsxe2e/solid-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/static.tsxe2e/solid-start/basic-nitro-spa/src/routeTree.gen.tse2e/solid-start/basic-nitro-spa/src/router.tsxe2e/solid-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro/src/routes/static.tsxe2e/solid-start/basic-nitro/src/router.tsxe2e/solid-start/basic-nitro-spa/src/routes/static.tsxe2e/react-start/basic-nitro-spa/src/routes/index.tsxe2e/solid-start/basic-nitro/.prettierignoree2e/react-start/basic-nitro/src/routes/index.tsxe2e/solid-start/basic-nitro/src/routeTree.gen.tse2e/solid-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routeTree.gen.ts
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
e2e/react-start/basic-nitro-spa/src/routes/__root.tsx
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/react-start/basic-nitro/tests/app.spec.ts
📚 Learning: 2025-10-09T12:59:14.842Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/public/site.webmanifest:2-3
Timestamp: 2025-10-09T12:59:14.842Z
Learning: In e2e test fixtures (files under e2e directories), empty or placeholder values in configuration files like site.webmanifest are acceptable and should not be flagged unless the test specifically validates those fields.
Applied to files:
e2e/react-start/basic-nitro/tests/app.spec.tse2e/react-start/basic-nitro/playwright.config.tse2e/react-start/basic-nitro-spa/playwright.config.tse2e/react-start/basic-nitro-spa/public/site.webmanifeste2e/solid-start/basic-nitro/playwright.config.tse2e/solid-start/basic-nitro/package.jsone2e/solid-start/basic-nitro/public/site.webmanifeste2e/react-start/basic-nitro/public/site.webmanifeste2e/solid-start/basic-nitro-spa/playwright.config.tse2e/solid-start/basic-nitro-spa/public/site.webmanifest
📚 Learning: 2025-11-25T00:18:21.282Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.282Z
Learning: Applies to **/package.json : Use workspace protocol for internal dependencies (workspace:*)
Applied to files:
e2e/react-start/basic-nitro-spa/package.jsone2e/solid-start/basic-nitro-spa/package.json
📚 Learning: 2025-11-25T00:18:21.282Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.282Z
Learning: Applies to **/*.{ts,tsx} : Use TypeScript strict mode with extensive type safety throughout the codebase
Applied to files:
e2e/react-start/basic-nitro-spa/tsconfig.jsone2e/react-start/basic-nitro/tsconfig.jsone2e/solid-start/basic-nitro/tsconfig.jsone2e/solid-start/basic-nitro-spa/tsconfig.json
🧬 Code graph analysis (17)
e2e/react-start/basic-nitro-spa/src/components/NotFound.tsx (2)
e2e/react-start/server-routes/src/components/NotFound.tsx (1)
NotFound(3-25)e2e/react-start/basic-cloudflare/src/components/NotFound.tsx (1)
NotFound(3-25)
e2e/react-start/basic-nitro/playwright.config.ts (1)
scripts/set-ts-version.js (1)
packageJson(33-33)
e2e/react-start/basic-nitro-spa/src/components/DefaultCatchBoundary.tsx (2)
e2e/react-start/server-routes/src/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)examples/react/start-basic-cloudflare/src/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)
e2e/react-start/basic-nitro-spa/playwright.config.ts (1)
scripts/set-ts-version.js (1)
packageJson(33-33)
e2e/react-start/basic-nitro/src/router.tsx (2)
e2e/react-start/basic-cloudflare/src/router.tsx (1)
getRouter(6-16)e2e/solid-start/basic-tsr-config/src/app/router.tsx (1)
createRouter(4-11)
e2e/react-start/basic-nitro/src/components/DefaultCatchBoundary.tsx (2)
packages/router-core/src/router.ts (1)
state(1088-1090)e2e/react-start/server-routes/src/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)
e2e/solid-start/basic-nitro-spa/src/utils/seo.ts (1)
scripts/llms-generate.mjs (1)
title(101-101)
packages/start-plugin-core/src/post-server-build.ts (1)
packages/start-plugin-core/src/schema.ts (1)
TanStackStartOutputConfig(207-207)
e2e/solid-start/basic-nitro/src/components/DefaultCatchBoundary.tsx (2)
examples/solid/start-basic-nitro/src/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)e2e/solid-start/server-routes/src/components/DefaultCatchBoundary.tsx (1)
DefaultCatchBoundary(10-53)
e2e/solid-start/basic-nitro-spa/src/routes/index.tsx (2)
examples/solid/kitchen-sink/src/useMutation.tsx (1)
data(47-49)packages/router-core/src/ssr/tsrScript.ts (1)
p(11-13)
e2e/solid-start/basic-nitro-spa/src/routes/__root.tsx (4)
e2e/solid-start/server-functions/src/routes/__root.tsx (1)
RootComponent(33-47)e2e/solid-start/custom-basepath/src/routes/__root.tsx (1)
RootComponent(56-114)e2e/solid-start/serialization-adapters/src/routes/__root.tsx (1)
RootDocument(35-74)e2e/solid-start/website/src/routes/__root.tsx (1)
RootComponent(59-73)
e2e/solid-start/basic-nitro/src/utils/seo.ts (1)
examples/solid/start-basic-nitro/src/utils/seo.ts (1)
title(1-33)
e2e/solid-start/basic-nitro-spa/src/router.tsx (1)
examples/solid/start-basic-nitro/src/router.tsx (1)
getRouter(6-16)
packages/start-plugin-core/src/prerender-nitro.ts (3)
packages/start-plugin-core/src/schema.ts (2)
TanStackStartOutputConfig(207-207)Page(202-202)packages/router-core/src/route.ts (1)
path(1553-1555)packages/history/src/index.ts (1)
location(162-164)
e2e/solid-start/basic-nitro-spa/src/routes/static.tsx (1)
examples/solid/kitchen-sink/src/useMutation.tsx (1)
data(47-49)
e2e/solid-start/basic-nitro-spa/playwright.config.ts (1)
scripts/set-ts-version.js (1)
packageJson(33-33)
e2e/react-start/basic-nitro-spa/src/utils/seo.ts (1)
scripts/llms-generate.mjs (1)
title(101-101)
| async function localFetch( | ||
| urlPath: string, | ||
| options?: RequestInit, | ||
| maxRedirects: number = 5, | ||
| ): Promise<Response> { | ||
| const url = new URL(urlPath, baseUrl) | ||
| const request = new Request(url, options) | ||
| const response = await fetch(request) | ||
|
|
||
| if (isRedirectResponse(response) && maxRedirects > 0) { | ||
| const location = response.headers.get('location')! | ||
| if (location.startsWith('http://localhost') || location.startsWith('/')) { | ||
| const newUrl = location.replace('http://localhost', '') | ||
| return localFetch(newUrl, options, maxRedirects - 1) | ||
| } else { | ||
| logger.warn(`Skipping redirect to external location: ${location}`) | ||
| } | ||
| } | ||
|
|
||
| return response | ||
| } |
There was a problem hiding this comment.
Redirect URL handling may not preserve port numbers correctly.
Line 125 replaces 'http://localhost' but redirects may include port numbers like 'http://localhost:3000/path'. The current logic would result in :3000/path being passed to the next localFetch call.
if (location.startsWith('http://localhost') || location.startsWith('/')) {
- const newUrl = location.replace('http://localhost', '')
+ // Parse the URL to properly extract the pathname
+ const redirectUrl = new URL(location, baseUrl)
+ const newUrl = redirectUrl.pathname + redirectUrl.search + redirectUrl.hash
return localFetch(newUrl, options, maxRedirects - 1)This ensures the path is correctly extracted regardless of the port in the redirect URL.
🤖 Prompt for AI Agents
In packages/start-plugin-core/src/prerender-nitro.ts around lines 113 to 133,
the redirect handling currently strips only the exact 'http://localhost' prefix
and will leave port numbers (e.g. ':3000/path') in the next localFetch call;
instead parse the redirect Location header into a URL and extract the pathname +
search + hash so port and origin are removed cleanly before recursing. Change
the branch that handles local redirects to detect http(s)://localhost (with
optional port), construct a URL object from location (or relative to baseUrl if
needed), build a new path using url.pathname + url.search + url.hash, and call
localFetch with that new path; keep the existing maxRedirects decrement and
warning behavior for external origins.
| } catch (error) { | ||
| if (retries < (prerenderOptions.retryCount ?? 0)) { | ||
| logger.warn(`Encountered error, retrying: ${page.path} in 500ms`) | ||
| await new Promise((resolve) => | ||
| setTimeout(resolve, prerenderOptions.retryDelay), | ||
| ) | ||
| retriesByPath.set(page.path, retries + 1) | ||
| addCrawlPageTask(page) |
There was a problem hiding this comment.
Log message doesn't match actual retry delay.
Line 367 logs "retrying: ${page.path} in 500ms" but line 369 uses prerenderOptions.retryDelay, which may differ from 500ms.
- logger.warn(`Encountered error, retrying: ${page.path} in 500ms`)
+ logger.warn(`Encountered error, retrying: ${page.path} in ${prerenderOptions.retryDelay ?? 500}ms`)🤖 Prompt for AI Agents
In packages/start-plugin-core/src/prerender-nitro.ts around lines 365 to 372,
the log message hardcodes "in 500ms" but the actual delay uses
prerenderOptions.retryDelay; update the logger to reflect the actual value (e.g.
use the retryDelay variable or prerenderOptions.retryDelay) so the message
matches the real delay, and ensure retryDelay has a sensible default when
undefined before logging.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/start-plugin-core/src/prerender.ts (2)
2-2: Fix import ordering and use top-level type import.The static analysis correctly flags that
ChildProcessshould be sorted beforespawnand should use a top-level type-only import for consistency.-import { spawn, type ChildProcess } from 'node:child_process' +import type { ChildProcess } from 'node:child_process' +import { spawn } from 'node:child_process'
317-321: Consider more robust process shutdown handling.The fixed 500ms wait may not be sufficient for graceful shutdown, and there's no fallback if the process doesn't terminate. Consider waiting for the
'exit'event or adding a SIGKILL fallback.close: async () => { - child.kill('SIGTERM') - // Wait a bit for graceful shutdown - await new Promise((r) => setTimeout(r, 500)) + return new Promise<void>((resolve) => { + const forceKillTimeout = setTimeout(() => { + child.kill('SIGKILL') + }, 5000) + child.once('exit', () => { + clearTimeout(forceKillTimeout) + resolve() + }) + child.kill('SIGTERM') + }) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/react-start/basic-nitro-spa/tests/app.spec.ts(1 hunks)e2e/solid-start/basic-nitro-spa/tests/app.spec.ts(1 hunks)packages/start-plugin-core/src/plugin.ts(3 hunks)packages/start-plugin-core/src/post-server-build.ts(3 hunks)packages/start-plugin-core/src/prerender.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/react-start/basic-nitro-spa/tests/app.spec.ts
- e2e/solid-start/basic-nitro-spa/tests/app.spec.ts
- packages/start-plugin-core/src/plugin.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety throughout the codebase
Files:
packages/start-plugin-core/src/post-server-build.tspackages/start-plugin-core/src/prerender.ts
🧬 Code graph analysis (1)
packages/start-plugin-core/src/prerender.ts (2)
packages/start-plugin-core/src/schema.ts (1)
TanStackStartOutputConfig(207-207)packages/start-plugin-core/src/constants.ts (1)
VITE_ENVIRONMENT_NAMES(1-6)
🪛 ESLint
packages/start-plugin-core/src/prerender.ts
[error] 2-2: Member 'ChildProcess' of the import declaration should be sorted alphabetically.
(sort-imports)
[error] 2-2: Prefer using a top-level type-only import instead of inline type specifiers.
(import/consistent-type-specifier-style)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (8)
packages/start-plugin-core/src/prerender.ts (5)
12-65: LGTM - Well-structured function signature and output resolution.The updated function signature properly handles the dual-mode operation (Nitro vs Vite), with clear type definitions and appropriate error handling when neither
buildernoroutputDiris provided.
69-87: LGTM - Clean dual-backend abstraction.The server startup logic properly abstracts both Nitro and Vite preview backends with a unified cleanup interface, enabling consistent resource management in the
finallyblock.
309-330: Server readiness check accepts 4xx responses.The check
res.ok || res.status < 500will consider the server ready even for 4xx client errors (e.g., 404, 401). If the root endpoint requires authentication or doesn't exist, this could lead to premature resolution. Consider checking only forres.okor specific expected status codes if the root path is guaranteed to return a success response.
115-128: LGTM - Proper error handling and cleanup.Good improvement: errors are now logged and rethrown rather than swallowed, and the cleanup function is consistently called in the
finallyblock regardless of success or failure.
348-369: LGTM!Clean update to accept an optional
configFileparameter with proper error wrapping.packages/start-plugin-core/src/post-server-build.ts (3)
8-42: LGTM - Good extraction of shared configuration logic.The
setupPrerenderConfighelper cleanly consolidates the prerender configuration logic that was previously duplicated, enabling reuse betweenpostServerBuildandpostServerBuildForNitro.
44-70: LGTM!Clean refactoring with the new
skipPrerenderparameter enabling the caller to defer prerendering to a later stage (e.g., post-Nitro build).
72-97: LGTM - Well-structured Nitro-specific post-build handler.The function correctly mirrors the structure of
postServerBuildwhile using Nitro-specific parameters (outputDir,nitroServerPath) instead of the Vite builder. The reuse ofsetupPrerenderConfigensures consistent behavior across both paths.
| async function startNitroServer( | ||
| serverPath: string, | ||
| ): Promise<{ url: URL; close: () => Promise<void> }> { | ||
| return new Promise((resolve, reject) => { | ||
| // Find a random port | ||
| const port = 3000 + Math.floor(Math.random() * 10000) | ||
| const env = { ...process.env, PORT: String(port) } | ||
|
|
||
| const child: ChildProcess = spawn('node', [serverPath], { | ||
| env, | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the prerender.ts file
find . -type f -name "prerender.ts" | head -20Repository: TanStack/router
Length of output: 226
🏁 Script executed:
# Get file line count and examine the function
wc -l ./packages/start-plugin-core/src/prerender.tsRepository: TanStack/router
Length of output: 108
🏁 Script executed:
# Read the complete function starting from line 280
sed -n '280,350p' ./packages/start-plugin-core/src/prerender.tsRepository: TanStack/router
Length of output: 1764
🏁 Script executed:
# Check if get-port is already a dependency
cat ./packages/start-plugin-core/package.json | grep -A 20 "dependencies"Repository: TanStack/router
Length of output: 803
🏁 Script executed:
# Search for all usages of startNitroServer function
rg -n "startNitroServer" ./packages/start-plugin-core/src/prerender.tsRepository: TanStack/router
Length of output: 165
🏁 Script executed:
# Check the context where startNitroServer is called
sed -n '60,90p' ./packages/start-plugin-core/src/prerender.tsRepository: TanStack/router
Length of output: 953
🏁 Script executed:
# Check if there's any error handling or retry logic for port issues elsewhere
rg -n "EADDRINUSE|port.*collision|port.*in.*use" ./packages/start-plugin-core/src/ -iRepository: TanStack/router
Length of output: 41
🏁 Script executed:
# Let's look at the imports to understand what packages are available
head -30 ./packages/start-plugin-core/src/prerender.tsRepository: TanStack/router
Length of output: 1031
🌐 Web query:
Node.js child_process port 0 OS assign available port
💡 Result:
Short answer: bind the child to port 0 (server.listen(0)) so the OS picks an available port, then read the actual port from server.address().port and communicate it back to the parent (e.g. via IPC or stdout).
Why/how (references):
- The OS assigns an unused port when you pass 0 to listen/bind; Node exposes the chosen port in server.address().port. [1][2][3]
Minimal example (child + parent using fork + IPC):
-
child.js
const http = require('http');
const server = http.createServer((req,res)=>res.end('ok'));
server.listen(0, ()=> {
const port = server.address().port;
if (process.send) process.send({port}); // send to parent when forked
console.log('listening', port); // or stdout
}); -
parent.js
const { fork } = require('child_process');
const child = fork('./child.js');
child.on('message', msg => console.log('child port', msg.port));
References:
- Node.js child_process docs (server handle / IPC examples). [Node.js docs]. [4]
- StackOverflow / Node patterns: use port 0 and server.address().port to get assigned port. [3][5]
- (--inspect=0 note) Node can auto-assign debugger ports with =0 when launching processes. [0]
(If you want, I can paste a runnable snippet or show using spawn + stdout parsing instead of fork.)
🌐 Web query:
Nitro server startup output port logging console
💡 Result:
Do you mean Nitro (the Nuxt / nitro.build server runtime) or AWS Nitro (EC2 Nitro Enclaves)?
🌐 Web query:
nitro.build server startup console output messages
💡 Result:
Typical Nitro server startup console messages (common outputs you’ll see)
- "Nitro vX.Y.Z" or framework banner (on some installs). [1]
- Dev server ready / listening message, e.g. "Nitro is ready at http://localhost:3000" or "Listening on http://localhost:3000". [2]
- Runtime preset / entry info when running built server (node .output/server/index.mjs). [1][2]
- Environment / port info (reads NITRO_PORT / PORT, NITRO_HOST / HOST). [1]
- Plugin/Hook logs: any console.log() from server/plugins run during startup (e.g. "Nitro plugin ..."). [3]
- Warnings or errors about missing SSL keys, UNIX socket usage, or shutdown handling if relevant environment vars are set (NITRO_SSL_CERT / NITRO_SSL_KEY, NITRO_UNIX_SOCKET, NITRO_SHUTDOWN_*). [1]
Sources:
[1] Nitro — Node.js runtime / deploy docs.
[2] Nitro — Getting Started / dev server messages.
[3] Nitro — Plugins (plugin console output runs at startup).
🌐 Web query:
Node.js child_process stdio pipe buffer size limit blocking
💡 Result:
Short answer
- Child stdio use OS pipes with limited, platform-specific kernel buffers; if the child writes faster than the parent consumes, the kernel pipe fills and the child will block on write. [1][2]
- On Linux the default pipe capacity is typically 65,536 bytes (64 KiB) (can be queried/changed with F_GETPIPE_SZ / F_SETPIPE_SZ). [2]
- In Node, spawned children’s stdout/stderr are exposed as Readable streams (with Node-level buffering controlled by stream highWaterMark, default 16 KiB) and exec()/execSync() also enforce a JS-side maxBuffer. If you don’t read the stream fast enough the kernel pipe still fills and the child will block. [1][3][4]
Practical fixes
- Always consume the child’s stdout/stderr (attach 'data' handler or pipe it) so the kernel buffer drains. [1]
- Use stdio: 'inherit' (shares parent stdio) or stdio: 'ignore' (drops output) if you don’t need the data. [1]
- For very high-throughput cases: (a) stream data incrementally (pipe to a file or process), (b) tune Node stream buffers via highWaterMark where applicable, or (c) on Linux adjust pipe size with fcntl(F_SETPIPE_SZ) from native code (not exposed directly by Node core). [1][2][3]
- If using child_process.exec(), raise maxBuffer (or switch to spawn()) to avoid JS-side buffering limits. [4]
Sources
- Node.js child_process docs (spawn/stdio behavior, exec maxBuffer). [1]
- Linux pipe(7) / man pages (pipe capacity, F_GET/SETPIPE_SZ). [2]
- Node.js stream docs (default highWaterMark = 16 KiB). [3]
- child_process.exec/execSync maxBuffer description. [4]
🏁 Script executed:
# Let's verify the exact stdio configuration and check if there are tests
grep -n "stdio" ./packages/start-plugin-core/src/prerender.ts -B 2 -A 2Repository: TanStack/router
Length of output: 200
🏁 Script executed:
# Check if there are any tests for the prerender function
find ./packages/start-plugin-core -name "*.test.ts" -o -name "*.spec.ts" | xargs grep -l "startNitroServer\|prerender" 2>/dev/null | head -5Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check tsconfig for strict mode
find ./packages/start-plugin-core -name "tsconfig.json" | xargs cat 2>/dev/null | head -50Repository: TanStack/router
Length of output: 283
🏁 Script executed:
# Check root tsconfig
cat ./tsconfig.json | grep -A 30 "compilerOptions"Repository: TanStack/router
Length of output: 818
Based on my verification of the review comment, I can now provide the rewritten review:
Address stdout buffer blocking and improve port collision handling.
The code has two legitimate concerns:
-
stdout buffer blocking: Setting
stdio: 'pipe'without consuming stdout can cause the child process to block if Nitro's startup output exceeds the kernel pipe buffer (~64 KiB on Linux). stderr is correctly consumed, but stdout is not. Usingstdio: 'inherit'fixes this by inheriting the parent's stdout. -
Port collision risk: The random port selection (3000–12999) may collide with existing services, causing a confusing "startup timed out" error instead of a clear port-in-use failure. However, the suggested fix using
port: 0requires additional infrastructure to communicate the OS-assigned port back from the Nitro process (e.g., parsing startup output or using IPC), which the diff does not address. Alternatively, consider using a library likeget-portor performing an availability check before spawning.
- const port = 3000 + Math.floor(Math.random() * 10000)
+ // Use port 0 to let OS assign an available ephemeral port,
+ // then parse the actual port from Nitro's startup output
+ const port = 0
const env = { ...process.env, PORT: String(port) }
const child: ChildProcess = spawn('node', [serverPath], {
env,
- stdio: ['ignore', 'pipe', 'pipe'],
+ stdio: ['ignore', 'inherit', 'pipe'],
})The stdout fix is straightforward and recommended. The port collision fix needs clarification on how to retrieve the actual port Nitro binds to when using port 0.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/start-plugin-core/src/prerender.ts around lines 288-299, change the
child spawn options to avoid stdout buffer blocking by either inheriting stdout
(stdio: ['ignore','inherit','pipe']) or actively consuming child.stdout stream;
and replace the naive random port selection with a reliable approach such as
using get-port to pick an available port before spawning (or, if you choose port
0, implement a mechanism to read the OS-assigned port from Nitro startup output
or via IPC) so you fail fast on port collisions and can determine the actual
bound port.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/start-plugin-core/src/prerender.ts (1)
292-350: Address stdout buffer blocking and port collision risks.The issues from the previous review remain:
stdout buffer blocking:
stdio: ['ignore', 'pipe', 'pipe']pipes stdout but it's never consumed. Only stderr is consumed (lines 343-345). If Nitro outputs significant data to stdout, the kernel pipe buffer (~64 KiB) fills and the process blocks.Port collision risk: Random port selection (3000–12999) may collide with existing services, resulting in a confusing "startup timed out" error rather than a clear port-in-use failure.
Apply this diff to fix the stdout issue (minimal change):
const child: ChildProcess = spawn('node', [serverPath], { env, - stdio: ['ignore', 'pipe', 'pipe'], + stdio: ['ignore', 'inherit', 'pipe'], })For port collision, consider using the
get-portlibrary (already in some Vite tooling) or implement a retry loop that catches EADDRINUSE.
🧹 Nitpick comments (2)
packages/start-plugin-core/src/prerender.ts (1)
352-397: Complex but necessary workaround for Nitro's signal handling.The
process.exitoverride and SIGHUP emit is a pragmatic solution to trigger Nitro's child process cleanup without exiting the parent process. A few observations:
- The 100ms delay (line 392) is somewhat arbitrary – if Nitro's cleanup is slow, this might not be enough.
- There's a brief window where
process.exitis no-op'd; if other code calls it during this time, it would be silently ignored.Consider adding a brief comment explaining why this workaround exists (Nitro's configurePreviewServer behavior) for future maintainers.
packages/start-plugin-core/src/plugin.ts (1)
378-409: Consider adding type annotations for the Nitro module interface.The
nitro: anyparameter and untyped access tonitro.options.output.dir,nitro.options.output.publicDir, andnitro.options.presetreduce type safety. Per the coding guidelines requiring TypeScript strict mode, consider defining a minimal interface for the Nitro options you depend on.+// Minimal type for Nitro module interop (Nitro v3.0.1-alpha.1+) +interface NitroModuleContext { + options: { + preset: string + output: { + dir: string + publicDir: string + } + } + hooks: { + hook: (name: 'compiled', fn: () => Promise<void>) => void + } +} + { name: 'tanstack-start-core:nitro-prerender', nitro: { name: 'tanstack-start-prerender', - setup(nitro: any) { + setup(nitro: NitroModuleContext) { nitro.hooks.hook('compiled', async () => {This provides documentation and catches API drift at compile time. Based on coding guidelines requiring TypeScript strict mode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/start-plugin-core/src/plugin.ts(3 hunks)packages/start-plugin-core/src/post-server-build.ts(3 hunks)packages/start-plugin-core/src/prerender.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/start-plugin-core/src/post-server-build.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety throughout the codebase
Files:
packages/start-plugin-core/src/plugin.tspackages/start-plugin-core/src/prerender.ts
🧬 Code graph analysis (2)
packages/start-plugin-core/src/plugin.ts (2)
packages/nitro-v2-vite-plugin/src/index.ts (1)
config(58-139)packages/start-plugin-core/src/post-server-build.ts (3)
postServerBuild(76-102)writeNitroBuildInfo(16-38)postServerBuildForNitro(104-129)
packages/start-plugin-core/src/prerender.ts (2)
packages/start-plugin-core/src/schema.ts (1)
TanStackStartOutputConfig(207-207)packages/start-plugin-core/src/constants.ts (1)
VITE_ENVIRONMENT_NAMES(1-6)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (6)
packages/start-plugin-core/src/prerender.ts (3)
13-26: LGTM – Function signature update enables flexible invocation.The updated signature properly supports both Vite builder-based and Nitro-based prerendering flows. The optional parameters with clear JSDoc for
nitroServerPathprovide good API clarity.
52-66: LGTM – Output directory resolution handles both invocation modes.The conditional logic correctly prioritizes the explicit override, falls back to builder's client environment config, and throws a clear error when neither is available.
419-445: LGTM – Clean polling implementation for server readiness.The function uses reasonable defaults (100ms interval, 30s timeout) and correctly treats any non-5xx response as "ready" since a 404 still indicates the server is accepting requests.
packages/start-plugin-core/src/plugin.ts (3)
53-53: LGTM – Config file path tracking for Nitro integration.Adding
viteConfigFiletoResolvedStartConfigprovides the necessary context for the Nitro prerender flow to invokevite.preview()with the correct configuration.Also applies to: 80-80
356-358: LGTM – Correct handling of configFile value.The
|| undefinedcoercion correctly converts Vite'sfalse(no config file) toundefined, aligning with the expected type inResolvedStartConfig.
363-374: LGTM – Nitro detection enables deferred prerendering.The plugin name prefix check (
nitro:) is a reasonable heuristic for detecting Nitro's presence, and passingskipPrerender: truecorrectly defers prerendering to the Nitro module'scompiledhook.
|
|
||
| // Wait for the server to be ready (handles Nitro's child process startup time) | ||
| await waitForServerReady(baseUrl, logger) | ||
| } |
There was a problem hiding this comment.
Misleading comment – This is the Vite preview path, not Nitro.
The comment says "handles Nitro's child process startup time" but this code is in the else branch where we're using Vite preview, not Nitro. The Nitro server path (lines 73-77) already handles readiness internally via polling in startNitroServer.
- // Wait for the server to be ready (handles Nitro's child process startup time)
+ // Wait for the Vite preview server to be ready
await waitForServerReady(baseUrl, logger)📝 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.
| // Wait for the server to be ready (handles Nitro's child process startup time) | |
| await waitForServerReady(baseUrl, logger) | |
| } | |
| // Wait for the Vite preview server to be ready | |
| await waitForServerReady(baseUrl, logger) | |
| } |
🤖 Prompt for AI Agents
In packages/start-plugin-core/src/prerender.ts around lines 88 to 91, the inline
comment incorrectly states it "handles Nitro's child process startup time" while
this branch uses Vite preview; update the comment to accurately describe that
waitForServerReady is used for the Vite preview server readiness (or remove the
misleading reference to Nitro), e.g. mention Vite preview/startup latency or
remove Nitro-specific wording so comments reflect the actual code path.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/start-plugin-core/src/prerender.ts (2)
89-90: Fix comment: this readiness wait is for Vite preview, not Nitro.This branch is only executed when
nitroServerPathis falsy and you’ve started a Vite preview server. The comment still refers to “Nitro’s child process startup time”, which is misleading and was already flagged in a previous review.Something like:
// Wait for the Vite preview server (and any framework plugins) to be readywould better reflect what this code is doing.
2-3: Nitro subprocess: stdout piping and port selection are fragile.This block is doing the right thing conceptually (spawn Nitro, poll until ready, expose a close handle), but there are a few practical issues – some already raised in a previous review:
stdout pipe can block the child process.
You setstdio: ['ignore', 'pipe', 'pipe']but never consumechild.stdout. If Nitro writes enough to stdout during startup, the OS pipe buffer can fill and block the child, making the readiness check hang.Easiest fix: inherit stdout if you don’t need to capture it:
- const child: ChildProcess = spawn('node', [serverPath], {
env,stdio: ['ignore', 'pipe', 'pipe'],- })
- const child: ChildProcess = spawn('node', [serverPath], {
env,// Inherit stdout so the child can't block on a full pipe bufferstdio: ['ignore', 'inherit', 'pipe'],- })
2. **Random port selection risks collisions and false “readiness”.** Picking `3000 + Math.random() * 10000` can collide with existing services: - If another HTTP service is already on that port, your polling `fetch("http://localhost:port/")` may see a valid response and treat an unrelated server as “Nitro is ready”. - If Nitro fails with `EADDRINUSE`, you’ll time out after 30 seconds without a clear port‑in‑use signal. Consider a more robust approach, e.g.: - Use a port‑discovery helper (like `get-port`) to pick an available port before spawning Nitro, or - Allow `PORT=0` and have Nitro (or a small wrapper) communicate the actual bound port back (e.g. via stdout structured log or IPC) so you don’t have to guess or risk collisions. 3. **Optional: react to early child exit.** Right now, if the Nitro process exits quickly (e.g. due to a config error), you keep polling until the 30s timeout. Adding a `child.on('exit', ...)` that rejects immediately when `!resolved` would surface those failures faster. Points (1) and (2) are the most important to address; (3) is a nice quality‑of‑life improvement. Also applies to: 9-9, 292-350 </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (3)</summary><blockquote> <details> <summary>packages/start-plugin-core/src/prerender.ts (3)</summary><blockquote> `16-26`: **Server setup vs try/finally – consider pulling startup into the guarded section.** Right now Nitro/Vite server startup (and `waitForServerReady` in the preview path) happens before the `try` block, while `cleanup()` is only called from the `finally`. That means if anything throws between assigning `baseUrl/cleanup` and entering the `try` (notably `waitForServerReady` timing out), the preview server will stay running until the outer caller tears down the process. This isn’t catastrophic for a short‑lived CLI, but for long‑lived processes/tests it’s safer to: - Move the `if (nitroServerPath) { ... } else { ...; await waitForServerReady(...) }` block inside the `try`, or - Add an outer `try/finally` that ensures `cleanup` is awaited whenever the server has been started. This keeps server lifecycle management clearly tied to the guarded region where prerendering actually happens. Also applies to: 52-66, 70-72, 119-132 --- `10-10`: **Make `process.exit` override robust when cleaning up Nitro’s preview child.** The Nitro‑aware `startPreviewServer` logic is clever and solves a real problem (Nitro’s `configurePreviewServer` killing the parent process on `SIGHUP`). A couple of robustness tweaks would make it safer: - `server.close` currently does: ```ts const originalExit = process.exit process.exit = (() => {}) as typeof process.exit process.emit('SIGHUP', 'SIGHUP') process.exit = originalExit await new Promise((resolve) => setTimeout(resolve, 100)) return originalClose()If any handler attached to
SIGHUPthrows synchronously,process.exitwill never be restored. Wrapping the override in atry/finallykeeps the global state consistent:- server.close = async () => { - const originalExit = process.exit - process.exit = (() => {}) as typeof process.exit - - // Emit SIGHUP to trigger Nitro's child process cleanup - process.emit('SIGHUP', 'SIGHUP') - - // Restore process.exit - process.exit = originalExit - - // Give the child process a moment to terminate - await new Promise((resolve) => setTimeout(resolve, 100)) - - // Now close the preview server - return originalClose() - } + server.close = async () => { + const originalExit = process.exit + try { + process.exit = (() => {}) as typeof process.exit + + // Emit SIGHUP to trigger Nitro's child process cleanup + process.emit('SIGHUP', 'SIGHUP') + } finally { + // Always restore process.exit, even if a handler throws + process.exit = originalExit + } + + // Give the child process a moment to terminate + await new Promise((resolve) => setTimeout(resolve, 100)) + + // Now close the preview server + return originalClose() + }
- The
hasNitroPlugindetection byp.name.startsWith('nitro:')is reasonable, but if Nitro ever changes its plugin names this branch will silently stop running. Non‑blocking, but you may want a short comment noting that assumption.Overall, the preview/Nitro interop strategy looks solid; this is mainly about hardening the global process mutation.
Also applies to: 352-403
423-447:waitForServerReadylooks good; consider reusing it for Nitro as well.The readiness helper is straightforward and matches the semantics you use elsewhere (server “ready” once it responds with any status
< 500). Implementation is clear and bounded by a timeout.Given that
startNitroServerhas its own polling logic with similar behavior, you might consider reusingwaitForServerReadythere (accepting aURLinstead of re‑encodinghttp://localhost:${port}/) to keep the readiness semantics in one place and reduce duplication. Not required, but would simplify future changes (e.g. tweaking timeouts or readiness criteria) to a single helper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/start-plugin-core/src/prerender.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety throughout the codebase
Files:
packages/start-plugin-core/src/prerender.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: Test
- GitHub Check: Preview
| async function localFetch( | ||
| path: string, | ||
| fetchPath: string, | ||
| options?: RequestInit, | ||
| maxRedirects: number = 5, | ||
| ): Promise<Response> { | ||
| const url = new URL(path, baseUrl) | ||
| const url = new URL(fetchPath, baseUrl) | ||
| const request = new Request(url, options) |
There was a problem hiding this comment.
Redirect handling for http://localhost:<port>/… is broken.
In localFetch, when following redirects you do:
if (location.startsWith('http://localhost') || location.startsWith('/')) {
const newUrl = location.replace('http://localhost', '')
return localFetch(newUrl, options, maxRedirects - 1)
}For a URL like http://localhost:3000/foo, replace('http://localhost', '') yields ':3000/foo', which is not a valid path. The next new URL(fetchPath, baseUrl) call then builds a wrong URL.
Suggested fix:
- Treat absolute
http://localhost[:port]redirects by parsing and reusing only the path/query/hash, and keep the host/port frombaseUrl. - Keep the simple code path for already-relative URLs.
For example:
- if (isRedirectResponse(response) && maxRedirects > 0) {
- const location = response.headers.get('location')!
- if (location.startsWith('http://localhost') || location.startsWith('/')) {
- const newUrl = location.replace('http://localhost', '')
- return localFetch(newUrl, options, maxRedirects - 1)
- } else {
- logger.warn(`Skipping redirect to external location: ${location}`)
- }
- }
+ if (isRedirectResponse(response) && maxRedirects > 0) {
+ const location = response.headers.get('location')!
+
+ if (location.startsWith('http://localhost')) {
+ const url = new URL(location)
+ const newPath = `${url.pathname}${url.search}${url.hash}`
+ return localFetch(newPath, options, maxRedirects - 1)
+ }
+
+ if (location.startsWith('/')) {
+ return localFetch(location, options, maxRedirects - 1)
+ }
+
+ logger.warn(`Skipping redirect to external location: ${location}`)
+ }The new headers: prerenderOptions.headers usage looks good and preserves the ability to customize per‑request headers.
Also applies to: 106-113, 195-201
|
Hey! @birkskyum |
|
@theoludwig , we need the nitro 3 to implement a fetch method |
|
@birkskyum thanks for your work on this. Is there an existing issue we can use to track the status of implementing the fetch method in Nitro 3? |
birkskyum
force-pushed
the
nitro-with-vite-preview
branch
from
b17e4b6 to
e6f7f47
Compare
|
@theoludwig @xentobias this feature is tracked in the Nitro repo as the "Support prerendering" check box in this ticket: |
birkskyum
force-pushed
the
nitro-with-vite-preview
branch
from
00b4d6f to
9bf435d
Compare
birkskyum
force-pushed
the
nitro-with-vite-preview
branch
from
54420d4 to
7b76b61
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
e2e/react-start/basic-nitro-spa/src/routes/index.tsx (1)
9-14: Server function implementation looks good.The
createServerFn().handlerpattern correctly returns data for the loader. For stricter type safety, you could optionally add an explicit return type annotation, but type inference works fine here.🔎 Optional: Add explicit return type for stricter typing
-const getData = createServerFn().handler(() => { +const getData = createServerFn().handler((): { message: string; timestamp: string } => { return { message: 'Hello from Nitro server!', timestamp: new Date().toISOString(), } })e2e/react-start/basic-nitro-spa/src/components/DefaultCatchBoundary.tsx (1)
27-27: Consider extracting the repeated button className.The same className string appears three times (lines 27, 34, 41). Extracting it to a constant would improve maintainability and reduce duplication.
🔎 Suggested refactor
+const buttonClassName = 'px-2 py-1 bg-gray-600 dark:bg-gray-700 rounded-sm text-white uppercase font-extrabold' + export function DefaultCatchBoundary({ error }: ErrorComponentProps) { const router = useRouter() const isRoot = useMatch({ strict: false, select: (state) => state.id === rootRouteId, }) console.error(error) return ( <div className="min-w-0 flex-1 p-4 flex flex-col items-center justify-center gap-6"> <ErrorComponent error={error} /> <div className="flex gap-2 items-center flex-wrap"> <button onClick={() => { router.invalidate() }} - className={`px-2 py-1 bg-gray-600 dark:bg-gray-700 rounded-sm text-white uppercase font-extrabold`} + className={buttonClassName} > Try Again </button> {isRoot ? ( <Link to="/" - className={`px-2 py-1 bg-gray-600 dark:bg-gray-700 rounded-sm text-white uppercase font-extrabold`} + className={buttonClassName} > Home </Link> ) : ( <Link to="/" - className={`px-2 py-1 bg-gray-600 dark:bg-gray-700 rounded-sm text-white uppercase font-extrabold`} + className={buttonClassName} onClick={(e) => { e.preventDefault() window.history.back() }} > Go Back </Link> )} </div> </div> ) }Also applies to: 34-34, 41-41
e2e/react-start/basic-nitro/playwright.config.ts (1)
17-44: Well-structured Playwright configuration.The configuration follows best practices:
- Good explanatory comment about using Node directly instead of vite preview
- Correct
reuseExistingServerpattern for CI environments (based on learnings)- Proper baseURL and port setup
- Appropriate stdout piping for visibility
Consider adding a brief comment explaining why
workers: 1is used (e.g., to avoid port conflicts or shared state issues in e2e tests):🔎 Optional: Add clarifying comment for workers setting
export default defineConfig({ testDir: './tests', + // Single worker to avoid port conflicts during e2e testing workers: 1,e2e/react-start/basic-nitro-spa/vite.config.v2.ts (1)
6-21: Configuration structure is correct; consider adding documentation.The plugin configuration correctly mirrors the v3 setup while using
nitroV2Plugin(). The structure and ordering are appropriate for SPA prerendering with Nitro v2 integration.📝 Consider adding a file header comment to document the v2/v3 split
Adding a brief comment at the top of both configuration files would improve maintainability by explaining why parallel v2 and v3 configs exist:
+/** + * Vite configuration for Nitro v2 e2e tests. + * See vite.config.v3.ts for the Nitro v3 equivalent. + */ import { defineConfig } from 'vite'This helps future maintainers understand the intentional duplication.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
e2e/react-start/basic-nitro-spa/public/android-chrome-192x192.pngis excluded by!**/*.pnge2e/react-start/basic-nitro-spa/public/android-chrome-512x512.pngis excluded by!**/*.pnge2e/react-start/basic-nitro-spa/public/apple-touch-icon.pngis excluded by!**/*.pnge2e/react-start/basic-nitro-spa/public/favicon-16x16.pngis excluded by!**/*.pnge2e/react-start/basic-nitro-spa/public/favicon-32x32.pngis excluded by!**/*.pnge2e/react-start/basic-nitro-spa/public/favicon.icois excluded by!**/*.icoe2e/react-start/basic-nitro-spa/public/favicon.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/android-chrome-192x192.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/android-chrome-512x512.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/apple-touch-icon.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/favicon-16x16.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/favicon-32x32.pngis excluded by!**/*.pnge2e/react-start/basic-nitro/public/favicon.icois excluded by!**/*.icoe2e/react-start/basic-nitro/public/favicon.pngis excluded by!**/*.png
📒 Files selected for processing (30)
e2e/react-start/basic-nitro-spa/.gitignoree2e/react-start/basic-nitro-spa/package.jsone2e/react-start/basic-nitro-spa/playwright.config.tse2e/react-start/basic-nitro-spa/postcss.config.mjse2e/react-start/basic-nitro-spa/public/site.webmanifeste2e/react-start/basic-nitro-spa/src/components/DefaultCatchBoundary.tsxe2e/react-start/basic-nitro-spa/src/components/NotFound.tsxe2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/router.tsxe2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/src/routes/index.tsxe2e/react-start/basic-nitro-spa/src/routes/static.tsxe2e/react-start/basic-nitro-spa/src/styles/app.csse2e/react-start/basic-nitro-spa/src/utils/seo.tse2e/react-start/basic-nitro-spa/tests/app.spec.tse2e/react-start/basic-nitro-spa/tsconfig.jsone2e/react-start/basic-nitro-spa/vite.config.v2.tse2e/react-start/basic-nitro-spa/vite.config.v3.tse2e/react-start/basic-nitro/.gitignoree2e/react-start/basic-nitro/.prettierignoree2e/react-start/basic-nitro/package.jsone2e/react-start/basic-nitro/playwright.config.tse2e/react-start/basic-nitro/postcss.config.mjse2e/react-start/basic-nitro/public/site.webmanifeste2e/react-start/basic-nitro/src/components/DefaultCatchBoundary.tsxe2e/react-start/basic-nitro/src/components/NotFound.tsxe2e/react-start/basic-nitro/src/routeTree.gen.tse2e/react-start/basic-nitro/src/router.tsxe2e/react-start/basic-nitro/src/routes/__root.tsxe2e/react-start/basic-nitro/src/routes/index.tsx
✅ Files skipped from review due to trivial changes (2)
- e2e/react-start/basic-nitro/.prettierignore
- e2e/react-start/basic-nitro-spa/.gitignore
🚧 Files skipped from review as they are similar to previous changes (18)
- e2e/react-start/basic-nitro-spa/src/router.tsx
- e2e/react-start/basic-nitro-spa/tsconfig.json
- e2e/react-start/basic-nitro/src/router.tsx
- e2e/react-start/basic-nitro-spa/playwright.config.ts
- e2e/react-start/basic-nitro-spa/package.json
- e2e/react-start/basic-nitro-spa/src/routes/static.tsx
- e2e/react-start/basic-nitro/public/site.webmanifest
- e2e/react-start/basic-nitro/src/components/DefaultCatchBoundary.tsx
- e2e/react-start/basic-nitro/src/routes/index.tsx
- e2e/react-start/basic-nitro-spa/tests/app.spec.ts
- e2e/react-start/basic-nitro-spa/src/components/NotFound.tsx
- e2e/react-start/basic-nitro/package.json
- e2e/react-start/basic-nitro/src/routeTree.gen.ts
- e2e/react-start/basic-nitro-spa/src/styles/app.css
- e2e/react-start/basic-nitro-spa/src/utils/seo.ts
- e2e/react-start/basic-nitro-spa/public/site.webmanifest
- e2e/react-start/basic-nitro/src/routes/__root.tsx
- e2e/react-start/basic-nitro/.gitignore
🧰 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:
e2e/react-start/basic-nitro/src/components/NotFound.tsxe2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/vite.config.v3.tse2e/react-start/basic-nitro-spa/vite.config.v2.tse2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/index.tsxe2e/react-start/basic-nitro/playwright.config.tse2e/react-start/basic-nitro-spa/src/components/DefaultCatchBoundary.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/react-start/basic-nitro/src/components/NotFound.tsxe2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/vite.config.v3.tse2e/react-start/basic-nitro-spa/vite.config.v2.tse2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/index.tsxe2e/react-start/basic-nitro/playwright.config.tse2e/react-start/basic-nitro-spa/src/components/DefaultCatchBoundary.tsx
🧠 Learnings (10)
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/src/routeTree.gen.ts
📚 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:
e2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/index.tsx
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
e2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/src/routeTree.gen.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
e2e/react-start/basic-nitro-spa/src/routes/__root.tsxe2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/index.tsx
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
e2e/react-start/basic-nitro-spa/src/routes/__root.tsx
📚 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:
e2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/index.tsx
📚 Learning: 2025-12-21T12:52:35.231Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:35.231Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
e2e/react-start/basic-nitro-spa/src/routeTree.gen.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Use file-based routing in `src/routes/` directories or code-based routing with route definitions
Applied to files:
e2e/react-start/basic-nitro-spa/src/routeTree.gen.tse2e/react-start/basic-nitro-spa/src/routes/index.tsx
📚 Learning: 2025-12-25T13:04:55.492Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 6215
File: e2e/react-start/custom-basepath/package.json:13-17
Timestamp: 2025-12-25T13:04:55.492Z
Learning: In the TanStack Router repository, e2e test scripts are specifically designed to run in CI (which uses a Unix environment), so Unix-specific commands (like `rm -rf`, `&` for backgrounding, and direct environment variable assignments without `cross-env`) are acceptable in e2e test npm scripts.
Applied to files:
e2e/react-start/basic-nitro/playwright.config.ts
📚 Learning: 2025-10-09T12:59:14.842Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/public/site.webmanifest:2-3
Timestamp: 2025-10-09T12:59:14.842Z
Learning: In e2e test fixtures (files under e2e directories), empty or placeholder values in configuration files like site.webmanifest are acceptable and should not be flagged unless the test specifically validates those fields.
Applied to files:
e2e/react-start/basic-nitro/playwright.config.ts
🧬 Code graph analysis (1)
e2e/react-start/basic-nitro-spa/vite.config.v2.ts (1)
packages/nitro-v2-vite-plugin/src/index.ts (1)
nitroV2Plugin(19-142)
🔇 Additional comments (13)
e2e/react-start/basic-nitro-spa/src/routeTree.gen.ts (1)
1-86: Skipping review of autogenerated file.This file is autogenerated by TanStack Router and should not be manually modified or reviewed. Based on learnings, files named
routeTree.gen.tsare generated artifacts that will be overwritten by the router generator.e2e/react-start/basic-nitro/postcss.config.mjs (1)
1-5: LGTM! Standard PostCSS configuration for Tailwind.This configuration follows the modern approach for integrating Tailwind CSS with PostCSS. The empty options object appropriately uses Tailwind's default configuration.
e2e/react-start/basic-nitro-spa/postcss.config.mjs (1)
1-5: LGTM! Consistent PostCSS configuration.This configuration is identical to the one in
e2e/react-start/basic-nitro/postcss.config.mjs, which provides consistency across the Nitro e2e test variants. The setup correctly enables Tailwind CSS processing for the SPA variant.e2e/react-start/basic-nitro-spa/src/routes/index.tsx (2)
1-7: LGTM! Clean route setup following TanStack Router patterns.The file-based route definition correctly uses
createFileRoute('/')with a loader that fetches server data and a component to render it.
16-26: LGTM! Well-structured component with proper e2e test IDs.The Home component correctly consumes loader data via
Route.useLoaderData()and includes appropriatedata-testidattributes for end-to-end testing.e2e/react-start/basic-nitro-spa/src/routes/__root.tsx (3)
1-13: LGTM! All necessary imports are present and correctly structured.The Vite client types reference and TanStack Router primitives are properly imported. The tilde alias for local imports is consistent with project conventions.
15-38: LGTM! Root route configuration is complete and well-structured.The root route properly configures:
- Document head with SEO meta tags and stylesheet links
- Error and not-found boundaries
- Custom shell component for HTML structure
All settings follow TanStack Router best practices.
40-73: LGTM! RootDocument shell component is properly implemented.The HTML shell correctly:
- Injects head content and scripts via TanStack Router primitives
- Implements navigation with active link styling
- Uses
exact: trueon the Home link to prevent it from matching all paths- Positions devtools for development convenience
- Properly types children as
React.ReactNodeThe structure is compatible with React 19.2 and follows TanStack Router conventions.
e2e/react-start/basic-nitro/playwright.config.ts (2)
3-3: No action needed. Import attributes syntax (with { type: 'json' }) is already widely used throughout the codebase (in multiple playwright.config.ts and test files), and the project's TypeScript configuration (5.9.0 withmodule: "ESNext",target: "ES2022", andresolveJsonModule: true) fully supports this syntax. This code follows an established project pattern and presents no compatibility concerns.Likely an incorrect or invalid review comment.
7-12: NITRO_VARIANT setup and build scripts are correctly configured.Verification confirms:
NITRO_VARIANTis properly set viatest:e2e:v2andtest:e2e:v3npm scripts in package.json- Both
build:v2andbuild:v3scripts exist and are implemented- TypeScript strict mode is enabled in tsconfig.json
The validation pattern ensures tests fail fast with a clear error if the environment variable is misconfigured.
e2e/react-start/basic-nitro-spa/vite.config.v3.ts (2)
1-4: Imports look clean.The import structure is straightforward and follows standard Vite configuration patterns.
6-21: Code configuration is correct for SPA prerendering with Nitro v3.The vite configuration properly sets up Nitro for SPA prerendering. Nitro v3 is available (v3.0.1-alpha.1+) with fetch method support already implemented, so no version or API compatibility concerns remain.
e2e/react-start/basic-nitro-spa/vite.config.v2.ts (1)
1-4: Correct imports for Nitro v2 integration.The v2 configuration properly uses the TanStack-provided
nitroV2Pluginwrapper instead of the native Nitro plugin. This is appropriate for maintaining backward compatibility during the transition to Nitro v3.
| <Link | ||
| to="/" | ||
| className={`px-2 py-1 bg-gray-600 dark:bg-gray-700 rounded-sm text-white uppercase font-extrabold`} | ||
| onClick={(e) => { | ||
| e.preventDefault() | ||
| window.history.back() | ||
| }} | ||
| > | ||
| Go Back | ||
| </Link> |
There was a problem hiding this comment.
Use a button element instead of Link for the "Go Back" action.
The Link component with to="/" generates an href="/" attribute, but the onClick handler prevents default navigation and uses window.history.back() instead. This creates several issues:
- Accessibility: Screen readers will announce this as a link to the home page ("/"), but it actually navigates backward in history.
- User experience: Right-click "Open in new tab", middle-click, or keyboard navigation could navigate to "/" instead of going back.
- Semantics: A non-navigation action (going back in history) should use a
buttonelement, not aLink.
🔎 Recommended fix
- <Link
- to="/"
+ <button
+ onClick={() => {
+ window.history.back()
+ }}
className={`px-2 py-1 bg-gray-600 dark:bg-gray-700 rounded-sm text-white uppercase font-extrabold`}
- onClick={(e) => {
- e.preventDefault()
- window.history.back()
- }}
>
Go Back
- </Link>
+ </button>🤖 Prompt for AI Agents
In e2e/react-start/basic-nitro-spa/src/components/DefaultCatchBoundary.tsx
around lines 39–48, replace the Link element used for the "Go Back" action with
a semantic button: remove the to="/" href, render a <button type="button"> that
keeps the same className/styling, attach the onClick handler to call
window.history.back(), and include an accessible label (e.g. aria-label="Go
back") if the visible text is not sufficient; ensure you no longer call
e.preventDefault() since there is no link navigation to prevent.
| @@ -0,0 +1,25 @@ | |||
| import { Link } from '@tanstack/react-router' | |||
|
|
|||
| export function NotFound({ children }: { children?: any }) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace any type with React.ReactNode for strict type safety.
The children prop is typed as any, which violates the coding guideline to use TypeScript strict mode with extensive type safety. Use React.ReactNode instead.
🔎 Proposed fix
-export function NotFound({ children }: { children?: any }) {
+export function NotFound({ children }: { children?: React.ReactNode }) {As per coding guidelines, TypeScript files must use strict mode with extensive type safety.
📝 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 function NotFound({ children }: { children?: any }) { | |
| export function NotFound({ children }: { children?: React.ReactNode }) { |
🤖 Prompt for AI Agents
In e2e/react-start/basic-nitro/src/components/NotFound.tsx around line 3, the
children prop is typed as any; replace that with React.ReactNode to satisfy
strict TypeScript typing. Import React if not already imported (or use the React
namespace available in JSX settings), update the function signature to use
children?: React.ReactNode, and ensure no other uses rely on any; run typecheck
to confirm no further errors.
Nitro Prerendering (both v2 and v3)
Likely resolves
Also, adds e2e tests for Nitro integration (each testing both nitro v2 and v3):
Summary by CodeRabbit
Release Notes
New Features
Infrastructure
✏️ Tip: You can customize this high-level summary in your review settings.