fix: update error handling in MatchInner to access error data directly #4746
Conversation
|
View your CI Pipeline Execution ↗ for commit b0b6c9e
☁️ Nx Cloud last updated this comment at |
Signed-off-by: leesb971204 <leesb971204@gmail.com>
leesb971204
force-pushed
the
fix/nested-notFound-data
branch
from
04e715a to
f907711
Compare
leesb971204
changed the title
leesb971204
changed the title
Signed-off-by: leesb971204 <leesb971204@gmail.com>
| if (!route.options.notFoundComponent) { | ||
| if (router.options.defaultNotFoundComponent) { | ||
| return <router.options.defaultNotFoundComponent data={data} /> | ||
| return <router.options.defaultNotFoundComponent {...data} /> |
There was a problem hiding this comment.
This part causes the structure to become nested like data: { data: ... }.
|
@schiller-manuel Is there anything else I should do to get a review on this? |
|
thanks for this PR!
|
leesb971204
force-pushed
the
fix/nested-notFound-data
branch
from
28b5de3 to
a3bdffd
Compare
…s data Signed-off-by: leesb971204 <leesb971204@gmail.com>
…nent Signed-off-by: leesb971204 <leesb971204@gmail.com>
…and isNotFound, routeId Signed-off-by: leesb971204 <leesb971204@gmail.com>
…enerics Signed-off-by: leesb971204 <leesb971204@gmail.com>
…or data props Signed-off-by: leesb971204 <leesb971204@gmail.com>
Signed-off-by: leesb971204 <leesb971204@gmail.com>
…PI reference Signed-off-by: leesb971204 <leesb971204@gmail.com>
WalkthroughAdds documentation for a NotFoundComponent; changes React and Solid renderers to spread not-found payload props into not-found components; extends NotFoundRouteProps with optional data, isNotFound, and routeId; adds tests and example usage validating data propagation and exports a notFound helper and NotFoundRouteProps type. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Loader as Route Loader
participant Router as Router
participant Renderer as renderRouteNotFound
participant NotFoundComp as NotFoundComponent
User->>Loader: navigate
Loader-->>Router: throw notFound({ data: ... })
Router->>Renderer: renderRouteNotFound(notFoundPayload)
note right of Renderer #D6F5D6: New: spread payload into props<br/>{...data, isNotFound, routeId}
Renderer->>NotFoundComp: call NotFoundComponent({...data, isNotFound, routeId})
NotFoundComp-->>User: render UI using props
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
Signed-off-by: leesb971204 <leesb971204@gmail.com>
![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 (2)
examples/react/kitchen-sink/src/main.tsx (2)
45-51: Tighten the type guard to exclude NaN/non-integersUse Number.isInteger to align with zod’s int and avoid NaN.
-function isMissingUserData(data: unknown): data is MissingUserData { - return ( - typeof data === 'object' && - data !== null && - typeof (data as { userId?: unknown }).userId === 'number' - ) -} +function isMissingUserData(data: unknown): data is MissingUserData { + return ( + typeof data === 'object' && + data !== null && + Number.isInteger((data as { userId?: unknown }).userId as number) + ) +}
53-72: Optionally type the not-found props to your payload and simplify accessIf NotFoundRouteProps is generic (per this PR’s intent), parameterizing removes the runtime guard.
-function UsersNotFoundComponent({ data, routeId }: NotFoundRouteProps) { - const userId = isMissingUserData(data) ? data.userId : undefined +function UsersNotFoundComponent({ data, routeId }: NotFoundRouteProps<MissingUserData>) { + const userId = data?.userIdPlease confirm NotFoundRouteProps is available in this branch; otherwise ignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
examples/react/kitchen-sink/src/main.tsx(5 hunks)
🔇 Additional comments (6)
examples/react/kitchen-sink/src/main.tsx (6)
14-14: LGTM: Imported notFound helper correctlyMatches new not-found flow.
33-33: LGTM: Typed NotFoundRouteProps importType-only import is correct.
41-43: LGTM: MissingUserData shapeClear and minimal payload type for the example.
487-487: LGTM: Route-level notFoundComponent registrationCorrectly wires the per-route not-found UI.
602-613: LGTM: Discoverability link for not-found scenarioNice UX touch for demonstrating the behavior.
661-673: LGTM: Throwing notFound with payloadUses the helper as intended for missing user case.
Signed-off-by: leesb971204 <leesb971204@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/router-core/tests/load.test-d.ts (5)
6-8: Consider makingdatageneric, not fixedunknown.Issue #4744 calls for generic-aware typing so user payload type flows to the component. Today this asserts
unknown | undefined. PreferNotFoundRouteProps<TData = unknown>withdata?: TDatato preserve strong typing by defaulting tounknownbut allowing concreteTData.If you adopt a generic, extend tests like:
// Example additional check type StringProps = NotFoundRouteProps<string> expectTypeOf<StringProps['data']>().toEqualTypeOf<string | undefined>()And in core types (outside this file), shape it as:
export type NotFoundRouteProps<TData = unknown> = { data?: TData isNotFound: true routeId: RouteIds<RegisteredRouter['routeTree']> }
9-13: TypeisNotFoundas the literaltrueinstead ofboolean.At runtime this is always true; a literal type improves narrowing and matches the issue’s expectations.
If you change the core type, update this test accordingly:
-expectTypeOf<NotFoundRouteProps['isNotFound']>().toEqualTypeOf<boolean>() +expectTypeOf<NotFoundRouteProps['isNotFound']>().toEqualTypeOf<true>()And similarly adjust later checks (e.g., Line 89) to expect
true.
35-41: Prefersatisfiesfor stricter object conformance in tests.Using
satisfiespreserves the object’s inferred shape while ensuring it conforms, catching excess props without widening.Apply:
-const props: NotFoundRouteProps = { +const props satisfies NotFoundRouteProps = { isNotFound: true, routeId: '/' as RouteIds<RegisteredRouter['routeTree']>, }Repeat similarly for other object literals in this file.
43-50: Samesatisfiessuggestion for the “all properties” case.-const props: NotFoundRouteProps = { +const props satisfies NotFoundRouteProps = { data: { message: 'Not found' }, isNotFound: true, routeId: '/' as RouteIds<RegisteredRouter['routeTree']>, }
1-1: Rename file for clarity.Content focuses on not-found props, not “load”. Consider renaming to not-found-props.test.ts for discoverability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/tests/load.test-d.ts(1 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/router-core/tests/load.test-d.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/tests/load.test-d.ts
🔇 Additional comments (3)
packages/router-core/tests/load.test-d.ts (3)
23-33: LGTM: required-ness checks forisNotFoundandrouteId.These assertions are good and align with the intended public surface.
84-98: Component-acceptance checks look correct.These validations correctly ensure the component parameter matches the public NotFoundRouteProps surface.
110-126: Spreadability checks look good.Props spreading and parameter inference assertions are appropriate and align with the intended usage.
| import { describe, expectTypeOf, test } from 'vitest' | ||
| import type { NotFoundRouteProps, RegisteredRouter, RouteIds } from '../src' | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Wrong test runner/file type: .test-d.ts with Vitest will not run (and .d.ts cannot contain runtime code).
This file imports/uses Vitest APIs but has a .d.ts suffix. By default, Vitest won’t pick up .test-d.ts, and .d.ts files are for ambient declarations, not executable code.
- Fix: rename to .test.ts (recommended), or switch to tsd-runner and its assertions (then don’t import from vitest).
Run to verify current test matching and whether .test-d.ts is configured:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Vitest configs referencing .test-d.ts:"
rg -nP '(test|include).*d\.ts' -C2 -g 'vitest.config.*' || true
echo
echo "Existing .test-d.ts files:"
fd '\.test-d\.ts$' || true
echo
echo "Any tsd/tsd-runner usage:"
rg -nP "from 'tsd'|tsd-runner" -C2 || trueLength of output: 1004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Imports from 'vitest' (all files):"
rg -n "from\s+['\"]vitest['\"]" -S || true
echo
echo "2) .test-d.ts files that contain vitest/test/describe/expectTypeOf/expectType:"
fd '\.test-d\.ts$' -e || true | while read -r f; do
if rg -n "(from\s+['\"]vitest['\"]|describe\s*\(|\btest\s*\(|expectTypeOf\b|expectType\b|it\s*\()" -n -S "$f" >/dev/null 2>&1; then
echo "---- $f ----"
rg -n "(from\s+['\"]vitest['\"]|describe\s*\(|\btest\s*\(|expectTypeOf\b|expectType\b|it\s*\()" -n -S "$f" || true
fi
done
echo
echo "3) Head (first 20 lines) of .test-d.ts files that matched:"
fd '\.test-d\.ts$' -e || true | while read -r f; do
if rg -n "(from\s+['\"]vitest['\"]|describe\s*\(|\btest\s*\(|expectTypeOf\b|expectType\b|it\s*\()" -n -S "$f" >/dev/null 2>&1; then
echo "---- $f (head) ----"
sed -n '1,20p' "$f" || true
fi
done
echo
echo "4) Look for tsd / tsd-runner usage and references in package.json files:"
rg -n "from\s+['\"]tsd['\"]|tsd-runner" -S || true
rg -n '"(tsd|tsd-runner|vitest)"' -S **/package.json || true
echo
echo "5) Vitest config files / references:"
rg -n --hidden --glob '!node_modules' "vitest" -S || true
rg -n --hidden --glob '!node_modules' "vitest.config" -S || trueLength of output: 18513
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
"packages/router-core/tests/load.test-d.ts"
"packages/router-core/tests/utils.test-d.ts"
"packages/router-core/tests/load.test.ts"
"packages/start-client-core/src/tests/createServerFn.test-d.ts"
"packages/arktype-adapter/tests/index.test-d.ts"
)
for f in "${files[@]}"; do
if [ -f "$f" ]; then
echo "---- $f ----"
sed -n '1,240p' "$f" || true
echo
else
echo "missing: $f"
fi
done
echo "---- vitest config files (if any) ----"
rg -n --hidden --glob '!node_modules' "vitest.config" -S || trueLength of output: 22946
Rename .test-d.ts files that contain runtime Vitest code to .test.ts (or convert them to a type-only runner)
packages/router-core/tests/load.test-d.ts imports/uses Vitest runtime APIs (describe/test/expectTypeOf) but ends with .d.ts (a declaration file) and will not be executed by Vitest. Fix: rename to .test.ts (recommended) or convert to a type-only test runner (tsd/tsd-runner) and use type-only imports.
🤖 Prompt for AI Agents
In packages/router-core/tests/load.test-d.ts around lines 1 to 3, the file
imports and uses Vitest runtime APIs but is named as a TypeScript declaration
file (.d.ts) so Vitest won't run it; rename the file to load.test.ts so Vitest
executes it, or if you intend a type-only test convert to a type-only runner
(e.g., tsd/tsd-runner) and change imports to type-only to remove runtime usage.
08a206c to
f7509ba
Compare
When you have some time, could you take another look at this PR? |
4 participants
fixes #4744
Summary by CodeRabbit
New Features
Documentation
Examples
Tests