fix: restart dev server for route entry changes#43
Merged
ScriptedAlchemy merged 7 commits intoJun 19, 2026
Conversation
![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: aa996c8093
ℹ️ 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".
commit: |
ScriptedAlchemy
force-pushed
the
fix/dev-route-entry-watch
branch
from
344fa75 to
42887da
Compare
ScriptedAlchemy
commented
Jun 19, 2026
ScriptedAlchemy
commented
Jun 19, 2026
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (49)
📝 WalkthroughWalkthroughThis PR migrates 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 docstrings
🧪 Generate unit tests (beta)
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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/plugin-utils.ts (1)
40-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
AssignmentPatternhandling allows invalid removals to slip through.Destructuring patterns with default values (e.g.,
export const { action = defaultAction } = obj) useAssignmentPatternnodes. The current code only checks forIdentifier,RestElement, and nestedArray/ObjectPatterntypes, so patterns like{ action = default }or[action = default]won't trigger the validation error when attempting to removeaction.🐛 Proposed fix to handle AssignmentPattern
if (element.type === 'ArrayPattern' || element.type === 'ObjectPattern') { validateDestructuredExports(element, exportsToRemove); } + + if (element.type === 'AssignmentPattern') { + validateDestructuredExports(element.left, exportsToRemove); + } } } if (id.type === 'ObjectPattern') { for (const property of id.properties ?? []) { if (!property) { continue; } 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); } + + if (property.value.type === 'AssignmentPattern') { + validateDestructuredExports(property.value.left, exportsToRemove); + } }🤖 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 40 - 65, The validateDestructuredExports function does not handle AssignmentPattern nodes, which represent destructuring with default values like { action = defaultValue }. Add a check for when property.value.type equals 'AssignmentPattern' (alongside the existing checks for 'Identifier', 'ArrayPattern', and 'ObjectPattern'). For AssignmentPattern nodes, access the left property which contains the actual identifier or pattern, and validate it the same way you would validate a bare Identifier or recursively validate nested patterns.src/index.ts (2)
1286-1306:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip empty server bundles before generating virtual modules.
Lines 1307-1312 and 1373-1374 already treat
bundleRoutesas possibly empty/falsy, but Lines 1286-1306 still callgenerateServerBuild(bundleRoutes, ...)for every entry. That can crash config generation or emit invalid virtual modules for empty server bundle entries.Proposed fix
const bundleVirtualModules = Object.fromEntries( - Object.entries(routesByServerBundleId).map( - ([bundleId, bundleRoutes]) => [ - `virtual/react-router/server-build-${bundleId}`, - generateServerBuild(bundleRoutes, { - entryServerPath: finalEntryServerPath, - assetsBuildDirectory, - basename, - appDirectory, - ssr, - federation: options.federation, - future, - allowedActionOrigins: allowedActionOriginsForBuild, - prerender: prerenderPaths, - routeDiscovery, - publicPath, - serverManifestId: `virtual/react-router/server-manifest-${bundleId}`, - }), - ] - ) + Object.entries(routesByServerBundleId).flatMap( + ([bundleId, bundleRoutes]) => { + if (!bundleRoutes || Object.keys(bundleRoutes).length === 0) { + return []; + } + return [ + [ + `virtual/react-router/server-build-${bundleId}`, + generateServerBuild(bundleRoutes, { + entryServerPath: finalEntryServerPath, + assetsBuildDirectory, + basename, + appDirectory, + ssr, + federation: options.federation, + future, + allowedActionOrigins: allowedActionOriginsForBuild, + prerender: prerenderPaths, + routeDiscovery, + publicPath, + serverManifestId: `virtual/react-router/server-manifest-${bundleId}`, + }), + ], + ]; + } + ) );🤖 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 1286 - 1306, The bundleVirtualModules assignment in the Object.fromEntries block calls generateServerBuild for every bundleId entry without first checking if bundleRoutes is empty or falsy. Add a filter step to the Object.entries(routesByServerBundleId) chain before the map operation to skip entries where bundleRoutes is empty or falsy, ensuring generateServerBuild is only called for non-empty server bundles. This aligns with how the code already handles potentially empty bundleRoutes in later sections like lines 1307-1312 and 1373-1374.
874-888:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape and validate redirect targets before writing HTML.
Line 875 reads
Locationand Lines 879-885 interpolate it directly into<title>,content, andhref. A redirect target containing quotes or markup can break attributes and create stored XSS in the prerendered redirect page; a missingLocationalso rendersnull.Proposed fix
if (redirectStatusCodes.has(response.status)) { const location = response.headers.get('Location'); + if (!location) { + throw new Error( + `Prerender (html): Received redirect status ${response.status} without a Location header ` + + `while prerendering the \`${normalizedPath}\` path.` + ); + } + const escapedLocation = location.replace(/[&<>"']/g, char => { + switch (char) { + case '&': + return '&'; + case '<': + return '<'; + case '>': + return '>'; + case '"': + return '"'; + case "'": + return '&`#39`;'; + default: + return char; + } + }); 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>${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/index.ts` around lines 874 - 888, The Location header value is being interpolated directly into the HTML template without validation or escaping, which creates an XSS vulnerability and allows null values to appear as "null" strings in the output. First, validate that the Location header exists before proceeding with the redirect HTML generation. Second, properly escape or sanitize the location value retrieved from response.headers.get('Location') before inserting it into the HTML attributes (the meta refresh content attribute and the href attribute) and the title element. Use appropriate escaping functions to prevent both attribute breakout and script injection in the redirect page template.
🧹 Nitpick comments (7)
.changeset/bright-routes-run.md (1)
1-6: ⚡ Quick winConsider consolidating overlapping changesets.
Two changeset files (
bright-routes-run.mdandfast-routes-dance.md) both document patch releases forrsbuild-plugin-react-routerwith overlapping descriptions of route analysis and chunking performance improvements. Having multiple changesets may result in multiple separate releases for the same body of work.Consider consolidating these into a single changeset that comprehensively describes all changes in this PR.
🤖 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 @.changeset/bright-routes-run.md around lines 1 - 6, The changeset files bright-routes-run.md and fast-routes-dance.md both describe patch releases for rsbuild-plugin-react-router with overlapping content about route analysis and chunking performance improvements, which will cause multiple separate releases for the same work. Delete one of these duplicate changeset files and ensure the remaining changeset file comprehensively describes all the changes documented in both files, including both the route analysis improvements and the chunking performance enhancements along with the benchmark tooling additions.benchmarks/manifest-performance-methodology.md (1)
161-169: ⚡ Quick winDrop the hard-coded repo path from the debug command.
Use a repo-relative invocation so the recipe works from any checkout, not just this workstation.
Suggested rewrite
- node /home/zack/projects/rsbuild-plugin-react-router/node_modules/@rsbuild/core/bin/rsbuild.js \ + pnpm exec rsbuild \🤖 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 `@benchmarks/manifest-performance-methodology.md` around lines 161 - 169, The benchmark command contains a hard-coded absolute workstation path that prevents the recipe from working in other checkouts. Replace the hard-coded `/home/zack/projects/rsbuild-plugin-react-router/node_modules/@rsbuild/core/bin/rsbuild.js` path with a repo-relative path (such as `./node_modules/@rsbuild/core/bin/rsbuild.js` or using `npx rsbuild`) so the command works from any checkout regardless of where the repository is cloned.performance-timing-semantics-analysis.md (1)
87-102: ⚡ Quick winReword the
wallMssection to match the current profiler.
src/performance.tsalready computes interval-unionwallMs; the remaining problem istotalMsovercounting under concurrency. Reframe this as a clarification of the existing metrics, not a proposed new metric.Suggested rewrite
-### Recommended: interval-union accounting in `flush()` (Option D) +### Existing: interval-union accounting in `flush()` (Option D)🤖 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 `@performance-timing-semantics-analysis.md` around lines 87 - 102, The section describing "Option D" and `wallMs` presents this as a newly recommended approach, but `src/performance.ts` already implements interval-union accounting for `wallMs`. Reword this section to clarify that `wallMs` (the wall-time union of intervals per operation) is already computed by the existing profiler; the remaining problem to address is that `totalMs` overcounts under concurrency because it sums all resource durations without deduplication. Refocus the explanation on how the current implementation already handles the interval-union deduplication, and frame the concern as clarifying existing metrics rather than proposing a new feature.benchmarks/chunk-precompute-methodology.md (1)
121-123: ⚡ Quick winMake the benchmark commands path-agnostic.
Hard-coding one machine's checkout path makes the recipe fail everywhere else. Resolve the repo root at runtime and invoke Rsbuild through
pnpm execinstead.Suggested rewrite
-`/home/zack/projects/rsbuild-plugin-react-router` +`$(git rev-parse --show-toplevel)` - /home/zack/projects/rsbuild-plugin-react-router/node_modules/@rsbuild/core/bin/rsbuild.js \ + pnpm exec rsbuild \Also applies to: 163-169
🤖 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 `@benchmarks/chunk-precompute-methodology.md` around lines 121 - 123, The benchmark commands contain hard-coded absolute paths like `/home/zack/projects/rsbuild-plugin-react-router` that make them fail when run on other machines or in different directory structures. Replace these hard-coded paths with dynamic path resolution that determines the repo root at runtime (such as using shell variables to capture the current working directory), and update all Rsbuild command invocations to use `pnpm exec` instead of relying on absolute paths. Apply this same change across all occurrences in the file, including the sections at lines 163-169 and other locations where similar hard-coded paths appear.task/lexer-route-export-triage.md (1)
9-13: ⚡ Quick winAdd a clear “historical snapshot” banner to avoid outdated guidance reuse.
Line 9–13 conclusions are tied to
es-module-lexerhot-path assumptions. Since this stack migrated route analysis tooling, add an explicit pre-migration scope note near the top so this doesn’t get applied to current internals.🤖 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 `@task/lexer-route-export-triage.md` around lines 9 - 13, The document contains conclusions about es-module-lexer optimization assumptions (lines 9-13) that are based on the pre-migration architecture of the route analysis tooling. Add an explicit "historical snapshot" or pre-migration scope banner near the top of the document to clearly indicate that the analysis and conclusions in this file reference the codebase state before the route analysis tooling migration, preventing these outdated architectural assumptions from being misapplied to the current implementation.route-analysis-duplication-audit.md (1)
18-20: 🏗️ Heavy liftMark this audit as historical (or refresh it to current Yuku internals).
Line 18 and Line 68 frame the route pipeline as Babel/esbuild/lexer-based. In this PR stack, route analysis moved to
yuku-*; leaving this unqualified can send follow-up perf work toward stale hot paths.Also applies to: 68-70
🤖 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 `@route-analysis-duplication-audit.md` around lines 18 - 20, The audit document describes the route analysis pipeline using outdated Babel/esbuild/lexer-based implementations, but in the current PR stack route analysis has moved to yuku-* packages. Either mark the entire audit as historical with a clear disclaimer, or update the table entries describing src/route-chunks.ts, src/manifest.ts, and src/modify-browser-manifest.ts to reflect that route analysis now uses yuku-* implementations instead of the old hot paths. Ensure the document clarifies which sections are outdated to prevent follow-up work from being directed at stale performance optimization targets.route-chunk-parse-traverse-analysis.md (1)
51-71: 🏗️ Heavy liftUpdate this analysis to the current implementation model.
Line 51 onward centers on
codeToAst+ Babel parse/traverse assumptions, but current route-chunk analysis is analyzer-based with different internals. Please either relabel this as pre-migration analysis or refresh Sections 3–7 to current symbols.Also applies to: 82-93
🤖 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 `@route-chunk-parse-traverse-analysis.md` around lines 51 - 71, The analysis in sections 3 through 7 starting at line 51 describes an outdated implementation centered on codeToAst, Babel parse, and direct AST mutations that no longer apply to the current analyzer-based route-chunk analysis system. Either clearly label these sections as "Pre-Migration Analysis" to mark them as historical context, or refresh the content to describe the current analyzer-based implementation approach by replacing references to the old codeToAst symbol, Babel parse mechanics, and AST mutation patterns with the current symbols and flow used in the analyzer-based model. Ensure the updated analysis accurately reflects what the current implementation actually does.
🤖 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 @.changeset/bright-routes-run.md:
- Line 2: In the file `.changeset/bright-routes-run.md`, update the version bump
type for the "rsbuild-plugin-react-router" entry from "patch" to "minor" to
accurately reflect that this PR introduces new optional plugin options and
functionality rather than just bug fixes. The versioning should align with the
project's semver guidelines where new backward-compatible features require a
minor version bump.
In `@examples/default-template/tests/e2e/dev-route-watch.test.ts`:
- Around line 124-131: In the dev-route-watch.test.ts file, add a validation
check after reading the routesConfig file to ensure the anchor string ' // Docs
section with nested routes' exists in the content before attempting the replace
operation. If the anchor is not found in routesConfig, throw an error with a
clear message indicating that the expected anchor is missing, rather than
silently proceeding with a no-op replacement that would cause confusing timeout
failures later. This validation should occur before the writeFileSync call that
performs the route configuration modification.
- Around line 138-149: The test currently checks that the restart marker is not
null, but this only verifies the marker exists from dev-server startup, not that
a restart occurred after adding the route. To fix this, capture the initial
restart marker value after confirming it exists (after the first expect.poll
call), then after writing the route file and waiting for the text to appear,
assert that the marker has changed to a different value (not just that it
remained stable). This would prove the topology-triggered restart actually
happened in response to adding the route, rather than just confirming the marker
exists from startup.
In `@scripts/bench-builds.mjs`:
- Around line 185-206: The median calculation in the summarizeMetric function
currently picks only the upper middle element for even-length arrays, which
introduces bias. Modify the median calculation by checking whether sorted.length
is even or odd: if the length is even, calculate the median as the average of
the two middle elements (elements at indices sorted.length / 2 - 1 and
sorted.length / 2); if the length is odd, use the current approach of picking
the single middle element at Math.floor(sorted.length / 2). This ensures
accurate median reporting for all array sizes.
- Around line 116-123: The hasGnuTime function currently only checks if the
/usr/bin/time binary exists, but does not verify it supports the GNU-specific -v
flag that is used later in the code. Modify the function to actually execute
/usr/bin/time with the -v flag against a simple test command (such as echo) and
check the stderr output for the "User time (seconds):" string, which is a
reliable indicator that GNU time is available. Return true only if this marker
is found in the output, otherwise return false.
In `@scripts/benchmark-yuku.mjs`:
- Around line 10-12: The iterations and sampleCount variables are converted from
environment variables without validation, which can cause failures if the values
are invalid such as zero, negative, non-integer, or non-numeric. Add validation
checks immediately after the Number() conversions to ensure both iterations and
sampleCount are positive integers, and throw a descriptive error if either value
fails validation.
In `@scripts/benchmark/fixture.mjs`:
- Line 404: The profile rotation at the line using `index %
routeExportProfiles.length` suffers from an off-by-one error because `index` is
1-based while array indexing requires 0-based values. Fix the calculation by
adjusting the index to be 0-based before applying the modulo operation, changing
it to `(index - 1) % routeExportProfiles.length` so that the first profile is
selected correctly and the distribution across all profiles remains even.
In `@scripts/compare-benchmarks.mjs`:
- Line 28: The operations being parsed from the comma-separated `--operations`
argument are not having whitespace trimmed, which causes silent mismatches when
`operationMetric(...)` tries to match them. Modify the operations assignment
where the Set is created to add a `.map()` call after `.split(',')` that trims
each token using `.trim()`, ensuring that operations like "test " (with
whitespace) match correctly against their expected values.
In `@src/index.ts`:
- Around line 236-249: The runRescan function checks the closed flag only before
awaiting readRouteDirectoryState, but if the dev server closes while that await
is pending, the function will continue and call syncDirectoryWatchers and
touchRestartMarker on already-cleared watchers, causing FSWatcher leaks. Add a
re-check of the closed flag immediately after the await completes and before
calling syncDirectoryWatchers and touchRestartMarker to ensure no operations
occur after the server has shut down.
- Around line 1609-1618: The regex pattern in the bundleId assignment uses an
incorrect escape sequence that matches a literal backslash instead of a dot. In
the bundleId variable assignment where bundleMatch?.[1]?.replace() is called,
the regex pattern needs to be corrected from /\\.js$/ to /.js$/ so that it
properly strips the .js file extension from the captured bundle identifier. This
will ensure that bundle IDs like admin.js are correctly transformed to admin and
can properly match entries in the latestServerManifestsByBundleId object instead
of falling back to the full server manifest.
In `@src/manifest.ts`:
- Around line 223-225: The catch block handling route file analysis errors is
silently logging the error and continuing with empty export metadata, which
masks downstream failures in functions like
getRouteManifestModuleExports(manifest). Instead of just logging and continuing
in this catch block, re-throw the error or propagate the failure appropriately
so that manifest creation fails when route analysis fails, preventing invalid
manifests from being created with missing export metadata.
In `@src/route-chunks.ts`:
- Around line 442-447: The filter in the FunctionDeclaration and
ClassDeclaration check is dropping exported dependencies that do not match the
exportName, causing undefined references in the chunk. Instead of returning null
when declaration.id?.name does not equal exportName, preserve these dependent
declarations by either converting them to non-exported declarations (remove the
export keyword) or prevent chunking entirely for this declaration shape. Ensure
that inline exported helpers referenced by clientLoader are retained in the
output chunk regardless of whether their name matches the target exportName.
- Around line 405-418: The filtering logic in the map function only preserves
ImportDeclaration nodes that have identifiers in
dependencies.importedIdentifierNames, which causes side-effect imports (like
import './setup' or CSS imports) to be dropped since they have no local
identifiers. Modify the filter condition before the map to also include
ImportDeclaration nodes that are side-effect imports (those with specifiers that
don't create local bindings), ensuring these imports are preserved in the
generated route chunks even when they're not tracked in
dependencies.topLevelStatements. This requires checking each ImportDeclaration
to determine if it has side-effects and preserving it regardless of whether its
identifiers are in the dependencies tracking.
In `@src/virtual-modules.ts`:
- Around line 10-19: The validation in the module ID check only validates
forward slash-separated segments and does not account for backslash-based path
traversal attacks on Windows systems, allowing bypass attempts using sequences
like `..\\`. Add an additional validation check to reject module IDs containing
backslashes before or in addition to the existing segment validation. This
ensures that path traversal attempts using backslashes are properly caught and
rejected when validating the relativeId or moduleId parameter.
In `@task/route-chunk-correctness-test-spec.md`:
- Line 32: The markdown file contains unlabeled fenced code blocks which violate
markdownlint rule MD040. Locate the fenced code blocks at line 32 and line 325.
For the first fenced block at line 32 that contains the
detectRouteChunksIfEnabled function call, add the language tag "text" to the
opening fence. For the second fenced block at line 325 that contains the export
statements, add the language tag "ts" to the opening fence. Both blocks should
change from unlabeled opening fences (triple backticks with no language
identifier) to labeled opening fences (triple backticks followed by the
appropriate language tag).
In `@task/route-chunk-precompute-plan.md`:
- Around line 33-43: The fenced code block starting at line 33 in the file
task/route-chunk-precompute-plan.md is missing a language identifier, which
triggers markdownlint rule MD040. To fix this, add a language identifier to the
opening fence by changing the opening triple backticks from ``` to ```text,
since the content consists of plain text describing function names and their
behavior. This will satisfy the markdown linting requirement while keeping the
content unchanged.
In `@tests/index.test.ts`:
- Around line 43-48: The regex patterns in the stringMatching assertions use
hardcoded forward slashes which will fail on Windows where paths use
backslashes. Update the regex patterns in both the app routes path assertion and
the route-watch path assertion to use character classes that match both forward
and backward slashes (replacing each \/ with [\\/]) so the patterns work
correctly on both Unix-like and Windows systems.
---
Outside diff comments:
In `@src/index.ts`:
- Around line 1286-1306: The bundleVirtualModules assignment in the
Object.fromEntries block calls generateServerBuild for every bundleId entry
without first checking if bundleRoutes is empty or falsy. Add a filter step to
the Object.entries(routesByServerBundleId) chain before the map operation to
skip entries where bundleRoutes is empty or falsy, ensuring generateServerBuild
is only called for non-empty server bundles. This aligns with how the code
already handles potentially empty bundleRoutes in later sections like lines
1307-1312 and 1373-1374.
- Around line 874-888: The Location header value is being interpolated directly
into the HTML template without validation or escaping, which creates an XSS
vulnerability and allows null values to appear as "null" strings in the output.
First, validate that the Location header exists before proceeding with the
redirect HTML generation. Second, properly escape or sanitize the location value
retrieved from response.headers.get('Location') before inserting it into the
HTML attributes (the meta refresh content attribute and the href attribute) and
the title element. Use appropriate escaping functions to prevent both attribute
breakout and script injection in the redirect page template.
In `@src/plugin-utils.ts`:
- Around line 40-65: The validateDestructuredExports function does not handle
AssignmentPattern nodes, which represent destructuring with default values like
{ action = defaultValue }. Add a check for when property.value.type equals
'AssignmentPattern' (alongside the existing checks for 'Identifier',
'ArrayPattern', and 'ObjectPattern'). For AssignmentPattern nodes, access the
left property which contains the actual identifier or pattern, and validate it
the same way you would validate a bare Identifier or recursively validate nested
patterns.
---
Nitpick comments:
In @.changeset/bright-routes-run.md:
- Around line 1-6: The changeset files bright-routes-run.md and
fast-routes-dance.md both describe patch releases for
rsbuild-plugin-react-router with overlapping content about route analysis and
chunking performance improvements, which will cause multiple separate releases
for the same work. Delete one of these duplicate changeset files and ensure the
remaining changeset file comprehensively describes all the changes documented in
both files, including both the route analysis improvements and the chunking
performance enhancements along with the benchmark tooling additions.
In `@benchmarks/chunk-precompute-methodology.md`:
- Around line 121-123: The benchmark commands contain hard-coded absolute paths
like `/home/zack/projects/rsbuild-plugin-react-router` that make them fail when
run on other machines or in different directory structures. Replace these
hard-coded paths with dynamic path resolution that determines the repo root at
runtime (such as using shell variables to capture the current working
directory), and update all Rsbuild command invocations to use `pnpm exec`
instead of relying on absolute paths. Apply this same change across all
occurrences in the file, including the sections at lines 163-169 and other
locations where similar hard-coded paths appear.
In `@benchmarks/manifest-performance-methodology.md`:
- Around line 161-169: The benchmark command contains a hard-coded absolute
workstation path that prevents the recipe from working in other checkouts.
Replace the hard-coded
`/home/zack/projects/rsbuild-plugin-react-router/node_modules/@rsbuild/core/bin/rsbuild.js`
path with a repo-relative path (such as
`./node_modules/@rsbuild/core/bin/rsbuild.js` or using `npx rsbuild`) so the
command works from any checkout regardless of where the repository is cloned.
In `@performance-timing-semantics-analysis.md`:
- Around line 87-102: The section describing "Option D" and `wallMs` presents
this as a newly recommended approach, but `src/performance.ts` already
implements interval-union accounting for `wallMs`. Reword this section to
clarify that `wallMs` (the wall-time union of intervals per operation) is
already computed by the existing profiler; the remaining problem to address is
that `totalMs` overcounts under concurrency because it sums all resource
durations without deduplication. Refocus the explanation on how the current
implementation already handles the interval-union deduplication, and frame the
concern as clarifying existing metrics rather than proposing a new feature.
In `@route-analysis-duplication-audit.md`:
- Around line 18-20: The audit document describes the route analysis pipeline
using outdated Babel/esbuild/lexer-based implementations, but in the current PR
stack route analysis has moved to yuku-* packages. Either mark the entire audit
as historical with a clear disclaimer, or update the table entries describing
src/route-chunks.ts, src/manifest.ts, and src/modify-browser-manifest.ts to
reflect that route analysis now uses yuku-* implementations instead of the old
hot paths. Ensure the document clarifies which sections are outdated to prevent
follow-up work from being directed at stale performance optimization targets.
In `@route-chunk-parse-traverse-analysis.md`:
- Around line 51-71: The analysis in sections 3 through 7 starting at line 51
describes an outdated implementation centered on codeToAst, Babel parse, and
direct AST mutations that no longer apply to the current analyzer-based
route-chunk analysis system. Either clearly label these sections as
"Pre-Migration Analysis" to mark them as historical context, or refresh the
content to describe the current analyzer-based implementation approach by
replacing references to the old codeToAst symbol, Babel parse mechanics, and AST
mutation patterns with the current symbols and flow used in the analyzer-based
model. Ensure the updated analysis accurately reflects what the current
implementation actually does.
In `@task/lexer-route-export-triage.md`:
- Around line 9-13: The document contains conclusions about es-module-lexer
optimization assumptions (lines 9-13) that are based on the pre-migration
architecture of the route analysis tooling. Add an explicit "historical
snapshot" or pre-migration scope banner near the top of the document to clearly
indicate that the analysis and conclusions in this file reference the codebase
state before the route analysis tooling migration, preventing these outdated
architectural assumptions from being misapplied to the current implementation.
🪄 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: 77113f68-e26d-4f91-9c14-1064d0df8ee7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (49)
.changeset/bright-routes-run.md.changeset/fast-routes-dance.md.gitignorebenchmarks/README.mdbenchmarks/chunk-precompute-methodology.mdbenchmarks/manifest-performance-methodology.mdconfig/rslib.config.tsexamples/default-template/tests/e2e/dev-route-watch.test.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/route-watch.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/route-watch.test.tstests/setup.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.
🛑 Comments failed to post (17)
.changeset/bright-routes-run.md (1)
2-2:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Check the plugin options interface and public API surface to assess the scope of changes # Search for new plugin option definitions echo "=== New plugin options ===" rg -n "lazyCompilation|logPerformance" --type=ts -g '!node_modules' -g '!tests' -C2 # Search for route watch related exports to confirm it's a new feature echo -e "\n=== Route watch feature exports ===" rg -n "createRouteTopologyWatcher|RouteManifestSnapshot" --type=ts -g '!node_modules' -g '!tests' -C2 # Check for breaking changes or API modifications echo -e "\n=== Plugin interface/type definitions ===" ast-grep --pattern 'export interface PluginOptions { $$$ }'Repository: rstackjs/rsbuild-plugin-react-router
Length of output: 3684
🏁 Script executed:
# Search for versioning documentation in the project fd -i 'changelog|CHANGELOG|changelog.md|CHANGELOG.md|contributing|CONTRIBUTING' --type=f | head -20 # Look for any documented versioning strategy rg -n "semver|semantic.versioning|patch|minor|major" -i --type=md --type=json -g '!node_modules' -C2 | head -50 # Check for any existing version bump patterns in changesets find .changeset -type f -name "*.md" | head -10 | xargs cat 2>/dev/null | head -100Repository: rstackjs/rsbuild-plugin-react-router
Length of output: 6763
🏁 Script executed:
# Read the CHANGELOG to understand versioning patterns head -150 CHANGELOG.md # Read CONTRIBUTING to check for versioning guidelines cat CONTRIBUTING.md # Look at a few previous changesets to understand versioning patterns ls -la .changeset/*.md | head -20 cat .changeset/*.md 2>/dev/null | grep -A2 '"rsbuild-plugin-react-router"' | head -40Repository: rstackjs/rsbuild-plugin-react-router
Length of output: 3502
Correct the changeset: new features warrant a minor version bump, not patch.
According to the project's documented versioning guidelines:
- Patch: Bug fixes and small changes that don't affect the public API
- Minor: New features that don't break backward compatibility
This PR introduces two new optional plugin options (
lazyCompilation,logPerformance) and route-watching functionality in dev mode. These are backwards-compatible feature additions. Per semver conventions and the project's own guidelines, this should be a minor version bump.Update
.changeset/bright-routes-run.mdfrompatchtominor.🤖 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 @.changeset/bright-routes-run.md at line 2, In the file `.changeset/bright-routes-run.md`, update the version bump type for the "rsbuild-plugin-react-router" entry from "patch" to "minor" to accurately reflect that this PR introduces new optional plugin options and functionality rather than just bug fixes. The versioning should align with the project's semver guidelines where new backward-compatible features require a minor version bump.examples/default-template/tests/e2e/dev-route-watch.test.ts (2)
124-131:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast if the route-config insertion anchor is missing.
String.prototype.replaceis a silent no-op when the anchor is absent, which turns this into a long timeout/debugging failure instead of an immediate actionable error.💡 Suggested fix
const routesConfig = readFileSync(routesConfigPath, 'utf8'); + const insertionAnchor = ' // Docs section with nested routes'; + if (!routesConfig.includes(insertionAnchor)) { + throw new Error( + `Could not find insertion anchor in ${routesConfigPath}: ${insertionAnchor}` + ); + } writeFileSync( routesConfigPath, routesConfig.replace( - ' // Docs section with nested routes', - `${addedRouteConfigEntry}\n\n // Docs section with nested routes` + insertionAnchor, + `${addedRouteConfigEntry}\n\n${insertionAnchor}` ) );📝 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 routesConfig = readFileSync(routesConfigPath, 'utf8'); const insertionAnchor = ' // Docs section with nested routes'; if (!routesConfig.includes(insertionAnchor)) { throw new Error( `Could not find insertion anchor in ${routesConfigPath}: ${insertionAnchor}` ); } writeFileSync( routesConfigPath, routesConfig.replace( insertionAnchor, `${addedRouteConfigEntry}\n\n${insertionAnchor}` ) );🤖 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, In the dev-route-watch.test.ts file, add a validation check after reading the routesConfig file to ensure the anchor string ' // Docs section with nested routes' exists in the content before attempting the replace operation. If the anchor is not found in routesConfig, throw an error with a clear message indicating that the expected anchor is missing, rather than silently proceeding with a no-op replacement that would cause confusing timeout failures later. This validation should occur before the writeFileSync call that performs the route configuration modification.
138-149:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert marker transition, not just marker existence.
This test currently allows a false positive because the marker is created during dev-server startup;
not.toBe(null)doesn’t prove a topology-triggered restart happened after adding the route.💡 Suggested fix
await page.goto('/'); await expect(page.locator('h1')).toContainText('Welcome to React Router'); + const markerBeforeRouteAdd = readRestartMarker(); writeFileSync( addedRoutePath, @@ await waitForRouteText(page, addedRouteUrl, addedRouteText); + await expect + .poll(readRestartMarker, { timeout: 10000 }) + .not.toBe(markerBeforeRouteAdd); await page.goto(addedRouteUrl); await expect(page.locator('h1')).toHaveText(addedRouteText); - await expect.poll(readRestartMarker, { timeout: 10000 }).not.toBe(null); const restartMarkerBefore = readRestartMarker();🤖 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 138 - 149, The test currently checks that the restart marker is not null, but this only verifies the marker exists from dev-server startup, not that a restart occurred after adding the route. To fix this, capture the initial restart marker value after confirming it exists (after the first expect.poll call), then after writing the route file and waiting for the text to appear, assert that the marker has changed to a different value (not just that it remained stable). This would prove the topology-triggered restart actually happened in response to adding the route, rather than just confirming the marker exists from startup.scripts/bench-builds.mjs (2)
116-123:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify whether /usr/bin/time supports GNU -v output on this machine. if [ -x /usr/bin/time ]; then /usr/bin/time -v node -e "process.exit(0)" >/dev/null 2> /tmp/time_probe.stderr status=$? echo "exit_code=$status" if grep -q "User time (seconds):" /tmp/time_probe.stderr; then echo "gnu_time_fields_present=yes" else echo "gnu_time_fields_present=no" fi else echo "/usr/bin/time not found" fiRepository: rstackjs/rsbuild-plugin-react-router
Length of output: 120
🏁 Script executed:
cat -n scripts/bench-builds.mjs | sed -n '110,140p'Repository: rstackjs/rsbuild-plugin-react-router
Length of output: 862
🏁 Script executed:
head -30 scripts/bench-builds.mjsRepository: rstackjs/rsbuild-plugin-react-router
Length of output: 957
Probe GNU
timecapability, not just binary presence.Line 118 only verifies that
/usr/bin/timeexists. On systems where thetimecommand is non-GNU (e.g., macOS, BSD), the binary exists but does not support the-vflag used at line 134, causing measured runs to fail immediately.The suggested fix probes for actual GNU
timecapability by running/usr/bin/time -vwith a test invocation and checking for the "User time (seconds):" field in stderr output—a reliable marker of GNU time.💡 Suggested fix
const hasGnuTime = async () => { try { await access('/usr/bin/time'); + const probe = await execa( + '/usr/bin/time', + ['-v', process.execPath, '-e', ''], + { reject: false } + ); + return ( + probe.exitCode === 0 && + /User time \(seconds\):/.test(probe.stderr ?? '') + ); } catch { 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 `@scripts/bench-builds.mjs` around lines 116 - 123, The hasGnuTime function currently only checks if the /usr/bin/time binary exists, but does not verify it supports the GNU-specific -v flag that is used later in the code. Modify the function to actually execute /usr/bin/time with the -v flag against a simple test command (such as echo) and check the stderr output for the "User time (seconds):" string, which is a reliable indicator that GNU time is available. Return true only if this marker is found in the output, otherwise return false.
185-206:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix median calculation for even-length run sets.
Line 201 currently picks one middle element. For even run counts, the median should be the average of the two middle values; otherwise reported medians are biased.
💡 Suggested fix
const summarizeMetric = values => { const sorted = values .filter(value => typeof value === 'number') .sort((a, b) => a - b); if (sorted.length === 0) { return { min: null, median: null, mean: null, p95: null, stdev: null }; } + const mid = Math.floor(sorted.length / 2); + const median = + sorted.length % 2 === 0 + ? (sorted[mid - 1] + sorted[mid]) / 2 + : sorted[mid]; const mean = sorted.reduce((sum, value) => sum + value, 0) / sorted.length; const variance = sorted.reduce((sum, value) => sum + (value - mean) ** 2, 0) / sorted.length; const percentileIndex = Math.min( sorted.length - 1, Math.ceil(sorted.length * 0.95) - 1 ); return { min: sorted[0], - median: sorted[Math.floor(sorted.length / 2)], + median, mean, p95: sorted[percentileIndex], stdev: Math.sqrt(variance), }; };🤖 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 185 - 206, The median calculation in the summarizeMetric function currently picks only the upper middle element for even-length arrays, which introduces bias. Modify the median calculation by checking whether sorted.length is even or odd: if the length is even, calculate the median as the average of the two middle elements (elements at indices sorted.length / 2 - 1 and sorted.length / 2); if the length is odd, use the current approach of picking the single middle element at Math.floor(sorted.length / 2). This ensures accurate median reporting for all array sizes.scripts/benchmark-yuku.mjs (1)
10-12:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
BENCH_ITERATIONSandBENCH_SAMPLESbefore use.If either env var is invalid (e.g.,
0, negative, non-integer, or non-numeric), loop/index logic can fail with unclear errors.💡 Suggested fix
-const iterations = Number(process.env.BENCH_ITERATIONS ?? 250); -const sampleCount = Number(process.env.BENCH_SAMPLES ?? 24); +const parsePositiveInt = (raw, name) => { + const value = Number(raw); + if (!Number.isInteger(value) || value < 1) { + throw new Error(`${name} must be a positive integer. Received: ${raw}`); + } + return value; +}; + +const iterations = parsePositiveInt( + process.env.BENCH_ITERATIONS ?? '250', + 'BENCH_ITERATIONS' +); +const sampleCount = parsePositiveInt( + process.env.BENCH_SAMPLES ?? '24', + 'BENCH_SAMPLES' +);🤖 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-yuku.mjs` around lines 10 - 12, The iterations and sampleCount variables are converted from environment variables without validation, which can cause failures if the values are invalid such as zero, negative, non-integer, or non-numeric. Add validation checks immediately after the Number() conversions to ensure both iterations and sampleCount are positive integers, and throw a descriptive error if either value fails validation.scripts/benchmark/fixture.mjs (1)
404-404:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix profile rotation off-by-one in route profile selection.
Line 404 uses
index % routeExportProfiles.lengthwhileindexis 1-based, so the first profile (plain) is skipped until every 6th route and the intended distribution is shifted.Suggested patch
- const profile = routeExportProfiles[index % routeExportProfiles.length]; + const profile = + routeExportProfiles[(index - 1) % routeExportProfiles.length];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/benchmark/fixture.mjs` at line 404, The profile rotation at the line using `index % routeExportProfiles.length` suffers from an off-by-one error because `index` is 1-based while array indexing requires 0-based values. Fix the calculation by adjusting the index to be 0-based before applying the modulo operation, changing it to `(index - 1) % routeExportProfiles.length` so that the first profile is selected correctly and the distribution across all profiles remains even.scripts/compare-benchmarks.mjs (1)
28-28:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim operation tokens when parsing
--operations.Without trimming, whitespace in comma-separated values causes silent mismatches in
operationMetric(...).💡 Suggested fix
-const operations = new Set(values.operations.split(',').filter(Boolean)); +const operations = new Set( + values.operations + .split(',') + .map(operation => operation.trim()) + .filter(Boolean) +);📝 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 operations = new Set( values.operations .split(',') .map(operation => operation.trim()) .filter(Boolean) );🤖 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/compare-benchmarks.mjs` at line 28, The operations being parsed from the comma-separated `--operations` argument are not having whitespace trimmed, which causes silent mismatches when `operationMetric(...)` tries to match them. Modify the operations assignment where the Set is created to add a `.map()` call after `.split(',')` that trims each token using `.trim()`, ensuring that operations like "test " (with whitespace) match correctly against their expected values.src/index.ts (2)
236-249:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-check
closedafter the async rescan.Line 237 guards only before
readRouteDirectoryState(). If the dev server closes while that await is pending, Line 245 can recreate watchers afterclose()already cleared them, leakingFSWatchers and potentially touching the restart marker during shutdown.Proposed fix
const nextState = await readRouteDirectoryState({ watchDirectory, getRouteTopology, }); + if (closed) { + return; + } syncDirectoryWatchers(nextState.directories); if (!areSetsEqual(state.routeTopology, nextState.routeTopology)) { state = nextState; - await touchRestartMarker(); + if (!closed) { + await touchRestartMarker(); + } return; }📝 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 runRescan = async (): Promise<void> => { if (closed) { return; } try { const nextState = await readRouteDirectoryState({ watchDirectory, getRouteTopology, }); if (closed) { return; } syncDirectoryWatchers(nextState.directories); if (!areSetsEqual(state.routeTopology, nextState.routeTopology)) { state = nextState; if (!closed) { await touchRestartMarker(); } return;🤖 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 236 - 249, The runRescan function checks the closed flag only before awaiting readRouteDirectoryState, but if the dev server closes while that await is pending, the function will continue and call syncDirectoryWatchers and touchRestartMarker on already-cleared watchers, causing FSWatcher leaks. Add a re-check of the closed flag immediately after the await completes and before calling syncDirectoryWatchers and touchRestartMarker to ensure no operations occur after the server has shut down.
1609-1618:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrip
.jsfrom bundle IDs with the correct regex.Line 1612 uses
/\\.js$/, which matches a literal backslash before anyjssuffix, soserver-manifest-admin.jskeeps the capturedadmin.js. That misseslatestServerManifestsByBundleId['admin']and falls back to the full server manifest for bundle-specific manifest modules.Proposed fix
const bundleId = bundleMatch?.[1]?.replace(/\\.js$/, ''); + const bundleId = bundleMatch?.[1]?.replace(/\.js$/, '');📝 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 bundleMatch = args.resource.match( /virtual\/react-router\/server-manifest(?:-([^?]+))?/ ); const bundleId = bundleMatch?.[1]?.replace(/\.js$/, ''); const manifest = (isBuild && latestServerManifest ? bundleId && latestServerManifestsByBundleId[bundleId] ? latestServerManifestsByBundleId[bundleId] : latestServerManifest🤖 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 1609 - 1618, The regex pattern in the bundleId assignment uses an incorrect escape sequence that matches a literal backslash instead of a dot. In the bundleId variable assignment where bundleMatch?.[1]?.replace() is called, the regex pattern needs to be corrected from /\\.js$/ to /.js$/ so that it properly strips the .js file extension from the captured bundle identifier. This will ensure that bundle IDs like admin.js are correctly transformed to admin and can properly match entries in the latestServerManifestsByBundleId object instead of falling back to the full server manifest.src/manifest.ts (1)
223-225:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not swallow route-analysis failures during manifest creation.
Line [223]-Line [225] logs and continues with empty export metadata. That can mask downstream invalid-export checks that consume
getRouteManifestModuleExports(manifest)and produce a manifest that looks valid when analysis actually failed.Suggested patch
} catch (error) { - console.error(`Failed to analyze route file ${routeFilePath}:`, error); + const message = + error instanceof Error ? error.message : String(error); + throw new Error( + `Failed to analyze route file ${routeFilePath}: ${message}` + ); }📝 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.} catch (error) { const message = error instanceof Error ? error.message : String(error); throw new Error( `Failed to analyze route file ${routeFilePath}: ${message}` ); }🤖 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 handling route file analysis errors is silently logging the error and continuing with empty export metadata, which masks downstream failures in functions like getRouteManifestModuleExports(manifest). Instead of just logging and continuing in this catch block, re-throw the error or propagate the failure appropriately so that manifest creation fails when route analysis fails, preventing invalid manifests from being created with missing export metadata.src/route-chunks.ts (2)
405-418:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve side-effect imports in generated route chunks.
import './setup'/ CSS imports have no local identifier, so they never enterdependencies.topLevelStatements; the chunk forclientLoader,HydrateFallback, etc. is emitted without those side effects.Proposed fix
const program = analyzeCode(code, cache, cacheKey).program; const body = program.body - .filter((node: AnyNode) => dependencies.topLevelStatements.has(node)) + .filter( + (node: AnyNode) => + dependencies.topLevelStatements.has(node) || + (node.type === 'ImportDeclaration' && + node.specifiers.length === 0) + ) .map((node: AnyNode) => {🤖 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 405 - 418, The filtering logic in the map function only preserves ImportDeclaration nodes that have identifiers in dependencies.importedIdentifierNames, which causes side-effect imports (like import './setup' or CSS imports) to be dropped since they have no local identifiers. Modify the filter condition before the map to also include ImportDeclaration nodes that are side-effect imports (those with specifiers that don't create local bindings), ensuring these imports are preserved in the generated route chunks even when they're not tracked in dependencies.topLevelStatements. This requires checking each ImportDeclaration to determine if it has side-effects and preserving it regardless of whether its identifiers are in the dependencies tracking.
442-447:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not drop exported function/class dependencies from chunks.
If
clientLoadercalls an inline exported helper, e.g.export function helper() {}, dependency analysis includes that statement but this filter removes it becausehelper !== clientLoader, producing a chunk with an undefined helper. Preserve dependent declarations as non-exported declarations or mark this shape unchunkable.🤖 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 442 - 447, The filter in the FunctionDeclaration and ClassDeclaration check is dropping exported dependencies that do not match the exportName, causing undefined references in the chunk. Instead of returning null when declaration.id?.name does not equal exportName, preserve these dependent declarations by either converting them to non-exported declarations (remove the export keyword) or prevent chunking entirely for this declaration shape. Ensure that inline exported helpers referenced by clientLoader are retained in the output chunk regardless of whether their name matches the target exportName.src/virtual-modules.ts (1)
10-19:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject backslash-based traversal in virtual module IDs.
Line [11]-Line [19] only validates
/-separated segments. A module id containing..\\bypasses this check and can normalize outside the intendednode_modules/virtual/react-router/*scope on Windows.Suggested patch
const relativeId = moduleId.slice(VIRTUAL_MODULE_PREFIX.length); + if (relativeId.includes('\\')) { + throw new Error(`Invalid virtual module id: ${moduleId}`); + } const segments = relativeId.split('/'); if ( !relativeId || segments.some(segment => !segment || segment === '.' || segment === '..') ) {📝 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 relativeId = moduleId.slice(VIRTUAL_MODULE_PREFIX.length); if (relativeId.includes('\\')) { throw new Error(`Invalid virtual module id: ${moduleId}`); } const segments = relativeId.split('/'); if ( !relativeId || segments.some(segment => !segment || segment === '.' || segment === '..') ) { 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 10 - 19, The validation in the module ID check only validates forward slash-separated segments and does not account for backslash-based path traversal attacks on Windows systems, allowing bypass attempts using sequences like `..\\`. Add an additional validation check to reject module IDs containing backslashes before or in addition to the existing segment validation. This ensures that path traversal attempts using backslashes are properly caught and rejected when validating the relativeId or moduleId parameter.task/route-chunk-correctness-test-spec.md (1)
32-32:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd fence language tags to satisfy markdownlint (MD040).
Line 32 and Line 325 use unlabeled fenced blocks, which keeps markdownlint warnings active.
Suggested patch
-``` +```text detectRouteChunksIfEnabled(cache, config, id, code) ───────────────────────────────────────────────── ... -``` +```-``` +```ts export { default } from "./r.tsx?route-chunk=main"; export { clientAction } from "./r.tsx?route-chunk=clientAction"; -``` +```Also applies to: 325-325
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@task/route-chunk-correctness-test-spec.md` at line 32, The markdown file contains unlabeled fenced code blocks which violate markdownlint rule MD040. Locate the fenced code blocks at line 32 and line 325. For the first fenced block at line 32 that contains the detectRouteChunksIfEnabled function call, add the language tag "text" to the opening fence. For the second fenced block at line 325 that contains the export statements, add the language tag "ts" to the opening fence. Both blocks should change from unlabeled opening fences (triple backticks with no language identifier) to labeled opening fences (triple backticks followed by the appropriate language tag).Source: Linters/SAST tools
task/route-chunk-precompute-plan.md (1)
33-43:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language identifier to the fenced code block.
The fence beginning on Line 33 is unlabeled and triggers markdownlint MD040.
💡 Suggested fix
-``` +```text codeToAst (L87-97) → parse() cached at ${ck}::codeToAst; structuredClone RUNS ON EVERY CALL (cache hit or miss) getExportDependencies (L158-315)→ one traverse() building Map<name, ExportDependencies{node-ref Sets}>; cached ${ck}::getExportDependencies hasChunkableExport (L460-516) → set-intersection over ExportDependencies; cached ${ck}::hasChunkableExport::${name} getChunkedExport (L518-617) → codeToAst(CLONE) + filter body via t.isNodesEquivalent + generate(); cached ${ck}::getChunkedExport::${name}::opts omitChunkedExports (L619-758) → codeToAst(CLONE) + filter body via t.isNodesEquivalent + generate(); cached ${ck}::omitChunkedExports::${names}::opts detectRouteChunks (L760-780) → hasChunkableExport ×4 getRouteChunkCode (L782-797) → dispatch 'main'→omitChunkedExports, named→getChunkedExport detectRouteChunksIfEnabled (L834-868) → guards (splitRouteModules / root / substring) then detectRouteChunks getRouteChunkIfEnabled (L870-888) → guards (splitRouteModules only — NO root guard, intentional) then getRouteChunkCode</details> <details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.22.1)</summary> [warning] 33-33: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@task/route-chunk-precompute-plan.mdaround lines 33 - 43, The fenced code
block starting at line 33 in the file task/route-chunk-precompute-plan.md is
missing a language identifier, which triggers markdownlint rule MD040. To fix
this, add a language identifier to the opening fence by changing the opening
triple backticks fromtotext, since the content consists of plain text
describing function names and their behavior. This will satisfy the markdown
linting requirement while keeping the content unchanged.</details> <!-- fingerprinting:phantom:poseidon:hawk --> <!-- cr-comment:v1:1728b159f4f2b708c610de0f --> _Source: Linters/SAST tools_ <!-- This is an auto-generated comment by CodeRabbit --> </blockquote></details> <details> <summary>tests/index.test.ts (1)</summary><blockquote> 43-48: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Use cross-platform path matching for watch-file assertions.** These regexes are slash-specific; they can fail on Windows where resolved paths use backslashes. <details> <summary>💡 Suggested fix</summary> ```diff { - paths: expect.stringMatching(/app\/routes\.[cm]?[jt]sx?$/), + paths: expect.stringMatching(/app[\\/]routes\.[cm]?[jt]sx?$/), type: 'reload-server', }, { - paths: expect.stringMatching(/\.react-router\/route-watch$/), + paths: expect.stringMatching(/\.react-router[\\/]route-watch$/), type: 'reload-server', }, ``` </details> <!-- suggestion_start --> <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion paths: expect.stringMatching(/app[\\/]routes\.[cm]?[jt]sx?$/), type: 'reload-server', }, { paths: expect.stringMatching(/\.react-router[\\/]route-watch$/), type: 'reload-server', ``` </details> <!-- suggestion_end --> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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 43 - 48, The regex patterns in the stringMatching assertions use hardcoded forward slashes which will fail on Windows where paths use backslashes. Update the regex patterns in both the app routes path assertion and the route-watch path assertion to use character classes that match both forward and backward slashes (replacing each \/ with [\\/]) so the patterns work correctly on both Unix-like and Windows systems. ``` </details> <!-- fingerprinting:phantom:poseidon:hawk --> <!-- cr-comment:v1:272b35722ce9be618795dfbe --> <!-- This is an auto-generated comment by CodeRabbit --> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
ScriptedAlchemy
force-pushed
the
fix/dev-route-entry-watch
branch
from
fe4e06e to
343faf4
Compare
ScriptedAlchemy
force-pushed
the
fix/dev-route-entry-watch
branch
from
343faf4 to
dcf9fe9
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
perf/bundling-performance) so this PR only contains the dev route-entry watch changes.reload-serverwatch list narrow: the route config file plus a generated route restart marker.reload-serverwatches on all existing route files.Test plan
pnpm test:core tests/index.test.ts tests/route-watch.test.tspnpm buildexamples/default-templatePlaywright e2e includestests/e2e/dev-route-watch.test.ts, which adds a route after dev startup and verifies later route edits do not restart the dev server.