fix: restructure server function environment handling #6160
Conversation
WalkthroughAdds a server-only createServerFn and route, threads provider-vs-caller awareness through compilation and manifest generation, makes directive compilation client-aware, updates runtime server-fn resolution (importer/getServerFnById opts), and switches AsyncLocalStorage to global singletons; includes e2e tests validating server-only behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SSR
participant Manifest
participant Provider
Note over Client,SSR: Client-initiated server call
Client->>SSR: HTTP/RPC -> invoke SSR handler
SSR->>Manifest: getServerFnById(functionId, {fromClient: true})
Manifest->>Manifest: check entry.isClientReferenced
alt isClientReferenced == true
Manifest-->>SSR: reject (500)
SSR-->>Client: 500 error
else
Manifest->>Provider: invoke implementation (direct or via importer)
Provider-->>Manifest: result
Manifest-->>SSR: result
SSR-->>Client: result
end
Note over SSR,Provider: Server-side proxy call (internal)
SSR->>Provider: invoke server-only fn (importer or direct call)
Provider-->>SSR: result
SSR-->>Client: wrapped result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Points to focus during review:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
⏰ 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 (1)
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 12m 44s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 27s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-12-20 21:44:04 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: 3
♻️ Duplicate comments (1)
packages/start-server-core/src/request-response.ts (1)
54-56: Same initialization race condition as in async-local-storage.ts.This follows the same non-atomic check-then-initialize pattern as the start storage context. While the race window is minimal in practice, consider using the same fix if you address it in the other file for consistency.
🧹 Nitpick comments (2)
packages/server-functions-plugin/src/index.ts (2)
270-287: Consider extracting indentation constants for cleaner template generation.The template literal mixing indentation levels results in inconsistent whitespace in generated code. While functionally correct, this affects readability when debugging generated manifests.
🔎 Proposed fix to normalize indentation
function generateManifestModule( directiveFnsById: Record<string, DirectiveFn>, includeClientReferencedCheck: boolean, ): string { const manifestEntries = Object.entries(directiveFnsById) .map(([id, fn]) => { - const baseEntry = `'${id}': { - functionName: '${fn.functionName}', - importer: () => import(${JSON.stringify(fn.extractedFilename)})${ - includeClientReferencedCheck - ? `, - isClientReferenced: ${fn.isClientReferenced ?? true}` - : '' - } - }` + const clientRefPart = includeClientReferencedCheck + ? `,\n isClientReferenced: ${fn.isClientReferenced ?? true}` + : '' + const baseEntry = `'${id}': { + functionName: '${fn.functionName}', + importer: () => import(${JSON.stringify(fn.extractedFilename)})${clientRefPart} + }` return baseEntry }) .join(',')
289-297: Consider adding the function ID to the error message for easier debugging.The error message could be more specific about which function failed the client-referenced check.
🔎 Proposed improvement
const clientReferencedCheck = includeClientReferencedCheck ? ` // If called from client, only allow client-referenced functions if (opts?.fromClient && !serverFnInfo.isClientReferenced) { - throw new Error('Server function not accessible from client: ' + id) + throw new Error('Server function not accessible from client (not client-referenced): ' + id) } ` : ''
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
e2e/react-start/server-functions/src/functions/fnOnlyCalledByServer.ts(1 hunks)e2e/react-start/server-functions/src/routeTree.gen.ts(11 hunks)e2e/react-start/server-functions/src/routes/index.tsx(1 hunks)e2e/react-start/server-functions/src/routes/server-only-fn.tsx(1 hunks)e2e/react-start/server-functions/tests/server-functions.spec.ts(1 hunks)packages/directive-functions-plugin/src/compilers.ts(8 hunks)packages/directive-functions-plugin/src/index.ts(4 hunks)packages/server-functions-plugin/src/index.ts(4 hunks)packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts(2 hunks)packages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.ts(2 hunks)packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts(2 hunks)packages/start-plugin-core/src/plugin.ts(5 hunks)packages/start-plugin-core/src/start-manifest-plugin/plugin.ts(1 hunks)packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.ts(1 hunks)packages/start-plugin-core/tests/createServerFn/createServerFn.test.ts(7 hunks)packages/start-server-core/src/createSsrRpc.ts(1 hunks)packages/start-server-core/src/request-response.ts(1 hunks)packages/start-server-core/src/server-functions-handler.ts(1 hunks)packages/start-server-core/src/tanstack-start.d.ts(1 hunks)packages/start-storage-context/src/async-local-storage.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:
packages/start-plugin-core/src/start-manifest-plugin/plugin.tspackages/start-server-core/src/tanstack-start.d.tse2e/react-start/server-functions/src/functions/fnOnlyCalledByServer.tspackages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.tspackages/start-plugin-core/src/create-server-fn-plugin/compiler.tse2e/react-start/server-functions/src/routes/index.tsxpackages/start-server-core/src/request-response.tspackages/start-plugin-core/src/plugin.tse2e/react-start/server-functions/src/routeTree.gen.tspackages/start-server-core/src/server-functions-handler.tspackages/directive-functions-plugin/src/index.tse2e/react-start/server-functions/tests/server-functions.spec.tspackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.tse2e/react-start/server-functions/src/routes/server-only-fn.tsxpackages/start-plugin-core/tests/createServerFn/createServerFn.test.tspackages/directive-functions-plugin/src/compilers.tspackages/start-storage-context/src/async-local-storage.tspackages/start-server-core/src/createSsrRpc.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.tspackages/server-functions-plugin/src/index.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-plugin-core/src/start-manifest-plugin/plugin.tspackages/start-server-core/src/tanstack-start.d.tse2e/react-start/server-functions/src/functions/fnOnlyCalledByServer.tspackages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.tspackages/start-plugin-core/src/create-server-fn-plugin/compiler.tse2e/react-start/server-functions/src/routes/index.tsxpackages/start-server-core/src/request-response.tspackages/start-plugin-core/src/plugin.tse2e/react-start/server-functions/src/routeTree.gen.tspackages/start-server-core/src/server-functions-handler.tspackages/directive-functions-plugin/src/index.tse2e/react-start/server-functions/tests/server-functions.spec.tspackages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.tse2e/react-start/server-functions/src/routes/server-only-fn.tsxpackages/start-plugin-core/tests/createServerFn/createServerFn.test.tspackages/directive-functions-plugin/src/compilers.tspackages/start-storage-context/src/async-local-storage.tspackages/start-server-core/src/createSsrRpc.tspackages/start-plugin-core/src/create-server-fn-plugin/plugin.tspackages/server-functions-plugin/src/index.ts
🧠 Learnings (3)
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/react-start/server-functions/src/routeTree.gen.ts
📚 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/src/routeTree.gen.tse2e/react-start/server-functions/src/routes/server-only-fn.tsxpackages/start-plugin-core/tests/createServerFn/createServerFn.test.ts
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
e2e/react-start/server-functions/src/routeTree.gen.ts
🧬 Code graph analysis (5)
packages/start-server-core/src/tanstack-start.d.ts (1)
packages/start-server-core/src/fake-start-server-fn-manifest.ts (1)
getServerFnById(1-1)
packages/directive-functions-plugin/src/index.ts (1)
packages/directive-functions-plugin/src/compilers.ts (2)
DirectiveFn(12-24)GenerateFunctionIdFn(31-35)
e2e/react-start/server-functions/src/routes/server-only-fn.tsx (1)
e2e/react-start/server-functions/src/functions/fnOnlyCalledByServer.ts (1)
fnOnlyCalledByServer(4-6)
packages/start-plugin-core/tests/createServerFn/createServerFn.test.ts (1)
packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (1)
compile(204-282)
packages/server-functions-plugin/src/index.ts (1)
packages/directive-functions-plugin/src/compilers.ts (1)
DirectiveFn(12-24)
⏰ 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: Preview
- GitHub Check: Test
🔇 Additional comments (41)
e2e/react-start/server-functions/src/functions/fnOnlyCalledByServer.ts (1)
1-6: LGTM! Clean test fixture for server-only function.The implementation is straightforward and appropriate for E2E testing. The function correctly uses
createServerFn().handler()and returns predictable test data.e2e/react-start/server-functions/src/routes/index.tsx (1)
91-96: LGTM! Navigation link properly added.The new link is consistent with the existing navigation structure and provides clear context about the test scenario.
e2e/react-start/server-functions/tests/server-functions.spec.ts (2)
547-565: LGTM! Test correctly validates server-to-server function calls.The test follows established patterns and properly validates that the server-only function works when called from another server function.
567-582: LGTM! Test correctly validates security boundary.The test properly verifies that server-only functions cannot be called from the client, and the use of
whitelistErrorsis appropriate for this negative test case.e2e/react-start/server-functions/src/routes/server-only-fn.tsx (5)
1-12: LGTM! Clear documentation and appropriate imports.The documentation clearly explains the test scenario and the expected behavior.
14-22: LGTM! Proxy function correctly demonstrates server-to-server calls.This function properly demonstrates the intended use case where a server-only function is called from within another server function.
24-26: LGTM! Test function correctly demonstrates security boundary.This function intentionally returns a server-only function to test that the client cannot execute it. The corresponding E2E test (lines 572-582 in server-functions.spec.ts) correctly validates that this fails with an error.
28-30: LGTM! Route properly configured.
32-105: LGTM! Component correctly implements both test scenarios.The component provides appropriate UI for testing both the valid (server-to-server) and invalid (client-to-server) scenarios. Error handling is properly implemented in the second handler.
packages/start-storage-context/src/async-local-storage.ts (1)
13-25: Good use of global singleton pattern to fix bundle isolation.The Symbol.for() approach ensures the same key across different realm contexts, and storing the singleton on globalThis correctly addresses the cross-bundle AsyncLocalStorage sharing requirement mentioned in the PR objectives.
packages/start-server-core/src/request-response.ts (1)
46-58: Consistent implementation of global singleton pattern.The event storage follows the same global singleton approach as
packages/start-storage-context/src/async-local-storage.ts, which maintains consistency across the codebase and ensures both storage contexts work correctly across bundle boundaries.packages/start-plugin-core/src/start-manifest-plugin/plugin.ts (1)
84-90: LGTM!The guard condition correctly restricts manifest generation to the provider environment, which aligns with the new
serverFnProviderEnvconcept. This ensures the manifest is only generated in the appropriate environment where server functions are provided.packages/start-server-core/src/createSsrRpc.ts (1)
5-16: LGTM!The optional importer mechanism elegantly solves the tree-shaking issue. When an importer is provided (server-only functions in SSR provider environment), it uses the direct dynamic import to maintain the dependency chain. For client-referenced functions, it correctly falls back to manifest lookup. The inline comment clearly documents the two resolution paths.
packages/start-plugin-core/src/plugin.ts (3)
75-79: LGTM!Good defaulting logic—when no custom provider environment is configured, SSR serves as the provider, maintaining backward compatibility while enabling the new provider environment feature.
359-374: LGTM!The build ordering is correct: building the provider environment last ensures that server functions discovered during client/SSR builds are properly included in the manifest. The guard conditions and error handling are appropriate.
415-421: No issue detected —functionNameis already properly sanitized.The concern about
functionNamecontaining characters that would break the string literal is already addressed. ThefunctionNameis sanitized via themakeIdentifierSafefunction (packages/directive-functions-plugin/src/compilers.ts, lines 225-232), which removes all characters except alphanumeric characters, underscores, and dollar signs. This meansfunctionNamecan only contain valid JavaScript identifier characters and cannot include single quotes, backslashes, or other problematic characters. The bracket notation inm['${d.functionName}']is safe and requires no additional escaping.Likely an incorrect or invalid review comment.
packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.ts (1)
35-35: LGTM!The new
directiveSplitParamoption aligns with the updatedServerFnCompilerinterface and follows a consistent naming convention.packages/start-server-core/src/server-functions-handler.ts (1)
50-50: LGTM!The
fromClient: trueflag correctly indicates that this handler processes HTTP requests originating from clients, aligning with the extendedgetServerFnByIdsignature for client/server-origin tracking.packages/start-plugin-core/src/create-server-fn-plugin/compiler.ts (2)
204-212: LGTM!The extended
compilemethod signature withisProviderFile: booleanis well-typed and correctly propagates the flag through the compilation pipeline tohandleCreateServerFn.
262-273: LGTM!The
isProviderFileflag is correctly passed tohandleCreateServerFn, enabling the conditional second-argument inclusion for provider files.packages/start-plugin-core/src/create-server-fn-plugin/handleCreateServerFn.ts (2)
14-18: LGTM!Clear and helpful documentation explaining the purpose of
isProviderFileand why only provider files should include the handler implementation as a second argument.
159-165: LGTM!The conditional logic correctly ensures the handler implementation is only included as a second argument for server-side provider files, preventing code duplication in caller files that import from the extracted chunk.
packages/start-plugin-core/src/create-server-fn-plugin/plugin.ts (2)
42-45: LGTM!The
buildDirectiveSplitParamhelper correctly sanitizes the directive string into a URL-safe parameter format, enabling reliable detection of provider/extracted files.
129-132: LGTM!The
isProviderFiledetection correctly happens beforecleanId(id)strips query parameters, ensuring the directive split param is detected. The flag is then properly passed to the compiler.packages/start-plugin-core/tests/createServerFn/createServerFn.test.ts (3)
67-131: LGTM!Excellent test coverage for the provider/caller distinction. The tests clearly differentiate:
- Client compilation (no second argument, dead code eliminated)
- Server caller (no second argument, implementation from extracted chunk)
- Server provider (includes second argument as the implementation file)
The inline snapshots accurately reflect the expected compiled output for each scenario.
133-216: LGTM!The DCE (dead code elimination) tests are properly expanded to cover both caller and provider scenarios, verifying that:
- Server callers don't include variable definitions needed only by handlers
- Server providers retain those variables and include the handler as a second argument
50-64: Verify the parameterized testisProviderFileconfiguration.The parameterized tests set
isProviderFile: env === 'server', meaning all server environment tests compile as provider files. This appears intentional to match existing snapshot expectations. If the existing snapshots in./snapshots/server/expect provider-style output, this is correct. However, consider whether test coverage would benefit from parameterized tests that also exercise the caller scenario (isProviderFile: false) via additional file snapshots, especially since the explicit tests below (lines 82-98) demonstrate both patterns should be tested.packages/directive-functions-plugin/src/index.ts (3)
36-53: LGTM!Well-designed type extensions:
envConsumerenables explicit client/server designation independent of environment namegetKnownDirectiveFnsallows SSR callers to look up canonical extracted filenames from client builds, ensuring consistent imports across environments
93-129: LGTM!The refined selection logic correctly handles the provider/caller distinction:
- Provider options for extracted files (directive split param present)
- Caller options for non-extracted files based on current environment
- Fallback to provider when no matching caller exists
The debug logging clearly indicates which path is taken.
130-149: The concern about undefinedenvConsumerhandling is incorrect.The expression
envOptions.envConsumer === 'client'always evaluates to abooleanvalue (trueorfalse), notundefined. WhenenvConsumeris undefined, the strict equality comparison returnsfalse, which is the appropriate safe default. The type signature (isClientEnvironment?: boolean) and usage in the compiler (opts.isClientEnvironment || !!knownFn) both correctly handle this boolean value, making explicit default handling unnecessary.Likely an incorrect or invalid review comment.
packages/server-functions-plugin/src/index.ts (4)
26-32: LGTM - Clear documentation for the new getServerFnById field.The JSDoc clearly explains when this custom implementation is needed for server callers loading modules from different environments.
161-164: LGTM - Correct determination of SSR provider relationship.The logic properly identifies when the SSR environment is the provider, enabling the appropriate handling of server-only-referenced functions.
175-177: LGTM - Enables SSR callers to use canonical extracted filenames.Passing
directiveFnsByIdviagetKnownDirectiveFnsallows SSR callers to resolve functions consistently with the client build.
279-282: Verify the defaulttrueforisClientReferencedis intentional.When
fn.isClientReferencedisundefined, defaulting totruemeans unknown functions are treated as client-referenced. Confirm this is the intended fallback behavior for security (allowing access) vs. safety (denying by default).packages/directive-functions-plugin/src/compilers.ts (7)
19-24: LGTM - Interface extension aligns with the relevant code snippet.The
isClientReferencedfield with clear documentation matches the expected interface frompackages/directive-functions-plugin/src/compilers.tsshown in the relevant snippets.
42-50: LGTM - ReplacerFn type properly extended.Adding
functionNameandisClientReferencedwith clear documentation enables downstream consumers to handle client-aware transformations.
65-77: LGTM - Well-documented options for client-aware compilation.The JSDoc explains when and why these options are used, making the API clear for consumers.
441-458: LGTM - Consistent function ID generation across builds.Using relative filename ensures consistent
functionIdregardless of machine, and the local extracted filename is computed correctly before checkingknownDirectiveFns.
460-469: Clear logic for determining client-referenced status.The two-condition check (
isClientEnvironment || !!knownFn) correctly identifies functions that are client-referenced either directly or from a previous build.
566-569: LGTM - Replacer receives all necessary context.The additional
functionNameandisClientReferencedparameters enable replacers to generate appropriate code based on client-awareness.
585-593: LGTM - Directive registration includes isClientReferenced.The
directiveFnsByIdnow stores theisClientReferencedflag, enabling downstream manifest generation to include this metadata.
| if (isClientReferenced && !compileDirectiveOpts.isDirectiveSplitParam) { | ||
| // Find if this directive function is inside a .handler() call expression | ||
| // The structure is: .handler(directiveFn, originalImpl) | ||
| // We want to remove originalImpl (the second argument) | ||
| let currentPath: babel.NodePath | null = | ||
| directiveFn as babel.NodePath | null | ||
| while (currentPath && !currentPath.parentPath?.isProgram()) { | ||
| const parent: babel.NodePath | null = currentPath.parentPath | ||
| if ( | ||
| parent?.isCallExpression() && | ||
| babel.types.isMemberExpression(parent.node.callee) && | ||
| babel.types.isIdentifier(parent.node.callee.property) && | ||
| parent.node.callee.property.name === 'handler' && | ||
| parent.node.arguments.length === 2 | ||
| ) { | ||
| // Remove the second argument (the original implementation) | ||
| parent.node.arguments.pop() | ||
| break | ||
| } | ||
| currentPath = parent | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for .handler() patterns in TS/TSX files
echo "=== Searching for .handler() call patterns ==="
rg -n --type ts --type tsx '\.handler\s*\(' | head -40
echo ""
echo "=== Searching for createServerFn or directive handler patterns ==="
rg -n --type ts --type tsx 'handler\s*\(' -A 5 | head -50Repository: TanStack/router
Length of output: 246
🏁 Script executed:
#!/bin/bash
# Check if the file exists and get its structure
echo "=== Checking file existence ==="
find . -name "compilers.ts" -o -name "compilers.js" 2>/dev/null | head -5
echo ""
echo "=== Searching for .handler patterns without type restriction ==="
rg -n '\.handler\s*\(' | grep -v node_modules | head -30
echo ""
echo "=== Searching for handler calls in directive context ==="
rg -n 'handler' packages/directive-functions-plugin/src/ 2>/dev/null | head -40Repository: TanStack/router
Length of output: 4304
🏁 Script executed:
#!/bin/bash
# Read the compilers.ts file around the mentioned lines
echo "=== Reading compilers.ts around lines 460-510 ==="
sed -n '460,510p' packages/directive-functions-plugin/src/compilers.ts | cat -n
echo ""
echo "=== Reading broader context 440-520 ==="
wc -l packages/directive-functions-plugin/src/compilers.tsRepository: TanStack/router
Length of output: 3114
🏁 Script executed:
#!/bin/bash
# Search for directive function patterns and .handler usage
echo "=== Searching for how directive functions use .handler ==="
rg -n -B 5 -A 5 '\.handler' packages/directive-functions-plugin/ | head -100
echo ""
echo "=== Searching for directive function test examples ==="
find packages/directive-functions-plugin -name "*.test.*" -o -name "*.spec.*" | head -5
echo ""
echo "=== Looking for examples of .handler with arguments ==="
rg -n 'handler\s*\(' packages/directive-functions-plugin/src/ -A 3 | head -60Repository: TanStack/router
Length of output: 12369
🏁 Script executed:
#!/bin/bash
# Search for method chaining patterns that include .handler()
echo "=== Searching for chained method calls with .handler ==="
rg -n '\.\w+\s*\(' packages/directive-functions-plugin/tests/compiler.test.ts | grep -B 5 -A 5 'handler' | head -80
echo ""
echo "=== Search for nested call expressions in compiler code ==="
rg -n 'CallExpression\|MemberExpression' packages/directive-functions-plugin/src/compilers.ts | head -20
echo ""
echo "=== Check if there are tests for edge cases like chained calls ==="
grep -n 'inputValidator\|middleware' packages/directive-functions-plugin/tests/compiler.test.ts | head -10Repository: TanStack/router
Length of output: 2397
🏁 Script executed:
#!/bin/bash
# Search more specifically for method chaining patterns
echo "=== Searching for chained methods before .handler in directive context ==="
rg -n 'createServerFn\(\)' packages/directive-functions-plugin/tests/compiler.test.ts -A 1 | head -40
echo ""
echo "=== Check the compilers code for how it handles traversal ==="
sed -n '476,510p' packages/directive-functions-plugin/src/compilers.ts
echo ""
echo "=== Look for any real-world examples with .inputValidator or .middleware ==="
rg -n '\.inputValidator\|\.middleware' packages/ --type ts -B 2 -A 5 | grep -A 5 '\.handler' | head -40Repository: TanStack/router
Length of output: 3181
🏁 Script executed:
#!/bin/bash
# Check if there are any test cases that fail or test edge cases
echo "=== Running tests to verify edge cases ==="
cd packages/directive-functions-plugin && npm test 2>&1 | tail -30
echo ""
echo "=== Searching for any comments about limitations or edge cases in compilers.ts ==="
rg -n 'TODO\|FIXME\|NOTE\|edge case\|limitation' packages/directive-functions-plugin/src/compilers.ts | head -20Repository: TanStack/router
Length of output: 609
The .handler() argument removal assumes exactly 2 arguments without handling unexpected cases.
The code correctly guards against processing .handler() calls that don't have exactly 2 arguments (line 489: parent.node.arguments.length === 2), which aligns with the tested directive function pattern where the second argument is the original implementation. However, the code silently skips processing if the condition isn't met—there's no error, warning, or fallback behavior. If the future API changes or edge cases arise with different argument counts, the second argument won't be removed without any indication of failure.
Consider adding:
- An explicit error or warning if
.handler()is found but has an unexpected argument count - Documentation clarifying the assumption that directive
.handler()always receives exactly 2 arguments in this compilation phase
🤖 Prompt for AI Agents
In packages/directive-functions-plugin/src/compilers.ts around lines 476 to 497,
the walk that locates a .handler(...) call silently ignores cases where the call
exists but does not have exactly two arguments; update the logic to emit a clear
diagnostic when a .handler member call is detected with an unexpected argument
count (e.g., log a warning or throw an error depending on severity) so consumers
are alerted to the mismatch, include the callee location/context in the message,
and add a brief code comment documenting the assumption that .handler should
have exactly two args for this transform.
| export function getServerFnById( | ||
| id: string, | ||
| opts?: { fromClient?: boolean }, | ||
| ): Promise<ServerFn> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "fake-start-server-fn-manifest.ts" --exec cat {}Repository: TanStack/router
Length of output: 115
Update fake manifest implementation to match the new signature.
The fake-start-server-fn-manifest.ts implementation has export async function getServerFnById(): Promise<any> {} but must accept the id: string parameter and optional opts?: { fromClient?: boolean } to match the declaration. Update to:
export async function getServerFnById(
id: string,
opts?: { fromClient?: boolean },
): Promise<ServerFn> {}This mismatch will cause TypeScript strict mode type errors.
🤖 Prompt for AI Agents
In packages/start-server-core/src/tanstack-start.d.ts around lines 15 to 18, the
fake manifest implementation does not match the declared signature of
getServerFnById; update the fake implementation
(fake-start-server-fn-manifest.ts) to accept the parameters id: string and
opts?: { fromClient?: boolean } and to return Promise<ServerFn> so its exported
function signature exactly matches the declaration, ensuring TypeScript strict
mode passes.
| if (!globalObj[GLOBAL_STORAGE_KEY]) { | ||
| globalObj[GLOBAL_STORAGE_KEY] = new AsyncLocalStorage<StartStorageContext>() | ||
| } |
There was a problem hiding this comment.
Be aware of potential race condition in initialization.
The check-then-initialize pattern is not atomic. While Node.js module loading is typically single-threaded, dynamic imports or concurrent worker threads could theoretically create multiple AsyncLocalStorage instances, undermining the singleton guarantee.
Consider whether this edge case matters for your use case. If concurrent initialization is possible, you could use a more defensive pattern (though it adds complexity):
🔎 Alternative atomic initialization pattern
-if (!globalObj[GLOBAL_STORAGE_KEY]) {
- globalObj[GLOBAL_STORAGE_KEY] = new AsyncLocalStorage<StartStorageContext>()
-}
+globalObj[GLOBAL_STORAGE_KEY] ??= new AsyncLocalStorage<StartStorageContext>()Note: The ??= operator is atomic at the JavaScript level and more concise, though the race condition risk is minimal in practice.
📝 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.
| if (!globalObj[GLOBAL_STORAGE_KEY]) { | |
| globalObj[GLOBAL_STORAGE_KEY] = new AsyncLocalStorage<StartStorageContext>() | |
| } | |
| globalObj[GLOBAL_STORAGE_KEY] ??= new AsyncLocalStorage<StartStorageContext>() |
🤖 Prompt for AI Agents
In packages/start-storage-context/src/async-local-storage.ts around lines 21 to
23, the current check-then-assign pattern for initializing
globalObj[GLOBAL_STORAGE_KEY] can race in rare concurrent import/worker
scenarios; replace it with an atomic assignment (e.g., nullish coalescing
assignment) so the global key is set exactly once without a separate existence
check, ensuring a single AsyncLocalStorage instance is produced even under
concurrent initialization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/start-server-core/src/createSsrRpc.ts (1)
10-14: Consider adding error context for debugging server function resolution failures.While error propagation is working correctly, adding try-catch blocks or error context could improve debugging when server function resolution fails in production (especially for the tree-shaking scenarios this PR addresses).
Optional: Add error context for better debugging
- const serverFn = importer - ? await importer() - : await getServerFnById(functionId) + try { + const serverFn = importer + ? await importer() + : await getServerFnById(functionId) + } catch (error) { + throw new Error( + `Failed to resolve server function "${functionId}" via ${importer ? 'importer' : 'manifest lookup'}: ${error}`, + { cause: error } + ) + }This is optional since error propagation may be handled at a higher level.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.ts(1 hunks)packages/start-server-core/src/createSsrRpc.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/start-plugin-core/tests/createMiddleware-create-server-fn-plugin/createMiddleware.test.ts
🧰 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:
packages/start-server-core/src/createSsrRpc.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-server-core/src/createSsrRpc.ts
⏰ 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: Preview
- GitHub Check: Test
🔇 Additional comments (2)
packages/start-server-core/src/createSsrRpc.ts (2)
3-5: LGTM! Clear type definitions for importer-based resolution.The
ServerFnimport andSsrRpcImportertype alias provide good type safety for the new optional importer parameter, supporting the fix for server-side-only function tree-shaking.
7-14: Pass{ fromClient: false }togetServerFnByIdin the fallback path.The
getServerFnByIdsignature accepts an optional second parameteropts?: { fromClient?: boolean }(seetanstack-start.d.ts). When handling client-to-server calls,server-functions-handler.tsexplicitly passes{ fromClient: true }to enforce client-referenced function restrictions. IncreateSsrRpc.tsline 14, the fallback path should clarify its context by passing{ fromClient: false }to indicate these are server-only lookups, not client-initiated calls.
fixes #5976
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.