fix: refactor middleware #5517
fix: refactor middleware #5517
Conversation
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 24m 27s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 28s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-10-19 21:07:11 UTC
WalkthroughAdds a new E2E route /middleware/unhandled-exception with an auth middleware that throws; refactors server/client middleware and server-function request/response handling across start-client-core and start-server-core, and updates exports/constants and client fetcher behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller (client/serverFn)
participant Runner as createServerFn.execute
participant Chain as callNextMiddleware
participant MW as Middleware (authMiddleware / others)
participant Handler as Final Handler / ServerFn
participant Fetcher as serverFnFetcher
participant Server as start-server-core handler
Client->>Fetcher: invoke(serverFn args, handler)
Fetcher->>Server: proxied fetch with serverFn headers
Server->>Runner: handleServerAction -> execute middlewares
Runner->>Chain: start(index=0, ctx)
Chain->>MW: execute(ctx, userNext)
alt MW calls userNext
MW->>Chain: userNext(mergedCtx) -> next index
else MW throws
MW-->>Runner: error (propagated)
end
opt reached handler
Chain->>Handler: invoke with final ctx
Handler-->>Server: result or error
end
Server-->>Fetcher: serialized or raw response (NDJSON/JSON/Response)
Fetcher-->>Client: parsed/streamed result or redirect/error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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: 2
🧹 Nitpick comments (3)
packages/start-client-core/src/createServerFn.ts (3)
259-263: Check for undefined explicitly and improve the error message.
if (!(result as any))treats falsy values as undefined and the message doesn’t include context. Prefer a strict undefined check and include env/functionId for easier debugging.- if (!(result as any)) { - throw new Error( - 'User middleware returned undefined. You must call next() or return a result in your middlewares.', - ) - } + if (typeof result === 'undefined') { + throw new Error( + `User middleware returned undefined (env=${env}, functionId=${opts.functionId}). You must call next() or return a result.` + ) + }
182-190: Optional: avoid mutating the flattened array with shift().Using
shift()mutates shared state in the closure. An index-basedcallNextMiddleware(ctx, i)is clearer and safer if you ever need re-entrancy or debug indices.I can provide a small refactor to pass an index parameter instead of mutating the array if you want to pursue this.
146-154: Style consistency: prefer await over .then here.Elsewhere you use
await; using it here keeps style consistent and simplifies stack traces.- return executeMiddleware(resolvedMiddleware, 'server', ctx).then( - (d) => ({ - // Only send the result and sendContext back to the client - result: d.result, - error: d.error, - context: d.sendContext, - }), - ) + const d = await executeMiddleware(resolvedMiddleware, 'server', ctx) + return { + // Only send the result and sendContext back to the client + result: d.result, + error: d.error, + context: d.sendContext, + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
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/middleware/unhandled-exception.tsx(1 hunks)packages/start-client-core/src/createServerFn.ts(3 hunks)packages/start-client-core/src/index.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- packages/start-client-core/src/index.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/src/routes/index.tsxe2e/react-start/server-functions/src/routeTree.gen.tse2e/react-start/server-functions/src/routes/middleware/unhandled-exception.tsxpackages/start-client-core/src/createServerFn.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/server-functions/src/routes/index.tsxe2e/react-start/server-functions/src/routes/middleware/unhandled-exception.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/server-functions/src/routes/index.tsxe2e/react-start/server-functions/src/routeTree.gen.tse2e/react-start/server-functions/src/routes/middleware/unhandled-exception.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/createServerFn.ts
🧬 Code graph analysis (2)
e2e/react-start/server-functions/src/routes/middleware/unhandled-exception.tsx (1)
packages/start-client-core/src/createServerFn.ts (1)
createServerFn(48-169)
packages/start-client-core/src/createServerFn.ts (2)
packages/start-client-core/src/index.tsx (4)
executeMiddleware(79-79)NextFn(71-71)ServerFnMiddlewareResult(68-68)mergeHeaders(6-6)packages/router-core/src/ssr/client.ts (1)
mergeHeaders(1-1)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (3)
e2e/react-start/server-functions/src/routes/index.tsx (1)
91-95: LGTM: adds nav to new middleware error route.e2e/react-start/server-functions/src/routes/middleware/unhandled-exception.tsx (1)
1-33: Intent check: loader will reject due to middleware throw.This looks correct for exercising the unhandled-exception path. Ensure the E2E asserts the error boundary/response instead of relying on the component render.
Can you confirm the E2E expects a rejected loader (e.g., checks for router error state) rather than querying
data-testid="regular-person"?e2e/react-start/server-functions/src/routeTree.gen.ts (1)
30-33: Generated: additions look consistent with the new route.No action; keep as-is.
Also applies to: 127-132, 180-181, 206-207, 233-234, 261-262, 287-288, 313-314, 340-341, 477-483
| // Execute the middleware | ||
| try { | ||
| if (middlewareFn) { | ||
| const userNext = async ( | ||
| userCtx: ServerFnMiddlewareResult | undefined = {} as any, | ||
| ) => { | ||
| // Return the next middleware | ||
| const nextCtx = { | ||
| ...ctx, | ||
| ...userCtx, | ||
| context: { | ||
| ...ctx.context, | ||
| ...userCtx.context, | ||
| }, | ||
| sendContext: { | ||
| ...ctx.sendContext, | ||
| ...(userCtx.sendContext ?? {}), | ||
| }, | ||
| headers: mergeHeaders(ctx.headers, userCtx.headers), | ||
| result: | ||
| userCtx.result !== undefined | ||
| ? userCtx.result | ||
| : userCtx instanceof Response | ||
| ? userCtx | ||
| : (ctx as any).result, | ||
| error: userCtx.error ?? (ctx as any).error, | ||
| } | ||
|
|
||
| try { | ||
| return await callNextMiddleware(nextCtx) | ||
| } catch (error: any) { | ||
| return { | ||
| ...newCtx, | ||
| ...nextCtx, | ||
| error, | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prevent multiple next() calls within a single middleware.
As written, a middleware can call next() multiple times, advancing the chain repeatedly and producing hard-to-debug behavior. Add a simple guard.
- if (middlewareFn) {
- const userNext = async (
+ if (middlewareFn) {
+ let nextCalled = false
+ const userNext = async (
userCtx: ServerFnMiddlewareResult | undefined = {} as any,
) => {
+ if (nextCalled) {
+ throw new Error('next() called multiple times in a middleware')
+ }
+ nextCalled = true
// Return the next middleware
const nextCtx = {
...ctx,
...userCtx,
context: {
...ctx.context,
...userCtx.context,
},
sendContext: {
...ctx.sendContext,
...(userCtx.sendContext ?? {}),
},
headers: mergeHeaders(ctx.headers, userCtx.headers),
result:
userCtx.result !== undefined
? userCtx.result
: userCtx instanceof Response
? userCtx
: (ctx as any).result,
error: userCtx.error ?? (ctx as any).error,
}
try {
return await callNextMiddleware(nextCtx)
} catch (error: any) {
return {
...nextCtx,
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.
| // Execute the middleware | |
| try { | |
| if (middlewareFn) { | |
| const userNext = async ( | |
| userCtx: ServerFnMiddlewareResult | undefined = {} as any, | |
| ) => { | |
| // Return the next middleware | |
| const nextCtx = { | |
| ...ctx, | |
| ...userCtx, | |
| context: { | |
| ...ctx.context, | |
| ...userCtx.context, | |
| }, | |
| sendContext: { | |
| ...ctx.sendContext, | |
| ...(userCtx.sendContext ?? {}), | |
| }, | |
| headers: mergeHeaders(ctx.headers, userCtx.headers), | |
| result: | |
| userCtx.result !== undefined | |
| ? userCtx.result | |
| : userCtx instanceof Response | |
| ? userCtx | |
| : (ctx as any).result, | |
| error: userCtx.error ?? (ctx as any).error, | |
| } | |
| try { | |
| return await callNextMiddleware(nextCtx) | |
| } catch (error: any) { | |
| return { | |
| ...newCtx, | |
| ...nextCtx, | |
| error, | |
| } | |
| } | |
| } | |
| // Execute the middleware | |
| try { | |
| if (middlewareFn) { | |
| let nextCalled = false | |
| const userNext = async ( | |
| userCtx: ServerFnMiddlewareResult | undefined = {} as any, | |
| ) => { | |
| if (nextCalled) { | |
| throw new Error('next() called multiple times in a middleware') | |
| } | |
| nextCalled = true | |
| // Return the next middleware | |
| const nextCtx = { | |
| ...ctx, | |
| ...userCtx, | |
| context: { | |
| ...ctx.context, | |
| ...userCtx.context, | |
| }, | |
| sendContext: { | |
| ...ctx.sendContext, | |
| ...(userCtx.sendContext ?? {}), | |
| }, | |
| headers: mergeHeaders(ctx.headers, userCtx.headers), | |
| result: | |
| userCtx.result !== undefined | |
| ? userCtx.result | |
| : userCtx instanceof Response | |
| ? userCtx | |
| : (ctx as any).result, | |
| error: userCtx.error ?? (ctx as any).error, | |
| } | |
| try { | |
| return await callNextMiddleware(nextCtx) | |
| } catch (error: any) { | |
| return { | |
| ...nextCtx, | |
| error, | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/start-client-core/src/createServerFn.ts around lines 214 to 251, the
userNext function allows a middleware to call next() multiple times; add a
simple reentrancy guard inside the userNext closure (e.g., a boolean "called"
flag set on first invocation) that throws or returns an error if next() is
invoked again, ensuring subsequent calls are rejected; keep the rest of next's
behavior (building nextCtx, calling callNextMiddleware, and catching errors)
unchanged but bail early when the guard detects a repeated call.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/start-client-core/src/createServerFn.ts (2)
114-125: Still dropping caller-provided context in client fetcher.Line 120 sets
context: {}, ignoring anyopts?.contextpassed by the caller. This prevents client-side middlewares from receiving user-supplied context.Apply this fix:
const result = await executeMiddleware(resolvedMiddleware, 'client', { ...extractedFn, ...newOptions, data: opts?.data as any, headers: opts?.headers, signal: opts?.signal, - context: {}, + context: opts?.context ?? {}, })
218-252: Still missing guard against multiple next() calls.The
userNextfunction allows a middleware to callnext()multiple times, which can advance the chain repeatedly and cause unpredictable behavior.Apply this diff to add a reentrancy guard:
if (middlewareFn) { + let nextCalled = false const userNext = async ( userCtx: ServerFnMiddlewareResult | undefined = {} as any, ) => { + if (nextCalled) { + throw new Error('next() called multiple times in a middleware') + } + nextCalled = true // Return the next middleware const nextCtx = { ...ctx,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/start-client-core/src/createServerFn.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/start-client-core/src/createServerFn.ts
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/createServerFn.ts
🧬 Code graph analysis (1)
packages/start-client-core/src/createServerFn.ts (1)
packages/start-client-core/src/index.tsx (6)
executeMiddleware(79-79)NextFn(71-71)execValidator(77-77)MiddlewareFn(66-66)ServerFnMiddlewareResult(68-68)mergeHeaders(6-6)
⏰ 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: Test
- GitHub Check: Preview
🔇 Additional comments (2)
packages/start-client-core/src/createServerFn.ts (2)
182-217: LGTM: Middleware selection and validation logic.The refactored
callNextMiddlewarecorrectly shifts through the middleware chain, validates input on the server side, and selects the appropriate middleware function based on the environment.
269-285: LGTM: Fallback handling and chain initialization.The fallback correctly continues the middleware chain when no middleware function is present, and the chain initialization properly sets up the initial context with appropriate defaults.
schiller-manuel
force-pushed
the
fix/refactor-middleware
branch
from
621a2aa to
ed54879
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
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/createStartHandler.ts (1)
92-116: Per-request global fetch override is unsafe (cross-request race/leak).Overwriting globalThis.fetch per request can affect concurrent requests (origin/requestOpts bleed), producing incorrect routing and privacy issues. At minimum, restore in a finally; preferably use an ALS-scoped wrapper or pass a handler instead of patching global.
Apply a minimal containment fix:
- globalThis.fetch = async function (input, init) { + const prevFetch = globalThis.fetch + globalThis.fetch = async function (input, init) { function resolve(url: URL, requestOptions: RequestInit | undefined) { const fetchRequest = new Request(url, requestOptions) return startRequestResolver(fetchRequest, requestOpts) } ... - } as typeof fetch + } as typeof fetch + try { + // existing logic continues... + // ... + } finally { + globalThis.fetch = prevFetch + }If you want a robust fix, wrap a single global fetch once with AsyncLocalStorage and consult per-request context to resolve same-origin URLs. I can draft that if helpful.
♻️ Duplicate comments (3)
packages/start-client-core/src/createServerFn.ts (3)
228-262: Guard against multiple next() calls in a middleware.A middleware can call next() multiple times and reenter the chain.
- const userNext = async ( + let nextCalled = false + const userNext = async ( userCtx: ServerFnMiddlewareResult | undefined = {} as any, ) => { + if (nextCalled) { + throw new Error('next() called multiple times in a middleware') + } + nextCalled = true // Return the next middleware const nextCtx = { ...ctx, ...userCtx, context: { ...ctx.context, ...userCtx.context, },
115-123: Don’t drop caller-provided context.You’re passing context: {} and ignore opts?.context; this blocks client middlewares from receiving user context.
- context: {}, + context: opts?.context ?? {},
286-290: Bug: undefined check rejects valid falsy middleware returns.
!(result as any)treats 0/''/false as undefined; check strictly for undefined.- if (!(result as any)) { + if (result === undefined) { throw new Error( 'User middleware returned undefined. You must call next() or return a result in your middlewares.', ) }
🧹 Nitpick comments (5)
e2e/react-start/server-functions/src/routes/submit-post-formdata.tsx (1)
36-36: Remove unnecessary empty string template literal.The
{''}at the end of the line renders nothing and appears to be a leftover from editing. Consider removing it or replacing with a colon for better readability:Apply this diff:
- It should navigate to a raw response of {''} + It should navigate to a raw response of:Or simply:
- It should navigate to a raw response of {''} + It should navigate to a raw responsepackages/start-server-core/src/createStartHandler.ts (1)
202-211: Return 406 for unsupported Accept instead of 500.Not an internal error; 406 Not Acceptable fits better.
- return json( - { error: 'Only HTML requests are supported here' }, - { status: 500 }, - ) + return json( + { error: 'Only HTML requests are supported here' }, + { status: 406 }, + )packages/start-client-core/src/client-rpc/serverFnFetcher.ts (2)
66-70: Avoid double serialization work.serializePayload() is called twice; reuse the computed value.
- 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, + })
175-181: Avoid unconditional console.log on errors.Noise and potential sensitive data leak. Use warning in dev only.
- console.log(error) + if (process.env.NODE_ENV === 'development') { + // console.warn(error) + }packages/start-server-core/src/server-functions-handler.ts (1)
288-291: Optional: return 204 No Content for undefined results.serializeResult(undefined) currently responds with empty body and no content-type; set a 204 to clarify semantics.
- return new Response(undefined, { - status: alsResponse?.status, - statusText: alsResponse?.statusText, - }) + return new Response(null, { + status: alsResponse?.status ?? 204, + statusText: alsResponse?.statusText, + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/react-start/server-functions/src/routes/submit-post-formdata.tsx(1 hunks)packages/start-client-core/src/client-rpc/serverFnFetcher.ts(7 hunks)packages/start-client-core/src/constants.ts(1 hunks)packages/start-client-core/src/createServerFn.ts(6 hunks)packages/start-client-core/src/index.tsx(1 hunks)packages/start-server-core/src/createStartHandler.ts(2 hunks)packages/start-server-core/src/server-functions-handler.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/start-client-core/src/index.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/start-server-core/src/createStartHandler.tspackages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/start-client-core/src/constants.tspackages/start-client-core/src/createServerFn.tspackages/start-server-core/src/server-functions-handler.tse2e/react-start/server-functions/src/routes/submit-post-formdata.tsx
packages/{*-start,start-*}/**
📄 CodeRabbit inference engine (AGENTS.md)
Name and place Start framework packages under packages/-start/ or packages/start-/
Files:
packages/start-server-core/src/createStartHandler.tspackages/start-client-core/src/client-rpc/serverFnFetcher.tspackages/start-client-core/src/constants.tspackages/start-client-core/src/createServerFn.tspackages/start-server-core/src/server-functions-handler.ts
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/server-functions/src/routes/submit-post-formdata.tsx
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/server-functions/src/routes/submit-post-formdata.tsx
🧬 Code graph analysis (4)
packages/start-client-core/src/client-rpc/serverFnFetcher.ts (1)
packages/start-client-core/src/constants.ts (1)
X_TSS_SERIALIZED(7-7)
packages/start-client-core/src/constants.ts (1)
packages/start-client-core/src/index.tsx (1)
X_TSS_CONTEXT(87-87)
packages/start-client-core/src/createServerFn.ts (1)
packages/router-core/src/index.ts (3)
redirect(370-370)parseRedirect(373-373)isRedirect(371-371)
packages/start-server-core/src/server-functions-handler.ts (3)
packages/start-client-core/src/index.tsx (3)
json(6-6)X_TSS_RAW_RESPONSE(86-86)X_TSS_SERIALIZED(85-85)packages/start-client-core/src/constants.ts (2)
X_TSS_RAW_RESPONSE(8-8)X_TSS_SERIALIZED(7-7)packages/start-server-core/src/request-response.ts (1)
getResponse(328-331)
🔇 Additional comments (1)
packages/start-client-core/src/constants.ts (1)
9-9: X_TSS_CONTEXT is properly integrated into the public API.The constant is correctly exported as part of the public API surface in
index.tsxalongside other similar constants (TSS_FORMDATA_CONTEXT,X_TSS_RAW_RESPONSE, etc.). While not used internally within the package, this is the expected pattern for public API constants defined in a barrel export. No action needed.
| invariant(contentType, 'expected content-type header to be set') | ||
| const serializedByStart = !!response.headers.get(X_TSS_SERIALIZED) | ||
| // If the response is not ok, throw an error | ||
| if (!response.ok) { | ||
| if (serializedByStart && contentType.includes('application/json')) { | ||
| const jsonPayload = await response.json() | ||
| const result = fromCrossJSON(jsonPayload, { plugins: serovalPlugins! }) | ||
| throw result | ||
| } | ||
|
|
||
| throw new Error(await response.text()) | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle no-content responses; don’t assert content-type.
Server may return undefined/204 with no content-type (e.g., serializeResult(undefined)). Current invariant throws. Treat “no content” as undefined when ok.
- invariant(contentType, 'expected content-type header to be set')
+ const contentType = response.headers.get('content-type')
+ if (!contentType) {
+ if (response.ok && (response.status === 204 || !response.body)) {
+ return undefined
+ }
+ if (!response.ok) {
+ throw new Error(await response.text())
+ }
+ return response
+ }Also applies to: 288-294
🤖 Prompt for AI Agents
In packages/start-client-core/src/client-rpc/serverFnFetcher.ts around lines
189-191 (and similarly at 288-294), the code currently invariants that a
content-type header exists which breaks for no-content responses (e.g., 204 or
serializeResult(undefined)); change the logic to allow missing content-type for
empty responses: first detect no-content (response.status === 204 or
content-length === '0' or response.headers.get('content-type') === null and
response.bodyUsed is false/response has no body), treat those cases as undefined
result instead of asserting, and only assert/parse content-type when a body is
actually present; preserve the existing check for the X_TSS_SERIALIZED header
when present and ensure parsing flows handle both serialized and
plain/no-content responses.
| invariant(result, 'expected result to be resolved') | ||
| if (result instanceof Error) { | ||
| throw result | ||
| } | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
Bug: falsy results (0, '', false) are incorrectly rejected.
invariant(result, ...) rejects valid falsy values; check only undefined.
- invariant(result, 'expected result to be resolved')
+ invariant(typeof result !== 'undefined', 'expected result to be resolved')📝 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.
| invariant(result, 'expected result to be resolved') | |
| if (result instanceof Error) { | |
| throw result | |
| } | |
| return result | |
| } | |
| invariant(typeof result !== 'undefined', 'expected result to be resolved') | |
| if (result instanceof Error) { | |
| throw result | |
| } | |
| return result | |
| } |
🤖 Prompt for AI Agents
In packages/start-client-core/src/client-rpc/serverFnFetcher.ts around lines 215
to 221, the code uses invariant(result, 'expected result to be resolved') which
treats valid falsy values (0, '', false) as errors; change the check to only
reject undefined (e.g., replace the invariant call with a check like if (result
=== undefined) { throw new Error('expected result to be resolved') } or use
invariant(result !== undefined, ...)) so that falsy but valid results are
preserved, keep the existing Error instance handling (if (result instanceof
Error) throw result) and then return result.
| ValidateSerializableInput, | ||
| Validator, | ||
| } from '@tanstack/router-core' | ||
| import type { TSS_SERVER_FUNCTION } from './constants' |
There was a problem hiding this comment.
Invalid type-only import for value symbol.
TSS_SERVER_FUNCTION is a value, not a type. Using it as a computed key requires a value import; type-only import breaks under strict TS/isolatedModules.
-import type { TSS_SERVER_FUNCTION } from './constants'
+import { TSS_SERVER_FUNCTION } from './constants'If you want a type-only reference, declare/export it as declare const TSS_SERVER_FUNCTION: unique symbol in constants and keep a value export to use as a key. As per coding guidelines.
🤖 Prompt for AI Agents
In packages/start-client-core/src/createServerFn.ts around line 18, the import
uses a type-only import for TSS_SERVER_FUNCTION but that identifier is a runtime
value used as a computed key; change the import to a value import (remove the
leading "type") so the symbol exists at runtime. If constants currently only
export a type, update packages/start-client-core/src/constants to export a
runtime symbol (e.g., export const TSS_SERVER_FUNCTION = Symbol(...) or export
declare const TSS_SERVER_FUNCTION: unique symbol plus a value export) so callers
can import the value; ensure callers use the value import for computed property
access.
| console.log('response', response) | ||
|
|
There was a problem hiding this comment.
Remove noisy response logging (potential PII leakage).
console.log(response) can log bodies/headers in prod. Guard by NODE_ENV or delete.
- console.log('response', response)
+ if (process.env.NODE_ENV === 'development') {
+ // console.debug('response', response)
+ }📝 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.
| console.log('response', response) | |
| if (process.env.NODE_ENV === 'development') { | |
| // console.debug('response', response) | |
| } | |
🤖 Prompt for AI Agents
In packages/start-server-core/src/createStartHandler.ts around lines 277-278,
the console.log('response', response) is noisy and may leak PII; remove this raw
response logging or guard it so it only runs in non-production (e.g. check
NODE_ENV !== 'production') and/or replace with a structured logger at a debug
level that only emits safe fields (status/code, timing) rather than full
headers/bodies.
| console.log( | ||
| { | ||
| isServerFn, | ||
| isCreateServerFn, | ||
| isFormData, | ||
| isCtxResult, | ||
| }, | ||
| res, | ||
| ) |
There was a problem hiding this comment.
Remove stray debug logging.
Avoid logging payloads/results in prod.
- console.log(
- {
- isServerFn,
- isCreateServerFn,
- isFormData,
- isCtxResult,
- },
- res,
- )
+ // Debug block removed📝 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.
| console.log( | |
| { | |
| isServerFn, | |
| isCreateServerFn, | |
| isFormData, | |
| isCtxResult, | |
| }, | |
| res, | |
| ) | |
| // Debug block removed |
🤖 Prompt for AI Agents
In packages/start-server-core/src/server-functions-handler.ts around lines 145
to 153, remove the stray console.log that prints full payloads/results; replace
it either by deleting the log entirely or by using a non-production gated logger
(e.g. wrap with NODE_ENV !== 'production' or use a debug/logger library) so
sensitive data is not printed in production. Ensure no raw payload or result is
logged in the production path.
| if ( | ||
| isPlainObject(result) && | ||
| ('result' in result || 'error' in result) | ||
| ) { | ||
| console.log('tanner') | ||
| return result.result || result.error | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Fix unwrap logic; truthiness breaks falsy results.
result.result || result.error misroutes 0/''/false to error. Prefer explicit presence checks.
- function unwrapResultOrError(result: any) {
- if (
- isPlainObject(result) &&
- ('result' in result || 'error' in result)
- ) {
- console.log('tanner')
- return result.result || result.error
- }
- return result
- }
+ function unwrapResultOrError(result: any) {
+ if (isPlainObject(result) && ('result' in result || 'error' in result)) {
+ return 'error' in result ? result.error : result.result
+ }
+ return result
+ }🤖 Prompt for AI Agents
In packages/start-server-core/src/server-functions-handler.ts around lines 156
to 164, the unwrap logic uses truthiness (result.result || result.error) which
incorrectly treats valid falsy values (0, '', false) as absent; replace this
with explicit presence checks: if the plain object hasOwnProperty('result')
return result.result; else if it hasOwnProperty('error') return result.error;
also remove the stray console.log and ensure you check property presence rather
than truthiness to preserve falsy payloads.
Summary by CodeRabbit
New Features
Bug Fixes
Chores