fix(start-client-core): allow middleware to return custom error structures#7364
fix(start-client-core): allow middleware to return custom error structures#7364Zelys-DFKH wants to merge 5 commits intoTanStack:mainfrom
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change refines middleware error propagation: imports ChangesMiddleware Error Propagation
Sequence Diagram(s)sequenceDiagram
participant Client
participant Fetcher
participant executeMiddleware
participant Middleware
Client->>Fetcher: call server function
Fetcher->>executeMiddleware: invoke middleware chain
executeMiddleware->>Middleware: run middleware -> returns { result?, error? }
Middleware->>executeMiddleware: returns result object
executeMiddleware->>executeMiddleware: if isRedirect(error) or error instanceof Error or isNotFound(error) -> throw
executeMiddleware->>Fetcher: else return result.result ?? result.error
Fetcher->>Fetcher: if redirect -> throw redirect
Fetcher->>Client: resolved value or thrown redirect/Error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 c4a563d
☁️ Nx Cloud last updated this comment at |
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/createServerFn.ts (1)
160-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNon-
Errorvalues inresult.errorare silently swallowed at the client boundaryAfter the fix, the client handler path is:
parseRedirect(result.error)— handles redirect objects ✓if (result.error instanceof Error) throw result.error— rethrows actual errors ✓return result.result— returns the resolved valueThe problem: when
result.erroris a non-Errortruthy value (e.g.{ code: 'auth', success: false }) andresult.resultisundefined, the caller silently receivesundefined. The structured error is discarded without any signal.This is reachable in practice:
__executeServerexplicitly shipsd.errorback to the client (line 202), so a non-Errorthrown on the server (captured at the catch block, line 344–348) travels through the HTTP layer and arrives at the client middleware chain, only to be dropped at line 165–166.Consider surfacing non-
Error, non-redirect values throughresult.resultso callers can observe them:💡 Suggested approach
const redirect = parseRedirect(result.error) if (redirect) { throw redirect } if (result.error instanceof Error) throw result.error - return result.result + // Non-Error, non-redirect values in result.error are application-level + // error payloads; surface them as the resolved value when no result is present. + return result.result !== undefined ? result.result : result.error === undefined ? result.result : result.errorOr, more readably:
if (result.error instanceof Error) throw result.error - return result.result + return result.result ?? (result.error !== undefined ? result.error : undefined)If the intent is that middleware must place structured responses in
result.resultand never inresult.error, a comment documenting that invariant here would prevent future confusion:if (result.error instanceof Error) throw result.error + // Non-Error values in result.error are intentionally not forwarded to the caller; + // middleware must place structured responses in `result.result`. return result.result🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/start-client-core/src/createServerFn.ts` around lines 160 - 166, The client-side handler in createServerFn.ts is dropping truthy non-Error errors: after parseRedirect(result.error) and the instanceof Error check, detect when result.error is a non-Error, non-redirect value (e.g., an object like {code:'auth'}) and surface it via the normal success path instead of swallowing it — e.g., assign that value into result.result (or return it directly) so callers receive the structured payload; update the branch around parseRedirect/result.error/result.result to return non-Error errors as the result, and add a short comment documenting that invariant for future readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/start-client-core/src/createServerFn.ts`:
- Around line 160-166: The client-side handler in createServerFn.ts is dropping
truthy non-Error errors: after parseRedirect(result.error) and the instanceof
Error check, detect when result.error is a non-Error, non-redirect value (e.g.,
an object like {code:'auth'}) and surface it via the normal success path instead
of swallowing it — e.g., assign that value into result.result (or return it
directly) so callers receive the structured payload; update the branch around
parseRedirect/result.error/result.result to return non-Error errors as the
result, and add a short comment documenting that invariant for future readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec769bbe-644c-4188-8ce5-ea14fa53f787
📒 Files selected for processing (1)
packages/start-client-core/src/createServerFn.ts
…tures from catch blocks Middleware that wanted to return structured error responses from catch blocks would have those errors thrown immediately to the client. The middleware protocol was using `.error` property for two incompatible purposes: framework errors (Error instances) and application error data (plain objects). This fix uses JavaScript's type system to distinguish them: only Error instances are thrown, allowing middleware to return custom error structures while preserving proper error propagation for real framework errors. Fixes TanStack#7238 BREAKING CHANGE: Middleware that throw non-Error values will now have those values captured in result.error instead of being thrown to the client. Only affects non-standard code patterns; best practice is to throw Error instances.
Zelys-DFKH
force-pushed
the
fix-shallow-error-plugin-custom-props
branch
from
e6214af to
1abc348
Compare
Merging this PR will not alter performance
Comparing 
Footnotes
|
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The ?? operator treats null as a value, not nullish, so null ?? undefined returns null. However, we need to distinguish between: 1. result.result explicitly set to null (should return null) 2. result.result undefined (should fallback to result.error for custom error payloads) Change to: result.result !== undefined ? result.result : result.error This preserves null while still supporting middleware error payloads. Fixes TanStack#7364
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We extended the instanceof Error guards at lines 165 and 312 of createServerFn.ts to also re-throw TanStack Router's redirect() and notFound() framework signals, which are not Error instances. This fixes the failing SSR and client-navigation tests by ensuring those signals propagate to the router instead of being silently returned as application data.
Tip
✅ We verified this fix by re-running tanstack-react-start-e2e-rsc:test:e2e--vite-ssr--shard-5-of-6.
Suggested Fix changes
diff --git a/packages/start-client-core/src/createServerFn.ts b/packages/start-client-core/src/createServerFn.ts
index ba9f7920..7a5a8e98 100644
--- a/packages/start-client-core/src/createServerFn.ts
+++ b/packages/start-client-core/src/createServerFn.ts
@@ -1,6 +1,6 @@
import { mergeHeaders } from '@tanstack/router-core/ssr/client'
-import { isRedirect, parseRedirect } from '@tanstack/router-core'
+import { isNotFound, isRedirect, parseRedirect } from '@tanstack/router-core'
import { TSS_SERVER_FUNCTION_FACTORY } from './constants'
import { getStartOptions } from './getStartOptions'
import { getStartContextServerOnly } from './getStartContextServerOnly'
@@ -162,7 +162,12 @@ export const createServerFn: CreateServerFn<Register> = (options, __opts) => {
throw redirect
}
- if (result.error instanceof Error) throw result.error
+ if (
+ result.error instanceof Error ||
+ isRedirect(result.error) ||
+ isNotFound(result.error)
+ )
+ throw result.error
// Non-Error values in result.error are application-level error payloads;
// return them as the resolved value when no explicit result is present.
return result.result !== undefined ? result.result : result.error
@@ -304,7 +309,11 @@ export async function executeMiddleware(
const result = await callNextMiddleware(nextCtx)
- if (result.error instanceof Error) {
+ if (
+ result.error instanceof Error ||
+ isRedirect(result.error) ||
+ isNotFound(result.error)
+ ) {
throw result.error
}
Because this branch comes from a fork, it is not possible for us to apply fixes directly, but you can apply the changes locally using the available options below.
Apply changes locally with:
npx nx-cloud apply-locally at1I-bhFr
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
When unwinding middleware, redirect() and notFound() framework signals must be re-thrown to reach the router—they are not Error instances. The previous instanceof Error guard would silently return them as application data, breaking SSR and client-navigation flow detection. Extended both error-handling locations (client fetcher and middleware executor) to also check isRedirect() and isNotFound(), ensuring framework signals propagate correctly while custom error payloads continue to be returned as resolved values.
1 participant
Fixes #7238
Problem
Middleware that wanted to return structured error responses from catch blocks would have those errors thrown immediately to the client, preventing global error classification middleware from working:
The middleware protocol was using
.errorproperty for two incompatible purposes:This collision meant middleware couldn't distinguish between "I'm returning an error as application data" and "this is a framework-level failure that should be thrown."
Solution
Use JavaScript's type system to distinguish errors:
This allows middleware to return custom error structures while preserving proper error propagation for real framework errors.
Changes
Breaking Change (Minor)
Middleware that throw non-Error values will now have those values captured in `result.error` instead of being thrown to the client.
Impact: Only affects middleware using non-standard error throwing patterns. Best practice is to throw `Error` instances.
Summary by CodeRabbit