Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.4k
feat(router-core): validate params while matching [WIP]#5936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
coderabbitaibot commentedNov 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
WalkthroughThis change extends the routing engine to support route-level custom parameter parsing. Route options now accept a Changes
Sequence DiagramsequenceDiagram participant Router as Route Matching Engine participant Tree as Segment Tree participant Node as Segment Node participant Parse as Parse Function participant Result as Match Result Router->>Tree: Start traversal Router->>Node: Check for parse function alt parse function exists Node-->>Router: parse function found Router->>Router: Extract params early Router->>Parse: Apply parse(params) alt parse succeeds Parse-->>Router: Parsed params Router->>Router: Attach to frame Router->>Result: Continue matching with parsed params else parse throws error Parse-->>Router: Error Router->>Router: Prune current path Router->>Result: Try alternate routes end else no parse function Node-->>Router: parse = null Router->>Result: Continue standard matching endEstimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
nx-cloudbot commentedNov 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... | ❌ Failed | 1m 53s | View ↗ |
nx run-many --target=build --exclude=examples/*... | ✅ Succeeded | 1s | View ↗ |
☁️Nx Cloud last updated this comment at2025-11-30 18:08:04 UTC
pkg-pr-newbot commentedNov 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/router-core/src/new-process-route-tree.ts (1)
917-919:Silent error handling may hinder debugging.Parse errors are caught and silently cause the route to be skipped via
continue. While this enables fallback to alternative routes, it provides no visibility into why a route was rejected, making debugging difficult for developers.Consider adding optional logging or a debug mode that reports parse failures, especially during development.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/new-process-route-tree.ts(21 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: FatahChanRepo: TanStack/router PR: 5475File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0Timestamp: 2025-10-14T18:59:33.990ZLearning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.Learnt from: nlynzaadRepo: TanStack/router PR: 5284File: e2e/react-start/basic/server.js:50-0Timestamp: 2025-09-28T21:41:45.233ZLearning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.Learnt from: nlynzaadRepo: TanStack/router PR: 5182File: e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts:167-172Timestamp: 2025-09-22T00:56:53.426ZLearning: In TanStack Router, underscores are intentionally stripped from route segments during path parsing, but preserved in base path segments. This is the expected behavior implemented in PR #5182.📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaadRepo: TanStack/router PR: 5402File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21Timestamp: 2025-10-08T08:11:47.088ZLearning: 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:
packages/router-core/src/new-process-route-tree.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: Test
- GitHub Check: Preview
🔇 Additional comments (4)
packages/router-core/src/new-process-route-tree.ts (4)
375-376:Verify parse prioritization is always desired.The sort function unconditionally prioritizes nodes with parse functions over those without. While this makes sense for validation (try more specific routes first), it's hardcoded with no way to customize priority.
Consider whether this ordering is always appropriate, especially when multiple routes with different parse functions compete. The current tie-breaking logic (lines 377-394) would then determine order, which might be arbitrary.
236-244:Node reuse prevented when parse is present.Routes with parse functions cannot share segment nodes, even if they're otherwise identical. This conservative approach is safe (prevents incorrect sharing of different parse functions), but increases memory usage if many routes have parse functions.
This is likely acceptable for correctness, but worth noting for performance-sensitive applications with large route trees.
Also applies to: 278-286
765-767:LGTM: Param extraction correctly resumes from saved state.The logic properly handles incremental param extraction:
- Resumes from
leaf.extractstate if present (line 765-767)- Extracts params from remaining nodes
- Merges previously extracted params back in (line 823)
This avoids re-extracting and re-validating params when multiple parse functions exist in the route path.
Also applies to: 823-824
534-547:LGTM: Type definitions are consistent with implementation.The parse field is properly typed across
SegmentNode,RouteLike, andMatchStackFrame. The function signature(params: Record<string, string>) => anyaccurately reflects that params are strings and the return type is currently unused.
| if(node.parse){ | ||
| // if there is a parse function, we need to extract the params that we have so far and run it. | ||
| // if this function throws, we cannot consider this a valid match | ||
| try{ | ||
| ;[params,extract]=extractParams(path,parts,frame) | ||
| // TODO: can we store the parsed value somewhere to avoid re-parsing later? | ||
| node.parse(params) | ||
| frame.extract=extract | ||
| frame.params=params | ||
| }catch{ | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🧩 Analysis chain
Parse function return value is ignored.
The parse function is typed to returnany (line 546), but the return value is discarded on line 914. This creates ambiguity about whether parse should validate (throw on error) or transform (return modified params). The current implementation only supports validation.
Consider either:
- Documenting that parse is validation-only and updating the return type to
void - Or storing and using the return value to support param transformation
The TODO on line 913 hints at storing results, which would enable transformation. If transformation isn't needed, clarify this in the type signature.
🌐 Web query:
Does TanStack Router support parameter transformation or only validation in parse functions?💡 Result:
Short answer: parse functions can transform (coerce/convert) parameters — not just validate them. The router calls your params.parse with the raw string values and uses whatever typed/transformed values you return (stringify is the reverse). That said, there are a few reported type-inference/coercion issues in some versions/adapters (see the Zod issue). [1][2]
Sources:
- TanStack Router docs — custom parameter parsing (params.parse / params.stringify) and search‑param parsing/validation. [1]
- GitHub issue reporting type/coercion/inference problems with validators/adapters (example with Zod). [2]
Parse function return value must be captured and used for parameter transformation.
The web search confirms that TanStack Router parse functions can transform (coerce/convert) parameters, and the router uses whatever typed/transformed values the parse function returns. However, the PR code on line 914 callsnode.parse(params) but discards the return value, preventing parameter transformation.
Fix required at line 908-920: Capture the parse return value and use it to update params:
try{;[params,extract]=extractParams(path,parts,frame)params=node.parse(params)??params// Capture and use transformed valueframe.extract=extractframe.params=params}catch{continue}
Without this fix, routes with parse functions cannot perform parameter type coercion/conversion.
🤖 Prompt for AI Agents
In packages/router-core/src/new-process-route-tree.ts around lines 908 to 920,the parse function's return value is ignored so transformed/coerced params arenot applied; update the try block to capture node.parse's return and assign itback to params (using a nullish fallback to the original params if parse returnsundefined), then set frame.extract and frame.params to the updated values; keepthe existing error handling (catch/continue) intact.
Uh oh!
There was an error while loading.Please reload this page.
Warning
UNTESTED, DO NOT MERGE
Use the
params.parseoption (https://tanstack.com/router/latest/docs/framework/react/api/router/RouteOptionsType#paramsparse-method) to validate path params while matching a route. This allows us to look foranother match if the validation fails.Pros: more flexible routing. Any
$paramcan now become a regex route segment (or any validation you like).Cons: routes with a
params.parsemethod cannot share the same node as other routes of otherwise the same shape. If used frequently, this increases branching, which increases the number of branches we need to explore to find a match.Some details:
params.parsehave priority over routes without (all else being equal)extractParamsis now "resumable" so we can call it at several point while matching without repeating work (at the expense of allocating more objects)TODO:
params.parseto avoid duplicate work after matchingparams.parsefunction are on the way, should the 2nd receive the output of the 1st? Or do they both receive the raw Record<string,string>?params.parseshould be preserved to be set in the store or thrown ifopts?.throwOnErrorrouteParamsandstrictParams?options.parseParamsin addition tooptions.params.parsenotFoundandredirecterrors still be respected when we're trying to "continue matching" despite the parsing error?params.parseis set on a pathless layout route? This has no representation in the segment trie.params.parse? then we need to handle an array of them??params.parseI think each parse function receives the output of the previous one, see below. But I also think this means we can't decode it to something not stringifyable by
String()(i.e. objects) because this partially parsed object will be used ininterpolatePathto generate part of thematchIdrouter/packages/router-core/src/router.ts
Lines 1393 to 1417 ind9e403b