fix: trace nested server function factory re-export chain #6166
Conversation
WalkthroughAdds a three-stage nested re-export chain (A → B → C) for a server-function factory with middleware and corresponding e2e test; simultaneously refactors the create-server-fn compiler to use precomputed maps, parallel candidate resolution, a MethodChain/RewriteCandidate model, and updates handler/middleware rewrite APIs to accept RewriteCandidate. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner / Browser
participant UI as Factory UI Route
participant Server as Dev Server / Route Handler
participant Compiler as start-plugin-core Compiler
participant FS as Module Filesystem
participant Middleware as Middleware (nestedReexportC)
Test->>UI: Click nestedReexportedFactoryFn button
UI->>Server: Invoke factory endpoint
Server->>Compiler: Ensure compiled factory (resolve imports)
Compiler->>FS: Resolve import graph (A -> B -> C) using findExportInModule
FS-->>Compiler: Return resolved factory symbol (from nestedReexportC)
Compiler-->>Server: Emit compiled handler bound to factory (with middleware)
Server->>Middleware: Execute middleware (adds context.nested)
Middleware-->>Server: Continue to handler with augmented context
Server-->>Test: Respond with name + context (contains nested-middleware-executed)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-11-02T16:16:24.898ZApplied to files:
📚 Learning: 2025-10-08T08:11:47.088ZApplied to files:
🧬 Code graph analysis (2)packages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.ts (1)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (29)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 13m 57s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 20s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-12-21 00:43:52 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.ts (1)
39-44: Inconsistent guard condition betweeninputValidatorandserverchecks.Line 22 checks
if (inputValidator), but line 39 checksif (server && server.firstArgPath?.node). Theserver.firstArgPath?.nodecheck seems unnecessary since the removal logic (lines 41-43) only usesserver.callPath, notfirstArgPath. This could silently skip valid server call expressions that have no arguments.Consider aligning with the
inputValidatorpattern:Proposed fix
- if (server && server.firstArgPath?.node) { + if (server) { // remove the server call expression if (t.isMemberExpression(server.callPath.node.callee)) { server.callPath.replaceWith(server.callPath.node.callee.object) } }packages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.ts (2)
33-34: Unsafe type assertion on variable declarator id.The assertion
(variableDeclarator.id as t.Identifier).nameassumes the variable declarator always uses a simple identifier. This would fail for destructuring patterns likeconst { fn } = ...orconst [fn] = ....Consider adding a guard:
Proposed fix
const variableDeclarator = path.parentPath.node - const existingVariableName = (variableDeclarator.id as t.Identifier).name + if (!t.isIdentifier(variableDeclarator.id)) { + throw new Error('createServerFn must be assigned to a simple variable, not a destructuring pattern!') + } + const existingVariableName = variableDeclarator.id.name
58-68: Type assertion onfirstArgPath.nodemay be unsafe.Line 68 casts
handlerFnPath.node as t.Expression, butfirstArgPathis typed asbabel.NodePath | nullwhich could point to any node type (e.g.,SpreadElement). While the validation on line 60 checks for existence, it doesn't verify the node type.Consider adding explicit type validation:
Proposed fix
- const handlerFn = handlerFnPath.node as t.Expression + if (!t.isExpression(handlerFnPath.node)) { + throw codeFrameError( + opts.code, + path.node.callee.loc!, + `createServerFn handler must be an expression!`, + ) + } + const handlerFn = handlerFnPath.nodepackages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (1)
359-372: Passing entire objectpinstead of structuredRewriteCandidate.The handlers expect a
RewriteCandidatewithpathandmethodChainproperties. The objectphas{ path, kind, methodChain }, which includes an extrakindproperty. This works due to structural typing, but could be cleaner.Optional: Extract RewriteCandidate explicitly
if (p.kind === 'ServerFn') { - handleCreateServerFn(p, { + handleCreateServerFn({ path: p.path, methodChain: p.methodChain }, { env: this.options.env, code, directive: this.options.directive, isProviderFile, }) } else { - handleCreateMiddleware(p, { + handleCreateMiddleware({ path: p.path, methodChain: p.methodChain }, { env: this.options.env, }) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e/react-start/server-functions/src/routes/factory/-functions/functions.ts(2 hunks)e2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportA.ts(1 hunks)e2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportB.ts(1 hunks)e2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportC.ts(1 hunks)e2e/react-start/server-functions/src/routes/factory/index.tsx(2 hunks)e2e/react-start/server-functions/tests/server-functions.spec.ts(1 hunks)packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts(9 hunks)packages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.ts(2 hunks)packages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.ts(5 hunks)packages/start-plugin-core/src/create-server-fn-plugin/types.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/routes/factory/-functions/functions.tspackages/start-plugin-core/src/create-server-fn-plugin/types.tse2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportB.tspackages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.tspackages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.tse2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportA.tse2e/react-start/server-functions/src/routes/factory/index.tsxpackages/start-plugin-core/src/create-server-fn-plugin/compiler.tse2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportC.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/routes/factory/-functions/functions.tspackages/start-plugin-core/src/create-server-fn-plugin/types.tse2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportB.tspackages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.tspackages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.tse2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportA.tse2e/react-start/server-functions/src/routes/factory/index.tsxpackages/start-plugin-core/src/create-server-fn-plugin/compiler.tse2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportC.ts
🧠 Learnings (1)
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/routes/factory/index.tsx
🧬 Code graph analysis (4)
e2e/react-start/server-functions/src/routes/factory/-functions/functions.ts (1)
e2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportC.ts (1)
nestedReexportFactory(18-20)
packages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.ts (1)
packages/start-plugin-core/src/create-server-fn-plugin/types.ts (1)
RewriteCandidate(39-42)
e2e/react-start/server-functions/src/routes/factory/index.tsx (1)
e2e/react-start/server-functions/src/routes/factory/-functions/functions.ts (1)
nestedReexportedFactoryFn(123-130)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (3)
packages/start-plugin-core/src/create-server-fn-plugin/types.ts (1)
MethodChainPaths(18-24)packages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.ts (1)
handleCreateServerFn(11-129)packages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.ts (1)
handleCreateMiddleware(10-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (26)
e2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportC.ts (2)
1-16: LGTM! Well-documented middleware implementation.The JSDoc clearly explains the position in the re-export chain, and the middleware implementation correctly augments the context with type-safe values using
as const.
18-20: LGTM! Factory configuration is correct.The server function factory is properly configured with the GET method and middleware attachment, following the established pattern in the codebase.
e2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportA.ts (1)
1-7: LGTM! Clean re-export with clear documentation.The JSDoc clearly documents this module's role as the top-level entry point in the nested re-export chain, and the re-export syntax is correct.
e2e/react-start/server-functions/src/routes/factory/-functions/nestedReexportB.ts (1)
1-7: LGTM! Clean re-export with clear documentation.The JSDoc clearly documents this module's role as the middle link in the nested re-export chain, and the re-export syntax is correct.
e2e/react-start/server-functions/src/routes/factory/index.tsx (2)
15-15: LGTM! Import is correctly added.The import of
nestedReexportedFactoryFnis properly placed with other factory function imports.
156-165: LGTM! Test case configuration is correct and well-documented.The test case properly configures the expected behavior for the nested re-export chain, with expected context matching the middleware output from
nestedReexportC.ts. The comment clearly explains the A -> B -> C chain pattern.e2e/react-start/server-functions/tests/server-functions.spec.ts (1)
595-617: LGTM! Comprehensive e2e test for nested re-export chain.The test properly verifies that middleware executes correctly through the A -> B -> C nested re-export chain. The test pattern is consistent with existing re-export tests and includes appropriate assertions for both the middleware context and result comparison.
e2e/react-start/server-functions/src/routes/factory/-functions/functions.ts (2)
9-10: LGTM! Import correctly references the top of the re-export chain.The import properly starts from
nestedReexportA, which is the entry point of the A -> B -> C chain, and the comment clearly explains the test scenario.
121-130: LGTM! Factory function implementation is correct and consistent.The
nestedReexportedFactoryFnimplementation follows the established pattern for factory-based server functions, properly accessing the middleware context and returning the expected structure for testing.packages/start-plugin-core/src/create-server-fn-plugin/handleCreateMiddleware.ts (2)
2-3: LGTM!Clean import structure with the new
RewriteCandidatetype from the types module.
10-18: LGTM!The function signature refactor to accept
RewriteCandidateis well-documented and the server environment guard is appropriate.packages/start-plugin-core/src/create-server-fn-plugin/types.ts (3)
1-12: LGTM!Well-structured type definitions with appropriate JSDoc documentation. The
MethodCallInfointerface cleanly captures the call path and optional first argument path.
14-34: LGTM!The
MethodChainPathsinterface andMETHOD_CHAIN_KEYSconstant provide a clear contract for the method chain structure. UsingReadonlyArraywithas constensures immutability.
36-42: LGTM!The
RewriteCandidateinterface cleanly encapsulates the path and method chain information needed for rewrite operations.packages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.ts (3)
2-3: LGTM!Clean import updates for the new type structure.
11-25: LGTM!Well-documented function signature with the new
directivefield and clear JSDoc forisProviderFile.
103-120: LGTM!The handler replacement logic correctly constructs the arrow function with the
'use server'directive and references the original variable name for__executeServer.packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (9)
45-53: LGTM!Good performance optimization with pre-computed
IdentifierToKindmap enabling O(1) candidate detection instead of iterating over lookup kinds.
261-276: LGTM!Parallel resolution of candidates using
Promise.allis a sound performance improvement. The subsequent filtering intotoRewriteMapcorrectly handles the resolved results.
288-298: LGTM!Collecting all
CallExpressionpaths into a Map for O(1) lookup is an effective optimization that avoids repeated AST traversals.
316-348: Method chain walking logic looks correct.The downward traversal through the call chain correctly collects method call info for each known method name. The use of
callExprPaths.get(currentNode)!is safe here since these nodes were collected in the prior traversal.One minor observation: if a method appears multiple times in the chain (e.g.,
.handler().handler()), only the outermost occurrence is recorded since the inner one would overwrite. This appears intentional given the fluent API design.
350-355: LGTM!Good defensive check ensuring all candidates were successfully resolved to paths.
429-482: LGTM!The
findExportInModulemethod correctly implements recursive export resolution throughexport * fromchains with:
- Cycle detection via
visitedModulesset- Parallel resolution of re-export sources for performance
- First-match-wins semantics appropriate for valid code without duplicate exports
453-478: Parallel resolution may mask errors from failed module loads.If one re-export source fails to resolve while another succeeds, the error is silently ignored and the first valid result is returned. This is likely intentional for robustness, but could mask configuration issues during development.
Consider whether failed resolutions should be logged at debug level for troubleshooting.
543-550: LGTM!Efficiently unwrapping TypeScript wrapper expressions (
TSAsExpression,TSNonNullExpression,ParenthesizedExpression) before processing improves resolution efficiency by avoiding repeated checks.
669-686: LGTM!The refactored
isCandidateCallExpressioncorrectly uses the pre-computedIdentifierToKindmap for O(1) lookup, improving performance over the previous iteration-based approach.
also implement perf optimizations
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.