Conversation
- check `params.length > 0` instead of using a `Set` to deduplicate static nodes - prevent deopts from accessing `s` out of bound for some cases in compiler output - use `.concat` instead of spread syntax for faster `params` array cloning and disable the lint rule that prefer spread
📝 WalkthroughWalkthroughThe compiler's route-matching emission is refactored to use on-the-fly conditional branching (currentIdx/hasIf) instead of static node collections, adjusts length semantics from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![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 (1)
test/.snapshot/compiled-jit.mjs (1)
24-26: Minor:s[3]accessed whenl === 3for the optional*param.When
l === 3,s[3]isundefined(OOB). JS handles this gracefully (returnsundefined), and the route correctly matches with the param absent. However, this OOB access pattern can still cause V8 to deoptimize the function by switching to a slower element access mode.If the PR's goal of preventing deopt from OOB access extends to this case, the compiler could emit a conditional:
_0: l > 3 ? s[3] : undefinedOtherwise, this is an acceptable trade-off for simplicity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/.snapshot/compiled-jit.mjs` around lines 24 - 26, The snippet emits an out-of-bounds array access for the optional "*" param by using s[3] when l === 3; change the emitter that generates the branch producing { data: $7, params: { _0: s[3] } } to guard the access and emit a conditional expression (e.g., _0: l > 3 ? s[3] : undefined) so the code reads _0 safely when l can be 3; update the generation logic that composes this branch (look for the branch that checks l === 4 || l === 3 and returns params._0) to produce the guarded form.
🤖 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/compiler.ts`:
- Around line 88-97: The bug is that hasIf is set unconditionally which causes
stray "else if" when a prior static node had no methods; change the logic in the
loop over ctx.router.static so hasIf is only set to true when you actually emit
code for that node (i.e., inside the if(node?.methods) block after appending the
compiled code from compileMethodMatch(ctx, node.methods, [], -1)); ensure nodes
without methods do not flip hasIf so subsequent emitted branches generate a
correct first "if" rather than "else if".
---
Nitpick comments:
In `@test/.snapshot/compiled-jit.mjs`:
- Around line 24-26: The snippet emits an out-of-bounds array access for the
optional "*" param by using s[3] when l === 3; change the emitter that generates
the branch producing { data: $7, params: { _0: s[3] } } to guard the access and
emit a conditional expression (e.g., _0: l > 3 ? s[3] : undefined) so the code
reads _0 safely when l can be 3; update the generation logic that composes this
branch (look for the branch that checks l === 4 || l === 3 and returns
params._0) to produce the guarded form.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/compiler.ts (1)
191-202: Theparams.length > 0guard intentionally prevents double-emission of fully-static routes.Static routes are handled in two places: first in the
ctx.router.staticlookup (lines 88–97), then skipped during tree traversal via this guard. This is not an implicit invariant that could be violated—it's the intended separation of concerns. Tree nodes with methods can be reached withparams.length === 0during tree traversal (for fully-static paths), and the guard ensures they're emitted only once via the static lookup.A brief comment here would help future maintainers understand that the guard is a deliberate deduplication mechanism, not a safety check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/compiler.ts` around lines 191 - 202, Add a short clarifying comment above the params.length > 0 guard to state that this check is intentional deduplication: fully-static routes are emitted via the ctx.router.static lookup and should not be re-emitted during tree traversal, so nodes with methods may legitimately be reachable with params.length === 0 and must be skipped here; reference the compileMethodMatch call and the ctx.router.static lookup to make the separation of concerns explicit for future maintainers.
🤖 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/compiler.ts`:
- Around line 88-97: The prior bug was that hasIf was being set unconditionally
causing incorrect else-if generation; ensure hasIf is assigned only when a node
has methods by keeping hasIf = true inside the if (node?.methods) block while
iterating ctx.router.static keys and using compileMethodMatch(ctx, node.methods,
[], -1) for the generated branch for each key (use key.replace(/\/$/, "") ||
"/") for p comparison); verify the code under the for loop updates hasIf only
when node?.methods is truthy.
---
Nitpick comments:
In `@src/compiler.ts`:
- Around line 191-202: Add a short clarifying comment above the params.length >
0 guard to state that this check is intentional deduplication: fully-static
routes are emitted via the ctx.router.static lookup and should not be re-emitted
during tree traversal, so nodes with methods may legitimately be reachable with
params.length === 0 and must be skipped here; reference the compileMethodMatch
call and the ctx.router.static lookup to make the separation of concerns
explicit for future maintainers.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
there are two other cases that can cause deopts, I will try to fix later @pi0
|
2 participants
params.length > 0instead of using aSetto deduplicate static nodes.sout of bound for some cases in compiler output..concatinstead of spread syntax for fasterparamsarray cloning and disable the lint rule that prefer spread.resolves #176
Summary by CodeRabbit
Chores
Refactor
Tests