response reconciliation#7588
Open
schiller-manuel wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
documentation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (86)
✅ Files skipped from review due to trivial changes (15)
🚧 Files skipped from review as they are similar to previous changes (63)
📝 WalkthroughWalkthroughUpdates Start runtime request/response reconciliation, server-function framing/error handling, server-entry error recovery, documentation, and adds two React E2E workspaces with Playwright tests covering response reconciliation and session handling. ChangesStart runtime and E2E validation
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ServerEntry
participant StartHandler
participant Middleware
participant ServerFn
Browser->>ServerEntry: HTTP request (headers/cookies)
ServerEntry->>StartHandler: fetch(request, opts)
StartHandler->>Middleware: run request middleware (before/after)
Middleware-->>StartHandler: mutate StartEvent (status/headers/cookies)
StartHandler->>ServerFn: invoke server-function (if applicable)
ServerFn-->>StartHandler: serialized result or framed stream / error
StartHandler->>StartHandler: reconcile helper-driven state with returned Response
StartHandler-->>ServerEntry: final Response (status, headers, Set-Cookie, body)
ServerEntry-->>Browser: send response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
|
View your CI Pipeline Execution ↗ for commit 97e14ca
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗. ☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
e2e/react-start/session-handling/src/session.ts (3)
23-31: ⚡ Quick winReplace
Record<string, any>withRecord<string, unknown>for strict type safety.Line 24 uses
Record<string, any>which violates the TypeScript strict mode guideline.♻️ Suggested fix
export async function readSession(overrides: Record<string, unknown> = {}) { - const session = await useSession<Record<string, any>>( + const session = await useSession<Record<string, unknown>>( sessionConfig(overrides), )🤖 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 `@e2e/react-start/session-handling/src/session.ts` around lines 23 - 31, In readSession, replace the loose generic Record<string, any> passed to useSession with Record<string, unknown> to conform to strict TypeScript; update the useSession<Record<string, unknown>>(...) type argument and ensure any downstream accesses to session.data or session.id are typed/guarded appropriately (function names: readSession and useSession, identifier: session.data/session.id).Source: Coding guidelines
15-21: 💤 Low valueConsider logging or rethrowing JSON parsing errors.
Line 18 silently catches all errors and returns an empty object. While this provides resilience for E2E tests, it could mask malformed JSON or other issues during debugging.
💡 Optional improvement
export async function readJson(request: Request) { try { return (await request.json()) as Record<string, unknown> } catch { + console.warn('Failed to parse request JSON, returning empty object') return {} } }🤖 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 `@e2e/react-start/session-handling/src/session.ts` around lines 15 - 21, The readJson function currently swallows all JSON parse errors and returns {} silently; modify readJson to catch the error as a variable (e.g., catch (err)) and either log the error details (using console.error or the project logger) before returning {} or rethrow the error so callers can handle it; optionally add a boolean parameter like suppressErrors (default true) to preserve current behavior in tests while allowing callers to surface errors when needed.
33-45: ⚡ Quick winReplace
Record<string, any>withRecord<string, unknown>for strict type safety.Line 37 uses
Record<string, any>which violates the TypeScript strict mode guideline.♻️ Suggested fix
export async function updateSessionData( update: Record<string, unknown>, overrides: Record<string, unknown> = {}, ) { - const session = await useSession<Record<string, any>>( + const session = await useSession<Record<string, unknown>>( sessionConfig(overrides), )🤖 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 `@e2e/react-start/session-handling/src/session.ts` around lines 33 - 45, The function updateSessionData uses useSession with an explicit type Record<string, any> which breaks strict TypeScript rules; change that generic to Record<string, unknown> so the call becomes useSession<Record<string, unknown>>(sessionConfig(overrides)), ensure any local references to session.data or returned types are compatible with unknown (e.g., keep them typed as unknown instead of any) and run type checks to fix any resulting type errors in updateSessionData, useSession, or sessionConfig.Source: Coding guidelines
e2e/react-start/session-handling/src/routes/server-functions.tsx (3)
16-22: ⚡ Quick winReplace
Record<string, any>withRecord<string, unknown>for strict type safety.Line 17 uses
Record<string, any>which violates the TypeScript strict mode guideline.♻️ Suggested fix
const readSessionFn = createServerFn().handler(async () => { - const session = await useSession<Record<string, any>>(sessionConfig()) + const session = await useSession<Record<string, unknown>>(sessionConfig()) return {🤖 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 `@e2e/react-start/session-handling/src/routes/server-functions.tsx` around lines 16 - 22, In readSessionFn replace the loose generic Record<string, any> passed to useSession with Record<string, unknown> to satisfy strict TypeScript rules; update any downstream usages in the handler that assume mutable or specific typed values from session.data to use type-safe access or narrow/cast as needed (refer to readSessionFn, useSession, and sessionConfig to locate the call).Source: Coding guidelines
7-14: ⚡ Quick winReplace
Record<string, any>withRecord<string, unknown>for strict type safety.Line 8 uses
Record<string, any>which violates the TypeScript strict mode guideline. Consider usingunknowninstead ofanyto maintain type safety while preserving flexibility in this E2E test.♻️ Suggested fix
const updateSessionFn = createServerFn().handler(async () => { - const session = await useSession<Record<string, any>>(sessionConfig()) + const session = await useSession<Record<string, unknown>>(sessionConfig()) await session.update({ serverFn: 'updated' })🤖 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 `@e2e/react-start/session-handling/src/routes/server-functions.tsx` around lines 7 - 14, Change the session generic from Record<string, any> to Record<string, unknown> in the updateSessionFn handler: locate the createServerFn().handler block where useSession<Record<string, any>>(sessionConfig()) is called and update the generic to use Record<string, unknown> (affecting updateSessionFn and its use of useSession/sessionConfig) to enforce strict TypeScript safety without losing flexibility.Source: Coding guidelines
41-70: ⚡ Quick winReplace
anytypes with stricter alternatives for type safety.Line 46 uses
anyfor function parameters and return type, which violates the TypeScript strict mode guideline.♻️ Suggested fix
function ServerFunctionButton({ name, fn, }: { name: string - fn: (...args: Array<any>) => Promise<any> + fn: () => Promise<{ id: string; data: Record<string, unknown> }> }) {🤖 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 `@e2e/react-start/session-handling/src/routes/server-functions.tsx` around lines 41 - 70, The parameter types using any are too loose; change the fn prop type from (...args: Array<any>) => Promise<any> to (...args: unknown[]) => Promise<unknown> (or an appropriate specific generic) and update the useServerFn invocation to pass/declare that same generic so TypeScript infers safer types for serverFn (e.g., useServerFn<typeof fn> or useServerFn<(...args: unknown[]) => Promise<unknown>>(fn)); also update any local uses/annotations that relied on any (for example ensure result state remains a string but document/convert serverFn() output via JSON.stringify) to avoid Array<any> and any return types.Source: Coding guidelines
e2e/react-start/session-handling/src/routes/ssr.tsx (1)
6-14: ⚡ Quick winReplace
Record<string, any>withRecord<string, unknown>for strict type safety.Line 7 uses
Record<string, any>which violates the TypeScript strict mode guideline.♻️ Suggested fix
const loadSession = createServerFn().handler(async () => { - const session = await useSession<Record<string, any>>(sessionConfig()) + const session = await useSession<Record<string, unknown>>(sessionConfig()) const count = Number(session.data.ssrCount ?? 0) + 1🤖 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 `@e2e/react-start/session-handling/src/routes/ssr.tsx` around lines 6 - 14, The handler type for loadSession uses Record<string, any>; update it to use Record<string, unknown> to satisfy strict TypeScript rules: locate the createServerFn().handler callback where useSession<Record<string, any>>(sessionConfig()) is called and change the generic to useSession<Record<string, unknown>>(sessionConfig()), keeping the rest of the logic (session.id, session.data, session.update) intact.Source: Coding guidelines
e2e/react-start/session-handling/src/start.ts (1)
5-26: ⚡ Quick winReplace
Record<string, any>withRecord<string, unknown>for strict type safety.Lines 10 and 18 use
Record<string, any>which violates the TypeScript strict mode guideline.♻️ Suggested fix
if (scenario === 'before') { - await updateSession<Record<string, any>>(sessionConfig(), { + await updateSession<Record<string, unknown>>(sessionConfig(), { middleware: 'before', }) return next() } if (scenario === 'after') { const result = await next() - await updateSession<Record<string, any>>(sessionConfig(), { + await updateSession<Record<string, unknown>>(sessionConfig(), { middleware: 'after', })🤖 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 `@e2e/react-start/session-handling/src/start.ts` around lines 5 - 26, The two updateSession calls inside the sessionMiddleware createMiddleware().server async handler (the branches for scenario === 'before' and scenario === 'after') use Record<string, any>; replace both type arguments with Record<string, unknown> to comply with strict TypeScript typing (i.e., updateSession<Record<string, unknown>>(sessionConfig(), { middleware: 'before' }) and similarly for the 'after' branch).Source: Coding guidelines
🤖 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.
Inline comments:
In `@e2e/react-start/response-reconciliation/.gitignore`:
- Around line 14-15: The .gitignore entry "/public/build# Sentry Config File" is
malformed so "/public/build" won't be ignored; split the line into a pure
pattern and a comment: replace the single malformed line with a standalone
pattern "/public/build" and a separate comment line "# Sentry Config File"
(i.e., update the entry in response-reconciliation/.gitignore that currently
reads "/public/build# Sentry Config File").
In `@e2e/react-start/response-reconciliation/package.json`:
- Line 28: Update the internal dependency declaration for
"`@tanstack/router-e2e-utils`" in package.json to use the workspace protocol
exactly as "workspace:*" instead of "workspace:^"; locate the dependency entry
for "`@tanstack/router-e2e-utils`" and replace its version specifier "workspace:^"
with "workspace:*", then reinstall/update the lockfile to ensure consistent
dependency resolution.
In `@e2e/react-start/response-reconciliation/src/routes/server-functions.tsx`:
- Around line 46-55: The replacementMiddleware is using an `as any` escape to
return a Response—remove the `as any` and either cast through unknown to the
declared server middleware result type or update
FunctionMiddlewareServerFnResult to include Response so the returned new
Response is representable; specifically modify the return cast in the anonymous
function created by createMiddleware({ type: 'function' }).server(...) (the
block that checks resultWithResponse.result instanceof Response and returns new
Response) to use an `unknown`-based cast or extend
FunctionMiddlewareServerFnResult to accept Response. Also change
ServerFunctionButton’s `fn` prop type from Array<any>/Promise<any> to a safer
signature like `(...args: Array<unknown>) => Promise<unknown>` so callers retain
the instanceof Response check without `any`.
In `@e2e/react-start/session-handling/package.json`:
- Line 28: The dependency entry for `@tanstack/router-e2e-utils` uses the wrong
workspace protocol ("workspace:^"); update the package.json dependency for
"`@tanstack/router-e2e-utils`" to use the workspace protocol exactly as
"workspace:*" so it follows the repo's internal dependency rule; locate the
dependency in e2e/react-start/session-handling/package.json and replace the
version specifier accordingly.
In `@e2e/react-start/session-handling/src/routes/api/session-seal.ts`:
- Line 10: The session is currently typed as Record<string, any> which disables
strict typing; define a concrete session type (e.g., interface SessionData { /*
fields used below */ }) or use unknown with runtime type checks, then replace
useSession<Record<string, any>>(config) with useSession<SessionData>(config) (or
useSession<unknown>(config) plus narrow at runtime) and update any code that
accesses session properties to conform to the new type (adjust property names on
the SessionData interface to match existing usage).
---
Nitpick comments:
In `@e2e/react-start/session-handling/src/routes/server-functions.tsx`:
- Around line 16-22: In readSessionFn replace the loose generic Record<string,
any> passed to useSession with Record<string, unknown> to satisfy strict
TypeScript rules; update any downstream usages in the handler that assume
mutable or specific typed values from session.data to use type-safe access or
narrow/cast as needed (refer to readSessionFn, useSession, and sessionConfig to
locate the call).
- Around line 7-14: Change the session generic from Record<string, any> to
Record<string, unknown> in the updateSessionFn handler: locate the
createServerFn().handler block where useSession<Record<string,
any>>(sessionConfig()) is called and update the generic to use Record<string,
unknown> (affecting updateSessionFn and its use of useSession/sessionConfig) to
enforce strict TypeScript safety without losing flexibility.
- Around line 41-70: The parameter types using any are too loose; change the fn
prop type from (...args: Array<any>) => Promise<any> to (...args: unknown[]) =>
Promise<unknown> (or an appropriate specific generic) and update the useServerFn
invocation to pass/declare that same generic so TypeScript infers safer types
for serverFn (e.g., useServerFn<typeof fn> or useServerFn<(...args: unknown[])
=> Promise<unknown>>(fn)); also update any local uses/annotations that relied on
any (for example ensure result state remains a string but document/convert
serverFn() output via JSON.stringify) to avoid Array<any> and any return types.
In `@e2e/react-start/session-handling/src/routes/ssr.tsx`:
- Around line 6-14: The handler type for loadSession uses Record<string, any>;
update it to use Record<string, unknown> to satisfy strict TypeScript rules:
locate the createServerFn().handler callback where useSession<Record<string,
any>>(sessionConfig()) is called and change the generic to
useSession<Record<string, unknown>>(sessionConfig()), keeping the rest of the
logic (session.id, session.data, session.update) intact.
In `@e2e/react-start/session-handling/src/session.ts`:
- Around line 23-31: In readSession, replace the loose generic Record<string,
any> passed to useSession with Record<string, unknown> to conform to strict
TypeScript; update the useSession<Record<string, unknown>>(...) type argument
and ensure any downstream accesses to session.data or session.id are
typed/guarded appropriately (function names: readSession and useSession,
identifier: session.data/session.id).
- Around line 15-21: The readJson function currently swallows all JSON parse
errors and returns {} silently; modify readJson to catch the error as a variable
(e.g., catch (err)) and either log the error details (using console.error or the
project logger) before returning {} or rethrow the error so callers can handle
it; optionally add a boolean parameter like suppressErrors (default true) to
preserve current behavior in tests while allowing callers to surface errors when
needed.
- Around line 33-45: The function updateSessionData uses useSession with an
explicit type Record<string, any> which breaks strict TypeScript rules; change
that generic to Record<string, unknown> so the call becomes
useSession<Record<string, unknown>>(sessionConfig(overrides)), ensure any local
references to session.data or returned types are compatible with unknown (e.g.,
keep them typed as unknown instead of any) and run type checks to fix any
resulting type errors in updateSessionData, useSession, or sessionConfig.
In `@e2e/react-start/session-handling/src/start.ts`:
- Around line 5-26: The two updateSession calls inside the sessionMiddleware
createMiddleware().server async handler (the branches for scenario === 'before'
and scenario === 'after') use Record<string, any>; replace both type arguments
with Record<string, unknown> to comply with strict TypeScript typing (i.e.,
updateSession<Record<string, unknown>>(sessionConfig(), { middleware: 'before'
}) and similarly for the 'after' branch).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23200d41-9026-4b02-bf02-6895e9cb3185
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (86)
.changeset/start-response-state.mddocs/start/framework/react/guide/server-entry-point.mde2e/react-start/response-reconciliation/.gitignoree2e/react-start/response-reconciliation/.prettierignoree2e/react-start/response-reconciliation/package.jsone2e/react-start/response-reconciliation/playwright.config.tse2e/react-start/response-reconciliation/rsbuild.config.tse2e/react-start/response-reconciliation/server.jse2e/react-start/response-reconciliation/src/components/DefaultCatchBoundary.tsxe2e/react-start/response-reconciliation/src/components/NotFound.tsxe2e/react-start/response-reconciliation/src/routeTree.gen.tse2e/react-start/response-reconciliation/src/router.tsxe2e/react-start/response-reconciliation/src/routes/__root.tsxe2e/react-start/response-reconciliation/src/routes/api/-response.tse2e/react-start/response-reconciliation/src/routes/api/base.tse2e/react-start/response-reconciliation/src/routes/api/bulk-headers.tse2e/react-start/response-reconciliation/src/routes/api/clear-returned-headers.tse2e/react-start/response-reconciliation/src/routes/api/direct-mutation-visible.tse2e/react-start/response-reconciliation/src/routes/api/explicit-set-cookie-header.tse2e/react-start/response-reconciliation/src/routes/api/get-response-header-helper.tse2e/react-start/response-reconciliation/src/routes/api/get-response-headers-helper.tse2e/react-start/response-reconciliation/src/routes/api/multiple-cookies.tse2e/react-start/response-reconciliation/src/routes/api/null-body-status.tse2e/react-start/response-reconciliation/src/routes/api/readonly-after-next.tse2e/react-start/response-reconciliation/src/routes/api/redirect-with-cookies.tse2e/react-start/response-reconciliation/src/routes/api/remove-returned-header.tse2e/react-start/response-reconciliation/src/routes/api/replace-after-direct-mutation.tse2e/react-start/response-reconciliation/src/routes/api/replace-explicit-set-cookie.tse2e/react-start/response-reconciliation/src/routes/api/route-after-next.tse2e/react-start/response-reconciliation/src/routes/api/route-before-next.tse2e/react-start/response-reconciliation/src/routes/api/same-boundary-conflict.tse2e/react-start/response-reconciliation/src/routes/api/throw-after-status.tse2e/react-start/response-reconciliation/src/routes/api/two-returned-responses.tse2e/react-start/response-reconciliation/src/routes/index.tsxe2e/react-start/response-reconciliation/src/routes/server-functions.tsxe2e/react-start/response-reconciliation/src/routes/ssr.tsxe2e/react-start/response-reconciliation/src/server-entry.tse2e/react-start/response-reconciliation/src/start.tse2e/react-start/response-reconciliation/src/styles/app.csse2e/react-start/response-reconciliation/tests/response-reconciliation.spec.tse2e/react-start/response-reconciliation/tsconfig.jsone2e/react-start/response-reconciliation/vite.config.tse2e/react-start/session-handling/.gitignoree2e/react-start/session-handling/.prettierignoree2e/react-start/session-handling/package.jsone2e/react-start/session-handling/playwright.config.tse2e/react-start/session-handling/rsbuild.config.tse2e/react-start/session-handling/server.jse2e/react-start/session-handling/src/components/DefaultCatchBoundary.tsxe2e/react-start/session-handling/src/components/NotFound.tsxe2e/react-start/session-handling/src/routeTree.gen.tse2e/react-start/session-handling/src/router.tsxe2e/react-start/session-handling/src/routes/__root.tsxe2e/react-start/session-handling/src/routes/api/session-clear.tse2e/react-start/session-handling/src/routes/api/session-cookie-disabled.tse2e/react-start/session-handling/src/routes/api/session-header.tse2e/react-start/session-handling/src/routes/api/session-helper-cookie.tse2e/react-start/session-handling/src/routes/api/session-middleware.tse2e/react-start/session-handling/src/routes/api/session-named.tse2e/react-start/session-handling/src/routes/api/session-seal.tse2e/react-start/session-handling/src/routes/api/session.tse2e/react-start/session-handling/src/routes/index.tsxe2e/react-start/session-handling/src/routes/server-functions.tsxe2e/react-start/session-handling/src/routes/ssr.tsxe2e/react-start/session-handling/src/session.tse2e/react-start/session-handling/src/start.tse2e/react-start/session-handling/src/styles/app.csse2e/react-start/session-handling/tests/session-handling.spec.tse2e/react-start/session-handling/tsconfig.jsone2e/react-start/session-handling/vite.config.tspackages/react-start/src/default-entry/server.tspackages/solid-start/src/default-entry/server.tspackages/start-plugin-core/src/vite/plugin.tspackages/start-plugin-core/tests/entry-planning.test.tspackages/start-server-core/docs/RECONCILIATION.mdpackages/start-server-core/package.jsonpackages/start-server-core/skills/start-server-core/SKILL.mdpackages/start-server-core/src/createStartHandler.tspackages/start-server-core/src/headers.tspackages/start-server-core/src/internal-request-response.tspackages/start-server-core/src/request-response.tspackages/start-server-core/src/server-functions-handler.tspackages/start-server-core/tests/createStartHandler.test.tspackages/start-server-core/tests/server-functions-handler.test.tspackages/start-server-core/vite.config.tspackages/vue-start/src/default-entry/server.ts
| "@playwright/test": "^1.57.0", | ||
| "@rsbuild/core": "^2.0.8", | ||
| "@rsbuild/plugin-react": "^2.0.0", | ||
| "@tanstack/router-e2e-utils": "workspace:^", |
Contributor
There was a problem hiding this comment.
Use workspace:* for internal dependency on Line 28.
@tanstack/router-e2e-utils is currently workspace:^; this conflicts with the repository rule for internal packages.
As per coding guidelines, "**/package.json: Use workspace protocol (workspace:*) for internal dependencies".
🤖 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 `@e2e/react-start/response-reconciliation/package.json` at line 28, Update the
internal dependency declaration for "`@tanstack/router-e2e-utils`" in package.json
to use the workspace protocol exactly as "workspace:*" instead of "workspace:^";
locate the dependency entry for "`@tanstack/router-e2e-utils`" and replace its
version specifier "workspace:^" with "workspace:*", then reinstall/update the
lockfile to ensure consistent dependency resolution.
Source: Coding guidelines
Collaborator
Author
There was a problem hiding this comment.
Keeping workspace:^ — 37 of 40 e2e apps use it for @tanstack/router-e2e-utils; matching the dominant local convention.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
schiller-manuel
force-pushed
the
response-reconciliation
branch
from
fe11405 to
97e14ca
Compare
Contributor
🚀 Changeset Version Preview8 package(s) bumped directly, 8 bumped as dependents. 🟩 Patch bumps
|
Contributor
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. |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We updated requestHandler in internal-request-response.ts to validate the URL pathname with decodeURIComponent after URL parsing, which restores the 400 Bad Request behaviour for malformed UTF-8 percent-encoded sequences (e.g. %E0%A4, %80, %FF) that was previously provided by H3Event's constructor. Without this fix, the WHATWG URL parser silently accepts those byte sequences and the request falls through to the router, resulting in a 404 instead of the expected 400.
Tip
✅ We verified this fix by re-running @tanstack/vue-router:test:unit, tanstack-react-start-e2e-basic:test:e2e--vite-prerender.
diff --git a/packages/start-server-core/src/internal-request-response.ts b/packages/start-server-core/src/internal-request-response.ts
index ae8f1a6d..fc615534 100644
--- a/packages/start-server-core/src/internal-request-response.ts
+++ b/packages/start-server-core/src/internal-request-response.ts
@@ -674,6 +674,18 @@ export function requestHandler<TRegister = unknown>(
throw error
}
+ try {
+ decodeURIComponent(requestUrl.pathname)
+ } catch (error) {
+ if (error instanceof URIError) {
+ return new Response(null, {
+ status: 400,
+ statusText: 'Bad Request',
+ })
+ }
+ throw error
+ }
+
const response = createStartResponse()
const responseMeta = createResponseMeta()
trackStartResponse(response, responseMeta)
Or Apply changes locally with:
npx nx-cloud apply-locally bI6l-OhXi
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
Sheraff
reviewed
Jun 9, 2026
Comment on lines
+22
to
+32
| export function applyHeaders(source: Headers, target: Headers): void { | ||
| for (const [name, value] of source) { | ||
| if (name !== 'set-cookie') { | ||
| target.set(name, value) | ||
| } | ||
| } | ||
| for (const cookie of getSetCookieValues(source)) { | ||
| target.append('set-cookie', cookie) | ||
| } | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
In theory there are other headers than set-cookie that you are allowed to set multiple times (for example on a Response that would be Link, Vary, ...). I'm not sure this method is correctly handling that.
Collaborator
Author
There was a problem hiding this comment.
Multi-value headers are actually handled losslessly here: iterating a Headers object yields repeated values as one comma-combined string ([...h] for two Link values gives '<a>, <b>'), and per RFC 9110 §5.3 that combined form is semantically equivalent — so set(name, combinedValue) preserves them. Set-Cookie is the one header where comma-joining is unsafe (Expires contains commas), which is why it's special-cased via getSetCookie(). Added a comment in code explaining this.
Sheraff
reviewed
Jun 9, 2026
| let sanitized = '' | ||
| for (const char of statusMessage) { | ||
| const code = char.charCodeAt(0) | ||
| if (code === 0x09 || (code >= 0x20 && code <= 0x7e)) { |
Collaborator
There was a problem hiding this comment.
more of a nitpick for my puny human mind: it's nice to leave comments on char code values to make it easier to know which char they are referring to.
Collaborator
Author
There was a problem hiding this comment.
Done — added char comments (tab / printable ASCII space–'~').
Sheraff
reviewed
Jun 9, 2026
| return fallbackStatusCode | ||
| } | ||
| const code = typeof statusCode === 'string' ? Number(statusCode) : statusCode | ||
| if (!Number.isInteger(code) || code < 200 || code > 599) { |
Collaborator
There was a problem hiding this comment.
idk if it matters here, but some services send some quite exotic status codes (https://en.wikipedia.org/wiki/List_of_HTTP_status_codes)
- shopify => 783 Unexpected Token
- linkedin => 999 Request denied
maybe users of Start also want to do weird things like this?
Collaborator
Author
There was a problem hiding this comment.
Good question, but those exotic codes structurally can't flow through Start: the entire server pipeline (server routes, SSR documents, and server fns alike) uses Fetch Response objects as its currency, and the constructor throws outside 200-599:
new Response(null, { status: 999 })
// RangeError: init["status"] must be in the range of 200 to 599, inclusive.So LinkedIn's 999 can't even be returned from a handler, let alone reconciled. The clamp just mirrors the platform constraint (and old h3-based behavior, which silently fell back too). New in this PR: setResponseStatus now warns in development when a value gets ignored, so this no longer fails silently. Also added a comment above sanitizeStatusCode documenting the constraint.
Sheraff
reviewed
Jun 9, 2026
| protectedHeaders?: ProtectedHeaders, | ||
| ): void { | ||
| if (meta.clearHeaders) { | ||
| for (const name of Array.from(target.keys())) { |
Collaborator
There was a problem hiding this comment.
silly AI doesn't know javascript
Suggested change
| for (const name of Array.from(target.keys())) { | |
| for (const name of target.keys()) { |
Collaborator
Author
There was a problem hiding this comment.
Careful — this one is load-bearing. Headers iteration is live; deleting while iterating skips entries:
const h = new Headers()
h.set('a','1'); h.set('b','2'); h.set('c','3'); h.set('d','4')
for (const name of h.keys()) h.delete(name)
console.log([...h.keys()]) // [ 'b', 'd' ] — half survive!The Array.from snapshot is required. Added a comment in code so this doesn't get "simplified" later.
Sheraff
reviewed
Jun 9, 2026
| const headers = getRequestHeaders() | ||
| if (opts?.xForwardedHost) { | ||
| const forwardedHost = headers.get('x-forwarded-host') | ||
| const host = forwardedHost?.split(',')[0]?.trim() |
Collaborator
There was a problem hiding this comment.
Nit: if you know you only need the 1st segment, limit .split() to that segment
Suggested change
| const host = forwardedHost?.split(',')[0]?.trim() | |
| const host = forwardedHost?.split(',', 1)[0]?.trim() |
Collaborator
Author
There was a problem hiding this comment.
Applied — also to the same pattern in getRequestIP and getRequestProtocol.
Contributor
|
Thanks for accomodating my request 🙏🙏 |
Contributor
I am curious, is it possible to customize the error response, but still using Start's reconciliation behavior? |
Collaborator
Author
|
@hokkyss can you explain what you want to do exactly? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #5107
fixes #5464
fixes #5407
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests