-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat(start-plugin-core): vite preview support #5910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a preview test mode across React and Solid e2e suites with new scripts and preview-aware Playwright ports; introduces an isPreview test utility; broadens Vite config types in output-directory utilities; and adds a new preview-server Vite plugin that lazily loads and proxies the server build during preview. Changes
Sequence DiagramsequenceDiagram
participant Client
participant VitePreview as Vite Preview Server
participant PreviewPlugin as Preview Server Plugin
participant ServerBuild as Server Build (dynamic)
Client->>VitePreview: HTTP Request
VitePreview->>PreviewPlugin: Invoke middleware (post-order)
alt Cache miss (first request)
PreviewPlugin->>PreviewPlugin: Resolve server entry path from output dir
PreviewPlugin->>ServerBuild: Dynamic import(server entry)
ServerBuild-->>PreviewPlugin: Return module (cached)
end
PreviewPlugin->>ServerBuild: Call fetch(NodeRequest)
ServerBuild-->>PreviewPlugin: Return WebResponse
PreviewPlugin->>VitePreview: Write headers & status, send response
VitePreview-->>Client: HTTP Response
note right of PreviewPlugin: On error -> call next() to forward
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
🔇 Additional comments (3)
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 5b7b4bf
☁️ Nx Cloud last updated this comment at |
| // Temporary workaround | ||
| // Vite preview's compression middleware doesn't support flattened array headers that srvx sets | ||
| // Call writeHead() before srvx to avoid corruption | ||
| res.setHeaders(webRes.headers) | ||
| res.writeHead(webRes.status, webRes.statusText) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context:
Vite preview server uses @polka/compression to handle compression. This middleware overwrites the res.writeHead method, which expects headers to be an object and uses a for...in loop to iterate over them.
However, srvx passes headers as a flattened array to writeHead(). When the middleware's for...in loop runs through an array, it iterates over array indices ('0', '1', '2', ...) instead of the actual header names.
This causes header corruption where a header like:
Content-Type: text/html; charset=utf-8
Becomes:
0: Content-Type
1: text/html; charset=utf-8
The workaround is to call res.writeHead() directly before sendNodeResponse(), which sets res.headersSent = true and causes sendNodeResponse() to skip header writing entirely.
I will try to submit a patch to either Vite or @polka/compression, but we will likely want to keep this workaround for compatibility with earlier versions of Vite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
e2e/react-start/basic/package.json(1 hunks)e2e/react-start/basic/playwright.config.ts(2 hunks)e2e/react-start/basic/tests/redirect.spec.ts(1 hunks)e2e/react-start/basic/tests/utils/isPreview.ts(1 hunks)e2e/solid-start/basic/package.json(1 hunks)e2e/solid-start/basic/playwright.config.ts(2 hunks)e2e/solid-start/basic/tests/redirect.spec.ts(1 hunks)e2e/solid-start/basic/tests/utils/isPreview.ts(1 hunks)packages/start-plugin-core/src/output-directory.ts(1 hunks)packages/start-plugin-core/src/plugin.ts(2 hunks)packages/start-plugin-core/src/preview-server-plugin/plugin.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
packages/start-plugin-core/src/plugin.tse2e/solid-start/basic/tests/redirect.spec.tse2e/react-start/basic/tests/redirect.spec.tse2e/solid-start/basic/playwright.config.tse2e/react-start/basic/playwright.config.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
e2e/solid-start/basic/tests/redirect.spec.tse2e/react-start/basic/tests/redirect.spec.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/solid-start/basic/tests/redirect.spec.tse2e/react-start/basic/tests/redirect.spec.tse2e/solid-start/basic/playwright.config.tse2e/react-start/basic/playwright.config.ts
🧬 Code graph analysis (9)
e2e/solid-start/basic/tests/utils/isPreview.ts (1)
e2e/react-start/basic/tests/utils/isPreview.ts (1)
isPreview(1-1)
packages/start-plugin-core/src/output-directory.ts (1)
packages/start-plugin-core/src/constants.ts (1)
VITE_ENVIRONMENT_NAMES(1-6)
packages/start-plugin-core/src/plugin.ts (1)
packages/start-plugin-core/src/preview-server-plugin/plugin.ts (1)
previewServerPlugin(8-64)
e2e/solid-start/basic/tests/redirect.spec.ts (2)
e2e/e2e-utils/src/derivePort.ts (1)
getTestServerPort(25-27)e2e/solid-start/basic/tests/utils/isPreview.ts (1)
isPreview(1-1)
e2e/react-start/basic/tests/redirect.spec.ts (3)
e2e/e2e-utils/src/derivePort.ts (1)
getTestServerPort(25-27)e2e/react-start/basic/tests/utils/isSpaMode.ts (1)
isSpaMode(1-1)e2e/react-start/basic/tests/utils/isPreview.ts (1)
isPreview(1-1)
e2e/react-start/basic/tests/utils/isPreview.ts (1)
e2e/solid-start/basic/tests/utils/isPreview.ts (1)
isPreview(1-1)
packages/start-plugin-core/src/preview-server-plugin/plugin.ts (2)
packages/start-plugin-core/src/constants.ts (1)
VITE_ENVIRONMENT_NAMES(1-6)packages/start-plugin-core/src/output-directory.ts (1)
getServerOutputDirectory(12-16)
e2e/solid-start/basic/playwright.config.ts (5)
e2e/react-start/server-functions/playwright.config.ts (1)
PORT(5-5)e2e/e2e-utils/src/derivePort.ts (1)
getTestServerPort(25-27)e2e/solid-start/basic/tests/utils/isSpaMode.ts (1)
isSpaMode(1-1)e2e/solid-start/basic/tests/utils/isPreview.ts (1)
isPreview(1-1)e2e/solid-start/basic/tests/utils/isPrerender.ts (1)
isPrerender(1-1)
e2e/react-start/basic/playwright.config.ts (4)
e2e/e2e-utils/src/derivePort.ts (1)
getTestServerPort(25-27)e2e/react-start/basic/tests/utils/isSpaMode.ts (1)
isSpaMode(1-1)e2e/react-start/basic/tests/utils/isPreview.ts (1)
isPreview(1-1)e2e/react-start/basic/tests/utils/isPrerender.ts (1)
isPrerender(1-1)
🔇 Additional comments (10)
e2e/react-start/basic/tests/redirect.spec.ts (1)
10-18: Preview port key wiring looks consistentUsing
isPreviewto append the_previewsuffix to thegetTestServerPortkey mirrors the Playwright config and cleanly separates preview ports from SSR/SPA runs. Looks good.e2e/solid-start/basic/package.json (1)
12-21: Preview scripts align with new test modeThe
previewandtest:e2e:previewscripts, plus the updatedtest:e2echain, match the MODE-based setup and existing patterns for spa/prerender/SSR. No changes needed.e2e/react-start/basic/tests/utils/isPreview.ts (1)
1-1: MODE-based isPreview flag is straightforward
isPreviewmirrors the existingisSpaModepattern and cleanly expresses preview mode fromMODE. Looks good as-is.e2e/solid-start/basic/tests/utils/isPreview.ts (1)
1-1: Solid isPreview helper matches React counterpartSame
MODE === 'preview'check as in the React tests, keeping the mode handling consistent between suites.e2e/solid-start/basic/tests/redirect.spec.ts (1)
11-18: Preview-aware port derivation matches Solid configIncluding
isPreviewin thegetTestServerPortkey ensures the redirect tests hit the same preview-specific port that the Playwright config uses. Looks correct.e2e/solid-start/basic/playwright.config.ts (1)
8-33: Preview mode integration into Solid Playwright config looks soundThe
isPreviewflag,_preview-suffixed port key, andpreviewModeCommandbranch ingetCommand()fit the existing spa/prerender/SSR pattern and stay in sync with the test expectations. Logging the preview state should also help debug CI runs.e2e/react-start/basic/playwright.config.ts (1)
8-33: React Playwright preview support is consistent and minimalThe
isPreviewimport,_previewport key, andpreviewModeCommandhook neatly into the existing mode switch without affecting spa/prerender/SSR behavior. This looks correct and aligned with the tests.e2e/react-start/basic/package.json (1)
12-21: React preview scripts mirror Solid and fit the new flowThe new
previewandtest:e2e:previewscripts, plus the extendedtest:e2echain, line up with the React Playwright config and isPreview helper. All good.packages/start-plugin-core/src/plugin.ts (1)
14-14: Preview plugin wiring looks solid.Importing and registering
previewServerPlugin()ahead of the bundle capture keeps the lifecycle intact for preview runs. 👍Also applies to: 403-403
packages/start-plugin-core/src/output-directory.ts (1)
6-20: AcceptingResolvedConfighere is helpful.The widened signatures let runtime hooks share the same helper without duplicating logic—looks good.
| res.setHeaders(webRes.headers) | ||
| res.writeHead(webRes.status, webRes.statusText) | ||
|
|
||
| return sendNodeResponse(res, webRes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard res.setHeaders for older Node runtimes.
During vite preview, many projects still run on Node 18.14 or earlier. On those versions ServerResponse#setHeaders is undefined, so these lines throw and the preview server never responds. Please gate this by checking typeof res.setHeaders === 'function' and fall back to iterating webRes.headers with res.setHeader(...) when it isn’t available.
🤖 Prompt for AI Agents
In packages/start-plugin-core/src/preview-server-plugin/plugin.ts around lines
52 to 55, res.setHeaders may be undefined on older Node versions (e.g., Node
18.14) which causes a crash; guard that call by checking if typeof
res.setHeaders === 'function' and call it when available, otherwise iterate over
webRes.headers and call res.setHeader(headerName, headerValue) for each entry
(normalize headerValue to a string or join arrays with ',') before calling
res.writeHead and returning sendNodeResponse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
We should probably update all the e2e sandboxes to use the preview command, but that might make more sense to ship once this PR lands in, just to avoid huge-PR-syndrome.
|
Tested this out, and it seems to work as expected |
birkskyum
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried it out a bit, and it sure does appear to work. Great to get support for this!
|
Thanks for the review @birkskyum @SeanCassiere! |
This adds
vite previewsupport with a new preview server plugin that loads the built server bundle and serves SSR requests. Both thereact-startandsolid-startbasic e2e projects have been updated to re-run their test suites in preview mode.Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.