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 |
|
WalkthroughA new compact message-parser format module is introduced with bidirectional conversion APIs (expand, compactify, validateRoundtrip). The grammar is refactored to simplify parsing by removing skip-state flags and standardizing value returns. Comprehensive tests verify round-trip integrity and size reduction. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
ggazzo
force-pushed
the
chore/grammar
branch
from
ca42b1f to
2e7731d
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38959 +/- ##
===========================================
+ Coverage 70.73% 70.79% +0.05%
===========================================
Files 3195 3196 +1
Lines 113105 113535 +430
Branches 20510 20663 +153
===========================================
+ Hits 80010 80376 +366
- Misses 31049 31113 +64
Partials 2046 2046
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…rity and functionality
…g efficiency and clarity
… conversion and validation functions
ggazzo
force-pushed
the
chore/grammar
branch
from
2e7731d to
ffa58e1
Compare
…main in grammar rules
coderabbitai
bot
added
the
type: feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/message-parser/src/compact.ts (2)
91-93: Consider adding validation for span bounds.The type guard correctly identifies spans but doesn't validate that indices are non-negative or that
start <= end. Invalid spans could causemsg.slice()to return unexpected results (empty string for inverted spans, or from end of string for negative indices).This is low risk since spans are generated by internal code, but defensive validation could help catch bugs earlier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/src/compact.ts` around lines 91 - 93, The isSpan type guard currently checks array shape but not bounds; update isSpan (and any Span type checks) to also validate that both elements are integers >= 0 and that the first element is <= the second (start <= end) so downstream uses like msg.slice() won't receive negative or inverted indices; locate the function isSpan and add these numeric/ordering checks (and consider using Number.isInteger on v[0]/v[1]) to return false for invalid spans.
121-131: Type narrowing could be cleaner.The current approach uses
(node as any).cwhich loses type safety. Consider using a type guard or checking the discriminated property more explicitly.♻️ Suggested improvement
case 'a': { - if ('c' in node && Array.isArray((node as any).c)) { - const rich = node as { t: 'a'; c: CInline[]; s: string }; + if ('c' in node) { + const rich = node as Extract<CInlineNode, { t: 'a'; c: CInline[] }>; return { type: 'LINK', value: { src: { type: 'PLAIN_TEXT', value: rich.s } as Plain, label: rich.c.map((c) => expandInline(msg, c)) as Markup[], }, } as Link; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/src/compact.ts` around lines 121 - 131, The case 'a' branch uses (node as any).c which bypasses type safety; replace that check with a proper type guard or narrower type assertion so TypeScript can infer c exists and is CInline[]—for example add a predicate like isAnchorNode(node): node is { t: 'a'; c: CInline[]; s: string } and use it in the if, then cast node to rich (or let the guard narrow it) before mapping with expandInline to construct the Link/Plain/Markup objects; update the branch that returns Link to use the narrowed type instead of any.packages/message-parser/tests/compact.test.ts (1)
322-339: Consider making benchmark logging conditional.The
console.logstatement on line 338 will output on every test run, which may clutter CI logs. Consider gating this behind an environment variable or using a test reporter.♻️ Suggested improvement
expect(result.ok).toBe(true); expect(result.sizeNew).toBeLessThan(result.sizeOld); - console.log(` ${_label}: ${result.sizeOld}B → ${result.sizeNew}B (${result.reduction})`); + if (process.env.DEBUG) { + console.log(` ${_label}: ${result.sizeOld}B → ${result.sizeNew}B (${result.reduction})`); + } }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/message-parser/tests/compact.test.ts` around lines 322 - 339, The console.log inside the 'size reduction' test is unconditionally printing on every run; guard that diagnostic output by checking an env var (e.g., process.env.SHOW_BENCHMARK) or a test-specific flag before calling console.log so CI stays clean; locate the console.log in the size reduction describe/it block in compact.test.ts and wrap it with the conditional check (or replace with a test reporter/debug logger) so the message only prints when the env var/flag is set.
🤖 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/src/compact.ts`:
- Around line 268-271: The function advancePast(ctx: Ctx, str: string) currently
silently does nothing if str isn't found; change its signature to return a
boolean (true if found and ctx.pos advanced, false otherwise) and update callers
to check the return value and handle the failure (either propagate an error or
throw at the appropriate parse boundary). Locate advancePast and all call sites
that pass ctx.msg/ctx.pos, implement the boolean return, and add explicit error
handling where a missing delimiter would desynchronize parsing (or throw from
the caller if preferred).
In `@packages/message-parser/tests/compact.test.ts`:
- Around line 24-26: The size-reduction assertion in the test is too strict for
tiny inputs: update the expectation in
packages/message-parser/tests/compact.test.ts to allow equality for edge cases
by replacing expect(result.sizeNew).toBeLessThan(result.sizeOld) with
expect(result.sizeNew).toBeLessThanOrEqual(result.sizeOld), or alternatively add
a minimum message-length guard so the check only runs when the original message
length (or result.sizeOld) exceeds a small threshold; ensure you update the
assertion around the same `result` variable used in the test.
---
Nitpick comments:
In `@packages/message-parser/src/compact.ts`:
- Around line 91-93: The isSpan type guard currently checks array shape but not
bounds; update isSpan (and any Span type checks) to also validate that both
elements are integers >= 0 and that the first element is <= the second (start <=
end) so downstream uses like msg.slice() won't receive negative or inverted
indices; locate the function isSpan and add these numeric/ordering checks (and
consider using Number.isInteger on v[0]/v[1]) to return false for invalid spans.
- Around line 121-131: The case 'a' branch uses (node as any).c which bypasses
type safety; replace that check with a proper type guard or narrower type
assertion so TypeScript can infer c exists and is CInline[]—for example add a
predicate like isAnchorNode(node): node is { t: 'a'; c: CInline[]; s: string }
and use it in the if, then cast node to rich (or let the guard narrow it) before
mapping with expandInline to construct the Link/Plain/Markup objects; update the
branch that returns Link to use the narrowed type instead of any.
In `@packages/message-parser/tests/compact.test.ts`:
- Around line 322-339: The console.log inside the 'size reduction' test is
unconditionally printing on every run; guard that diagnostic output by checking
an env var (e.g., process.env.SHOW_BENCHMARK) or a test-specific flag before
calling console.log so CI stays clean; locate the console.log in the size
reduction describe/it block in compact.test.ts and wrap it with the conditional
check (or replace with a test reporter/debug logger) so the message only prints
when the env var/flag is set.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/message-parser/src/compact.tspackages/message-parser/src/grammar.pegjspackages/message-parser/tests/compact.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/compact.test.tspackages/message-parser/src/compact.ts
🧠 Learnings (11)
📓 Common learnings
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.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
📚 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/compact.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/compact.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/compact.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 `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
packages/message-parser/tests/compact.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/compact.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 **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
packages/message-parser/tests/compact.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/compact.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/compact.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/compact.test.tspackages/message-parser/src/compact.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/compact.test.tspackages/message-parser/src/compact.ts
🧬 Code graph analysis (1)
packages/message-parser/tests/compact.test.ts (1)
packages/message-parser/src/compact.ts (4)
validateRoundtrip(595-615)CRoot(85-85)expand(241-248)compactify(549-569)
🔇 Additional comments (23)
packages/message-parser/src/compact.ts (9)
1-9: LGTM!Clear module documentation explaining the bidirectional conversion purpose and the three main APIs.
49-86: LGTM!Compact type definitions are well-structured with appropriate discriminated unions. The short property names effectively reduce serialized size.
177-239: LGTM!The
expandBlockfunction comprehensively handles all block types with correct reconstruction logic for code blocks and nested structures.
241-248: LGTM!Clean entry point that correctly handles the BigEmoji special case and delegates to
expandBlock.
303-432: LGTM!The
compactInlinefunction correctly handles all inline node types with appropriate delimiter detection and span calculation.
436-468: LGTM with observation.The block compactification handles all cases correctly. The code block handling assumes the grammar-enforced structure with newlines around content.
549-569: LGTM!Clean entry point that properly handles the BigEmoji case and block iteration with appropriate newline handling.
575-593: LGTM!The
deepEqualimplementation is appropriate for comparing AST structures. Key sorting ensures consistent comparison regardless of property order.
595-615: LGTM!The validation function provides comprehensive roundtrip testing with useful size metrics for analysis.
packages/message-parser/tests/compact.test.ts (3)
39-150: LGTM!Comprehensive test coverage for the
expandfunction covering all major inline and block types.
152-207: LGTM!Good basic coverage for
compactify. More complex cases are covered in the roundtrip tests.
209-273: LGTM!Excellent roundtrip test coverage spanning plain text, emphasis, mentions, code, emoji, links, structured blocks, katex, timestamps, emoticons, and real-world message scenarios.
packages/message-parser/src/grammar.pegjs (11)
97-97: LGTM!The simplified Unixtime rule with exact 10 digits is appropriate for current and near-future timestamps.
132-134: LGTM!The
CodeChunkCharrefactor using character classes is a sound optimization that reduces per-character lookahead while correctly handling backtick sequences.
150-150: LGTM!Simplified heading text capture is cleaner and functionally equivalent.
253-291: LGTM!The
InlineItemPatternreorganization improves parsing clarity. ThePlainRunBeforeEmoticonrule correctly handles the edge case for emoticons like-_-.
298-307: LGTM!Spoiler parsing simplified by removing unnecessary state flags. The direct return pattern is cleaner.
336-337: LGTM!The head+tail URL composition pattern is efficient and consistent across
MarkdownLinkURL,URL, andAutoLinkURLrules.
439-440: LGTM!The
PlainUnderscoreThenDomainrule correctly handles edge cases where underscore-prefixed domains should be plain text rather than italic markers.
530-593: LGTM!The simplified emphasis content rules with specialized
PlainRunvariants correctly exclude characters needed for nested formatting while removing state flag complexity.
625-631: LGTM!The
UserMentionrule update correctly excludes usernames ending with__using a semantic predicate, preventing conflicts with emphasis parsing.
781-781: LGTM!The
InlineCoderule using[^\n]+` character class is more efficient than per-character lookahead.
808-808: LGTM!The
Anyrule change to match any non-EndOfLine character is appropriate as a fallback for characters not captured by specialized rules.
| function advancePast(ctx: Ctx, str: string) { | ||
| const i = ctx.msg.indexOf(str, ctx.pos); | ||
| if (i >= 0) ctx.pos = i + str.length; | ||
| } |
There was a problem hiding this comment.
Silent failure when string not found could mask bugs.
advancePast silently does nothing when the target string isn't found. This could cause the parser position to become desynchronized, leading to incorrect spans or errors later. Consider throwing or returning a boolean to indicate success.
🛡️ Suggested improvement
function advancePast(ctx: Ctx, str: string) {
const i = ctx.msg.indexOf(str, ctx.pos);
- if (i >= 0) ctx.pos = i + str.length;
+ if (i === -1) {
+ throw new Error(`Expected "${str}" at pos ${ctx.pos} in msg`);
+ }
+ ctx.pos = i + str.length;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function advancePast(ctx: Ctx, str: string) { | |
| const i = ctx.msg.indexOf(str, ctx.pos); | |
| if (i >= 0) ctx.pos = i + str.length; | |
| } | |
| function advancePast(ctx: Ctx, str: string) { | |
| const i = ctx.msg.indexOf(str, ctx.pos); | |
| if (i === -1) { | |
| throw new Error(`Expected "${str}" at pos ${ctx.pos} in msg`); | |
| } | |
| ctx.pos = i + str.length; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/message-parser/src/compact.ts` around lines 268 - 271, The function
advancePast(ctx: Ctx, str: string) currently silently does nothing if str isn't
found; change its signature to return a boolean (true if found and ctx.pos
advanced, false otherwise) and update callers to check the return value and
handle the failure (either propagate an error or throw at the appropriate parse
boundary). Locate advancePast and all call sites that pass ctx.msg/ctx.pos,
implement the boolean return, and add explicit error handling where a missing
delimiter would desynchronize parsing (or throw from the caller if preferred).
| expect(result.ok).toBe(true); | ||
| expect(result.sizeNew).toBeLessThan(result.sizeOld); | ||
| } |
There was a problem hiding this comment.
Size reduction assertion may be too strict for edge cases.
The assertion expect(result.sizeNew).toBeLessThan(result.sizeOld) assumes compact is always smaller. For very short messages (e.g., single character), the compact format's structural overhead may actually be larger. Consider using toBeLessThanOrEqual or adding a minimum message length threshold.
🛡️ Suggested fix
expect(result.ok).toBe(true);
- expect(result.sizeNew).toBeLessThan(result.sizeOld);
+ // For very short messages, compact overhead may equal or slightly exceed verbose
+ expect(result.sizeNew).toBeLessThanOrEqual(result.sizeOld);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(result.ok).toBe(true); | |
| expect(result.sizeNew).toBeLessThan(result.sizeOld); | |
| } | |
| expect(result.ok).toBe(true); | |
| expect(result.sizeNew).toBeLessThanOrEqual(result.sizeOld); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/message-parser/tests/compact.test.ts` around lines 24 - 26, The
size-reduction assertion in the test is too strict for tiny inputs: update the
expectation in packages/message-parser/tests/compact.test.ts to allow equality
for edge cases by replacing expect(result.sizeNew).toBeLessThan(result.sizeOld)
with expect(result.sizeNew).toBeLessThanOrEqual(result.sizeOld), or
alternatively add a minimum message-length guard so the check only runs when the
original message length (or result.sizeOld) exceeds a small threshold; ensure
you update the assertion around the same `result` variable used in the test.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 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/src/compact.ts">
<violation number="1" location="packages/message-parser/src/compact.ts:270">
P2: `advancePast` silently no-ops when the target string is not found (`indexOf` returns -1), which leaves `ctx.pos` unchanged. This can silently desynchronize the cursor position, causing all subsequent `spanFor` / `advance` calls to produce incorrect spans. Unlike `spanFor` which throws on missing text, `advancePast` masks the error. Consider throwing an error (consistent with `spanFor`'s behavior) so that bugs in delimiter expectations surface immediately rather than producing subtly corrupted compact output.</violation>
<violation number="2" location="packages/message-parser/src/compact.ts:426">
P1: Bug: `COLOR` case in `compactInline` does not advance `ctx.pos` past the color source text (e.g., `color:#ff0000`), which corrupts span tracking for all subsequent sibling nodes in the same paragraph. Every other inline case advances the position, but this one is missing.</violation>
</file>
<file name="packages/message-parser/src/grammar.pegjs">
<violation number="1" location="packages/message-parser/src/grammar.pegjs:133">
P1: `CodeChunkChar` rejects code lines ending with one or two backticks, causing valid fenced code blocks to fail parsing.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| CodeChunk = text:$(!EndOfLine !"```" .)+ { return plain(text); } | ||
| // Charclass avoids per-char lookahead; never consume start of "```" | ||
| CodeChunkChar = [^\r\n`] / "`" [^`\r\n] / "`" "`" [^`\r\n] |
There was a problem hiding this comment.
P1: CodeChunkChar rejects code lines ending with one or two backticks, causing valid fenced code blocks to fail parsing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/message-parser/src/grammar.pegjs, line 133:
<comment>`CodeChunkChar` rejects code lines ending with one or two backticks, causing valid fenced code blocks to fail parsing.</comment>
<file context>
@@ -132,7 +129,9 @@ CodeLine
-CodeChunk = text:$(!EndOfLine !"```" .)+ { return plain(text); }
+// Charclass avoids per-char lookahead; never consume start of "```"
+CodeChunkChar = [^\r\n`] / "`" [^`\r\n] / "`" "`" [^`\r\n]
+CodeChunk = text:$(CodeChunkChar)+ { return plain(text); }
</file context>
| return { t: 'ts', r, f: node.value.format }; | ||
| } | ||
|
|
||
| case 'COLOR': |
There was a problem hiding this comment.
P1: Bug: COLOR case in compactInline does not advance ctx.pos past the color source text (e.g., color:#ff0000), which corrupts span tracking for all subsequent sibling nodes in the same paragraph. Every other inline case advances the position, but this one is missing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/message-parser/src/compact.ts, line 426:
<comment>Bug: `COLOR` case in `compactInline` does not advance `ctx.pos` past the color source text (e.g., `color:#ff0000`), which corrupts span tracking for all subsequent sibling nodes in the same paragraph. Every other inline case advances the position, but this one is missing.</comment>
<file context>
@@ -0,0 +1,615 @@
+ return { t: 'ts', r, f: node.value.format };
+ }
+
+ case 'COLOR':
+ return { t: 'c', v: [node.value.r, node.value.g, node.value.b, node.value.a] };
+
</file context>
|
|
||
| function advancePast(ctx: Ctx, str: string) { | ||
| const i = ctx.msg.indexOf(str, ctx.pos); | ||
| if (i >= 0) ctx.pos = i + str.length; |
There was a problem hiding this comment.
P2: advancePast silently no-ops when the target string is not found (indexOf returns -1), which leaves ctx.pos unchanged. This can silently desynchronize the cursor position, causing all subsequent spanFor / advance calls to produce incorrect spans. Unlike spanFor which throws on missing text, advancePast masks the error. Consider throwing an error (consistent with spanFor's behavior) so that bugs in delimiter expectations surface immediately rather than producing subtly corrupted compact output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/message-parser/src/compact.ts, line 270:
<comment>`advancePast` silently no-ops when the target string is not found (`indexOf` returns -1), which leaves `ctx.pos` unchanged. This can silently desynchronize the cursor position, causing all subsequent `spanFor` / `advance` calls to produce incorrect spans. Unlike `spanFor` which throws on missing text, `advancePast` masks the error. Consider throwing an error (consistent with `spanFor`'s behavior) so that bugs in delimiter expectations surface immediately rather than producing subtly corrupted compact output.</comment>
<file context>
@@ -0,0 +1,615 @@
+
+function advancePast(ctx: Ctx, str: string) {
+ const i = ctx.msg.indexOf(str, ctx.pos);
+ if (i >= 0) ctx.pos = i + str.length;
+}
+
</file context>
1 participant
This pull request refactors the
packages/message-parser/src/grammar.pegjsfile to simplify and optimize the parsing rules for inline formatting, links, and code blocks. The changes remove unnecessary state variables, streamline rule definitions, and improve performance by reducing per-character lookaheads and simplifying fallback logic. The parser is now more maintainable and efficient, with clearer separation between plain text and formatting elements.Inline formatting and emphasis simplification:
skipBoldEmoji,skipItalicEmoji, andskipInlineEmojistate variables, and refactored emphasis (Bold,Italic,Strikethrough) and inline item rules to eliminate reliance on these flags, simplifying the parsing logic for emojis and emoticons within formatting. [1] [2] [3]Link and URL parsing improvements:
URL,MarkdownLinkURL, andAutoLinkURLrules to concatenate head and tail components, reducing per-character lookahead and improving performance. [1] [2] [3]LinkTitlerule with complex negative lookahead to prevent accidental matching of link end patterns, making link parsing more robust.Code block and inline code parsing:
CodeChunkandInlineCoderules to use character classes and direct matches, avoiding inefficient per-character lookahead and ensuring correct handling of backticks and newlines. [1] [2]General parser optimizations and cleanup:
User mention parsing enhancement:
UserMentionrule to exclude usernames ending with double underscores, preventing invalid mentions.Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
before
after
Summary by CodeRabbit
Release Notes
New Features
Tests