feat: matched route pattern and method on results (opt-in routes: true) - #192
feat: matched route pattern and method on results (opt-in routes: true)#192pi0x wants to merge 4 commits into
routes: true)#192Conversation
…ue`) addRoute stores the original registered pattern and method on each entry; findRoute/findAllRoutes/findOverlappingRoutes and the compiler expose them behind an opt-in `routes: true` flag so compiled output size is unchanged for consumers that don't need attribution. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds opt-in route and method attribution across route lookup, overlap matching, and compiled output. It also updates stored match shapes, public type contracts, docs, tests, and bundle-size thresholds. ChangesRoute attribution feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: Stream initialization permanently failed: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
- findOverlappingRoutes dedups per registration (data + route + method),
so distinct patterns sharing one data reference are all reported
- default findRoute results never leak route/method/paramsRegexp: param-less
matches return a per-entry object precomputed in addRoute (zero allocation),
param matches stay a fresh { data, params } — no attribution cost unless
routes: true is set
- params: false is the only raw-entry return path (findRoute + findAllRoutes)
- compiler dedups route/method strings through the existing $N data table
- MethodData route/method are required; overlap drops positional flag threading
Co-Authored-By: Claude Fable 5 <noreply@anthropic.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: 1
🧹 Nitpick comments (2)
src/operations/find.ts (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared
optsshape into a namedinterface.This inline object-literal type is duplicated verbatim in
findAllRoutes(src/operations/find-all.ts, Line 11). Consider hoisting a singleinterface FindOptions { params?: boolean; routes?: boolean; normalize?: boolean }(e.g. insrc/types.ts) and reusing it here and infindAllRoutes.As per coding guidelines, "Prefer
interfacefor defining object shapes in TypeScript."♻️ Proposed refactor
+// in src/types.ts +export interface FindOptions { + params?: boolean; + routes?: boolean; + normalize?: boolean; +}- opts?: { params?: boolean; routes?: boolean; normalize?: boolean }, + opts?: FindOptions,🤖 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 `@src/operations/find.ts` at line 11, Extract the duplicated inline opts object shape into a named interface, such as FindOptions, instead of keeping it inline in find and findAllRoutes. Define the shared object shape once in a common types location (for example, a shared types module) and update the find and findAllRoutes signatures to reference that interface. This keeps the opts type consistent across both functions and follows the guideline to prefer interface for object shapes.Source: Coding guidelines
src/operations/overlap.ts (1)
179-197: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a regression test for the primitive-data + expansion tradeoff.
The dedup intentionally skips primitive/absent
data(per the comment above), so a route registered with optional/group syntax (e.g./a/:x?) and a primitivedatavalue will report its pattern once per expanded internal entry instead of collapsing to a single match — unlike the reference-type case, which is already covered intest/overlap.test.ts(Lines 223-231) using objectdata. Adding an equivalent test with primitivedatawould lock in this documented tradeoff and guard against an unintended future "fix" that silently changes duplicate-registration detection.🤖 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 `@src/operations/overlap.ts` around lines 179 - 197, Add a regression test in test/overlap.test.ts covering optional/group expansion with primitive data, since overlap dedup in src/operations/overlap.ts intentionally does not collapse non-reference data. Mirror the existing object-data coverage around the overlap/registration duplicate cases, but register a route like /a/:x? with a primitive payload and assert the expanded entries are reported separately. This will lock in the current tradeoff and prevent future changes to shapesOverlap, seen, or the entry processing loop from accidentally deduping primitive-data registrations.
🤖 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 `@src/operations/add.ts`:
- Line 128: The route data assignment in add should preserve valid falsy values
instead of converting them to null; using data || (null as T) causes 0, empty
strings, and false to be dropped. Update the _data initialization in add to use
nullish coalescing so only undefined or null fall back to the sentinel, keeping
falsy payloads intact when routes match.
---
Nitpick comments:
In `@src/operations/find.ts`:
- Line 11: Extract the duplicated inline opts object shape into a named
interface, such as FindOptions, instead of keeping it inline in find and
findAllRoutes. Define the shared object shape once in a common types location
(for example, a shared types module) and update the find and findAllRoutes
signatures to reference that interface. This keeps the opts type consistent
across both functions and follows the guideline to prefer interface for object
shapes.
In `@src/operations/overlap.ts`:
- Around line 179-197: Add a regression test in test/overlap.test.ts covering
optional/group expansion with primitive data, since overlap dedup in
src/operations/overlap.ts intentionally does not collapse non-reference data.
Mirror the existing object-data coverage around the overlap/registration
duplicate cases, but register a route like /a/:x? with a primitive payload and
assert the expanded entries are reported separately. This will lock in the
current tradeoff and prevent future changes to shapesOverlap, seen, or the entry
processing loop from accidentally deduping primitive-data registrations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b14480c-b986-4294-83eb-360a4199e6b4
📒 Files selected for processing (12)
AGENTS.mdREADME.mdsrc/compiler.tssrc/operations/add.tssrc/operations/find-all.tssrc/operations/find.tssrc/operations/overlap.tssrc/types.tstest/bench/bundle.test.tstest/find-all.test.tstest/find.test.tstest/overlap.test.ts
|
|
||
| // Assign index, params and data to the node | ||
| const hasParams = paramsMap.length > 0; | ||
| const _data = data || (null as T); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect current line and its history to determine if `||` predates this PR.
fd -t f 'add.ts' src/operations --exec rg -n 'null as T' {}
git log -L '/const _data/,+1:src/operations/add.ts' -n 2 --oneline 2>/dev/null || \
git log -S 'null as T' --oneline -- src/operations/add.tsRepository: h3js/rou3
Length of output: 421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files first.
ast-grep outline src/operations/add.ts --view expanded || true
ast-grep outline src/operations/overlap.ts --view expanded || true
# Read the relevant sections with line numbers.
printf '\n--- src/operations/add.ts ---\n'
sed -n '1,220p' src/operations/add.ts | cat -n
printf '\n--- src/operations/overlap.ts ---\n'
sed -n '1,240p' src/operations/overlap.ts | cat -n
# Search for null sentinel usage and route data handling.
printf '\n--- sentinel searches ---\n'
rg -n "null as T|null\)|=== null|== null|data \\|\\||data \\?\\?" src/operations src | sed -n '1,200p'Repository: h3js/rou3
Length of output: 18148
Preserve falsy route data data || (null as T) turns 0, "", and false into null, so those registered values are lost on match. null is already the missing-data sentinel here, so this should be data ?? (null as T) if falsy payloads are allowed.
🤖 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 `@src/operations/add.ts` at line 128, The route data assignment in add should
preserve valid falsy values instead of converting them to null; using data ||
(null as T) causes 0, empty strings, and false to be dropped. Update the _data
initialization in add to use nullish coalescing so only undefined or null fall
back to the sentinel, keeping falsy payloads intact when routes match.
- unify the `routes` flag check (truthiness everywhere, matching
findRoute/compiler) so all entry points agree for plain-JS callers
- key registration dedup in findOverlappingRoutes on nested maps
(data -> method -> route) instead of a space-delimited composite
string that unvalidated method strings could collide
- fall back to a fresh { data, params } when an entry lacks the
precomputed `res` (context built by pre-attribution rou3) instead
of silently reporting a match as a miss
- document shared-res scope and the data-snapshot limitation in
README/AGENTS.md
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
dataRef scanned ctx.data with indexOf per reference — quadratic over the table, which routes:true roughly doubles (route/method strings). A value->index Map makes it O(1): compileRouterToString on a 10k-route router drops 263ms -> 40ms (889ms -> 36ms with routes:true). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
🔍 What & why
findAllRoutes/ compiledmatchAll/findOverlappingRoutesreturned{ data, params }with no way to tell which registered pattern produced a layer. Consumers like h3-rules need per-layer pattern attribution (params attribution across pre-merged subsumption chains) and currently work around it by wrapping every registration's data in{ route, method, rules }.This adds an opt-in flag:
🛠️ How
addRoutenow stores the original registered pattern (before internal group/modifier expansion, so/a/:x?reports/a/:x?) and the uppercased method on every tree entry — threaded through the expansion recursion via an internal_addRoute.routes: trueis accepted byfindRoute,findAllRoutes,findOverlappingRoutes(newoptsarg), and as a compiler option forcompileRouter/compileRouterToString(applies to single-match andmatchAll).methodis""for method-agnostic registrations.routes: trueis pinned with exacttoEqualintest/find-all.test.ts; docs added to README ("Matched route attribution") and AGENTS.md.Two default-path (no-flag) behaviors change for existing consumers:
findOverlappingRoutesno longer collapses distinct registrations sharing onedatareference. Registering the same middleware object on/a/**and/b/**previously returned one match; it now returns both (useroutes: trueto tell them apart). Optional/group expansions of a single registration still collapse to one match. Consumers deduplicating by result count must dedup bydatareference themselves.findRoutefor static paths returns a mapped{ data, params }object instead of the raw tree entry (parity with the dynamic path and compiled output). Internal fields likeparamsRegexpare no longer visible on default results,findRoute(...) === findRoute(..., { params: false })no longer holds for static routes, and mutating a raw entry'sdata(viaparams: false) after registration is not supported — default results snapshotdataataddRoutetime. Param-less default matches are one shared object per entry (documented in README; clone before mutating).Resolves item 4 of the h3-rules needs list.
🤖 Generated with Claude Code
Summary by CodeRabbit
routes: trueforfindRoute,findAllRoutes,findOverlappingRoutes, andcompileRouter(..., { routes: true }), returningroute/methodalongsidedata.optsparameter support and match/mutability semantics.