feat(start-plugin-core): support Rsbuild preview SSR middleware#7372
Open
elecmonkey wants to merge 8 commits into
Open
feat(start-plugin-core): support Rsbuild preview SSR middleware#7372elecmonkey wants to merge 8 commits into
elecmonkey wants to merge 8 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors dev SSR middleware into a unified server-middleware usable in dev and preview, updates rsbuild schema/options, adds rsbuild preview E2E tooling, wires React dedupe overrides for rsbuild, and removes the old dev-server implementation. ChangesRsbuild Preview Mode Support
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as ServerMiddleware
participant DevEnv as RsbuildDevEnv
participant BuildOut as BuildOutput
participant Handler as FetchHandler
Client->>Middleware: HTTP request
alt preview mode
Middleware->>BuildOut: import preview bundle (index.js)
BuildOut-->>Middleware: FetchHandler
else dev mode
Middleware->>DevEnv: loadBundle('index')
DevEnv-->>Middleware: FetchHandler
end
Middleware->>Handler: invoke fetch(Request)
Handler-->>Middleware: Response
Middleware-->>Client: send response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/start-plugin-core/src/rsbuild/schema.ts`:
- Around line 11-13: The rsbuild schema currently only accepts
installServerMiddleware which breaks configs using the old
installDevServerMiddleware key; update the rsbuild zod object to accept both
installServerMiddleware and installDevServerMiddleware as optional booleans,
then normalize to a single canonical property (prefer installServerMiddleware
when both present) by adding a transform/preprocess so downstream code uses only
installServerMiddleware; mark installDevServerMiddleware as deprecated in the
schema/comments/types so it remains supported for one release.
In `@packages/start-plugin-core/src/rsbuild/server-middleware.ts`:
- Around line 185-193: The HTML response in the error path of
server-middleware.ts currently injects the raw exception message (e.message)
into the <pre>, causing reflected XSS; update the error rendering inside the new
Response to never output unescaped user-controlled text by either replacing the
displayed message with a generic string (e.g., "Internal server error") or by
HTML-escaping the value first (add a small helper like escapeHtml and call it
where e or e.message is interpolated). Ensure you reference the same Response
construction site and the exception variable e so the template uses the
escaped/generic text instead of e.message directly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42518d46-d3b2-436c-8ce5-3d554a2f3e68
📒 Files selected for processing (8)
e2e/react-start/basic/package.jsone2e/react-start/basic/playwright.config.tse2e/react-start/custom-server-rsbuild/rsbuild.config.tspackages/react-start/src/plugin/rsbuild.tspackages/start-plugin-core/src/rsbuild/dev-server.tspackages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/src/rsbuild/schema.tspackages/start-plugin-core/src/rsbuild/server-middleware.ts
💤 Files with no reviewable changes (1)
- packages/start-plugin-core/src/rsbuild/dev-server.ts
Comment on lines
11
to
13
| rsbuild: z | ||
| .object({ installDevServerMiddleware: z.boolean().optional() }) | ||
| .object({ installServerMiddleware: z.boolean().optional() }) | ||
| .optional(), |
Contributor
There was a problem hiding this comment.
Keep the old option as a deprecated alias for one release.
This rename is currently a breaking change. Existing configs that still set rsbuild.installDevServerMiddleware will silently re-enable the middleware after upgrade, because only installServerMiddleware is recognized now. Please accept both keys for now and prefer the new one when both are present.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/start-plugin-core/src/rsbuild/schema.ts` around lines 11 - 13, The
rsbuild schema currently only accepts installServerMiddleware which breaks
configs using the old installDevServerMiddleware key; update the rsbuild zod
object to accept both installServerMiddleware and installDevServerMiddleware as
optional booleans, then normalize to a single canonical property (prefer
installServerMiddleware when both present) by adding a transform/preprocess so
downstream code uses only installServerMiddleware; mark
installDevServerMiddleware as deprecated in the schema/comments/types so it
remains supported for one release.
# Conflicts: # packages/start-plugin-core/src/rsbuild/schema.ts
Contributor
|
View your CI Pipeline Execution ↗ for commit 0b3d961
☁️ Nx Cloud last updated this comment at |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/react-start/basic/package.json (1)
31-31: 💤 Low valueConsider moving unrelated dependency updates to a separate PR.
The updates to
tailwind-merge(line 31) andvite(line 49) appear unrelated to this PR's objective of supporting Rsbuild preview SSR middleware. While these updates might be routine maintenance, including them in this PR:
- Complicates review by introducing additional risk surface
- Makes it harder to isolate issues if problems arise
- Obscures the PR's focused intent
Consider moving these dependency updates to a separate maintenance PR unless they are specifically required for the Rsbuild preview functionality.
Also applies to: 49-49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/react-start/basic/package.json` at line 31, The change updates unrelated dependencies ("tailwind-merge" and "vite") in package.json which should be isolated; revert or remove the "tailwind-merge": "^3.6.0" and the updated "vite" entry from this branch and move those dependency bumps into a separate maintenance PR (or a dedicated commit) so this PR only contains the Rsbuild preview SSR middleware changes; update package.json to the previous versions for both the "tailwind-merge" and "vite" entries (or undo their edits) and ensure package-lock / yarn.lock is regenerated in the separate PR if you re-apply the bumps later.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@e2e/react-start/basic/package.json`:
- Line 31: The change updates unrelated dependencies ("tailwind-merge" and
"vite") in package.json which should be isolated; revert or remove the
"tailwind-merge": "^3.6.0" and the updated "vite" entry from this branch and
move those dependency bumps into a separate maintenance PR (or a dedicated
commit) so this PR only contains the Rsbuild preview SSR middleware changes;
update package.json to the previous versions for both the "tailwind-merge" and
"vite" entries (or undo their edits) and ensure package-lock / yarn.lock is
regenerated in the separate PR if you re-apply the bumps later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25c0ed05-ae70-447b-8ea2-299a69b91ca1
📒 Files selected for processing (2)
e2e/react-start/basic/package.jsonpackages/start-plugin-core/src/rsbuild/plugin.ts
… inventing 500 HTML
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/start-plugin-core/src/rsbuild/server-middleware.ts (1)
157-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn JSON 500s for server-function requests by route, not by request
content-type.This catch path is shared by page SSR and
serverFnBasecalls, but it only emits JSON when the request body is JSON. GET server functions and form posts can therefore fall through tonext(e)and get an HTML error page instead of the response shape the client expects.Possible fix
} catch (e) { console.error('[tanstack-start] SSR error:', e) + const url = req.originalUrl ?? req.url ?? '/' + const isServerFnRequest = + url === serverFnBase || url.startsWith(`${serverFnBase}/`) + const webReq = new NodeRequest({ req, res }) - if (webReq.headers.get('content-type')?.includes('application/json')) { + if ( + isServerFnRequest || + webReq.headers.get('content-type')?.includes('application/json') + ) { return sendNodeResponse( res, new Response(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-plugin-core/src/rsbuild/server-middleware.ts` around lines 157 - 181, The current catch block in server-middleware uses the request Content-Type to decide whether to return a JSON 500, which is wrong for distinguishing SSR pages vs server-function calls; update the condition around NodeRequest/new NodeRequest and sendNodeResponse so it detects server-function routes instead of content-type (e.g., add or call an isServerFunctionRequest(req|webReq) helper that checks the request path/method against the server-function route pattern used by serverFnBase or your router), and only emit the JSON error response for those server-function requests (otherwise continue to return next(e) for regular SSR page errors).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/start-plugin-core/src/rsbuild/server-middleware.ts`:
- Around line 264-266: The current check in the middleware trims
publicBasePathname from pathname using pathname.startsWith(publicBasePathname),
which incorrectly matches sibling prefixes (e.g., "/app" matching "/app2");
update the predicate to only strip when pathname === publicBasePathname or when
pathname has publicBasePathname followed by a "/" boundary (e.g., pathname ===
publicBasePathname || pathname.startsWith(publicBasePathname + "/")), then
perform the slice as before (pathname =
pathname.slice(publicBasePathname.length) || '/'); also apply the same
boundary-aware predicate to the other code path that handles restorePreviewUrl
so both preview branches use identical logic.
---
Outside diff comments:
In `@packages/start-plugin-core/src/rsbuild/server-middleware.ts`:
- Around line 157-181: The current catch block in server-middleware uses the
request Content-Type to decide whether to return a JSON 500, which is wrong for
distinguishing SSR pages vs server-function calls; update the condition around
NodeRequest/new NodeRequest and sendNodeResponse so it detects server-function
routes instead of content-type (e.g., add or call an
isServerFunctionRequest(req|webReq) helper that checks the request path/method
against the server-function route pattern used by serverFnBase or your router),
and only emit the JSON error response for those server-function requests
(otherwise continue to return next(e) for regular SSR page errors).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e40c862-cbdc-4f49-93c7-126237273b03
📒 Files selected for processing (2)
packages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/src/rsbuild/server-middleware.ts
Comment on lines
+264
to
+266
| if (publicBasePathname !== '/' && pathname.startsWith(publicBasePathname)) { | ||
| pathname = pathname.slice(publicBasePathname.length) || '/' | ||
| } |
Contributor
There was a problem hiding this comment.
Make public-base stripping path-segment aware.
pathname.startsWith(publicBasePathname) also matches sibling prefixes like /app2 when the configured public base is /app, so preview can resolve the wrong asset path. Require an exact match or a / boundary before trimming. If you touch this branch, it would be worth using the same predicate in restorePreviewUrl too so both preview code paths stay consistent.
Possible fix
- if (publicBasePathname !== '/' && pathname.startsWith(publicBasePathname)) {
+ const hasPublicBasePrefix =
+ pathname === publicBasePathname ||
+ pathname.startsWith(`${publicBasePathname}/`)
+
+ if (publicBasePathname !== '/' && hasPublicBasePrefix) {
pathname = pathname.slice(publicBasePathname.length) || '/'
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/start-plugin-core/src/rsbuild/server-middleware.ts` around lines 264
- 266, The current check in the middleware trims publicBasePathname from
pathname using pathname.startsWith(publicBasePathname), which incorrectly
matches sibling prefixes (e.g., "/app" matching "/app2"); update the predicate
to only strip when pathname === publicBasePathname or when pathname has
publicBasePathname followed by a "/" boundary (e.g., pathname ===
publicBasePathname || pathname.startsWith(publicBasePathname + "/")), then
perform the slice as before (pathname =
pathname.slice(publicBasePathname.length) || '/'); also apply the same
boundary-aware predicate to the other code path that handles restorePreviewUrl
so both preview branches use identical logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Support Rsbuild preview SSR middleware.
Previously, because the output generated by
rsbuild buildincluded server-side code for SSR,rsbuild previewtreated it as static assets, which caused the preview to malfunction. The middleware code shared between dev mode and preview mode has now been extracted.Details
This updates the Rsbuild integration so Start’s server middleware is no longer dev-only.
Because the option now controls both dev and preview middleware installation, the Rsbuild option has been renamed:
to:
Summary by CodeRabbit
New Features
Improvements
Documentation