feat(add): allow escaping : and * using backslash#175
Conversation
|
Warning Rate limit exceeded@pi0 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughaddRoute now normalizes backslash-escaped literals before segment splitting: escaped colons are percent-encoded, escaped single/double asterisks are unescaped to literal Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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! |
: using backslash: and * using backslash
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/router.test.ts (1)
489-489: Add test coverage for routing behavior, not just insertion.The test validates that the escaped route is inserted correctly into the tree, but doesn't verify the routing behavior. Consider adding tests to confirm:
- The route matches the literal path
/test:special(with encoded colon in the request)- The route does NOT match a path like
/test/specialor treat:specialas a parameter- Edge cases like
/test%3Aspecial(pre-encoded) match correctlyAdd a dedicated test in the "Router lookup" section:
describe("escaped special characters", function () { testRouter( ["/test\\:special", "/test/:param"], (router) => expect(formatTree(router.root)).toMatchInlineSnapshot(`...`), { "/test:special": { data: { path: "/test\\:special" }, }, "/test%3Aspecial": { data: { path: "/test\\:special" }, }, "/test/value": { data: { path: "/test/:param" }, params: { param: "value" }, }, }, ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/operations/add.ts(1 hunks)test/router.test.ts(2 hunks)
🔇 Additional comments (2)
test/router.test.ts (1)
514-515: LGTM - Snapshot correctly reflects the encoding behavior.The snapshot update correctly shows:
- Line 514: Incidental formatting fix (removed extraneous quote)
- Line 515: The escaped route is stored as
/test%3Aspecial(URL-encoded) in the tree while displaying the original/test\\:specialformThis validates that the escape sequence is processed as intended: the backslash-escaped colon becomes a literal
%3Ain the route tree, preventing it from being interpreted as a parameter delimiter.src/operations/add.ts (1)
29-32: Correct regex pattern: the implementation escapes both:and*, not just:.The actual regex at line 30 is
/\\([:*])/g, which encodes both escaped colons (\:→%3A) and escaped asterisks (\*→%2A). A test case confirms the colon escape functionality (/test\:special→/test%3Aspecial).However, there are gaps in test coverage:
- Asterisk escaping (
\*) has no test case despite being included in the regex pattern- Edge cases remain untested: multiple backslashes (
\\:,\\\:), interaction with regex params like/:id:regexp\:escaped- No escape sequence behavior is documented in the README
Consider adding test cases for asterisk escaping and edge cases to ensure the broader escape handling is reliable.
Likely an incorrect or invalid review comment.
: and * using backslash:, * and / using backslash
:, * and / using backslash: and * using backslash
: and * using backslash: and * using backslash
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
94-95: Clarify escapeable characters and simplify the exampleThe TIP is great, but the wording says only
:and*are escapeable while the example also escapes/(\\/), and the combined/static\\:path\\/\\*\\*example is quite dense to parse.Consider explicitly listing all supported escapeable chars (e.g.
:,*,/) and using one or two simpler examples, for instance:
/static\\:path→ matches/static:path/files\\/download→ matches/files/download/literal\\*stars→ matches/literal*starsThis would make the feature and the required double backslashes in JS string literals easier to grasp.
test/router.test.ts (1)
489-517: Escaped-route insertion snapshot looks good; consider adding direct matching testsThe new
"/static\\:path\\/\\*\\*"entry and corresponding snapshot for/static%3Apath\\→/**correctly exercise the insertion and tree formatting for an escaped pattern.To fully lock in the intended behavior, it would help to add explicit
findRoute(and ideally compiled matcher) assertions for this case, e.g. along the lines of:const router = createRouter([ "/static\\:path\\/\\*\\*", ]); expect(findRoute(router, "GET", "/static:path/**")).toMatchObject({ data: { path: "/static\\:path\\/\\*\\*" }, }); // And possibly clarify via a second assertion whether this should or should not match: expect(findRoute(router, "GET", "/static:path/other")).toBeUndefined(); // or adjust to desired behaviorThat way, regressions in how escaped
:,*, and/are interpreted would surface even if the internal tree formatting changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/operations/add.ts(1 hunks)test/router.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/operations/add.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~94-~94: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 1625 characters long)
Context: ...hod should always be UPPERCASE. > [!TIP] > If you need to register a pattern...
(EN_EXCESSIVE_EXCLAMATION)
resolves h3js/h3#1266
:and*in patterns has special meaning. It is possible to register routes url encoded like/some%3A3to match/some:3but it is less intuative.This PR allows using backslash escape (like regex) limited to register patterns like
/some\\:3Summary by CodeRabbit
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.