Add React Router plugin performance optimizations#39
Open
ScriptedAlchemy wants to merge 57 commits into
Open
Conversation
ScriptedAlchemy
changed the title
commit: |
![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: f28c70be5b
ℹ️ 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 pull request replaces the 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugin-utils.ts (1)
15-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle assignment patterns before allowing destructured exports through.
Line 603 uses this as the only guard for destructured exports, but
AssignmentPatternfalls through.export const { loader = fn } = objwould keep a server-only export in the client transform instead of failing closed.Proposed fix
export function validateDestructuredExports( id: AnyNode, exportsToRemove: readonly string[] ): void { + if (id.type === 'Identifier') { + if (exportsToRemove.includes(id.name)) { + throw invalidDestructureError(id.name); + } + return; + } + + if (id.type === 'RestElement') { + validateDestructuredExports(id.argument, exportsToRemove); + return; + } + + if (id.type === 'AssignmentPattern') { + validateDestructuredExports(id.left, exportsToRemove); + return; + } + if (id.type === 'ArrayPattern') { for (const element of id.elements ?? []) { if (!element) { continue; } - if ( - element.type === 'Identifier' && - exportsToRemove.includes(element.name) - ) { - throw invalidDestructureError(element.name); - } - - if ( - element.type === 'RestElement' && - element.argument.type === 'Identifier' && - exportsToRemove.includes(element.argument.name) - ) { - throw invalidDestructureError(element.argument.name); - } - - if (element.type === 'ArrayPattern' || element.type === 'ObjectPattern') { - validateDestructuredExports(element, exportsToRemove); - } + validateDestructuredExports(element, exportsToRemove); } } if (id.type === 'ObjectPattern') { @@ if (property.type === 'Property') { - if ( - property.value.type === 'Identifier' && - exportsToRemove.includes(property.value.name) - ) { - throw invalidDestructureError(property.value.name); - } - - if ( - property.value.type === 'ArrayPattern' || - property.value.type === 'ObjectPattern' - ) { - validateDestructuredExports(property.value, exportsToRemove); - } + validateDestructuredExports(property.value, exportsToRemove); } - if ( - property.type === 'RestElement' && - property.argument.type === 'Identifier' && - exportsToRemove.includes(property.argument.name) - ) { - throw invalidDestructureError(property.argument.name); + if (property.type === 'RestElement') { + validateDestructuredExports(property, exportsToRemove); } } } }Also applies to: 589-604
🤖 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/plugin-utils.ts` around lines 15 - 76, The validateDestructuredExports function does not handle AssignmentPattern nodes, which allows default values in destructured exports to bypass validation. When destructuring with default values like { loader = fn }, the AssignmentPattern case falls through without checking if the identifier should be removed. Add checks for AssignmentPattern in both the ArrayPattern and ObjectPattern iteration sections: for ArrayPattern elements that are AssignmentPattern, validate the argument property if it's an Identifier, and recursively validate if it's a nested pattern; similarly for ObjectPattern properties with AssignmentPattern values, validate the argument and recursively process nested patterns. This ensures server-only exports with default values are properly caught and fail closed.
🧹 Nitpick comments (3)
scripts/bench-builds.mjs (2)
300-327: ⚡ Quick winAdd legend clarifying metric semantics in Markdown output.
The plugin operations table (lines 300-323) displays
TotalandWallcolumns (line 304) but includes no legend explaining which metric accounts for concurrency. Perperformance-timing-semantics-analysis.md, readers need clear guidance on which metric to use for comparisons.Suggested addition before the table:
lines.push( '', `## Plugin Operations Breakdown`, '', '**Metric definitions:**', '- **Total (ms):** Cumulative wall time across all invocations (may overcount overlapping work)', '- **Wall (ms):** Wall-clock interval with overlap deduplication', '- **Max (ms):** Slowest single invocation (always accurate, no double-counting)', '', '| Environment | Operation | Count | Total | Wall | Max | Reports |', ... );🤖 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/bench-builds.mjs` around lines 300 - 327, The markdown table for plugin operations lacks a legend explaining the metrics, making it unclear which metric accounts for concurrency. Before pushing the table header with the markdown column separators in the lines array, add explanatory text that defines what each metric represents: Total should explain it is cumulative wall time across invocations, Wall should explain it deduplicates overlapping work, and Max should explain it is the slowest single invocation. Insert these legend lines into the lines array before the existing table header markdown lines to provide readers with clear guidance on metric semantics.
497-508: 💤 Low valueBenchmark failures are logged but allow process to continue.
Line 497-502 checks for any failed runs (non-zero status) and sets
process.exitCode = 1, but the script continues afterwriteOutputs()at line 492. This means:
- Results file is written even if builds failed
- Process exits with code 1 only after all benchmarks complete
- If
--fail-fastis NOT enabled, a single build failure doesn't stop the loopThis is generally correct (allow all benchmarks to run, report overall status), but may be confusing if a partial failure occurs — the output file will contain failed runs mixed with passed runs without a clear signal in the file itself.
Consider adding a
failedflag to the result JSON so downstream comparison tools can detect and warn on partial failures.🤖 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/bench-builds.mjs` around lines 497 - 508, The benchmark results are written to output regardless of whether failures occurred, making it difficult for downstream tools to detect partial failures. Add a `failed` boolean flag to the benchmark results object (the structure being passed to `writeOutputs()`) that gets set to true when the condition at line 497 detects any runs with non-zero status. This flag should be determined using the same logic that checks if benchmarks.some(benchmark => benchmark.runs.some(run => run.status !== 0)), allowing the output JSON file to include a clear indicator of whether the benchmark run had any failures.src/manifest.ts (1)
223-225: ⚡ Quick winConsider failing the build on route analysis errors in production builds.
When
getRouteModuleAnalysisthrows, the error is logged but the build continues with empty exports. This produces an incorrect manifest wherehasAction,hasLoader, etc., will all befalseregardless of what the route actually exports. In dev mode this may be acceptable for hot-reload resilience, but in build mode (isBuild: true) this could cause silent runtime failures.Consider re-throwing in build mode:
Suggested approach
} catch (error) { console.error(`Failed to analyze route file ${routeFilePath}:`, error); + if (isBuild) { + throw error; + } }🤖 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/manifest.ts` around lines 223 - 225, The catch block for the getRouteModuleAnalysis error logs the error but allows the build to continue, resulting in an incorrect manifest with all route properties set to false. In the catch block where the error is logged, add a check for isBuild mode and re-throw the error if isBuild is true, ensuring the build fails on route analysis errors in production builds while remaining resilient in dev mode. Keep the current logging behavior but add the conditional re-throw after the console.error call.
🤖 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 `@benchmarks/chunk-precompute-methodology.md`:
- Around line 69-71: The fenced code blocks at lines 69 and 341 in the markdown
file lack language tags, triggering the MD040 markdownlint rule. Add the
language tag `text` to both unlabeled code blocks by changing the opening triple
backticks from ``` to ```text for the array example and the metrics table
example to satisfy linting requirements.
- Around line 121-123: The documentation contains a hardcoded machine-specific
absolute path `/home/zack/projects/rsbuild-plugin-react-router` on line 122 that
will not be valid on other developers' machines or environments. Replace this
specific absolute path with a relative reference or generic placeholder that
conveys the same meaning (such as simply referring to "the repo root" or using a
relative path notation like `.`) to make the instructions portable across
different machines and development environments.
In `@package.json`:
- Line 85: Remove the unused rspack-plugin-virtual-module dependency from
package.json by deleting the line "rspack-plugin-virtual-module": "^1.0.1" since
virtual module handling has been migrated to
rspack.experiments.VirtualModulesPlugin. After removing this line from
package.json, also update the lockfile to remove any references to this package
by running the appropriate package manager command (npm install or yarn install
depending on which lockfile is being used).
In `@performance-timing-semantics-analysis.md`:
- Around line 118-122: Add a documentation comment or docstring to
`src/performance.ts` that explains the timing semantics of the performance
profiler fields. The documentation should clarify that totalMs is measured using
wall-clock deltas captured at record() calls and that due to concurrent async
transforms interleaving on the Node.js event loop, totalMs double-counts
overlapping wait time and can exceed the actual serial cost or even
compilerLifecycleMs. Include guidance that totalMs should be treated as an upper
bound, not precise attribution; explain which fields remain accurate under
concurrency (count, maxMs, slowest[], and compilerLifecycleMs); and advise users
to use the interval-union wallMs aggregate instead of totalMs for
concurrency-safe cost attribution across operations. You can paste the
ready-to-paste paragraph provided in the analysis document or adapt it to fit
your code style.
In `@route-analysis-duplication-audit.md`:
- Around line 1-170: The audit document contains stale line number references
that no longer match the current codebase—for example, cache declarations in
export-utils.ts are documented at lines 24–29 but actually exist at lines 49–55,
and getReactRouterManifestForDev is documented at line 110 but is actually at
line 147. Update all line number references throughout the document by
cross-referencing the specified files (export-utils.ts, manifest.ts, index.ts,
modify-browser-manifest.ts, build-manifest.ts) and the named functions and
caches (transformCache, exportNamesCache, routeModuleAnalysisCache,
getReactRouterManifestForDev, getRouteModuleAnalysis,
detectRouteChunksIfEnabled, etc.) to match their actual current locations.
Additionally, add a header note specifying the current commit hash or date when
the line numbers were verified to prevent future confusion.
In `@src/export-utils.ts`:
- Around line 183-197: The collectExportNames function is missing support for
TypeScript enum declarations, which are runtime exports that should be tracked.
Add a new else if condition after the FunctionDeclaration and ClassDeclaration
check to handle TSEnumDeclaration types. Follow the same pattern as
FunctionDeclaration and ClassDeclaration by checking if the declaration type is
'TSEnumDeclaration' and if declaration.id?.name exists, then add the name to the
exportNames set. This ensures exported enums are properly recorded alongside
other exported declarations.
In `@src/index.ts`:
- Around line 1365-1374: In the bundleId assignment on the line with the replace
method call, the regex pattern is incorrectly escaping the dot character. Change
the regex pattern from /\\.js$/ to /\.js$/ so that it correctly matches and
removes the actual `.js` suffix from the captured bundle ID string. This ensures
that when the module loader appends `.js` to virtual module requests, the
bundleId value is properly stripped (e.g., from `admin.js` to `admin`) so that
subsequent lookups in latestServerManifestsByBundleId work correctly.
In `@src/plugin-utils.ts`:
- Around line 784-829: The component wrapping logic in this export processing
block has gaps that skip certain valid component exports. First, remove the
condition checking expr.type !== 'ClassDeclaration' in the
ExportDefaultDeclaration branch (around line 791) so that default class exports
are also wrapped with the withComponentProps HOC. Second, add handling for
ExportNamedDeclaration statements that contain ExportSpecifier elements (such as
export { ErrorBoundary } or export { ErrorBoundary as Component }) to wrap those
component exports with the appropriate HOC. Third, extend the
FunctionDeclaration handling in the ExportNamedDeclaration branch to also handle
ClassDeclaration exports in the same way, ensuring that named class exports are
wrapped with the with*Props HOC just like function exports.
- Around line 550-566: The export removal logic in the loop that processes
program.body statements only handles ExportNamedDeclaration but ignores
ExportAllDeclaration (export * from '...'). This allows export * statements to
remain in the bundle even when they should be removed. Add a check for
statement.type === 'ExportAllDeclaration' alongside the existing
ExportNamedDeclaration check, and remove the entire ExportAllDeclaration
statement when its source module contains exports that are marked for removal in
the exportsToRemove set. This ensures export * statements don't leak server-only
exports into the web bundle.
---
Outside diff comments:
In `@src/plugin-utils.ts`:
- Around line 15-76: The validateDestructuredExports function does not handle
AssignmentPattern nodes, which allows default values in destructured exports to
bypass validation. When destructuring with default values like { loader = fn },
the AssignmentPattern case falls through without checking if the identifier
should be removed. Add checks for AssignmentPattern in both the ArrayPattern and
ObjectPattern iteration sections: for ArrayPattern elements that are
AssignmentPattern, validate the argument property if it's an Identifier, and
recursively validate if it's a nested pattern; similarly for ObjectPattern
properties with AssignmentPattern values, validate the argument and recursively
process nested patterns. This ensures server-only exports with default values
are properly caught and fail closed.
---
Nitpick comments:
In `@scripts/bench-builds.mjs`:
- Around line 300-327: The markdown table for plugin operations lacks a legend
explaining the metrics, making it unclear which metric accounts for concurrency.
Before pushing the table header with the markdown column separators in the lines
array, add explanatory text that defines what each metric represents: Total
should explain it is cumulative wall time across invocations, Wall should
explain it deduplicates overlapping work, and Max should explain it is the
slowest single invocation. Insert these legend lines into the lines array before
the existing table header markdown lines to provide readers with clear guidance
on metric semantics.
- Around line 497-508: The benchmark results are written to output regardless of
whether failures occurred, making it difficult for downstream tools to detect
partial failures. Add a `failed` boolean flag to the benchmark results object
(the structure being passed to `writeOutputs()`) that gets set to true when the
condition at line 497 detects any runs with non-zero status. This flag should be
determined using the same logic that checks if benchmarks.some(benchmark =>
benchmark.runs.some(run => run.status !== 0)), allowing the output JSON file to
include a clear indicator of whether the benchmark run had any failures.
In `@src/manifest.ts`:
- Around line 223-225: The catch block for the getRouteModuleAnalysis error logs
the error but allows the build to continue, resulting in an incorrect manifest
with all route properties set to false. In the catch block where the error is
logged, add a check for isBuild mode and re-throw the error if isBuild is true,
ensuring the build fails on route analysis errors in production builds while
remaining resilient in dev mode. Keep the current logging behavior but add the
conditional re-throw after the console.error call.
🪄 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: d22b41ff-52e3-4adc-ab3f-0b8c033cc04d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (46)
.changeset/bright-routes-run.md.changeset/fast-routes-dance.md.gitignorebenchmarks/README.mdbenchmarks/chunk-precompute-methodology.mdbenchmarks/manifest-performance-methodology.mdconfig/rslib.config.tspackage.jsonperformance-timing-semantics-analysis.mdroute-analysis-duplication-audit.mdroute-chunk-parse-traverse-analysis.mdscripts/bench-builds.mjsscripts/bench-client-entry-analysis.mjsscripts/benchmark-yuku.mjsscripts/benchmark/fixture.mjsscripts/compare-benchmarks.mjsscripts/compare-client-entry-analysis.mjssrc/babel.tssrc/constants.tssrc/export-utils.tssrc/index.tssrc/manifest.tssrc/modify-browser-manifest.tssrc/performance.tssrc/plugin-utils.tssrc/route-artifacts.tssrc/route-chunks.tssrc/templates/entry.server.tsxsrc/types.tssrc/virtual-modules.tstask/lexer-route-export-triage.mdtask/route-chunk-correctness-test-spec.mdtask/route-chunk-precompute-plan.mdtask/unified-route-module-analysis-cache-triage.mdtests/benchmark-fixture.test.tstests/export-utils.test.tstests/features.test.tstests/index.test.tstests/manifest-split-route-modules.test.tstests/manifest.test.tstests/performance.test.tstests/remove-exports.test.tstests/route-artifacts.test.tstests/route-chunks-cache.test.tstests/route-chunks.test.tstests/setup.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/index.test.ts (1)
35-65: 💤 Low valueConsider adding test for explicit
lazyCompilation: true.The test suite covers the default behavior (implicit
true), explicit object configuration, and explicitfalse, but not an explicitlazyCompilation: true. While the default test already validatestruebehavior, an explicit test would distinguish between "default true" and "user-provided true" for clarity.🧪 Optional test case
it('should respect explicit lazy compilation true', async () => { const rsbuild = await createStubRsbuild({ rsbuildConfig: {}, }); rsbuild.addPlugins([pluginReactRouter({ lazyCompilation: true })]); const config = await rsbuild.unwrapConfig(); expect(config.dev.lazyCompilation).toBe(true); });🤖 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 35 - 65, Add a new test case to the test suite that validates the behavior when lazyCompilation is explicitly set to the boolean value true. Create a test following the same pattern as the existing tests in tests/index.test.ts (using createStubRsbuild, addPlugins with pluginReactRouter, and unwrapConfig), but pass lazyCompilation: true as a boolean to distinguish between the default implicit true behavior and an explicit user-provided true value. Assert that config.dev.lazyCompilation equals true in this case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/index.test.ts`:
- Around line 35-65: Add a new test case to the test suite that validates the
behavior when lazyCompilation is explicitly set to the boolean value true.
Create a test following the same pattern as the existing tests in
tests/index.test.ts (using createStubRsbuild, addPlugins with pluginReactRouter,
and unwrapConfig), but pass lazyCompilation: true as a boolean to distinguish
between the default implicit true behavior and an explicit user-provided true
value. Assert that config.dev.lazyCompilation equals true in this case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 56e43e66-4109-4ba7-a17e-6ec8d1c71821
📒 Files selected for processing (4)
scripts/benchmark/fixture.mjssrc/index.tssrc/types.tstests/index.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/types.ts
- scripts/benchmark/fixture.mjs
- src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/default-template/tests/e2e/dev-route-watch.test.ts (1)
124-131: ⚡ Quick winFail fast if the route insertion anchor is missing.
Line 127 silently leaves
routes.tsunchanged when the anchor comment is absent, then the test fails later via timeout. Add an explicit anchor check so this fails immediately with a clear error.Suggested hardening
const routesConfig = readFileSync(routesConfigPath, 'utf8'); + const anchor = ' // Docs section with nested routes'; + if (!routesConfig.includes(anchor)) { + throw new Error(`Missing expected insertion anchor in ${routesConfigPath}`); + } writeFileSync( routesConfigPath, routesConfig.replace( - ' // Docs section with nested routes', - `${addedRouteConfigEntry}\n\n // Docs section with nested routes` + anchor, + `${addedRouteConfigEntry}\n\n${anchor}` ) );🤖 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/default-template/tests/e2e/dev-route-watch.test.ts` around lines 124 - 131, The code silently continues if the anchor comment is missing from the routes configuration, causing test failures later with confusing timeout errors. Before the writeFileSync call that uses routesConfig.replace(), add an explicit check to verify that the anchor comment string ' // Docs section with nested routes' exists in the routesConfig. If the anchor is not found using an includes() or indexOf() check, throw an error immediately with a clear message indicating the anchor comment is missing from the routes file, so failures are caught and diagnosed right away instead of during test execution.
🤖 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/route-watch.ts`:
- Around line 216-231: The runRescan function checks if closed is true at the
start, but if close() executes while awaiting readRouteDirectoryState (line
221), the function continues and can still call syncDirectoryWatchers and
touchRestartMarker, creating FSWatchers after shutdown. Add a check for the
closed flag immediately after the await completes and before calling
syncDirectoryWatchers to prevent post-close side effects and watcher leaks. This
same pattern should be applied to the other location mentioned at lines 254-263.
---
Nitpick comments:
In `@examples/default-template/tests/e2e/dev-route-watch.test.ts`:
- Around line 124-131: The code silently continues if the anchor comment is
missing from the routes configuration, causing test failures later with
confusing timeout errors. Before the writeFileSync call that uses
routesConfig.replace(), add an explicit check to verify that the anchor comment
string ' // Docs section with nested routes' exists in the routesConfig. If the
anchor is not found using an includes() or indexOf() check, throw an error
immediately with a clear message indicating the anchor comment is missing from
the routes file, so failures are caught and diagnosed right away instead of
during test execution.
🪄 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: bb9ee7a1-447c-449f-8ea4-b512716636b8
📒 Files selected for processing (7)
examples/default-template/playwright.config.tsexamples/default-template/tests/e2e/dev-route-watch.test.tssrc/index.tssrc/route-watch.tstests/index.test.tstests/route-watch.test.tstests/setup.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/index.test.ts
- src/index.ts
|
I've been working on the precedence-aware printer in yuku, and it's been released in the latest yuku So I just had to mention it here when I saw aa33f69. Now |
Contributor
Benchmark ResultsCompared PR head Total median wall time: 16.35s -> 11.00s (-32.7%, 1.49x speedup)
Profile: |
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
src/route-export-pruning.ts (1)
475-488: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winFail closed for unaliased
export *when pruning named exports.
export * from './server'has nostatement.exported, sohasRemovableExportcan returnfalseand the pruning loop never rejects or removes it. That can leave server-only route exports in the web module.Proposed guard
const hasRemovableExport = ( program: AnyNode, exportsToRemove: ReadonlySet<string> ): boolean => { + const removesNamedExports = [...exportsToRemove].some( + name => name !== 'default' + ); for (const statement of program.body ?? []) { if (statement.type === 'ExportAllDeclaration') { const exportedName = statement.exported ? getExportedName({ exported: statement.exported }) : null; if (exportedName && exportsToRemove.has(exportedName)) { return true; } + if (!exportedName && removesNamedExports) { + return true; + } continue; }const removedExportLocalNames = new Set<string>(); const removedExportReferencedNames = new Set<string>(); + const removesNamedExports = exportsToRemove.some(name => name !== 'default'); const trackRemovedExportReferences = (node: AnyNode | null | undefined) => { @@ if (statement.type === 'ExportAllDeclaration') { const exportedName = statement.exported ? getExportedName({ exported: statement.exported }) : null; if (exportedName && exportsToRemoveSet.has(exportedName)) { exportsChanged = true; removeFromArray(program.body, statement); } + if (!exportedName && removesNamedExports) { + throw new Error( + 'Cannot remove named exports from `export *`; use explicit named re-exports.' + ); + } continue; }Also applies to: 535-543, 564-573
🤖 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 475 - 488, The `hasRemovableExport` / pruning flow in `route-export-pruning.ts` should fail closed for unaliased `ExportAllDeclaration` cases. Update the logic around `hasRemovableExport`, `getExportedName`, and the pruning loop so that `export * from './server'` without `statement.exported` is treated as a removable/rejectable export when pruning named exports, instead of being ignored. Make the same guard consistent in the other affected export-pruning paths so unaliased star re-exports cannot slip through into the web module.
🧹 Nitpick comments (4)
src/manifest.ts (1)
265-369: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound route analysis concurrency.
Promise.all(Object.entries(routes).map(...))starts analysis for every route at once. Large apps can spike file I/O, parser memory, and route-chunk work; batching this with a small concurrency cap keeps the optimization predictable.🤖 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/manifest.ts` around lines 265 - 369, The route analysis in manifest generation is launching all route work at once via Promise.all over Object.entries(routes).map, which can overwhelm I/O and memory on large apps. Update the manifest-building flow in the route-entry processing logic to run these async tasks with a small concurrency limit instead of fully parallel execution, while preserving the existing per-route behavior from getRouteModuleAnalysis and detectRouteChunksIfEnabled.tests/build-manifest.test.ts (1)
58-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the ancestor branch too.
getRoutesByServerBundleId()populates each bundle from the full route branch, so this only proves the leaf route is present. Add an assertion for the parent/root route underbundle_2as well; otherwise a regression that drops ancestors would still pass here.🤖 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/build-manifest.test.ts` around lines 58 - 60, The current assertion only verifies the leaf route in getRoutesByServerBundleId(), so it can miss regressions where ancestor routes are dropped. Update the build-manifest test to also assert the parent/root route is present under bundle_2 using the same result object and routes fixture, alongside the existing routes/about check.tests/route-watch.test.ts (1)
10-10: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
resolve()for the expected marker path.This assertion hard-codes a POSIX path, but
getRouteRestartMarkerPath()usesnode:path.resolve, so the test will fail on Windows. Build the expected value with the same path helper.Suggested change
-import { join } from 'node:path'; +import { join, resolve } from 'node:path'; ... expect(getRouteRestartMarkerPath('/project/build/client')).toBe( - '/project/build/client/.react-router/route-watch' + resolve('/project/build/client', '.react-router/route-watch') );Also applies to: 113-117
🤖 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/route-watch.test.ts` at line 10, The test expectation for the route restart marker path is hard-coded with a POSIX-style join, which can break on Windows because `getRouteRestartMarkerPath()` uses `node:path.resolve`. Update the affected assertions in `route-watch.test.ts` to build the expected marker path with `resolve()` instead of `join()`, keeping the test aligned with the implementation and fixing the platform-specific path mismatch.tests/react-router-config.test.ts (1)
34-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the SSR precondition explicit.
This test currently passes because the default config leaves
ssrenabled. Settingssr: truehere keeps the assertion scoped toserverBundlespreservation instead of also depending on an unrelated default.Suggested change
- const result = await resolveReactRouterConfig({ serverBundles }); + const result = await resolveReactRouterConfig({ + ssr: true, + serverBundles, + });🤖 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/react-router-config.test.ts` around lines 34 - 39, The test for resolveReactRouterConfig currently relies on the default SSR setting, so make the SSR precondition explicit in the setup. Update the it('preserves server bundle selection in SSR mode') case to pass ssr: true alongside serverBundles when calling resolveReactRouterConfig, so the assertion only verifies serverBundles preservation and not an implicit default.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/custom-node-server/scripts/smoke-production.mjs`:
- Around line 41-48: The readiness probe in the smoke test can hang because
fetch() has no per-request timeout, even though the loop has an overall
deadline. Update the probe around the fetch call in the smoke-production script
to use a timeout for each attempt so stalled responses fail fast, and keep the
retry loop and outer deadline behavior unchanged.
In `@README.md`:
- Around line 92-99: The README documentation for lazyCompilation is
inconsistent between the option description and the later forwarding section.
Update the docs around lazyCompilation in the main option block and the opt-in
forwarding section so they use the same default/behavior wording, and make sure
the symbols lazyCompilation and the Rsbuild/Rspack option description clearly
state whether the default is false or undefined. Keep the phrasing aligned
across both sections so users see one consistent configuration story.
In `@scripts/test-package-interop.mjs`:
- Around line 48-49: The `starts.find(...).handler` and
`closes.find(...).handler` lookups in `test-package-interop.mjs` can throw
before the test reports a meaningful failure if a `pre` hook is missing. Update
the setup around those hook arrays to first assert that a `pre` hook exists for
both `starts` and `closes`, then dereference the `handler` from the найден hooks
only after those assertions pass, so failures point to missing hook registration
rather than a null access.
In `@src/bounded-cache.ts`:
- Around line 7-13: Handle non-positive maxEntries explicitly in boundedCache so
the cache never stores entries when the limit is 0 or less. Update the eviction
logic around cache.has, cache.size, and cache.set so that maxEntries <= 0
short-circuits without inserting anything, and keep the existing bounded
eviction behavior for positive limits in the same helper.
In `@src/dev-generation.ts`:
- Around line 286-291: The initial incoherent compiler pair in the startup path
can leave pending load() waiters unresolved because the early return happens
before commit() or rejectAttempt(). Update the handling in dev-generation.ts
around the nodeChanged/identity.nodeWeb check so that even when the first
web/node results are mismatched, the current attempt is finalized by either
committing the last-good state or rejecting the attempt before returning. Use
the surrounding logic in load(), commit(), and rejectAttempt() to ensure startup
waiters are always settled.
In `@src/route-component-transform.ts`:
- Around line 332-338: The re-export handling in route component transformation
is skipping default re-exported components because the `isNamedComponentExport`
guard rejects `exportedName === 'default'`. Update the logic around
`getExportedName`, `getModuleExportName`, and the
`withComponentProps`/`withdefaultProps` selection so that both `export {
Component as default }` and `export { default } from './Component'` are
recognized and wrapped with `withComponentProps` instead of falling through to
`withdefaultProps`.
- Around line 406-412: The HOC import insertion in route-component-transform is
currently using program.body.unshift, which can push directive prologues like
"use client" out of the leading position. Update the import placement logic in
the transform so the importDeclaration for virtual/react-router/with-props is
inserted after any leading directive ExpressionStatements in program.body rather
than at the very front. Use the existing hocs handling in
src/route-component-transform.ts to locate the import insertion point and
preserve directive semantics.
In `@src/route-transform-tasks.ts`:
- Around line 186-188: The bare specifier resolution in createClientOnlyStub
should use ESM resolution instead of createRequire(...).resolve(), since the
current path can pick the require condition and miss named exports for packages
with conditional exports. Update the resolver logic in route-transform-tasks so
export * stubs are generated from the ESM entry that matches import semantics,
keeping the existing fallback behavior only if ESM resolution fails.
In `@src/route-watch.ts`:
- Around line 232-240: The synchronous failure path in route topology
notifications can return before state advances, causing repeated reporting of
the same change. In route-watch.ts, update the state assignment in the
onRouteTopologyChange branch of the route topology comparison logic before
invoking the callback, and wrap the callback invocation so synchronous throws
are handled the same way as Promise rejections with onError. Keep the existing
notification boundary behavior in the route change handling around areSetsEqual
and onRouteTopologyChange.
In `@tests/dev-runtime-controller.test.ts`:
- Around line 248-255: The close harness is counting every call to close() even
when it returns the already-memoized closing promise, so only the first close
attempt should increment record.count. Update the close() implementation in the
test helper so the count is increased only when closing is first initialized,
while subsequent calls just return the existing promise; use the existing
close(), closing, and record.count symbols to locate the change.
In `@tests/modify-browser-manifest.test.ts`:
- Line 152: The success-path tests in modify-browser-manifest.test.ts are using
optional emit calls, which can silently no-op and hide broken tapPromise
registration. In the affected test cases, first assert that tapPromise returned
a valid emit function, then invoke emit directly instead of using optional
chaining. Make the same change in both test blocks that currently call
emit?.(...), so the assertions fail if registration breaks.
In `@tests/setup.ts`:
- Around line 13-22: The RR_TEST_ROUTE_COUNT fixture in the test setup is off by
one because pluginReactRouter prepends root before the count is checked, so the
helper currently generates one extra route and weakens the 256-route threshold
coverage. Update the route factory logic in setup.ts so the env var represents
the total number of routes exercised, accounting for root when building the
Array.from result, and keep the route-count tests aligned with the
pluginReactRouter behavior and the routeCount check.
---
Duplicate comments:
In `@src/route-export-pruning.ts`:
- Around line 475-488: The `hasRemovableExport` / pruning flow in
`route-export-pruning.ts` should fail closed for unaliased
`ExportAllDeclaration` cases. Update the logic around `hasRemovableExport`,
`getExportedName`, and the pruning loop so that `export * from './server'`
without `statement.exported` is treated as a removable/rejectable export when
pruning named exports, instead of being ignored. Make the same guard consistent
in the other affected export-pruning paths so unaliased star re-exports cannot
slip through into the web module.
---
Nitpick comments:
In `@src/manifest.ts`:
- Around line 265-369: The route analysis in manifest generation is launching
all route work at once via Promise.all over Object.entries(routes).map, which
can overwhelm I/O and memory on large apps. Update the manifest-building flow in
the route-entry processing logic to run these async tasks with a small
concurrency limit instead of fully parallel execution, while preserving the
existing per-route behavior from getRouteModuleAnalysis and
detectRouteChunksIfEnabled.
In `@tests/build-manifest.test.ts`:
- Around line 58-60: The current assertion only verifies the leaf route in
getRoutesByServerBundleId(), so it can miss regressions where ancestor routes
are dropped. Update the build-manifest test to also assert the parent/root route
is present under bundle_2 using the same result object and routes fixture,
alongside the existing routes/about check.
In `@tests/react-router-config.test.ts`:
- Around line 34-39: The test for resolveReactRouterConfig currently relies on
the default SSR setting, so make the SSR precondition explicit in the setup.
Update the it('preserves server bundle selection in SSR mode') case to pass ssr:
true alongside serverBundles when calling resolveReactRouterConfig, so the
assertion only verifies serverBundles preservation and not an implicit default.
In `@tests/route-watch.test.ts`:
- Line 10: The test expectation for the route restart marker path is hard-coded
with a POSIX-style join, which can break on Windows because
`getRouteRestartMarkerPath()` uses `node:path.resolve`. Update the affected
assertions in `route-watch.test.ts` to build the expected marker path with
`resolve()` instead of `join()`, keeping the test aligned with the
implementation and fixing the platform-specific path mismatch.
🪄 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: 9f4b6d38-ee4a-4898-916d-53c52a631341
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (96)
.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.mddocs/coherent-dev-generations.mdexamples/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.tspackage.jsonrslib.config.tsscripts/bench-builds.mjsscripts/benchmark/fixture.mjsscripts/test-package-interop.mjssrc/bounded-cache.tssrc/build-manifest.tssrc/concurrency.tssrc/dev-generation.tssrc/dev-runtime-artifacts.tssrc/dev-runtime-controller.tssrc/dev-server.tssrc/export-utils.tssrc/index.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.tssrc/react-router-config.tssrc/route-artifacts.tssrc/route-chunks.tssrc/route-component-transform.tssrc/route-export-pruning.tssrc/route-transform-tasks.tssrc/route-watch.tssrc/server-utils.tssrc/types.tssrc/warnings/warn-on-client-source-maps.tssrc/yuku.tstests/benchmark-fixture.test.tstests/bounded-cache.test.tstests/build-manifest.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/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-utils.test.tstests/setup.tstests/warn-on-client-source-maps.test.ts
💤 Files with no reviewable changes (1)
- examples/custom-node-server/server/index.ts
✅ Files skipped from review due to trivial changes (23)
- examples/default-template/app/dev-routes.ts
- examples/custom-node-server/app/routes/projects/index.tsx
- tests/fixtures/dev-runtime/app/routes/other.tsx
- tests/server-utils.test.ts
- .changeset/sharp-routes-heal.md
- tests/fixtures/dev-runtime/app/routes.ts
- .changeset/bright-routes-run.md
- tests/fixtures/dev-runtime/react-router.config.ts
- tests/fixtures/dev-runtime/app/routes/index.tsx
- .changeset/quiet-topology-order.md
- examples/custom-node-server/app/load-context.d.ts
- examples/custom-node-server/app/routes/projects/project.tsx
- .changeset/lazy-entry-hydration.md
- tests/fixtures/dev-runtime/app/entry.server.tsx
- docs/coherent-dev-generations.md
- tests/fixtures/dev-runtime/app/root.tsx
- .changeset/warm-watchers-rest.md
- examples/custom-node-server/app/routes/projects/edit.tsx
- examples/custom-node-server/tsconfig.json
- .changeset/coherent-dev-generations.md
- benchmarks/README.md
- examples/custom-node-server/app/routes/projects/settings.tsx
- scripts/benchmark/fixture.mjs
🚧 Files skipped from review as they are similar to previous changes (12)
- examples/default-template/playwright.config.ts
- src/types.ts
- tests/route-chunks-cache.test.ts
- tests/remove-exports.test.ts
- tests/route-artifacts.test.ts
- tests/features.test.ts
- src/performance.ts
- src/route-artifacts.ts
- scripts/bench-builds.mjs
- src/route-chunks.ts
- tests/route-chunks.test.ts
- src/index.ts
ScriptedAlchemy
force-pushed
the
perf/bundling-performance
branch
from
1bf79cc to
af1f2fb
Compare
Collaborator
Author
|
Added the new Validation on this checkout:
Fresh {
"profile": "large",
"failed": false,
"benchmark": "large-355-ssr-esm",
"wallMs": 6043.8751520000005,
"status": 0,
"fixtureStats": {
"codeModules": 10895,
"dynamicImports": 1065,
"routes": 355,
"components": 6390,
"utilities": 1420,
"lazyModules": 1065,
"workers": 120,
"restrictedModules": 1187,
"svgAssets": 1184,
"cssModules": 217,
"localeFiles": 111,
"localeTotalBytes": 151289856
}
} |
ScriptedAlchemy
force-pushed
the
perf/bundling-performance
branch
2 times, most recently
from
8081eba to
1133507
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ScriptedAlchemy
force-pushed
the
perf/bundling-performance
branch
from
1133507 to
7ab5645
Compare
Remove redundant wrappers and duplicated test setup from the route transform follow-up work without changing behavior.
Trim unused route artifact return data and dedupe server build candidate probing while preserving behavior.
Keep the post-removal CSS reload state armed through unrelated web updates and only fire the follow-up reload when CSS ownership is added back.
ScriptedAlchemy
force-pushed
the
perf/bundling-performance
branch
from
95bc09e to
470c3c8
Compare
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
5 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.
Summary
lazyCompilationplugin option that forwards to Rsbuild's dev-onlydev.lazyCompilationconfig when explicitly set.Performance
Latest local benchmark highlights:
Yuku microbench after consolidation:
Lazy compilation note:
lazyCompilationis dev-only in Rsbuild.Test plan
pnpm test:core tests/index.test.ts tests/remove-exports.test.tspnpm buildCI=true pnpm --filter './examples/client-only' test:e2epnpm testpnpm test tests/index.test.ts tests/benchmark-fixture.test.tspnpm bench --profile default --filter synthetic-256-spa --iterations 5 --warmup 1 --clean build --format both --out .benchmark/results/spa-abort-request-finalREACT_ROUTER_BENCHMARK_LAZY_COMPILATION=1 pnpm bench --profile default --filter synthetic-256-ssr-esm --iterations 5 --warmup 1 --clean build --format both --out .benchmark/results/lazy-on-ssr-esm