fix: Links with parentheses in URLs not working correctly #36948
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
|
WalkthroughUpdated URL parsing in the PEG grammar to allow closing parentheses within URL bodies and removed an inline comment. Added a test to verify parsing of links whose URLs include parentheses in query strings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant MD as Markdown Source
participant Parser as Message Parser
participant PEG as PEG Grammar
participant URL as URLBody Rule
MD->>Parser: [link](https://example.com/query?this=(is)&a=problem)
Parser->>PEG: Tokenize and parse
PEG->>URL: Match URL body characters
Note over URL: ')' allowed in URL body (updated)
URL-->>PEG: URL token with parentheses
PEG-->>Parser: Link node with href and text
Parser-->>MD: AST (Paragraph -> Link("link"))
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (1)
packages/message-parser/tests/link.test.ts (1)
45-54: Good regression test; please add a few more edge cases to prevent future regressions.To cover delimiter handling and nesting at string ends, consider adding:
@@ test.each([ [ '[link](https://example.com/query?this=(is)&a=problem)', [ paragraph([ link('https://example.com/query?this=(is)&a=problem', [ plain('link'), ]), ]), ], ], + [ + '[link](https://example.com/path_(with)_parens)', + [paragraph([link('https://example.com/path_(with)_parens', [plain('link')])])], + ], + [ + '[link](https://example.com/a(b(c)d)e?x=(y)z)', + [paragraph([link('https://example.com/a(b(c)d)e?x=(y)z', [plain('link')])])], + ], + [ + '[link](https://example.com/endswithparen())', + [paragraph([link('https://example.com/endswithparen()', [plain('link')])])], + ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/message-parser/src/grammar.pegjs(2 hunks)packages/message-parser/tests/link.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/message-parser/tests/link.test.ts (1)
packages/message-parser/src/utils.ts (2)
paragraph(27-27)plain(70-70)
🔇 Additional comments (2)
packages/message-parser/src/grammar.pegjs (2)
297-302: Fix is correct; ensure we don’t swallow the closing “)” delimiter in link.Allowing “)” in URLBody solves the truncation for URLs containing parentheses. Given References expects a literal “)” after LinkRef, please verify we don’t consume the delimiter in cases where the URL ends with a “)” (e.g., https://example.com/path_(with)_parens). Existing tests suggest backtracking keeps the delimiter intact, but adding a couple of guard tests would lock this down.
285-285: OK to remove the stale TODO here.No functional impact; LinkRef alternatives remain intact.
Proposed changes (including videos or screenshots)
This PR fixes a bug in the message parser where Markdown links containing unencoded parentheses in URLs were being incorrectly truncated. The issue occurred because the PEG grammar's
URLBodyrule included opening parentheses(in its character class but was missing the closing parenthesis), causing the parser to stop at the first closing parenthesis encountered.Fixes: #36927
Before this fix:
Would be parsed as:
https://example.com/query?this=(is(truncated)After this fix:
Is correctly parsed as:
https://example.com/query?this=(is)&a=problem(full URL preserved)Issue(s)
Fixes the issue where links with unencoded parentheses
()in URLs are not correctly converted/parsed in Markdown syntax.Steps to test or reproduce
Create a message with a Markdown link containing unencoded parentheses in the URL:
Verify that the link renders correctly and points to the full URL:
https://example.com/query?this=(is)&a=problemRun the test suite to ensure the new test passes:
Test edge cases:
[link](https://example.com/path/(section)/(subsection))[link](https://example.com/path/((nested)))[link](https://example.com/query?param=(value)&other=test)This is a minimal fix that only adds the missing
)character to the existing URLBody character class in the PEG grammar. The change is surgical and backward-compatible, addressing the specific issue without affecting other parsing behavior. The fix maintains consistency with the existing(character already present in the character class.