test(message-parser): implement property-based fuzz testing via fast-check#39215
Open
Harshit2405-2004 wants to merge 15 commits intoRocketChat:developfrom
Open
test(message-parser): implement property-based fuzz testing via fast-check#39215Harshit2405-2004 wants to merge 15 commits intoRocketChat:developfrom
Harshit2405-2004 wants to merge 15 commits intoRocketChat:developfrom
Conversation
…ent newline absorptions
… to prevent backtracking overflows
Contributor
|
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: d0bab64 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 |
Contributor
|
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:
WalkthroughAdds property-based fuzz testing (fast-check) and a changeset, plus a grammar change: Heading now consumes one-or-more 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 9 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="packages/message-parser/tests/fuzz.test.ts">
<violation number="1" location="packages/message-parser/tests/fuzz.test.ts:16">
P2: Fuzz tests swallow all parse errors, so unexpected regressions or crashes will still pass; only discard known syntax errors or rethrow unexpected exceptions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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 | 🔴 CriticalFix type definition:
Heading.valueshould accept inline formatting, not just plain text.The grammar parses
HeadingChunkasInline, which can containEmoji,Bold,Italic, and other formatting elements per theInlinesunion. However,Heading.valueis currently typed asPlain[], which excludes these types. The test cases confirm headings can contain emoji alongside plain text (lines 47-49 ofheading.test.ts), revealing a type inconsistency. UpdateHeading.valueto accept the full range of inline elements that the parser produces.🤖 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, The Heading type's value currently restricts to Plain[] but the parser produces Inline/Inlines (e.g., HeadingChunk → Inline which can be Emoji, Bold, Italic, etc.), so update the Heading.type definition to allow the full inline union: replace value: Plain[] with value: Inline[] (or the project's Inlines/Inline union type) so headings can contain emoji and other inline formatting; locate the Heading type in definitions.ts and change its value field to reference the existing Inline/Inlines type used elsewhere.
🧹 Nitpick comments (1)
packages/message-parser/tests/fuzz.test.ts (1)
12-14: Consider using proper typing instead ofany.Using
anybypasses TypeScript's type checking. SinceASTNodeis now properly typed viaTypes[keyof Types], you can import and use it:Proposed typing improvement
+import type { ASTNode } from '../src'; import fc from 'fast-check'; import { parse } from '../src';Then replace
node: anywithnode: ASTNode:- ast.forEach((node: any) => { + ast.forEach((node) => {The type will be inferred from the
parsereturn type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/tests/fuzz.test.ts` around lines 12 - 14, Replace the use of `any` in the fuzz test by importing and using the proper AST type (e.g., `ASTNode` from your Types index) and annotate the iterator variable in `ast.forEach((node: any) => ...)` as `node: ASTNode`; ensure the import references `Types[keyof Types]` or the exported `ASTNode` type and that the `parse` return type is preserved so TypeScript can infer node properties like `type`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/fix-astnode-completeness.md:
- Around line 1-5: This changeset duplicates the same fix already described in
`.changeset/fix-astnode-exhaustiveness.md`; consolidate by keeping a single
changeset that explains making the ASTNode union exhaustive via mapping to the
Types interface (referencing ASTNode and Types), remove the duplicate file
`.changeset/fix-astnode-completeness.md` (or merge its content into the existing
changeset), and ensure the remaining changeset title/body clearly describe the
same change to avoid duplicate changelog entries.
In @.changeset/fix-astnode-exhaustiveness.md:
- Around line 1-5: This changeset duplicates the other entry for the same fix
(same package header "@rocket.chat/message-parser": patch and identical summary
"Fix incomplete `ASTNode` union type representation..."); remove this duplicate
changeset file or consolidate it into the existing fix-astnode-completeness
change so there is only one changelog entry for that ASTNode/Types fix, ensuring
the package header and summary remain present exactly once.
In `@packages/message-parser/tests/fuzz.test.ts`:
- Around line 48-50: The second test currently swallows all errors in the catch
block (catch (e) { /* Ignored */ }), so update that catch to only suppress
expected parser syntax errors and rethrow any other unexpected exceptions:
inspect the caught error (e) in the second fuzz test and if it is a
syntax/parser error (e.g., e instanceof SyntaxError or e.name === 'SyntaxError'
or matches the parser's error type) then ignore/log it, otherwise rethrow e so
runtime exceptions surface; modify the catch in that test accordingly to
distinguish parser syntax errors from unexpected runtime exceptions.
- Around line 16-18: The catch-all in the test (the catch(error) block in
packages/message-parser/tests/fuzz.test.ts around the parser invocation)
silently swallows all exceptions; change it to only ignore expected parse/syntax
failures and surface everything else by either rethrowing unexpected errors or
failing the test. Concretely, inspect the caught error (e.g., check instanceof
SyntaxError or match known parser error messages/types that your parser
produces) and return/continue for those, but for any other error call throw
error (or use the test framework's fail) so genuine runtime bugs are not hidden.
---
Outside diff comments:
In `@packages/message-parser/src/definitions.ts`:
- Around line 75-79: The Heading type's value currently restricts to Plain[] but
the parser produces Inline/Inlines (e.g., HeadingChunk → Inline which can be
Emoji, Bold, Italic, etc.), so update the Heading.type definition to allow the
full inline union: replace value: Plain[] with value: Inline[] (or the project's
Inlines/Inline union type) so headings can contain emoji and other inline
formatting; locate the Heading type in definitions.ts and change its value field
to reference the existing Inline/Inlines type used elsewhere.
---
Nitpick comments:
In `@packages/message-parser/tests/fuzz.test.ts`:
- Around line 12-14: Replace the use of `any` in the fuzz test by importing and
using the proper AST type (e.g., `ASTNode` from your Types index) and annotate
the iterator variable in `ast.forEach((node: any) => ...)` as `node: ASTNode`;
ensure the import references `Types[keyof Types]` or the exported `ASTNode` type
and that the `parse` return type is preserved so TypeScript can infer node
properties like `type`.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/add-parser-fuzz-testing.md.changeset/fix-astnode-completeness.md.changeset/fix-astnode-exhaustiveness.md.changeset/fix-data-export-crash.mdpackages/message-parser/package.jsonpackages/message-parser/src/definitions.tspackages/message-parser/src/grammar.pegjspackages/message-parser/tests/fuzz.test.tspackages/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/tests/heading.test.tspackages/message-parser/tests/fuzz.test.tspackages/message-parser/src/definitions.ts
🧠 Learnings (8)
📚 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.tspackages/message-parser/tests/fuzz.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/message-parser/tests/heading.test.tspackages/message-parser/tests/fuzz.test.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/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 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/tests/heading.test.tspackages/message-parser/tests/fuzz.test.tspackages/message-parser/src/definitions.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/tests/heading.test.tspackages/message-parser/tests/fuzz.test.tspackages/message-parser/src/definitions.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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/message-parser/tests/fuzz.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 **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.test.ts
🧬 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-data-export-crash.md (1)
1-5: Changeset entry looks correct and well-scoped.Patch bump and description clearly communicate the crash fix and affected edge case (
emails: []).packages/message-parser/package.json (1)
55-55: LGTM!The addition of
fast-checkas a devDependency is appropriate for implementing property-based fuzz testing. The caret version specifier allows for compatible updates.packages/message-parser/tests/heading.test.ts (1)
47-50: LGTM!The new test cases properly validate emoji parsing within headings at various positions (start, end, multiple), aligning with the grammar changes that allow
Inlinecontent inHeadingChunk..changeset/add-parser-fuzz-testing.md (1)
1-5: LGTM!The changeset correctly documents the addition of fuzz testing as a patch-level change, with an appropriate description of its purpose.
packages/message-parser/src/grammar.pegjs (1)
147-152: LGTM — grammar now supports inline elements in headings.The change from
plain(text)toInlineforHeadingChunkenables parsing of emojis, mentions, and other inline formatting within headings. The callheading(text, count)correctly passes the parsed inline content and heading level.packages/message-parser/src/definitions.ts (1)
202-208: LGTM — ASTNode is now exhaustive.Deriving
ASTNodefromTypes[keyof Types]ensures the union automatically includes all node types defined in theTypesinterface, preventing future omissions.
coderabbitai
bot
added
type: feature
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/message-parser/tests/fuzz.test.ts (1)
12-14: Replaceanywith the properASTNodetype for accurate typing.Using
anybypasses TypeScript's type checking. The parser'sASTNodetype is exported from../srcand should be used to ensure type safety throughout the test.♻️ Proposed fix
-import { parse } from '../src'; +import { parse, ASTNode } from '../src';Then update both usages at lines 12 and 43:
- ast.forEach((node: any) => { + ast.forEach((node: ASTNode) => {Applies to lines 12-14 and 43-46.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/tests/fuzz.test.ts` around lines 12 - 14, Import the parser's ASTNode type from '../src' (e.g. add "import type { ASTNode } from '../src';") and replace the two occurrences of the loose "any" in the test's forEach callbacks (the callbacks that declare "node: any") with the proper "ASTNode" type so the assertions use the concrete type; update both places mentioned in the review (the forEach at the top of the file and the second forEach around lines ~43) to use "node: ASTNode".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/message-parser/tests/fuzz.test.ts`:
- Around line 12-14: Import the parser's ASTNode type from '../src' (e.g. add
"import type { ASTNode } from '../src';") and replace the two occurrences of the
loose "any" in the test's forEach callbacks (the callbacks that declare "node:
any") with the proper "ASTNode" type so the assertions use the concrete type;
update both places mentioned in the review (the forEach at the top of the file
and the second forEach around lines ~43) to use "node: ASTNode".
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/message-parser/tests/fuzz.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/tests/fuzz.test.ts
🧠 Learnings (7)
📚 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/fuzz.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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/message-parser/tests/fuzz.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 **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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 : Maintain test isolation between test cases in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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 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/tests/fuzz.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/tests/fuzz.test.ts
🧬 Code graph analysis (1)
packages/message-parser/tests/fuzz.test.ts (1)
apps/meteor/server/services/omnichannel-integrations/providers/twilio.ts (1)
parse(50-103)
🔇 Additional comments (4)
packages/message-parser/tests/fuzz.test.ts (4)
1-3: LGTM!Imports are appropriate for the fuzz testing purpose.
16-18: Silent error swallowing may mask genuine bugs.This concern was previously raised. The catch block should distinguish expected parse failures from unexpected runtime exceptions.
48-50: Same concern: silent error swallowing in second test.This was previously flagged. Apply the same fix to rethrow unexpected exceptions while only ignoring parser syntax errors.
5-22: Test structure and property-based approach look solid.The use of
fc.assertwithfc.propertycorrectly implements property-based testing. The first test validates AST array structure for arbitrary strings, and the second test uses a clever token-sequence approach to generate markdown-like inputs for testing nested structures. The 1000 runs configuration provides good coverage.Also applies to: 24-55
Contributor
There was a problem hiding this comment.
1 issue found across 6 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">
P2: Heading parsing now bypasses Inline rules and wraps raw text with plain(), so headings lose emoji/emphasis/link parsing and become plain text only.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
coderabbitai
bot
added
type: chore
and removed
type: feature
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/message-parser/tests/fuzz.test.ts`:
- Line 11: In the fuzz.test.ts test implementation remove the inline explanatory
comments (e.g., the "// For any payload, if it's an array, it should be an array
of nodes containing a 'type'" comment and the comments at the other referenced
spots) so the test body contains only executable test code; locate these
comments in the fuzz tests and delete them without changing assertions or logic
(keep the test names, describe/it blocks, and any helper functions intact).
- Around line 8-23: The test currently swallows SyntaxError and will pass
vacuously if every generated input fails to parse; update the fuzz test around
the parse(...) usage to track successful parses by adding a success counter
(e.g., let successCount = 0), increment it inside the try when parse(text)
returns an AST and the AST node checks pass, and after the property run assert
successCount > 0 so the test fails if no input parsed successfully; reference
the parse function and the ast/node property assertions in the existing test
block to locate where to increment and assert.
- Around line 12-15: The test iterates ast nodes using `any`, weakening type
safety; change the iterator type to the actual union returned by `parse()` by
defining a local type alias like `type RootNode = Paragraph | Blocks | BigEmoji`
(or inline `Paragraph | Blocks | BigEmoji`) and use it in the
`ast.forEach((node: RootNode) => { ... })` assertion block so `node` is properly
typed in the `ast.forEach` assertions; reference `parse()`, `Root`, `Paragraph`,
`Blocks`, and `BigEmoji` to ensure the union matches the parser output.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/message-parser/tests/fuzz.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/tests/fuzz.test.ts
🧠 Learnings (8)
📚 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/fuzz.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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/message-parser/tests/fuzz.test.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/message-parser/tests/fuzz.test.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
packages/message-parser/tests/fuzz.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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 **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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 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/tests/fuzz.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/tests/fuzz.test.ts
🧬 Code graph analysis (1)
packages/message-parser/tests/fuzz.test.ts (1)
apps/meteor/app/ui-message/client/messageBox/createComposerAPI.ts (1)
text(325-327)
Contributor
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/grammar.pegjs (1)
151-151:⚠️ Potential issue | 🟡 MinorRemove unused
HeadingChunkrule.The
HeadingChunkrule (line 151) is defined but never referenced anywhere in the grammar file. TheHeadingrule usesHeadingStartandInline+instead, makingHeadingChunkdead code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/src/grammar.pegjs` at line 151, Remove the dead/unused grammar rule HeadingChunk: locate the HeadingChunk production and delete it, since Heading uses HeadingStart and Inline+ instead; ensure no other rules reference HeadingChunk and run the grammar build/tests to confirm no references remain (check Heading, HeadingStart, Inline).
♻️ Duplicate comments (2)
packages/message-parser/tests/fuzz.test.ts (2)
6-24:⚠️ Potential issue | 🟠 MajorAvoid vacuous property passes when parsing always fails.
Both property tests ignore
SyntaxError, so they can pass even if every generated case fails to parse. Add a success counter and assert at least one successful parse per test to ensure meaningful validation.🛡️ Proposed fix to track successful parses
it('should return a valid array of ASTNodes for any arbitrary string', () => { + let successfulParses = 0; fc.assert( fc.property(fc.string({ maxLength: 500 }), (text) => { try { const ast = parse(text); + successfulParses += 1; expect(Array.isArray(ast)).toBe(true); - // For any payload, if it's an array, it should be an array of nodes containing a "type" ast.forEach((val: unknown) => { const node = val as Record<string, unknown>; expect(node).toHaveProperty('type'); expect(typeof node.type).toBe('string'); }); } catch (error) { if (!(error instanceof Error && error.name === 'SyntaxError')) { throw error; } } }), - { numRuns: 1000 } // Execute 1000 random string checks + { numRuns: 1000 } ); + expect(successfulParses).toBeGreaterThan(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/tests/fuzz.test.ts` around lines 6 - 24, The test currently swallows SyntaxError and can pass vacuously; add a success counter around the fuzz run: declare a counter (e.g., let successfulParses = 0) before calling fc.assert(fc.property(..., { numRuns: 1000 })), increment successfulParses inside the try block when parse(text) yields a valid AST, and after fc.assert finishes add an assertion (e.g., expect(successfulParses).toBeGreaterThan(0)) to ensure at least one successful parse occurred; reference the parse function, the fc.assert/fc.property call and the numRuns option to locate where to add the counter and final assertion.
31-60:⚠️ Potential issue | 🟠 MajorSame vacuous pass concern in the second test.
Apply the same success counter pattern to ensure the nested markdown test validates at least one successful parse.
🛡️ Proposed fix for the second test
it('should guarantee structural integrity for deeply nested markdown', () => { - // Valid sequences that generate various inline tokens const sequences = ['*', '_', '~', '`', '# ', '## ', '> ', 'http://', 'https://', ' [', '](', ':', '!)', '\n']; + let successfulParses = 0; fc.assert( fc.property( fc.array( fc.tuple( fc.constantFrom(...sequences), fc.string({ maxLength: 20 }) ), { maxLength: 50 } ), (tokens) => { const text = tokens.map(([token, str]) => `${token}${str}`).join(''); try { const ast = parse(text); + successfulParses += 1; - // Verify that any Paragraph node contains valid Elements ast.forEach((val: unknown) => { const node = val as Record<string, unknown>; if (node.type === 'PARAGRAPH') { expect(Array.isArray(node.value)).toBe(true); } }); } catch (e) { if (!(e instanceof Error && e.name === 'SyntaxError')) { throw e; } } } ), { numRuns: 1000 } ); + expect(successfulParses).toBeGreaterThan(0); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/tests/fuzz.test.ts` around lines 31 - 60, The second fuzz test can vacuously pass without any successful parses; declare a success counter (e.g. successCount = 0) before the fc.assert call, increment it inside the property callback when a parse completes and the PARAGRAPH validation passes (after parse() and the Array.isArray(node.value) checks), keep catching SyntaxError as currently done, then after fc.assert finishes assert that successCount > 0 (or throw if it is zero) to ensure at least one successful parse occurred; reference parse, ast, the PARAGRAPH node check, and the fc.assert/fc.property closure to locate where to add the counter and the final assertion.
🤖 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/grammar.pegjs`:
- Line 151: Remove the dead/unused grammar rule HeadingChunk: locate the
HeadingChunk production and delete it, since Heading uses HeadingStart and
Inline+ instead; ensure no other rules reference HeadingChunk and run the
grammar build/tests to confirm no references remain (check Heading,
HeadingStart, Inline).
---
Duplicate comments:
In `@packages/message-parser/tests/fuzz.test.ts`:
- Around line 6-24: The test currently swallows SyntaxError and can pass
vacuously; add a success counter around the fuzz run: declare a counter (e.g.,
let successfulParses = 0) before calling fc.assert(fc.property(..., { numRuns:
1000 })), increment successfulParses inside the try block when parse(text)
yields a valid AST, and after fc.assert finishes add an assertion (e.g.,
expect(successfulParses).toBeGreaterThan(0)) to ensure at least one successful
parse occurred; reference the parse function, the fc.assert/fc.property call and
the numRuns option to locate where to add the counter and final assertion.
- Around line 31-60: The second fuzz test can vacuously pass without any
successful parses; declare a success counter (e.g. successCount = 0) before the
fc.assert call, increment it inside the property callback when a parse completes
and the PARAGRAPH validation passes (after parse() and the
Array.isArray(node.value) checks), keep catching SyntaxError as currently done,
then after fc.assert finishes assert that successCount > 0 (or throw if it is
zero) to ensure at least one successful parse occurred; reference parse, ast,
the PARAGRAPH node check, and the fc.assert/fc.property closure to locate where
to add the counter and the final assertion.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/message-parser/src/grammar.pegjspackages/message-parser/tests/fuzz.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/tests/fuzz.test.ts
🧠 Learnings (13)
📚 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/fuzz.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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/message-parser/tests/fuzz.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 : Group related tests in the same file
Applied to files:
packages/message-parser/tests/fuzz.test.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/message-parser/tests/fuzz.test.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
packages/message-parser/tests/fuzz.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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 **/*.{ts,tsx,js} : Avoid code comments in the implementation
Applied to files:
packages/message-parser/tests/fuzz.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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.test.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
packages/message-parser/tests/fuzz.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 **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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 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/tests/fuzz.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/tests/fuzz.test.ts
🔇 Additional comments (2)
packages/message-parser/tests/fuzz.test.ts (1)
11-11: Remove inline comments from the test implementation.The coding guidelines specify avoiding code comments in the implementation. The following lines contain explanatory comments that should be removed:
- Line 11:
// For any payload...- Line 23:
// Execute 1000 random string checks- Line 28:
// Valid sequences...- Line 45:
// Verify that any Paragraph...As per coding guidelines: "Avoid code comments in the implementation".
Also applies to: 23-23, 28-28, 45-45
packages/message-parser/src/grammar.pegjs (1)
147-147: LGTM! Heading now supports inline formatting.The change from
HeadingChunktoInline+allows rich inline elements (bold, italic, links, mentions, etc.) within headings. SinceInline+returns an array, passingtextdirectly toheading()preserves the expected array structure.
Contributor
There was a problem hiding this comment.
1 issue found across 2 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:147">
P2: Heading parsing can span multiple lines because `Inline+` can consume optional `EndOfLine`, allowing headings to absorb subsequent lines/blocks.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/message-parser/tests/fuzz.test.ts (2)
6-25:⚠️ Potential issue | 🟠 MajorPrevent vacuous property passes when all generated inputs fail parsing.
Line 6–25 and Line 32–62 can pass even if every run throws
SyntaxError. Add a successful-parse counter and assert it is greater than zero per test.Proposed fix
describe('Message Parser Property-Based Fuzz Testing', () => { it('should return a valid array of ASTNodes for any arbitrary string', () => { + let successfulParses = 0; fc.assert( fc.property(fc.string({ maxLength: 500 }), (text) => { try { const ast = parse(text); + successfulParses += 1; expect(Array.isArray(ast)).toBe(true); ast.forEach((val: unknown) => { const node = val as Record<string, unknown>; expect(node).toHaveProperty('type'); expect(typeof node.type).toBe('string'); @@ }), { numRuns: 1000 } ); + expect(successfulParses).toBeGreaterThan(0); }); @@ it('should guarantee structural integrity for deeply nested markdown', () => { + let successfulParses = 0; const sequences = ['*', '_', '~', '`', '# ', '## ', '> ', 'http://', 'https://', ' [', '](', ':', '!)', '\n']; @@ (tokens) => { const text = tokens.map(([token, str]) => `${token}${str}`).join(''); try { const ast = parse(text); + successfulParses += 1; ast.forEach((val: unknown) => { const node = val as Record<string, unknown>; if (node.type === 'PARAGRAPH') { expect(Array.isArray(node.value)).toBe(true); @@ ), { numRuns: 1000 } ); + expect(successfulParses).toBeGreaterThan(0); }); });Also applies to: 32-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/tests/fuzz.test.ts` around lines 6 - 25, The fuzz tests currently treat any case where every generated input throws a SyntaxError as a pass; update both fc.property blocks (the one using parse and the other similar block) to track successful parses by declaring a counter (e.g., successfulParses = 0) outside the property callback, incrementing it whenever parse(text) returns without throwing and the subsequent assertions on the resulting ast succeed, and after fc.assert completes add an assertion that successfulParses > 0 to ensure at least one input parsed successfully; reference the existing parse call, ast variable, and the fc.assert / fc.property setup when making this change.
11-11:⚠️ Potential issue | 🟡 MinorRemove inline implementation comments in this test file.
Line 11, Line 21, Line 24, Line 29, Line 46, and Line 57 should be removed to keep the implementation concise.
Proposed cleanup
- // For any payload, if it's an array, it should be an array of nodes containing a "type" ast.forEach((val: unknown) => { @@ - // Intentionally ignore SyntaxError during fuzzing } }), - { numRuns: 1000 } // Execute 1000 random string checks + { numRuns: 1000 } ); @@ - // Valid sequences that generate various inline tokens const sequences = ['*', '_', '~', '`', '# ', '## ', '> ', 'http://', 'https://', ' [', '](', ':', '!)', '\n']; @@ - // Verify that any Paragraph node contains valid Elements ast.forEach((val: unknown) => { @@ - // Intentionally ignore SyntaxError during fuzzing } }As per coding guidelines, "Avoid code comments in the implementation".
Also applies to: 21-21, 24-24, 29-29, 46-46, 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/tests/fuzz.test.ts` at line 11, Remove the inline implementation comment lines in the fuzz test that start with "// For any payload, if it's an array, it should be an array of nodes containing a "type"" and the other similar inline comments present in the test (the comments on the same lines as the test implementation around the fuzz cases), leaving only the test code and assertions; locate these comments in packages/message-parser/tests/fuzz.test.ts by searching for comment text fragments and delete those comment lines so the test contains no implementation comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/message-parser/tests/fuzz.test.ts`:
- Around line 6-25: The fuzz tests currently treat any case where every
generated input throws a SyntaxError as a pass; update both fc.property blocks
(the one using parse and the other similar block) to track successful parses by
declaring a counter (e.g., successfulParses = 0) outside the property callback,
incrementing it whenever parse(text) returns without throwing and the subsequent
assertions on the resulting ast succeed, and after fc.assert completes add an
assertion that successfulParses > 0 to ensure at least one input parsed
successfully; reference the existing parse call, ast variable, and the fc.assert
/ fc.property setup when making this change.
- Line 11: Remove the inline implementation comment lines in the fuzz test that
start with "// For any payload, if it's an array, it should be an array of nodes
containing a "type"" and the other similar inline comments present in the test
(the comments on the same lines as the test implementation around the fuzz
cases), leaving only the test code and assertions; locate these comments in
packages/message-parser/tests/fuzz.test.ts by searching for comment text
fragments and delete those comment lines so the test contains no implementation
comments.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/message-parser/package.jsonpackages/message-parser/tests/fuzz.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/message-parser/package.json
📜 Review details
🧰 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/tests/fuzz.test.ts
🧠 Learnings (13)
📚 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/fuzz.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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/message-parser/tests/fuzz.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 : Group related tests in the same file
Applied to files:
packages/message-parser/tests/fuzz.test.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/message-parser/tests/fuzz.test.ts
📚 Learning: 2026-02-24T19:36:55.089Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 38493
File: apps/meteor/tests/e2e/page-objects/fragments/home-content.ts:60-82
Timestamp: 2026-02-24T19:36:55.089Z
Learning: In RocketChat/Rocket.Chat e2e tests (apps/meteor/tests/e2e/page-objects/fragments/home-content.ts), thread message preview listitems do not have aria-roledescription="message", so lastThreadMessagePreview locator cannot be scoped to messageListItems (which filters for aria-roledescription="message"). It should remain scoped to page.getByRole('listitem') or mainMessageList.getByRole('listitem').
Applied to files:
packages/message-parser/tests/fuzz.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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 **/*.{ts,tsx,js} : Avoid code comments in the implementation
Applied to files:
packages/message-parser/tests/fuzz.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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.test.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
packages/message-parser/tests/fuzz.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 **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
packages/message-parser/tests/fuzz.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 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/tests/fuzz.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/tests/fuzz.test.ts
🔇 Additional comments (1)
packages/message-parser/tests/fuzz.test.ts (1)
17-20: Good exception filtering in fuzz tests.Line 17–20 and Line 53–56 correctly rethrow unexpected errors instead of swallowing everything.
Also applies to: 53-56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
To prepare for tracking edge cases inside the High-Performance Message Parser project, this implements property-based fuzz testing leveraging \ ast-check. Instead of providing granular strings, this systematically feeds randomized structural AST combinations to ensure the payload outputs correct AST schemas, and prevents recursion issues like exponential backtracking.
Currently tests structural schema output bindings and deeply nested inline formatting.
Checklist
Summary by CodeRabbit