Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds group-delimiter expansion, regex-constrained and unnamed parenthesized captures, and URL modifiers ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Closed
URLPattern-compatible unnamed regex groups are auto-named `_0`, `_1`, etc. Co-Authored-By: Claude Opus 4.6 <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: 4
🧹 Nitpick comments (1)
test/router.test.ts (1)
480-536: Add regression tests for multi-unnamed groups and mid-path+/*modifiers.Please add cases like
/path/(\\d+)/(\\w+)(expect_0,_1) and/a/:id+/tail,/a/:id*/tailto lock in route-wide unnamed-group indexing and suffix-preserving modifier behavior.Also applies to: 538-620
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/router.test.ts` around lines 480 - 536, Add regression tests to cover multi-unnamed regex groups and mid-path modifiers: add testRouter cases for a route like "/path/(\\d+)/(\\w+)" asserting params { _0, _1 } capture both groups, and add cases for modifier-preserving suffixes such as "/a/:id+/tail" and "/a/:id*/tail" verifying the route selection and that the "tail" suffix remains matched; update the existing unnamed regex group tests (the testRouter invocations) to include these new patterns and corresponding expected data/params so unnamed-group indexing and suffix-preserving modifier behavior are asserted.
🤖 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/operations/add.ts`:
- Around line 65-66: getParamRegexp currently resets unnamed capture naming per
segment, causing duplicate capture names like "_0" when multiple segments are
combined; update the call site where paramsRegexp[i] = regexp (and the similar
block at lines 131-138) to ensure unnamed captures are globally unique by
passing a segment-unique prefix or global counter into getParamRegexp (e.g.,
include the segment index or a route-level counter) and have getParamRegexp
incorporate that prefix into generated unnamed names; modify getParamRegexp
signature to accept that prefix and adjust callers (the place assigning
paramsRegexp[i] and the other block) so that assembled param names cannot
collide across segments.
- Around line 124-126: The expansion currently throws away suf and any inline
constraint because it only uses the param name (const name =
m[1].match(/:(\w+)/)?.[1]) and builds wc from pre.concat(`**:${name}`); fix by
extracting the full parameter token (including its constraint/regex) from m[1]
rather than just the name, and include suf when composing wc and the fallback
path; i.e., use the original matched segment (the full ":param(...)" or
":param+" token) in the wc construction and append the suf segments (if any) to
the returned paths so patterns like `/a/:id+/tail` and `:id(\d+)+/*` preserve
trailing segments and inline constraints.
In `@src/regexp.ts`:
- Around line 12-13: The current branch that routes segments into the
regex-parameter path triggers on any "(" or ":" even when the "(" is escaped
(e.g., "\("), causing literal parentheses to be treated as captures; update the
condition and pattern matching to only consider unescaped "(" characters —
replace segment.includes("(") with a check for an unescaped parenthesis (use a
negative-lookbehind or equivalent check for a preceding backslash) and update
the modMatch logic (the pattern matching anchored in the code that currently
uses /^(.*:\w+(?:\([^)]*\))?)([?+*])$/) to require unescaped "(" when detecting
parameter captures; apply the same change at the other occurrence referenced
(around line 37) so escaped "\(" remain literal and are not rewritten as named
captures.
- Around line 27-29: The current branch in src/regexp.ts that pushes into
reSegments replaces any inline constraint with .+ / .*, which ignores user regex
like :id(\d+)+; instead, preserve the original inner pattern when mod is '+' or
'*' by applying the quantifier to the inner pattern rather than swapping it for
`.+`/`.*`: construct the named capture using the original constraint (e.g., the
extracted pattern variable) and apply the modifier outside it (for example wrap
as a non-capturing group if needed) so you emit a capture like
`(?<name>(?:<originalPattern>)+)` or `(?<name>(?:<originalPattern>)* )` rather
than `(?<name>.+)`/`(?<name>.*)`; update the push to reSegments to reference the
pattern variable instead of hardcoded .+/.* while keeping existing name and mod
variables.
---
Nitpick comments:
In `@test/router.test.ts`:
- Around line 480-536: Add regression tests to cover multi-unnamed regex groups
and mid-path modifiers: add testRouter cases for a route like
"/path/(\\d+)/(\\w+)" asserting params { _0, _1 } capture both groups, and add
cases for modifier-preserving suffixes such as "/a/:id+/tail" and "/a/:id*/tail"
verifying the route selection and that the "tail" suffix remains matched; update
the existing unnamed regex group tests (the testRouter invocations) to include
these new patterns and corresponding expected data/params so unnamed-group
indexing and suffix-preserving modifier behavior are asserted.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/operations/add.tssrc/regexp.tstest/bench/bundle.test.tstest/regexp.test.tstest/router.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/operations/add.ts (2)
127-129:⚠️ Potential issue | 🔴 Critical
+/*expansion drops trailing segments and widens constrained params.Lines [128]-[129] only compose from
pre, so/a/:id+/tailloses/tail. This branch also strips:id(\d+)+down to**:id, removing its constraint.💡 Proposed fix
- const name = m[1].match(/:(\w+)/)?.[1] || "_"; - const wc = "/" + pre.concat(`**:${name}`).join("/"); - return m[2] === "+" ? [wc] : [wc, "/" + pre.join("/")]; + const [, base, mod] = m; + const name = base.match(/:(\w+)/)?.[1] || "_"; + const hasInlinePattern = /\([^)]*\)/.test(base); + if (hasInlinePattern && (mod === "+" || mod === "*")) { + throw new Error(`Unsupported modifier with inline pattern: "${segments[i]}"`); + } + const withWildcard = "/" + [...pre, `**:${name}`, ...suf].join("/"); + const withoutWildcard = "/" + [...pre, ...suf].join("/"); + return mod === "+" ? [withWildcard] : [withWildcard, withoutWildcard];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/operations/add.ts` around lines 127 - 129, The '+' expansion branch currently builds wc from only pre and a simplified `**:${name}`, which drops any trailing segments and strips parameter constraints; update the m[2] === "+" branch so wc is composed from pre + the wildcard-replaced current segment + any original following segments (preserve the tail after the matched param) and preserve the original param constraint/text (use the full matched token from m[1] rather than only the captured name) when forming the wildcard segment; adjust the code that sets `name`, `wc`, and the return for the `m[2] === "+"` case accordingly so `/a/:id+(\\d+)/tail` becomes `/a/**:id(\\d+)/tail` (and similar) instead of losing `/tail` or the constraint.
68-71:⚠️ Potential issue | 🟠 MajorUnnamed regex capture names can collide across segments.
Line [134] resets unnamed capture indexing per segment. Routes with multiple unnamed regex segments can reuse
_0and overwrite params.💡 Proposed fix
- } else if (segment.includes(":", 1) || segment.includes("(")) { - const regexp = getParamRegexp(segment); + } else if (segment.includes(":", 1) || segment.includes("(")) { + const [regexp, nextUnnamedIndex] = getParamRegexp( + segment, + _unnamedParamIndex, + ); + _unnamedParamIndex = nextUnnamedIndex; paramsRegexp[i] = regexp; node.hasRegexParam = true; paramsMap.push([i, regexp, false]); } else { paramsMap.push([i, segment.slice(1), false]); } @@ -function getParamRegexp(segment: string): RegExp { - let _i = 0; +function getParamRegexp( + segment: string, + unnamedStart = 0, +): [RegExp, number] { + let _i = unnamedStart; @@ - return new RegExp(`^${regex}$`); + return [new RegExp(`^${regex}$`), _i]; }Also applies to: 133-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/operations/add.ts` around lines 68 - 71, The unnamed regex capture names are being reset per segment causing collisions when a route has multiple unnamed regex segments; update the logic around getParamRegexp/paramsRegexp/paramsMap so unnamed capture names are globally unique across the entire route: introduce and pass a shared unnamedIndex (or use the segment index as part of the name) into getParamRegexp instead of resetting it per segment, increment the shared counter as each unnamed capture is created, and store that unique capture name in paramsMap (the entry pushed to paramsMap for each param should reference the generated unique name); ensure node.hasRegexParam remains set but do not reset the unnamed counter per segment.src/regexp.ts (2)
27-29:⚠️ Potential issue | 🟠 Major
+/*modifiers discard inline regex constraints.Line [28] emits
.+/.*, so constrained routes like:id(\d+)+and:id(\d+)*accept non-matching values.💡 Proposed fix
- // + or * - reSegments.push(mod === "+" ? `?(?<${name}>.+)` : `?(?<${name}>.*)`); + // + or * (preserve inline constraint when present) + const pattern = base.match(/:(\w+)(?:\(([^)]*)\))?/)?.[2] || "[^/]+"; + const repeated = `${pattern}(?:/${pattern})*`; + reSegments.push( + mod === "+" + ? `?(?<${name}>${repeated})` + : `?(?<${name}>${repeated})?`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/regexp.ts` around lines 27 - 29, The current branch that handles +/* modifiers always emits .+/.*, discarding any inline constraint; update the reSegments push to reuse the parsed constraint (e.g., a variable like pattern/constraint) when present by wrapping it in a non-capturing group and applying the quantifier: for "+" emit ?(?<name>(?:<constraint>)+) and for "*" emit ?(?<name>(?:<constraint>)*); if no constraint exists, fall back to the existing .+/.* behavior. Locate the code building reSegments (the block that checks mod === "+" / mod === "*" and references name) and replace the string templates so they inject the constraint variable instead of hard-coded .+/.* while preserving the existing optional prefix and named capture.
12-13:⚠️ Potential issue | 🟠 MajorEscaped literal parentheses are still parsed as capture groups.
Line [12] routes any
(to regex-parameter logic, and Line [37] rewrites escaped\(too. That breaks literal-parenthesis routes.💡 Proposed fix
- } else if (segment.includes(":") || segment.includes("(")) { + } else if (segment.includes(":") || /(^|[^\\])\(/.test(segment)) { @@ - .replace(/\((?![?<])/g, () => `(?<_${idCtr++}>`) + .replace(/(^|[^\\])\((?![?<])/g, (_, p) => `${p}(?<_${idCtr++}>`)Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/regexp.ts` around lines 12 - 13, The branch `if (segment.includes(":") || segment.includes("("))` incorrectly treats escaped literals like "\(" or "\:" as regex parameters; change the guard to detect unescaped characters only (e.g. test for an unescaped "(" and ":" using something like /(^|[^\\])\(/ and /(^|[^\\]):/), and ensure the subsequent `modMatch` logic (the `const modMatch = segment.match(/^(.*:\w+(?:\([^)]*\))?)([?+*])$/);` handling) only runs for segments with unescaped colons/parentheses so escaped `\(` and `\:` remain literal and are not parsed as capture groups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/operations/add.ts`:
- Around line 127-129: The '+' expansion branch currently builds wc from only
pre and a simplified `**:${name}`, which drops any trailing segments and strips
parameter constraints; update the m[2] === "+" branch so wc is composed from pre
+ the wildcard-replaced current segment + any original following segments
(preserve the tail after the matched param) and preserve the original param
constraint/text (use the full matched token from m[1] rather than only the
captured name) when forming the wildcard segment; adjust the code that sets
`name`, `wc`, and the return for the `m[2] === "+"` case accordingly so
`/a/:id+(\\d+)/tail` becomes `/a/**:id(\\d+)/tail` (and similar) instead of
losing `/tail` or the constraint.
- Around line 68-71: The unnamed regex capture names are being reset per segment
causing collisions when a route has multiple unnamed regex segments; update the
logic around getParamRegexp/paramsRegexp/paramsMap so unnamed capture names are
globally unique across the entire route: introduce and pass a shared
unnamedIndex (or use the segment index as part of the name) into getParamRegexp
instead of resetting it per segment, increment the shared counter as each
unnamed capture is created, and store that unique capture name in paramsMap (the
entry pushed to paramsMap for each param should reference the generated unique
name); ensure node.hasRegexParam remains set but do not reset the unnamed
counter per segment.
In `@src/regexp.ts`:
- Around line 27-29: The current branch that handles +/* modifiers always emits
.+/.*, discarding any inline constraint; update the reSegments push to reuse the
parsed constraint (e.g., a variable like pattern/constraint) when present by
wrapping it in a non-capturing group and applying the quantifier: for "+" emit
?(?<name>(?:<constraint>)+) and for "*" emit ?(?<name>(?:<constraint>)*); if no
constraint exists, fall back to the existing .+/.* behavior. Locate the code
building reSegments (the block that checks mod === "+" / mod === "*" and
references name) and replace the string templates so they inject the constraint
variable instead of hard-coded .+/.* while preserving the existing optional
prefix and named capture.
- Around line 12-13: The branch `if (segment.includes(":") ||
segment.includes("("))` incorrectly treats escaped literals like "\(" or "\:" as
regex parameters; change the guard to detect unescaped characters only (e.g.
test for an unescaped "(" and ":" using something like /(^|[^\\])\(/ and
/(^|[^\\]):/), and ensure the subsequent `modMatch` logic (the `const modMatch =
segment.match(/^(.*:\w+(?:\([^)]*\))?)([?+*])$/);` handling) only runs for
segments with unescaped colons/parentheses so escaped `\(` and `\:` remain
literal and are not parsed as capture groups.
Document all supported route patterns including static, named params, wildcards, regex constraints, unnamed groups, and modifiers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 103: Replace each unlabeled fenced code block in README.md (the example
blocks showing routes and arrow outputs) with a labeled block by changing the
opening ``` to ```text so Markdown lint MD040 is satisfied; specifically update
the unlabeled fences wrapping the route examples (the blocks containing lines
like "/path/to/resource", "/users/:name → /users/foo → { name:
\"foo\" }", "/path/** → /path/foo/bar → {}", the regex and
optional/greedy param examples, and all similar example fences) to use ```text.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/operations/add.ts (1)
46-57:⚠️ Potential issue | 🔴 Critical
+/*expansion is still incorrect for constrained or non-terminal routes.Line 129 rewrites
+/*into**:${name}, which strips inline constraints (e.g.:id(\d+)+). Also, wildcard matching is terminal (Line 56break), so suffixes added in Line 129 are never enforced (/a/:id+/taileffectively degrades to/a/**:id).💡 Safe short-term guard (prevents silent misrouting until full support exists)
function _expandModifiers(segments: string[]): string[] | undefined { for (let i = 0; i < segments.length; i++) { const m = segments[i].match(/^(.*:\w+(?:\([^)]*\))?)([?+*])$/); if (!m) continue; const pre = segments.slice(0, i); const suf = segments.slice(i + 1); if (m[2] === "?") { return [ "/" + pre.concat(m[1]).concat(suf).join("/"), "/" + pre.concat(suf).join("/"), ]; } + const hasInlineConstraint = /\([^)]*\)/.test(m[1]); + const hasTrailingSegments = suf.length > 0; + if (hasInlineConstraint || hasTrailingSegments) { + throw new Error(`Unsupported modifier pattern: "${segments[i]}"`); + } const name = m[1].match(/:(\w+)/)?.[1] || "_"; - const wc = "/" + [...pre, `**:${name}`, ...suf].join("/"); - const without = "/" + [...pre, ...suf].join("/"); + const wc = "/" + [...pre, `**:${name}`].join("/"); + const without = "/" + pre.join("/"); return m[2] === "+" ? [wc] : [wc, without]; } }Also applies to: 128-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/operations/add.ts` around lines 46 - 57, The current handling of "**" rewrites "+/*" into a terminal wildcard that strips inline constraints and stops processing suffixes; change the logic in src/operations/add.ts where segment.startsWith("**") is handled so that you (1) preserve the entire inline parameter token (everything after ":" including constraint regex and quantifier) when pushing into paramsMap instead of truncating to just the name, and (2) do not unconditionally break out of the segment-processing loop so that non-terminal wildcard expansions can still allow subsequent segments to be enforced. Update the block that sets node.wildcard and the paramsMap push (referencing node.wildcard, paramsMap, and the segment.startsWith("**") branch) to keep the original parameter string and to continue processing remaining segments when the wildcard is non-terminal.
🧹 Nitpick comments (1)
test/router.test.ts (1)
520-582: Add tests for constrained/non-terminal+/*to lock behavior.Current modifier tests cover terminal unconstrained
:name+and:name*, but not cases like/:id(\d+)+or/a/:id+/tail. Those are high-risk paths for expansion logic regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/router.test.ts` around lines 520 - 582, Add tests to cover constrained and non-terminal +/* modifiers to prevent regressions: inside the "url pattern modifiers" describe block add testRouter cases using patterns like "/:id(\\d+)+", "/:id(\\d+)*", "/a/:id+/tail" and "/a/:id*/tail" (i.e., pass those pattern arrays into testRouter) and assert expected matches and non-matches — for constrained "+/*" ensure numeric regex-only matches (e.g., "/123/456" matches "/:id(\\d+)+", "/abc" does not), and for non-terminal variants ensure trailing segments are required/optional appropriately (e.g., "/a/1/2/tail" matches "/a/:id+/tail", "/a/tail" does not for + but does for *); put these alongside the existing tests so testRouter and the existing pattern strings (e.g., "/files/:path+", "/files/:path*") are referenced for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/operations/add.ts`:
- Around line 46-57: The current handling of "**" rewrites "+/*" into a terminal
wildcard that strips inline constraints and stops processing suffixes; change
the logic in src/operations/add.ts where segment.startsWith("**") is handled so
that you (1) preserve the entire inline parameter token (everything after ":"
including constraint regex and quantifier) when pushing into paramsMap instead
of truncating to just the name, and (2) do not unconditionally break out of the
segment-processing loop so that non-terminal wildcard expansions can still allow
subsequent segments to be enforced. Update the block that sets node.wildcard and
the paramsMap push (referencing node.wildcard, paramsMap, and the
segment.startsWith("**") branch) to keep the original parameter string and to
continue processing remaining segments when the wildcard is non-terminal.
---
Nitpick comments:
In `@test/router.test.ts`:
- Around line 520-582: Add tests to cover constrained and non-terminal +/*
modifiers to prevent regressions: inside the "url pattern modifiers" describe
block add testRouter cases using patterns like "/:id(\\d+)+", "/:id(\\d+)*",
"/a/:id+/tail" and "/a/:id*/tail" (i.e., pass those pattern arrays into
testRouter) and assert expected matches and non-matches — for constrained "+/*"
ensure numeric regex-only matches (e.g., "/123/456" matches "/:id(\\d+)+",
"/abc" does not), and for non-terminal variants ensure trailing segments are
required/optional appropriately (e.g., "/a/1/2/tail" matches "/a/:id+/tail",
"/a/tail" does not for + but does for *); put these alongside the existing tests
so testRouter and the existing pattern strings (e.g., "/files/:path+",
"/files/:path*") are referenced for consistency.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.mdsrc/operations/add.tssrc/regexp.tstest/regexp.test.tstest/router.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
expand {…} routes across add/remove/regexp with tests and docs
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/operations/add.ts (1)
139-143:⚠️ Potential issue | 🟠 MajorConstrained
+/*routes lose their inline regex during expansion.
_expandModifiersreduces:id(\d+)+/:id(\d+)*to**:id, so constraints are dropped and non-matching paths are accepted.💡 Proposed fix (safe behavior until constrained wildcard validation exists)
- const name = m[1].match(/:(\w+)/)?.[1] || "_"; + const base = m[1]; + const name = base.match(/:(\w+)/)?.[1] || "_"; + const hasInlinePattern = /:\w+\([^)]*\)/.test(base); + if (hasInlinePattern) { + throw new Error( + `unsupported constrained repeat modifier in radix matcher: "${segments[i]}"`, + ); + } const wc = "/" + [...pre, `**:${name}`, ...suf].join("/"); const without = "/" + [...pre, ...suf].join("/"); return m[2] === "+" ? [wc] : [wc, without];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/operations/add.ts` around lines 139 - 143, _expandModifiers currently extracts only the parameter name with m[1].match(/:(\w+)/) which drops any inline regex, so when building wc it emits `**:name` and loses constraints; update the extraction to capture the optional inline constraint (e.g. use something like m[1].match(/:(\w+)(\([^)]+\))?/) to get both name and regex) and build the wildcard segment as `**:${name}${regex || ''}` (i.e. preserve the captured `(regex)` when constructing wc in the _expandModifiers logic) so constrained modifiers keep their inline regex.
🤖 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/operations/remove.ts`:
- Around line 13-23: removeRoute currently only expands group delimiters (via
expandGroupDelimiters(path)) and can miss routes that were stored after modifier
expansion (e.g., :id?, :path+, :path*); update removeRoute to mirror addRoute by
also expanding modifier delimiters before proceeding: call the same
modifier-expansion helper used by addRoute (the function that returns expanded
modifier paths), iterate over each expanded modifier path and call
removeRoute(ctx, method, expandedModifierPath) just like you do for group
expansions, then fall back to splitPath and _remove(ctx.root, method || "",
segments, 0) when no expansions apply.
---
Duplicate comments:
In `@src/operations/add.ts`:
- Around line 139-143: _expandModifiers currently extracts only the parameter
name with m[1].match(/:(\w+)/) which drops any inline regex, so when building wc
it emits `**:name` and loses constraints; update the extraction to capture the
optional inline constraint (e.g. use something like
m[1].match(/:(\w+)(\([^)]+\))?/) to get both name and regex) and build the
wildcard segment as `**:${name}${regex || ''}` (i.e. preserve the captured
`(regex)` when constructing wc in the _expandModifiers logic) so constrained
modifiers keep their inline regex.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
AGENTS.mdREADME.mdsrc/_group-delimiters.tssrc/operations/add.tssrc/operations/remove.tssrc/regexp.tstest/bench/bundle.test.tstest/regexp.test.tstest/router.test.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
Comment on lines
+13
to
23
| const groupExpanded = expandGroupDelimiters(path); | ||
| if (groupExpanded) { | ||
| for (const expandedPath of groupExpanded) { | ||
| removeRoute(ctx, method, expandedPath); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| const segments = splitPath(path); | ||
| return _remove(ctx.root, method || "", segments, 0); | ||
| } |
There was a problem hiding this comment.
Make removeRoute mirror modifier expansion used by addRoute.
addRoute expands modifier routes (:id?, :path+, :path*) before insertion, but this function only expands {...} groups. Removing by original modifier path can silently fail.
💡 Proposed fix
export function removeRoute<T>(
ctx: RouterContext<T>,
method: string,
path: string,
): void {
const groupExpanded = expandGroupDelimiters(path);
if (groupExpanded) {
for (const expandedPath of groupExpanded) {
removeRoute(ctx, method, expandedPath);
}
return;
}
- const segments = splitPath(path);
+ const segments = splitPath(path);
+ const modifierExpanded = _expandModifiers(segments);
+ if (modifierExpanded) {
+ for (const expandedPath of modifierExpanded) {
+ removeRoute(ctx, method, expandedPath);
+ }
+ return;
+ }
return _remove(ctx.root, method || "", segments, 0);
}
+
+function _expandModifiers(segments: string[]): string[] | undefined {
+ for (let i = 0; i < segments.length; i++) {
+ const m = segments[i].match(/^(.*:\w+(?:\([^)]*\))?)([?+*])$/);
+ if (!m) continue;
+ const pre = segments.slice(0, i);
+ const suf = segments.slice(i + 1);
+ if (m[2] === "?") {
+ return [
+ "/" + pre.concat(m[1]).concat(suf).join("/"),
+ "/" + pre.concat(suf).join("/"),
+ ];
+ }
+ const name = m[1].match(/:(\w+)/)?.[1] || "_";
+ const wc = "/" + [...pre, `**:${name}`, ...suf].join("/");
+ const without = "/" + [...pre, ...suf].join("/");
+ return m[2] === "+" ? [wc] : [wc, without];
+ }
+}🧰 Tools
🪛 Biome (2.4.4)
[error] 22-22: The function should not return a value because its return type is void.
(lint/correctness/noVoidTypeReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/operations/remove.ts` around lines 13 - 23, removeRoute currently only
expands group delimiters (via expandGroupDelimiters(path)) and can miss routes
that were stored after modifier expansion (e.g., :id?, :path+, :path*); update
removeRoute to mirror addRoute by also expanding modifier delimiters before
proceeding: call the same modifier-expansion helper used by addRoute (the
function that returns expanded modifier paths), iterate over each expanded
modifier path and call removeRoute(ctx, method, expandedModifierPath) just like
you do for group expansions, then fall back to splitPath and _remove(ctx.root,
method || "", segments, 0) when no expansions apply.
Comment on lines
+4
to
+10
| const groupExpanded = expandGroupDelimiters(route); | ||
| if (groupExpanded) { | ||
| const sources = groupExpanded.map((expandedRoute) => | ||
| routeToRegExp(expandedRoute).source.slice(1, -1), | ||
| ); | ||
| return new RegExp(`^(?:${sources.join("|")})$`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
JavaScript RegExp duplicate named capture groups alternation (?<name>pattern)|(?<name>pattern)
💡 Result:
In JavaScript, duplicate named capture groups are now allowed only when they are in different alternation branches and can’t both participate in the same match—e.g. (?<name>a)|(?<name>b) is valid in engines that implemented the newer behavior. If the duplicates could be matched “at the same time” (i.e., ambiguous), it’s still a SyntaxError. [1]
Example (valid where supported)
const re = /(?<year>\d{4})-\d{2}|\d{2}-(?<year>\d{4})/;
re.exec("02-1999").groups.year; // "1999"This is the canonical “same concept, two formats” use case. [1]
Support (important)
This works only in newer runtimes (roughly Chrome/Edge 125+, Firefox 129+, Safari 17+). Older browsers throw “Duplicate capture group name”. [2]
If you need it to work everywhere
- Use distinct names and coalesce:
const re = /(?<year1>\d{4})-\d{2}|\d{2}-(?<year2>\d{4})/; const m = re.exec("02-1999"); const year = m.groups.year1 ?? m.groups.year2;
- Or transpile regex literals with Babel’s transform for duplicate named groups. [3]
Background: this started as a TC39 proposal to permit reusing names (primarily for alternation-style patterns). [4]
Sources: MDN error doc (and rule/exception) [1], Can I use support table [2], Babel plugin docs [3], TC39 proposal repo [4].
Address runtime compatibility for duplicate named capture groups in alternations.
Line 9 joins expanded regex sources with | (alternation), which can create patterns like (?<id>a)|(?<id>b) with duplicate named capture groups. While this is valid in modern JavaScript engines (Chrome 125+, Firefox 129+, Safari 17+), it will throw SyntaxError: Invalid regular expression: Duplicate capture group name in older runtimes.
If older browser support is required, either use distinct group names and coalesce them from groups, or transpile with Babel's regex duplicate-name-groups plugin. Confirm the target runtime and add appropriate handling.
🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 8-8: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^(?:${sources.join("|")})$)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
handle mid-segment * patterns in matching, removal, regexp, docs, and tests
align matcher output with urlpattern-style unnamed group keys
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add URLPattern-inspired pathname syntax to route definitions:
:id(\d+),:ext(png|jpg|gif),:version(v\d+)(\d+),(png|jpg|gif)*unnamed captures —/*.png,/file-*-*.png(including mid-segment wildcards):name?(optional),:name+(one-or-more),:name*(zero-or-more):id(\d+)?,:id(\d+)+,:id(\d+)*/users/:id(\d+)+/users/:slug){...}and{...}?(e.g./book{s}?,/blog/:id(\d+){-:title}?)Unnamed captures now use URLPattern-style numeric keys in matcher results:
_0,_1, ..."0","1", ...Applies to both unnamed regex groups and unnamed
*captures infindRoute()/findAllRoutes()/ compiled matchers.Type-level inference (
InferRouteParams) now also uses numeric keys for unnamed*params.Implementation details
:name?expands into two routes (with/without),:name+/:name*expand into wildcard (**) routes. Trailing segments after+/*are preserved.expandGroupDelimiters()runs before add/remove/regexp conversion.{...}-> inline expansion{...}?-> with/without expansions{...}+/{...}*-> regex fragment form(?:...)+/(?:...)*(single-segment only)*at segment top-level is converted to unnamed captures in route insertion androuteToRegExp()conversion.src/_segment-wildcards.tsis used by add/remove/regexp paths to keep wildcard semantics consistent.removeRoute()classifies wildcard-segment patterns as dynamic routes, so entries like/assets/*.pngremove correctly.getParamRegexp()accepts a shared unnamed counter so unnamed keys remain unique across mixed unnamed regex + wildcard captures across segments.getMatchParams) and compiler output normalize internal unnamed group keys to public numeric keys.Current limitations
{/foo/bar}+/(foo|bar)?,/(\d+)+ignoreCaseoption{ ignoreCase: true }protocol,hostname, etc./books≠/books/Test plan
*unnamed captures (/*.png,/file-*-*.png, cross-segment indexing)"0","1", ...)/path/(\d+)/(\w+)->"0","1"):name?,:name+,:name*):id(\d+)?,:id(\d+)+,:id(\d+)*)/book{s}?,/blog/:id(\d+){-:title}?,/foo{/bar}?)routeToRegExp()updated for all new patternsremoveRoute()for wildcard-segment patterns (/assets/*.png)