Conversation
…s in route matching
|
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:
📝 WalkthroughWalkthroughReplaces per-segment counts with positional bitmask scoring, enqueues wildcard candidates onto the DFS stack (reverse order) with pruning against a discovered best match, refines end-of-path acceptance for index/wildcard, and consolidates wildcard param-parsing to avoid redundant or premature evaluations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit e942d11
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 22 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/router-core/src/new-process-route-tree.ts`:
- Around line 1350-1355: The numeric bitmask operations lose precision for deep
paths; update shiftScore and isPerfectStaticMatch (and any comparisons in
isFrameMoreSpecific) to use BigInt for bit shifting and masks instead of Number
so masks remain exact beyond 53 bits: change shiftScore to perform BigInt shifts
(e.g., 1n << BigInt(amount) or use multiplication with BigInt), ensure
statics/dynamics/optionals are stored/compared as BigInt, and update
isPerfectStaticMatch to compute 2**(partsLength-1)-1 using BigInt (or bit-shift)
so route specificity comparisons remain correct for long paths. Ensure callers
that pass numbers coerce to BigInt consistently and maintain interop where
necessary.
🪄 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: bd54ac60-b009-4bf4-8b31-e14a78b2ed67
📒 Files selected for processing (2)
packages/router-core/src/new-process-route-tree.tspackages/router-core/tests/new-process-route-tree.test.ts
| function shiftScore(score: number, amount = 1): number { | ||
| return score * 2 ** amount | ||
| } | ||
|
|
||
| function isPerfectStaticMatch(statics: number, partsLength: number): boolean { | ||
| return statics === 2 ** (partsLength - 1) - 1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/router-core/src && head -c 200000 new-process-route-tree.ts | tail -c 100000 | grep -n "function shiftScore\|function isPerfectStaticMatch\|MatchStackFrame\|statics.*number\|dynamics.*number" -A 5 -B 2Repository: TanStack/router
Length of output: 2204
🏁 Script executed:
# First, check the exact lines mentioned (1350-1355)
sed -n '1345,1360p' packages/router-core/src/new-process-route-tree.ts
# Then check the MatchStackFrame definition (1026-1030)
sed -n '1020,1035p' packages/router-core/src/new-process-route-tree.ts
# Check the isFrameMoreSpecific function to understand how scores are used
rg "isFrameMoreSpecific" packages/router-core/src/new-process-route-tree.ts -A 10 -B 2Repository: TanStack/router
Length of output: 2561
🏁 Script executed:
# Search for any depth limits or safeguards
rg "MAX.*DEPTH|MAX.*SEGMENT|53|Number.MAX_SAFE" packages/router-core/src/new-process-route-tree.ts -B 2 -A 2
# Check if there are tests for deep routes
find . -path "*/test*" -name "*.test.*" -o -name "*.spec.*" | head -20 | xargs grep -l "deep\|depth.*segment\|53.*segment" 2>/dev/null | head -5
# Check for any comments about the bitmasking approach and limitations
rg "BigInt|bitmask|precision|safe.*integer" packages/router-core/src/new-process-route-tree.ts -B 1 -A 1Repository: TanStack/router
Length of output: 658
🏁 Script executed:
# Check if there are practical limits preventing deep routes
rg "partsLength|\.length.*parts" packages/router-core/src/new-process-route-tree.ts | head -20
# Look for any validation or limits on route depth
rg "throw.*depth|throw.*segment|validate.*path.*length" packages/router-core/src/new-process-route-tree.ts -B 2 -A 2
# Check how shiftScore is actually used - see the caller context
rg "shiftScore\(" packages/router-core/src/new-process-route-tree.ts -B 3 -A 3Repository: TanStack/router
Length of output: 1934
Bitmask precision loss breaks route specificity comparison on deep paths.
Lines 1350-1355 use JavaScript number bitmasks to track route specificity. The shiftScore() helper multiplies scores by powers of two for each consumed segment, and isFrameMoreSpecific() relies on direct numeric comparison of these masks. Once a path exceeds ~53 segments, statics, dynamics, and optionals lose precision beyond JavaScript's Number.MAX_SAFE_INTEGER, causing incorrect route ordering and incorrect results from isPerfectStaticMatch(). While the codebase acknowledges this risk for the skipped field (with a comment suggesting BigInt as fallback), the positional score fields face the same vulnerability and require the same mitigation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/router-core/src/new-process-route-tree.ts` around lines 1350 - 1355,
The numeric bitmask operations lose precision for deep paths; update shiftScore
and isPerfectStaticMatch (and any comparisons in isFrameMoreSpecific) to use
BigInt for bit shifting and masks instead of Number so masks remain exact beyond
53 bits: change shiftScore to perform BigInt shifts (e.g., 1n << BigInt(amount)
or use multiplication with BigInt), ensure statics/dynamics/optionals are
stored/compared as BigInt, and update isPerfectStaticMatch to compute
2**(partsLength-1)-1 using BigInt (or bit-shift) so route specificity
comparisons remain correct for long paths. Ensure callers that pass numbers
coerce to BigInt consistently and maintain interop where necessary.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7396793323
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| function shiftScore(score: number, amount = 1): number { | ||
| return score * 2 ** amount | ||
| } |
There was a problem hiding this comment.
Using number bitmasks for statics/dynamics/optionals means scores stop being exact once a match consumes ~54 path segments (values exceed Number.MAX_SAFE_INTEGER). In that range, distinct specificity patterns can collapse to the same numeric value (for example 2**53 and 2**53 + 1), so isFrameMoreSpecific can pick the wrong route. This is a functional regression from the previous count-based scoring and affects very deep route trees/URLs.
Useful? React with 👍 / 👎.
Merging this PR will not alter performance
Comparing 
Footnotes
|
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/router-core/src/new-process-route-tree.ts (1)
1403-1407: Boolean-to-number coercion is valid but unusual.The expression
(next.node.kind === SEGMENT_TYPE_INDEX) > (prev.node.kind === SEGMENT_TYPE_INDEX)relies on boolean-to-number coercion (true→ 1,false→ 0). This is correct and likely intentional for performance in this hot path, but a brief inline comment would help future readers understand the intent.📝 Optional: Add clarifying comment
((next.node.kind === SEGMENT_TYPE_INDEX) > - (prev.node.kind === SEGMENT_TYPE_INDEX) || + (prev.node.kind === SEGMENT_TYPE_INDEX) || // true > false, INDEX wins over non-INDEX🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/new-process-route-tree.ts` around lines 1403 - 1407, The comparison using numeric coercion in the conditional—specifically the expression "(next.node.kind === SEGMENT_TYPE_INDEX) > (prev.node.kind === SEGMENT_TYPE_INDEX) || ((next.node.kind === SEGMENT_TYPE_INDEX) === (prev.node.kind === SEGMENT_TYPE_INDEX) && next.depth > prev.depth)"—relies on boolean-to-number coercion (true→1, false→0) to rank index segments and is intentional; add a brief inline comment next to that expression (referencing next.node.kind, prev.node.kind and SEGMENT_TYPE_INDEX) stating that the boolean comparison is purposely used as numeric ordering for performance/clarity and should not be converted to a different form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/router-core/src/new-process-route-tree.ts`:
- Around line 1403-1407: The comparison using numeric coercion in the
conditional—specifically the expression "(next.node.kind === SEGMENT_TYPE_INDEX)
> (prev.node.kind === SEGMENT_TYPE_INDEX) || ((next.node.kind ===
SEGMENT_TYPE_INDEX) === (prev.node.kind === SEGMENT_TYPE_INDEX) && next.depth >
prev.depth)"—relies on boolean-to-number coercion (true→1, false→0) to rank
index segments and is intentional; add a brief inline comment next to that
expression (referencing next.node.kind, prev.node.kind and SEGMENT_TYPE_INDEX)
stating that the boolean comparison is purposely used as numeric ordering for
performance/clarity and should not be converted to a different form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a09b6e92-0193-4682-93fd-b9389512c8ea
📒 Files selected for processing (1)
packages/router-core/src/new-process-route-tree.ts
Sheraff
deleted the
fix-router-core-route-matching-wildcard-nodes-respect-priority
branch
1 participant
Summary by CodeRabbit
Bug Fixes
Tests
Chores