Open
Conversation
github-actions
Bot
added
documentation
Contributor
📝 WalkthroughWalkthroughAdd Rsbuild/Rspack support across Start (React, Solid, Vue), refactor import-protection into a shared adapter-agnostic core with Vite and Rsbuild adapters, introduce RSC rsbuild/rs-pack decode entrypoints and types, abstract post-build/prerender into adapter interfaces, and update E2E/test/toolchain wiring for vite/rsbuild selection. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,230,201,0.5)
participant Dev as Developer (CLI)
participant Rsbuild as Rsbuild Plugin
participant Rspack as Rspack Compiler
participant Virtual as VirtualModules (fs/.virtual)
participant Post as PostBuild/Prerender
end
Dev->>Rsbuild: build/dev (uses tanStackStartRsbuild)
Rsbuild->>Rspack: modify config, register transforms & virtual modules
Rspack->>Rsbuild: transform modules (post)
Rsbuild->>Virtual: write virtual mock/manifest modules (queued if not ready)
Rsbuild->>Rsbuild: run import-protection analysis & record deferred violations
Rsbuild->>Post: on build completion -> postBuildWithRsbuild(adapter)
Post->>Virtual: read client output dir manifest -> prerender pages
Post->>Dev: finish (reports/prerender outputs)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
|
View your CI Pipeline Execution ↗ for commit 4cf4ea3
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (13)
e2e/solid-start/basic/server.js (1)
16-28: Use an explicit error instead of a silent fallback to a missing file.Line 27 currently returns
server.jseven when neitherserver.jsnorindex.jsexists, which defers failure to the dynamic import on Line 32. Throwing early makes e2e failures much easier to diagnose.Suggested patch
function resolveDistServerEntryPath() { const serverJsPath = path.resolve(distDir, 'server', 'server.js') if (fs.existsSync(serverJsPath)) { return serverJsPath } const indexJsPath = path.resolve(distDir, 'server', 'index.js') if (fs.existsSync(indexJsPath)) { return indexJsPath } - return serverJsPath + throw new Error( + `Could not find server entry. Checked: ${serverJsPath} and ${indexJsPath}`, + ) }Also applies to: 31-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/solid-start/basic/server.js` around lines 16 - 28, The function resolveDistServerEntryPath currently returns serverJsPath even if neither server.js nor index.js exist; change it to check both serverJsPath and indexJsPath and if neither exists throw an explicit Error (including the resolved paths like serverJsPath and indexJsPath) instead of returning a missing path so the failure surfaces immediately before the dynamic import that uses the returned value.e2e/react-start/basic/server.js (1)
16-28: Fail fast when no built server entry exists.Line 27 returns a non-existent path if both candidates are missing, so the import on Line 32 fails later with a less clear error. Throw an explicit error after both checks.
Suggested patch
function resolveDistServerEntryPath() { const serverJsPath = path.resolve(distDir, 'server', 'server.js') if (fs.existsSync(serverJsPath)) { return serverJsPath } const indexJsPath = path.resolve(distDir, 'server', 'index.js') if (fs.existsSync(indexJsPath)) { return indexJsPath } - return serverJsPath + throw new Error( + `Could not find server entry. Checked: ${serverJsPath} and ${indexJsPath}`, + ) }Also applies to: 31-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/basic/server.js` around lines 16 - 28, The helper resolveDistServerEntryPath currently returns serverJsPath even when neither server/server.js nor server/index.js exist; change it to throw a clear Error when both fs.existsSync checks fail instead of returning a non-existent path (include the tested paths like serverJsPath and indexJsPath or distDir in the error message) so callers (e.g., the subsequent dynamic import of the resolved path) fail fast with a descriptive message; update any similar logic around the import to rely on this thrown error rather than receiving a bogus path.e2e/vue-start/basic/server.js (1)
16-28: Prefer explicit failure when no server entry exists.If neither candidate file exists, Line 27 returns a non-existent path and fails later during
import(...). Throwing here gives clearer diagnostics.Proposed refactor
function resolveDistServerEntryPath() { const serverJsPath = path.resolve(distDir, 'server', 'server.js') if (fs.existsSync(serverJsPath)) { return serverJsPath } const indexJsPath = path.resolve(distDir, 'server', 'index.js') if (fs.existsSync(indexJsPath)) { return indexJsPath } - return serverJsPath + throw new Error( + `No server entry found. Checked:\n- ${serverJsPath}\n- ${indexJsPath}`, + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/vue-start/basic/server.js` around lines 16 - 28, The function resolveDistServerEntryPath currently returns serverJsPath even when neither candidate exists, causing a later import to fail with unclear diagnostics; update resolveDistServerEntryPath to check both serverJsPath and indexJsPath and throw a clear Error (including the checked paths) if neither fs.existsSync(serverJsPath) nor fs.existsSync(indexJsPath) is true so callers get immediate, descriptive failure instead of a broken path.e2e/react-start/import-protection/tests/error-mode.setup.ts (1)
22-25: ResolveE2E_TOOLCHAINonce and reuse it consistently.Line 22 and Line 126 compute toolchain separately. Keeping one resolved value avoids drift between port-key derivation and spawned command selection.
♻️ Proposed refactor
-const toolchain = process.env.E2E_TOOLCHAIN ?? 'vite' +const toolchain = process.env.E2E_TOOLCHAIN ?? 'vite' const e2ePortKey = process.env.E2E_PORT_KEY ?? `${packageJson.name}-${toolchain}` @@ - const toolchain = process.env.E2E_TOOLCHAIN ?? 'vite' const command = toolchain === 'rsbuild' ? ['exec', 'rsbuild', 'dev', '--port', String(port)] : ['exec', 'vite', 'dev', '--port', String(port)]Also applies to: 126-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection/tests/error-mode.setup.ts` around lines 22 - 25, Resolve E2E_TOOLCHAIN once at module load and reuse that single constant (currently declared as toolchain alongside e2ePortKey) instead of recomputing it later; modify the later block that currently recomputes toolchain (the code that selects the spawned command and port key around lines 126–130) to reference the module-scoped toolchain and the existing e2ePortKey, ensuring the same resolved value is used for both port-key derivation and spawned command selection.e2e/react-start/import-protection/package.json (1)
25-26: Version pinning inconsistency with main package.The
@rsbuildpackages here are pinned to exact versions (2.0.0-rc.3) without the caret, whilepackages/react-start/package.jsonuses^2.0.0-rc.3. Consider aligning the version specification style for consistency across the monorepo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection/package.json` around lines 25 - 26, In package.json update the dependency entries for "@rsbuild/core" and "@rsbuild/plugin-react" to use the same caret-style version specifier as the main package (e.g. change "2.0.0-rc.3" to "^2.0.0-rc.3") so the version pinning is consistent across the monorepo; locate the two keys "@rsbuild/core" and "@rsbuild/plugin-react" in e2e/react-start/import-protection/package.json and modify their version strings to include the leading caret.packages/start-plugin-core/tests/rsbuild-post-build.test.ts (1)
15-16: Replaceanyin test setup to preserve strict typing.
anyis used in three places and can hide real API drift in this integration test.Suggested refactor
- const prerenderSpy = vi.fn(async ({ handler }: any) => { + const prerenderSpy = vi.fn( + async ({ handler }: { handler: { request: (path: string) => Promise<Response> } }) => { const response = await handler.request('/posts') expect(await response.text()).toBe('ok') - }) + }, + ) vi.doMock('../src/prerender', async () => { - const actual = await vi.importActual<any>('../src/prerender') + const actual = + await vi.importActual<typeof import('../src/prerender')>( + '../src/prerender', + ) return { ...actual, prerender: prerenderSpy, @@ - } as any, + } satisfies Parameters<typeof postBuildWithRsbuild>[0]['startConfig'],As per coding guidelines
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.Also applies to: 21-21, 49-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/tests/rsbuild-post-build.test.ts` around lines 15 - 16, The test uses the unsafe type any in the prerenderSpy callback (and two other spots), which weakens strict-mode checks; update the signature to use a concrete type for the callback parameter (e.g., import or declare an appropriate Handler/PrerenderContext type and use { handler: Handler } or the specific interface used by the prerender API) and replace the other two any usages with the same type so the test preserves strict typing and catches API drift (refer to prerenderSpy and the handler parameter name to locate affected code).packages/start-plugin-core/tests/importProtection/sourceLocation.test.ts (1)
2-16: Import order should be alphabetically sorted.ESLint reports that
ImportLocCacheshould be sorted alphabetically among the imports. Consider reordering to satisfy thesort-importsrule.Proposed fix
import { + ImportLocCache, + buildCodeSnippet, + buildLineIndex, createImportSpecifierLocationIndex, - ImportLocCache, - buildCodeSnippet, - buildLineIndex, findOriginalUsageLocation, getOrCreateOriginalTransformResult, normalizeSourceMap, pickOriginalCodeFromSourcesContent, } from '../../src/import-protection/sourceLocation'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/tests/importProtection/sourceLocation.test.ts` around lines 2 - 16, Reorder the named imports in the import statement from '../../src/import-protection/sourceLocation' so they are alphabetically sorted (e.g., place ImportLocCache in its correct alphabetical position among createImportSpecifierLocationIndex, buildCodeSnippet, buildLineIndex, findOriginalUsageLocation, getOrCreateOriginalTransformResult, normalizeSourceMap, pickOriginalCodeFromSourcesContent) to satisfy the sort-imports ESLint rule; do the same for the type imports (SourceMapLike, TransformResult, TransformResultProvider) so both import lists are alphabetized.packages/start-plugin-core/src/vite/import-protection-plugin/plugin.ts (1)
436-438: Type assertion suggests potential type mismatch.The cast
as EnvRuleson the return value ofgetImportProtectionRulesForEnvironmentsuggests the shared utility's return type may not perfectly match the Vite-specificEnvRulestype. Consider aligning the types or adding a comment explaining why the cast is necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/vite/import-protection-plugin/plugin.ts` around lines 436 - 438, The explicit cast in getRulesForEnvironment indicates a type mismatch; remove the unsafe "as EnvRules" and align types by either changing getRulesForEnvironment's return type to match the utility (e.g., use ReturnType<typeof getImportProtectionRulesForEnvironment>) or update getImportProtectionRulesForEnvironment to return the EnvRules shape; if you must keep a cast, add a brief comment explaining why the shapes are compatible and add a runtime validation (or a narrow mapper) inside getRulesForEnvironment to convert/validate the utility output into EnvRules (referencing getRulesForEnvironment and getImportProtectionRulesForEnvironment).packages/start-plugin-core/src/rsbuild/import-protection.ts (2)
1361-1363: Redundant assignment - was filtering intended?
relevantModulesis assigned directly fromallModuleswithout any filtering. If filtering was intended (e.g., to exclude certain module types), this should be added. Otherwise, consider usingallModulesdirectly.♻️ Remove redundant assignment
const allModules = Array.from(context.compilation.modules) - const relevantModules = allModules const provider = await buildTransformResultProvider({ - modules: relevantModules, + modules: allModules, root: config.root, loadOriginalCode: loadOriginalCodeFromCompilation, preloaded: envState.buildTransformResults, })And update subsequent usages of
relevantModulestoallModules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/rsbuild/import-protection.ts` around lines 1361 - 1363, The assignment of relevantModules to Array.from(context.compilation.modules) is redundant because relevantModules just mirrors allModules; either remove the irrelevant variable or implement the intended filtering. Replace usages of relevantModules with allModules and delete the declaration of relevantModules, or if filtering was intended, apply the filter to produce relevantModules (e.g., filter by module type/path) where currently const relevantModules = allModules is declared and used in this file (look for relevantModules and allModules references in import-protection.ts and update accordingly).
1-3: ImportingnormalizePathfrom vite in a Rsbuild adapter.This imports
normalizePathfrom thevitepackage in a Rsbuild-specific module. While this works (vite is likely a dev dependency), consider extracting path normalization to a shared utility to avoid coupling Rsbuild code to Vite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/rsbuild/import-protection.ts` around lines 1 - 3, The import of normalizePath from 'vite' in import-protection.ts couples the Rsbuild adapter to Vite; remove the direct import and instead call a shared path-normalization utility (e.g., create or use an existing util like normalizePathSafe or normalizeFilePath) from a common helper module; update references in import-protection.ts to use that utility and keep the existing resolvePath import for node:path unchanged so code uses resolvePath and the new shared normalize function rather than importing normalizePath from 'vite'.packages/start-plugin-core/src/rsbuild/normalized-client-build.ts (1)
268-282: Minor: Redundant null check forentryChunkFileName.At line 270,
entryChunkFileNameis checked again, but it's guaranteed to be defined due to the throw on line 258-260. The check is harmless but redundant.♻️ Optional simplification
const rscEntrypoint = compilation.entrypoints.get('rsc') - if (rscEntrypoint && entryChunkFileName) { + if (rscEntrypoint) { const mainEntryChunk = chunksByFileName.get(entryChunkFileName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/rsbuild/normalized-client-build.ts` around lines 268 - 282, The code redundantly checks entryChunkFileName even though earlier logic guarantees it; simplify the conditional by removing the redundant entryChunkFileName check so the block uses only rscEntrypoint (i.e., change `if (rscEntrypoint && entryChunkFileName)` to `if (rscEntrypoint)`), keeping the inner lookup of `mainEntryChunk = chunksByFileName.get(entryChunkFileName)` and the rest of the logic around `mainEntryChunk` and `rscEntrypoint.chunks` unchanged.packages/start-plugin-core/src/rsbuild/start-router-plugin.ts (1)
58-60: Empty catch blocks may hide unexpected errors.The empty catch blocks silently swallow all errors, not just "module not found" errors. Consider logging or checking the error type to avoid masking unexpected failures during development.
♻️ Proposed improvement to differentiate error types
} catch { - // router-plugin/rspack not available — skip + // router-plugin/rspack not available — skip generator + // Note: This also silently ignores other potential errors }Alternatively, check the error code:
} catch (e: unknown) { if (!(e instanceof Error && 'code' in e && e.code === 'ERR_MODULE_NOT_FOUND')) { console.warn('[TanStack Start] Unexpected error loading router-plugin/rspack:', e) } }Also applies to: 81-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/rsbuild/start-router-plugin.ts` around lines 58 - 60, Replace the empty catch blocks around the dynamic import of 'router-plugin/rspack' (and the similar block at lines handling the second import) with typed error handling: catch the error as e: unknown, check if it's an Error with a code property equal to 'ERR_MODULE_NOT_FOUND' and silently ignore only that case, otherwise log or warn the unexpected error (e.g., console.warn('[TanStack Start] Unexpected error loading router-plugin/rspack:', e)); this preserves the original behavior for missing module but surfaces other failures for debugging.packages/start-plugin-core/src/import-protection/sourceLocation.ts (1)
108-130: Unify source-map normalization in one path.
normalizeSourceMap()now hardensfile,version,names, andsourcesContent, butgetSourceMapConsumer()still goes throughtoRawSourceMap()instead. Keeping two normalization paths here is easy to drift and leaves theSourceMapConsumercall site with different behavior than the new helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/import-protection/sourceLocation.ts` around lines 108 - 130, The code duplicates source-map normalization: normalizeSourceMap hardens file/version/names/sourcesContent but getSourceMapConsumer still uses toRawSourceMap(), causing drift; update the getSourceMapConsumer (or the call site that uses toRawSourceMap) to pass its input through normalizeSourceMap() before constructing the SourceMapConsumer (or have toRawSourceMap delegate to normalizeSourceMap) so there is a single normalization path; ensure you reference normalizeSourceMap(...) when preparing the map and remove the separate hardening in toRawSourceMap/getSourceMapConsumer so both consumers receive the same normalized map shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/react-start/import-protection/package.json`:
- Line 10: The current "start" script pipes the raw stdout of node -e importing
server.js (and calling resolveStartCommand()) directly into sh, which is fragile
if server.js prints any extra output; change this by replacing the one-liner
with a dedicated script (e.g., start-server.js) that imports ./server.js, calls
resolveStartCommand(), validates/escapes the returned string, and then executes
it via a safe API (child_process.exec/execFile/spawn) or writes a JSON payload
to stdout for a wrapper to parse; update the package.json "start" to run that
dedicated script instead.
In `@e2e/react-start/import-protection/server.js`:
- Around line 10-22: The function resolveDistServerEntryPath currently returns
serverJsPath even when neither server.js nor index.js exists; update
resolveDistServerEntryPath to detect the missing entry and throw a clear error
(or log and throw) instead of returning a non-existent path: after checking
serverJsPath and indexJsPath, if neither exists use distDir, serverJsPath and
indexJsPath in the error message to make it clear which files were expected and
where, and ensure any caller of resolveDistServerEntryPath will receive the
thrown error rather than a bogus path.
In `@e2e/react-start/import-protection/start-mode-config.ts`:
- Line 2: The current line force-casts process.env.BEHAVIOR with "as 'mock' |
'error'", which allows invalid values; replace it with runtime validation:
implement a small validator (e.g., isBehavior or a Set of allowed values) that
checks process.env.BEHAVIOR === 'mock' || === 'error' and only then assigns it
to the behavior const, otherwise fall back to the default 'mock' (or throw if
you prefer stricter behavior); update the variable declaration for behavior to
use that validated value instead of the type assertion so invalid env values are
handled safely.
In `@e2e/react-start/import-protection/tests/violations.utils.ts`:
- Around line 32-36: The stripBoxPrefix function is over-greedy: its /^\s*\|\s?/
pattern removes leading '|' even when it's part of code-gutter lines like " |
^"; update the logic in stripBoxPrefix so it only strips a pipe used as a true
line prefix (e.g. at column 0) rather than any pipe after leading whitespace —
change the pipe removal from /^\s*\|\s?/ to a stricter match (e.g. /^\|\s?/) or
equivalent check so code-gutter continuation lines are preserved while still
removing box-border pipes and the existing '│' handling remains intact.
In `@e2e/vue-start/basic-vue-query/src/utils/posts.tsx`:
- Around line 10-14: The code builds a mutable queryURL and can produce
"http://localhost:undefined" when VITE_EXTERNAL_PORT is missing; change to
immutable URL construction and validate the test port: when
import.meta.env.VITE_NODE_ENV === 'test' verify
import.meta.env.VITE_EXTERNAL_PORT is present (and optionally parseInt/validate
it's a number) and throw a clear error if not, otherwise construct a const
testBase = `http://localhost:${port}` and assign queryURL to that value (do not
mutate a top-level let); reference the queryURL variable and the
import.meta.env.VITE_EXTERNAL_PORT check in posts.tsx so failures fail fast with
an explicit error message.
In `@packages/react-start-rsc/src/react-server-dom-rspack.d.ts`:
- Around line 33-45: The ambient declaration incorrectly exposes
createFromReadableStream and loadServerAction from
react-server-dom-rspack/server even though those symbols are not exported there;
remove the createFromReadableStream and loadServerAction function declarations
from the server ambient module so the typings match runtime exports (keep
decodeReply and createTemporaryReferenceSet), and if you need typings for
createFromReadableStream or loadServerAction add them to the correct module
declarations (e.g., the client-side declaration for createFromReadableStream and
the TanStack RSC resolver entry for loadServerAction) so invalid imports no
longer type-check.
In `@packages/react-start/package.json`:
- Around line 156-170: The package currently pins "@rsbuild/core" to an
unavailable pre-release "^2.0.0-rc.3"; update the dependency entry for
"@rsbuild/core" in package.json to the latest known stable "1.7.5" (or
alternatively confirm the correct pre-release tag exists and replace with the
accurate tag) and ensure the same change is reflected in devDependencies to keep
runtime and dev-time versions consistent.
In `@packages/start-plugin-core/src/import-protection/analysis.ts`:
- Around line 658-668: When handling function nodes in the visitor (the branch
that builds functionCtx using collectFunctionBindings and calls
visit(node.body)), also traverse parameter initializers so default values and
destructuring defaults are analyzed; for example, iterate node.params (and
nested patterns) and call visit on any param.initializer or default expression
using the same functionCtx before visiting node.body so usages like fn(x =
serverOnly()) are detected.
In `@packages/start-plugin-core/src/import-protection/sourceLocation.ts`:
- Around line 346-355: In createImportSpecifierLocationIndex's find
implementation remove the preliminary text-scan short-circuit that uses
result.code.includes(source) and instead always call
getImportSpecifierLocationFromResult(result, source) as the single source of
truth; update the find function in the createImportSpecifierLocationIndex export
to return the result of getImportSpecifierLocationFromResult(result, source) (or
-1 only if that helper indicates no match), ensuring TransformResult is passed
through unchanged.
In `@packages/start-plugin-core/src/post-build.ts`:
- Around line 42-45: The code spreads startConfig.spa.prerender.headers without
guaranteeing it exists, which can cause a runtime/type error; update the code
that builds the prerender headers (the object using ...startConfig.spa.prerender
and HEADERS.TSS_SHELL) to guard against undefined by either defaulting headers
in the schema transform (e.g., set headers: opts.headers ?? {} inside the
prerender transform) or by changing the spread site to use a safe fallback like
... (startConfig.spa.prerender.headers ?? {}), ensuring HEADERS.TSS_SHELL is
still added.
In `@packages/start-plugin-core/src/prerender.ts`:
- Around line 235-244: The code only follows absolute and root-relative Location
headers; fix by resolving relative Location values before deciding same-origin:
read the header via response.headers.get('location'), create a resolved URL
using new URL(location, new URL(requestPathOrUrl, 'http://localhost')) (use the
same original request identifier passed into requestWithRedirects), then if
resolvedUrl.origin is localhost (or resolvedUrl.hostname === 'localhost') or the
resolved path is same-origin, call requestWithRedirects(resolvedUrl.pathname +
resolvedUrl.search + resolvedUrl.hash, options, maxRedirects - 1); otherwise
keep the existing logger.warn branch. Update handling in the isRedirectResponse
block and keep references to isRedirectResponse, requestWithRedirects, logger,
response.headers.get('location'), maxRedirects, and options.
In `@packages/start-plugin-core/src/rsbuild/dev-server.ts`:
- Around line 93-109: The error HTML directly interpolates the exception message
(e instanceof Error ? e.message : String(e)) into the Response body causing
possible HTML injection; update the dev-server error path that constructs the
Response (the sendNodeResponse / new Response call) to escape HTML special
characters in the error text before interpolation (e.g., use an escapeHtml
utility or encode the message) or render the error as text/plain, ensuring you
sanitize the value derived from e.message (or String(e)) before placing it into
the HTML response.
In `@packages/start-plugin-core/src/rsbuild/plugin.ts`:
- Around line 207-217: The plugin is reading resolvedStartConfig.root during
setup (used in registerStartCompilerTransforms) before modifyRsbuildConfig
copies rsbuildConfig.root into the resolved config, causing defaulting to
process.cwd() for apps with a custom rsbuild root; fix by deferring use of the
start root until after rsbuild config resolution—e.g. change the root argument
passed to registerStartCompilerTransforms to read from rsbuildConfig.root (or,
if rsbuildConfig may not yet exist, pass a function/closure that returns the
resolved start root after modifyRsbuildConfig runs) so
registerStartCompilerTransforms always receives the correct root instead of
process.cwd(); reference registerStartCompilerTransforms,
resolvedStartConfig.root, modifyRsbuildConfig, and rsbuildConfig.root when
making the change.
In `@packages/start-plugin-core/src/rsbuild/virtual-modules.ts`:
- Around line 315-352: getInitialContent() currently treats the provider
environment as a client and emits 'export {}' for paths.serverFnResolver, so the
provider bundle never gets the real server-fn resolver; modify the logic that
sets content[paths.serverFnResolver] to also generate the resolver when
environmentName equals the provider env (i.e., treat the provider env like the
server for this module). Concretely, update the condition that builds
resolverContent (the block using generateServerFnResolverModule with
opts.serverFnsById, includeClientReferencedCheck, useStaticImports) so it runs
when isServerEnv OR environmentName === serverFnProviderEnv (or by expanding
isServerEnv to include the provider env), and ensure
content[paths.serverFnResolver] is assigned that resolverContent for both SSR
and provider environments instead of 'export {}'. Also mirror this same change
for the other occurrence noted (lines ~511-537) so both bundles get rewritten
resolver modules.
In `@packages/start-plugin-core/tests/importProtection/analysis.exports.test.ts`:
- Around line 4-7: The import specifier list for the import from
'../../src/import-protection/analysis' is not alphabetically sorted and fails
the sort-imports rule; reorder the named imports so they are alphabetically
sorted (getMockExportNamesBySource, getNamedExports, isValidExportName) in the
import statement that currently lists isValidExportName,
getMockExportNamesBySource, getNamedExports.
In `@packages/start-plugin-core/tests/importProtection/utils.test.ts`:
- Line 23: The import of formatViolation in
packages/start-plugin-core/tests/importProtection/utils.test.ts is unused;
remove formatViolation from the import statement (the import line that currently
reads "import { formatViolation } from '../../src/import-protection/trace'") so
the test file no longer imports that symbol.
In `@packages/start-plugin-core/tests/rsbuild-post-build.test.ts`:
- Around line 1-4: Reorder the import block to satisfy the import/order rule by
grouping built-in modules first, then external packages: move "import { mkdtemp,
rm, writeFile } from 'node:fs/promises'" and "import { tmpdir } from 'node:os'"
to appear before external imports, then "import { join } from 'pathe'" and
finally the test helpers "import { describe, expect, it, vi } from 'vitest'";
ensure imports are consistently ordered (and optionally alphabetized) within
each group so symbols like mkdtemp, rm, writeFile, tmpdir, join, describe,
expect, it, vi are easy to locate.
---
Nitpick comments:
In `@e2e/react-start/basic/server.js`:
- Around line 16-28: The helper resolveDistServerEntryPath currently returns
serverJsPath even when neither server/server.js nor server/index.js exist;
change it to throw a clear Error when both fs.existsSync checks fail instead of
returning a non-existent path (include the tested paths like serverJsPath and
indexJsPath or distDir in the error message) so callers (e.g., the subsequent
dynamic import of the resolved path) fail fast with a descriptive message;
update any similar logic around the import to rely on this thrown error rather
than receiving a bogus path.
In `@e2e/react-start/import-protection/package.json`:
- Around line 25-26: In package.json update the dependency entries for
"@rsbuild/core" and "@rsbuild/plugin-react" to use the same caret-style version
specifier as the main package (e.g. change "2.0.0-rc.3" to "^2.0.0-rc.3") so the
version pinning is consistent across the monorepo; locate the two keys
"@rsbuild/core" and "@rsbuild/plugin-react" in
e2e/react-start/import-protection/package.json and modify their version strings
to include the leading caret.
In `@e2e/react-start/import-protection/tests/error-mode.setup.ts`:
- Around line 22-25: Resolve E2E_TOOLCHAIN once at module load and reuse that
single constant (currently declared as toolchain alongside e2ePortKey) instead
of recomputing it later; modify the later block that currently recomputes
toolchain (the code that selects the spawned command and port key around lines
126–130) to reference the module-scoped toolchain and the existing e2ePortKey,
ensuring the same resolved value is used for both port-key derivation and
spawned command selection.
In `@e2e/solid-start/basic/server.js`:
- Around line 16-28: The function resolveDistServerEntryPath currently returns
serverJsPath even if neither server.js nor index.js exist; change it to check
both serverJsPath and indexJsPath and if neither exists throw an explicit Error
(including the resolved paths like serverJsPath and indexJsPath) instead of
returning a missing path so the failure surfaces immediately before the dynamic
import that uses the returned value.
In `@e2e/vue-start/basic/server.js`:
- Around line 16-28: The function resolveDistServerEntryPath currently returns
serverJsPath even when neither candidate exists, causing a later import to fail
with unclear diagnostics; update resolveDistServerEntryPath to check both
serverJsPath and indexJsPath and throw a clear Error (including the checked
paths) if neither fs.existsSync(serverJsPath) nor fs.existsSync(indexJsPath) is
true so callers get immediate, descriptive failure instead of a broken path.
In `@packages/start-plugin-core/src/import-protection/sourceLocation.ts`:
- Around line 108-130: The code duplicates source-map normalization:
normalizeSourceMap hardens file/version/names/sourcesContent but
getSourceMapConsumer still uses toRawSourceMap(), causing drift; update the
getSourceMapConsumer (or the call site that uses toRawSourceMap) to pass its
input through normalizeSourceMap() before constructing the SourceMapConsumer (or
have toRawSourceMap delegate to normalizeSourceMap) so there is a single
normalization path; ensure you reference normalizeSourceMap(...) when preparing
the map and remove the separate hardening in toRawSourceMap/getSourceMapConsumer
so both consumers receive the same normalized map shape.
In `@packages/start-plugin-core/src/rsbuild/import-protection.ts`:
- Around line 1361-1363: The assignment of relevantModules to
Array.from(context.compilation.modules) is redundant because relevantModules
just mirrors allModules; either remove the irrelevant variable or implement the
intended filtering. Replace usages of relevantModules with allModules and delete
the declaration of relevantModules, or if filtering was intended, apply the
filter to produce relevantModules (e.g., filter by module type/path) where
currently const relevantModules = allModules is declared and used in this file
(look for relevantModules and allModules references in import-protection.ts and
update accordingly).
- Around line 1-3: The import of normalizePath from 'vite' in
import-protection.ts couples the Rsbuild adapter to Vite; remove the direct
import and instead call a shared path-normalization utility (e.g., create or use
an existing util like normalizePathSafe or normalizeFilePath) from a common
helper module; update references in import-protection.ts to use that utility and
keep the existing resolvePath import for node:path unchanged so code uses
resolvePath and the new shared normalize function rather than importing
normalizePath from 'vite'.
In `@packages/start-plugin-core/src/rsbuild/normalized-client-build.ts`:
- Around line 268-282: The code redundantly checks entryChunkFileName even
though earlier logic guarantees it; simplify the conditional by removing the
redundant entryChunkFileName check so the block uses only rscEntrypoint (i.e.,
change `if (rscEntrypoint && entryChunkFileName)` to `if (rscEntrypoint)`),
keeping the inner lookup of `mainEntryChunk =
chunksByFileName.get(entryChunkFileName)` and the rest of the logic around
`mainEntryChunk` and `rscEntrypoint.chunks` unchanged.
In `@packages/start-plugin-core/src/rsbuild/start-router-plugin.ts`:
- Around line 58-60: Replace the empty catch blocks around the dynamic import of
'router-plugin/rspack' (and the similar block at lines handling the second
import) with typed error handling: catch the error as e: unknown, check if it's
an Error with a code property equal to 'ERR_MODULE_NOT_FOUND' and silently
ignore only that case, otherwise log or warn the unexpected error (e.g.,
console.warn('[TanStack Start] Unexpected error loading router-plugin/rspack:',
e)); this preserves the original behavior for missing module but surfaces other
failures for debugging.
In `@packages/start-plugin-core/src/vite/import-protection-plugin/plugin.ts`:
- Around line 436-438: The explicit cast in getRulesForEnvironment indicates a
type mismatch; remove the unsafe "as EnvRules" and align types by either
changing getRulesForEnvironment's return type to match the utility (e.g., use
ReturnType<typeof getImportProtectionRulesForEnvironment>) or update
getImportProtectionRulesForEnvironment to return the EnvRules shape; if you must
keep a cast, add a brief comment explaining why the shapes are compatible and
add a runtime validation (or a narrow mapper) inside getRulesForEnvironment to
convert/validate the utility output into EnvRules (referencing
getRulesForEnvironment and getImportProtectionRulesForEnvironment).
In `@packages/start-plugin-core/tests/importProtection/sourceLocation.test.ts`:
- Around line 2-16: Reorder the named imports in the import statement from
'../../src/import-protection/sourceLocation' so they are alphabetically sorted
(e.g., place ImportLocCache in its correct alphabetical position among
createImportSpecifierLocationIndex, buildCodeSnippet, buildLineIndex,
findOriginalUsageLocation, getOrCreateOriginalTransformResult,
normalizeSourceMap, pickOriginalCodeFromSourcesContent) to satisfy the
sort-imports ESLint rule; do the same for the type imports (SourceMapLike,
TransformResult, TransformResultProvider) so both import lists are alphabetized.
In `@packages/start-plugin-core/tests/rsbuild-post-build.test.ts`:
- Around line 15-16: The test uses the unsafe type any in the prerenderSpy
callback (and two other spots), which weakens strict-mode checks; update the
signature to use a concrete type for the callback parameter (e.g., import or
declare an appropriate Handler/PrerenderContext type and use { handler: Handler
} or the specific interface used by the prerender API) and replace the other two
any usages with the same type so the test preserves strict typing and catches
API drift (refer to prerenderSpy and the handler parameter name to locate
affected code).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3daa192-f5bb-408e-a4fb-0ce3c1137ec3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (110)
docs/start/framework/react/guide/import-protection.mde2e/.gitignoree2e/react-start/basic-rsc/.devcontainer/devcontainer.jsone2e/react-start/basic/.gitignoree2e/react-start/basic/package.jsone2e/react-start/basic/postcss.config.mjse2e/react-start/basic/rsbuild.config.tse2e/react-start/basic/server.jse2e/react-start/basic/src/routes/__root.tsxe2e/react-start/basic/start-mode-config.tse2e/react-start/basic/vite.config.tse2e/react-start/import-protection/package.jsone2e/react-start/import-protection/playwright.config.tse2e/react-start/import-protection/rsbuild.config.tse2e/react-start/import-protection/server.jse2e/react-start/import-protection/start-mode-config.tse2e/react-start/import-protection/tests/error-mode.setup.tse2e/react-start/import-protection/tests/import-protection.spec.tse2e/react-start/import-protection/tests/violations.setup.tse2e/react-start/import-protection/tests/violations.utils.tse2e/react-start/import-protection/vite.config.tse2e/react-start/serialization-adapters/src/routes/server-function/late-raw-stream.tsxe2e/solid-start/basic/package.jsone2e/solid-start/basic/postcss.config.mjse2e/solid-start/basic/rsbuild.config.tse2e/solid-start/basic/server.jse2e/solid-start/basic/src/routes/__root.tsxe2e/vue-start/basic-vue-query/src/utils/posts.tsxe2e/vue-start/basic/package.jsone2e/vue-start/basic/postcss.config.mjse2e/vue-start/basic/rsbuild.config.tse2e/vue-start/basic/server.jse2e/vue-start/basic/src/routes/__root.tsxpackages/react-router/src/Asset.tsxpackages/react-router/src/ClientOnly.tsxpackages/react-start-rsc/package.jsonpackages/react-start-rsc/src/ServerComponentTypes.tspackages/react-start-rsc/src/createCompositeComponent.tspackages/react-start-rsc/src/global.d.tspackages/react-start-rsc/src/react-server-dom-rspack.d.tspackages/react-start-rsc/src/renderServerComponent.tspackages/react-start-rsc/src/rsbuild/browser-decode.tspackages/react-start-rsc/src/rsbuild/ssr-decode.tspackages/react-start-rsc/vite.config.tspackages/react-start/package.jsonpackages/react-start/src/plugin/rsbuild.tspackages/react-start/vite.config.tspackages/router-plugin/package.jsonpackages/router-utils/src/ast.tspackages/router-utils/tests/stripTypeExports.test.tspackages/solid-start/package.jsonpackages/solid-start/src/plugin/rsbuild.tspackages/solid-start/vite.config.tspackages/start-plugin-core/package.jsonpackages/start-plugin-core/src/import-protection-plugin/INTERNALS.mdpackages/start-plugin-core/src/import-protection-plugin/postCompileUsage.tspackages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.tspackages/start-plugin-core/src/import-protection/INTERNALS.mdpackages/start-plugin-core/src/import-protection/adapterUtils.tspackages/start-plugin-core/src/import-protection/analysis.tspackages/start-plugin-core/src/import-protection/ast.tspackages/start-plugin-core/src/import-protection/constants.tspackages/start-plugin-core/src/import-protection/defaults.tspackages/start-plugin-core/src/import-protection/extensionlessAbsoluteIdResolver.tspackages/start-plugin-core/src/import-protection/matchers.tspackages/start-plugin-core/src/import-protection/rewrite.tspackages/start-plugin-core/src/import-protection/sourceLocation.tspackages/start-plugin-core/src/import-protection/trace.tspackages/start-plugin-core/src/import-protection/utils.tspackages/start-plugin-core/src/import-protection/virtualModules.tspackages/start-plugin-core/src/index.tspackages/start-plugin-core/src/post-build.tspackages/start-plugin-core/src/prerender.tspackages/start-plugin-core/src/rsbuild/INTERNALS-import-protection.mdpackages/start-plugin-core/src/rsbuild/dev-server.tspackages/start-plugin-core/src/rsbuild/import-protection.tspackages/start-plugin-core/src/rsbuild/normalized-client-build.tspackages/start-plugin-core/src/rsbuild/planning.tspackages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/src/rsbuild/post-build.tspackages/start-plugin-core/src/rsbuild/schema.tspackages/start-plugin-core/src/rsbuild/start-compiler-host.tspackages/start-plugin-core/src/rsbuild/start-router-plugin.tspackages/start-plugin-core/src/rsbuild/types.tspackages/start-plugin-core/src/rsbuild/virtual-modules.tspackages/start-plugin-core/src/start-compiler/load-module.tspackages/start-plugin-core/src/vite/import-protection-plugin/INTERNALS.mdpackages/start-plugin-core/src/vite/import-protection-plugin/plugin.tspackages/start-plugin-core/src/vite/import-protection-plugin/types.tspackages/start-plugin-core/src/vite/import-protection-plugin/virtualModules.tspackages/start-plugin-core/src/vite/plugin.tspackages/start-plugin-core/src/vite/post-server-build.tspackages/start-plugin-core/src/vite/prerender.tspackages/start-plugin-core/src/vite/start-compiler-plugin/plugin.tspackages/start-plugin-core/tests/importProtection/analysis.exports.test.tspackages/start-plugin-core/tests/importProtection/analysis.usage.test.tspackages/start-plugin-core/tests/importProtection/defaults.test.tspackages/start-plugin-core/tests/importProtection/matchers.test.tspackages/start-plugin-core/tests/importProtection/rewrite.test.tspackages/start-plugin-core/tests/importProtection/sourceLocation.test.tspackages/start-plugin-core/tests/importProtection/trace.test.tspackages/start-plugin-core/tests/importProtection/utils.test.tspackages/start-plugin-core/tests/importProtection/virtualModules.test.tspackages/start-plugin-core/tests/post-server-build.test.tspackages/start-plugin-core/tests/prerender-ssrf.test.tspackages/start-plugin-core/tests/rsbuild-output-directory.test.tspackages/start-plugin-core/tests/rsbuild-post-build.test.tspackages/vue-start/package.jsonpackages/vue-start/src/plugin/rsbuild.tspackages/vue-start/vite.config.ts
💤 Files with no reviewable changes (7)
- e2e/react-start/basic-rsc/.devcontainer/devcontainer.json
- packages/start-plugin-core/src/import-protection/trace.ts
- packages/react-start-rsc/src/renderServerComponent.ts
- packages/react-start-rsc/src/ServerComponentTypes.ts
- packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md
- packages/start-plugin-core/src/import-protection-plugin/postCompileUsage.ts
- packages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.ts
| "dev": "sh -c 'if [ \"${E2E_TOOLCHAIN:-vite}\" = \"rsbuild\" ]; then pnpm exec rsbuild dev --port 3000; else pnpm exec vite dev --port 3000; fi'", | ||
| "dev:e2e": "pnpm dev", | ||
| "build": "sh -c 'if [ \"${E2E_TOOLCHAIN:-vite}\" = \"rsbuild\" ]; then pnpm exec rsbuild build && tsc --noEmit; else vite build && tsc --noEmit; fi'", | ||
| "start": "node -e 'import(\"./server.js\").then(m => process.stdout.write(m.resolveStartCommand()))' | sh", |
Contributor
There was a problem hiding this comment.
Fragile command piping pattern.
The start script pipes node -e output directly to sh. If server.js or any imported module emits unexpected stdout (e.g., debug logs, warnings), it will be interpreted as shell commands, potentially causing failures or unexpected behavior.
Consider using a more robust approach:
🛠️ Suggested alternative
- "start": "node -e 'import(\"./server.js\").then(m => process.stdout.write(m.resolveStartCommand()))' | sh",
+ "start": "node --eval 'import(\"./server.js\").then(m => console.log(m.resolveStartCommand()))' 2>/dev/null | sh",Or extract to a dedicated script file that executes directly rather than piping to shell.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/import-protection/package.json` at line 10, The current
"start" script pipes the raw stdout of node -e importing server.js (and calling
resolveStartCommand()) directly into sh, which is fragile if server.js prints
any extra output; change this by replacing the one-liner with a dedicated script
(e.g., start-server.js) that imports ./server.js, calls resolveStartCommand(),
validates/escapes the returned string, and then executes it via a safe API
(child_process.exec/execFile/spawn) or writes a JSON payload to stdout for a
wrapper to parse; update the package.json "start" to run that dedicated script
instead.
Comment on lines
+10
to
+22
| function resolveDistServerEntryPath() { | ||
| const serverJsPath = path.resolve(distDir, 'server', 'server.js') | ||
| if (fs.existsSync(serverJsPath)) { | ||
| return serverJsPath | ||
| } | ||
|
|
||
| const indexJsPath = path.resolve(distDir, 'server', 'index.js') | ||
| if (fs.existsSync(indexJsPath)) { | ||
| return indexJsPath | ||
| } | ||
|
|
||
| return serverJsPath | ||
| } |
Contributor
There was a problem hiding this comment.
Fallback returns potentially non-existent path.
When neither server.js nor index.js exists in the server directory, line 21 returns serverJsPath anyway. This will cause a confusing runtime error from srvx rather than a clear error message here.
Consider either throwing an error or logging a warning when no valid server entry is found:
🛠️ Suggested improvement
const indexJsPath = path.resolve(distDir, 'server', 'index.js')
if (fs.existsSync(indexJsPath)) {
return indexJsPath
}
- return serverJsPath
+ console.warn(`Warning: No server entry found at ${serverJsPath} or ${indexJsPath}`)
+ return serverJsPath📝 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.
Suggested change
| function resolveDistServerEntryPath() { | |
| const serverJsPath = path.resolve(distDir, 'server', 'server.js') | |
| if (fs.existsSync(serverJsPath)) { | |
| return serverJsPath | |
| } | |
| const indexJsPath = path.resolve(distDir, 'server', 'index.js') | |
| if (fs.existsSync(indexJsPath)) { | |
| return indexJsPath | |
| } | |
| return serverJsPath | |
| } | |
| function resolveDistServerEntryPath() { | |
| const serverJsPath = path.resolve(distDir, 'server', 'server.js') | |
| if (fs.existsSync(serverJsPath)) { | |
| return serverJsPath | |
| } | |
| const indexJsPath = path.resolve(distDir, 'server', 'index.js') | |
| if (fs.existsSync(indexJsPath)) { | |
| return indexJsPath | |
| } | |
| console.warn(`Warning: No server entry found at ${serverJsPath} or ${indexJsPath}`) | |
| return serverJsPath | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/import-protection/server.js` around lines 10 - 22, The
function resolveDistServerEntryPath currently returns serverJsPath even when
neither server.js nor index.js exists; update resolveDistServerEntryPath to
detect the missing entry and throw a clear error (or log and throw) instead of
returning a non-existent path: after checking serverJsPath and indexJsPath, if
neither exists use distDir, serverJsPath and indexJsPath in the error message to
make it clear which files were expected and where, and ensure any caller of
resolveDistServerEntryPath will receive the thrown error rather than a bogus
path.
| @@ -0,0 +1,13 @@ | |||
| export function getStartModeConfig() { | |||
| const behavior = (process.env.BEHAVIOR ?? 'mock') as 'mock' | 'error' | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no unsafe assertion remains for BEHAVIOR union narrowing
rg -nP "process\.env\.BEHAVIOR.*as\s*'mock'\s*\|\s*'error'" -C2Repository: TanStack/router
Length of output: 973
Replace the type assertion with runtime validation for BEHAVIOR.
Line 2 uses as 'mock' | 'error' to force-cast any string, allowing invalid env values to silently pass through. Implement proper runtime narrowing instead.
Suggested fix
export function getStartModeConfig() {
- const behavior = (process.env.BEHAVIOR ?? 'mock') as 'mock' | 'error'
+ const rawBehavior = process.env.BEHAVIOR
+ const behavior: 'mock' | 'error' =
+ rawBehavior === 'error' || rawBehavior === 'mock' ? rawBehavior : 'mock'📝 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.
Suggested change
| const behavior = (process.env.BEHAVIOR ?? 'mock') as 'mock' | 'error' | |
| export function getStartModeConfig() { | |
| const rawBehavior = process.env.BEHAVIOR | |
| const behavior: 'mock' | 'error' = | |
| rawBehavior === 'error' || rawBehavior === 'mock' ? rawBehavior : 'mock' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/import-protection/start-mode-config.ts` at line 2, The
current line force-casts process.env.BEHAVIOR with "as 'mock' | 'error'", which
allows invalid values; replace it with runtime validation: implement a small
validator (e.g., isBehavior or a Set of allowed values) that checks
process.env.BEHAVIOR === 'mock' || === 'error' and only then assigns it to the
behavior const, otherwise fall back to the default 'mock' (or throw if you
prefer stricter behavior); update the variable declaration for behavior to use
that validated value instead of the type assertion so invalid env values are
handled safely.
Comment on lines
+32
to
+36
| const stripBoxPrefix = (line: string) => | ||
| line | ||
| .replace(/^\s*│\s?/, '') | ||
| .replace(/^\s*\|\s?/, '') | ||
| .trimEnd() |
Contributor
There was a problem hiding this comment.
Preserve code-gutter lines when stripping | prefixes
Line 35 removes any leading |, which can also strip snippet continuation lines (e.g. | ^) in non-boxed logs and break snippet parsing.
Proposed fix
- const stripBoxPrefix = (line: string) =>
- line
- .replace(/^\s*│\s?/, '')
- .replace(/^\s*\|\s?/, '')
- .trimEnd()
+ const stripBoxPrefix = (line: string) => {
+ const stripped = line.replace(/^\s*│\s?/, '').trimEnd()
+ // Strip ASCII border prefixes, but keep code-gutter continuation lines.
+ return /^\s*\|\s+(?:Importer:|Import:|Resolved:|Denied by|Trace:|Code:|\d+\.|[> ]\s*\d+\s*\|)/.test(
+ stripped,
+ )
+ ? stripped.replace(/^\s*\|\s?/, '').trimEnd()
+ : stripped
+ }📝 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.
Suggested change
| const stripBoxPrefix = (line: string) => | |
| line | |
| .replace(/^\s*│\s?/, '') | |
| .replace(/^\s*\|\s?/, '') | |
| .trimEnd() | |
| const stripBoxPrefix = (line: string) => { | |
| const stripped = line.replace(/^\s*│\s?/, '').trimEnd() | |
| // Strip ASCII border prefixes, but keep code-gutter continuation lines. | |
| return /^\s*\|\s+(?:Importer:|Import:|Resolved:|Denied by|Trace:|Code:|\d+\.|[> ]\s*\d+\s*\|)/.test( | |
| stripped, | |
| ) | |
| ? stripped.replace(/^\s*\|\s?/, '').trimEnd() | |
| : stripped | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-start/import-protection/tests/violations.utils.ts` around lines 32
- 36, The stripBoxPrefix function is over-greedy: its /^\s*\|\s?/ pattern
removes leading '|' even when it's part of code-gutter lines like " | ^";
update the logic in stripBoxPrefix so it only strips a pipe used as a true line
prefix (e.g. at column 0) rather than any pipe after leading whitespace — change
the pipe removal from /^\s*\|\s?/ to a stricter match (e.g. /^\|\s?/) or
equivalent check so code-gutter continuation lines are preserved while still
removing box-border pipes and the existing '│' handling remains intact.
Comment on lines
+10
to
+14
| let queryURL = 'https://jsonplaceholder.typicode.com' | ||
|
|
||
| if (import.meta.env.VITE_NODE_ENV === 'test') { | ||
| queryURL = `http://localhost:${import.meta.env.VITE_EXTERNAL_PORT}` | ||
| } |
Contributor
There was a problem hiding this comment.
Fail fast when test port is missing instead of building an invalid URL.
Line 13 can silently become http://localhost:undefined when VITE_EXTERNAL_PORT is absent, which makes E2E failures harder to diagnose. Prefer immutable URL construction with explicit validation.
Suggested patch
-let queryURL = 'https://jsonplaceholder.typicode.com'
-
-if (import.meta.env.VITE_NODE_ENV === 'test') {
- queryURL = `http://localhost:${import.meta.env.VITE_EXTERNAL_PORT}`
-}
+const queryURL =
+ import.meta.env.VITE_NODE_ENV === 'test'
+ ? (() => {
+ const port = import.meta.env.VITE_EXTERNAL_PORT
+ if (!port) {
+ throw new Error(
+ 'Missing VITE_EXTERNAL_PORT while VITE_NODE_ENV is "test"',
+ )
+ }
+ return `http://localhost:${port}`
+ })()
+ : 'https://jsonplaceholder.typicode.com'As per coding guidelines: **/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety.
📝 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.
Suggested change
| let queryURL = 'https://jsonplaceholder.typicode.com' | |
| if (import.meta.env.VITE_NODE_ENV === 'test') { | |
| queryURL = `http://localhost:${import.meta.env.VITE_EXTERNAL_PORT}` | |
| } | |
| const queryURL = | |
| import.meta.env.VITE_NODE_ENV === 'test' | |
| ? (() => { | |
| const port = import.meta.env.VITE_EXTERNAL_PORT | |
| if (!port) { | |
| throw new Error( | |
| 'Missing VITE_EXTERNAL_PORT while VITE_NODE_ENV is "test"', | |
| ) | |
| } | |
| return `http://localhost:${port}` | |
| })() | |
| : 'https://jsonplaceholder.typicode.com' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/vue-start/basic-vue-query/src/utils/posts.tsx` around lines 10 - 14, The
code builds a mutable queryURL and can produce "http://localhost:undefined" when
VITE_EXTERNAL_PORT is missing; change to immutable URL construction and validate
the test port: when import.meta.env.VITE_NODE_ENV === 'test' verify
import.meta.env.VITE_EXTERNAL_PORT is present (and optionally parseInt/validate
it's a number) and throw a clear error if not, otherwise construct a const
testBase = `http://localhost:${port}` and assign queryURL to that value (do not
mutate a top-level let); reference the queryURL variable and the
import.meta.env.VITE_EXTERNAL_PORT check in posts.tsx so failures fail fast with
an explicit error message.
Comment on lines
4
to
+7
| isValidExportName, | ||
| } from '../../src/import-protection-plugin/rewriteDeniedImports' | ||
| getMockExportNamesBySource, | ||
| getNamedExports, | ||
| } from '../../src/import-protection/analysis' |
Contributor
There was a problem hiding this comment.
Fix import member order to satisfy sort-imports.
Line 5 is currently out of the expected alphabetical member order for this import declaration.
✅ Proposed fix
import {
- isValidExportName,
getMockExportNamesBySource,
getNamedExports,
+ isValidExportName,
} from '../../src/import-protection/analysis'📝 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.
Suggested change
| isValidExportName, | |
| } from '../../src/import-protection-plugin/rewriteDeniedImports' | |
| getMockExportNamesBySource, | |
| getNamedExports, | |
| } from '../../src/import-protection/analysis' | |
| import { | |
| getMockExportNamesBySource, | |
| getNamedExports, | |
| isValidExportName, | |
| } from '../../src/import-protection/analysis' |
🧰 Tools
🪛 ESLint
[error] 5-5: Member 'getMockExportNamesBySource' of the import declaration should be sorted alphabetically.
(sort-imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/tests/importProtection/analysis.exports.test.ts`
around lines 4 - 7, The import specifier list for the import from
'../../src/import-protection/analysis' is not alphabetically sorted and fails
the sort-imports rule; reorder the named imports so they are alphabetically
sorted (getMockExportNamesBySource, getNamedExports, isValidExportName) in the
import statement that currently lists isValidExportName,
getMockExportNamesBySource, getNamedExports.
| } from '../../src/import-protection-plugin/utils' | ||
| } from '../../src/import-protection/utils' | ||
| import { compileMatchers } from '../../src/import-protection/matchers' | ||
| import { formatViolation } from '../../src/import-protection/trace' |
Contributor
There was a problem hiding this comment.
Remove unused import formatViolation.
Static analysis indicates formatViolation is imported but never used in this test file.
🧹 Proposed fix
-import { formatViolation } from '../../src/import-protection/trace'📝 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.
Suggested change
| import { formatViolation } from '../../src/import-protection/trace' |
🧰 Tools
🪛 ESLint
[error] 23-23: 'formatViolation' is defined but never used.
(unused-imports/no-unused-imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/tests/importProtection/utils.test.ts` at line 23,
The import of formatViolation in
packages/start-plugin-core/tests/importProtection/utils.test.ts is unused;
remove formatViolation from the import statement (the import line that currently
reads "import { formatViolation } from '../../src/import-protection/trace'") so
the test file no longer imports that symbol.
Comment on lines
+1
to
+4
| import { describe, expect, it, vi } from 'vitest' | ||
| import { mkdtemp, rm, writeFile } from 'node:fs/promises' | ||
| import { tmpdir } from 'node:os' | ||
| import { join } from 'pathe' |
Contributor
There was a problem hiding this comment.
Fix import ordering to satisfy import/order.
This import block order will trigger the lint rule in this file.
Suggested fix
-import { describe, expect, it, vi } from 'vitest'
import { mkdtemp, rm, writeFile } from 'node:fs/promises'
import { tmpdir } from 'node:os'
import { join } from 'pathe'
+import { describe, expect, it, vi } from 'vitest'📝 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.
Suggested change
| import { describe, expect, it, vi } from 'vitest' | |
| import { mkdtemp, rm, writeFile } from 'node:fs/promises' | |
| import { tmpdir } from 'node:os' | |
| import { join } from 'pathe' | |
| import { mkdtemp, rm, writeFile } from 'node:fs/promises' | |
| import { tmpdir } from 'node:os' | |
| import { join } from 'pathe' | |
| import { describe, expect, it, vi } from 'vitest' |
🧰 Tools
🪛 ESLint
[error] 1-1: vitest import should occur after import of node:os
(import/order)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/tests/rsbuild-post-build.test.ts` around lines 1 -
4, Reorder the import block to satisfy the import/order rule by grouping
built-in modules first, then external packages: move "import { mkdtemp, rm,
writeFile } from 'node:fs/promises'" and "import { tmpdir } from 'node:os'" to
appear before external imports, then "import { join } from 'pathe'" and finally
the test helpers "import { describe, expect, it, vi } from 'vitest'"; ensure
imports are consistently ordered (and optionally alphabetized) within each group
so symbols like mkdtemp, rm, writeFile, tmpdir, join, describe, expect, it, vi
are easy to locate.
Contributor
🚀 Changeset Version Preview6 package(s) bumped directly, 4 bumped as dependents. 🟨 Minor bumps
🟩 Patch bumps
|
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/start-plugin-core/src/import-protection/sourceLocation.ts (1)
348-353:⚠️ Potential issue | 🟡 MinorRemove the raw
includesshort-circuit.This reintroduces a text scan ahead of the parser-based resolver, so valid specifiers can still be missed when the emitted string literal is escaped or rewritten differently.
getImportSpecifierLocationFromResult()should stay the source of truth here.Proposed change
return { find(result: TransformResult, source: string): number { - if (!result.code.includes(source)) { - return -1 - } - return getImportSpecifierLocationFromResult(result, source) }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/import-protection/sourceLocation.ts` around lines 348 - 353, The short-circuit text-scan using result.code.includes(source) in the find method should be removed so getImportSpecifierLocationFromResult remains the single source of truth; update the find(TransformResult, source) implementation to always call and return getImportSpecifierLocationFromResult(result, source) (and not pre-check with includes), ensuring no manual string-based checks are performed before delegating to the parser-based resolver.packages/start-plugin-core/tests/rsbuild-post-build.test.ts (1)
1-4:⚠️ Potential issue | 🟡 MinorFix import ordering to satisfy
import/order.Node.js built-in modules should be imported before external packages.
📦 Suggested reordering
-import { describe, expect, it, vi } from 'vitest' import { mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'pathe' +import { describe, expect, it, vi } from 'vitest'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/tests/rsbuild-post-build.test.ts` around lines 1 - 4, Reorder the imports in the test so Node.js built-in modules are imported before external packages: move "import { mkdtemp, rm, writeFile } from 'node:fs/promises'" and "import { tmpdir } from 'node:os'" to appear before the external imports ("import { join } from 'pathe'" and "import { describe, expect, it, vi } from 'vitest'"); keep the same imported symbols (mkdtemp, rm, writeFile, tmpdir, join, describe, expect, it, vi) but adjust the import order to satisfy import/order.
🧹 Nitpick comments (11)
e2e/react-start/import-protection/tests/error-mode.setup.ts (1)
126-126: Remove redundant shadowed variable.The local
toolchainon line 126 shadows the identical module-level variable from line 22. Use the module-leveltoolchaindirectly for consistency with howe2ePortKeyis used.♻️ Proposed fix
const out = fs.createWriteStream(logFile) - const toolchain = process.env.E2E_TOOLCHAIN ?? 'vite' const command = toolchain === 'rsbuild'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-start/import-protection/tests/error-mode.setup.ts` at line 126, Remove the redundant local declaration of toolchain in error-mode.setup.ts (the const toolchain on line 126) which shadows the module-level toolchain; instead, delete that local const and reference the existing module-level variable wherever that local was used (same pattern as e2ePortKey) so the file consistently uses the single module-level toolchain variable.packages/solid-start/src/plugin/rsbuild.ts (1)
51-74: Consider adding a type annotation forbabelOptionsto improve type clarity.The
babelOptionsparameter relies on implicit typing from thetapcallback. While webpack-chain's types are loose by design, an explicit type annotation would improve readability.♻️ Suggested type annotation
- rule.use(CHAIN_ID.USE.BABEL).tap((babelOptions) => { + rule.use(CHAIN_ID.USE.BABEL).tap((babelOptions: { presets?: Array<unknown> }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/solid-start/src/plugin/rsbuild.ts` around lines 51 - 74, Add an explicit type annotation for the tap callback parameter `babelOptions` to improve clarity: in the `rule.use(CHAIN_ID.USE.BABEL).tap(...)` callback annotate `babelOptions` (used where you access `babelOptions.presets` and map presets) with an appropriate Babel type such as `TransformOptions` (imported from '@babel/core') or your project's babel options type so the presets mapping is typed and clearer to readers.packages/start-plugin-core/src/import-protection/sourceLocation.ts (1)
335-340: Make the synthesized original result terminal.The object created on Lines 336-340 keeps
originalCode, so callinggetOrCreateOriginalTransformResult()on that returned value creates another wrapper instead of reusing the same result. ClearingoriginalCodehere keeps the helper idempotent and avoids unbounded nesting.Proposed change
result.originalResult = { code: result.originalCode, map: undefined, - originalCode: result.originalCode, + originalCode: undefined, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/import-protection/sourceLocation.ts` around lines 335 - 340, The synthesized originalResult object assigned when result.originalResult is missing keeps originalCode set, which causes getOrCreateOriginalTransformResult() to wrap it again; update the creation of the fallback object (the block that sets result.originalResult = { code: result.originalCode, map: undefined, originalCode: result.originalCode }) to clear or omit originalCode (e.g., set originalCode: undefined) so the synthesized original result is terminal and the helper becomes idempotent.packages/start-plugin-core/src/rsbuild/virtual-modules.ts (1)
68-74: Consider escaping thedevClientEntryUrlin the generated module.The
devClientEntryUrlis interpolated directly into a template string. If it contains special characters (unlikely but possible with custom configurations), it could break the generated JavaScript.Suggested fix
function generateManifestModuleDev(devClientEntryUrl: string): string { return `const fallbackManifest = { routes: {}, - clientEntry: '${devClientEntryUrl}', + clientEntry: ${JSON.stringify(devClientEntryUrl)}, } export const tsrStartManifest = () => globalThis[${JSON.stringify(DEV_START_MANIFEST_GLOBAL)}] ?? fallbackManifest` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/rsbuild/virtual-modules.ts` around lines 68 - 74, The template in generateManifestModuleDev interpolates devClientEntryUrl directly which can break emitted JS if it contains special characters; update generateManifestModuleDev to safely escape/serialize devClientEntryUrl when embedding it (e.g. use JSON.stringify(devClientEntryUrl) or an equivalent serializer) so the generated module assigns clientEntry to a properly quoted/escaped string, keeping DEV_START_MANIFEST_GLOBAL usage unchanged.packages/start-plugin-core/src/prerender.ts (1)
114-116: MutatingstartConfig.pagesduring crawl could cause issues.When
page.fromCrawlis true, the page is pushed directly tostartConfig.pages. This mutates the config object that was passed in, which may have unintended side effects if the caller expects the config to remain unchanged.Consider using a local array for crawled pages
async function prerenderPages({ outputDir }: { outputDir: string }) { const seen = new Set<string>() const prerendered = new Set<string>() + const crawledPages: Array<Page> = [] // ... existing code ... function addCrawlPageTask(page: Page) { if (seen.has(page.path)) return seen.add(page.path) if (page.fromCrawl) { - startConfig.pages.push(page) + crawledPages.push(page) }Then merge
crawledPagesback if needed after prerendering completes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/prerender.ts` around lines 114 - 116, The code mutates the incoming config by pushing crawled pages directly into startConfig.pages when page.fromCrawl is true; instead, collect crawled pages into a local array (e.g., crawledPages) inside the prerender routine and avoid modifying startConfig.pages during the crawl, then merge crawledPages back into the config only after prerendering completes (or return them separately); update references to startConfig.pages and the logic that checks page.fromCrawl so it uses the local crawledPages array until the final merge.packages/start-plugin-core/src/vite/prerender.ts (1)
41-43: Consider addingredirect: 'manual'for consistency with Rsbuild adapter.The Rsbuild adapter in
post-build.ts(line 62) addsredirect: 'manual'to prevent fetch from auto-following redirects, allowing the prerender logic to handle them. The Vite adapter doesn't include this, which could lead to inconsistent behavior between bundlers.Suggested fix
request(path, options) { const url = new URL(path, baseUrl) - return fetch(new Request(url, options)) + return fetch(new Request(url, { + ...options, + redirect: 'manual', + })) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/vite/prerender.ts` around lines 41 - 43, The request method in prerender.ts currently constructs a Request with new Request(url, options) but doesn't set redirect mode; update the Request creation in request(path, options) to ensure redirect: 'manual' is applied (e.g., merge options with redirect: 'manual' so existing options are preserved) before calling fetch, to match the Rsbuild adapter behavior and let the prerender logic handle redirects; reference the request function and the baseUrl/Request construction when making the change.packages/start-plugin-core/src/rsbuild/planning.ts (2)
79-85: Guard against missingreact-server-dom-rspackwhen RSC is enabled.
require.resolve('react-server-dom-rspack/server.node')will throw if the package isn't installed. Since RSC is optional, this could cause confusing errors whenrsc: trueis passed but the package is missing.Suggested improvement
Consider wrapping with a try-catch or documenting the peer dependency requirement:
...(opts.rsc ? { 'react-server-dom-rspack/server$': - require.resolve('react-server-dom-rspack/server.node'), + (() => { + try { + return require.resolve('react-server-dom-rspack/server.node') + } catch { + throw new Error( + 'react-server-dom-rspack is required when RSC is enabled. Please install it as a dependency.', + ) + } + })(), } : {}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/rsbuild/planning.ts` around lines 79 - 85, When opts.rsc is true, the inline require.resolve('react-server-dom-rspack/server.node') can throw if that package isn't installed; wrap the resolution in a try-catch around the expression used in the object spread (or extract into a helper like resolveRscServerModule) and on failure either omit the mapping (so RSC is effectively disabled) or throw a clear, actionable error message that mentions the missing peer dependency and how to install it; reference opts.rsc and the 'react-server-dom-rspack/server.node' resolution in your change so the mapping is guarded and fails with a descriptive message instead of an unhandled exception.
340-345: Clarify the inverted boolean logic forincludeClientReferencedCheck.The expression
!(opts.rscEnabled ? true : opts.ssrIsProvider)is difficult to parse. WhenrscEnabledis true, this evaluates tofalse. WhenrscEnabledis false, it evaluates to!ssrIsProvider.Suggested clarification
- includeClientReferencedCheck: !(opts.rscEnabled - ? true - : opts.ssrIsProvider), + // Skip client-referenced checks when RSC is enabled or when SSR is the provider + includeClientReferencedCheck: !opts.rscEnabled && !opts.ssrIsProvider,This applies to lines 342-344 in this file. The same pattern appears in
virtual-modules.tsat lines 342-344 and 514-516.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/rsbuild/planning.ts` around lines 340 - 345, The inverted boolean expression for includeClientReferencedCheck is hard to read; replace the negated ternary "!(opts.rscEnabled ? true : opts.ssrIsProvider)" with an explicit condition that reads the intent: includeClientReferencedCheck should be true only when opts.rscEnabled is false AND opts.ssrIsProvider is false (i.e., assign includeClientReferencedCheck = !opts.rscEnabled && !opts.ssrIsProvider using the includeClientReferencedCheck identifier and opts.rscEnabled/opts.ssrIsProvider symbols), and apply the same replacement in the corresponding occurrences in virtual-modules.ts.packages/start-plugin-core/src/rsbuild/start-compiler-host.ts (1)
112-117: TheloadModulecallback has a self-reference that could be confusing.The
compiler!non-null assertion inside theloadModulecallback references the outercompilervariable, which is assigned on line 143 after this closure is created. While this works becauseloadModuleis called asynchronously aftercompileris set, it's a subtle temporal dependency.Consider using a getter pattern for clarity
+ const getCompiler = () => compiler! loadModule: async (moduleId: string) => { await loadModuleForRsbuildCompiler({ - compiler: compiler!, + compiler: getCompiler(), id: moduleId, }) },This makes the deferred access pattern more explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/rsbuild/start-compiler-host.ts` around lines 112 - 117, The loadModule callback captures the outer variable compiler with a non-null assertion (compiler!) creating a subtle temporal dependency; change loadModule to read the latest compiler via a getter or accessor at call time (e.g., create a getCompiler() helper that throws if compiler is unset) and pass that result into loadModuleForRsbuildCompiler instead of using compiler! directly; update the loadModule implementation and reference loadModuleForRsbuildCompiler, loadModule, and the compiler variable accordingly so the deferred access is explicit and safe.packages/start-plugin-core/src/rsbuild/post-build.ts (1)
45-74: Consider adding error handling for handler loading failures.If
loadRequestHandlerfails (e.g., server bundle not found, syntax error), the error will propagate without context. Adding a try-catch with a descriptive error would improve debuggability.Suggested improvement
function getRequestHandler() { if (!requestHandlerPromise) { - requestHandlerPromise = loadRequestHandler(serverOutputDirectory) + requestHandlerPromise = loadRequestHandler(serverOutputDirectory).catch( + (error) => { + throw new Error( + `Failed to load Rsbuild server bundle for prerendering`, + { cause: error }, + ) + }, + ) } return requestHandlerPromise }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/rsbuild/post-build.ts` around lines 45 - 74, The getRequestHandler function currently assigns requestHandlerPromise = loadRequestHandler(serverOutputDirectory) without handling failures; wrap the loadRequestHandler call in a try/catch (or attach a .catch) so any load error is rethrown with a clear, descriptive message including serverOutputDirectory and the original error (reference symbols: getRequestHandler, requestHandlerPromise, loadRequestHandler, serverOutputDirectory) and ensure the rejected state is preserved so callers of request() receive the enhanced error.packages/start-plugin-core/tests/rsbuild-post-build.test.ts (1)
39-58: Consider adding explicit mock cleanup for test isolation.While Vitest automatically cleans up mocks between test files, explicitly resetting the module cache ensures this test won't affect others if additional tests are added to this suite later.
🧹 Proposed addition for mock cleanup
} finally { await rm(serverOutputDirectory, { recursive: true, force: true }) + vi.resetModules() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/tests/rsbuild-post-build.test.ts` around lines 39 - 58, Add explicit mock/module cleanup to this test by resetting Vitest's module cache and mocks after the test run: call vi.resetModules() (to clear the module cache) and vi.resetAllMocks() or vi.restoreAllMocks() (to clear spies/mocks such as prerenderSpy) either in the test's finally block after the rm(...) cleanup or in an afterEach for the file; this ensures imports like postBuildWithRsbuild and spies on prerenderSpy can't leak into other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/start-plugin-core/src/prerender.ts`:
- Around line 44-50: The call to validateAndNormalizePrerenderPages is being
performed twice: once when assigning startConfig.pages and again inside the
prerenderPages function; remove the redundant second call inside prerenderPages
(the validateAndNormalizePrerenderPages invocation within prerenderPages) and
rely on the already-normalized startConfig.pages, ensuring any code in
prerenderPages uses startConfig.pages (or its local parameter) directly without
re-validating.
In `@packages/start-plugin-core/tests/importProtection/sourceLocation.test.ts`:
- Around line 2-16: The import list in sourceLocation.test.ts is not
alphabetically ordered per eslint sort-imports; move the ImportLocCache
specifier so that it appears after buildCodeSnippet (i.e., reorder the named
imports in the import statement to: createImportSpecifierLocationIndex,
buildCodeSnippet, ImportLocCache, buildLineIndex, findOriginalUsageLocation,
getOrCreateOriginalTransformResult, normalizeSourceMap,
pickOriginalCodeFromSourcesContent) to satisfy the rule and keep the rest of the
specifiers unchanged.
---
Duplicate comments:
In `@packages/start-plugin-core/src/import-protection/sourceLocation.ts`:
- Around line 348-353: The short-circuit text-scan using
result.code.includes(source) in the find method should be removed so
getImportSpecifierLocationFromResult remains the single source of truth; update
the find(TransformResult, source) implementation to always call and return
getImportSpecifierLocationFromResult(result, source) (and not pre-check with
includes), ensuring no manual string-based checks are performed before
delegating to the parser-based resolver.
In `@packages/start-plugin-core/tests/rsbuild-post-build.test.ts`:
- Around line 1-4: Reorder the imports in the test so Node.js built-in modules
are imported before external packages: move "import { mkdtemp, rm, writeFile }
from 'node:fs/promises'" and "import { tmpdir } from 'node:os'" to appear before
the external imports ("import { join } from 'pathe'" and "import { describe,
expect, it, vi } from 'vitest'"); keep the same imported symbols (mkdtemp, rm,
writeFile, tmpdir, join, describe, expect, it, vi) but adjust the import order
to satisfy import/order.
---
Nitpick comments:
In `@e2e/react-start/import-protection/tests/error-mode.setup.ts`:
- Line 126: Remove the redundant local declaration of toolchain in
error-mode.setup.ts (the const toolchain on line 126) which shadows the
module-level toolchain; instead, delete that local const and reference the
existing module-level variable wherever that local was used (same pattern as
e2ePortKey) so the file consistently uses the single module-level toolchain
variable.
In `@packages/solid-start/src/plugin/rsbuild.ts`:
- Around line 51-74: Add an explicit type annotation for the tap callback
parameter `babelOptions` to improve clarity: in the
`rule.use(CHAIN_ID.USE.BABEL).tap(...)` callback annotate `babelOptions` (used
where you access `babelOptions.presets` and map presets) with an appropriate
Babel type such as `TransformOptions` (imported from '@babel/core') or your
project's babel options type so the presets mapping is typed and clearer to
readers.
In `@packages/start-plugin-core/src/import-protection/sourceLocation.ts`:
- Around line 335-340: The synthesized originalResult object assigned when
result.originalResult is missing keeps originalCode set, which causes
getOrCreateOriginalTransformResult() to wrap it again; update the creation of
the fallback object (the block that sets result.originalResult = { code:
result.originalCode, map: undefined, originalCode: result.originalCode }) to
clear or omit originalCode (e.g., set originalCode: undefined) so the
synthesized original result is terminal and the helper becomes idempotent.
In `@packages/start-plugin-core/src/prerender.ts`:
- Around line 114-116: The code mutates the incoming config by pushing crawled
pages directly into startConfig.pages when page.fromCrawl is true; instead,
collect crawled pages into a local array (e.g., crawledPages) inside the
prerender routine and avoid modifying startConfig.pages during the crawl, then
merge crawledPages back into the config only after prerendering completes (or
return them separately); update references to startConfig.pages and the logic
that checks page.fromCrawl so it uses the local crawledPages array until the
final merge.
In `@packages/start-plugin-core/src/rsbuild/planning.ts`:
- Around line 79-85: When opts.rsc is true, the inline
require.resolve('react-server-dom-rspack/server.node') can throw if that package
isn't installed; wrap the resolution in a try-catch around the expression used
in the object spread (or extract into a helper like resolveRscServerModule) and
on failure either omit the mapping (so RSC is effectively disabled) or throw a
clear, actionable error message that mentions the missing peer dependency and
how to install it; reference opts.rsc and the
'react-server-dom-rspack/server.node' resolution in your change so the mapping
is guarded and fails with a descriptive message instead of an unhandled
exception.
- Around line 340-345: The inverted boolean expression for
includeClientReferencedCheck is hard to read; replace the negated ternary
"!(opts.rscEnabled ? true : opts.ssrIsProvider)" with an explicit condition that
reads the intent: includeClientReferencedCheck should be true only when
opts.rscEnabled is false AND opts.ssrIsProvider is false (i.e., assign
includeClientReferencedCheck = !opts.rscEnabled && !opts.ssrIsProvider using the
includeClientReferencedCheck identifier and opts.rscEnabled/opts.ssrIsProvider
symbols), and apply the same replacement in the corresponding occurrences in
virtual-modules.ts.
In `@packages/start-plugin-core/src/rsbuild/post-build.ts`:
- Around line 45-74: The getRequestHandler function currently assigns
requestHandlerPromise = loadRequestHandler(serverOutputDirectory) without
handling failures; wrap the loadRequestHandler call in a try/catch (or attach a
.catch) so any load error is rethrown with a clear, descriptive message
including serverOutputDirectory and the original error (reference symbols:
getRequestHandler, requestHandlerPromise, loadRequestHandler,
serverOutputDirectory) and ensure the rejected state is preserved so callers of
request() receive the enhanced error.
In `@packages/start-plugin-core/src/rsbuild/start-compiler-host.ts`:
- Around line 112-117: The loadModule callback captures the outer variable
compiler with a non-null assertion (compiler!) creating a subtle temporal
dependency; change loadModule to read the latest compiler via a getter or
accessor at call time (e.g., create a getCompiler() helper that throws if
compiler is unset) and pass that result into loadModuleForRsbuildCompiler
instead of using compiler! directly; update the loadModule implementation and
reference loadModuleForRsbuildCompiler, loadModule, and the compiler variable
accordingly so the deferred access is explicit and safe.
In `@packages/start-plugin-core/src/rsbuild/virtual-modules.ts`:
- Around line 68-74: The template in generateManifestModuleDev interpolates
devClientEntryUrl directly which can break emitted JS if it contains special
characters; update generateManifestModuleDev to safely escape/serialize
devClientEntryUrl when embedding it (e.g. use JSON.stringify(devClientEntryUrl)
or an equivalent serializer) so the generated module assigns clientEntry to a
properly quoted/escaped string, keeping DEV_START_MANIFEST_GLOBAL usage
unchanged.
In `@packages/start-plugin-core/src/vite/prerender.ts`:
- Around line 41-43: The request method in prerender.ts currently constructs a
Request with new Request(url, options) but doesn't set redirect mode; update the
Request creation in request(path, options) to ensure redirect: 'manual' is
applied (e.g., merge options with redirect: 'manual' so existing options are
preserved) before calling fetch, to match the Rsbuild adapter behavior and let
the prerender logic handle redirects; reference the request function and the
baseUrl/Request construction when making the change.
In `@packages/start-plugin-core/tests/rsbuild-post-build.test.ts`:
- Around line 39-58: Add explicit mock/module cleanup to this test by resetting
Vitest's module cache and mocks after the test run: call vi.resetModules() (to
clear the module cache) and vi.resetAllMocks() or vi.restoreAllMocks() (to clear
spies/mocks such as prerenderSpy) either in the test's finally block after the
rm(...) cleanup or in an afterEach for the file; this ensures imports like
postBuildWithRsbuild and spies on prerenderSpy can't leak into other tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 288ebc7d-db5f-4392-8e02-3abfee4526a0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (111)
.changeset/add-rsbuild-start-support.mddocs/start/framework/react/guide/import-protection.mde2e/.gitignoree2e/react-start/basic-rsc/.devcontainer/devcontainer.jsone2e/react-start/basic/.gitignoree2e/react-start/basic/package.jsone2e/react-start/basic/postcss.config.mjse2e/react-start/basic/rsbuild.config.tse2e/react-start/basic/server.jse2e/react-start/basic/src/routes/__root.tsxe2e/react-start/basic/start-mode-config.tse2e/react-start/basic/vite.config.tse2e/react-start/import-protection/package.jsone2e/react-start/import-protection/playwright.config.tse2e/react-start/import-protection/rsbuild.config.tse2e/react-start/import-protection/server.jse2e/react-start/import-protection/start-mode-config.tse2e/react-start/import-protection/tests/error-mode.setup.tse2e/react-start/import-protection/tests/import-protection.spec.tse2e/react-start/import-protection/tests/violations.setup.tse2e/react-start/import-protection/tests/violations.utils.tse2e/react-start/import-protection/vite.config.tse2e/react-start/serialization-adapters/src/routes/server-function/late-raw-stream.tsxe2e/solid-start/basic/package.jsone2e/solid-start/basic/postcss.config.mjse2e/solid-start/basic/rsbuild.config.tse2e/solid-start/basic/server.jse2e/solid-start/basic/src/routes/__root.tsxe2e/vue-start/basic-vue-query/src/utils/posts.tsxe2e/vue-start/basic/package.jsone2e/vue-start/basic/postcss.config.mjse2e/vue-start/basic/rsbuild.config.tse2e/vue-start/basic/server.jse2e/vue-start/basic/src/routes/__root.tsxpackages/react-router/src/Asset.tsxpackages/react-router/src/ClientOnly.tsxpackages/react-start-rsc/package.jsonpackages/react-start-rsc/src/ServerComponentTypes.tspackages/react-start-rsc/src/createCompositeComponent.tspackages/react-start-rsc/src/global.d.tspackages/react-start-rsc/src/react-server-dom-rspack.d.tspackages/react-start-rsc/src/renderServerComponent.tspackages/react-start-rsc/src/rsbuild/browser-decode.tspackages/react-start-rsc/src/rsbuild/ssr-decode.tspackages/react-start-rsc/vite.config.tspackages/react-start/package.jsonpackages/react-start/src/plugin/rsbuild.tspackages/react-start/vite.config.tspackages/router-plugin/package.jsonpackages/router-utils/src/ast.tspackages/router-utils/tests/stripTypeExports.test.tspackages/solid-start/package.jsonpackages/solid-start/src/plugin/rsbuild.tspackages/solid-start/vite.config.tspackages/start-plugin-core/package.jsonpackages/start-plugin-core/src/import-protection-plugin/INTERNALS.mdpackages/start-plugin-core/src/import-protection-plugin/postCompileUsage.tspackages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.tspackages/start-plugin-core/src/import-protection/INTERNALS.mdpackages/start-plugin-core/src/import-protection/adapterUtils.tspackages/start-plugin-core/src/import-protection/analysis.tspackages/start-plugin-core/src/import-protection/ast.tspackages/start-plugin-core/src/import-protection/constants.tspackages/start-plugin-core/src/import-protection/defaults.tspackages/start-plugin-core/src/import-protection/extensionlessAbsoluteIdResolver.tspackages/start-plugin-core/src/import-protection/matchers.tspackages/start-plugin-core/src/import-protection/rewrite.tspackages/start-plugin-core/src/import-protection/sourceLocation.tspackages/start-plugin-core/src/import-protection/trace.tspackages/start-plugin-core/src/import-protection/utils.tspackages/start-plugin-core/src/import-protection/virtualModules.tspackages/start-plugin-core/src/index.tspackages/start-plugin-core/src/post-build.tspackages/start-plugin-core/src/prerender.tspackages/start-plugin-core/src/rsbuild/INTERNALS-import-protection.mdpackages/start-plugin-core/src/rsbuild/dev-server.tspackages/start-plugin-core/src/rsbuild/import-protection.tspackages/start-plugin-core/src/rsbuild/normalized-client-build.tspackages/start-plugin-core/src/rsbuild/planning.tspackages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/src/rsbuild/post-build.tspackages/start-plugin-core/src/rsbuild/schema.tspackages/start-plugin-core/src/rsbuild/start-compiler-host.tspackages/start-plugin-core/src/rsbuild/start-router-plugin.tspackages/start-plugin-core/src/rsbuild/types.tspackages/start-plugin-core/src/rsbuild/virtual-modules.tspackages/start-plugin-core/src/start-compiler/load-module.tspackages/start-plugin-core/src/vite/import-protection-plugin/INTERNALS.mdpackages/start-plugin-core/src/vite/import-protection-plugin/plugin.tspackages/start-plugin-core/src/vite/import-protection-plugin/types.tspackages/start-plugin-core/src/vite/import-protection-plugin/virtualModules.tspackages/start-plugin-core/src/vite/plugin.tspackages/start-plugin-core/src/vite/post-server-build.tspackages/start-plugin-core/src/vite/prerender.tspackages/start-plugin-core/src/vite/start-compiler-plugin/plugin.tspackages/start-plugin-core/tests/importProtection/analysis.exports.test.tspackages/start-plugin-core/tests/importProtection/analysis.usage.test.tspackages/start-plugin-core/tests/importProtection/defaults.test.tspackages/start-plugin-core/tests/importProtection/matchers.test.tspackages/start-plugin-core/tests/importProtection/rewrite.test.tspackages/start-plugin-core/tests/importProtection/sourceLocation.test.tspackages/start-plugin-core/tests/importProtection/trace.test.tspackages/start-plugin-core/tests/importProtection/utils.test.tspackages/start-plugin-core/tests/importProtection/virtualModules.test.tspackages/start-plugin-core/tests/post-server-build.test.tspackages/start-plugin-core/tests/prerender-ssrf.test.tspackages/start-plugin-core/tests/rsbuild-output-directory.test.tspackages/start-plugin-core/tests/rsbuild-post-build.test.tspackages/vue-start/package.jsonpackages/vue-start/src/plugin/rsbuild.tspackages/vue-start/vite.config.ts
💤 Files with no reviewable changes (7)
- packages/react-start-rsc/src/renderServerComponent.ts
- e2e/react-start/basic-rsc/.devcontainer/devcontainer.json
- packages/start-plugin-core/src/import-protection/trace.ts
- packages/start-plugin-core/src/import-protection-plugin/INTERNALS.md
- packages/start-plugin-core/src/import-protection-plugin/postCompileUsage.ts
- packages/react-start-rsc/src/ServerComponentTypes.ts
- packages/start-plugin-core/src/import-protection-plugin/rewriteDeniedImports.ts
✅ Files skipped from review due to trivial changes (36)
- e2e/react-start/basic/.gitignore
- packages/react-router/src/ClientOnly.tsx
- .changeset/add-rsbuild-start-support.md
- packages/react-start-rsc/src/createCompositeComponent.ts
- packages/start-plugin-core/src/vite/plugin.ts
- packages/start-plugin-core/tests/importProtection/defaults.test.ts
- packages/react-router/src/Asset.tsx
- packages/vue-start/vite.config.ts
- e2e/react-start/basic/src/routes/__root.tsx
- e2e/vue-start/basic/postcss.config.mjs
- e2e/solid-start/basic/postcss.config.mjs
- packages/start-plugin-core/src/vite/start-compiler-plugin/plugin.ts
- packages/router-plugin/package.json
- packages/react-start/vite.config.ts
- e2e/solid-start/basic/rsbuild.config.ts
- packages/start-plugin-core/tests/importProtection/virtualModules.test.ts
- e2e/.gitignore
- e2e/react-start/import-protection/start-mode-config.ts
- packages/react-start-rsc/src/rsbuild/browser-decode.ts
- packages/start-plugin-core/src/index.ts
- packages/react-start-rsc/src/rsbuild/ssr-decode.ts
- e2e/react-start/basic/postcss.config.mjs
- packages/start-plugin-core/tests/prerender-ssrf.test.ts
- packages/start-plugin-core/tests/importProtection/rewrite.test.ts
- packages/start-plugin-core/tests/rsbuild-output-directory.test.ts
- e2e/react-start/basic/start-mode-config.ts
- packages/start-plugin-core/src/import-protection/constants.ts
- e2e/react-start/import-protection/rsbuild.config.ts
- packages/start-plugin-core/tests/importProtection/matchers.test.ts
- e2e/react-start/serialization-adapters/src/routes/server-function/late-raw-stream.tsx
- packages/start-plugin-core/src/rsbuild/INTERNALS-import-protection.md
- packages/react-start-rsc/src/react-server-dom-rspack.d.ts
- packages/start-plugin-core/src/vite/import-protection-plugin/INTERNALS.md
- packages/start-plugin-core/src/rsbuild/plugin.ts
- packages/start-plugin-core/src/rsbuild/import-protection.ts
- packages/start-plugin-core/src/vite/import-protection-plugin/types.ts
🚧 Files skipped from review as they are similar to previous changes (40)
- e2e/react-start/basic/server.js
- packages/start-plugin-core/src/start-compiler/load-module.ts
- e2e/react-start/basic/rsbuild.config.ts
- e2e/solid-start/basic/server.js
- packages/start-plugin-core/tests/post-server-build.test.ts
- e2e/solid-start/basic/package.json
- packages/vue-start/src/plugin/rsbuild.ts
- e2e/react-start/basic/vite.config.ts
- e2e/react-start/import-protection/server.js
- e2e/react-start/import-protection/tests/violations.utils.ts
- packages/react-start-rsc/vite.config.ts
- e2e/vue-start/basic/rsbuild.config.ts
- packages/solid-start/vite.config.ts
- e2e/vue-start/basic/src/routes/__root.tsx
- e2e/vue-start/basic/package.json
- packages/react-start/src/plugin/rsbuild.ts
- docs/start/framework/react/guide/import-protection.md
- packages/vue-start/package.json
- e2e/react-start/import-protection/vite.config.ts
- packages/react-start/package.json
- packages/start-plugin-core/src/rsbuild/dev-server.ts
- packages/start-plugin-core/src/rsbuild/schema.ts
- e2e/vue-start/basic-vue-query/src/utils/posts.tsx
- packages/start-plugin-core/tests/importProtection/analysis.usage.test.ts
- e2e/react-start/import-protection/package.json
- packages/start-plugin-core/src/import-protection/utils.ts
- packages/start-plugin-core/src/vite/import-protection-plugin/virtualModules.ts
- packages/start-plugin-core/src/rsbuild/start-router-plugin.ts
- packages/react-start-rsc/src/global.d.ts
- packages/start-plugin-core/src/post-build.ts
- e2e/react-start/import-protection/tests/import-protection.spec.ts
- e2e/solid-start/basic/src/routes/__root.tsx
- packages/start-plugin-core/src/rsbuild/normalized-client-build.ts
- packages/start-plugin-core/src/rsbuild/types.ts
- packages/start-plugin-core/tests/importProtection/trace.test.ts
- e2e/react-start/import-protection/tests/violations.setup.ts
- packages/solid-start/package.json
- packages/react-start-rsc/package.json
- packages/router-utils/tests/stripTypeExports.test.ts
- packages/start-plugin-core/src/import-protection/virtualModules.ts
Comment on lines
+44
to
+50
| const routerBasePath = joinURL('/', startConfig.router.basepath ?? '') | ||
| const routerBaseUrl = new URL(routerBasePath, 'http://localhost') | ||
|
|
||
| startConfig.pages = validateAndNormalizePrerenderPages( | ||
| startConfig.pages, | ||
| routerBaseUrl, | ||
| ) |
Contributor
There was a problem hiding this comment.
validateAndNormalizePrerenderPages is called twice on the same pages.
The function is called at line 47-50 on startConfig.pages, then again at lines 93-96 inside prerenderPages. Since the first call already normalizes the pages and assigns back to startConfig.pages, the second call is redundant.
Suggested fix
Remove the duplicate validation inside prerenderPages:
async function prerenderPages({ outputDir }: { outputDir: string }) {
const seen = new Set<string>()
const prerendered = new Set<string>()
const retriesByPath = new Map<string, number>()
const concurrency = startConfig.prerender?.concurrency ?? os.cpus().length
logger.info(`Concurrency: ${concurrency}`)
const queue = new Queue({ concurrency })
const routerBasePath = joinURL('/', startConfig.router.basepath ?? '')
- const routerBaseUrl = new URL(routerBasePath, 'http://localhost')
-
- startConfig.pages = validateAndNormalizePrerenderPages(
- startConfig.pages,
- routerBaseUrl,
- )
startConfig.pages.forEach((page) => addCrawlPageTask(page))Also applies to: 93-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/src/prerender.ts` around lines 44 - 50, The call
to validateAndNormalizePrerenderPages is being performed twice: once when
assigning startConfig.pages and again inside the prerenderPages function; remove
the redundant second call inside prerenderPages (the
validateAndNormalizePrerenderPages invocation within prerenderPages) and rely on
the already-normalized startConfig.pages, ensuring any code in prerenderPages
uses startConfig.pages (or its local parameter) directly without re-validating.
Comment on lines
2
to
+16
| import { | ||
| createImportSpecifierLocationIndex, | ||
| ImportLocCache, | ||
| buildCodeSnippet, | ||
| buildLineIndex, | ||
| findOriginalUsageLocation, | ||
| getOrCreateOriginalTransformResult, | ||
| normalizeSourceMap, | ||
| pickOriginalCodeFromSourcesContent, | ||
| } from '../../src/import-protection-plugin/sourceLocation' | ||
| } from '../../src/import-protection/sourceLocation' | ||
| import type { | ||
| SourceMapLike, | ||
| TransformResult, | ||
| TransformResultProvider, | ||
| } from '../../src/import-protection-plugin/sourceLocation' | ||
| } from '../../src/import-protection/sourceLocation' |
Contributor
There was a problem hiding this comment.
Fix alphabetical import member order.
ESLint reports that ImportLocCache should come after buildCodeSnippet to satisfy the sort-imports rule.
✅ Proposed fix
import {
+ buildCodeSnippet,
+ buildLineIndex,
createImportSpecifierLocationIndex,
- ImportLocCache,
- buildCodeSnippet,
- buildLineIndex,
findOriginalUsageLocation,
getOrCreateOriginalTransformResult,
+ ImportLocCache,
normalizeSourceMap,
pickOriginalCodeFromSourcesContent,
} from '../../src/import-protection/sourceLocation'🧰 Tools
🪛 ESLint
[error] 4-4: Member 'ImportLocCache' of the import declaration should be sorted alphabetically.
(sort-imports)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/tests/importProtection/sourceLocation.test.ts`
around lines 2 - 16, The import list in sourceLocation.test.ts is not
alphabetically ordered per eslint sort-imports; move the ImportLocCache
specifier so that it appears after buildCodeSnippet (i.e., reorder the named
imports in the import statement to: createImportSpecifierLocationIndex,
buildCodeSnippet, ImportLocCache, buildLineIndex, findOriginalUsageLocation,
getOrCreateOriginalTransformResult, normalizeSourceMap,
pickOriginalCodeFromSourcesContent) to satisfy the rule and keep the rest of the
specifiers unchanged.
Contributor
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/start-plugin-core/src/rsbuild/import-protection.ts`:
- Around line 1180-1191: When you detect a self-denying module (fileMatch ||
markerViolation) and are about to return
generateSelfContainedMockModule(exportNames) in the build branch, also record a
self-violation first so it isn't lost; construct a deferred file-violation entry
describing this module (using the module context ctx and the reason from
fileMatch/markerViolation) and push it to the deferredFileViolations collection
(or call the existing defer/emitter used elsewhere) before returning the mock
from the config.command === 'build' branch.
- Around line 1-3: There are duplicate imports from 'node:path' (resolve aliased
as resolvePath and extname) causing import/no-duplicates; replace the two
separate imports with a single import statement that imports both symbols
(resolve as resolvePath and extname) from 'node:path' and remove the extra
import so only one import from 'node:path' remains (update the import that
currently defines resolvePath and extname).
- Around line 432-447: The function getModuleResource calls the optional
property nameForCondition without guarding it; update the call in
getModuleResource to use optional chaining (candidate.nameForCondition?.()) so
it only invokes the function when present and otherwise falls through to
candidate.resourceResolveData?.resource, candidate.resource,
candidate.userRequest, then candidate.request; this prevents a possible runtime
error and satisfies strict null checking for the candidate.nameForCondition
property.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b164dc2-79ed-4733-aacb-770d0c3aec24
📒 Files selected for processing (3)
.changeset/add-rsbuild-start-support.mdpackages/start-plugin-core/src/rsbuild/import-protection.tspackages/start-plugin-core/src/rsbuild/plugin.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/add-rsbuild-start-support.md
- packages/start-plugin-core/src/rsbuild/plugin.ts
Comment on lines
+1
to
+3
| import { resolve as resolvePath } from 'node:path' | ||
|
|
||
| import { extname } from 'node:path' |
Contributor
There was a problem hiding this comment.
Merge the duplicated node:path imports.
This currently fails import/no-duplicates, so CI will block on it.
Suggested fix
-import { resolve as resolvePath } from 'node:path'
-
-import { extname } from 'node:path'
+import { extname, resolve as resolvePath } from 'node:path'📝 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.
Suggested change
| import { resolve as resolvePath } from 'node:path' | |
| import { extname } from 'node:path' | |
| import { extname, resolve as resolvePath } from 'node:path' |
🧰 Tools
🪛 ESLint
[error] 1-1: 'node:path' imported multiple times.
(import/no-duplicates)
[error] 3-3: 'node:path' imported multiple times.
(import/no-duplicates)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/src/rsbuild/import-protection.ts` around lines 1 -
3, There are duplicate imports from 'node:path' (resolve aliased as resolvePath
and extname) causing import/no-duplicates; replace the two separate imports with
a single import statement that imports both symbols (resolve as resolvePath and
extname) from 'node:path' and remove the extra import so only one import from
'node:path' remains (update the import that currently defines resolvePath and
extname).
Comment on lines
+432
to
+447
| function getModuleResource(module: RspackModule): string | undefined { | ||
| const candidate = module as RspackModule & { | ||
| nameForCondition?: () => string | undefined | ||
| resourceResolveData?: { resource?: string } | ||
| resource?: string | ||
| userRequest?: string | ||
| request?: string | ||
| } | ||
|
|
||
| return ( | ||
| candidate.nameForCondition() ?? | ||
| candidate.resourceResolveData?.resource ?? | ||
| candidate.resource ?? | ||
| candidate.userRequest ?? | ||
| candidate.request | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the file
find . -name "import-protection.ts" -type fRepository: TanStack/router
Length of output: 120
🏁 Script executed:
# Check the tsconfig.json to understand the TypeScript configuration
find . -name "tsconfig.json" -type f | head -5Repository: TanStack/router
Length of output: 265
🏁 Script executed:
# Get the full context of the getModuleResource function
cat -n packages/start-plugin-core/src/rsbuild/import-protection.ts | sed -n '425,455p'Repository: TanStack/router
Length of output: 1080
🏁 Script executed:
# Check the tsconfig for the packages/start-plugin-core directory
find packages/start-plugin-core -name "tsconfig.json" -o -name "tsconfig*.json" | xargs cat -nRepository: TanStack/router
Length of output: 533
🏁 Script executed:
# Verify the RspackModule type definition
rg "type RspackModule|interface RspackModule" -A 5 packages/start-plugin-core/Repository: TanStack/router
Length of output: 1241
🏁 Script executed:
# Check if nameForCondition is called elsewhere in this file with proper guards
rg "nameForCondition" packages/start-plugin-core/src/rsbuild/import-protection.tsRepository: TanStack/router
Length of output: 142
🏁 Script executed:
# Check how getModuleResource is used in the codebase
rg "getModuleResource" packages/start-plugin-core/src/rsbuild/Repository: TanStack/router
Length of output: 558
🏁 Script executed:
# Check the root tsconfig.json for strict mode settings
cat packages/start-plugin-core/tsconfig.jsonRepository: TanStack/router
Length of output: 306
🏁 Script executed:
# Also check the repository root tsconfig.json
cat tsconfig.json | head -50Repository: TanStack/router
Length of output: 846
Guard nameForCondition before calling it.
You typed this property as optional (line 434), but line 442 invokes it unconditionally. With strict: true enabled in tsconfig.json, TypeScript's strictNullChecks should catch this—calling an optional function without optional chaining violates type safety. Use candidate.nameForCondition?.() ?? ... to safely fall through to the next property in the chain.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/src/rsbuild/import-protection.ts` around lines 432
- 447, The function getModuleResource calls the optional property
nameForCondition without guarding it; update the call in getModuleResource to
use optional chaining (candidate.nameForCondition?.()) so it only invokes the
function when present and otherwise falls through to
candidate.resourceResolveData?.resource, candidate.resource,
candidate.userRequest, then candidate.request; this prevents a possible runtime
error and satisfies strict null checking for the candidate.nameForCondition
property.
Comment on lines
+1180
to
+1191
| if (fileMatch || markerViolation) { | ||
| let exportNames: Array<string> = [] | ||
|
|
||
| try { | ||
| exportNames = getNamedExports(ctx.code) | ||
| } catch { | ||
| exportNames = [] | ||
| } | ||
|
|
||
| if (config.command === 'build') { | ||
| return generateSelfContainedMockModule(exportNames) | ||
| } |
Contributor
There was a problem hiding this comment.
Don't drop diagnostics for self-denied modules in build mode.
This branch swaps the module for a self-contained mock, but it never records a violation before returning. The later reporting code only sees mock-edge modules, graph edges, and deferred file violations, so an entry/root module that violates its own file or marker rule can slip through without any warning/error. Please defer or emit a self-violation here before returning the mock.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/src/rsbuild/import-protection.ts` around lines
1180 - 1191, When you detect a self-denying module (fileMatch ||
markerViolation) and are about to return
generateSelfContainedMockModule(exportNames) in the build branch, also record a
self-violation first so it isn't lost; construct a deferred file-violation entry
describing this module (using the module context ctx and the reason from
fileMatch/markerViolation) and push it to the deferredFileViolations collection
(or call the existing defer/emitter used elsewhere) before returning the mock
from the config.command === 'build' branch.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/start-plugin-core/src/rsbuild/virtual-modules.ts (1)
76-86: Add type safety for the global routes manifest access.
globalThis.TSS_ROUTES_MANIFESTis accessed without type checking. If this global isn't set whenbuildStartManifestDatais called,routeTreeRouteswill beundefined, which may cause downstream issues inbuildStartManifest.♻️ Proposed fix to add type safety
function buildStartManifestData( clientBuild: NormalizedClientBuild, publicBase: string, ) { - const routeTreeRoutes = globalThis.TSS_ROUTES_MANIFEST + const routeTreeRoutes = (globalThis as typeof globalThis & { TSS_ROUTES_MANIFEST?: unknown }).TSS_ROUTES_MANIFEST return buildStartManifest({ clientBuild, routeTreeRoutes, basePath: publicBase, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/start-plugin-core/src/rsbuild/virtual-modules.ts` around lines 76 - 86, buildStartManifestData reads globalThis.TSS_ROUTES_MANIFEST without any type/existence checks, which can yield undefined and break buildStartManifest; update buildStartManifestData to safely access and type the global: declare an appropriate globalThis interface/type for TSS_ROUTES_MANIFEST (or import the manifest type), then read globalThis.TSS_ROUTES_MANIFEST into routeTreeRoutes with a runtime guard that either throws a clear error (e.g., "TSS_ROUTES_MANIFEST is missing or invalid") or supplies a typed default before calling buildStartManifest; reference the symbols buildStartManifestData, globalThis.TSS_ROUTES_MANIFEST, routeTreeRoutes, and buildStartManifest when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/start-plugin-core/src/rsbuild/plugin.ts`:
- Around line 239-250: registerVirtualModules is capturing root eagerly (using
resolvedStartConfig.root || process.cwd()) before modifyRsbuildConfig sets the
real root, causing virtual paths to be computed against the wrong directory;
change the call to pass root lazily (a function) like
registerStartCompilerTransforms does — e.g., replace the root property with
getRoot: () => resolvedStartConfig.root || process.cwd() (and update
registerVirtualModules/virtual-modules.ts to accept and call getRoot when
computing virtualPath()), so paths are generated after
applyResolvedBaseAndOutput updates resolvedStartConfig; keep
updateServerFnResolver assignment as-is.
---
Nitpick comments:
In `@packages/start-plugin-core/src/rsbuild/virtual-modules.ts`:
- Around line 76-86: buildStartManifestData reads globalThis.TSS_ROUTES_MANIFEST
without any type/existence checks, which can yield undefined and break
buildStartManifest; update buildStartManifestData to safely access and type the
global: declare an appropriate globalThis interface/type for TSS_ROUTES_MANIFEST
(or import the manifest type), then read globalThis.TSS_ROUTES_MANIFEST into
routeTreeRoutes with a runtime guard that either throws a clear error (e.g.,
"TSS_ROUTES_MANIFEST is missing or invalid") or supplies a typed default before
calling buildStartManifest; reference the symbols buildStartManifestData,
globalThis.TSS_ROUTES_MANIFEST, routeTreeRoutes, and buildStartManifest when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77e2a05c-c23b-4bf1-bf5c-dadd7f11b401
📒 Files selected for processing (3)
packages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/src/rsbuild/start-compiler-host.tspackages/start-plugin-core/src/rsbuild/virtual-modules.ts
Comment on lines
+239
to
+250
| const virtualModuleState = registerVirtualModules(api, { | ||
| root: resolvedStartConfig.root || process.cwd(), | ||
| getConfig, | ||
| serverFnsById, | ||
| providerEnvName: serverFnProviderEnv, | ||
| ssrIsProvider, | ||
| serializationAdapters: corePluginOpts.serializationAdapters, | ||
| getDevClientEntryUrl: (publicBase: string) => | ||
| joinURL(publicBase, 'static/js/index.js'), | ||
| rscEnabled, | ||
| }) | ||
| updateServerFnResolver = virtualModuleState.updateServerFnResolver |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how createStartConfigContext initializes resolvedStartConfig.root
# and whether it has a sensible default before modifyRsbuildConfig runs
rg -n -A10 'createStartConfigContext' packages/start-plugin-core/src/
rg -n 'resolvedStartConfig.*root' packages/start-plugin-core/src/config-context.tsRepository: TanStack/router
Length of output: 5101
🏁 Script executed:
# Get the full createStartConfigContext function to see when root is set
cat -n packages/start-plugin-core/src/config-context.ts | head -150Repository: TanStack/router
Length of output: 4589
🏁 Script executed:
# Get the full context around registerVirtualModules call in plugin.ts
sed -n '50,260p' packages/start-plugin-core/src/rsbuild/plugin.ts | cat -nRepository: TanStack/router
Length of output: 10092
🏁 Script executed:
# Check the registerVirtualModules function and its timing
rg -n -B5 -A15 'export.*function registerVirtualModules' packages/start-plugin-core/src/Repository: TanStack/router
Length of output: 2229
🏁 Script executed:
# Check if virtual module paths can be updated after initial registration
rg -n -A20 'const paths = {' packages/start-plugin-core/src/rsbuild/virtual-modules.tsRepository: TanStack/router
Length of output: 152
🏁 Script executed:
# Check how paths are used in virtual module registration
rg -n 'paths\.' packages/start-plugin-core/src/rsbuild/virtual-modules.ts | head -20Repository: TanStack/router
Length of output: 1228
🏁 Script executed:
# Verify if modifyBundlerChain or other dynamic updates could recompute paths
rg -n 'modifyBundlerChain\|modifyRspackConfig' packages/start-plugin-core/src/rsbuild/virtual-modules.tsRepository: TanStack/router
Length of output: 41
🏁 Script executed:
# Search for modifyRspackConfig usage
rg -n 'modifyRspackConfig' packages/start-plugin-core/src/rsbuild/virtual-modules.tsRepository: TanStack/router
Length of output: 430
🏁 Script executed:
# Get the full registerVirtualModules function to see how paths are used
sed -n '224,450p' packages/start-plugin-core/src/rsbuild/virtual-modules.ts | cat -nRepository: TanStack/router
Length of output: 10250
🏁 Script executed:
# Check what virtualPath does and if it depends on root in a way that matters
rg -n -B5 -A10 'function virtualPath\|const virtualPath' packages/start-plugin-core/src/rsbuild/Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check if there's any mechanism to update paths after root is set
rg -n 'paths =' packages/start-plugin-core/src/rsbuild/virtual-modules.tsRepository: TanStack/router
Length of output: 80
🏁 Script executed:
# Verify the exact timing: when does modifyRsbuildConfig run vs setup
sed -n '1,300p' packages/start-plugin-core/src/rsbuild/plugin.ts | grep -n 'modifyRsbuildConfig\|registerVirtualModules\|setup'Repository: TanStack/router
Length of output: 615
🏁 Script executed:
# Find virtualPath implementation
rg -n -B2 -A10 'export.*virtualPath|function virtualPath' packages/start-plugin-core/src/Repository: TanStack/router
Length of output: 1504
🏁 Script executed:
# Check VIRTUAL_MODULES constants to understand what virtualPath generates
rg -n 'VIRTUAL_MODULES|RSC_RUNTIME_VIRTUAL_ID|RSC_HMR_VIRTUAL_ID' packages/start-plugin-core/src/rsbuild/ | head -20Repository: TanStack/router
Length of output: 2446
Root path captured before modifyRsbuildConfig sets it.
Unlike registerStartCompilerTransforms (line 163) which defers root access via a function, registerVirtualModules receives the root value directly at setup time. The virtualPath() calls (lines 233–237 in virtual-modules.ts) compute paths immediately as ${root}/node_modules/.virtual/... when registerVirtualModules is called during setup(), before the modifyRsbuildConfig callback runs.
When registerVirtualModules is called, resolvedStartConfig.root is still an empty string (initialized in config-context.ts:30), so the fallback process.cwd() is used. The paths object is created once and never recomputed, even after modifyRsbuildConfig later sets resolvedStartConfig.root to the actual rsbuildConfig.root (via applyResolvedBaseAndOutput at line 52–68). This causes all virtual module paths to be registered under the wrong root, which will break builds if rsbuildConfig.root differs from the current working directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/start-plugin-core/src/rsbuild/plugin.ts` around lines 239 - 250,
registerVirtualModules is capturing root eagerly (using resolvedStartConfig.root
|| process.cwd()) before modifyRsbuildConfig sets the real root, causing virtual
paths to be computed against the wrong directory; change the call to pass root
lazily (a function) like registerStartCompilerTransforms does — e.g., replace
the root property with getRoot: () => resolvedStartConfig.root || process.cwd()
(and update registerVirtualModules/virtual-modules.ts to accept and call getRoot
when computing virtualPath()), so paths are generated after
applyResolvedBaseAndOutput updates resolvedStartConfig; keep
updateServerFnResolver assignment as-is.
Contributor
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We identified three code-change failures introduced by this PR and applied targeted fixes: merging duplicate node:path imports in import-protection.ts to resolve the ESLint import/no-duplicates error, adding src/rsbuild/types.ts as a build entry in vite.config.ts so the declared ./rsbuild/types package export actually produces its dist file, and replacing the incorrect invalidateModule call in loadModuleForRsbuildCompiler with a proper readFile + ingestModule sequence that populates the StartCompiler's module cache the same way the Vite build-mode path does.
Tip
✅ We verified this fix by re-running @tanstack/start-plugin-core:test:build, @tanstack/start-plugin-core:test:eslint, tanstack-react-start-e2e-import-protection:build:rsbuild:ssr.
Suggested Fix changes
diff --git a/packages/start-plugin-core/src/rsbuild/import-protection.ts b/packages/start-plugin-core/src/rsbuild/import-protection.ts
index 2fbf2592b3..e263083426 100644
--- a/packages/start-plugin-core/src/rsbuild/import-protection.ts
+++ b/packages/start-plugin-core/src/rsbuild/import-protection.ts
@@ -1,6 +1,4 @@
-import { resolve as resolvePath } from 'node:path'
-
-import { extname } from 'node:path'
+import { extname, resolve as resolvePath } from 'node:path'
import { normalizePath } from 'vite'
diff --git a/packages/start-plugin-core/src/start-compiler/load-module.ts b/packages/start-plugin-core/src/start-compiler/load-module.ts
index 3c0ff38919..5a9ce5d391 100644
--- a/packages/start-plugin-core/src/start-compiler/load-module.ts
+++ b/packages/start-plugin-core/src/start-compiler/load-module.ts
@@ -1,3 +1,4 @@
+import { readFile } from 'node:fs/promises'
import { SERVER_FN_LOOKUP } from '../constants'
import { cleanId } from './utils'
import type { StartCompiler } from './compiler'
@@ -35,5 +36,9 @@ export async function loadModuleForRsbuildCompiler(opts: {
compiler: StartCompiler
id: string
}): Promise<void> {
- opts.compiler.invalidateModule(cleanId(opts.id))
+ const filePath = cleanId(opts.id)
+ const code = await readFile(filePath, 'utf-8').catch(() => null)
+ if (code != null) {
+ opts.compiler.ingestModule({ code, id: opts.id })
+ }
}
diff --git a/packages/start-plugin-core/vite.config.ts b/packages/start-plugin-core/vite.config.ts
index ee363461f3..0f4bdddedc 100644
--- a/packages/start-plugin-core/vite.config.ts
+++ b/packages/start-plugin-core/vite.config.ts
@@ -15,7 +15,7 @@ export default mergeConfig(
config,
tanstackViteConfig({
tsconfigPath: './tsconfig.build.json',
- entry: ['./src/index.ts', './src/utils.ts'],
+ entry: ['./src/index.ts', './src/utils.ts', './src/rsbuild/types.ts'],
srcDir: './src',
outDir: './dist',
cjs: false,
Or Apply changes locally with:
npx nx-cloud apply-locally TvGe-bple
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
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
3 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 by CodeRabbit
New Features
Behavior/UX
Documentation
onViolationdocs (snippet shape adjusted)Chores