chore(message-parser): Fix incomplete ASTNode union type#39212
chore(message-parser): Fix incomplete ASTNode union type#39212Harshit2405-2004 wants to merge 9 commits intoRocketChat:developfrom
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 |
🦋 Changeset detectedLatest commit: 276176a The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (2)
WalkthroughThis PR refactors the ASTNode type to ensure exhaustiveness by mapping it to the Types interface, updates heading grammar rules to properly handle inline content including emojis, extends heading test coverage for emoji parsing, and includes a separate data export crash fix for the Rocket.Chat meteor package. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/message-parser/src/definitions.ts (1)
75-79:⚠️ Potential issue | 🟠 MajorType mismatch:
Heading.valueshould beInlines[], notPlain[].The test cases explicitly include emojis in heading values (e.g.,
heading([emoji('newspaper'), plain(' Headline')], 1)), but the type definition restricts it toPlain[]. Similar container types in the same file—ListItem(line 18) andTask(line 29)—correctly useInlines[]for their value fields. TheEmojitype is part of theInlinesunion and must be supported in heading content.🛠️ Proposed fix
export type Heading = { type: 'HEADING'; level: 1 | 2 | 3 | 4; - value: Plain[]; + value: Inlines[]; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/src/definitions.ts` around lines 75 - 79, Heading.value is incorrectly typed as Plain[] but must allow emojis and other inline nodes; change the Heading type's value from Plain[] to Inlines[] (matching ListItem and Task) so heading content can include Emoji and other Inlines. Update the Heading type definition (symbol: Heading) to use Inlines[] instead of Plain[] and run type checks/tests to confirm compatibility with existing heading test cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/message-parser/src/definitions.ts`:
- Around line 75-79: Heading.value is incorrectly typed as Plain[] but must
allow emojis and other inline nodes; change the Heading type's value from
Plain[] to Inlines[] (matching ListItem and Task) so heading content can include
Emoji and other Inlines. Update the Heading type definition (symbol: Heading) to
use Inlines[] instead of Plain[] and run type checks/tests to confirm
compatibility with existing heading test cases.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/fix-astnode-completeness.md.changeset/fix-heading-emoji-parsing.mdpackages/message-parser/src/definitions.tspackages/message-parser/src/grammar.pegjspackages/message-parser/tests/heading.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/definitions.tspackages/message-parser/tests/heading.test.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 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/definitions.tspackages/message-parser/tests/heading.test.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/definitions.tspackages/message-parser/tests/heading.test.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
packages/message-parser/tests/heading.test.ts
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
.changeset/fix-heading-emoji-parsing.md
🧬 Code graph analysis (1)
packages/message-parser/tests/heading.test.ts (2)
packages/message-parser/src/utils.ts (3)
heading(36-40)emoji(143-147)plain(65-65)packages/message-parser/tests/helpers.ts (3)
heading(14-18)emoji(71-75)plain(34-34)
🔇 Additional comments (6)
.changeset/fix-heading-emoji-parsing.md (1)
1-5: LGTM!Changeset correctly documents the patch-level fix for emoji and inline element parsing within headings.
packages/message-parser/src/definitions.ts (1)
202-208: LGTM! Clean approach to ensure ASTNode exhaustiveness.Using
Types[keyof Types]guarantees the union automatically stays in sync with theTypesinterface, eliminating manual drift.packages/message-parser/src/grammar.pegjs (1)
147-152: LGTM!The grammar changes correctly pass
Inlineresults directly toheading(), consistent with how other constructs likeTaskandOrderedListItemhandle inline content. This enables proper parsing of emojis and other inline elements within headings.packages/message-parser/tests/heading.test.ts (2)
2-2: LGTM!Import updated correctly to include
emojihelper for the new test cases.
47-50: LGTM!Good test coverage for emoji parsing in headings: emoji at start (level 1), at end (level 2), and multiple emojis with interleaved text (level 3).
.changeset/fix-astnode-completeness.md (1)
1-5: LGTM!Changeset accurately documents the
ASTNodetype exhaustiveness fix.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".changeset/fix-heading-emoji-parsing.md">
<violation number="1" location=".changeset/fix-heading-emoji-parsing.md:5">
P1: Changeset description incorrectly describes a runtime parser bug fix (nested emojis/markdown in headings) when the actual change is purely TypeScript type definitions (`ASTNode = Types[keyof Types]`). This will publish misleading release notes implying behavioral changes that don't exist.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
Thanks for catching that! The changeset was incorrectly copied over from a previous branch. I've updated .changeset/fix-astnode-exhaustiveness.md to accurately reflect the ASTNode union exhaustiveness fix. |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".changeset/fix-data-export-crash.md">
<violation number="1" location=".changeset/fix-data-export-crash.md:1">
P2: Changeset content does not match the PR's described scope. The PR title indicates a message-parser ASTNode type fix, but this changeset declares a patch for @rocket.chat/meteor describing a sendViaEmail data-export crash. This mismatch would cause incorrect package versioning and changelog entries.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/message-parser/src/grammar.pegjs">
<violation number="1" location="packages/message-parser/src/grammar.pegjs:151">
P2: Missing EndOfLine consumption in HeadingChunk will cause spurious empty paragraph nodes after headings</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
|
Hey @Harshit2405-2004, thank you for your contribution! 🙏 Your changes have been consolidated into #39853, where we merged all message-parser PRs together to make it easier to test and validate everything as a single unit. That PR has already been merged into |
2 participants
Description
Currently, packages/message-parser/src/definitions.ts manually maintains an ASTNode union type that is decoupled from the exhaustive Types interface. Several valid AST node types produced by the message parser are missing from the union:
This PR bridges this gap by directly mapping the ASTNode union to the exhaustive Types interface (ASTNode = Types[keyof Types]). This guarantees zero-drift type safety when writing type guards like isNodeOfType(...) across the repository.
Checklist
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes