throw 405 response instead of 500 internal error on method mismatch #6091
Conversation
WalkthroughAdds a 405-response helper to server function request handling and replaces a thrown error with that helper; adds two new example routes (/method-not-allowed/get and /method-not-allowed/post) with client UIs and tests; registers their server-function IDs in Vite and updates the generated route tree/types. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser UI)
participant Route as File Route (React)
participant ServerFn as Server Function Endpoint
participant Handler as server-functions-handler
Browser->>Route: navigate to /method-not-allowed/(get|post)
Route->>Browser: render component with method buttons
Browser->>ServerFn: fetch /_serverFn/<id>?createServerFn using chosen HTTP method
ServerFn->>Handler: handler invoked, validate HTTP method
alt method allowed
Handler->>ServerFn: invoke function handler -> Response 200 "Hello, World!"
ServerFn->>Browser: 200 + body
else method not allowed
Handler->>ServerFn: return Response 405 with Allow header
ServerFn->>Browser: 405 + error text
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 3m 8s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 2s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-12-13 19:55:59 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/start-server-core/src/server-functions-handler.ts (1)
15-22: Consider listing all supported methods in the Allow header.The RFC 9110 specification states that the Allow header in a 405 response should list all methods currently supported by the endpoint. This endpoint supports both GET (line 116) and POST (line 130+), so the Allow header should ideally be
"GET, POST"rather than just the expected method for the current code path.Apply this diff to list all supported methods:
-const methodNotAllowed = (expectedMethod: string, actualMethod: string) => { +const methodNotAllowed = (actualMethod: string) => { - throw new Response(`expected ${expectedMethod} method. Got ${actualMethod}`, { + throw new Response(`Method ${actualMethod} is not allowed. Use GET or POST`, { status: 405, headers: { - Allow: expectedMethod, + Allow: 'GET, POST', }, }) }And update the call site at line 131:
- throw methodNotAllowed('POST', method) + throw methodNotAllowed(method)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/start-server-core/src/server-functions-handler.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/start-server-core/src/server-functions-handler.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/start-server-core/src/server-functions-handler.ts
⏰ 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
🔇 Additional comments (1)
packages/start-server-core/src/server-functions-handler.ts (1)
130-132: Excellent improvement—proper HTTP semantics for method validation.This change correctly replaces a generic error (which resulted in a 500 Internal Server Error) with a standards-compliant 405 Method Not Allowed response. This aligns with the PR objective to reduce log noise and provide more accurate HTTP status codes for method mismatches.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
e2e/react-start/server-functions/src/routes/method-not-allowed/post.tsx (1)
15-20: Same note: narrow the method type and consider avoiding hard-codedconstant_id_3.-const fetchFn = async (method: string) => { +type TestMethod = 'GET' | 'POST' | 'PUT' | 'OPTIONS' +const fetchFn = async (method: TestMethod) => { const response = await fetch('/_serverFn/constant_id_3?createServerFn', { method, }) return [response.status, await response.text()] as const }e2e/react-start/server-functions/src/routes/method-not-allowed/get.tsx (1)
13-18: UseTestMethodtype and referencegetableServerFn.urlinstead of hard-coding the ID.
Narrowing the method type aligns with TS strict mode. The hard-codedconstant_id_2is brittle when the server function ID can be accessed directly:getableServerFn.urlis available on the server function object and is used elsewhere in the codebase (e.g.,e2e/react-start/custom-basepath/src/routes/logout.tsx).-const fetchFn = async (method: string) => { - const response = await fetch('/_serverFn/constant_id_2?createServerFn', { +type TestMethod = 'GET' | 'POST' | 'PUT' | 'OPTIONS' +const fetchFn = async (method: TestMethod) => { + const response = await fetch(`${getableServerFn.url}?createServerFn`, { method, }) return [response.status, await response.text()] as const }
📜 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/method-not-allowed/get.tsx(1 hunks)e2e/react-start/server-functions/src/routes/method-not-allowed/post.tsx(1 hunks)e2e/react-start/server-functions/tests/server-functions.spec.ts(1 hunks)e2e/react-start/server-functions/vite.config.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/routes/method-not-allowed/get.tsxe2e/react-start/server-functions/src/routes/method-not-allowed/post.tsxe2e/react-start/server-functions/vite.config.tse2e/react-start/server-functions/src/routeTree.gen.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/routes/method-not-allowed/get.tsxe2e/react-start/server-functions/src/routes/method-not-allowed/post.tsxe2e/react-start/server-functions/vite.config.tse2e/react-start/server-functions/src/routeTree.gen.ts
🧠 Learnings (2)
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
e2e/react-start/server-functions/src/routeTree.gen.ts
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-start/server-functions/src/routeTree.gen.ts
🧬 Code graph analysis (3)
e2e/react-start/server-functions/src/routes/method-not-allowed/get.tsx (1)
e2e/react-start/server-functions/src/routes/method-not-allowed/post.tsx (1)
Route(5-7)
e2e/react-start/server-functions/src/routes/method-not-allowed/post.tsx (1)
e2e/react-start/server-functions/src/routes/method-not-allowed/get.tsx (1)
Route(5-7)
e2e/react-start/server-functions/vite.config.ts (1)
packages/router-core/src/route.ts (1)
id(1549-1551)
⏰ 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/vite.config.ts (1)
6-11: New constant-id serverFn entries look fine.
Keeps the new method-not-allowed examples aligned with the constant-id test setup.e2e/react-start/server-functions/src/routes/method-not-allowed/get.tsx (1)
5-11: Route + GET serverFn look good for exercising method validation.e2e/react-start/server-functions/src/routes/method-not-allowed/post.tsx (1)
5-13: Route + POST serverFn look good for exercising method validation.
e2e/react-start/server-functions/tests/server-functions.spec.ts
Outdated
Show resolved
Hide resolved
juliusmarminge
force-pushed
the
julius/serverfn-method-not-allowed
branch
from
506eaa7 to
95ae2c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
e2e/react-start/server-functions/tests/server-functions.spec.ts (1)
545-605: Optional: refactor to reduce duplication.Both test cases follow an identical pattern (navigate, click 4 buttons, assert 4 results). Consider extracting a shared helper function that accepts the route path, expected method, and test data to eliminate ~30 lines of duplication.
Example refactor:
async function testMethodNotAllowed( page: Page, route: string, allowedMethod: 'GET' | 'POST', expectedSuccess: string, ) { await page.goto(route) await page.waitForLoadState('networkidle') const methods = [ { name: 'get', method: 'GET' }, { name: 'post', method: 'POST' }, { name: 'put', method: 'PUT' }, { name: 'options', method: 'OPTIONS' }, ] for (const { name, method } of methods) { await page.getByTestId(`${name}-button`).click() await page.waitForLoadState('networkidle') const expected = method === allowedMethod ? `[200,"${expectedSuccess}"]` : `[405,"expected ${allowedMethod} method. Got ${method}"]` await expect(page.getByTestId('fetch-result')).toContainText(expected) } } test('serverFn defined with GET method', async ({ page }) => { await testMethodNotAllowed(page, '/method-not-allowed/get', 'GET', 'Hello, World!') }) test('serverFn defined with POST method', async ({ page }) => { await testMethodNotAllowed(page, '/method-not-allowed/post', 'POST', 'Hello, World!') })e2e/react-start/server-functions/src/routes/method-not-allowed/get.tsx (1)
20-56: Optional: reduce button duplication.The four button elements (lines 28-51) follow an identical pattern. For improved maintainability, consider mapping over method definitions:
function MethodNotAllowedFn() { const [fetchResult, setFetchResult] = useState< readonly [number, string] | null >(null) const methods = ['GET', 'POST', 'PUT', 'OPTIONS'] as const return ( <div className="flex flex-col gap-2"> <h1>Method Not Allowed GET</h1> {methods.map((method) => ( <button key={method} data-testid={`${method.toLowerCase()}-button`} onClick={() => fetchFn(method).then(setFetchResult)} > Fetch {method} </button> ))} <pre data-testid="fetch-result">{JSON.stringify(fetchResult)}</pre> </div> ) }That said, explicit button definitions may be clearer for this test/example file, so this refactor is entirely optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
e2e/react-start/server-functions/src/routes/method-not-allowed/get.tsx(1 hunks)e2e/react-start/server-functions/src/routes/method-not-allowed/post.tsx(1 hunks)e2e/react-start/server-functions/tests/server-functions.spec.ts(1 hunks)e2e/react-start/server-functions/vite.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/react-start/server-functions/src/routes/method-not-allowed/post.tsx
- e2e/react-start/server-functions/vite.config.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/routes/method-not-allowed/get.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/react-start/server-functions/tests/server-functions.spec.tse2e/react-start/server-functions/src/routes/method-not-allowed/get.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
e2e/react-start/server-functions/tests/server-functions.spec.ts
🧬 Code graph analysis (1)
e2e/react-start/server-functions/src/routes/method-not-allowed/get.tsx (1)
e2e/react-start/server-functions/src/routes/method-not-allowed/post.tsx (1)
Route(5-7)
⏰ 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). (1)
- GitHub Check: Test
🔇 Additional comments (3)
e2e/react-start/server-functions/src/routes/method-not-allowed/get.tsx (3)
1-7: LGTM!Route definition and imports are properly structured using TanStack patterns.
9-11: LGTM!Server function properly configured for GET method and returns a standard Response.
13-18: This review comment is incorrect. The code is correct as written.The hardcoded
constant_id_2is intentionally configured invite.config.ts(line 9) to assign deterministic, stable IDs to specific server functions for testing purposes. This is a stability feature, not fragile coupling.The test purpose is to validate HTTP method handling (testing 405 Method Not Allowed responses when POST/PUT/OPTIONS are sent to a GET-only endpoint). Direct invocation of
getableServerFn()cannot achieve this because it uses the RPC protocol for the configured GET method only, whereas the test needs to send raw HTTP requests with different methods to verify proper error responses.No changes are required to this code.
Likely an incorrect or invalid review comment.
1 participant
our server logs are filled with this:
this PR demotes those to 405 errors
I didn't find any existing tests for this functionality, but this catch block should handle this thrown response afaict? https://github.com/juliusmarminge/router/blob/a317d252087df7ccb1b597d4f6ce2e1b495df3e1/packages/start-server-core/src/createStartHandler.ts#L232-L234
a simple fetch verifies as much:
but there should probably be tests
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.