perf: parallel dev compilers, worker-pool route transforms, and startup/HMR optimizations#54
Merged
Merged
Conversation
commit: |
Contributor
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: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1095e45d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
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:
📝 WalkthroughWalkthroughThis PR refactors 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
examples/custom-node-server/scripts/smoke-production.mjs-64-70 (1)
64-70: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid the fixed 5s teardown delay after early exits.
If the child has already exited before this block runs,
once(child, 'exit')subscribes too late and the script always waits the full timeout. Capture the exit promise before signaling, and skip the wait whenexitCodeis already set.Proposed fix
} finally { - child.kill('SIGTERM'); - await Promise.race([ - once(child, 'exit'), - new Promise(resolve => setTimeout(resolve, 5_000)), - ]); - if (child.exitCode === null) { - child.kill('SIGKILL'); - } + if (child.exitCode === null) { + const exited = once(child, 'exit'); + child.kill('SIGTERM'); + await Promise.race([ + exited, + new Promise(resolve => setTimeout(resolve, 5_000)), + ]); + if (child.exitCode === null) { + child.kill('SIGKILL'); + } + } }🤖 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/custom-node-server/scripts/smoke-production.mjs` around lines 64 - 70, The teardown in smoke-production.mjs waits on once(child, 'exit') only after sending SIGTERM, which can miss an exit that already happened and force the full timeout. Update the child process handling around the existing child.kill and Promise.race flow by creating the exit promise before signaling, and guard the wait with child.exitCode so you skip the timeout path when the process has already exited.README.md-107-112 (1)
107-112: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument one
parallelTransformsdefault.Line 110 says
@default true, but the defaults section on Lines 309-314 says the option defaults toundefinedwith worker execution enabled implicitly. Pick one representation so the public API contract is not self-contradictory.🤖 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 `@README.md` around lines 107 - 112, The README documentation for parallelTransforms is contradictory about its default value. Update the public API docs so the JSDoc on parallelTransforms and the defaults section describe the same behavior, using one consistent default representation in both places. Refer to parallelTransforms in the options docs and the defaults section, and make sure the wording matches whether worker execution is enabled implicitly or via an explicit true default.README.md-527-527 (1)
527-527: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winExit on unrecoverable startup failure.
Line 527 only logs boot errors, but the section above explicitly requires a clean process restart after startup failure. Exit non-zero after logging so the example does not stay alive in a broken state.
Proposed fix
-startServer().catch(console.error); +startServer().catch(error => { + console.error(error); + process.exit(1); +});🤖 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 `@README.md` at line 527, The startup example in startServer() currently only logs failures with console.error and leaves the process running, which conflicts with the documented requirement for unrecoverable startup failures. Update the startup handling so the catch path for startServer() both logs the error and terminates the process with a non-zero exit code, keeping the example from continuing in a broken state.scripts/benchmark/fixture.mjs-648-658 (1)
648-658: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail fast on invalid
largeConfigcounts.The new
largeConfigoverride path is unvalidated, but several generators later use counts likeworkers,localeFiles,cssModules,svgAssets,utilitiesPerRoute, andpayloadEntriesPerComponentin%/division and import-path generation. Passing0currently emits broken fixture code instead of rejecting the config up front.🛠️ Proposed fix
+const assertPositiveInteger = (name, value) => { + if (!Number.isInteger(value) || value < 1) { + throw new Error( + `${name} must be a positive integer for the large benchmark fixture.` + ); + } +}; + const generateLargeFixture = async ({ root, routeCount, variant, @@ }) => { const config = normalizeLargeConfig(routeCount, largeConfig); + for (const [name, value] of Object.entries({ + routes: config.routes, + componentsPerRoute: config.componentsPerRoute, + utilitiesPerRoute: config.utilitiesPerRoute, + lazyModulesPerRoute: config.lazyModulesPerRoute, + workers: config.workers, + svgAssets: config.svgAssets, + cssModules: config.cssModules, + localeFiles: config.localeFiles, + payloadEntriesPerComponent: config.payloadEntriesPerComponent, + })) { + assertPositiveInteger(name, value); + } const isSpa = variant === 'spa';Also applies to: 814-834
🤖 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 `@scripts/benchmark/fixture.mjs` around lines 648 - 658, The largeConfig override path in generateLargeFixture/normalizeLargeConfig accepts invalid zero values, which later break generators that use workers, localeFiles, cssModules, svgAssets, utilitiesPerRoute, and payloadEntriesPerComponent in division, modulo, or import-path logic. Add upfront validation for these count fields in normalizeLargeConfig (or immediately after calling it in generateLargeFixture) and reject non-positive values with a clear error before any fixture generation starts.rslib.config.ts-23-28 (1)
23-28: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winEmit the route transform worker from the CJS build too.
createWorkerUrl()always resolves./parallel-route-transform-worker.js, but the CJSsource.entryblock doesn’t include that file. CJS consumers won’t get the worker asset, so parallel transforms will fall back to the serial path.Proposed fix
source: { entry: { index: './src/index.ts', + 'parallel-route-transform-worker': + './src/parallel-route-transform-worker.ts', 'templates/entry.server': './src/templates/entry.server.tsx', 'templates/entry.client': './src/templates/entry.client.tsx', },🤖 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 `@rslib.config.ts` around lines 23 - 28, The CJS source.entry map is missing the route transform worker entry, so CJS builds won’t emit the asset that createWorkerUrl() resolves. Update the source.entry block in rslib.config.ts to include the parallel route transform worker alongside the existing index, templates/entry.server, and templates/entry.client entries, so CJS consumers also receive parallel-route-transform-worker.js.src/route-export-pruning.ts-544-549 (1)
544-549: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t track sourced re-export names as local bindings.
For
export { loader as clientLoader } from "./x",specifier.local.nameis the imported name from the source module, not a local binding. Adding it toremovedExportLocalNamescan make the later assignment cleanup remove unrelated localloader.* = ...statements.Proposed fix
const exportedName = getExportedName(specifier); if (exportedName && exportsToRemoveSet.has(exportedName)) { exportsChanged = true; - if (specifier.local?.name) { + if (!statement.source && specifier.local?.name) { removedExportLocalNames.add(specifier.local.name); removedExportReferencedNames.add(specifier.local.name); } return false; }🤖 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 `@src/route-export-pruning.ts` around lines 544 - 549, The re-export cleanup in the export pruning flow is incorrectly treating sourced re-export names as local bindings. In the logic around the export specifier handling in route-export-pruning and the sets removedExportLocalNames / removedExportReferencedNames, only add names that are actually declared locally in the file; for `export { loader as clientLoader } from "./x"`, avoid recording `specifier.local.name` as a local binding so later assignment cleanup does not delete unrelated `loader.*` statements. Use the existing export-specifier and assignment-pruning checks to distinguish sourced re-exports from true local exports before adding names to those tracking sets.tests/index.test.ts-106-139 (1)
106-139: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore the previous
existsSyncmock implementation.Line 111 overrides a shared mock, but Line 138 leaves it hardcoded to
true. Capture and restore the previous implementation so this test cannot leak filesystem behavior into later cases.🤖 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/index.test.ts` around lines 106 - 139, Restore the shared fs.existsSync mock in this test by capturing the previous mock implementation before overriding it and reinstating that exact implementation in the finally block. Update the test around createStubRsbuild and pluginReactRouter so the temporary behavior used for the watchFiles assertion does not leak into later tests.src/index.ts-231-239 (1)
231-239: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat
serverOutput: undefinedas omitted.
hasOwnPropertymakes{ serverOutput: undefined }explicit, soresolvedServerOutputcan becomeundefinedand flow into the node Rspack config.Proposed fix
- const hasExplicitServerOutput = Object.prototype.hasOwnProperty.call( - options, - 'serverOutput' - ); - let resolvedServerOutput = pluginOptions.serverOutput; - if (!hasExplicitServerOutput) { - resolvedServerOutput = - serverModuleFormat === 'cjs' ? 'commonjs' : 'module'; - } + const hasExplicitServerOutput = options.serverOutput !== undefined; + const resolvedServerOutput = hasExplicitServerOutput + ? options.serverOutput + : serverModuleFormat === 'cjs' + ? 'commonjs' + : 'module';🤖 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 `@src/index.ts` around lines 231 - 239, Treat serverOutput: undefined the same as an omitted option in the server output resolution logic. Update the hasExplicitServerOutput check in the src/index.ts flow around pluginOptions.serverOutput so that undefined does not count as explicitly set, and keep the fallback based on serverModuleFormat ('cjs' => commonjs, otherwise module) from flowing into the Node Rspack config. Reference the server output resolution branch and the resolvedServerOutput assignment when making the fix.src/dev-generation.ts-551-553 (1)
551-553: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
load()asynchronous in the ready path.
selectBuild()can throw here, which escapes synchronously instead of rejecting the promised API. That makesloadReactRouterServerBuild()behave differently betweenstartingandreadystates for the same missing-build failure.Proposed fix
if (state.kind === 'ready') { - return Promise.resolve(selectBuild(state.committed, entryName)); + return Promise.resolve().then(() => + selectBuild(state.committed, entryName) + ); }🤖 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 `@src/dev-generation.ts` around lines 551 - 553, The `load()` implementation in `src/dev-generation.ts` should stay asynchronous in the `state.kind === 'ready'` branch, because calling `selectBuild()` directly can throw synchronously instead of returning a rejected promise. Update the ready-path logic around `selectBuild()` and `loadReactRouterServerBuild()` so missing-build failures are handled through promise rejection consistently with the `starting` state.src/typegen.ts-12-19 (1)
12-19: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse the local
react-routerCLI and keep watch failures visible.
npx --yescan fall back to a registry install when the binary is missing; run the app’s installed CLI directly so typegen stays on the pinned version.- The watch process currently swallows every rejection. Only ignore shutdown, or restart/log unexpected exits so typegen doesn’t die silently.
🤖 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 `@src/typegen.ts` around lines 12 - 19, The typegen launcher in typegen.ts should use the project’s locally installed react-router CLI instead of npx --yes, so update the execa invocation to call the pinned binary directly from the app dependencies. In the same typegen/watch flow, avoid swallowing every rejected process exit: adjust the process handling around the execa call so only intentional shutdown is ignored, and unexpected failures from the watch process are surfaced or restarted/logged. Use the existing typegen process setup and its process variable as the place to make these changes.
🤖 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 `@src/build-output-transforms.ts`:
- Around line 123-136: The fallback in build-output-transforms should stay
bundle-scoped even when latestServerManifestsByBundleId is not yet populated. In
the manifest selection logic around getLatestServerManifest,
getLatestServerManifestByBundleId, and getReactRouterManifestForDev, ensure the
cold-path dev manifest returned for
virtual/react-router/server-manifest-${bundleId} is filtered to the current
bundleId before it is used. Keep the per-bundle behavior consistent with the
virtual module wiring from src/index.ts so the fallback never embeds routes from
other bundles.
In `@src/dev-runtime-artifacts.ts`:
- Around line 145-174: The assertBuildMatchesManifest check only verifies routes
present in ReactRouterDevManifest, so stale ServerBuild entries for deleted
routes can slip through. Update assertBuildMatchesManifest to also compare
build.routes against manifest.routes and throw when the evaluated server build
contains any extra route IDs not in the manifest, alongside the existing
missing-route and loader/action export checks. Use the existing entryName,
build, manifest, and routeId logic to keep the validation in one place.
In `@src/dev-runtime-session.ts`:
- Around line 62-69: The completeClose flow in dev-runtime-session skips
closeBinding() when the session is already in the closing state, so successful
server.close() calls never fully clean up the runtime. Update completeClose to
invoke closeBinding(binding) for the successful closing path as well as active,
while still guarding with isCurrentBinding(binding) and preserving the
transition to idle; apply the same fix in the other matching close path around
the later close handler.
In `@src/prerender-build.ts`:
- Around line 175-177: The prerender write path logic in the output path
construction must prevent traversal outside clientBuildDir. Update the path
handling in prerender-build.ts around the outputPath/mkdir/writeFile flow to
validate normalizedPath segments before joining, reject any .. or
backslash-based traversal, and assert the final resolved path stays under
clientBuildDir. Apply the same safeguard to the other prerender output write
sites mentioned by the build flow helpers so all generated files remain confined
to the client build directory.
- Around line 496-498: The cleanup in enqueue path handling creates an
unconsumed promise from task.finally, which can surface prerender failures as
unhandled rejections; update the logic around enqueue, pending, and task so the
cleanup promise is handled explicitly. Use the task promise’s then with both
fulfillment and rejection handlers, or attach a catch to the promise returned by
finally, while still removing the task from pending.
- Around line 549-607: The prerender/build-request flow in prerender-build.ts
leaves process.env.IS_RR_BUILD_REQUEST mutated after the work completes. In the
block around resolveServerBuildModule, createRequestHandler, runPrerenderPaths,
and handleSpaMode, save the previous IS_RR_BUILD_REQUEST value before setting it
and restore it in a finally block after all prerender/Spa handling finishes.
This should be done around the existing build request logic so buildEnd and
subsequent runs/tests see the original process-global state.
- Around line 211-225: Escape the redirect target before interpolating it into
the fallback HTML in the redirect handling block so untrusted `Location` values
cannot inject markup or attributes. Update the logic around
`redirectStatusCodes`, `response.headers.get('Location')`, and the HTML template
in `src/prerender-build.ts` to use a safely escaped/sanitized version for the
`<title>`, `meta refresh`, and `<a href>`/link text. Keep `normalizedPath`
as-is, but ensure every use of the redirect destination is HTML- and
attribute-safe.
In `@src/route-chunks.ts`:
- Around line 211-216: The shared side-effect collection in route chunking is
incorrectly treating directive prologues like "use client" and "use strict" as
blocking statements. Update the filtering logic in the shared side-effect scan
and the related `topLevelNonModuleStatements` handling so `ExpressionStatement`s
that are directive prologues are excluded, while real side-effectful expressions
remain included. Use the existing `sharedTopLevelSideEffects` and export
analysis paths to ensure `splitRouteModules: "enforce"` only rejects genuinely
shared side effects.
In `@src/route-export-resolution.ts`:
- Around line 160-182: Update resolvePackageExports and related callers so
subpath imports are handled correctly: when packageJson.exports is absent, do
not always fall back to module/main for every packageSubpath, and make sure
pkg/subpath is not resolved as the package entry. In the exports branch, enhance
the lookup logic in resolvePackageExports to support non-exact export shapes
such as pattern mappings like "./*" instead of only direct packageSubpath keys.
Then adjust resolveExportAllModule so a null result from resolvePackageExports
is treated as “not matched yet” and it can still fall back to
createRequire(...).resolve() before rejecting the import.
In `@src/typegen.ts`:
- Around line 22-30: The `typegen --watch` promise handling in `typegenProcess`
is swallowing every rejection, which hides unexpected watcher exits. Update the
`.catch` path so it only ignores the intentional shutdown case from server
teardown, and for any other rejection in `src/typegen.ts` surface the failure by
logging or rethrowing it. Use the existing `typegenProcess` cleanup in
`.finally()` to keep the process reset behavior intact while making crashes or
invalid config failures visible.
In `@src/virtual-modules.ts`:
- Around line 11-19: The virtual module ID validation in the module path builder
does not block backslash separators, so a Windows-style traversal pattern can
slip through. Update the validation in the relativeId/segments check to reject
any segment containing backslashes before returning the node_modules path,
keeping the existing dot-segment and empty-segment checks in place.
---
Minor comments:
In `@examples/custom-node-server/scripts/smoke-production.mjs`:
- Around line 64-70: The teardown in smoke-production.mjs waits on once(child,
'exit') only after sending SIGTERM, which can miss an exit that already happened
and force the full timeout. Update the child process handling around the
existing child.kill and Promise.race flow by creating the exit promise before
signaling, and guard the wait with child.exitCode so you skip the timeout path
when the process has already exited.
In `@README.md`:
- Around line 107-112: The README documentation for parallelTransforms is
contradictory about its default value. Update the public API docs so the JSDoc
on parallelTransforms and the defaults section describe the same behavior, using
one consistent default representation in both places. Refer to
parallelTransforms in the options docs and the defaults section, and make sure
the wording matches whether worker execution is enabled implicitly or via an
explicit true default.
- Line 527: The startup example in startServer() currently only logs failures
with console.error and leaves the process running, which conflicts with the
documented requirement for unrecoverable startup failures. Update the startup
handling so the catch path for startServer() both logs the error and terminates
the process with a non-zero exit code, keeping the example from continuing in a
broken state.
In `@rslib.config.ts`:
- Around line 23-28: The CJS source.entry map is missing the route transform
worker entry, so CJS builds won’t emit the asset that createWorkerUrl()
resolves. Update the source.entry block in rslib.config.ts to include the
parallel route transform worker alongside the existing index,
templates/entry.server, and templates/entry.client entries, so CJS consumers
also receive parallel-route-transform-worker.js.
In `@scripts/benchmark/fixture.mjs`:
- Around line 648-658: The largeConfig override path in
generateLargeFixture/normalizeLargeConfig accepts invalid zero values, which
later break generators that use workers, localeFiles, cssModules, svgAssets,
utilitiesPerRoute, and payloadEntriesPerComponent in division, modulo, or
import-path logic. Add upfront validation for these count fields in
normalizeLargeConfig (or immediately after calling it in generateLargeFixture)
and reject non-positive values with a clear error before any fixture generation
starts.
In `@src/dev-generation.ts`:
- Around line 551-553: The `load()` implementation in `src/dev-generation.ts`
should stay asynchronous in the `state.kind === 'ready'` branch, because calling
`selectBuild()` directly can throw synchronously instead of returning a rejected
promise. Update the ready-path logic around `selectBuild()` and
`loadReactRouterServerBuild()` so missing-build failures are handled through
promise rejection consistently with the `starting` state.
In `@src/index.ts`:
- Around line 231-239: Treat serverOutput: undefined the same as an omitted
option in the server output resolution logic. Update the hasExplicitServerOutput
check in the src/index.ts flow around pluginOptions.serverOutput so that
undefined does not count as explicitly set, and keep the fallback based on
serverModuleFormat ('cjs' => commonjs, otherwise module) from flowing into the
Node Rspack config. Reference the server output resolution branch and the
resolvedServerOutput assignment when making the fix.
In `@src/route-export-pruning.ts`:
- Around line 544-549: The re-export cleanup in the export pruning flow is
incorrectly treating sourced re-export names as local bindings. In the logic
around the export specifier handling in route-export-pruning and the sets
removedExportLocalNames / removedExportReferencedNames, only add names that are
actually declared locally in the file; for `export { loader as clientLoader }
from "./x"`, avoid recording `specifier.local.name` as a local binding so later
assignment cleanup does not delete unrelated `loader.*` statements. Use the
existing export-specifier and assignment-pruning checks to distinguish sourced
re-exports from true local exports before adding names to those tracking sets.
In `@src/typegen.ts`:
- Around line 12-19: The typegen launcher in typegen.ts should use the project’s
locally installed react-router CLI instead of npx --yes, so update the execa
invocation to call the pinned binary directly from the app dependencies. In the
same typegen/watch flow, avoid swallowing every rejected process exit: adjust
the process handling around the execa call so only intentional shutdown is
ignored, and unexpected failures from the watch process are surfaced or
restarted/logged. Use the existing typegen process setup and its process
variable as the place to make these changes.
In `@tests/index.test.ts`:
- Around line 106-139: Restore the shared fs.existsSync mock in this test by
capturing the previous mock implementation before overriding it and reinstating
that exact implementation in the finally block. Update the test around
createStubRsbuild and pluginReactRouter so the temporary behavior used for the
watchFiles assertion does not leak into later tests.
🪄 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: 5108dcda-f5f7-41c8-9159-04e0bd83d220
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (124)
.changeset/bright-routes-run.md.changeset/coherent-dev-generations.md.changeset/lazy-entry-hydration.md.changeset/quiet-topology-order.md.changeset/sharp-routes-heal.md.changeset/warm-watchers-rest.mdREADME.mdbenchmarks/README.mdconfig/package.jsonconfig/rslib.config.tsdocs/coherent-dev-generations.mdexamples/client-only/package.jsonexamples/cloudflare/package.jsonexamples/custom-node-server/app/load-context.d.tsexamples/custom-node-server/app/routes/projects/edit.tsxexamples/custom-node-server/app/routes/projects/index.tsxexamples/custom-node-server/app/routes/projects/layout.tsxexamples/custom-node-server/app/routes/projects/project.tsxexamples/custom-node-server/app/routes/projects/settings.tsxexamples/custom-node-server/package.jsonexamples/custom-node-server/playwright.config.tsexamples/custom-node-server/rsbuild.config.tsexamples/custom-node-server/scripts/smoke-production.mjsexamples/custom-node-server/server.jsexamples/custom-node-server/server/index.tsexamples/custom-node-server/tsconfig.jsonexamples/default-template/app/dev-routes.tsexamples/default-template/app/routes.tsexamples/default-template/package.jsonexamples/default-template/playwright.config.tsexamples/default-template/rsbuild.config.tsexamples/default-template/tests/e2e/dev-route-watch.test.tsexamples/default-template/tests/e2e/lazy-compilation.test.tsexamples/epic-stack/package.jsonexamples/federation/epic-stack-remote/package.jsonexamples/federation/epic-stack/package.jsonexamples/prerender/package.jsonexamples/spa-mode/package.jsonpackage.jsonrslib.config.tsscripts/bench-builds.mjsscripts/benchmark/fixture.mjsscripts/compare-benchmarks.mjsscripts/test-package-interop.mjssrc/babel.tssrc/bounded-cache.tssrc/build-manifest.tssrc/build-output-transforms.tssrc/concurrency.tssrc/constants.tssrc/dev-generation.tssrc/dev-runtime-artifacts.tssrc/dev-runtime-compilation.tssrc/dev-runtime-controller.tssrc/dev-runtime-session.tssrc/dev-server.tssrc/export-utils.tssrc/index.tssrc/lazy-compilation.tssrc/manifest.tssrc/modify-browser-manifest.tssrc/parallel-route-transform-protocol.tssrc/parallel-route-transform-worker.tssrc/parallel-route-transforms.tssrc/performance.tssrc/plugin-utils.tssrc/prerender-build.tssrc/prerender.tssrc/react-router-config.tssrc/route-artifacts.tssrc/route-ast.tssrc/route-chunks.tssrc/route-component-transform.tssrc/route-export-pruning.tssrc/route-export-resolution.tssrc/route-transform-tasks.tssrc/route-watch.tssrc/server-build-plan.tssrc/server-utils.tssrc/templates/entry.server.tsxsrc/typegen.tssrc/types.tssrc/virtual-modules.tssrc/warnings/warn-on-client-source-maps.tssrc/yuku.tstests/benchmark-fixture.test.tstests/bounded-cache.test.tstests/build-manifest.test.tstests/build-output-transforms.test.tstests/client-modules.test.tstests/dev-generation-multi-entry.test.tstests/dev-generation.test.tstests/dev-runtime-controller.test.tstests/dev-runtime.integration.test.tstests/dev-server.test.tstests/export-utils.test.tstests/features.test.tstests/fixtures/dev-runtime/app/entry.client.tsxtests/fixtures/dev-runtime/app/entry.server.tsxtests/fixtures/dev-runtime/app/root.tsxtests/fixtures/dev-runtime/app/routes.tstests/fixtures/dev-runtime/app/routes/index.tsxtests/fixtures/dev-runtime/app/routes/other.tsxtests/fixtures/dev-runtime/react-router.config.tstests/fixtures/dev-runtime/server/index.tstests/index.test.tstests/lazy-compilation.test.tstests/manifest-split-route-modules.test.tstests/manifest.test.tstests/modify-browser-manifest.test.tstests/parallel-route-transforms.test.tstests/performance.test.tstests/plugin-utils.test.tstests/prerender.test.tstests/react-router-config.test.tstests/remove-exports.test.tstests/route-artifacts.test.tstests/route-chunks-cache.test.tstests/route-chunks.test.tstests/route-watch.test.tstests/server-build-plan.test.tstests/server-utils.test.tstests/setup.tstests/warn-on-client-source-maps.test.ts
💤 Files with no reviewable changes (2)
- examples/custom-node-server/server/index.ts
- src/babel.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (10)
examples/custom-node-server/scripts/smoke-production.mjs-64-70 (1)
64-70: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAvoid the fixed 5s teardown delay after early exits.
If the child has already exited before this block runs,
once(child, 'exit')subscribes too late and the script always waits the full timeout. Capture the exit promise before signaling, and skip the wait whenexitCodeis already set.Proposed fix
} finally { - child.kill('SIGTERM'); - await Promise.race([ - once(child, 'exit'), - new Promise(resolve => setTimeout(resolve, 5_000)), - ]); - if (child.exitCode === null) { - child.kill('SIGKILL'); - } + if (child.exitCode === null) { + const exited = once(child, 'exit'); + child.kill('SIGTERM'); + await Promise.race([ + exited, + new Promise(resolve => setTimeout(resolve, 5_000)), + ]); + if (child.exitCode === null) { + child.kill('SIGKILL'); + } + } }🤖 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/custom-node-server/scripts/smoke-production.mjs` around lines 64 - 70, The teardown in smoke-production.mjs waits on once(child, 'exit') only after sending SIGTERM, which can miss an exit that already happened and force the full timeout. Update the child process handling around the existing child.kill and Promise.race flow by creating the exit promise before signaling, and guard the wait with child.exitCode so you skip the timeout path when the process has already exited.README.md-107-112 (1)
107-112: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument one
parallelTransformsdefault.Line 110 says
@default true, but the defaults section on Lines 309-314 says the option defaults toundefinedwith worker execution enabled implicitly. Pick one representation so the public API contract is not self-contradictory.🤖 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 `@README.md` around lines 107 - 112, The README documentation for parallelTransforms is contradictory about its default value. Update the public API docs so the JSDoc on parallelTransforms and the defaults section describe the same behavior, using one consistent default representation in both places. Refer to parallelTransforms in the options docs and the defaults section, and make sure the wording matches whether worker execution is enabled implicitly or via an explicit true default.README.md-527-527 (1)
527-527: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winExit on unrecoverable startup failure.
Line 527 only logs boot errors, but the section above explicitly requires a clean process restart after startup failure. Exit non-zero after logging so the example does not stay alive in a broken state.
Proposed fix
-startServer().catch(console.error); +startServer().catch(error => { + console.error(error); + process.exit(1); +});🤖 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 `@README.md` at line 527, The startup example in startServer() currently only logs failures with console.error and leaves the process running, which conflicts with the documented requirement for unrecoverable startup failures. Update the startup handling so the catch path for startServer() both logs the error and terminates the process with a non-zero exit code, keeping the example from continuing in a broken state.scripts/benchmark/fixture.mjs-648-658 (1)
648-658: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail fast on invalid
largeConfigcounts.The new
largeConfigoverride path is unvalidated, but several generators later use counts likeworkers,localeFiles,cssModules,svgAssets,utilitiesPerRoute, andpayloadEntriesPerComponentin%/division and import-path generation. Passing0currently emits broken fixture code instead of rejecting the config up front.🛠️ Proposed fix
+const assertPositiveInteger = (name, value) => { + if (!Number.isInteger(value) || value < 1) { + throw new Error( + `${name} must be a positive integer for the large benchmark fixture.` + ); + } +}; + const generateLargeFixture = async ({ root, routeCount, variant, @@ }) => { const config = normalizeLargeConfig(routeCount, largeConfig); + for (const [name, value] of Object.entries({ + routes: config.routes, + componentsPerRoute: config.componentsPerRoute, + utilitiesPerRoute: config.utilitiesPerRoute, + lazyModulesPerRoute: config.lazyModulesPerRoute, + workers: config.workers, + svgAssets: config.svgAssets, + cssModules: config.cssModules, + localeFiles: config.localeFiles, + payloadEntriesPerComponent: config.payloadEntriesPerComponent, + })) { + assertPositiveInteger(name, value); + } const isSpa = variant === 'spa';Also applies to: 814-834
🤖 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 `@scripts/benchmark/fixture.mjs` around lines 648 - 658, The largeConfig override path in generateLargeFixture/normalizeLargeConfig accepts invalid zero values, which later break generators that use workers, localeFiles, cssModules, svgAssets, utilitiesPerRoute, and payloadEntriesPerComponent in division, modulo, or import-path logic. Add upfront validation for these count fields in normalizeLargeConfig (or immediately after calling it in generateLargeFixture) and reject non-positive values with a clear error before any fixture generation starts.rslib.config.ts-23-28 (1)
23-28: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winEmit the route transform worker from the CJS build too.
createWorkerUrl()always resolves./parallel-route-transform-worker.js, but the CJSsource.entryblock doesn’t include that file. CJS consumers won’t get the worker asset, so parallel transforms will fall back to the serial path.Proposed fix
source: { entry: { index: './src/index.ts', + 'parallel-route-transform-worker': + './src/parallel-route-transform-worker.ts', 'templates/entry.server': './src/templates/entry.server.tsx', 'templates/entry.client': './src/templates/entry.client.tsx', },🤖 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 `@rslib.config.ts` around lines 23 - 28, The CJS source.entry map is missing the route transform worker entry, so CJS builds won’t emit the asset that createWorkerUrl() resolves. Update the source.entry block in rslib.config.ts to include the parallel route transform worker alongside the existing index, templates/entry.server, and templates/entry.client entries, so CJS consumers also receive parallel-route-transform-worker.js.src/route-export-pruning.ts-544-549 (1)
544-549: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t track sourced re-export names as local bindings.
For
export { loader as clientLoader } from "./x",specifier.local.nameis the imported name from the source module, not a local binding. Adding it toremovedExportLocalNamescan make the later assignment cleanup remove unrelated localloader.* = ...statements.Proposed fix
const exportedName = getExportedName(specifier); if (exportedName && exportsToRemoveSet.has(exportedName)) { exportsChanged = true; - if (specifier.local?.name) { + if (!statement.source && specifier.local?.name) { removedExportLocalNames.add(specifier.local.name); removedExportReferencedNames.add(specifier.local.name); } return false; }🤖 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 `@src/route-export-pruning.ts` around lines 544 - 549, The re-export cleanup in the export pruning flow is incorrectly treating sourced re-export names as local bindings. In the logic around the export specifier handling in route-export-pruning and the sets removedExportLocalNames / removedExportReferencedNames, only add names that are actually declared locally in the file; for `export { loader as clientLoader } from "./x"`, avoid recording `specifier.local.name` as a local binding so later assignment cleanup does not delete unrelated `loader.*` statements. Use the existing export-specifier and assignment-pruning checks to distinguish sourced re-exports from true local exports before adding names to those tracking sets.tests/index.test.ts-106-139 (1)
106-139: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore the previous
existsSyncmock implementation.Line 111 overrides a shared mock, but Line 138 leaves it hardcoded to
true. Capture and restore the previous implementation so this test cannot leak filesystem behavior into later cases.🤖 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/index.test.ts` around lines 106 - 139, Restore the shared fs.existsSync mock in this test by capturing the previous mock implementation before overriding it and reinstating that exact implementation in the finally block. Update the test around createStubRsbuild and pluginReactRouter so the temporary behavior used for the watchFiles assertion does not leak into later tests.src/index.ts-231-239 (1)
231-239: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat
serverOutput: undefinedas omitted.
hasOwnPropertymakes{ serverOutput: undefined }explicit, soresolvedServerOutputcan becomeundefinedand flow into the node Rspack config.Proposed fix
- const hasExplicitServerOutput = Object.prototype.hasOwnProperty.call( - options, - 'serverOutput' - ); - let resolvedServerOutput = pluginOptions.serverOutput; - if (!hasExplicitServerOutput) { - resolvedServerOutput = - serverModuleFormat === 'cjs' ? 'commonjs' : 'module'; - } + const hasExplicitServerOutput = options.serverOutput !== undefined; + const resolvedServerOutput = hasExplicitServerOutput + ? options.serverOutput + : serverModuleFormat === 'cjs' + ? 'commonjs' + : 'module';🤖 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 `@src/index.ts` around lines 231 - 239, Treat serverOutput: undefined the same as an omitted option in the server output resolution logic. Update the hasExplicitServerOutput check in the src/index.ts flow around pluginOptions.serverOutput so that undefined does not count as explicitly set, and keep the fallback based on serverModuleFormat ('cjs' => commonjs, otherwise module) from flowing into the Node Rspack config. Reference the server output resolution branch and the resolvedServerOutput assignment when making the fix.src/dev-generation.ts-551-553 (1)
551-553: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep
load()asynchronous in the ready path.
selectBuild()can throw here, which escapes synchronously instead of rejecting the promised API. That makesloadReactRouterServerBuild()behave differently betweenstartingandreadystates for the same missing-build failure.Proposed fix
if (state.kind === 'ready') { - return Promise.resolve(selectBuild(state.committed, entryName)); + return Promise.resolve().then(() => + selectBuild(state.committed, entryName) + ); }🤖 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 `@src/dev-generation.ts` around lines 551 - 553, The `load()` implementation in `src/dev-generation.ts` should stay asynchronous in the `state.kind === 'ready'` branch, because calling `selectBuild()` directly can throw synchronously instead of returning a rejected promise. Update the ready-path logic around `selectBuild()` and `loadReactRouterServerBuild()` so missing-build failures are handled through promise rejection consistently with the `starting` state.src/typegen.ts-12-19 (1)
12-19: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse the local
react-routerCLI and keep watch failures visible.
npx --yescan fall back to a registry install when the binary is missing; run the app’s installed CLI directly so typegen stays on the pinned version.- The watch process currently swallows every rejection. Only ignore shutdown, or restart/log unexpected exits so typegen doesn’t die silently.
🤖 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 `@src/typegen.ts` around lines 12 - 19, The typegen launcher in typegen.ts should use the project’s locally installed react-router CLI instead of npx --yes, so update the execa invocation to call the pinned binary directly from the app dependencies. In the same typegen/watch flow, avoid swallowing every rejected process exit: adjust the process handling around the execa call so only intentional shutdown is ignored, and unexpected failures from the watch process are surfaced or restarted/logged. Use the existing typegen process setup and its process variable as the place to make these changes.
🤖 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 `@src/build-output-transforms.ts`:
- Around line 123-136: The fallback in build-output-transforms should stay
bundle-scoped even when latestServerManifestsByBundleId is not yet populated. In
the manifest selection logic around getLatestServerManifest,
getLatestServerManifestByBundleId, and getReactRouterManifestForDev, ensure the
cold-path dev manifest returned for
virtual/react-router/server-manifest-${bundleId} is filtered to the current
bundleId before it is used. Keep the per-bundle behavior consistent with the
virtual module wiring from src/index.ts so the fallback never embeds routes from
other bundles.
In `@src/dev-runtime-artifacts.ts`:
- Around line 145-174: The assertBuildMatchesManifest check only verifies routes
present in ReactRouterDevManifest, so stale ServerBuild entries for deleted
routes can slip through. Update assertBuildMatchesManifest to also compare
build.routes against manifest.routes and throw when the evaluated server build
contains any extra route IDs not in the manifest, alongside the existing
missing-route and loader/action export checks. Use the existing entryName,
build, manifest, and routeId logic to keep the validation in one place.
In `@src/dev-runtime-session.ts`:
- Around line 62-69: The completeClose flow in dev-runtime-session skips
closeBinding() when the session is already in the closing state, so successful
server.close() calls never fully clean up the runtime. Update completeClose to
invoke closeBinding(binding) for the successful closing path as well as active,
while still guarding with isCurrentBinding(binding) and preserving the
transition to idle; apply the same fix in the other matching close path around
the later close handler.
In `@src/prerender-build.ts`:
- Around line 175-177: The prerender write path logic in the output path
construction must prevent traversal outside clientBuildDir. Update the path
handling in prerender-build.ts around the outputPath/mkdir/writeFile flow to
validate normalizedPath segments before joining, reject any .. or
backslash-based traversal, and assert the final resolved path stays under
clientBuildDir. Apply the same safeguard to the other prerender output write
sites mentioned by the build flow helpers so all generated files remain confined
to the client build directory.
- Around line 496-498: The cleanup in enqueue path handling creates an
unconsumed promise from task.finally, which can surface prerender failures as
unhandled rejections; update the logic around enqueue, pending, and task so the
cleanup promise is handled explicitly. Use the task promise’s then with both
fulfillment and rejection handlers, or attach a catch to the promise returned by
finally, while still removing the task from pending.
- Around line 549-607: The prerender/build-request flow in prerender-build.ts
leaves process.env.IS_RR_BUILD_REQUEST mutated after the work completes. In the
block around resolveServerBuildModule, createRequestHandler, runPrerenderPaths,
and handleSpaMode, save the previous IS_RR_BUILD_REQUEST value before setting it
and restore it in a finally block after all prerender/Spa handling finishes.
This should be done around the existing build request logic so buildEnd and
subsequent runs/tests see the original process-global state.
- Around line 211-225: Escape the redirect target before interpolating it into
the fallback HTML in the redirect handling block so untrusted `Location` values
cannot inject markup or attributes. Update the logic around
`redirectStatusCodes`, `response.headers.get('Location')`, and the HTML template
in `src/prerender-build.ts` to use a safely escaped/sanitized version for the
`<title>`, `meta refresh`, and `<a href>`/link text. Keep `normalizedPath`
as-is, but ensure every use of the redirect destination is HTML- and
attribute-safe.
In `@src/route-chunks.ts`:
- Around line 211-216: The shared side-effect collection in route chunking is
incorrectly treating directive prologues like "use client" and "use strict" as
blocking statements. Update the filtering logic in the shared side-effect scan
and the related `topLevelNonModuleStatements` handling so `ExpressionStatement`s
that are directive prologues are excluded, while real side-effectful expressions
remain included. Use the existing `sharedTopLevelSideEffects` and export
analysis paths to ensure `splitRouteModules: "enforce"` only rejects genuinely
shared side effects.
In `@src/route-export-resolution.ts`:
- Around line 160-182: Update resolvePackageExports and related callers so
subpath imports are handled correctly: when packageJson.exports is absent, do
not always fall back to module/main for every packageSubpath, and make sure
pkg/subpath is not resolved as the package entry. In the exports branch, enhance
the lookup logic in resolvePackageExports to support non-exact export shapes
such as pattern mappings like "./*" instead of only direct packageSubpath keys.
Then adjust resolveExportAllModule so a null result from resolvePackageExports
is treated as “not matched yet” and it can still fall back to
createRequire(...).resolve() before rejecting the import.
In `@src/typegen.ts`:
- Around line 22-30: The `typegen --watch` promise handling in `typegenProcess`
is swallowing every rejection, which hides unexpected watcher exits. Update the
`.catch` path so it only ignores the intentional shutdown case from server
teardown, and for any other rejection in `src/typegen.ts` surface the failure by
logging or rethrowing it. Use the existing `typegenProcess` cleanup in
`.finally()` to keep the process reset behavior intact while making crashes or
invalid config failures visible.
In `@src/virtual-modules.ts`:
- Around line 11-19: The virtual module ID validation in the module path builder
does not block backslash separators, so a Windows-style traversal pattern can
slip through. Update the validation in the relativeId/segments check to reject
any segment containing backslashes before returning the node_modules path,
keeping the existing dot-segment and empty-segment checks in place.
---
Minor comments:
In `@examples/custom-node-server/scripts/smoke-production.mjs`:
- Around line 64-70: The teardown in smoke-production.mjs waits on once(child,
'exit') only after sending SIGTERM, which can miss an exit that already happened
and force the full timeout. Update the child process handling around the
existing child.kill and Promise.race flow by creating the exit promise before
signaling, and guard the wait with child.exitCode so you skip the timeout path
when the process has already exited.
In `@README.md`:
- Around line 107-112: The README documentation for parallelTransforms is
contradictory about its default value. Update the public API docs so the JSDoc
on parallelTransforms and the defaults section describe the same behavior, using
one consistent default representation in both places. Refer to
parallelTransforms in the options docs and the defaults section, and make sure
the wording matches whether worker execution is enabled implicitly or via an
explicit true default.
- Line 527: The startup example in startServer() currently only logs failures
with console.error and leaves the process running, which conflicts with the
documented requirement for unrecoverable startup failures. Update the startup
handling so the catch path for startServer() both logs the error and terminates
the process with a non-zero exit code, keeping the example from continuing in a
broken state.
In `@rslib.config.ts`:
- Around line 23-28: The CJS source.entry map is missing the route transform
worker entry, so CJS builds won’t emit the asset that createWorkerUrl()
resolves. Update the source.entry block in rslib.config.ts to include the
parallel route transform worker alongside the existing index,
templates/entry.server, and templates/entry.client entries, so CJS consumers
also receive parallel-route-transform-worker.js.
In `@scripts/benchmark/fixture.mjs`:
- Around line 648-658: The largeConfig override path in
generateLargeFixture/normalizeLargeConfig accepts invalid zero values, which
later break generators that use workers, localeFiles, cssModules, svgAssets,
utilitiesPerRoute, and payloadEntriesPerComponent in division, modulo, or
import-path logic. Add upfront validation for these count fields in
normalizeLargeConfig (or immediately after calling it in generateLargeFixture)
and reject non-positive values with a clear error before any fixture generation
starts.
In `@src/dev-generation.ts`:
- Around line 551-553: The `load()` implementation in `src/dev-generation.ts`
should stay asynchronous in the `state.kind === 'ready'` branch, because calling
`selectBuild()` directly can throw synchronously instead of returning a rejected
promise. Update the ready-path logic around `selectBuild()` and
`loadReactRouterServerBuild()` so missing-build failures are handled through
promise rejection consistently with the `starting` state.
In `@src/index.ts`:
- Around line 231-239: Treat serverOutput: undefined the same as an omitted
option in the server output resolution logic. Update the hasExplicitServerOutput
check in the src/index.ts flow around pluginOptions.serverOutput so that
undefined does not count as explicitly set, and keep the fallback based on
serverModuleFormat ('cjs' => commonjs, otherwise module) from flowing into the
Node Rspack config. Reference the server output resolution branch and the
resolvedServerOutput assignment when making the fix.
In `@src/route-export-pruning.ts`:
- Around line 544-549: The re-export cleanup in the export pruning flow is
incorrectly treating sourced re-export names as local bindings. In the logic
around the export specifier handling in route-export-pruning and the sets
removedExportLocalNames / removedExportReferencedNames, only add names that are
actually declared locally in the file; for `export { loader as clientLoader }
from "./x"`, avoid recording `specifier.local.name` as a local binding so later
assignment cleanup does not delete unrelated `loader.*` statements. Use the
existing export-specifier and assignment-pruning checks to distinguish sourced
re-exports from true local exports before adding names to those tracking sets.
In `@src/typegen.ts`:
- Around line 12-19: The typegen launcher in typegen.ts should use the project’s
locally installed react-router CLI instead of npx --yes, so update the execa
invocation to call the pinned binary directly from the app dependencies. In the
same typegen/watch flow, avoid swallowing every rejected process exit: adjust
the process handling around the execa call so only intentional shutdown is
ignored, and unexpected failures from the watch process are surfaced or
restarted/logged. Use the existing typegen process setup and its process
variable as the place to make these changes.
In `@tests/index.test.ts`:
- Around line 106-139: Restore the shared fs.existsSync mock in this test by
capturing the previous mock implementation before overriding it and reinstating
that exact implementation in the finally block. Update the test around
createStubRsbuild and pluginReactRouter so the temporary behavior used for the
watchFiles assertion does not leak into later tests.
🪄 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: 5108dcda-f5f7-41c8-9159-04e0bd83d220
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (124)
.changeset/bright-routes-run.md.changeset/coherent-dev-generations.md.changeset/lazy-entry-hydration.md.changeset/quiet-topology-order.md.changeset/sharp-routes-heal.md.changeset/warm-watchers-rest.mdREADME.mdbenchmarks/README.mdconfig/package.jsonconfig/rslib.config.tsdocs/coherent-dev-generations.mdexamples/client-only/package.jsonexamples/cloudflare/package.jsonexamples/custom-node-server/app/load-context.d.tsexamples/custom-node-server/app/routes/projects/edit.tsxexamples/custom-node-server/app/routes/projects/index.tsxexamples/custom-node-server/app/routes/projects/layout.tsxexamples/custom-node-server/app/routes/projects/project.tsxexamples/custom-node-server/app/routes/projects/settings.tsxexamples/custom-node-server/package.jsonexamples/custom-node-server/playwright.config.tsexamples/custom-node-server/rsbuild.config.tsexamples/custom-node-server/scripts/smoke-production.mjsexamples/custom-node-server/server.jsexamples/custom-node-server/server/index.tsexamples/custom-node-server/tsconfig.jsonexamples/default-template/app/dev-routes.tsexamples/default-template/app/routes.tsexamples/default-template/package.jsonexamples/default-template/playwright.config.tsexamples/default-template/rsbuild.config.tsexamples/default-template/tests/e2e/dev-route-watch.test.tsexamples/default-template/tests/e2e/lazy-compilation.test.tsexamples/epic-stack/package.jsonexamples/federation/epic-stack-remote/package.jsonexamples/federation/epic-stack/package.jsonexamples/prerender/package.jsonexamples/spa-mode/package.jsonpackage.jsonrslib.config.tsscripts/bench-builds.mjsscripts/benchmark/fixture.mjsscripts/compare-benchmarks.mjsscripts/test-package-interop.mjssrc/babel.tssrc/bounded-cache.tssrc/build-manifest.tssrc/build-output-transforms.tssrc/concurrency.tssrc/constants.tssrc/dev-generation.tssrc/dev-runtime-artifacts.tssrc/dev-runtime-compilation.tssrc/dev-runtime-controller.tssrc/dev-runtime-session.tssrc/dev-server.tssrc/export-utils.tssrc/index.tssrc/lazy-compilation.tssrc/manifest.tssrc/modify-browser-manifest.tssrc/parallel-route-transform-protocol.tssrc/parallel-route-transform-worker.tssrc/parallel-route-transforms.tssrc/performance.tssrc/plugin-utils.tssrc/prerender-build.tssrc/prerender.tssrc/react-router-config.tssrc/route-artifacts.tssrc/route-ast.tssrc/route-chunks.tssrc/route-component-transform.tssrc/route-export-pruning.tssrc/route-export-resolution.tssrc/route-transform-tasks.tssrc/route-watch.tssrc/server-build-plan.tssrc/server-utils.tssrc/templates/entry.server.tsxsrc/typegen.tssrc/types.tssrc/virtual-modules.tssrc/warnings/warn-on-client-source-maps.tssrc/yuku.tstests/benchmark-fixture.test.tstests/bounded-cache.test.tstests/build-manifest.test.tstests/build-output-transforms.test.tstests/client-modules.test.tstests/dev-generation-multi-entry.test.tstests/dev-generation.test.tstests/dev-runtime-controller.test.tstests/dev-runtime.integration.test.tstests/dev-server.test.tstests/export-utils.test.tstests/features.test.tstests/fixtures/dev-runtime/app/entry.client.tsxtests/fixtures/dev-runtime/app/entry.server.tsxtests/fixtures/dev-runtime/app/root.tsxtests/fixtures/dev-runtime/app/routes.tstests/fixtures/dev-runtime/app/routes/index.tsxtests/fixtures/dev-runtime/app/routes/other.tsxtests/fixtures/dev-runtime/react-router.config.tstests/fixtures/dev-runtime/server/index.tstests/index.test.tstests/lazy-compilation.test.tstests/manifest-split-route-modules.test.tstests/manifest.test.tstests/modify-browser-manifest.test.tstests/parallel-route-transforms.test.tstests/performance.test.tstests/plugin-utils.test.tstests/prerender.test.tstests/react-router-config.test.tstests/remove-exports.test.tstests/route-artifacts.test.tstests/route-chunks-cache.test.tstests/route-chunks.test.tstests/route-watch.test.tstests/server-build-plan.test.tstests/server-utils.test.tstests/setup.tstests/warn-on-client-source-maps.test.ts
💤 Files with no reviewable changes (2)
- examples/custom-node-server/server/index.ts
- src/babel.ts
🛑 Comments failed to post (11)
src/build-output-transforms.ts (1)
123-136: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Keep bundle-specific server-manifest fallbacks bundle-scoped.
When this transform handles
virtual/react-router/server-manifest-${bundleId}beforestageLatestManifests()has populatedlatestServerManifestsByBundleId, Lines 123-136 fall back togetReactRouterManifestForDev(routes, ...), which regenerates the full manifest. Insrc/index.ts, those virtual modules are wired into per-bundle server builds and later filtered by bundle, so a node-first dev compile can embed routes outside the current bundle until a rebuild corrects it. Please filter the cold-path fallback bybundleIdtoo, not just the cached path.Possible direction
- const manifest = + const baseManifest = (latestServerManifest ? ((bundleId && getLatestServerManifestByBundleId(bundleId)) ?? latestServerManifest) : null) ?? (await getReactRouterManifestForDev( routes, pluginOptions, getClientStats(), appDirectory, getAssetPrefix(), routeChunkOptions )); + const manifest = + bundleId && routesByServerBundleId[bundleId] + ? { + ...baseManifest, + routes: Object.fromEntries( + Object.entries(baseManifest.routes).filter(([routeId]) => + routeId in routesByServerBundleId[bundleId] + ) + ), + } + : baseManifest;🤖 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 `@src/build-output-transforms.ts` around lines 123 - 136, The fallback in build-output-transforms should stay bundle-scoped even when latestServerManifestsByBundleId is not yet populated. In the manifest selection logic around getLatestServerManifest, getLatestServerManifestByBundleId, and getReactRouterManifestForDev, ensure the cold-path dev manifest returned for virtual/react-router/server-manifest-${bundleId} is filtered to the current bundleId before it is used. Keep the per-bundle behavior consistent with the virtual module wiring from src/index.ts so the fallback never embeds routes from other bundles.src/dev-runtime-artifacts.ts (1)
145-174: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Also reject server builds with extra route IDs.
This check only walks
manifest.routes. If the node build is stale and still contains a deleted route, validation passes and the mixed pair gets pinned anyway. Route removals are one of the mismatch cases this flow needs to catch.Suggested fix
const assertBuildMatchesManifest = ( entryName: string, build: ServerBuild, manifest: ReactRouterDevManifest ): void => { + const manifestRouteIds = new Set(Object.keys(manifest.routes)); + for (const routeId of Object.keys(build.routes)) { + if (!manifestRouteIds.has(routeId)) { + throw new Error( + `[rsbuild-plugin-react-router] Server build ${JSON.stringify(entryName)} route ${JSON.stringify(routeId)} does not exist in the matching web manifest.` + ); + } + } + for (const [routeId, manifestRoute] of Object.entries(manifest.routes)) { const routeModule = build.routes[routeId]?.module;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const assertBuildMatchesManifest = ( entryName: string, build: ServerBuild, manifest: ReactRouterDevManifest ): void => { const manifestRouteIds = new Set(Object.keys(manifest.routes)); for (const routeId of Object.keys(build.routes)) { if (!manifestRouteIds.has(routeId)) { throw new Error( `[rsbuild-plugin-react-router] Server build ${JSON.stringify(entryName)} route ${JSON.stringify(routeId)} does not exist in the matching web manifest.` ); } } for (const [routeId, manifestRoute] of Object.entries(manifest.routes)) { const routeModule = build.routes[routeId]?.module; if (!routeModule) { throw new Error( `[rsbuild-plugin-react-router] Server build ${JSON.stringify(entryName)} route ${JSON.stringify(routeId)} is missing from the evaluated server build.` ); } if ( Boolean(manifestRoute.hasLoader) !== (typeof routeModule.loader === 'function') ) { throw new Error( `[rsbuild-plugin-react-router] Server build ${JSON.stringify(entryName)} route ${JSON.stringify(routeId)} loader export does not match its web manifest.` ); } if ( Boolean(manifestRoute.hasAction) !== (typeof routeModule.action === 'function') ) { throw new Error( `[rsbuild-plugin-react-router] Server build ${JSON.stringify(entryName)} route ${JSON.stringify(routeId)} action export does not match its web manifest.` ); } } };🤖 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 `@src/dev-runtime-artifacts.ts` around lines 145 - 174, The assertBuildMatchesManifest check only verifies routes present in ReactRouterDevManifest, so stale ServerBuild entries for deleted routes can slip through. Update assertBuildMatchesManifest to also compare build.routes against manifest.routes and throw when the evaluated server build contains any extra route IDs not in the manifest, alongside the existing missing-route and loader/action export checks. Use the existing entryName, build, manifest, and routeId logic to keep the validation in one place.src/dev-runtime-session.ts (1)
62-69: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Run
closeBinding()after successful closes from theclosingstate.Once
markClosing()switches the session toclosing, Line 66 stops invokingcloseBinding(). A normalserver.close()then moves toidlewithoutruntime.close(), unregistering the dev runtime, or clearing compiler identity state.Suggested fix
const completeClose = (binding: RuntimeBinding): void => { if (!isCurrentBinding(binding)) { return; } - if (state.status === 'active') { + if (state.status === 'active' || state.status === 'closing') { closeBinding(binding); } state = { status: 'idle' }; };Also applies to: 170-173
🤖 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 `@src/dev-runtime-session.ts` around lines 62 - 69, The completeClose flow in dev-runtime-session skips closeBinding() when the session is already in the closing state, so successful server.close() calls never fully clean up the runtime. Update completeClose to invoke closeBinding(binding) for the successful closing path as well as active, while still guarding with isCurrentBinding(binding) and preserving the transition to idle; apply the same fix in the other matching close path around the later close handler.src/prerender-build.ts (4)
175-177: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep prerender output paths inside
clientBuildDir.
normalizedPath.split('/')can include..or backslash segments from configured/generated prerender paths, letting dynamic/splat route paths write outside the client build directory. Validate segments and assert the resolved path remains underclientBuildDir.🛡️ Proposed fix
const createDataRequestPath = ( prerenderPath: string, trailingSlashAwareDataRequests: boolean ): string => { @@ : `${prerenderPath.replace(/\/$/, '')}.data`; }; + +const getClientBuildOutputPath = ( + clientBuildDir: string, + normalizedPath: string, + ...extraSegments: string[] +): string => { + const segments = normalizedPath.split('/').filter(Boolean); + if ( + segments.some( + segment => + segment === '.' || segment === '..' || segment.includes('\\') + ) + ) { + throw new Error(`Invalid prerender output path: ${normalizedPath}`); + } + + const outputPath = resolve(clientBuildDir, ...segments, ...extraSegments); + const relativeOutputPath = relative(clientBuildDir, outputPath); + if (relativeOutputPath === '..' || relativeOutputPath.startsWith('../')) { + throw new Error(`Invalid prerender output path: ${normalizedPath}`); + } + return outputPath; +}; @@ - const outputPath = resolve(clientBuildDir, ...normalizedPath.split('/')); + const outputPath = getClientBuildOutputPath(clientBuildDir, normalizedPath); @@ - const outputPath = resolve( - clientBuildDir, - ...normalizedPath.split('/'), - 'index.html' - ); + const outputPath = getClientBuildOutputPath( + clientBuildDir, + normalizedPath, + 'index.html' + ); @@ - const outputPath = resolve(clientBuildDir, ...normalizedPath.split('/')); + const outputPath = getClientBuildOutputPath(clientBuildDir, normalizedPath);Also applies to: 234-240, 284-286
🤖 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 `@src/prerender-build.ts` around lines 175 - 177, The prerender write path logic in the output path construction must prevent traversal outside clientBuildDir. Update the path handling in prerender-build.ts around the outputPath/mkdir/writeFile flow to validate normalizedPath segments before joining, reject any .. or backslash-based traversal, and assert the final resolved path stays under clientBuildDir. Apply the same safeguard to the other prerender output write sites mentioned by the build flow helpers so all generated files remain confined to the client build directory.
211-225: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape redirect
Locationbefore writing fallback HTML.The redirect page writes
Locationdirectly into text and attributes. A route redirect with<,", or similar characters can generate static HTML with script/markup injection.🛡️ Proposed fix
+const escapeHtml = (value: string): string => + value.replace(/[&<>"']/g, char => { + switch (char) { + case '&': + return '&'; + case '<': + return '<'; + case '>': + return '>'; + case '"': + return '"'; + default: + return '&`#39`;'; + } + }); + @@ if (redirectStatusCodes.has(response.status)) { const location = response.headers.get('Location'); + if (!location) { + throw new Error( + `Prerender (html): Redirect response for \`${normalizedPath}\` is missing a Location header.` + ); + } + const escapedLocation = escapeHtml(location); const delay = response.status === 302 ? 2 : 0; html = `<!doctype html> <head> -<title>Redirecting to: ${location}</title> -<meta http-equiv="refresh" content="${delay};url=${location}"> +<title>Redirecting to: ${escapedLocation}</title> +<meta http-equiv="refresh" content="${delay};url=${escapedLocation}"> <meta name="robots" content="noindex"> </head> <body> -\t<a href="${location}"> - Redirecting from <code>${normalizedPath}</code> to <code>${location}</code> +\t<a href="${escapedLocation}"> + Redirecting from <code>${escapeHtml(normalizedPath)}</code> to <code>${escapedLocation}</code> </a> </body> </html>`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const escapeHtml = (value: string): string => value.replace(/[&<>"']/g, char => { switch (char) { case '&': return '&'; case '<': return '<'; case '>': return '>'; case '"': return '"'; default: return '&`#39`;'; } }); if (redirectStatusCodes.has(response.status)) { const location = response.headers.get('Location'); if (!location) { throw new Error( `Prerender (html): Redirect response for \`${normalizedPath}\` is missing a Location header.` ); } const escapedLocation = escapeHtml(location); const delay = response.status === 302 ? 2 : 0; html = `<!doctype html> <head> <title>Redirecting to: ${escapedLocation}</title> <meta http-equiv="refresh" content="${delay};url=${escapedLocation}"> <meta name="robots" content="noindex"> </head> <body> \t<a href="${escapedLocation}"> Redirecting from <code>${escapeHtml(normalizedPath)}</code> to <code>${escapedLocation}</code> </a> </body> </html>`;🤖 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 `@src/prerender-build.ts` around lines 211 - 225, Escape the redirect target before interpolating it into the fallback HTML in the redirect handling block so untrusted `Location` values cannot inject markup or attributes. Update the logic around `redirectStatusCodes`, `response.headers.get('Location')`, and the HTML template in `src/prerender-build.ts` to use a safely escaped/sanitized version for the `<title>`, `meta refresh`, and `<a href>`/link text. Keep `normalizedPath` as-is, but ensure every use of the redirect destination is HTML- and attribute-safe.
496-498: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Consume the promise returned by the cleanup handler.
task.finally(...)creates a second promise that rejects whentaskrejects; because it is ignored, a prerender failure can also surface as an unhandled rejection. Usethenwith both handlers or catch the returned promise.🛠️ Proposed fix
const task = enqueue(path); pending.add(task); - task.finally(() => pending.delete(task)); + void task.then( + () => pending.delete(task), + () => pending.delete(task) + );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const task = enqueue(path); pending.add(task); void task.then( () => pending.delete(task), () => pending.delete(task) );🤖 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 `@src/prerender-build.ts` around lines 496 - 498, The cleanup in enqueue path handling creates an unconsumed promise from task.finally, which can surface prerender failures as unhandled rejections; update the logic around enqueue, pending, and task so the cleanup promise is handled explicitly. Use the task promise’s then with both fulfillment and rejection handlers, or attach a catch to the promise returned by finally, while still removing the task from pending.
549-607: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restore
IS_RR_BUILD_REQUESTafter prerendering.This mutates process-global state and leaves it set for
buildEnd, later builds, or tests in the same process. Preserve and restore the previous value in afinallyblock around the build-request work.🛠️ Proposed fix
if (!ssr || isPrerenderEnabled) { + const previousBuildRequest = process.env.IS_RR_BUILD_REQUEST; process.env.IS_RR_BUILD_REQUEST = 'yes'; - const buildModule = await import(pathToFileURL(serverBuildPath).toString()); - const build = (await resolveServerBuildModule( - buildModule, - `Server build ${JSON.stringify(serverBuildPath)}` - )) as PrerenderServerBuild; - const requestHandler = createRequestHandler(build, 'production'); + try { + const buildModule = await import(pathToFileURL(serverBuildPath).toString()); + const build = (await resolveServerBuildModule( + buildModule, + `Server build ${JSON.stringify(serverBuildPath)}` + )) as PrerenderServerBuild; + const requestHandler = createRequestHandler(build, 'production'); - if (isPrerenderEnabled) { + if (isPrerenderEnabled) { // existing prerender logic - } + } - if (!ssr) { + if (!ssr) { await handleSpaMode({ handler: requestHandler, build, clientBuildDir, basename, api, }); + } + } finally { + if (previousBuildRequest === undefined) { + delete process.env.IS_RR_BUILD_REQUEST; + } else { + process.env.IS_RR_BUILD_REQUEST = previousBuildRequest; } } }📝 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.if (!ssr || isPrerenderEnabled) { const previousBuildRequest = process.env.IS_RR_BUILD_REQUEST; process.env.IS_RR_BUILD_REQUEST = 'yes'; try { const buildModule = await import(pathToFileURL(serverBuildPath).toString()); const build = (await resolveServerBuildModule( buildModule, `Server build ${JSON.stringify(serverBuildPath)}` )) as PrerenderServerBuild; const requestHandler = createRequestHandler(build, 'production'); if (isPrerenderEnabled) { if (!ssr) { const generated = latestBrowserManifest ? { manifest: latestBrowserManifest, moduleExportsByRouteId: latestBrowserManifestModuleExports, } : await generateReactRouterManifestForDev( routes, pluginOptions, clientStats, appDirectory, assetPrefix, routeChunkOptions ); assertValidSsrFalsePrerenderExports({ routes, manifestRoutes: generated.manifest.routes, routeExports: generated.moduleExportsByRouteId, prerenderPaths, api, }); } validatePrerenderPathMatches(routes, prerenderPaths); if (prerenderPaths.length > 0) { api.logger.info( `Prerender (html): ${prerenderPaths.length} path(s)...` ); } await runPrerenderPaths({ build, requestHandler, clientBuildDir, options, }); } if (!ssr) { await handleSpaMode({ handler: requestHandler, build, clientBuildDir, basename, api, }); } } finally { if (previousBuildRequest === undefined) { delete process.env.IS_RR_BUILD_REQUEST; } else { process.env.IS_RR_BUILD_REQUEST = previousBuildRequest; } } }🤖 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 `@src/prerender-build.ts` around lines 549 - 607, The prerender/build-request flow in prerender-build.ts leaves process.env.IS_RR_BUILD_REQUEST mutated after the work completes. In the block around resolveServerBuildModule, createRequestHandler, runPrerenderPaths, and handleSpaMode, save the previous IS_RR_BUILD_REQUEST value before setting it and restore it in a finally block after all prerender/Spa handling finishes. This should be done around the existing build request logic so buildEnd and subsequent runs/tests see the original process-global state.src/route-chunks.ts (1)
211-216: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exclude directive prologues from shared side-effect blocking.
"use client"/"use strict"directives areExpressionStatements, so they are added to every export’stopLevelNonModuleStatements. That makes the intersection check reject otherwise split-safe exports, and can failsplitRouteModules: "enforce"for routes that only share a directive.Proposed fix
- const sharedTopLevelSideEffects = (module.ast as AnyNode).body.filter( + const sharedTopLevelSideEffects = (module.ast as AnyNode).body.filter( (statement: AnyNode) => (statement.type === 'ImportDeclaration' && statement.specifiers.length === 0) || statement.type === 'ExpressionStatement' );for (const statement of sharedTopLevelSideEffects) { dependencies.topLevelStatements.add(statement); - dependencies.topLevelNonModuleStatements.add(statement); + if (statement.type !== 'ExpressionStatement' || !statement.directive) { + dependencies.topLevelNonModuleStatements.add(statement); + } }Also applies to: 329-332
🤖 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 `@src/route-chunks.ts` around lines 211 - 216, The shared side-effect collection in route chunking is incorrectly treating directive prologues like "use client" and "use strict" as blocking statements. Update the filtering logic in the shared side-effect scan and the related `topLevelNonModuleStatements` handling so `ExpressionStatement`s that are directive prologues are excluded, while real side-effectful expressions remain included. Use the existing `sharedTopLevelSideEffects` and export analysis paths to ensure `splitRouteModules: "enforce"` only rejects genuinely shared side effects.src/route-export-resolution.ts (1)
160-182: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the file and inspect the relevant regions with line numbers. ast-grep outline src/route-export-resolution.ts --view expanded || true wc -l src/route-export-resolution.ts sed -n '1,280p' src/route-export-resolution.ts | cat -n # Locate the specific functions and their call sites. rg -n "resolvePackageExports|resolveExportAllModule|collectClientOnlyStubExportNames|PACKAGE_RESOLUTION_NOT_APPLICABLE|createRequire|packageSubpath" src/route-export-resolution.ts src -SRepository: rstackjs/rsbuild-plugin-react-router
Length of output: 14053
Package subpath resolution misses common
exportscases.
resolvePackageExports()ignorespackageSubpathwhen"exports"is absent, sopkg/subpathresolves to the package entry instead of the submodule. The"exports"branch also only does exact-key lookups, so pattern exports like"./*"returnnull, andresolveExportAllModule()treats that as final instead of falling back tocreateRequire(...).resolve(). This can produce the wrong stub surface or reject valid imports.🤖 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 `@src/route-export-resolution.ts` around lines 160 - 182, Update resolvePackageExports and related callers so subpath imports are handled correctly: when packageJson.exports is absent, do not always fall back to module/main for every packageSubpath, and make sure pkg/subpath is not resolved as the package entry. In the exports branch, enhance the lookup logic in resolvePackageExports to support non-exact export shapes such as pattern mappings like "./*" instead of only direct packageSubpath keys. Then adjust resolveExportAllModule so a null result from resolvePackageExports is treated as “not matched yet” and it can still fall back to createRequire(...).resolve() before rejecting the import.src/typegen.ts (1)
22-30: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don't swallow unexpected
typegen --watchexits.This catch path ignores every rejection. If the watcher exits because the CLI crashes or the route config is temporarily invalid, typegen is dead for the rest of the dev session and nothing reports that state. Only suppress the intentional shutdown path; unexpected exits should be logged or failed loudly.
🤖 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 `@src/typegen.ts` around lines 22 - 30, The `typegen --watch` promise handling in `typegenProcess` is swallowing every rejection, which hides unexpected watcher exits. Update the `.catch` path so it only ignores the intentional shutdown case from server teardown, and for any other rejection in `src/typegen.ts` surface the failure by logging or rethrowing it. Use the existing `typegenProcess` cleanup in `.finally()` to keep the process reset behavior intact while making crashes or invalid config failures visible.src/virtual-modules.ts (1)
11-19: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject backslash path separators in virtual module ids.
The current traversal check only splits on
/, sovirtual/react-router/foo\..\barpasses validation but can be interpreted as traversal on Windows-style paths. Reject\inside segments before constructing thenode_modules/...path.🛡️ Proposed fix
if ( !relativeId || - segments.some(segment => !segment || segment === '.' || segment === '..') + segments.some( + segment => + !segment || + segment === '.' || + segment === '..' || + segment.includes('\\') + ) ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const segments = relativeId.split('/'); if ( !relativeId || segments.some( segment => !segment || segment === '.' || segment === '..' || segment.includes('\\') ) ) { throw new Error(`Invalid virtual module id: ${moduleId}`); } return `node_modules/${moduleId}.js`;🤖 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 `@src/virtual-modules.ts` around lines 11 - 19, The virtual module ID validation in the module path builder does not block backslash separators, so a Windows-style traversal pattern can slip through. Update the validation in the relativeId/segments check to reject any segment containing backslashes before returning the node_modules path, keeping the existing dot-segment and empty-segment checks in place.
ScriptedAlchemy
force-pushed
the
codex/parallel-dev-compilers
branch
from
6ad99ea to
e16d85f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ScriptedAlchemy
force-pushed
the
codex/parallel-dev-compilers
branch
from
dff4b2a to
559bb2e
Compare
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 `@scripts/bench-builds.mjs`:
- Around line 256-283: The benchmark timing in the child process lifecycle is
measuring shutdown latency instead of pure readiness time. Update the
spawn/finish flow in the benchmark helper so the readiness timestamp is captured
when the server becomes ready (around the ready-marking logic) and have the
resolved result use that recorded start-to-ready duration, while keeping
finish() only for cleanup/settlement. Use the existing child spawn, ready, and
finish symbols to wire this in without including SIGTERM/SIGKILL teardown time
in wallMs.
In `@scripts/report-benchmark-ci.mjs`:
- Line 124: The benchmark report logic currently picks a single mode via the
report-generation path, which can hide a build-vs-dev mismatch and produce
misleading CI output. Update the comparison/reporting flow in
report-benchmark-ci.mjs so that the mode selection in the benchmark summary
checks both head and base modes explicitly, and reject the report when they
differ instead of defaulting through head.mode/base.mode. Use the existing
benchmark result handling around the mode field in the reporting script to
surface the mismatch before generating the final CI report.
🪄 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: 547b74ae-d610-4bf7-964c-5fc1e235c7a5
📒 Files selected for processing (18)
.github/workflows/benchmark.ymlREADME.mdbenchmarks/README.mdscripts/bench-builds.mjsscripts/benchmark/fixture.mjsscripts/report-benchmark-ci.mjsscripts/test-package-interop.mjssrc/dev-generation.tssrc/dev-runtime-artifacts.tssrc/dev-runtime-compilation.tssrc/dev-runtime-controller.tssrc/index.tssrc/parallel-route-transforms.tssrc/types.tstests/benchmark-fixture.test.tstests/dev-runtime-controller.test.tstests/index.test.tstests/parallel-route-transforms.test.ts
✅ Files skipped from review due to trivial changes (1)
- benchmarks/README.md
🚧 Files skipped from review as they are similar to previous changes (13)
- src/dev-runtime-compilation.ts
- src/types.ts
- scripts/test-package-interop.mjs
- tests/benchmark-fixture.test.ts
- src/dev-generation.ts
- src/dev-runtime-artifacts.ts
- src/dev-runtime-controller.ts
- tests/dev-runtime-controller.test.ts
- tests/index.test.ts
- src/parallel-route-transforms.ts
- scripts/benchmark/fixture.mjs
- README.md
- src/index.ts
ScriptedAlchemy
force-pushed
the
codex/parallel-dev-compilers
branch
from
559bb2e to
0c1f1e5
Compare
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 `@README.md`:
- Around line 447-457: The dev-server sample still closes over currentDevServer
and mounts its middlewares/handler on a shared app, so rerunning startServer()
would keep binding requests to the first server. Update the README example
around createDevServer, startServer, and onRouteTopologyChange to either
explicitly document this as a full-process-restart flow or refactor it to use
replaceable delegates that swap the active handler without stacking stale
middleware, and make sure the text matches the serialized close-then-recreate
behavior described in the surrounding prose.
In `@src/server-utils.ts`:
- Around line 117-124: The route discovery contract is inconsistent:
`resolveServerBuildModule` currently rejects `undefined` even though the
`routeDiscovery` field is treated as optional elsewhere. Update
`isRouteDiscovery` in `server-utils.ts` to accept `undefined` as valid, or make
the `routeDiscovery` requirement consistent across the
`resolveServerBuildModule` validation path and related type checks so local
builds with no route discovery do not throw.
🪄 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: b8c10ff6-a873-490f-a640-aeed1b40dcca
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (125)
.changeset/bright-routes-run.md.changeset/coherent-dev-generations.md.changeset/lazy-entry-hydration.md.changeset/quiet-topology-order.md.changeset/sharp-routes-heal.md.changeset/warm-watchers-rest.mdREADME.mdbenchmarks/README.mdconfig/package.jsonconfig/rslib.config.tsdocs/coherent-dev-generations.mdexamples/client-only/package.jsonexamples/cloudflare/package.jsonexamples/custom-node-server/app/load-context.d.tsexamples/custom-node-server/app/routes/projects/edit.tsxexamples/custom-node-server/app/routes/projects/index.tsxexamples/custom-node-server/app/routes/projects/layout.tsxexamples/custom-node-server/app/routes/projects/project.tsxexamples/custom-node-server/app/routes/projects/settings.tsxexamples/custom-node-server/package.jsonexamples/custom-node-server/playwright.config.tsexamples/custom-node-server/rsbuild.config.tsexamples/custom-node-server/scripts/smoke-production.mjsexamples/custom-node-server/server.jsexamples/custom-node-server/server/index.tsexamples/custom-node-server/tsconfig.jsonexamples/default-template/app/dev-routes.tsexamples/default-template/app/routes.tsexamples/default-template/package.jsonexamples/default-template/playwright.config.tsexamples/default-template/react-router.config.tsexamples/default-template/rsbuild.config.tsexamples/default-template/tests/e2e/dev-route-watch.test.tsexamples/default-template/tests/e2e/lazy-compilation.test.tsexamples/epic-stack/package.jsonexamples/federation/epic-stack-remote/package.jsonexamples/federation/epic-stack/package.jsonexamples/prerender/package.jsonexamples/spa-mode/package.jsonpackage.jsonrslib.config.tsscripts/bench-builds.mjsscripts/benchmark/fixture.mjsscripts/compare-benchmarks.mjsscripts/test-package-interop.mjssrc/babel.tssrc/bounded-cache.tssrc/build-manifest.tssrc/build-output-transforms.tssrc/concurrency.tssrc/constants.tssrc/dev-generation.tssrc/dev-runtime-artifacts.tssrc/dev-runtime-compilation.tssrc/dev-runtime-controller.tssrc/dev-runtime-session.tssrc/dev-server.tssrc/export-utils.tssrc/index.tssrc/lazy-compilation.tssrc/manifest.tssrc/modify-browser-manifest.tssrc/parallel-route-transform-protocol.tssrc/parallel-route-transform-worker.tssrc/parallel-route-transforms.tssrc/performance.tssrc/plugin-utils.tssrc/prerender-build.tssrc/prerender.tssrc/react-router-config.tssrc/route-artifacts.tssrc/route-ast.tssrc/route-chunks.tssrc/route-component-transform.tssrc/route-export-pruning.tssrc/route-export-resolution.tssrc/route-transform-tasks.tssrc/route-watch.tssrc/server-build-plan.tssrc/server-utils.tssrc/templates/entry.server.tsxsrc/typegen.tssrc/types.tssrc/virtual-modules.tssrc/warnings/warn-on-client-source-maps.tssrc/yuku.tstests/benchmark-fixture.test.tstests/bounded-cache.test.tstests/build-manifest.test.tstests/build-output-transforms.test.tstests/client-modules.test.tstests/dev-generation-multi-entry.test.tstests/dev-generation.test.tstests/dev-runtime-controller.test.tstests/dev-runtime.integration.test.tstests/dev-server.test.tstests/export-utils.test.tstests/features.test.tstests/fixtures/dev-runtime/app/entry.client.tsxtests/fixtures/dev-runtime/app/entry.server.tsxtests/fixtures/dev-runtime/app/root.tsxtests/fixtures/dev-runtime/app/routes.tstests/fixtures/dev-runtime/app/routes/index.tsxtests/fixtures/dev-runtime/app/routes/other.tsxtests/fixtures/dev-runtime/react-router.config.tstests/fixtures/dev-runtime/server/index.tstests/index.test.tstests/lazy-compilation.test.tstests/manifest-split-route-modules.test.tstests/manifest.test.tstests/modify-browser-manifest.test.tstests/parallel-route-transforms.test.tstests/performance.test.tstests/plugin-utils.test.tstests/prerender.test.tstests/react-router-config.test.tstests/remove-exports.test.tstests/route-artifacts.test.tstests/route-chunks-cache.test.tstests/route-chunks.test.tstests/route-watch.test.tstests/server-build-plan.test.tstests/server-utils.test.tstests/setup.tstests/warn-on-client-source-maps.test.ts
💤 Files with no reviewable changes (10)
- tests/plugin-utils.test.ts
- tests/react-router-config.test.ts
- src/babel.ts
- tests/manifest-split-route-modules.test.ts
- tests/prerender.test.ts
- tests/performance.test.ts
- examples/custom-node-server/server/index.ts
- tests/modify-browser-manifest.test.ts
- tests/manifest.test.ts
- tests/parallel-route-transforms.test.ts
✅ Files skipped from review due to trivial changes (16)
- tests/fixtures/dev-runtime/app/routes/index.tsx
- examples/custom-node-server/app/routes/projects/index.tsx
- examples/custom-node-server/app/routes/projects/layout.tsx
- config/package.json
- .changeset/warm-watchers-rest.md
- .changeset/bright-routes-run.md
- .changeset/lazy-entry-hydration.md
- .changeset/coherent-dev-generations.md
- .changeset/sharp-routes-heal.md
- src/warnings/warn-on-client-source-maps.ts
- examples/federation/epic-stack-remote/package.json
- .changeset/quiet-topology-order.md
- examples/custom-node-server/app/routes/projects/project.tsx
- benchmarks/README.md
- examples/client-only/package.json
- docs/coherent-dev-generations.md
🚧 Files skipped from review as they are similar to previous changes (83)
- tests/build-manifest.test.ts
- tests/fixtures/dev-runtime/app/root.tsx
- src/parallel-route-transform-protocol.ts
- examples/custom-node-server/tsconfig.json
- tests/fixtures/dev-runtime/react-router.config.ts
- rslib.config.ts
- tests/fixtures/dev-runtime/app/entry.server.tsx
- examples/default-template/app/routes.ts
- tests/fixtures/dev-runtime/app/routes/other.tsx
- tests/fixtures/dev-runtime/app/routes.ts
- examples/custom-node-server/app/load-context.d.ts
- examples/spa-mode/package.json
- tests/fixtures/dev-runtime/app/entry.client.tsx
- src/virtual-modules.ts
- tests/export-utils.test.ts
- examples/prerender/package.json
- examples/custom-node-server/rsbuild.config.ts
- examples/federation/epic-stack/package.json
- examples/custom-node-server/app/routes/projects/settings.tsx
- examples/default-template/app/dev-routes.ts
- examples/custom-node-server/app/routes/projects/edit.tsx
- examples/default-template/tests/e2e/dev-route-watch.test.ts
- src/bounded-cache.ts
- examples/cloudflare/package.json
- src/templates/entry.server.tsx
- examples/default-template/rsbuild.config.ts
- src/concurrency.ts
- src/types.ts
- tests/fixtures/dev-runtime/server/index.ts
- examples/custom-node-server/playwright.config.ts
- src/typegen.ts
- examples/epic-stack/package.json
- src/build-manifest.ts
- examples/default-template/react-router.config.ts
- scripts/compare-benchmarks.mjs
- tests/dev-server.test.ts
- src/lazy-compilation.ts
- examples/default-template/package.json
- src/dev-server.ts
- tests/bounded-cache.test.ts
- src/yuku.ts
- src/constants.ts
- examples/custom-node-server/package.json
- src/parallel-route-transform-worker.ts
- src/dev-runtime-compilation.ts
- tests/dev-generation-multi-entry.test.ts
- src/server-build-plan.ts
- src/route-ast.ts
- examples/custom-node-server/server.js
- src/dev-runtime-session.ts
- src/route-artifacts.ts
- examples/custom-node-server/scripts/smoke-production.mjs
- tests/build-output-transforms.test.ts
- src/performance.ts
- scripts/test-package-interop.mjs
- src/dev-runtime-artifacts.ts
- tests/client-modules.test.ts
- examples/default-template/playwright.config.ts
- src/dev-generation.ts
- tests/lazy-compilation.test.ts
- src/route-export-resolution.ts
- src/route-component-transform.ts
- package.json
- src/react-router-config.ts
- config/rslib.config.ts
- src/prerender.ts
- src/route-export-pruning.ts
- src/export-utils.ts
- tests/index.test.ts
- src/build-output-transforms.ts
- src/route-watch.ts
- src/parallel-route-transforms.ts
- src/route-transform-tasks.ts
- tests/dev-generation.test.ts
- src/plugin-utils.ts
- examples/default-template/tests/e2e/lazy-compilation.test.ts
- src/manifest.ts
- tests/features.test.ts
- src/modify-browser-manifest.ts
- tests/dev-runtime-controller.test.ts
- src/prerender-build.ts
- src/route-chunks.ts
- src/index.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@README.md`:
- Around line 447-457: The dev-server sample still closes over currentDevServer
and mounts its middlewares/handler on a shared app, so rerunning startServer()
would keep binding requests to the first server. Update the README example
around createDevServer, startServer, and onRouteTopologyChange to either
explicitly document this as a full-process-restart flow or refactor it to use
replaceable delegates that swap the active handler without stacking stale
middleware, and make sure the text matches the serialized close-then-recreate
behavior described in the surrounding prose.
In `@src/server-utils.ts`:
- Around line 117-124: The route discovery contract is inconsistent:
`resolveServerBuildModule` currently rejects `undefined` even though the
`routeDiscovery` field is treated as optional elsewhere. Update
`isRouteDiscovery` in `server-utils.ts` to accept `undefined` as valid, or make
the `routeDiscovery` requirement consistent across the
`resolveServerBuildModule` validation path and related type checks so local
builds with no route discovery do not throw.
🪄 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: b8c10ff6-a873-490f-a640-aeed1b40dcca
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (125)
.changeset/bright-routes-run.md.changeset/coherent-dev-generations.md.changeset/lazy-entry-hydration.md.changeset/quiet-topology-order.md.changeset/sharp-routes-heal.md.changeset/warm-watchers-rest.mdREADME.mdbenchmarks/README.mdconfig/package.jsonconfig/rslib.config.tsdocs/coherent-dev-generations.mdexamples/client-only/package.jsonexamples/cloudflare/package.jsonexamples/custom-node-server/app/load-context.d.tsexamples/custom-node-server/app/routes/projects/edit.tsxexamples/custom-node-server/app/routes/projects/index.tsxexamples/custom-node-server/app/routes/projects/layout.tsxexamples/custom-node-server/app/routes/projects/project.tsxexamples/custom-node-server/app/routes/projects/settings.tsxexamples/custom-node-server/package.jsonexamples/custom-node-server/playwright.config.tsexamples/custom-node-server/rsbuild.config.tsexamples/custom-node-server/scripts/smoke-production.mjsexamples/custom-node-server/server.jsexamples/custom-node-server/server/index.tsexamples/custom-node-server/tsconfig.jsonexamples/default-template/app/dev-routes.tsexamples/default-template/app/routes.tsexamples/default-template/package.jsonexamples/default-template/playwright.config.tsexamples/default-template/react-router.config.tsexamples/default-template/rsbuild.config.tsexamples/default-template/tests/e2e/dev-route-watch.test.tsexamples/default-template/tests/e2e/lazy-compilation.test.tsexamples/epic-stack/package.jsonexamples/federation/epic-stack-remote/package.jsonexamples/federation/epic-stack/package.jsonexamples/prerender/package.jsonexamples/spa-mode/package.jsonpackage.jsonrslib.config.tsscripts/bench-builds.mjsscripts/benchmark/fixture.mjsscripts/compare-benchmarks.mjsscripts/test-package-interop.mjssrc/babel.tssrc/bounded-cache.tssrc/build-manifest.tssrc/build-output-transforms.tssrc/concurrency.tssrc/constants.tssrc/dev-generation.tssrc/dev-runtime-artifacts.tssrc/dev-runtime-compilation.tssrc/dev-runtime-controller.tssrc/dev-runtime-session.tssrc/dev-server.tssrc/export-utils.tssrc/index.tssrc/lazy-compilation.tssrc/manifest.tssrc/modify-browser-manifest.tssrc/parallel-route-transform-protocol.tssrc/parallel-route-transform-worker.tssrc/parallel-route-transforms.tssrc/performance.tssrc/plugin-utils.tssrc/prerender-build.tssrc/prerender.tssrc/react-router-config.tssrc/route-artifacts.tssrc/route-ast.tssrc/route-chunks.tssrc/route-component-transform.tssrc/route-export-pruning.tssrc/route-export-resolution.tssrc/route-transform-tasks.tssrc/route-watch.tssrc/server-build-plan.tssrc/server-utils.tssrc/templates/entry.server.tsxsrc/typegen.tssrc/types.tssrc/virtual-modules.tssrc/warnings/warn-on-client-source-maps.tssrc/yuku.tstests/benchmark-fixture.test.tstests/bounded-cache.test.tstests/build-manifest.test.tstests/build-output-transforms.test.tstests/client-modules.test.tstests/dev-generation-multi-entry.test.tstests/dev-generation.test.tstests/dev-runtime-controller.test.tstests/dev-runtime.integration.test.tstests/dev-server.test.tstests/export-utils.test.tstests/features.test.tstests/fixtures/dev-runtime/app/entry.client.tsxtests/fixtures/dev-runtime/app/entry.server.tsxtests/fixtures/dev-runtime/app/root.tsxtests/fixtures/dev-runtime/app/routes.tstests/fixtures/dev-runtime/app/routes/index.tsxtests/fixtures/dev-runtime/app/routes/other.tsxtests/fixtures/dev-runtime/react-router.config.tstests/fixtures/dev-runtime/server/index.tstests/index.test.tstests/lazy-compilation.test.tstests/manifest-split-route-modules.test.tstests/manifest.test.tstests/modify-browser-manifest.test.tstests/parallel-route-transforms.test.tstests/performance.test.tstests/plugin-utils.test.tstests/prerender.test.tstests/react-router-config.test.tstests/remove-exports.test.tstests/route-artifacts.test.tstests/route-chunks-cache.test.tstests/route-chunks.test.tstests/route-watch.test.tstests/server-build-plan.test.tstests/server-utils.test.tstests/setup.tstests/warn-on-client-source-maps.test.ts
💤 Files with no reviewable changes (10)
- tests/plugin-utils.test.ts
- tests/react-router-config.test.ts
- src/babel.ts
- tests/manifest-split-route-modules.test.ts
- tests/prerender.test.ts
- tests/performance.test.ts
- examples/custom-node-server/server/index.ts
- tests/modify-browser-manifest.test.ts
- tests/manifest.test.ts
- tests/parallel-route-transforms.test.ts
✅ Files skipped from review due to trivial changes (16)
- tests/fixtures/dev-runtime/app/routes/index.tsx
- examples/custom-node-server/app/routes/projects/index.tsx
- examples/custom-node-server/app/routes/projects/layout.tsx
- config/package.json
- .changeset/warm-watchers-rest.md
- .changeset/bright-routes-run.md
- .changeset/lazy-entry-hydration.md
- .changeset/coherent-dev-generations.md
- .changeset/sharp-routes-heal.md
- src/warnings/warn-on-client-source-maps.ts
- examples/federation/epic-stack-remote/package.json
- .changeset/quiet-topology-order.md
- examples/custom-node-server/app/routes/projects/project.tsx
- benchmarks/README.md
- examples/client-only/package.json
- docs/coherent-dev-generations.md
🚧 Files skipped from review as they are similar to previous changes (83)
- tests/build-manifest.test.ts
- tests/fixtures/dev-runtime/app/root.tsx
- src/parallel-route-transform-protocol.ts
- examples/custom-node-server/tsconfig.json
- tests/fixtures/dev-runtime/react-router.config.ts
- rslib.config.ts
- tests/fixtures/dev-runtime/app/entry.server.tsx
- examples/default-template/app/routes.ts
- tests/fixtures/dev-runtime/app/routes/other.tsx
- tests/fixtures/dev-runtime/app/routes.ts
- examples/custom-node-server/app/load-context.d.ts
- examples/spa-mode/package.json
- tests/fixtures/dev-runtime/app/entry.client.tsx
- src/virtual-modules.ts
- tests/export-utils.test.ts
- examples/prerender/package.json
- examples/custom-node-server/rsbuild.config.ts
- examples/federation/epic-stack/package.json
- examples/custom-node-server/app/routes/projects/settings.tsx
- examples/default-template/app/dev-routes.ts
- examples/custom-node-server/app/routes/projects/edit.tsx
- examples/default-template/tests/e2e/dev-route-watch.test.ts
- src/bounded-cache.ts
- examples/cloudflare/package.json
- src/templates/entry.server.tsx
- examples/default-template/rsbuild.config.ts
- src/concurrency.ts
- src/types.ts
- tests/fixtures/dev-runtime/server/index.ts
- examples/custom-node-server/playwright.config.ts
- src/typegen.ts
- examples/epic-stack/package.json
- src/build-manifest.ts
- examples/default-template/react-router.config.ts
- scripts/compare-benchmarks.mjs
- tests/dev-server.test.ts
- src/lazy-compilation.ts
- examples/default-template/package.json
- src/dev-server.ts
- tests/bounded-cache.test.ts
- src/yuku.ts
- src/constants.ts
- examples/custom-node-server/package.json
- src/parallel-route-transform-worker.ts
- src/dev-runtime-compilation.ts
- tests/dev-generation-multi-entry.test.ts
- src/server-build-plan.ts
- src/route-ast.ts
- examples/custom-node-server/server.js
- src/dev-runtime-session.ts
- src/route-artifacts.ts
- examples/custom-node-server/scripts/smoke-production.mjs
- tests/build-output-transforms.test.ts
- src/performance.ts
- scripts/test-package-interop.mjs
- src/dev-runtime-artifacts.ts
- tests/client-modules.test.ts
- examples/default-template/playwright.config.ts
- src/dev-generation.ts
- tests/lazy-compilation.test.ts
- src/route-export-resolution.ts
- src/route-component-transform.ts
- package.json
- src/react-router-config.ts
- config/rslib.config.ts
- src/prerender.ts
- src/route-export-pruning.ts
- src/export-utils.ts
- tests/index.test.ts
- src/build-output-transforms.ts
- src/route-watch.ts
- src/parallel-route-transforms.ts
- src/route-transform-tasks.ts
- tests/dev-generation.test.ts
- src/plugin-utils.ts
- examples/default-template/tests/e2e/lazy-compilation.test.ts
- src/manifest.ts
- tests/features.test.ts
- src/modify-browser-manifest.ts
- tests/dev-runtime-controller.test.ts
- src/prerender-build.ts
- src/route-chunks.ts
- src/index.ts
🛑 Comments failed to post (2)
README.md (1)
447-457: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This sample still binds requests to the first dev server.
The prose says
onRouteTopologyChangeshould recreate the dev server, but this example mountscurrentDevServer.middlewaresand the request handler onto a sharedappand closes overcurrentDevServer. Re-runningstartServer()would stack stale middleware/handlers and keep requests on the old server. Please either document this as a full-process-restart flow or rewrite the sample around replaceable delegates.Also applies to: 459-525
🧰 Tools
🪛 LanguageTool
[style] ~450-~450: Consider replacing this word to strengthen your wording.
Context: ...tic API. The callback is a notification and is not awaited, so it can safely start ...(AND_THAT)
🤖 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 `@README.md` around lines 447 - 457, The dev-server sample still closes over currentDevServer and mounts its middlewares/handler on a shared app, so rerunning startServer() would keep binding requests to the first server. Update the README example around createDevServer, startServer, and onRouteTopologyChange to either explicitly document this as a full-process-restart flow or refactor it to use replaceable delegates that swap the active handler without stacking stale middleware, and make sure the text matches the serialized close-then-recreate behavior described in the surrounding prose.src/server-utils.ts (1)
117-124: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Accept
undefinedroute discovery or make the contract required.Lines 23-31 allow
routeDiscoveryto beundefined, but Line 162 rejects that viaisRouteDiscovery, causingresolveServerBuildModuleto throw for an otherwise valid local build shape.Proposed fix
function isRouteDiscovery(value: unknown): boolean { return ( - isRecord(value) && - (value.mode === 'initial' || + value === undefined || + (isRecord(value) && + (value.mode === 'initial' || (value.mode === 'lazy' && (value.manifestPath === undefined || - typeof value.manifestPath === 'string'))) + typeof value.manifestPath === 'string')))) ); }Also applies to: 162-162
🤖 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 `@src/server-utils.ts` around lines 117 - 124, The route discovery contract is inconsistent: `resolveServerBuildModule` currently rejects `undefined` even though the `routeDiscovery` field is treated as optional elsewhere. Update `isRouteDiscovery` in `server-utils.ts` to accept `undefined` as valid, or make the `routeDiscovery` requirement consistent across the `resolveServerBuildModule` validation path and related type checks so local builds with no route discovery do not throw.
ScriptedAlchemy
force-pushed
the
codex/parallel-dev-compilers
branch
from
90ea459 to
91b63d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/dev-generation.ts`:
- Around line 455-490: The reusePreviousNodeBuild logic is too broad because it
can reuse previous node output even when nodeChanged is true, which can skip
evaluateServerBuilds and keep stale server code. Tighten the condition around
reusePreviousNodeBuild in src/dev-generation.ts so it only reuses the prior node
build when the node side did not change, and make shouldEvaluateNode depend on
that corrected state; use the nearby symbols reusePreviousNodeBuild,
shouldEvaluateNode, evaluateServerBuilds, and discardUnsafeOneSidedResult to
update the control flow safely.
In `@src/dev-runtime-controller.ts`:
- Around line 125-143: `finishRuntimeAttempt` currently awaits
`binding.runtime.finishAttempt(...)` without handling rejections, so the
one-sided compile path can escape instead of moving the runtime into a failure
state. Update `finishRuntimeAttempt` to mirror `flushSettledAttempt()` by
catching `finishAttempt` errors and calling `binding.runtime.failAttempt(...)`
with the same `stats`, `changes`, and `identity`, then keep the existing
`retry-node` invalidation behavior for the successful path.
In `@tests/dev-server.test.ts`:
- Around line 26-31: The dev-server test is bypassing the injected listener path
by calling requestHandler directly, so it won’t catch wiring mistakes in
createDevServerMiddleware. Update the stub around createRequestListener and
listener so the captured handler argument is invoked inside the returned
listener callback, and keep the test focused on exercising the lazy
build-provider flow through createDevServerMiddleware.
🪄 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: cf6f8c8e-5b1d-4348-81fa-6ef02744cd05
📒 Files selected for processing (22)
.github/workflows/benchmark.ymlREADME.mdscripts/bench-builds.mjsscripts/benchmark/fixture.mjsscripts/report-benchmark-ci.mjsscripts/test-package-interop.mjssrc/concurrency.tssrc/dev-generation.tssrc/dev-runtime-artifacts.tssrc/dev-runtime-compilation.tssrc/dev-runtime-controller.tssrc/dev-server.tssrc/index.tssrc/parallel-route-transforms.tssrc/server-utils.tssrc/types.tstests/benchmark-fixture.test.tstests/dev-runtime-controller.test.tstests/dev-server.test.tstests/index.test.tstests/parallel-route-transforms.test.tstests/server-utils.test.ts
💤 Files with no reviewable changes (2)
- tests/server-utils.test.ts
- tests/parallel-route-transforms.test.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- scripts/benchmark/fixture.mjs
- src/concurrency.ts
- src/types.ts
- scripts/test-package-interop.mjs
- tests/dev-runtime-controller.test.ts
- src/index.ts
- src/dev-runtime-artifacts.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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 `@src/dev-generation.ts`:
- Around line 455-490: The reusePreviousNodeBuild logic is too broad because it
can reuse previous node output even when nodeChanged is true, which can skip
evaluateServerBuilds and keep stale server code. Tighten the condition around
reusePreviousNodeBuild in src/dev-generation.ts so it only reuses the prior node
build when the node side did not change, and make shouldEvaluateNode depend on
that corrected state; use the nearby symbols reusePreviousNodeBuild,
shouldEvaluateNode, evaluateServerBuilds, and discardUnsafeOneSidedResult to
update the control flow safely.
In `@src/dev-runtime-controller.ts`:
- Around line 125-143: `finishRuntimeAttempt` currently awaits
`binding.runtime.finishAttempt(...)` without handling rejections, so the
one-sided compile path can escape instead of moving the runtime into a failure
state. Update `finishRuntimeAttempt` to mirror `flushSettledAttempt()` by
catching `finishAttempt` errors and calling `binding.runtime.failAttempt(...)`
with the same `stats`, `changes`, and `identity`, then keep the existing
`retry-node` invalidation behavior for the successful path.
In `@tests/dev-server.test.ts`:
- Around line 26-31: The dev-server test is bypassing the injected listener path
by calling requestHandler directly, so it won’t catch wiring mistakes in
createDevServerMiddleware. Update the stub around createRequestListener and
listener so the captured handler argument is invoked inside the returned
listener callback, and keep the test focused on exercising the lazy
build-provider flow through createDevServerMiddleware.
🪄 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: cf6f8c8e-5b1d-4348-81fa-6ef02744cd05
📒 Files selected for processing (22)
.github/workflows/benchmark.ymlREADME.mdscripts/bench-builds.mjsscripts/benchmark/fixture.mjsscripts/report-benchmark-ci.mjsscripts/test-package-interop.mjssrc/concurrency.tssrc/dev-generation.tssrc/dev-runtime-artifacts.tssrc/dev-runtime-compilation.tssrc/dev-runtime-controller.tssrc/dev-server.tssrc/index.tssrc/parallel-route-transforms.tssrc/server-utils.tssrc/types.tstests/benchmark-fixture.test.tstests/dev-runtime-controller.test.tstests/dev-server.test.tstests/index.test.tstests/parallel-route-transforms.test.tstests/server-utils.test.ts
💤 Files with no reviewable changes (2)
- tests/server-utils.test.ts
- tests/parallel-route-transforms.test.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- scripts/benchmark/fixture.mjs
- src/concurrency.ts
- src/types.ts
- scripts/test-package-interop.mjs
- tests/dev-runtime-controller.test.ts
- src/index.ts
- src/dev-runtime-artifacts.ts
🛑 Comments failed to post (3)
src/dev-generation.ts (1)
455-490: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t reuse the previous node build when node also changed.
Line 455 makes CSS-only web-manifest changes reuse the previous node build even when
nodeChangedis true, so lines 488-490 can skipevaluateServerBuildsand commit stale server code for a combined CSS-ownership + node rebuild.Proposed fix
- const reusePreviousNodeBuild = !!previous && cssOnlyWebManifestChange; + const reusePreviousNodeBuild = + !!previous && cssOnlyWebManifestChange && !nodeChanged;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const reusePreviousNodeBuild = !!previous && cssOnlyWebManifestChange && !nodeChanged; const sameCompileAttempt = !!identity.webAttempt && !!identity.nodeAttempt && identity.webAttempt === identity.nodeAttempt; if ( nodeChanged && identity.nodeWeb !== webIdentity && !sameCompileAttempt && !reusePreviousNodeBuild ) { const message = '[rsbuild-plugin-react-router] Discarded web and node results from different compiler cycles and kept the last-good build.'; if (!previous) { return 'retry-node'; } onWarning(message); rejectAttempt(attemptId, new Error(message), false); return 'retry-node'; } const shouldEvaluateNode = nodeChanged && !reusePreviousNodeBuild; if ( previous && webChanged !== shouldEvaluateNode && !cssOnlyWebManifestChange && discardUnsafeOneSidedResult(attemptId, previous, webChanged, changes) ) { return 'ignored'; } try { const buildsByEntryName = shouldEvaluateNode ? await evaluateServerBuilds(server, buildPlan.entryNames) : previous!.buildsByEntryName;🤖 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 `@src/dev-generation.ts` around lines 455 - 490, The reusePreviousNodeBuild logic is too broad because it can reuse previous node output even when nodeChanged is true, which can skip evaluateServerBuilds and keep stale server code. Tighten the condition around reusePreviousNodeBuild in src/dev-generation.ts so it only reuses the prior node build when the node side did not change, and make shouldEvaluateNode depend on that corrected state; use the nearby symbols reusePreviousNodeBuild, shouldEvaluateNode, evaluateServerBuilds, and discardUnsafeOneSidedResult to update the control flow safely.src/dev-runtime-controller.ts (1)
125-143: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle
finishAttempt()rejections on the one-sided compile path.
flushSettledAttempt()already convertsbinding.runtime.finishAttempt()failures intobinding.runtime.failAttempt(), butfinishRuntimeAttempt()does not. When Line 469 takes the one-sided path, a load/evaluation error escapesonAfterDevCompileinstead of transitioning the dev runtime into its failure state.Suggested fix
const finishRuntimeAttempt = async ( binding: RuntimeBinding, pair: DevCompilerPair, stats: Rspack.Stats | Rspack.MultiStats, changes: Parameters<RuntimeBinding['runtime']['finishAttempt']>[1], identity: Parameters<RuntimeBinding['runtime']['finishAttempt']>[2] ): Promise<void> => { - const result = await binding.runtime.finishAttempt( - stats, - changes, - identity - ); - if ( - result === 'retry-node' && - sessions.getActiveBinding()?.id === binding.id - ) { - pair.node.watching?.invalidate(); + try { + const result = await binding.runtime.finishAttempt( + stats, + changes, + identity + ); + if ( + result === 'retry-node' && + sessions.getActiveBinding()?.id === binding.id + ) { + pair.node.watching?.invalidate(); + } + } catch (cause) { + if (sessions.getActiveBinding()?.id === binding.id) { + binding.runtime.failAttempt( + cause instanceof Error ? cause : new Error(String(cause)) + ); + } } };Also applies to: 469-471
🤖 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 `@src/dev-runtime-controller.ts` around lines 125 - 143, `finishRuntimeAttempt` currently awaits `binding.runtime.finishAttempt(...)` without handling rejections, so the one-sided compile path can escape instead of moving the runtime into a failure state. Update `finishRuntimeAttempt` to mirror `flushSettledAttempt()` by catching `finishAttempt` errors and calling `binding.runtime.failAttempt(...)` with the same `stats`, `changes`, and `identity`, then keep the existing `retry-node` invalidation behavior for the successful path.tests/dev-server.test.ts (1)
26-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Drive this test through the injected listener callback.
This stub calls
requestHandlerdirectly, so the test still passes ifcreateDevServerMiddleware()wires the wrong function intocreateRequestListener(). Capture thehandlerargument and invoke that instead so this test actually covers the lazy build-provider path.🤖 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/dev-server.test.ts` around lines 26 - 31, The dev-server test is bypassing the injected listener path by calling requestHandler directly, so it won’t catch wiring mistakes in createDevServerMiddleware. Update the stub around createRequestListener and listener so the captured handler argument is invoked inside the returned listener callback, and keep the test focused on exercising the lazy build-provider flow through createDevServerMiddleware.
Route the remaining ad-hoc error-normalization sites (dev-generation notify handlers, worker startup/postMessage failures, route topology watcher logging) through the shared normalizeEffectError helper, and call getBuildManifest directly instead of re-wrapping getBuildManifestEffect in runPluginEffect.
Cap the parallel route-transform worker-affinity map with the existing bounded-cache helper so it cannot grow unbounded across long dev sessions, and only reschedule lazy-compilation prewarm when the prewarm-relevant asset set actually changes (fingerprint compare in the controller's setManifest) to avoid restarting in-flight prewarm on every recompile.
Restore error-name fidelity in the topology watcher log, refresh route-source worker affinity entries on hit so bounded eviction behaves LRU-like instead of FIFO, and collect prewarm route assets without allocating every route value before the limit check.
Cold-importing effect costs ~450ms, and the worker thread graph pulled it in through two files with trivial usage: test-only Effect wrappers in route-artifacts and an Effect.async callback-to-promise conversion in route-export-resolution. Replace the conversion with a plain Promise and delete the unused wrappers so worker import drops from ~360ms to ~40ms and small production builds stop paying a fixed startup tax.
Cold-importing the effect barrel loads ~100 re-exported modules and costs ~440ms; the plugin only uses Effect, Cause, Duration, Exit, Fiber, Option, and Deferred, which cost ~120ms as direct subpath imports. Converting all 16 source files cuts plugin import time from ~490ms to ~300ms on every build and dev-server start.
The SSR middleware lazily imported react-router (~67ms) and node-fetch-server (~52ms) on the first request, adding ~120ms to the first route load; start that import when the middleware is created and reset on failure so the first request still surfaces errors. Also spawn route-transform workers during plugin setup instead of onAfterCreateCompiler so thread startup overlaps Rsbuild's own compiler creation.
Delete production-unused exports flagged by review: evaluateServerBuilds, performance recordEffect, runBoundedPrerenderTasks, and mapWithConcurrency (its last production caller migrated to Effect.forEach). Tests now call the surviving Effect APIs directly. concurrency.ts and performance.ts no longer import effect at all, trimming the plugin's module graph.
…y guard The controller's reschedule() became production-unused once setManifest started deduping and rescheduling internally, and the hasOwnProperty check on the internal manifest routes object is inconsistent with the plain for-in used elsewhere. Also stop exporting prewarmLazyCompilation, which has no external callers.
A 4-core cpuset sweep (large-355 dev fixture, interleaved vs the merge base) showed the single worker that small machines used to spawn never paid for itself: dev-server ready dropped from 9.09s to 8.57s median when the worker was disabled, while HMR, route requests, and production builds (synthetic-256: 2.38s both ways) were unchanged. Worker startup and per-task IPC only amortize once at least two workers can run, so getDefaultWorkerCount now returns 0 at or below 4 cores instead of capping at 1.
Move effect from dependencies to devDependencies so rslib's autoExternal inlines it into the built output, and stop exporting Effect-typed functions (resolveServerBuildModuleEffect, resolveReactRouterServerBuildEffect, evaluateServerBuildsEffect, generateReactRouterManifestForDevEffect) so no d.ts reachable from the public entry imports 'effect/Effect' — consumers no longer need effect installed for types to resolve. Measured on this machine (load avg ~10-16, alternating runs, 5 each): - ESM import dist/index.js: ~277ms -> ~192ms (median, -85ms) - CJS require dist/index.cjs: ~259ms -> ~177ms (median, -82ms) - dist size: index.js 193k -> 531k, index.cjs 250k -> 844k - synthetic-256-ssr-esm build median: 2.36s external vs 2.31s bundled (within noise under background benchmark load) - rstest 431/431 pass, tsc clean, package-interop passes, clean consumer typecheck (skipLibCheck false) shows no effect errors
Inline a structural RouteConfigEntry in manifest.ts instead of importing the type from @react-router/dev/routes (a devDependency), so the d.ts files reachable from the public entry no longer reference packages that consumers with skipLibCheck: false may not have installed.
Collaborator
Author
Performance investigation summaryEvidence backing the numbers in the PR description, measured locally with base ( Core-count sweep (4 / 8 / 16 / 48 / unlimited cores)Run with the new
Takeaways:
Update: typegen fixes in
|
| Metric | Before c93289d |
After c93289d |
Base |
|---|---|---|---|
| HMR update (large-355) | ~2.55s | 2.21s (matches typegen-disabled reference) | ≈2.2s |
| Production build (synthetic-256) | base faster | 2.02–2.13s (−5%) | 2.14–2.18s |
| Production build (synthetic-1024) | base faster | ~+1% (noise) | — |
| Dev ready | ~8.7–9.1s | unchanged | — |
| Plugin import (ESM) | ~192ms | 181ms (+19ms accepted fixed cost) | 163ms |
This also resolves the +20% HMR rollup benchmark CI reported at a463dca: that was a one-time startup collision (the deferred typegen watch's bootstrap landing in the benchmark's early update window; per-op plugin timings were ~2ms on both sides), not steady-state per-rebuild cost. With the idle-triggered start the collision is gone. Benchmark CI is re-running for c93289d.
Effect import-hygiene progression
Each step measured on this machine; times are plugin import cost unless noted.
| Step | Result |
|---|---|
Effect via barrel import (import ... from "effect") |
barrel alone ~450ms; plugin import ~490ms |
Switch to subpath imports (effect/Effect, etc.) |
plugin import ~490ms → ~300ms |
Remove effect from the route-transform worker module graph |
worker spawn ~360ms → ~52ms |
Bundle effect into dist (moved to devDependencies) |
plugin import ~181ms ESM / ~177ms CJS; consumers no longer install effect |
The bundling step also removed four Effect-typed exports from the public API; the published d.ts is verified clean for strict skipLibCheck: false consumers, including the @react-router/dev/routes structural-type fix.
Remaining fixed cost vs base: ~19ms of import overhead (~181ms vs base ~163ms ESM), accepted.
…trap Start the react-router typegen watch only after a 10s quiet period with no dev compiles (rescheduling the delayed task on every onAfterDevCompile) instead of a fixed 3s after the first compile, and spawn the CLI directly via the @react-router/dev bin instead of the npx bootstrap (npx remains a fallback when resolution fails). The one-shot build typegen also uses the direct spawn. At 4 cores (large-355 dev fixture, interleaved runs): HMR update dropped from 2.44-2.84s (median ~2.55s across 17 iterations) to 2.11-2.24s (median 2.21s), matching the typegen-disabled reference (~2.2s), with dev-server ready unchanged (~8.6-9.1s). Direct spawn also shaves the npx bootstrap from one-shot typegen: 0.83s vs 1.08s wall, 1.16s vs 1.41s CPU at 4 cores, which runs in every production build.
ScriptedAlchemy
changed the title
Collapse registerReactRouterTypegen's positional optionals into an options object, fold the Effect.sync/zipRight ceremony into the tryPluginPromise thunk, document the npx-fallback catch, un-export the internal idle-delay constant, drop the duplicated assetPrefix default on the private manifest Effect function, align the bench dev branch with the build branch's withCpuLimit spread, and widen the idle-defer test's timing margins.
…hines The 4-core CI runner showed synthetic-1024-ssr-esm build wall time at +5-6% vs the merge base (pooled medians 4459ms vs 4229ms over four interleaved rounds) while dev fixtures improved. The small-machine inline fallback from 8fafc8f was tuned on the dev path, where a lone worker never amortized its startup/IPC cost, but at 1024 routes a production build pushes ~6k transform tasks through the loader and two workers beat inline decisively (forced 2-worker A/B: 4080-4109ms). getDefaultWorkerCount now only disables workers on small machines in dev; builds keep the default worker count. Re-measured after the fix: pooled median 4033ms, -4.6% vs base, and synthetic-256 build stays ahead (1978ms vs 2509ms base).
…lookup resolveDirectTypegenCommand re-implemented the resolve-package-json-from-a- directory pattern that ssr-externals already had. Export resolvePackageJson and reuse it; the npx fallback still kicks in whenever @react-router/dev is not resolvable from the app directory.
…round-trip startServerBuildEvaluationEffect wrapped the Promise-based resolveServerBuildModule, which itself runs an Effect, so every dev server build evaluation crossed the Effect/Promise boundary twice. The Effect-based resolution now lives in an internal server-build-resolution module that dev-runtime-artifacts composes directly; server-utils keeps the public Promise wrappers so the published declaration graph stays free of effect types (re-verified by walking dist/index.d.ts imports).
Aligns the last two test files with the subpath-import convention used in src/ so the barrel pattern does not get copied back into runtime code.
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
2 participants
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.
Goal
Consolidate the perf stack into one mergeable PR against
mainso we can evaluate the combined dev-startup, route-transform, and lazy-compilation optimizations together.Folded PRs
This PR now includes the perf work from:
codex/parallel-dev-compilerscodex/parallel-loader-experimentcodex/lazy-route-worker-poolcodex/prewarm-route-workerscodex/lazy-compilation-prewarmcodex/default-lazy-compilation#67 React Router 8 compatibility stays separate because it is compatibility/API surface rather than core perf work.
What changed
webandnodedevelopment compilers to run in parallel when there are enough spare cores (6+), while keeping production compiler ordering explicit. Dev runtime publication stays coherent across parallel compiler attempts, and node compilation retries when a mixed generation is detected.parallelRouteTransformand auto-enables worker transforms for larger route graphs (256+ routes). Worker slots are created lazily, default worker counts are capped at 1-2, and machines with 4 or fewer cores run dev transforms inline instead of spawning workers (production builds keep workers at every machine size — at 1024 routes the ~6k build transform tasks make two workers decisively faster than inline, see the synthetic-1024 note below). Route-source worker affinity is LRU-bounded so long dev sessions cannot grow it unboundedly.onBeforeBuildone-shot) spawn the@react-router/devbin directly viaprocess.execPathinstead ofnpx --yes(npx kept as fallback), cutting ~250ms+ of bootstrap from every production build.unstableLazyCompilationPrewarm); prewarm reschedules are deduped by fingerprinting the prewarm-relevant asset set.effect/Effectetc.) instead of the barrel; keeps the route-transform worker graph free ofeffectimports entirely; and bundleseffectinto dist (moved todevDependencies), so consumers no longer install it. Four Effect-typed exports were removed from the public API, and the published d.ts is verified clean for strictskipLibCheck: falseconsumers (including a@react-router/dev/routesstructural-type fix).--cpu-limitflag to the benchmark harness for core-constrained benchmarking, plus synthetic profiling controls..mtsscripts using the shared Effect script runtime, consolidates the typegen bin lookup onto the ssr-externals package resolver, moves dev server-build resolution onto a direct Effect path (no Effect→Promise→Effect round-trip; public d.ts graph re-verified effect-free), aligns test files with the effect subpath-import convention, and carries forward the currentmainrelease/docs/Node 24 workflow updates.Measured results
Negative deltas mean the head of this branch is faster.
c93289d— matches typegen-disabled reference; base ≈2.2s, regression closedeec7564eec7564; was +5-6% before the fix575a69d(4-core runner)575a69d575a69d575a69dSee the "Performance investigation summary" comment below for the full core-count sweep and the effect import-hygiene progression.
Performance characteristics by machine size
From a base-vs-head interleaved sweep at 4/8/16/48/unlimited cores (measured before
c93289d, which further improved small-machine HMR and builds — see below):eec7564). The sweep showed HMR and build favoring base slightly at this size, butc93289dandeec7564closed both gaps: HMR now matches base (~2.2s) and 4-core builds are ahead on synthetic-256 (−21%) and synthetic-1024 (−4.6%).c93289d, so these are conservative).cpus − 2transform workers (98 on a 100-core box), which head caps at 2; that uncapped spawn is why base degrades at high core counts.Known tradeoffs
a463dca; per-op plugin timings were ~2ms on both sides).c93289dstarts the typegen watch only after a 10s compile-idle period: locally at 4 cores, large-355 HMR went from ~2.55s to 2.21s, matching the typegen-disabled reference and base (≈2.2s). CI at the current head (575a69d) confirms it: HMR rollup is +2.9% with large-355 at +1.8% (1.46s → 1.48s) — within noise. The fast-ready win is retained.a463dca; after effect bundling and the direct-bin typegen spawn, CI at575a69dshowed large-355 at −4.2% and synthetic-48/256 at −4.7% to −18.1%, with synthetic-1024 unsplit the one remaining positive reading (+7.0%).575a69d(four rounds per side, 6 iterations each) reproduced it: pooled medians 4.46s head vs 4.23s base (+5.4%). Root cause: the small-machine inline fallback (8fafc8f) was tuned on the dev path, where a lone worker never amortizes its startup/IPC cost, but it also disabled workers for production builds — and at 1024 routes a build pushes ~6k transform tasks through the loader, where two workers clearly win (forced 2-worker A/B: 4.08-4.11s).eec7564restricts the inline fallback to dev; builds keep the default worker count. Re-measured after the fix: 4.03s pooled median, −4.6% vs base, with synthetic-256 builds also ahead (1.98s vs 2.51s).575a69dit reads build +1.9%/+2.7% and dev +0.9% — effectively noise, and much improved from the +7.6-16.8% readings one commit earlier.Validation
npx tsc --noEmitnpx rstest run- 440/440 tests passingpnpm buildpnpm typecheck:scriptsskipLibCheck: false)prettier --checkon touched files,git diff --checkBenchmarks
Fresh benchmark CI runs on each push to this PR. The last full CI round compared
575a69dagainst base7a55f78(4-core runner): dev total −4.3% overall, dev ready −5.8%, HMR rollup +2.9% (noise; large-355 +1.8%), production builds −4.2% on large-355 and −4.7% to −18.1% on synthetic-48/256. The one positive reading from that round — synthetic-1024 unsplit build at +7.0% — was confirmed real in a local interleaved A/B and fixed byeec7564(build-mode transforms keep workers on small machines; locally now −4.6% vs base). The "Performance investigation summary" comment covers the local core-count sweep showing head winning across the board at 8+ cores.