feat: hand written parser - #40717
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 |
|
|
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:
WalkthroughThe PEG grammar entry point is replaced by a scanner-based parser. Character and emoticon helpers support ordered inline matching, while block parsing adds Markdown-like constructs including links, formatting, lists, spoilers, KaTeX, task items, and tables. PEG loading and build configuration are removed. ChangesMessage parser implementation
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant parse
participant Scanner
participant Root
Caller->>parse: provide message input and options
parse->>Scanner: scan characters and delimiters
Scanner-->>parse: positions and matched slices
parse->>Root: append inline and block nodes
Root-->>Caller: return parsed document
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
| if (scanner.matches('mailto:')) { | ||
| scanner.advance(7); | ||
| } |
There was a problem hiding this comment.
Looks like a magic number slipped through here! Could you please fix this one and double-check the rest of the code for any others?
37e61da to
c673fba
Compare
|
Hey @amitkumarashutosh , I'm getting a build failure in |
c673fba to
65fd782
Compare
@ahmed-n-abdeltwab, I resolved the unused variable ( |
Thanks. It's always a good idea to double-check everything before pushing a commit. It's okay to make these kinds of mistakes; I do it all the time and they're hard to catch. That's why I usually run the build to ensure everything compiles properly, and then run the tests and lint checks to make sure I don't mess up the code or the project. It's like evolving a third eye that's looking out for those mistakes! :) |
coderabbitai
Bot
added
the
type: feature
|
Nice work 🥳, @amitkumarashutosh! The cubic review is now underway. Please review and address its feedback once it finishes |
![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 (2)
packages/message-parser/src/chars.ts (1)
44-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove implementation comments from this TypeScript file.
The inline annotations, section header, and ordering note conflict with the repository rule to avoid implementation comments. Move important rationale to documentation or tests instead.
As per coding guidelines,
**/*.{ts,tsx,js}files should avoid code comments in the implementation.Also applies to: 55-55, 84-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/message-parser/src/chars.ts` around lines 44 - 47, Remove the inline implementation comments from the character-range conditions in chars.ts, including the annotations at the referenced locations and the section header or ordering note. Preserve the existing conditions and ordering; move only rationale that is necessary for maintainers into appropriate documentation or tests.Source: Coding guidelines
packages/message-parser/src/parser.ts (1)
85-98: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider trimming the section-banner and step-by-step comments.
Banners (lines 85, 98, 493, 1267, 1676) and running narration (
// consume '@',// consume ')') restate the code. The genuinely useful ones are the grammar-rationale comments (e.g. lines 552, 577, 1005, 1467) — worth keeping those and dropping the rest.As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/message-parser/src/parser.ts` around lines 85 - 98, Remove non-actionable section banners and step-by-step narration comments in parser.ts, including banners near the constants/helpers and other cited sections and comments such as “consume '@'” or “consume ')'”. Preserve grammar-rationale comments that explain non-obvious parsing decisions, including those around the cited grammar logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/message-parser/src/parser.ts`:
- Around line 1751-1755: Initialize align in the table-column alignment logic
before the conditional chain with the PEG grammar’s default for a colon-less
column, then retain the existing left, right, and center overrides. Ensure
aligns.push receives a valid TableCellAlignment for plain `---` columns.
- Around line 681-684: Update the trailing-domain cleanup loop in the parser to
remove both `.` and `-` characters, matching the documented behavior for domains
such as `joe.com-`; preserve the existing domain-start boundary.
- Around line 167-170: Make the parser re-entrant by removing the module-level
mutable guards skipBold, skipItalic, skipStrike, and skipReferences and storing
them in per-parse state passed through the relevant parsing functions. Update
each guard site and nested parseInline/scanner call to read and mutate that
state, ensuring guard values are restored with try/finally when temporarily
changed so exceptions cannot leak state across parse() calls.
- Around line 1501-1520: Update tryOrderedList to track the current item’s start
separately from the list start, and when a continuation item is malformed (such
as missing the required dot or space), backtrack only to that item start and
break rather than returning null. Preserve already-parsed ordered items while
leaving the malformed line for subsequent parsing, matching the unordered-list
behavior.
---
Nitpick comments:
In `@packages/message-parser/src/chars.ts`:
- Around line 44-47: Remove the inline implementation comments from the
character-range conditions in chars.ts, including the annotations at the
referenced locations and the section header or ordering note. Preserve the
existing conditions and ordering; move only rationale that is necessary for
maintainers into appropriate documentation or tests.
In `@packages/message-parser/src/parser.ts`:
- Around line 85-98: Remove non-actionable section banners and step-by-step
narration comments in parser.ts, including banners near the constants/helpers
and other cited sections and comments such as “consume '@'” or “consume ')'”.
Preserve grammar-rationale comments that explain non-obvious parsing decisions,
including those around the cited grammar logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57d36c1e-1e66-4f23-9d84-f8cbaeeedb01
📒 Files selected for processing (4)
packages/message-parser/src/chars.tspackages/message-parser/src/index.tspackages/message-parser/src/parser.tspackages/message-parser/src/scanner.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
⚠️ CI failures not shown inline (1)
GitHub Check: Dionisio QA: Some checks did not pass
Conclusion: failure
**Conclusion:** failure
### Steps
- ✅ **No merge conflicts**
- ❌ **QA assured** — This PR is missing the 'stat: QA assured' label
- ✅ **Mergeable**
- ❌ **Has milestone or project** — This PR is missing the required milestone or project
- ✅ **Valid PR title**
- ✅ **Correct target version**
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/message-parser/src/scanner.tspackages/message-parser/src/index.tspackages/message-parser/src/chars.tspackages/message-parser/src/parser.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/message-parser/src/scanner.tspackages/message-parser/src/index.tspackages/message-parser/src/chars.tspackages/message-parser/src/parser.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/message-parser/src/scanner.tspackages/message-parser/src/index.tspackages/message-parser/src/chars.tspackages/message-parser/src/parser.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
packages/message-parser/src/scanner.tspackages/message-parser/src/index.tspackages/message-parser/src/chars.tspackages/message-parser/src/parser.ts
🪛 ast-grep (0.45.0)
packages/message-parser/src/parser.ts
[warning] 90-92: 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(
^(\\d{4})-(\\d{2})-(\\d{2})T(\\d{2}):(\\d{2}):(\\d{2})\\.(\\d{3})${OPTIONAL_TIMEZONE_OFFSET}$,
)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 93-93: 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(^(\\d{4})-(\\d{2})-(\\d{2})T(\\d{2}):(\\d{2}):(\\d{2})${OPTIONAL_TIMEZONE_OFFSET}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 94-94: 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(^(\\d{2}):(\\d{2}):(\\d{2})${OPTIONAL_TIMEZONE_OFFSET}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
[warning] 95-95: 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(^(\\d{2}):(\\d{2})${OPTIONAL_TIMEZONE_OFFSET}$)
Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🪛 OpenGrep (1.26.0)
packages/message-parser/src/parser.ts
[ERROR] 764-764: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 775-775: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 785-785: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 787-787: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
[ERROR] 1179-1179: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (5)
packages/message-parser/src/chars.ts (1)
1-23: LGTM!Also applies to: 25-43, 48-53, 56-82
packages/message-parser/src/scanner.ts (1)
1-45: LGTM!packages/message-parser/src/parser.ts (2)
1173-1177: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the
32-char lookahead window.A magic number slipped in here, matching the earlier magic-number feedback on this PR. Extract it as a module constant (e.g.
MAX_EMOJI_SEQUENCE_LENGTH) alongside the other constants at lines 87-96.
88-88: 🩺 Stability & AvailabilityAlign the package baseline with the new regex support.
packages/message-parser/tsconfig.jsontargetses2020, butUNICODE_EMOJIuses the ES2024 RegExpvflag. Since this package is exported fromdist/messageParser.js, add browser/Node baseline documentation for Chrome 112+, Firefox 116+, Safari 17+, and Node 20+, or guard construction with an ASCII-safe fallback.packages/message-parser/src/index.ts (1)
2-2: LGTM!Also applies to: 20-20
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40717 +/- ##
===========================================
- Coverage 68.66% 68.14% -0.53%
===========================================
Files 4166 4168 +2
Lines 159310 151172 -8138
Branches 28264 26119 -2145
===========================================
- Hits 109398 103016 -6382
+ Misses 44742 42988 -1754
+ Partials 5170 5168 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ahmed-n-abdeltwab
left a comment
ahmed-n-abdeltwab
left a comment
There was a problem hiding this comment.
Please remove the old parser and run the tests
Just the grammar file, or the peggy loader folder as well? |
Anything we aren't using anymore should be removed, your changes should replace it and work |
coderabbitai
Bot
added
type: chore
and removed
type: feature
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
c8822be to
122d5dc
Compare
coderabbitai
Bot
added
the
type: feature
122d5dc to
62d2249
Compare
ahmed-n-abdeltwab
left a comment
ahmed-n-abdeltwab
left a comment
There was a problem hiding this comment.
Hey Amit, try removing anything related to PegJS as well. I tried it myself and the project runs perfectly fine.
Please remove these directories:
packages/peggy-loaderpackages/message-parser/loaders
Also update these files remove Peggy from them:
packages/message-parser/jest.config.tspackages/message-parser/package.jsonpackages/message-parser/webpack.config.ts
After making the changes, try running, building, and testing the project. Everything should work normally, because we no longer need these
|
Done. The build, tests, and all checks are passing. |
|
Hey @amitkumarashutosh, there is a conflict with the develop branch. Could you update the PR? 😁 |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
c618ab3 to
040bc19
Compare
2 participants
Handwritten Parser Foundation
This PR introduces a handwritten TypeScript parser implementation to replace the PeggyJS-generated parser, building it up incrementally while maintaining AST compatibility with the existing grammar.
What's Added
parser.ts— core handwritten parser with block dispatch and inline parsing loopscanner.ts— cursor abstraction withposition()/backtrack()for backtrackingchars.ts— character classification helpers (isAlpha, isDigit, isPlainChar, etc.) and the emoticon tableutils.tsfor node generation, and types fromdefinitions.tsindex.tsParsing Features Implemented
paragraphnodelineBreaknodes; trailing newlines are swallowed\*,\_,\~,\`,\#,\.codewith no markup parsing inside*text*and**text**with whitespace-only and triple-asterisk edge cases_text_and__text__with word-boundary guards to protectsnake_case~text~and~~text~~with whitespace-only and triple-tilde edge casesskipBold,skipItalic,skipStrike) mirroring PeggyJS skip flags#through####prefix with required space, inline content inside```with optional language tag, raw content (no markup inside)@usernamesupporting special characters like.-_:@in names#channelwith word-boundary guard>prefixed lines with inline markup support inside||text||inline and multi-line||\n...\n||block support[title](url)and<url|title>-/*and1.markers with inline content per item; asterisk bullets respect the grammar's trailing-*guard (so* *,* Hello*stay inline)tldts:emoji:parsing, plus big-emoji handling for shortcode-only messageslocal@domain→mailto:link, with TLD validation andmailto:prefix parts+number→tel:link viaphoneChecker, with prefix/grouping variants<t:...>supporting Unixtime, ISO-8601 (with/without milliseconds and timezone), relativeHH:MM[:SS]times, and thet/T/d/D/f/F/Rformat specifiers:),:D, etc. via a symbol→shortcode table with longest-match and word-boundary dispatch, integrated into big-emoji detectioncolor:#rgb/rgba/rrggbb/rrggbbaawhen thecolorsoption is enabled😀,❤️) including ZWJ sequences, skin-tone modifiers, and variation selectors, both inline and in big-emoji detection---(three or more dashes);***/___stay as emphasis- [ ]/- [x]checklist lines grouped into onetasksnodeTest Progress
Benchmarks
Throughput: Hand-written parser vs. PeggyJS-generated parser (ops/sec)
Plain Text
Emphasis / Formatting
URLs & Links
Emoji
Mentions
Code
Structured Blocks
KaTeX (Math)
Adversarial / Stress
Real-World Messages
Timestamps
Goal
Notes
Summary by CodeRabbit
New Features
Refactor