refactor: only handle createServerFn #6226
Conversation
schiller-manuel
changed the title
📝 WalkthroughWalkthroughStreamlines client and server server-function request/response flows: client simplifies argument/header handling and unifies GET/POST serialization; server adds cached Seroval plugins and FormData constants, simplifies payload extraction for GET/POST, and removes createServerFn-specific branches. (48 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant FetchLayer as Fetch
participant ServerHandler as Server
participant Seroval as SerovalPlugins
rect rgb(235,248,255)
Note over Client,FetchLayer: Client builds request
Client->>Client: determine options (headers/context)\nserialize payload (GET->query / POST->body)
Client->>FetchLayer: send HTTP request (x-tsr-redirect: manual)
end
rect rgb(250,245,235)
Note over FetchLayer,ServerHandler: Server receives request
FetchLayer->>ServerHandler: HTTP request (method, headers, payload)
ServerHandler->>ServerHandler: methodLower check\nextract payload (GET from searchParams / POST parse JSON)
ServerHandler->>Seroval: deserialize payload via cached plugins
Seroval-->>ServerHandler: deserialized payload/context
ServerHandler->>ServerHandler: invoke server function with normalized payload
ServerHandler-->>FetchLayer: HTTP response (serialized result or error)
end
rect rgb(235,255,240)
Note over FetchLayer,Client: Client processes response
FetchLayer-->>Client: response
Client->>Client: getResponse try/catch\nhandle Response or throw
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit bd33dee
☁️ 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: 3
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)
30-30: Improve type safety to align with TypeScript strict mode guidelines.Multiple uses of the
anytype throughout this file violate the coding guideline to "Use TypeScript strict mode with extensive type safety." While some dynamic typing may be necessary for server functions, consider:
- Adding a generic type parameter to
parsePayloadto preserve type information- Using
unknowninstead ofanywhere the type is truly dynamic and forcing type guards- Defining specific interfaces for
contextandpayloadshapesAs per coding guidelines, TypeScript strict mode should be used with extensive type safety.
🔎 Example improvements for type safety
// Define payload interface interface ServerFnPayload { context?: Record<string, unknown> data?: unknown } // Add type parameter to parsePayload function parsePayload<T = unknown>(payload: unknown): T { const parsedPayload = fromJSON(payload, { plugins: serovalPlugins }) return parsedPayload as T } // Use more specific context type interface ServerFnContext { [key: string]: unknown } // Update function signature export const handleServerAction = async ({ request, context, }: { request: Request context: ServerFnContext }) => { // ... }Also applies to: 64-67, 114-114, 131-131, 148-148
🧹 Nitpick comments (1)
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (1)
19-27: Consider a more descriptive loop variable name.The variable
_is conventionally used for unused variables, but here it's used in thehasOwnPropertycheck. A more descriptive name likekeyorpropwould improve readability.🔎 Proposed refactor
const hop = Object.prototype.hasOwnProperty function hasOwnProperties(obj: object): boolean { - for (const _ in obj) { - if (hop.call(obj, _)) { + for (const key in obj) { + if (hop.call(obj, key)) { return true } } return false }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/start-server-core/src/server-functions-handler.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/server-functions-handler.tspackages/start-client-core/src/client-rpc/serverFnFetcher.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/server-functions-handler.tspackages/start-client-core/src/client-rpc/serverFnFetcher.ts
🧠 Learnings (2)
📚 Learning: 2025-12-24T22:47:44.320Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6211
File: e2e/react-start/i18n-paraglide/src/server.ts:6-6
Timestamp: 2025-12-24T22:47:44.320Z
Learning: In TanStack Router projects using `inlang/paraglide-js`, the callback passed to `paraglideMiddleware` should use `() => handler.fetch(req)` (referencing the outer `req`) instead of `({ request }) => handler.fetch(request)`. This is intentional because the router needs the untouched URL to perform its own rewrite logic with `deLocalizeUrl`/`localizeUrl`. The middleware's processed request would delocalize the URL and interfere with the router's rewrite handling.
Applied to files:
packages/start-client-core/src/client-rpc/serverFnFetcher.ts
📚 Learning: 2025-08-30T09:12:13.852Z
Learnt from: Sheraff
Repo: TanStack/router PR: 5051
File: packages/router-core/src/utils.ts:310-315
Timestamp: 2025-08-30T09:12:13.852Z
Learning: In TanStack Router's deepEqual utility, using for..in instead of Object.keys() in getObjectKeys() when ignoreUndefined=true is acceptable because it's called only after isPlainObject() checks, which ensure objects have standard Object prototype chains with no inherited enumerable properties.
Applied to files:
packages/start-client-core/src/client-rpc/serverFnFetcher.ts
🧬 Code graph analysis (2)
packages/start-server-core/src/server-functions-handler.ts (2)
packages/start-client-core/src/index.tsx (1)
getDefaultSerovalPlugins(101-101)packages/start-client-core/src/getDefaultSerovalPlugins.ts (1)
getDefaultSerovalPlugins(8-17)
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (2)
packages/start-client-core/src/index.tsx (1)
FunctionMiddlewareClientFnOptions(29-29)packages/start-server-core/src/request-response.ts (1)
getResponse(399-402)
⏰ 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 (6)
packages/start-server-core/src/server-functions-handler.ts (2)
42-42: LGTM! Consistent use of cached values and variables.The lazy initialization of
serovalPlugins, the use ofmethodLowerfor consistency, and the reference toFORM_DATA_CONTENT_TYPESare all clean improvements.Also applies to: 57-60, 74-76, 80-80
122-133: The client-side code inserverFnFetcher.tsexplicitly ensuresContent-Type: application/jsonis set for all JSON payloads (line 132), and FormData requests are handled correctly with the browser-setmultipart/form-dataheader. POST requests with no data naturally result in an empty payload{}, which is the intended behavior. This implementation is working as designed.Likely an incorrect or invalid review comment.
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (4)
52-68: Excellent validation for GET requests with FormData.The explicit check to reject FormData for GET requests is good defensive programming. FormData cannot be serialized into URL query parameters, and the clear error message helps developers understand the constraint.
The unified handling of GET payload serialization into the query string is also clean and well-structured.
100-100: Good optimization withhasOwnProperties.Replacing
Object.keys(opts.context).length > 0withhasOwnProperties(opts.context)is more efficient since it short-circuits on the first property found. The proper truthy checkopts.context &&before calling the helper ensures safe handling.Also applies to: 122-122
146-156: Clean simplification of error handling.The refactored try/catch structure is much clearer than the previous implementation. Treating
Responseinstances thrown as errors as valid responses is a common pattern in fetch-based APIs, and the explicitinstanceofcheck makes the intent obvious.
37-42: The removal of theisPlainObjectcheck is safe. All calls toserverFnFetcheroriginate exclusively fromcreateClientRpc, which is only invoked throughcreateServerFn().handler(). The Babel plugin ensures proper argument structure at compile time, and the middleware chain increateServerFnconstructsargs[0]with the expectedFunctionMiddlewareClientFnOptionsshape. The narrowing is justified and requires no changes.
| import type { Plugin as SerovalPlugin } from 'seroval' | ||
|
|
||
| let regex: RegExp | undefined = undefined | ||
|
|
||
| // Cache serovalPlugins at module level to avoid repeated calls | ||
| let serovalPlugins: Array<SerovalPlugin<any, any>> | undefined = undefined | ||
|
|
||
| // Known FormData 'Content-Type' header values - module-level constant | ||
| const FORM_DATA_CONTENT_TYPES = [ | ||
| 'multipart/form-data', | ||
| 'application/x-www-form-urlencoded', | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the implementation of getDefaultSerovalPlugins
rg -n 'getDefaultSerovalPlugins' -t ts -t tsx -A 15
# Also check where seroval is imported from
rg -n 'from.*seroval|import.*seroval' -t ts -t tsx
# Check the file structure and surrounding code
cat -n packages/start-server-core/src/server-functions-handler.ts | head -50Repository: TanStack/router
Length of output: 2064
🏁 Script executed:
#!/bin/bash
# Find where serovalPlugins is initialized/assigned
rg -n 'serovalPlugins\s*=' packages/start-server-core/src/server-functions-handler.ts -A 3 -B 1
# Look for more context in the file, particularly around lines 57-60 mentioned in review
sed -n '50,75p' packages/start-server-core/src/server-functions-handler.ts
# Search for getDefaultSerovalPlugins in the client-core package
find packages/start-client-core -name '*.ts' -o -name '*.tsx' | head -20
# Look for any documentation or type definitions for SerovalPlugin
rg -n 'SerovalPlugin' packages/start-client-core -t ts -A 2 -B 1 | head -40Repository: TanStack/router
Length of output: 4979
🏁 Script executed:
#!/bin/bash
# Check the actual seroval Plugin type definition
rg -n 'Plugin' node_modules/seroval/package.json 2>/dev/null || echo "Checking alternative"
# Look for the actual type parameters being used in the parsePayload function
sed -n '65,75p' packages/start-server-core/src/server-functions-handler.ts
# Check if there are any other type safety issues in the file
rg -n ': any' packages/start-server-core/src/server-functions-handler.ts
# Look at how parsePayload uses the plugins
sed -n '65,95p' packages/start-server-core/src/server-functions-handler.tsRepository: TanStack/router
Length of output: 1841
Module-level caching is correct, but TypeScript strict mode compliance is violated throughout the file.
The lazy initialization and caching of serovalPlugins is a safe optimization—the plugins are stateless serialization adapters created from configuration. However, the file extensively violates the coding guideline requiring "TypeScript strict mode with extensive type safety":
- Line 17:
SerovalPlugin<any, any>usesanytype parameters - Lines 30, 64, 114, 148, 154, 156, 229, 281: Multiple function parameters and variables typed as
any
Replace all any type annotations with proper types. For serovalPlugins, use explicit generic parameters from the seroval library. For function parameters like context, payload, and error, provide concrete types instead of any.
🤖 Prompt for AI Agents
In packages/start-server-core/src/server-functions-handler.ts (lines 12–23 and
referenced lines 30, 64, 114, 148, 154, 156, 229, 281), replace all uses of the
any type with concrete types: change serovalPlugins from
Array<SerovalPlugin<any, any>> to an explicit generic such as
Array<SerovalPlugin<unknown, unknown>> or the exact serialization input/output
types from the seroval lib (import and use those actual generics), and update
all function parameters and variables currently typed as any (context, payload,
error, etc.) to the precise project types (e.g., ServerContext,
FunctionInvokePayload, InvokeError or equivalent types exported by your
server/types module) — if those types don't yet exist, define narrow interfaces
for the shape of context, payload and error and import them into this file;
ensure all replacements remove any and use strict nullability/readonly
annotations as appropriate so the file compiles under TypeScript strict mode.
| if (methodLower === 'get') { | ||
| // Get payload directly from searchParams | ||
| const payloadParam = url.searchParams.get('payload') | ||
| // If there's a payload, we should try to parse it | ||
| payload = payload ? parsePayload(JSON.parse(payload)) : {} | ||
| const payload: any = payloadParam | ||
| ? parsePayload(JSON.parse(payloadParam)) | ||
| : {} | ||
| payload.context = { ...context, ...payload.context } | ||
| // Send it through! | ||
| return await action(payload, signal) | ||
| } |
There was a problem hiding this comment.
Add error handling for JSON.parse to prevent unhandled exceptions.
Line 115 calls JSON.parse(payloadParam) without error handling. If the payload query parameter contains malformed JSON, this will throw an uncaught exception during request processing.
🔎 Proposed fix with error handling
if (methodLower === 'get') {
// Get payload directly from searchParams
const payloadParam = url.searchParams.get('payload')
// If there's a payload, we should try to parse it
- const payload: any = payloadParam
- ? parsePayload(JSON.parse(payloadParam))
- : {}
+ let payload: any = {}
+ if (payloadParam) {
+ try {
+ payload = parsePayload(JSON.parse(payloadParam))
+ } catch (error) {
+ throw new Error('Invalid JSON payload in query parameter', { cause: error })
+ }
+ }
payload.context = { ...context, ...payload.context }
// Send it through!
return await action(payload, signal)
}🤖 Prompt for AI Agents
In packages/start-server-core/src/server-functions-handler.ts around lines 110
to 120, the code calls JSON.parse(payloadParam) directly which can throw on
malformed JSON; wrap the JSON.parse call in a try/catch, and on parse failure
handle it gracefully (e.g., log the error and return a controlled client
error/response or throw a specific 400 error with a descriptive message like
"Invalid payload JSON"), only call parsePayload and merge context when parsing
succeeds, and ensure the original flow returns the action(payload, signal) on
success.
fc114d8 to
9f81fca
Compare
schiller-manuel
force-pushed
the
only-createServerFn
branch
from
9f81fca to
9cba6a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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)
117-133: Inconsistent context merge order between GET and POST requests.The context merging logic differs between request methods:
- Line 117 (GET):
{ ...context, ...payload.context }— payload context takes precedence- Line 132 (POST):
{ ...payload.context, ...context }— server context takes precedenceThis inconsistency could lead to different behavior depending on the HTTP method used. Verify this is intentional, otherwise unify the merge order.
🔎 Proposed fix to make POST consistent with GET
const payload = jsonPayload ? parsePayload(jsonPayload) : {} - payload.context = { ...payload.context, ...context } + payload.context = { ...context, ...payload.context } return await action(payload, signal)
♻️ Duplicate comments (4)
packages/start-server-core/src/server-functions-handler.ts (4)
16-23: Module-level caching is a good optimization; TypeScript strict mode issue already flagged.The lazy caching pattern for
serovalPluginsand theFORM_DATA_CONTENT_TYPESconstant extraction are appropriate optimizations. Theanytype parameters on line 17 were already flagged in a previous review.Consider adding
as consttoFORM_DATA_CONTENT_TYPESfor stricter typing:const FORM_DATA_CONTENT_TYPES = [ 'multipart/form-data', 'application/x-www-form-urlencoded', -] +] as const
57-67: Lazy initialization pattern is correct.The lazy initialization of
serovalPluginsis safe in JavaScript's single-threaded event loop and avoids repeated calls togetDefaultSerovalPlugins(). TheparsePayloadhelper cleanly encapsulates the deserialization logic.The
anytype annotations in the function signature and return type were already flagged in the previous review.
110-120: JSON.parse error handling issue already flagged.The lack of error handling for
JSON.parse(payloadParam)on line 115 was already identified in the previous review. Malformed JSON in the query parameter will throw an uncaught exception.
281-291: Helper function is correct.The
isNotFoundResponsefunction properly constructs a 404 response with the error payload. The defensive spread pattern...(headers || {})handles potentially undefined headers correctly.The
anyparameter type was already flagged in the previous review.
🧹 Nitpick comments (4)
packages/start-server-core/src/server-functions-handler.ts (3)
91-104: Consider logging parse failures for serialized context.The empty catch block at line 103 silently swallows JSON.parse errors for the serialized context. While this defensive approach prevents crashes from malformed context, it may hide issues during debugging.
🔎 Proposed improvement
try { const parsedContext = JSON.parse(serializedContext) const deserializedContext = fromJSON(parsedContext, { plugins: serovalPlugins, }) if ( typeof deserializedContext === 'object' && deserializedContext ) { params.context = { ...context, ...deserializedContext } } - } catch {} + } catch (e) { + // Log but continue - malformed context shouldn't fail the request + console.warn('Failed to parse serialized context:', e) + }
150-193: Streaming optimization pattern is well-designed.The approach of first attempting synchronous serialization and falling back to streaming only when needed is efficient. The callback reassignment pattern is unconventional but correct.
Minor nit: the type assertion on line 152 is redundant.
🔎 Remove redundant type assertion
- let done = false as boolean + let done = false
233-239: Remove commented-out code.The commented-out conditional block appears to be dead code. If it's no longer needed after this refactor, remove it to improve readability. If it might be needed in the future, consider documenting why it was removed.
🔎 Proposed removal
} catch (error: any) { if (error instanceof Response) { return error } - // else if ( - // isPlainObject(error) && - // 'result' in error && - // error.result instanceof Response - // ) { - // return error.result - // } // Currently this server-side context has no idea how topackages/start-client-core/src/client-rpc/serverFnFetcher.ts (1)
147-157: Useconsole.errorfor error logging.Line 154 uses
console.log(error)when logging an error before rethrowing. For consistency and proper error severity, this should beconsole.error(error).🔎 Suggested fix
} catch (error) { if (error instanceof Response) { response = error } else { - console.log(error) + console.error(error) throw error } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/start-server-core/src/server-functions-handler.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/server-functions-handler.tspackages/start-client-core/src/client-rpc/serverFnFetcher.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/server-functions-handler.tspackages/start-client-core/src/client-rpc/serverFnFetcher.ts
🧠 Learnings (4)
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{ts,tsx} : Use TypeScript strict mode with extensive type safety for all code
Applied to files:
packages/start-server-core/src/server-functions-handler.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
packages/start-server-core/src/server-functions-handler.ts
📚 Learning: 2025-12-24T22:47:44.320Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6211
File: e2e/react-start/i18n-paraglide/src/server.ts:6-6
Timestamp: 2025-12-24T22:47:44.320Z
Learning: In TanStack Router projects using `inlang/paraglide-js`, the callback passed to `paraglideMiddleware` should use `() => handler.fetch(req)` (referencing the outer `req`) instead of `({ request }) => handler.fetch(request)`. This is intentional because the router needs the untouched URL to perform its own rewrite logic with `deLocalizeUrl`/`localizeUrl`. The middleware's processed request would delocalize the URL and interfere with the router's rewrite handling.
Applied to files:
packages/start-client-core/src/client-rpc/serverFnFetcher.ts
📚 Learning: 2025-08-30T09:12:13.852Z
Learnt from: Sheraff
Repo: TanStack/router PR: 5051
File: packages/router-core/src/utils.ts:310-315
Timestamp: 2025-08-30T09:12:13.852Z
Learning: In TanStack Router's deepEqual utility, using for..in instead of Object.keys() in getObjectKeys() when ignoreUndefined=true is acceptable because it's called only after isPlainObject() checks, which ensure objects have standard Object prototype chains with no inherited enumerable properties.
Applied to files:
packages/start-client-core/src/client-rpc/serverFnFetcher.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). (1)
- GitHub Check: Test
🔇 Additional comments (7)
packages/start-server-core/src/server-functions-handler.ts (1)
1-12: Imports look good.The type-only import for
SerovalPluginwith aliasing is appropriate and follows TypeScript best practices.packages/start-client-core/src/client-rpc/serverFnFetcher.ts (6)
15-27: LGTM!The
hasOwnPropertieshelper is well-implemented with an efficient short-circuit approach. ThehasOwnProperty.callcheck correctly ensures only own enumerable properties are considered, consistent with similar patterns used elsewhere in TanStack Router. Based on learnings, thisfor...inpattern is acceptable when the context ensures proper object types.
39-42: Type inconsistency resolved.The
headersproperty is now correctly typed as optional (headers?: HeadersInit), matching the conditional check at line 45. This addresses the previous review feedback.
44-50: LGTM!Clean header construction with proper conditional handling. The accept header is correctly set only for payload types.
52-68: LGTM!The GET handling correctly rejects FormData (which requires a body) and properly serializes the payload into the query string. The URL construction logic handles both cases (with/without existing query params).
70-77: LGTM!POST handling cleanly delegates to
getFetchBodyand properly sets the content-type header when available.
99-103: LGTM!The combined check
opts.context && hasOwnProperties(opts.context)correctly handles both null/undefined contexts and empty object contexts, ensuring only meaningful context data is serialized.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.