fix(docs): keep notFound inside route boundaries#851
fix(docs): keep notFound inside route boundaries#851Varkoff wants to merge 4 commits intoTanStack:mainfrom
Conversation
✅ Deploy Preview for tanstack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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)
📝 WalkthroughWalkthroughIntroduces a DocsNotFoundError type and uses it across docs utilities; loaders and server GET handlers now catch this error to attempt resolving a docs redirect (308) or call notFound(), rethrowing other errors unchanged. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/utils/docs-errors.ts (1)
1-4: Consider exportingDocsNotFoundError.The type is only used internally now, but callers that want to type a caught error (e.g., in tests or future route handlers) currently have no public name for it. Minor — happy to skip if intentional.
♻️ Proposed change
-type DocsNotFoundError = Error & { +export type DocsNotFoundError = Error & { data: { message: string } isDocsNotFound: true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/docs-errors.ts` around lines 1 - 4, The type DocsNotFoundError is currently declared but not exported; make it a public type by exporting it (e.g., change the declaration to export type DocsNotFoundError = ...), so callers/tests can import and use it when typing caught errors; ensure any modules that need it import { DocsNotFoundError } from this module.src/routes/$libraryId/$version.docs.{$}[.]md.tsx (1)
27-42: Consider attempting a redirect before returning 404 for.mdendpoints.The HTML routes (
$version.docs.$.tsx,$version.docs.framework.$framework.$.tsx) callresolveDocsRedirecton not-found and issue a 308 if a redirect is configured. This.mdendpoint now just returnsnotFound(), which means tools/LLMs/curl hitting a renamed.mdURL will see a 404 even though the HTML URL would 308 to the new location. Minor consistency gap — worth aligning if feasible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/`$libraryId/$version.docs.{$}[.]md.tsx around lines 27 - 42, When loadDocs throws and isDocsNotFoundError(error) is true, attempt the same redirect logic used by the HTML routes: call resolveDocsRedirect(...) with the same repo, branch, docsRoot (root) and docsPath and if it returns a redirect target throw a 308 redirect to that URL (e.g. redirect(308, target)), otherwise fall back to throw notFound(); update the catch block around loadDocs to perform this check before throwing notFound so renamed .md URLs mirror the HTML endpoints' redirect behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/utils/docs-errors.ts`:
- Around line 1-25: The route error guards fail to detect serialized
DocsNotFound errors because custom Error properties are lost during TanStack
Start serialization; update the error handling in the route handlers that
currently use isDocsNotFound/isDocsNotFoundError to also check for the
serialized shape by adding the structural fallback (error && typeof error ===
'object' && 'isDocsNotFound' in error) alongside the existing checks so that
createDocsNotFoundError() thrown from
fetchDocs/fetchFile/fetchRepoDirectoryContents will be recognized and trigger
the redirect/notFound() logic.
---
Nitpick comments:
In `@src/routes/`$libraryId/$version.docs.{$}[.]md.tsx:
- Around line 27-42: When loadDocs throws and isDocsNotFoundError(error) is
true, attempt the same redirect logic used by the HTML routes: call
resolveDocsRedirect(...) with the same repo, branch, docsRoot (root) and
docsPath and if it returns a redirect target throw a 308 redirect to that URL
(e.g. redirect(308, target)), otherwise fall back to throw notFound(); update
the catch block around loadDocs to perform this check before throwing notFound
so renamed .md URLs mirror the HTML endpoints' redirect behavior.
In `@src/utils/docs-errors.ts`:
- Around line 1-4: The type DocsNotFoundError is currently declared but not
exported; make it a public type by exporting it (e.g., change the declaration to
export type DocsNotFoundError = ...), so callers/tests can import and use it
when typing caught errors; ensure any modules that need it import {
DocsNotFoundError } from this module.
🪄 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: ea289840-608c-4a40-9266-7ecce00e513e
⛔ Files ignored due to path filters (1)
.github/pr-assets/ai-guides-structured-outputs-error.pngis excluded by!**/*.png
📒 Files selected for processing (7)
src/routes/$libraryId/$version.docs.$.tsxsrc/routes/$libraryId/$version.docs.framework.$framework.$.tsxsrc/routes/$libraryId/$version.docs.framework.$framework.{$}[.]md.tsxsrc/routes/$libraryId/$version.docs.{$}[.]md.tsxsrc/utils/docs-errors.tssrc/utils/docs.functions.tssrc/utils/docs.ts
| type DocsNotFoundError = Error & { | ||
| data: { message: string } | ||
| isDocsNotFound: true | ||
| } | ||
|
|
||
| export function createDocsNotFoundError(message = 'No doc was found here!') { | ||
| const error = new Error(message) as DocsNotFoundError | ||
|
|
||
| error.name = 'DocsNotFoundError' | ||
| error.data = { message } | ||
| error.isDocsNotFound = true | ||
|
|
||
| return error | ||
| } | ||
|
|
||
| export function isDocsNotFoundError( | ||
| error: unknown, | ||
| ): error is DocsNotFoundError { | ||
| return ( | ||
| typeof error === 'object' && | ||
| error !== null && | ||
| 'isDocsNotFound' in error && | ||
| error.isDocsNotFound === true | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TanStack Start createServerFn thrown error serialization custom properties preserved client
💡 Result:
In TanStack Start, errors thrown inside a createServerFn handler are sent back to the client only after being serialized (TanStack uses JSON/seroval-style serialization). This means any non-JSON-serializable data on the thrown value (e.g., Error objects via JSON.stringify, or custom properties that don’t serialize) cannot be preserved as-is on the client. 1) Why “serialization/custom properties preserved client” fails - TanStack explicitly serializes the error to return it to the client. The server-function handler catches the thrown value and JSON.stringify’s the serialized form of the error before returning it in the response body. - As a result, custom properties that aren’t representable by the serializer (and plain Error instances) will be lost or break serialization instead of being preserved. 2) What you can do - Throw Error instances with only JSON-compatible fields (message, name), or construct a plain object error payload yourself and throw/return something that is known-serializable. - Avoid returning/throwing types that include unknown or other non-serializable values; TanStack validates serializability and will error if the return type includes fields like unknown (example shown in a createServerFn issue). - If you need to keep extra error fields, ensure they are serializable primitives/objects (and not instances with methods/prototypes like Error, Response, etc.). 3) Source-backed notes from TanStack - TanStack documents that “Errors are serialized to the client” for server functions. - TanStack Start issue reports that “server functions can’t serialize error objects” because JSON.stringify can’t serialize Error objects (originally). A later comment indicates it was likely fixed, but the core constraint remains: serialization rules apply. - The server runtime code shows the catch path JSON.stringify’ing a cross-JSON serialization of the thrown error. If you paste your exact error shape (what you throw, e.g., throw new Error(...) vs throw {message, ...custom} vs throw someObject) and your expected client-side access (e.g., error.customProp), I can tell you the safest way to structure it for TanStack Start’s serializer.
Citations:
- 1: https://tanstack.com/start/latest/docs/framework/react/guide/server-functions
- 2: Start: server functions can't serialize error objects router#2535
- 3: https://github.com/TanStack/router/blob/87a51603/packages/start-server-core/src/server-functions-handler.ts
- 4: createServerFn handler encounters a type error when the return type contains a property of type unknown router#5229
🏁 Script executed:
# First, find the files mentioned in the review comment
fd -t f "docs.functions.ts"
# Also find the route files mentioned
fd -t f "$libraryId" --extension tsx
# Find test files that might test error handling
fd -t f -e test.ts -e test.tsx -e spec.ts -e spec.tsxRepository: TanStack/tanstack.com
Length of output: 15260
🏁 Script executed:
# Search for usage of isDocsNotFoundError and how errors are handled
rg -t ts -t tsx "isDocsNotFoundError" -A 3 -B 3Repository: TanStack/tanstack.com
Length of output: 92
🏁 Script executed:
# Search for the isNotFound pattern mentioned as existing fallback
rg -t ts -t tsx "isNotFound" -A 2 -B 2 | head -50Repository: TanStack/tanstack.com
Length of output: 92
🏁 Script executed:
# Check if there's any serialization/error configuration in the codebase
rg -t ts -t tsx "seroval|Serializer|createServerFn" -l | head -20Repository: TanStack/tanstack.com
Length of output: 92
🏁 Script executed:
# Read the docs-errors.ts file
cat -n src/utils/docs-errors.tsRepository: TanStack/tanstack.com
Length of output: 800
🏁 Script executed:
# Read docs.functions.ts to see how errors are thrown
cat -n src/utils/docs.functions.tsRepository: TanStack/tanstack.com
Length of output: 11462
🏁 Script executed:
# Check the route file for error handling patterns
cat -n src/routes/'$libraryId'/'$version'.docs.'$'.tsx | head -100Repository: TanStack/tanstack.com
Length of output: 3477
🏁 Script executed:
# Look for isNotFound usage in route files
rg "isNotFound" src/routes --type-add 'tsx:*.tsx' -t tsx -A 3 -B 3 | head -80Repository: TanStack/tanstack.com
Length of output: 4512
🏁 Script executed:
# Look for test files that might test the error serialization
fd -type f "test\|spec" | grep -i "docs\|error" | head -20Repository: TanStack/tanstack.com
Length of output: 235
🏁 Script executed:
# Search for any test files testing the error behavior
rg "isDocsNotFoundError|createDocsNotFoundError" -t ts -t tsx -A 5 -B 5 | head -100Repository: TanStack/tanstack.com
Length of output: 92
🏁 Script executed:
# Check if there's a serialization configuration in the project
rg "serialize|serializer" src/ --type-add 'tsx:*.tsx' -l | head -10Repository: TanStack/tanstack.com
Length of output: 167
🏁 Script executed:
# Look at the other route file mentioned to see its error handling
sed -n '1,100p' 'src/routes/$libraryId/$version.docs.framework.$framework.$.tsx'Repository: TanStack/tanstack.com
Length of output: 3207
Add fallback structural check for serialized isDocsNotFound property.
TanStack Start serializes errors across the server-function boundary, and custom Error properties don't survive this serialization. The route handlers in src/routes/$libraryId/$version.docs.$.tsx and .framework.$framework.$.tsx already handle this for Router errors with the fallback 'isNotFound' in error check (lines 42 and 45), but lack an equivalent fallback for isDocsNotFound.
Update the error handling in both routes to include:
(error && typeof error === 'object' && 'isDocsNotFound' in error)
alongside the existing structural checks. This ensures that createDocsNotFoundError() thrown from fetchDocs, fetchFile, and fetchRepoDirectoryContents will be properly detected on the client after serialization, and the redirect/notFound() fallback logic will trigger correctly for client-side navigation into non-existent docs pages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/docs-errors.ts` around lines 1 - 25, The route error guards fail to
detect serialized DocsNotFound errors because custom Error properties are lost
during TanStack Start serialization; update the error handling in the route
handlers that currently use isDocsNotFound/isDocsNotFoundError to also check for
the serialized shape by adding the structural fallback (error && typeof error
=== 'object' && 'isDocsNotFound' in error) alongside the existing checks so that
createDocsNotFoundError() thrown from
fetchDocs/fetchFile/fetchRepoDirectoryContents will be recognized and trigger
the redirect/notFound() logic.
1 participant
Summary
Opening the official docs page for structured outputs currently breaks:
Unexpectedly client reference export 'notFound' is called on serverThe underlying problem is that the shared docs helpers and
createServerFnhandlers were throwingnotFound()directly. Once that crossed the server function boundary, SSR blew up before the route loader could do its normal redirect or 404 handling.This patch keeps
notFound()inside route boundaries:notFound()at the route layerThat keeps the redirect and 404 behavior in the route layer, and it stops the server function layer from leaking router
notFound()across the boundary.Screenshot
Test plan
pnpm run test:tscpnpm run test:lintpnpm run test:smokeSummary by CodeRabbit