feat: add rsbuild support for TanStack Start #6623
Conversation
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
…issues-2060 tanstack start rsbuild
Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
…on-refinement-1b7e Branch implementation refinement
Restore the PR GitHub Actions workflow to the main-branch behavior because the quality-audit PR changes were not needed for feat/rsbuild. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds rsbuild integration across Start: new rsbuild plugin core, loaders, prerender/post-build orchestration, server-fn manifest/resolver, framework rsbuild plugin entries and package exports, a bundler runner replacing direct Vite scripts, virtual module ID/type updates, CSS/test adjustments, and numerous package/config edits. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Runner as run-bundler.mjs
participant Bundler as Vite/Rsbuild
participant PluginCore as TanStackStartRsbuildPluginCore
participant Compiler as StartCompiler (loader)
participant Server as Dev/Prod Server
Dev->>Runner: npm run dev / build / preview
Runner->>Bundler: select bundler (BUNDLER env) and forward command + args
Bundler->>PluginCore: load rsbuild plugins / plugin factory
PluginCore->>Compiler: invoke start-compiler-loader / route-tree-loader
Compiler->>PluginCore: emit server-fn metadata & transformed modules
PluginCore->>Bundler: produce virtual modules (manifest, server-fn resolver, injected scripts)
alt build + prerender
PluginCore->>Server: start built server artifact for prerender
Server->>PluginCore: serve routes → prerender writes HTML
end
Bundler-->>Dev: build/dev finished (artifacts + manifest)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit ef21285
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/start-server-core/src/server-functions-handler.ts (1)
167-183: 🛠️ Refactor suggestion | 🟠 MajorRedundant redirect checks and dead code in
isRedirectResponse.Two issues in this block:
getRedirectOptions(unwrapped)is computed at Line 167, but if it returns falsy,getRedirectOptions(unwrapped)is computed again at Line 177. Cache the result from Line 167.
Boolean(getRedirectOptions(unwrapped))at Line 177 is dead code:getRedirectOptionsreturnsundefinedwhen!isRedirect(value), so this expression can only betruewhenisRedirect(unwrapped)is alreadytrue. The condition simplifies to justisRedirect(unwrapped).Proposed simplification
const redirectOptions = getRedirectOptions(unwrapped) if (redirectOptions) { return Response.json( { ...redirectOptions, isSerializedRedirect: true }, { headers: getResponseHeaders(unwrapped) }, ) } if (isResponseLike(unwrapped)) { - const isRedirectResponse = - isRedirect(unwrapped) || Boolean(getRedirectOptions(unwrapped)) - if (isRedirectResponse) { + if (isRedirect(unwrapped)) { return unwrapped }
🤖 Fix all issues with AI agents
In @.github/workflows/autofix.yml:
- Around line 12-14: The workflow currently grants broad runner token
permissions (permissions: contents: write and pull-requests: write); change the
workflow-level permissions block to set contents: read (remove contents: write
and pull-requests: write) so the GITHUB_TOKEN only has read access; leave write
actions (pushing fixes/commenting) to the autofix.ci GitHub App and ensure the
permissions block references the symbol permissions with contents: read to
enforce least privilege.
In `@e2e/react-start/basic/rsbuild.config.ts`:
- Around line 1-7: Reorder the import statements so Node.js built-ins come
first: move the imports for node:path and node:url (path and fileURLToPath)
before external package imports like { defineConfig } from '@rsbuild/core', {
pluginReact } from '@rsbuild/plugin-react', and { tanstackStart } from
'@tanstack/react-start/plugin/rsbuild'; ensure symbols referenced in this file
(defineConfig, pluginReact, path, fileURLToPath, tanstackStart, isSpaMode,
isPrerender) are preserved and only the import order is changed to satisfy the
Node built-in-before-external rule.
In `@e2e/react-start/basic/scripts/run-bundler.mjs`:
- Around line 21-35: The run function's Promise currently listens only for the
child's 'close' event and can hang if spawn emits an 'error'; update the run
implementation (the run function that calls spawn and the child variable) to
attach a child.on('error', ...) handler that rejects the Promise with the error
(ensuring no double-resolve/reject race with the existing 'close' handler) and
cleanly propagates spawn errors to callers.
In `@packages/react-start/package.json`:
- Around line 109-119: The peerDependencies entry for "vite" is not marked
optional in peerDependenciesMeta, causing unnecessary warnings; update the
package.json so that under "peerDependenciesMeta" you add an object for "vite"
with "optional": true (mirroring how "@rsbuild/core" is marked) so the "vite"
peer dependency becomes optional for consumers — locate the "peerDependencies"
and "peerDependenciesMeta" blocks in packages/react-start/package.json and add
"vite": { "optional": true } to the meta object.
In `@packages/router-plugin/src/rspack.ts`:
- Around line 70-80: The public API surface is inconsistent across bundlers;
make webpack.ts and vite.ts match rspack.ts by exporting the same set of
symbols: add the function variants tanstackRouterAutoImport,
tanstackRouterCodeSplitter, tanstackRouterGenerator and the class-style
auto-import export (e.g., TanStackRouterWebpackAutoImport) to webpack.ts (and if
missing in vite.ts add corresponding TanStackRouterViteAutoImport or the chosen
class names), or alternatively remove the extra class/function duplicates from
rspack.ts—pick one consistent pattern and apply it across
TanStackRouterRspack/TanStackRouterWebpack/TanStackRouterVite plus their
Generator, CodeSplitter, AutoImport and function helpers so all bundles export
the same identifiers (e.g., TanStackRouter{Bundler},
TanStackRouterGenerator{Bundler}, TanStackRouterCodeSplitter{Bundler},
TanStackRouterAutoImport{Bundler}, tanstackRouterGenerator,
tanstackRouterCodeSplitter, tanstackRouterAutoImport, tanstackRouter).
In `@packages/start-client-core/src/client-rpc/serverFnFetcher.ts`:
- Around line 38-50: isResponseLike currently claims value is Response but only
duck-types status and headers.get; change it to narrow to a minimal interface
(e.g., ResponseLike with the exact members your downstream code uses such as
json, text, status, headers) or add explicit checks for the methods used before
narrowing so downstream calls (in serverFnFetcher where isResponseLike is
consumed at lines referenced) are safe; update the function signature from
"value is Response" to "value is ResponseLike" (or similar) and include checks
that headers.get, json, and text are functions if those are invoked later.
In `@packages/start-plugin-core/package.json`:
- Around line 96-104: Update the package.json peerDependenciesMeta so that the
"vite" peer dependency is marked optional like "@rsbuild/core": add a "vite": {
"optional": true } entry under "peerDependenciesMeta" to prevent peer dependency
warnings for users who only use rsbuild; ensure the JSON object includes this
key alongside the existing "@rsbuild/core" entry.
In `@packages/start-plugin-core/src/rsbuild/plugin.ts`:
- Around line 716-725: The code hardcodes outputFilename = 'server.js' which
breaks if rspack's output.filename changes; update the logic in plugin.ts around
where serverBuild/serverEntryPath are set to derive the actual emitted filename
from the build output (e.g., inspect the rspack build stats/compilation assets
or the configured output.filename pattern) or accept a configurable option for
the server entry filename; specifically, use the rspack compilation output
mapping to find the emitted server entry file in serverOutputDir (referencing
serverBuild, serverEntryPath, serverOutputDir, and outputFilename) and import
that resolved filename instead of assuming 'server.js'.
- Around line 598-606: The forEach callback currently uses a concise arrow (fn:
any) => fn(middlewares, context) which implicitly returns fn's value; change it
to a block-bodied function so it does not return a value (e.g., (fn: any) => {
fn(middlewares, context) }) or replace the forEach with an explicit loop; update
the invocation around existingSetupMiddlewares (the branch handling
Array.isArray(existingSetupMiddlewares)) to call each fn with middlewares and
context without returning values before calling setupMiddlewares(middlewares,
context).
In `@packages/start-plugin-core/src/rsbuild/post-server-build.ts`:
- Around line 38-50: The spread of startConfig.spa.prerender.headers can be
undefined and causes a TypeError; replace the optional-chaining or raw spread
with a nullish-coalescing fallback so the spread always gets an object—i.e.,
change ...(startConfig.spa.prerender.headers) to
...(startConfig.spa.prerender.headers ?? {}) in the startConfig.pages.push block
(the code that sets prerender.headers and adds HEADERS.TSS_SHELL) and make the
identical change in the other occurrence where startConfig.spa.prerender.headers
is spread.
In `@packages/start-plugin-core/src/rsbuild/prerender.ts`:
- Around line 233-241: The retry path is broken because addCrawlPageTask(page)
returns early due to seen.has(page.path); in the catch block inside the
prerender retry logic, remove page.path from the seen set before incrementing
retriesByPath and re-queueing so addCrawlPageTask can enqueue it again;
specifically, update the catch handling around addCrawlPageTask, referencing
seen, retriesByPath, addCrawlPageTask, and
prerenderOptions.retryCount/retryDelay to remove the path from seen, increment
retriesByPath, wait retryDelay, then call addCrawlPageTask(page).
- Around line 234-237: The retry log message is hardcoded to "in 500ms" but the
code uses prerenderOptions.retryDelay (which may be undefined); update the retry
logic in the block that checks retries < (prerenderOptions.retryCount ?? 0) so
the logger.warn prints the actual delay value (use a resolvedDelay =
prerenderOptions.retryDelay ?? 500 to mirror the intended default) and pass that
resolvedDelay into setTimeout; refer to the variables logger.warn,
prerenderOptions.retryDelay, and the retries check to locate and fix the code.
In `@packages/start-plugin-core/src/rsbuild/start-compiler-loader.ts`:
- Around line 28-29: The module-level Map compilers and object serverFnsById are
never cleared and can retain stale state across rebuilds; update the build
lifecycle (e.g., inside beforeRun or the start of each build cycle where
beforeRun currently clears the disk manifest) to explicitly clear these
in-memory caches: call compilers.clear() to reset the Map and reassign or delete
all keys from serverFnsById (or set serverFnsById = {} if scope allows) before
each build, ensuring any envName-based cached StartCompiler entries and
accumulated server function entries are removed between rebuilds.
In `@packages/start-plugin-core/src/rsbuild/start-compiler-plugin.ts`:
- Around line 174-185: The getManifest() function currently sets the
module-level cached variable to {} inside the catch, causing transient read
errors to be cached permanently; change the logic so cached is only assigned on
successful read/JSON.parse (using fs.readFileSync(manifestPath, 'utf-8') and
JSON.parse(raw)) and do not set cached in the catch block—simply return an empty
object (or undefined) on error so subsequent calls will retry reading the file;
reference getManifest, cached, manifestPath, fs.readFileSync and JSON.parse to
locate and update the code.
- Around line 76-93: In generateManifestModule, id and fn.functionName are
interpolated into a single-quoted JS literal unescaped; replace those raw
interpolations with safe JSON.stringify usage (e.g., JSON.stringify(id) for the
property key and JSON.stringify(fn.functionName) for the functionName value) so
any quotes/backslashes are escaped like fn.extractedFilename currently is; keep
the existing importer and conditional isClientReferenced logic
(includeClientReferencedCheck and fn.isClientReferenced) unchanged while
switching the string construction to use the JSON-encoded values.
In `@packages/start-plugin-core/src/rsbuild/start-router-plugin.ts`:
- Around line 71-79: The function resolveLoaderPath is duplicated; extract it
into a single shared utility module (e.g., create and export resolveLoaderPath
from a new util file) and replace the duplicate implementations by importing
that exported resolveLoaderPath in both locations that currently define it (the
copy in start-router-plugin.ts and the other copy). Ensure the new util
preserves the same logic (uses fileURLToPath(import.meta.url), path.resolve, and
fs.existsSync) and update the imports in the modules that referenced the local
function to import the shared symbol instead.
- Around line 17-69: moduleDeclaration logic is duplicated across
moduleDeclaration (in start-router-plugin.ts), buildRouteTreeModuleDeclaration
(in plugin.ts), and the equivalent in start-router-plugin/plugin.ts; extract
this into a single shared utility function (e.g., createModuleDeclaration) that
accepts the same parameters (startFilePath, routerFilePath, corePluginOpts,
generatedRouteTreePath) and reuse it from all three locations. Implement the
helper to contain getImportPath and the string-building logic currently inside
moduleDeclaration, export it, then replace the existing inline functions by
importing and calling the new helper from each file (ensure callers still
receive the same returned string shape and update any referenced names like
getRouter/startInstance/createStart to match the helper’s implementation).
In `@packages/start-server-core/src/server-functions-handler.ts`:
- Around line 398-411: The duck-typing in isResponseLike currently only asserts
headers?.get exists but later code (in handlers that call isResponseLike) calls
headers.set; update isResponseLike to also verify that the headers object
exposes a set function (e.g., check typeof headers?.set === 'function') so
objects that only implement get won't be treated as Response-like, and keep the
rest of the existing guards (status and headers presence) intact; locate the
guard in function isResponseLike and add the set check to the returned
predicate.
- Around line 324-340: The catch block redundantly calls
getRedirectOptions(error) twice; compute const redirectOptions =
getRedirectOptions(error) once at the start of the isResponseLike(error) branch
and reuse it for the server-fn serialized redirect branch and for computing
isRedirectResponse (use isRedirect(error) || Boolean(redirectOptions)); then
remove the duplicate getRedirectOptions call so the logic in the isResponseLike
catch path (the redirectOptions && isServerFn serialized Response,
isRedirectResponse check, and setting X_TSS_RAW_RESPONSE) all use the single
redirectOptions variable.
🧹 Nitpick comments (22)
packages/router-plugin/src/rspack.ts (1)
47-49: Missing JSDoc onTanStackRouterAutoImportRspack.
TanStackRouterGeneratorRspackandTanStackRouterCodeSplitterRspackboth have@exampleJSDoc blocks showing usage in an rsbuild config. Consider adding one here for consistency.📝 Suggested JSDoc
+/** + * `@example` + * ```ts + * export default defineConfig({ + * // ... + * tools: { + * rspack: { + * plugins: [TanStackRouterAutoImportRspack()], + * }, + * }, + * }) + * ``` + */ const TanStackRouterAutoImportRspack = /* `#__PURE__` */ createRspackPlugin( unpluginRouteAutoImportFactory, )e2e/react-start/basic/src/styles/app.css (1)
26-28: Blanketoutline: none !importantremoves keyboard focus indicators.Even in an e2e test app, removing all outlines via
.using-mouse *can mask accessibility regressions if e2e tests ever assert focus behavior. Consider using:focus-visibleinstead so keyboard focus indicators remain intact while hiding outlines for mouse clicks::focus:not(:focus-visible) { outline: none; }packages/start-plugin-core/src/start-compiler-plugin/handleCreateServerFn.ts (1)
491-530: Minor:globalHandlersAST node created but not reused ininitHandlers.Line 492 creates
globalHandlersas aMemberExpression, butinitHandlers(Lines 496-512) builds its own duplicateMemberExpressionnodes forglobalThis.__tssServerFnHandlers. Consider reusingglobalHandlersviat.cloneNode()to reduce AST duplication.Proposed simplification
const globalHandlers = t.memberExpression( t.identifier('globalThis'), t.identifier('__tssServerFnHandlers'), ) const initHandlers = t.expressionStatement( t.assignmentExpression( '=', - t.memberExpression( - t.identifier('globalThis'), - t.identifier('__tssServerFnHandlers'), - ), + t.cloneNode(globalHandlers), t.logicalExpression( '||', - t.memberExpression( - t.identifier('globalThis'), - t.identifier('__tssServerFnHandlers'), - ), + t.cloneNode(globalHandlers), t.arrayExpression([]), ), ), )packages/start-plugin-core/src/rsbuild/start-compiler-plugin.ts (4)
104-155: Substantial code duplication betweengenerateManifestModuleandgenerateManifestModuleFromFile.The
getServerFnByIdfunction body (fallback resolution withfnModule,action,serverFnMeta,globalThis.__tssServerFnHandlers) is duplicated nearly verbatim between the two generators (Lines 107–154 and Lines 188–248). Any future bug fix or behavior change would need to be applied in both places.Consider extracting the shared resolution logic into a helper string or a separate runtime utility that both code paths can reference.
Also applies to: 188-248
511-520: Static analysis flagged ReDoS risk from variable in regex — low risk here but worth sanitizing.
handlerVar(Line 515) is derived from a prior regex match constraining it to[A-Za-z_$][\w$]*(valid JS identifiers), so the practical ReDoS risk is minimal. However, defensively escaping the variable is low-cost and eliminates the static analysis warning.♻️ Escape handlerVar before regex construction
+ const escapeRegex = (s: string) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') const exportMatch = scope.match( - new RegExp(`([A-Za-z_$][\\w$]*):\\(\\)=>${handlerVar}`) + new RegExp(`([A-Za-z_$][\\w$]*):\\(\\)=>${escapeRegex(handlerVar)}`) )
353-362: Asset content scanning for every unmatched server function may be slow on large builds.
findAssetMatch(Lines 353–362) reads and scans potentially every JS asset for each server function that doesn't have a direct module-graph match. This is O(N × M) where N = unmatched server fns and M = asset count, and each iteration may trigger filesystem reads (Line 346–348 fallback).Consider caching asset content reads (they're already partially used via
getAssetContent) or building an inverted index of tokens → asset names once per compilation.
39-42: Extensive use ofanytypes across rspack/webpack interop layer.
CompilationModule, the various function parameters (compiler: any,compilation: any,module: any,chunk: any), and helper functions all rely onany. While this is understandable given the untyped nature of rspack/webpack plugin APIs, consider using@rspack/coretypes or at minimum defining minimal structural interfaces for the accessed properties to catch shape mismatches at compile time.packages/start-plugin-core/src/rsbuild/prerender.ts (2)
42-48:validateAndNormalizePrerenderPagesis called twice on the same pages.Lines 45–48 normalize
startConfig.pagesand then Lines 122–125 insideprerenderPagesnormalize them again. The second call is redundant since the pages are already normalized and mutated in-place viastartConfig.pages.♻️ Remove the duplicate call inside prerenderPages
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: 119-125
97-110:extractLinkswon't capture links with explicit basepath or protocol-relative URLs.The regex on Line 104 only matches
hrefvalues starting with/or./. Links written as relative paths without./(e.g.,href="subpage") or with the full origin (e.g.,href="http://localhost/foo") won't be crawled. This may be acceptable for the intended use case, but it's worth noting.packages/solid-start/src/plugin/rsbuild.ts (1)
6-9: Consider importingRsbuildPluginfrom@rsbuild/coreinstead of defining a local type withany.The local type erases the
setupAPI parameter type. Since@rsbuild/coreis already declared as a peer dependency, importing the canonical type would provide compile-time safety for plugin implementations.That said, this avoids a hard import dependency at the cost of type safety, which may be intentional to keep the module loadable without
@rsbuild/coreinstalled.Suggested improvement
-type RsbuildPlugin = { - name: string - setup: (api: any) => void -} +import type { RsbuildPlugin } from '@rsbuild/core'As per coding guidelines, "Use TypeScript strict mode with extensive type safety for all code" (
**/*.{ts,tsx}).packages/vue-start/src/plugin/rsbuild.ts (1)
6-9: Consider sharing theRsbuildPlugintype across framework plugins.This identical type definition is duplicated in
react-start,solid-start, andvue-start. Consider exporting it from@tanstack/start-plugin-core/rsbuild(or colocating it there) to avoid drift. Also,setup: (api: any) => voidloses type safety — if@rsbuild/coreexposes a properRsbuildPlugintype, importing it (behind the optional peer dep) would be preferable.packages/start-plugin-core/src/rsbuild/index.ts (1)
1-3: Vite-named exports from an rsbuild barrel may cause confusion.
VITE_ENVIRONMENT_NAMESandresolveViteIdcarry Vite-specific naming but are exported from the rsbuild entry point. If these are intentionally shared across bundler backends, consider renaming to bundler-agnostic names (e.g.,ENVIRONMENT_NAMES,resolveModuleId) to avoid confusion for consumers.packages/start-plugin-core/src/rsbuild/route-tree-loader.ts (1)
3-8: Tighten loader context and options typing to comply with strict mode guidelines.The
this: anyandrouterConfig?: anybypass TypeScript strict mode.LoaderContextis available from@rspack/core(v1.2.2), andConfigis available from@tanstack/router-generator. Update the types to improve type safety per the strict mode requirement:Suggested improvement
+import type { Config } from '@tanstack/router-generator' +import type { LoaderContext } from '@rspack/core' import { getClientRouteTreeContent } from './route-tree-state' -export default function routeTreeLoader(this: any) { +export default function routeTreeLoader(this: LoaderContext) { const callback = this.async() - const options = this.getOptions() as { - routerConfig?: any + const options = this.getOptions() as { + routerConfig?: Config root?: string }packages/start-plugin-core/src/rsbuild/start-manifest-plugin.ts (2)
246-277: Rspack plugin usesanyfor compiler and stats — loses type safety.The
apply(compiler: any)andstats: anyparameters bypass TypeScript's type system entirely. While rspack types may not be readily available, consider importingCompilerandStatstypes from@rspack/coreif they're a dependency, or at minimum add a local interface for the subset of API used.As per coding guidelines, TypeScript strict mode with extensive type safety should be used.
279-311: Virtual module caches manifest permanently after first read — no invalidation path.The generated virtual module (lines 296-305) uses a module-scoped
cachedvariable. Once the manifest is read successfully, subsequent calls always return the stale cached value, even if the manifest file on disk changes. This is likely acceptable for production builds, but during development or watch-mode rebuilds this could serve outdated data.If this is intentionally production-only, a brief comment documenting that assumption would help future maintainers.
packages/start-plugin-core/src/rsbuild/start-router-plugin.ts (1)
98-122: Confusing control flow ingetRouteTreeFileFooter— conditional assignment is immediately overwritten.Lines 105-111 conditionally assign
routeTreeFileFooterfrom the original config, but line 112 unconditionally reassigns it. The original footer content is preserved via the spread on line 119, so the logic is functionally correct, but the intermediate assignment to the outerrouteTreeFileFootervariable is misleading.A clearer approach would use a local variable for the parsed original footer:
♻️ Suggested clarification
function getRouteTreeFileFooter() { if (routeTreeFileFooter) { return routeTreeFileFooter } const { startConfig, resolvedStartConfig } = getConfig() const ogRouteTreeFileFooter = startConfig.router.routeTreeFileFooter - if (ogRouteTreeFileFooter) { - if (Array.isArray(ogRouteTreeFileFooter)) { - routeTreeFileFooter = ogRouteTreeFileFooter - } else { - routeTreeFileFooter = ogRouteTreeFileFooter() - } - } - routeTreeFileFooter = [ + let existingFooter: Array<string> = [] + if (ogRouteTreeFileFooter) { + existingFooter = Array.isArray(ogRouteTreeFileFooter) + ? ogRouteTreeFileFooter + : ogRouteTreeFileFooter() + } + routeTreeFileFooter = [ moduleDeclaration({ generatedRouteTreePath: getGeneratedRouteTreePath(), corePluginOpts, startFilePath: resolvedStartConfig.startFilePath, routerFilePath: resolvedStartConfig.routerFilePath, }), - ...(routeTreeFileFooter ?? []), + ...existingFooter, ] return routeTreeFileFooter }packages/start-plugin-core/src/rsbuild/plugin.ts (3)
30-33:RsbuildPlugintype usesapi: any— consider narrowing.While the full rsbuild API type may be complex, defining at least the methods used (
modifyRsbuildConfig,onAfterBuild,onAfterStartProdServer,context) as a partial interface would provide better type safety across this file.As per coding guidelines, TypeScript strict mode with extensive type safety should be used.
100-119:mergeRspackConfigshallow-merges onlyplugins,module.rules, andresolve.alias— other nested keys may be silently dropped.If
basehas keys undermoduleother thanrules(e.g.,parser,generator), the spread...next?.modulewill overwrite them. The same applies toresolvekeys beyondalias(e.g.,extensions,mainFields).This is acceptable if the merge is only used for known, controlled configurations within this file, but it could cause subtle breakage if user-provided rsbuild configs include additional nested keys.
♻️ More robust merge for resolve
resolve: { ...base?.resolve, ...next?.resolve, alias: { ...(base?.resolve?.alias ?? {}), ...(next?.resolve?.alias ?? {}), }, + extensions: [ + ...(base?.resolve?.extensions ?? []), + ...(next?.resolve?.extensions ?? []), + ], },
406-418: Hardcoded monorepo pathpackages/start-client-core/dist/esmonly resolves in the monorepo workspace.Line 406-408 builds a path relative to
rootthat assumes the TanStack monorepo structure. For published packages consumed by end users, this path won't exist. TheexistsSyncguard (line 415) prevents a crash, and the regex fallback (line 411, pushed on line 418) provides the actual matching — so this is safe. But the dead code path is confusing.Consider adding a comment explaining this is a monorepo dev-time optimization, or remove the
existsSyncbranch entirely and rely solely on the regex.packages/start-plugin-core/src/rsbuild/start-compiler-loader.ts (3)
31-38: Sync I/O inappendServerFnsToManifestblocks the event loop.
fs.mkdirSyncandfs.appendFileSyncare used inside what is ultimately called from an async loader. While this is typically acceptable in build tooling, it could become a bottleneck with many server functions. ThemkdirSynccall on every append is also redundant after the first call.♻️ Optional: cache directory creation
+const createdDirs = new Set<string>() const appendServerFnsToManifest = ( manifestPath: string, data: Record<string, ServerFn>, ) => { if (!manifestPath || Object.keys(data).length === 0) return - fs.mkdirSync(path.dirname(manifestPath), { recursive: true }) + const dir = path.dirname(manifestPath) + if (!createdDirs.has(dir)) { + fs.mkdirSync(dir, { recursive: true }) + createdDirs.add(dir) + } fs.appendFileSync(manifestPath, `${JSON.stringify(data)}\n`) }
42-54:getTransformCodeFilterForEnvrecomputes patterns on every call.
KindDetectionPatternsandLookupKindsPerEnvare presumably static. This function is called for every file processed by the loader (viashouldTransformCode). Consider caching the result perenv:♻️ Simple memoization
+const transformCodeFilterCache = new Map<string, Array<RegExp>>() function getTransformCodeFilterForEnv(env: 'client' | 'server'): Array<RegExp> { + const cached = transformCodeFilterCache.get(env) + if (cached) return cached const validKinds = LookupKindsPerEnv[env] const patterns: Array<RegExp> = [] for (const [kind, pattern] of Object.entries(KindDetectionPatterns) as Array< [keyof typeof KindDetectionPatterns, RegExp] >) { if (validKinds.has(kind)) { patterns.push(pattern) } } + transformCodeFilterCache.set(env, patterns) return patterns }
114-151:resolveIdPromise may never settle ifresolverthrows synchronously.If the
resolverfunction (line 133) throws synchronously instead of calling the callback, the Promise will hang indefinitely. Wrapping the resolver call in a try/catch inside the Promise executor would make this more resilient:🛡️ Suggested defensive fix
return new Promise((resolve) => { const resolver = loaderContext.getResolve?.({ conditionNames: ['import', 'module', 'default'], }) ?? loaderContext.resolve - resolver(resolveContext, source, (err: Error | null, result?: string) => { + try { + resolver(resolveContext, source, (err: Error | null, result?: string) => { - if (!err && result) { - resolve(cleanId(result)) - return - } - try { - const resolved = require.resolve(source, { - paths: [ - baseContext, - loaderContext.rootContext || loaderContext.context, - ].filter(Boolean), - }) - resolve(cleanId(resolved)) - } catch { - resolve(null) - } - }) + if (!err && result) { + resolve(cleanId(result)) + return + } + try { + const resolved = require.resolve(source, { + paths: [ + baseContext, + loaderContext.rootContext || loaderContext.context, + ].filter(Boolean), + }) + resolve(cleanId(resolved)) + } catch { + resolve(null) + } + }) + } catch { + resolve(null) + } })
Re-run CI after an external pkg.pr.new Cloudflare worker error in the Preview publish step. Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve branch-introduced review findings by hardening response/redirect handling, fixing prerender retry behavior, tightening workflow and peer dependency metadata, and improving rsbuild/webpack/vite plugin consistency. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (1)
332-339:⚠️ Potential issue | 🟠 MajorType mismatch:
processServerFnResponseparameter expectsResponsebut receivesResponseLike.At line 276,
processServerFnResponseis called withresponsetyped asResponseLike(line 221), but the function parameter (line 337) is typed asResponse. SinceResponseLikeisPick<Response, 'status' | 'ok' | 'headers' | 'body' | 'json' | 'text'>, it's a structural subset and not assignable to the fullResponsetype under strict TypeScript mode.Update the parameter type to
ResponseLikefor consistency:Proposed fix
async function processServerFnResponse({ response, onMessage, onError, }: { - response: Response + response: ResponseLike onMessage: (msg: any) => any onError?: (msg: string, error?: any) => void }) {
🤖 Fix all issues with AI agents
In `@packages/start-plugin-core/src/rsbuild/plugin.ts`:
- Around line 386-394: The synchronous deletion using
fs.rmSync(generatedRouteTreePath) can race with concurrent generators; instead,
perform a safe, non-racing removal: attempt an atomic rename of
generatedRouteTreePath to a temp/backup name (e.g., generatedRouteTreePath +
'.stale') inside a try/catch and only unlink the backup afterwards, or at
minimum replace fs.rmSync with an unlink/rename wrapped in try/catch that
ignores ENOENT/EBUSY errors; update the code around generatedRouteTreePath /
existingTree / registerDeclaration to use this safe rename-then-remove or
try/catch-unlink pattern rather than calling fs.rmSync directly.
- Around line 558-590: The dev middleware inside setupMiddlewares mutates
req.url by assigning joinURL(resolvedStartConfig.viteAppBase, req.url ?? '/'),
which can break downstream middleware; instead create a new request object with
the modified URL (e.g., construct a new Request/NodeRequest using the composed
URL) and pass that to serverBuild.fetch while leaving the original req.url
untouched, updating the NodeRequest construction site (where NodeRequest({ req,
res }) is created) to use the new URL and returning sendNodeResponse(res,
webRes) as before; apply the same non-mutating change to the analogous
production middleware to keep behavior consistent.
In `@packages/start-plugin-core/src/rsbuild/prerender.ts`:
- Around line 86-95: The catch in the prerendering block swallows errors from
prerenderPages (and upstream addCrawlPageTask/queue.start), so update the catch
in the async block around prerenderPages to log the error via
logger.error(error) and then rethrow the error so callers (e.g.,
postServerBuildRsbuild) observe the failure; keep the existing logging but add a
`throw error` after it to propagate the failure.
- Around line 186-189: The code computing isSpaShell risks a TypeError because
it uses optional chaining on startConfig.spa but then accesses
.prerender.outputPath without guarding against prerender being undefined; update
the check in the isSpaShell assignment to safely access prerender (e.g., use
startConfig.spa?.prerender?.outputPath or check startConfig.spa?.prerender
before reading outputPath) so comparison with cleanPagePath is only performed
when outputPath is defined, preserving the existing isSpaShell logic and
variable names.
- Line 50: The code sets process.env.TSS_PRERENDERING = 'true' but never resets
it and also swallows errors; modify the prerender flow (the function that sets
process.env.TSS_PRERENDERING in
packages/start-plugin-core/src/rsbuild/prerender.ts) to ensure the env var is
restored (delete or reset to previous value) in a finally block after
prerendering completes, and change the catch block to rethrow the caught error
so failures are propagated.
In `@packages/start-plugin-core/src/rsbuild/start-compiler-loader.ts`:
- Around line 163-182: The loadModule function currently short-circuits relative
ids (cleaned.startsWith('.') or '/') and calls fsp.readFile with cleaned, which
reads from process.cwd(); instead, always call resolveId(loaderContext, cleaned)
to get a proper module path (use ((await resolveId(loaderContext, cleaned)) ??
cleaned) only as a fallback for already-absolute paths), then skip if the
resolved result is falsy or contains '\0', and finally use the resolvedPath for
fsp.readFile and compiler.ingestModule; update the logic around cleanId,
resolveId, resolvedPath, fsp.readFile, and compiler.ingestModule accordingly so
relative imports are resolved relative to the importer rather than the CWD.
In `@packages/start-plugin-core/src/rsbuild/start-compiler-plugin.ts`:
- Around line 254-270: The plugin currently only hooks compiler.hooks.beforeRun
so resetServerFnCompilerState() and the tempManifestPath removal (fsp.rm) won’t
run in watch mode; update createServerFnManifestRspackPlugin to also tapPromise
into compiler.hooks.watchRun (using the same callback) so both
resetServerFnCompilerState() and await fsp.rm(tempManifestPath, { force: true })
run for watch builds as well, referencing the existing beforeRun handler logic
to avoid duplicating behavior.
🧹 Nitpick comments (15)
packages/router-plugin/src/vite.ts (1)
37-37: Consider adding a@deprecatednotice ontanStackRouterCodeSplitter.The new
tanstackRouterCodeSplitteralias aligns naming with the rest of the exports (tanstackRouter,tanstackRouterGenerator,tanstackRouterAutoImport) — nice cleanup. However, unlikeTanStackRouterVite(Line 51-52), the oldtanStackRouterCodeSplitterdoesn't carry a@deprecatedJSDoc. Adding one would guide consumers toward the consistent name.Suggested change
+/** + * `@deprecated` Use `tanstackRouterCodeSplitter` instead. + */ const tanStackRouterCodeSplitter = createVitePlugin( unpluginRouterCodeSplitterFactory, )Also applies to: 60-61
packages/router-plugin/src/webpack.ts (1)
36-38: Missing JSDoc@exampleblock for consistency with sibling plugins.
TanStackRouterGeneratorWebpack(line 10) andTanStackRouterCodeSplitterWebpack(line 23) both have@exampleJSDoc blocks showing usage.TanStackRouterAutoImportWebpackis missing one.📝 Suggested JSDoc addition
+/** + * `@example` + * ```ts + * export default { + * // ... + * plugins: [TanStackRouterAutoImportWebpack()], + * } + * ``` + */ const TanStackRouterAutoImportWebpack = /* `#__PURE__` */ createWebpackPlugin( unpluginRouteAutoImportFactory, )packages/start-client-core/src/client-rpc/serverFnFetcher.ts (1)
71-85:redirect(payload as any)bypasses type checking on the redirect payload.If the shape of
payloaddoesn't match whatredirect()expects beyondisSerializedRedirect, this will silently produce a malformed redirect. Consider narrowing to at least validate thatpayloadhashrefor the required redirect fields before callingredirect().packages/start-plugin-core/src/rsbuild/start-compiler-loader.ts (4)
120-161: Promise may hang if the resolver callback is never invoked.If
resolver(...)neither throws nor invokes its callback (e.g., a buggy or custom resolver in the rspack configuration), this promise will never settle, silently stalling the build. Consider wrapping it with a timeout or anAbortSignal-based guard.🛡️ Suggested defensive timeout
return new Promise((resolve) => { + const timeout = setTimeout(() => { + resolve(null) + }, 10_000) const resolver = loaderContext.getResolve?.({ conditionNames: ['import', 'module', 'default'], }) ?? loaderContext.resolve try { resolver(resolveContext, source, (err: Error | null, result?: string) => { + clearTimeout(timeout) if (!err && result) { resolve(cleanId(result)) return } try { const resolved = require.resolve(source, { paths: [ baseContext, loaderContext.rootContext || loaderContext.context, ].filter(Boolean), }) resolve(cleanId(resolved)) } catch { resolve(null) } }) } catch { + clearTimeout(timeout) resolve(null) } })
48-60:getTransformCodeFilterForEnvallocates a new array on every loader call.
shouldTransformCode→getTransformCodeFilterForEnvis called for every file processed by the loader. SinceLookupKindsPerEnvandKindDetectionPatternsare module-level constants, the result perenvis always identical. Memoizing byenvavoids repeated iteration and allocation.♻️ Optional: memoize by env
+const transformCodeFilterCache = new Map<string, Array<RegExp>>() + function getTransformCodeFilterForEnv(env: 'client' | 'server'): Array<RegExp> { + const cached = transformCodeFilterCache.get(env) + if (cached) return cached const validKinds = LookupKindsPerEnv[env] const patterns: Array<RegExp> = [] for (const [kind, pattern] of Object.entries(KindDetectionPatterns) as Array< [keyof typeof KindDetectionPatterns, RegExp] >) { if (validKinds.has(kind)) { patterns.push(pattern) } } + transformCodeFilterCache.set(env, patterns) return patterns }Also applies to: 115-118
201-233: Compiler instance is cached byenvNamewith first-call options — subsequent calls with different options are silently ignored.If a loader call for the same
envNamearrives with a differentenv,root, orframework(e.g., due to misconfiguration), the cached compiler from the first invocation is silently reused. This is likely fine in practice, but there's no validation or assertion guarding this invariant.Consider adding a debug-mode assertion (e.g., checking
compiler.options.env === env) after the cache hit to catch misconfiguration early.
184-186: Apply proper types from@rsbuild/coreto improve type safety for the loader context.The
this: anytyping at line 184 andloaderContext: anyparameters (lines 121, 165) bypass TypeScript checks entirely. Since@rsbuild/coreis a peer dependency and re-exports theRspacknamespace, useRspack.LoaderContext<LoaderOptions>for proper typing:Example fix
import type { Rspack } from '@rsbuild/core' async function resolveId( loaderContext: Rspack.LoaderContext<LoaderOptions>, source: string, importer?: string, ): Promise<string | null> { // ... } export default function startCompilerLoader( this: Rspack.LoaderContext<LoaderOptions>, code: string, map: any, ) { // ... }This aligns with the coding guideline requirement for "TypeScript strict mode with extensive type safety for all code."
packages/start-plugin-core/src/rsbuild/start-compiler-plugin.ts (2)
328-365: Consider caching asset content to avoid repeated I/O in the manifest resolution loop.
getAssetContentis called per-server-function per-asset in the fallback paths offindAssetMatch,findExportName, andfindModuleIdByFunctionId. For a project with N server functions and M assets, this can result in O(N×M) redundant reads of the same assets. Since this runs inside a singleafterEmitcallback, asset content is immutable at that point.♻️ Proposed fix — add a simple content cache
+ const assetContentCache = new Map<string, string | undefined>() const getAssetContent = async (assetName: string) => { + if (assetContentCache.has(assetName)) { + return assetContentCache.get(assetName) + } const assetFromCompilation = ... try { const assetPath = path.join(opts.serverOutputDir, assetName) - return await fsp.readFile(assetPath, 'utf-8') + const content = await fsp.readFile(assetPath, 'utf-8') + assetContentCache.set(assetName, content) + return content } catch { + assetContentCache.set(assetName, undefined) return undefined } }Apply the same caching pattern at the function level, setting
assetContentCachefor all code paths before returning.
42-45: LooseCompilationModuletype with index signature.The
[key: string]: anyindex signature meansmodule.idreturnsanyinstead ofstring | number | undefined, defeating type safety for property access. Consider using a more specific intersection or a mapped type if feasible, though this is understandable for rspack interop.packages/start-plugin-core/src/rsbuild/plugin.ts (3)
406-418: Hardcoded monorepo-relative path forstart-client-core.
path.resolve(root, 'packages/start-client-core/dist/esm')on line 406 only resolves in the TanStack Router monorepo. For consumer projects, this path will never exist. TheexistsSyncguard (line 415) prevents a runtime error, and the regex on line 410–411 covers the installed-package case, so this isn't a bug — but the hardcoded path is dead code outside the monorepo and could confuse future maintainers.Consider adding a brief comment explaining this is a monorepo development convenience, or deriving the path from
require.resolve/import.meta.resolve.
100-119:mergeRspackConfigsilently drops nested properties frombasewhennextdefines the same top-level key.The shallow spread
...nextat line 103 replaces all base keys not explicitly deep-merged. Currently onlyplugins,module.rules, andresolve.aliasare deep-merged. If bothbaseandnextsupplyoutputorexperiments, the base values are completely lost.This appears intentional for the current usage (the plugin sets complete configs), but it's a latent footgun for future callers. A brief JSDoc noting the shallow-merge behavior would help.
30-33: LocalRsbuildPlugintype usesanyfor theapiparameter.This loses all type safety on the rsbuild API calls throughout the file.
@rsbuild/coreis available as a dependency (>=1.0.0) and should provide proper types. Consider importing the realRsbuildPlugintype. If not suitable, at minimum type the most-used API methods (modifyRsbuildConfig,onAfterBuild,onAfterStartProdServer,context) as a lightweight interface.packages/start-plugin-core/src/rsbuild/prerender.ts (3)
119-125: Duplicate page validation — pages are validated twice.
routerBasePath,routerBaseUrl, andvalidateAndNormalizePrerenderPagesare computed and applied identically on lines 42–48 (inprerender()) and again on lines 119–125 (inprerenderPages()). The second pass is redundant sincestartConfig.pageswas already normalized beforeprerenderPagesis called.♻️ Remove the redundant validation
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, - )Then reference the outer
routerBasePathvariable insideprerenderPages. Currently it's re-declared (line 119 shadows line 42).
127-127: Biome:forEachcallback implicitly returns a value.
addCrawlPageTaskreturnsvoid, so this is functionally harmless, but using a block body satisfies the linter.♻️ Proposed fix
- startConfig.pages.forEach((page) => addCrawlPageTask(page)) + startConfig.pages.forEach((page) => { + addCrawlPageTask(page) + })
97-110: Link extraction regex doesn't handle self-closing anchors or single-quoted/unquoted attributes correctly beyond the basic case.The regex
/<a[^>]+href=["']([^"']+)["'][^>]*>/gconflates single and double quotes in its character class —[^"']matches up to whichever quote type appears first rather than the one that opened the attribute. For example,<a href="it's">would incorrectly truncate at the apostrophe.This is a minor edge case for prerender crawling, and HTML from SSR is likely well-formed, but worth noting.
Use a Response type guard-compatible response variable while keeping stricter runtime checks so start-client-core type and eslint checks pass in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/start-client-core/src/client-rpc/serverFnFetcher.ts`:
- Around line 67-81: The parseRedirectFallback function uses an unsafe "as any"
cast before calling redirect; replace that with a proper cast to the redirect
input type (e.g., RedirectOptions) so TypeScript can check fields while still
allowing optional properties to be omitted—specifically change the call in
parseRedirectFallback from redirect(payload as any) to redirect(payload as
RedirectOptions) (or the exact exported type used by redirect), keeping the
existing existence checks and not adding extra validation.
Remove unnecessary optional chaining in the response-like header guard so start-server-core eslint passes in CI. Co-authored-by: Cursor <cursoragent@cursor.com>
Extract shared route-tree and loader helpers, harden rsbuild middleware and route tree cleanup behavior, and address prerender/loader edge cases surfaced by review feedback. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (1)
221-227:⚠️ Potential issue | 🟡 MinorUse
console.errorinstead ofconsole.logfor error logging.Line 225 logs a caught error using
console.log. This is an error path — other error handlers in this file (Lines 266, 279, 396, 406, 452) consistently useconsole.error.Proposed fix
} catch (error) { if (isResponseLike(error)) { response = error } else { - console.log(error) + console.error(error) throw error } }
🤖 Fix all issues with AI agents
In `@packages/start-plugin-core/src/rsbuild/plugin.ts`:
- Around line 361-373: The hardcoded startClientCoreDistPath and its
fs.existsSync check are redundant for published packages because the
startClientCoreDistPattern regex already matches both monorepo and
installed-package dist paths; remove the startClientCoreDistPath constant and
the conditional that pushes it into loaderIncludePaths, leaving
loaderIncludePaths initialized with resolvedStartConfig.srcDirectory and always
pushing startClientCoreDistPattern instead so only the regex
(startClientCoreDistPattern) and the srcDirectory are used.
In `@packages/start-plugin-core/src/rsbuild/prerender.ts`:
- Line 135: The forEach callback is using a concise arrow expression that
implicitly returns the result of addCrawlPageTask, violating the Biome lint
rule; change the call to use a block body so the callback does not return a
value, e.g. replace startConfig.pages.forEach((page) => addCrawlPageTask(page))
with startConfig.pages.forEach((page) => { addCrawlPageTask(page); }); — locate
the call to startConfig.pages.forEach and update the arrow function accordingly
(or alternatively convert to a for...of loop) so the callback has an explicit
void body.
- Around line 42-48: You've duplicated page normalization:
validateAndNormalizePrerenderPages is run once at module scope and again inside
prerenderPages, causing double-normalization and double decode (corrupting %
encodings). Remove the inner-call to validateAndNormalizePrerenderPages and rely
on the already-normalized startConfig.pages and the outer-scope
routerBasePath/routerBaseUrl (they are closed over and used elsewhere). Ensure
prerenderPages no longer mutates or reassigns startConfig.pages or re-decodes
paths so decodeURIComponent is only applied once.
In `@packages/start-plugin-core/src/rsbuild/start-compiler-loader.ts`:
- Around line 200-232: The cached compiler stored in the compilers Map captures
the original loader context via the loadModule and resolveId closures (created
when new StartCompiler(...) is called), causing stale this when
compiler.compile() is invoked for later files; fix by ensuring the loader
context is passed per-invocation rather than closed over: change StartCompiler's
API or its construction so loadModule and resolveId accept a loaderContext
parameter (or make them setters) and, before each compiler.compile call, pass
the current loader context into those callbacks (or update/replace
compiler.loadModule and compiler.resolveId with wrappers that forward the
current loader context) so resolution always uses the current rspack loader
context instead of the one captured during initial creation.
🧹 Nitpick comments (9)
packages/start-plugin-core/src/rsbuild/prerender.ts (1)
39-39:startConfig.pagesis mutated in-place as a side effect.Line 39 replaces the
pagesarray and line 147 pushes crawled pages into it. This silently alters the caller's config object. Ifprerenderis ever called more than once (or the config is reused), the accumulated mutations will cause unexpected behavior.Consider working with a local copy instead:
♻️ Sketch
- startConfig.pages = pages + // work with a local copy to avoid mutating the caller's config + const resolvedPages = [...pages]Then use
resolvedPagesthroughout instead ofstartConfig.pages.Also applies to: 146-148
packages/start-plugin-core/src/rsbuild/start-compiler-plugin.ts (4)
107-158: Duplicated server-function resolution logic across two generators.The runtime resolution chain (direct named-export lookup →
Object.valuesfallback scan →globalThis.__tssServerFnHandlers→ throw) is duplicated almost verbatim betweengenerateManifestModule(Lines 107–158) andgenerateManifestModuleFromFile(Lines 190–251). If the resolution strategy changes, both copies must be updated in lockstep.Consider extracting the shared resolution body into a helper string-builder (or a shared runtime module) so the logic lives in one place.
♻️ Sketch: extract a shared resolution helper
+function generateResolutionBody(includeClientReferencedCheck: boolean): string { + const clientReferencedCheck = includeClientReferencedCheck + ? ` + if (opts?.fromClient && !serverFnInfo.isClientReferenced) { + throw new Error('Server function not accessible from client: ' + id) + } +` + : '' + + return ` + if (!serverFnInfo) { + throw new Error('Server function info not found for ' + id) + } +${clientReferencedCheck} + // ... fnModule resolution assumed to be set before this point ... + + if (!fnModule) { + console.info('serverFnInfo', serverFnInfo) + throw new Error('Server function module not resolved for ' + id) + } + + let action = fnModule[serverFnInfo.functionName] + if (action?.serverFnMeta?.id && action.serverFnMeta.id !== id) { + action = undefined + } + if (!action) { + const fallbackAction = Object.values(fnModule).find( + (candidate) => + candidate?.serverFnMeta?.id && + candidate.serverFnMeta.id === id, + ) + if (fallbackAction) { + action = fallbackAction + } + } + if (Array.isArray(globalThis.__tssServerFnHandlers)) { + const globalMatch = globalThis.__tssServerFnHandlers.find( + (candidate) => + candidate?.serverFnMeta?.id && + candidate.serverFnMeta.id === id, + ) + if (globalMatch && (!action || action.__executeServer)) { + action = globalMatch + } + } + + if (!action) { + console.info('serverFnInfo', serverFnInfo) + console.info('fnModule', fnModule) + throw new Error( + \\\`Server function module export not resolved for serverFn ID: \\\${id}\\\`, + ) + } + return action` +}Then both
generateManifestModuleandgenerateManifestModuleFromFilecallgenerateResolutionBody(includeClientReferencedCheck)for the common tail.Also applies to: 190-251
505-535: Regex from variable is safe here, but the heuristic is inherently brittle.The static analysis tool flags the
new RegExp(…)on Line 530 as a ReDoS risk. In practice,escapedHandlerVaris properly escaped (Line 525-528) and the pattern contains no nested quantifiers, so there is no backtracking risk. The search scope is also capped at 4 000 chars (Line 515).However,
extractExportNamerelies on a very specific minified output format (exportName:()=>handler). Different rspackoptimization.minimizesettings, terser vs. SWC, or future bundler versions could produce a different shape. Consider adding a brief comment documenting the expected minifier output and the conditions under which this heuristic applies, to help future maintainers.
276-654: TheafterEmitcallback is ~375 lines — consider breaking it into named phases.The callback performs several distinct operations: merging manifests, extracting stats, building module-to-file mappings, matching server functions to importers, resolving export names via content scanning, and finally writing the manifest. Extracting each phase into a named helper (either module-level or at least as clearly labeled inner functions) would improve navigability and testability.
Not blocking, but it would make this critical build logic significantly easier to maintain and debug.
333-370: Repeated asset content reads are not cached — potentially redundant I/O.
getAssetContent(Line 333) can fall back tofsp.readFile(Line 356), and it is called for every asset infindAssetMatch,findExportName, andfindModuleIdByFunctionId— each of which iterates overassetFilesfor every server function. With N server functions and M assets, the same file can be read from disk up to N × 3 times.Consider caching the result of
getAssetContentin aMap<string, string | undefined>scoped to theafterEmitcallback to avoid redundant disk I/O.⚡ Sketch: add a content cache
+ const assetContentCache = new Map<string, string | undefined>() const getAssetContent = async (assetName: string) => { + if (assetContentCache.has(assetName)) { + return assetContentCache.get(assetName) + } // ... existing logic ... + assetContentCache.set(assetName, result) + return result }packages/start-plugin-core/src/start-router-plugin/route-tree-module-declaration.ts (2)
26-27: Redundant POSIX separator conversion when usingpathe.
pathealways returns POSIX-style paths with/separators, sorelativePath.split(path.sep).join('/')is a no-op. Not harmful, but unnecessary.♻️ Simplify
- // Use POSIX import separators for generated module declarations. - return relativePath.split(path.sep).join('/') + return relativePath
13-28: Remove file extensions from generated import paths to match the Vite plugin's behavior.The
getImportPathfunction does not strip file extensions from import paths (e.g.,./router.tsinstead of./router), unlike the Vite plugin'sgetImportPathinpackages/router-generator/src/utils.tswhich callsremoveExt(). While TypeScript'smoduleResolution: "bundler"can handle.tsimports, this divergence may cause issues with stricter resolution settings and is inconsistent with the established pattern. Import paths in generated module augmentations should not include file extensions.packages/start-plugin-core/src/rsbuild/plugin.ts (2)
56-75:mergeRspackConfigshallow-merges nested objects — deep properties beyondplugins,module, andresolveare silently lost.If
basehas other nested rspack config objects (e.g.,output,optimization,experiments),...nextwill completely overwrite them rather than merge. This is fine if callers only pass the expected keys, but could surprise future consumers.
32-35: ImportRsbuildPlugintype from@rsbuild/coreinstead of defining a local type withany.Replace the local type definition with an import from
@rsbuild/core:Suggested change
- type RsbuildPlugin = { - name: string - setup: (api: any) => void - } + import type { RsbuildPlugin } from '@rsbuild/core'The local type definition erases all type safety on the
apiparameter. Since@rsbuild/coreis already a peerDependency, importing the real type will catch misuse of the API (e.g., wrong hook names, incorrect return types) and provide proper autocomplete for methods likemodifyRsbuildConfig(),context,onAfterStartProdServer(), andonAfterBuild().
Remove redundant monorepo-only loader include path, avoid double prerender normalization, and ensure cached compilers always resolve modules with the current loader context. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Summary
Test plan
Made with Cursor
Summary by CodeRabbit
New Features
Improvements
Chores