fix: serve the SPA shell for ssr:false apps in rsbuild dev#76
fix: serve the SPA shell for ssr:false apps in rsbuild dev#76ScriptedAlchemy wants to merge 3 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
commit: |
📝 WalkthroughWalkthroughThis PR changes the rsbuild-plugin-react-router dev middleware setup so that dev-server middleware is registered whenever a custom server is not used, including SPA mode (ssr:false), rather than only when SSR is enabled. A corresponding unit test was added. Benchmark scripts (bench-builds.mts, profiles.mjs) were updated to reflect that SPA dev routes now require both web and node environments to be ready, removing the prior devRoutes: 'none' exemption. New Playwright dev-mode e2e configuration and tests were added for the SPA example, and its test:e2e script now runs both the default and dev-mode Playwright configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as "spa-mode-dev.test.ts"
participant DevServer as "rsbuild dev (port 3002)"
Test->>DevServer: GET /
DevServer-->>Test: HTML shell response
Test->>DevServer: GET /about, /docs/getting-started
DevServer-->>Test: HTML shell with hydration markers
Test->>DevServer: Click link, navigate client-side
DevServer-->>Test: Updated URL and heading
Estimated code review effort: 3/5 — Changes span core plugin logic, benchmark scripts, and new e2e test infrastructure, requiring verification of middleware behavior across SSR/SPA modes and benchmark correctness. Related issues: None specified. Related PRs: None specified. Suggested labels: bug, testing, examples Suggested reviewers: None specified. 🥕 A rabbit hopped through code so fine, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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 (1)
examples/spa-mode/tests/e2e-dev/spa-mode-dev.test.ts (1)
20-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo independent route checks combined in one test.
This test validates
/aboutand then separately navigates to/docs/getting-startedwithin the same test case. If the/aboutassertion fails, the nested-route coverage never runs, and failure diagnostics conflate two unrelated route checks.♻️ Split into separate test cases
- test('serves the SPA shell for deep links', async ({ page }) => { - const response = await page.goto('/about'); - - expect(response?.status()).toBe(200); - await expect(page.locator('h1:has-text("About This Demo")')).toBeVisible(); - - // Nested routes resolve as document requests too. - await page.goto('/docs/getting-started'); - await expect( - page.locator('h1:has-text("Getting Started")') - ).toBeVisible(); - }); + test('serves the SPA shell for deep links', async ({ page }) => { + const response = await page.goto('/about'); + + expect(response?.status()).toBe(200); + await expect(page.locator('h1:has-text("About This Demo")')).toBeVisible(); + }); + + test('serves the SPA shell for nested routes', async ({ page }) => { + await page.goto('/docs/getting-started'); + await expect( + page.locator('h1:has-text("Getting Started")') + ).toBeVisible(); + });🤖 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 `@examples/spa-mode/tests/e2e-dev/spa-mode-dev.test.ts` around lines 20 - 31, The SPA deep-link coverage in test('serves the SPA shell for deep links') combines two independent route checks, so split the /about assertion and the /docs/getting-started assertion into separate test cases. Keep the existing page.goto and expect checks, but isolate each route into its own test so failures are independent and easier to diagnose.
🤖 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 `@examples/spa-mode/playwright.dev.config.ts`:
- Around line 29-34: The Playwright webServer setup is only probing the dev
port, so the SPA can still race before the compiler is ready. Update the
Playwright dev config by changing the readiness check in the webServer block for
the dev server command/url to wait for the actual dev-ready signal from rsbuild
rather than only checking http://localhost:3002, keeping the existing webServer
configuration structure intact.
In `@tests/features.test.ts`:
- Around line 36-49: Clear the global test stub after the SPA middleware test so
it does not leak into later cases. In the `should register the dev middleware
for SPA mode (ssr: false)` test, clean up `globalThis.__reactRouterTestConfig`
after `pluginReactRouter()`/`createStubRsbuild()` assertions, using the same
`__reactRouterTestConfig` symbol to reset the state and keep tests isolated.
---
Nitpick comments:
In `@examples/spa-mode/tests/e2e-dev/spa-mode-dev.test.ts`:
- Around line 20-31: The SPA deep-link coverage in test('serves the SPA shell
for deep links') combines two independent route checks, so split the /about
assertion and the /docs/getting-started assertion into separate test cases. Keep
the existing page.goto and expect checks, but isolate each route into its own
test so failures are independent and easier to diagnose.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a8979cb-f831-4ede-81dd-dd2189270b5a
📒 Files selected for processing (7)
examples/spa-mode/package.jsonexamples/spa-mode/playwright.dev.config.tsexamples/spa-mode/tests/e2e-dev/spa-mode-dev.test.tsscripts/bench-builds.mtsscripts/benchmark/profiles.mjssrc/index.tstests/features.test.ts
💤 Files with no reviewable changes (1)
- scripts/benchmark/profiles.mjs
| webServer: { | ||
| command: 'pnpm run dev --port 3002', | ||
| url: 'http://localhost:3002', | ||
| reuseExistingServer: !process.env.CI, | ||
| timeout: 120000, | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does rsbuild dev server's HTTP port become available before all configured environments (e.g. node) finish compiling?
💡 Result:
Yes, the Rsbuild development server's HTTP port becomes available before the compilation of all configured environments is finished [1]. When you start the Rsbuild dev server—either via the command line or the JavaScript API (e.g., rsbuild.startDevServer() or devServer.listen())—the server initializes and binds to its designated port independently of the compilation process [2][3][4]. The development server process does not wait for all internal build tasks or environment compilations to complete before it starts listening for requests [1]. Because the server starts listening before compilation is necessarily finished, accessing the server while initial builds are still in progress may result in compilation-related delays or "compiling" states being served to the client [1][5]. To programmatically detect when the development compilation has actually finished, you should use the onDevCompileDone hook rather than relying on the dev server's startup status [1]. If your workflow requires ensuring compilation is complete before proceeding (for instance, in testing or automation environments), it is recommended to listen for this specific hook [1]: rsbuild.onDevCompileDone( => { console.log('Compilation finished'); });
Citations:
- 1: [Bug]: Using
rsbuild.createDevServerwithin vitest cause rspack panic web-infra-dev/rsbuild#2057 - 2: https://rsbuild.rs/api/javascript-api/server-api
- 3: https://v1.rsbuild.rs/api/javascript-api/dev-server-api.md
- 4: https://rsbuild.rs/plugins/dev/hooks
- 5: https://rsbuild.rs/config/dev/lazy-compilation.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the Playwright config and related benchmark/startup wait logic.
git ls-files 'examples/spa-mode/*' 'examples/**/playwright*.ts' 'examples/**/benchmark*' 'examples/**/wait*' 'examples/**/scripts*' | sed -n '1,200p'
echo '---'
# Inspect the target file with line numbers.
if [ -f examples/spa-mode/playwright.dev.config.ts ]; then
cat -n examples/spa-mode/playwright.dev.config.ts
fi
echo '---'
# Search for readiness checks and environment waits in the repo.
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
-e 'onDevCompileDone|wait.*node|wait.*web|webServer|reuseExistingServer|localhost:3002|benchmark' \
examples . | sed -n '1,240p'Repository: rstackjs/rsbuild-plugin-react-router
Length of output: 27141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- SPA example docs / scripts ---'
sed -n '1,220p' examples/spa-mode/package.json
echo '---'
sed -n '1,220p' examples/spa-mode/README.md
echo '--- Search for startup/readiness hooks in SPA and benchmark scripts ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
-e 'onDevCompileDone|wait.*compile|wait.*node|wait.*web|devBuild|loadBundle|startDevServer|listen\(' \
examples/spa-mode scripts benchmarks .github/workflows/benchmark.yml tests | sed -n '1,260p'Repository: rstackjs/rsbuild-plugin-react-router
Length of output: 15715
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- examples/spa-mode/rsbuild.config.ts ---'
cat -n examples/spa-mode/rsbuild.config.ts
echo '--- benchmarks/README.md excerpt ---'
sed -n '40,60p' benchmarks/README.md
echo '--- searches for spa-mode node environment / SSR toggles ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
-e 'ssr:\s*false|environments:\s*\{|node:\s*\{|web:\s*\{' \
examples/spa-mode examples/*/rsbuild.config.ts src tests | sed -n '1,260p'Repository: rstackjs/rsbuild-plugin-react-router
Length of output: 8861
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/index.ts excerpt around SPA/dev environment setup ---'
sed -n '760,880p' src/index.ts
echo '--- tests/features.test.ts excerpt for SPA dev middleware ---'
sed -n '1,90p' tests/features.test.ts
echo '--- examples/spa-mode/tests/e2e-dev/spa-mode-dev.test.ts ---'
cat -n examples/spa-mode/tests/e2e-dev/spa-mode-dev.test.tsRepository: rstackjs/rsbuild-plugin-react-router
Length of output: 9831
Add a startup wait before Playwright checks the dev server
webServer.url only probes that rsbuild dev is reachable; SPA dev still brings up the middleware and node environment asynchronously, so the first navigation can race compiler readiness. Use a readiness check that waits for the dev-ready signal instead of the bare port probe.
🤖 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 `@examples/spa-mode/playwright.dev.config.ts` around lines 29 - 34, The
Playwright webServer setup is only probing the dev port, so the SPA can still
race before the compiler is ready. Update the Playwright dev config by changing
the readiness check in the webServer block for the dev server command/url to
wait for the actual dev-ready signal from rsbuild rather than only checking
http://localhost:3002, keeping the existing webServer configuration structure
intact.
| it('should register the dev middleware for SPA mode (ssr: false)', async () => { | ||
| (globalThis as { __reactRouterTestConfig?: unknown }).__reactRouterTestConfig = | ||
| { ssr: false }; | ||
| const rsbuild = await createStubRsbuild({ | ||
| rsbuildConfig: {}, | ||
| }); | ||
|
|
||
| rsbuild.addPlugins([pluginReactRouter()]); | ||
| const config = await rsbuild.unwrapConfig(); | ||
|
|
||
| expect(config.dev.setupMiddlewares).toHaveLength(1); | ||
| expect(config.dev.setupMiddlewares[0]).toBeInstanceOf(Function); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for cleanup of __reactRouterTestConfig in the test file
rg -n '__reactRouterTestConfig' tests/features.test.ts -B2 -A2Repository: rstackjs/rsbuild-plugin-react-router
Length of output: 433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test file for any setup/teardown that clears the global stub,
# and show all references to the stub in context.
printf '\n== file outline ==\n'
wc -l tests/features.test.ts
printf '\n== cleanup hooks and stub references ==\n'
rg -n 'beforeEach|afterEach|beforeAll|afterAll|__reactRouterTestConfig|ssr:' tests/features.test.ts -B3 -A4Repository: rstackjs/rsbuild-plugin-react-router
Length of output: 637
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for all uses of the stub and surrounding test structure in the file.
sed -n '1,220p' tests/features.test.ts | cat -nRepository: rstackjs/rsbuild-plugin-react-router
Length of output: 9257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the file sections around any before/after hooks and the later tests that
# may depend on the default SPA/SSR config.
rg -n 'beforeEach|afterEach|beforeAll|afterAll|__reactRouterTestConfig|pluginReactRouter\(' tests/features.test.ts -B4 -A8Repository: rstackjs/rsbuild-plugin-react-router
Length of output: 5275
Clear __reactRouterTestConfig after this test The global stub is left behind as { ssr: false }, so later tests can inherit SPA mode and become order-dependent.
🤖 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 `@tests/features.test.ts` around lines 36 - 49, Clear the global test stub
after the SPA middleware test so it does not leak into later cases. In the
`should register the dev middleware for SPA mode (ssr: false)` test, clean up
`globalThis.__reactRouterTestConfig` after
`pluginReactRouter()`/`createStubRsbuild()` assertions, using the same
`__reactRouterTestConfig` symbol to reset the state and keep tests isolated.
ScriptedAlchemy
force-pushed
the
claude/musing-shaw-924569
branch
from
a5b36d5 to
e400982
Compare
ScriptedAlchemy
force-pushed
the
claude/peaceful-benz-6eae2b
branch
from
b3c7fea to
7d2914b
Compare
SPA mode apps returned 404 for every route in dev because the plugin skipped its dev middleware when ssr:false and all web entries disable HTML generation, so no HTML document existed. React Router's request handler natively renders the SPA shell for ssr:false builds, and the dev server build (isSpaMode: true) is already compiled by the node environment, so register the middleware for SPA mode too. - Add a dev-mode Playwright suite to examples/spa-mode (separate config since the dev server writes into the build/ directory the static suite asserts against) - Remove the devRoutes: 'none' benchmark override from PR #75 and wait for the node environment before fetching SPA dev routes, since route serving now depends on the server build Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review cleanup: derive playwright.dev.config.ts from the sibling static config instead of copying it, reuse the testGlobal pattern in features.test.ts, fold the hydration-payload assertions into the shell test to drop a duplicate navigation, and remove the per-profile benchmark devRoutes override mechanism now that nothing produces it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The benchmark harness waits for an explicit, caller-provided set of dev environments (['web', 'node'] for every fixture variant — SPA dev route serving goes through the node server build too, so 'node' must be ready before route fetches). The ready-line pattern captures any environment name, and a startup timeout now reports which expected environments were still awaited vs. observed instead of hanging silently. Also aligns the SPA-mode feature test with the server.setup registration that now includes ssr:false apps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ScriptedAlchemy
force-pushed
the
claude/musing-shaw-924569
branch
from
e400982 to
b3fce8d
Compare
Benchmark ResultsCompared PR head Dev Rollup
Production Build BenchmarksRendered 7 production build benchmarks.
full Dev Fixture SummaryRendered 7 dev benchmark fixtures from the
large-355-ssr-esm Plugin Operations
synthetic-1024-ssr-esm Plugin Operations
synthetic-1024-ssr-esm-split Plugin Operations
synthetic-256-sourcemaps Plugin Operations
synthetic-256-ssr-esm Plugin Operations
synthetic-256-ssr-esm-split Plugin Operations
synthetic-48-ssr-esm Plugin Operations
Synthetic Rsbuild AppRendered 2 production build benchmarks.
Rendered 1 dev benchmark fixture from the embedded complex app.
Profile: |
1 participant
Problem
ssr: false(SPA mode) apps returned HTTP 404 for every route inrsbuild dev, including/. Two things combined to cause this:setupMiddlewaresskipped the dev request middleware wheneverssr: false(pluginOptions.customServer || !ssr ? [] : [...])html: false, so no HTML document existed in dev at allProduction worked because the SPA build prerenders an
index.htmlshell — and theexamples/spa-modee2e suite only testedbuild+ static serve, so this was never caught.Fix
React Router's request handler natively supports
ssr: falseserver builds: whenbuild.ssris false (and no prerender paths are configured) it renders the SPA shell for any document request — the same shell the production build prerenders intoindex.html. The dev server build (withisSpaMode: true) is already compiled by the node environment, which always exists. So the fix is to drop the!ssrexclusion and register the dev middleware for SPA mode too.Unknown paths return the app shell with a 404 status, so the client-side error boundary renders — matching React Router's Vite dev behavior.
Tests
ssr: false(tests/features.test.ts)examples/spa-mode(playwright.dev.config.ts+tests/e2e-dev/) covering/, deep links to nested routes, shell hydration data, and client-side navigation. Kept as a separate config because the dev server writes into the samebuild/directory the static suite asserts against;test:e2enow runs both, so CI picks it up.tsc --noEmitclean.Benchmarks
Removes the
devRoutes: 'none'workaround onsynthetic-256-spaadded in #75, so benchmarks measure SPA dev route serving again. Also waits for thenodeenvironment (not justweb) before fetching SPA dev routes, since route serving now goes through the server build — otherwise node compile time would be misattributed to route-fetch latency. Verified with a real run: all 8 dev route fetches return 200 (~20–165 ms), including the post-HMR-update fetch.🤖 Generated with Claude Code