fix: undefined / null serialization #5501
Conversation
WalkthroughThis PR fixes a regression where null values sent to server functions were converted to undefined, and adds SSR query integration. It refactors client-side payload serialization and server-side parsing logic, introduces a new primitives test route to validate type handling, and extends routing to support the test endpoint. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ServerFn as Server<br/>(Fetcher)
participant Handler as Server<br/>(Handler)
participant DB as Test Route
rect rgb(220, 240, 255)
Note over Client,Handler: Old Flow (Bug: null → undefined)
Client->>ServerFn: POST { body: null }
ServerFn->>ServerFn: serializePayload()<br/>(treats null as falsy)
ServerFn->>Handler: payload: undefined
Handler->>DB: receives undefined
end
rect rgb(220, 255, 220)
Note over Client,Handler: New Flow (Fixed: null preserved)
Client->>ServerFn: POST { body: null }
ServerFn->>ServerFn: serializePayload()<br/>(explicitly checks for null)
ServerFn->>Handler: payload: "null"<br/>(serialized JSON)
Handler->>Handler: parsePayload()<br/>(JSON.parse preserves null)
Handler->>DB: receives null
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR spans multiple layers (client fetcher, server handler, routing, test harness) with heterogeneous changes. The core logic modifications in payload serialization and null-preservation require careful reasoning to ensure type safety and backward compatibility, while the new test route introduces a complex component hierarchy with strongly-typed generics and React Query integration. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 7695b8a
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
![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 (2)
e2e/react-start/server-functions/src/router.tsx (1)
1-6: Fix import order to satisfy lintThe new package imports must precede relative imports; otherwise the
import/orderrule keeps failing. Reorder these top-level imports so CI passes.-import { createRouter } from '@tanstack/react-router' -import { routeTree } from './routeTree.gen' -import { DefaultCatchBoundary } from './components/DefaultCatchBoundary' -import { NotFound } from './components/NotFound' -import { setupRouterSsrQueryIntegration } from '@tanstack/react-router-ssr-query' -import { QueryClient } from '@tanstack/react-query' +import { QueryClient } from '@tanstack/react-query' +import { setupRouterSsrQueryIntegration } from '@tanstack/react-router-ssr-query' +import { createRouter } from '@tanstack/react-router' +import { routeTree } from './routeTree.gen' +import { DefaultCatchBoundary } from './components/DefaultCatchBoundary' +import { NotFound } from './components/NotFound'As per coding guidelines
packages/start-server-core/src/server-functions-handler.ts (1)
135-135: Add validation or fallback for undefined jsonPayload when handling non-createServerFn POST requests.Line 135 spreads
jsonPayloadwithout checking if it's defined. For non-createServerFn POST requests with a content-type other thanapplication/json,jsonPayloadremainsundefined, causing a TypeError. Unlike theisCreateServerFnbranch (line 129) which safely handles this withjsonPayload ? parsePayload(jsonPayload) : {}, the non-createServerFn path has no such fallback.Either add validation to reject non-JSON POST requests, or provide a safe default like:
return await action(...(jsonPayload ?? []))
🧹 Nitpick comments (1)
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (1)
55-59: Avoid double serializing the GET payload
serializePayloadalready ran on Line 55, but we invoke it again when buildingencodedPayload. Seroval serialization is comparatively heavy, so this double work adds needless latency. Reuse the cached string instead.- const serializedPayload = await serializePayload(first) - if (serializedPayload !== undefined) { - const encodedPayload = encode({ - payload: await serializePayload(first), - }) + const serializedPayload = await serializePayload(first) + if (serializedPayload !== undefined) { + const encodedPayload = encode({ + payload: serializedPayload, + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
e2e/react-start/server-functions/package.json(1 hunks)e2e/react-start/server-functions/src/routeTree.gen.ts(11 hunks)e2e/react-start/server-functions/src/router.tsx(2 hunks)e2e/react-start/server-functions/src/routes/primitives/index.tsx(1 hunks)e2e/react-start/server-functions/tests/server-functions.spec.ts(1 hunks)packages/start-client-core/src/client-rpc/serverFnFetcher.ts(5 hunks)packages/start-server-core/src/server-functions-handler.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/router.tsxpackages/start-client-core/src/client-rpc/serverFnFetcher.tse2e/react-start/server-functions/src/routeTree.gen.tse2e/react-start/server-functions/src/routes/primitives/index.tsxpackages/start-server-core/src/server-functions-handler.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/router.tsxe2e/react-start/server-functions/package.jsone2e/react-start/server-functions/src/routeTree.gen.tse2e/react-start/server-functions/src/routes/primitives/index.tsx
packages/{*-start,start-*}/**
📄 CodeRabbit inference engine (AGENTS.md)
Name and place Start framework packages under packages/-start/ or packages/start-/
Files:
packages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/start-server-core/src/server-functions-handler.ts
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use workspace:* protocol for internal dependencies in package.json files
Files:
e2e/react-start/server-functions/package.json
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/server-functions/src/routes/primitives/index.tsx
🧬 Code graph analysis (2)
e2e/react-start/server-functions/src/router.tsx (1)
packages/react-router-ssr-query/src/index.tsx (1)
setupRouterSsrQueryIntegration(12-29)
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (1)
packages/start-client-core/src/constants.ts (1)
TSS_FORMDATA_CONTEXT(1-1)
🪛 ESLint
e2e/react-start/server-functions/src/router.tsx
[error] 5-5: @tanstack/react-router-ssr-query import should occur before import of ./routeTree.gen
(import/order)
[error] 6-6: @tanstack/react-query import should occur before import of ./routeTree.gen
(import/order)
e2e/react-start/server-functions/src/routes/primitives/index.tsx
[error] 5-5: Expected 1 empty line after import statement not followed by another import.
(import/newline-after-import)
⏰ 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
| payload = payload ? parsePayload(JSON.parse(payload)) : {} | ||
| payload.context = { ...context, ...payload.context } |
There was a problem hiding this comment.
Risk: Potential null-pointer exception if payload is null.
While the string check on line 109 correctly handles the string "null" by passing the parsed null value to parsePayload(), if parsePayload() returns null (which it should to preserve the null value), line 110 will throw a TypeError when attempting to access .context on null.
Consider wrapping the payload in a safe structure or checking if it's an object before accessing properties:
let payload: any = search.payload
payload = payload ? parsePayload(JSON.parse(payload)) : {}
-payload.context = { ...context, ...payload.context }
+if (typeof payload === 'object' && payload !== null) {
+ payload.context = { ...context, ...payload.context }
+} else {
+ payload = { data: payload, context }
+}Alternatively, if primitives should not be supported for GET requests, add validation and return an error.
📝 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.
| payload = payload ? parsePayload(JSON.parse(payload)) : {} | |
| payload.context = { ...context, ...payload.context } | |
| payload = payload ? parsePayload(JSON.parse(payload)) : {} | |
| if (typeof payload === 'object' && payload !== null) { | |
| payload.context = { ...context, ...payload.context } | |
| } else { | |
| payload = { data: payload, context } | |
| } |
🤖 Prompt for AI Agents
In packages/start-server-core/src/server-functions-handler.ts around lines
109-110, payload can be null after parsePayload(JSON.parse(payload)) which
causes a TypeError when accessing payload.context; fix by checking that payload
is a non-null object before merging context (e.g., if typeof payload ===
'object' && payload !== null then merge context into payload.context, otherwise
set payload = { context: { ...context } } or return a validation error for
unsupported primitives), ensuring no property access occurs on null.
| if (!contentType || !contentType.includes('application/json')) { | ||
| throw new Error('expected application/json content type') | ||
| let jsonPayload | ||
| if (contentType === 'application/json') { |
There was a problem hiding this comment.
Content-type check should handle charset and other parameters.
The exact equality check contentType === 'application/json' will fail if the header includes additional parameters like application/json; charset=utf-8, which is common with fetch requests. The existing FormData check on lines 71-73 uses .includes() for this reason.
Apply this diff to make the check more robust:
-if (contentType === 'application/json') {
+if (contentType?.includes('application/json')) {
jsonPayload = await request.json()
}📝 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 (contentType === 'application/json') { | |
| if (contentType?.includes('application/json')) { |
🤖 Prompt for AI Agents
In packages/start-server-core/src/server-functions-handler.ts around line 120,
the content-type equality check uses `contentType === 'application/json'` which
fails when the header contains parameters like charset; update the check to
safely handle null/undefined and match types with parameters by using a
normalized check such as
`contentType?.toLowerCase().startsWith('application/json')` (or
`.includes('application/json')`) so requests like `application/json;
charset=utf-8` are accepted.
| const payload = jsonPayload ? parsePayload(jsonPayload) : {} | ||
| payload.context = { ...payload.context, ...context } | ||
| return await action(payload, signal) |
There was a problem hiding this comment.
Critical: Falsy check converts null to empty object, breaking null serialization.
This is the root cause of issue #5416. When jsonPayload is null, the falsy check jsonPayload ? evaluates to false and returns {} instead of preserving the null value. The same issue affects other falsy primitives like 0, false, and "".
Additionally, line 128 will throw a TypeError if payload is null or any non-object primitive when attempting to access .context.
Apply this diff to preserve null and other falsy values while still defaulting to {} for missing payloads:
-const payload = jsonPayload ? parsePayload(jsonPayload) : {}
-payload.context = { ...payload.context, ...context }
+let payload = jsonPayload !== undefined ? parsePayload(jsonPayload) : {}
+if (typeof payload === 'object' && payload !== null) {
+ payload.context = { ...payload.context, ...context }
+} else {
+ payload = { data: payload, context }
+}This change:
- Preserves
nullby checking!== undefinedinstead of truthiness - Prevents
TypeErrorby checking if payload is an object before accessing.context - Wraps primitives in an object structure if needed
📝 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.
| const payload = jsonPayload ? parsePayload(jsonPayload) : {} | |
| payload.context = { ...payload.context, ...context } | |
| return await action(payload, signal) | |
| let payload = jsonPayload !== undefined ? parsePayload(jsonPayload) : {} | |
| if (typeof payload === 'object' && payload !== null) { | |
| payload.context = { ...payload.context, ...context } | |
| } else { | |
| payload = { data: payload, context } | |
| } | |
| return await action(payload, signal) |
🤖 Prompt for AI Agents
In packages/start-server-core/src/server-functions-handler.ts around lines 127
to 129, the current truthy check converts null and other falsy primitives to {}
and then accessing payload.context can throw; change the logic to use
jsonPayload !== undefined when deciding to call parsePayload so null (and other
falsy values) are preserved by parsePayload, then ensure payload is an object
before mutating .context: if payload is null or a non-object primitive, wrap it
into an object (e.g., { value: payload }) so you can safely set payload.context;
finally, default to {} only when jsonPayload is actually undefined.
fixes #5416
Summary by CodeRabbit
New Features
Improvements
Tests