fix(start-server-core): return a Response to non-RPC server function callers - #8220
fix(start-server-core): return a Response to non-RPC server function callers#8220theRizwan wants to merge 1 commit into
Conversation
…callers A server function invoked through `serverFn.url`, for example as the action of a native HTML form, does not send the `x-tsr-serverFn` header that the RPC fetcher sends. That branch returned `res.result || res.error` directly, so a handler that returned a plain object, returned nothing, or produced an error handed a raw JS value back to the HTTP layer. `getFinalResponse` then found no response on the context and threw ERR_NO_RESPONSE, surfacing as an unhandled 500 with "It looks like you forgot to return a response from your server route handler". Only handlers that returned a `Response` worked. Every caller went through the serialization path before the middleware refactor in TanStack#5517, which added this shortcut. Restrict the shortcut to values that are already a `Response`, which includes redirects since `redirect()` returns a `Response` subclass, and let everything else fall through to `serializeResult`. RPC callers are untouched, and a handler-provided `Response` still reaches a browser without the internal `x-tss-raw` marker.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughChangesServer function response handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Non-RPC server-function callers now consistently receive an HTTP Response while existing redirects, handler-provided responses, and RPC behavior remain unchanged. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes directly address issue Full details: Docstring CoverageExplanation Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (2 skipped: 2 unsupported.) ✨ 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 |
🎯 Changes
Fixes #7745.
Posting a native HTML form to
serverFn.urlfailed with an unhandled 500:A native form submission is not an RPC call, so it does not send the
x-tsr-serverFnheader that the RPC fetcher sends. InhandleServerActionthat branch returned the unwrapped handler value straight out:For a handler that returns a plain object, returns nothing, or produces an error,
unwrappedis a raw JS value rather than aResponse. It flows back throughexecuteMiddleware, wherehandleCtxResultonly recognisesResponse, redirect andSsrResponse, soctx.responseis never set andgetFinalResponsethrowsERR_NO_RESPONSE. Only handlers that already returned aResponseworked, which matches the reporter's workaround.This path is a regression from the middleware refactor in #5517, which introduced the shortcut. Before it, every caller went through the serialization path and always produced a
Response.The fix
Restrict the shortcut to values that are already a
Response, and let everything else fall through to the existingserializeResult:Redirects are covered by the same check, because
redirect()returns aResponsesubclass andisRedirectisobj instanceof Response && !!obj.options. So a form action that returnsredirect()still passes through byte for byte, and a handler-providedResponsestill reaches the browser without the internalx-tss-rawmarker that only RPC callers need.Non-RPC callers now get the same
application/jsonbody the RPC path produces, which is the pre-#5517 behaviour. If you would rather non-RPC callers receive plainResponse.json(value)instead of the seroval encoding, or a non-redirect error mapped to a 500 rather than a 200, I am happy to change it. I kept the restoration minimal because those are format decisions rather than part of the bug.Tests
New
packages/start-server-core/tests/server-functions-handler.test.tsdriveshandleServerActionthroughrequestHandlerandrunWithStartContextand asserts on the value it returns, since the invariant being broken is that it must always return aResponse.Three tests fail on
mainand pass with this change:{ ok: true }application/jsonundefinedErrorResponseFour more are regression guards that pass both before and after: a handler-provided
Responsepasses through with its status, content type and nox-tss-rawmarker, aredirect()keeps its 302 andLocation, and both RPC paths keep serializing and marking exactly as they did.Docs
Added a short note to the Progressive Enhancement section explaining what a native form submission receives, and that a
redirect()is usually what you want so the browser does not stay on the server function URL.✅ Checklist
Local verification:
pnpm nx run @tanstack/start-server-core:test:unitpasses, 8 files and 125 testspnpm nx run @tanstack/start-server-core:test:typespassespnpm nx run @tanstack/start-server-core:test:eslintpassesnode scripts/verify-links.tspasses for the docs change🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Documentation