refactor: make reducePlainTexts linear and allocation-light#38901
refactor: make reducePlainTexts linear and allocation-light#38901Shreyas2004wagh wants to merge 3 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 |
|
WalkthroughRefactored Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Shreyas2004wagh
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/message-parser/src/utils.ts
🧰 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/utils.ts
🧬 Code graph analysis (1)
packages/message-parser/src/utils.ts (1)
packages/message-parser/src/definitions.ts (1)
Paragraph(136-139)
⏰ 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
packages/message-parser/src/utils.ts (1)
185-202: LGTM — linear scan and neighbor-lookup semantics are correctly preserved.
flattenedValues[index - 1]/flattenedValues[index + 1]asjoinEmojineighbors matches what the oldreducereceived via itsarrayparameter (the original flattened array, not the accumulated result), so emoji-conversion decisions are identical. Out-of-bounds accesses (index=0→undefined;index=length-1→undefined) are handled correctly byjoinEmoji's(!previous && !next)guard.
🤖 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/utils.ts`:
- Around line 193-195: The code in joinEmoji is mutating caller-owned nodes by
doing previous.value += current.value; instead create a new merged node object
instead of mutating previous: when previous and current are both type
'PLAIN_TEXT', construct a fresh node (copying the previous node's meta/other
fields but with value = previous.value + current.value) and replace the last
entry in result with that new object (do not modify previous or current
references); update any local variable usage accordingly so result always
contains new objects while preserving all other logic in joinEmoji.
| if (previous && current.type === 'PLAIN_TEXT' && previous.type === 'PLAIN_TEXT') { | ||
| previous.value += current.value; | ||
| continue; |
There was a problem hiding this comment.
previous.value += mutates an object sourced from the caller's input array.
joinEmoji returns current unchanged (same reference) for all non-EMOJI nodes, so PLAIN_TEXT items pushed into result are the exact same objects as their counterparts in flattenedValues / values. The subsequent previous.value += current.value therefore mutates the caller-owned node.
The original reduce implementation created a new merged object on each concatenation (the whole point of spreading [...result, …] was to produce a fresh accumulator array with a newly merged tail). Replacing that with an in-place mutation is a subtle semantic change: any caller that retains a reference to an element of values will silently observe its value string extended.
While harmless in the parser's hot path (nodes are ephemeral, freshly allocated per parse), it breaks the "inputs are not modified" contract and could cause confusing failures in tests or future reuse outside the parser.
🛡️ Proposed fix — create a new merged node instead of mutating
- if (previous && current.type === 'PLAIN_TEXT' && previous.type === 'PLAIN_TEXT') {
- previous.value += current.value;
- continue;
- }
+ if (previous && current.type === 'PLAIN_TEXT' && previous.type === 'PLAIN_TEXT') {
+ result[result.length - 1] = { ...previous, value: previous.value + current.value };
+ continue;
+ }This still allocates only one object per merge (better than the O(n²) array spreads of the old reduce), while leaving the input nodes untouched.
📝 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.
| if (previous && current.type === 'PLAIN_TEXT' && previous.type === 'PLAIN_TEXT') { | |
| previous.value += current.value; | |
| continue; | |
| if (previous && current.type === 'PLAIN_TEXT' && previous.type === 'PLAIN_TEXT') { | |
| result[result.length - 1] = { ...previous, value: previous.value + current.value }; | |
| continue; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/message-parser/src/utils.ts` around lines 193 - 195, The code in
joinEmoji is mutating caller-owned nodes by doing previous.value +=
current.value; instead create a new merged node object instead of mutating
previous: when previous and current are both type 'PLAIN_TEXT', construct a
fresh node (copying the previous node's meta/other fields but with value =
previous.value + current.value) and replace the last entry in result with that
new object (do not modify previous or current references); update any local
variable usage accordingly so result always contains new objects while
preserving all other logic in joinEmoji.
There was a problem hiding this comment.
mutation is pre-existing behavior intentionally preserved.If maintainers want immutability, I will do it in a separate PR with explicit tests.
There was a problem hiding this comment.
🧩 Analysis chain
values.flat().reduce(
(result, item, index, values) => {
// ...
const previous: Inlines = result[result.length - 1];
if (previous) {
if (current.type === 'PLAIN_TEXT' && current.type === previous.type) {
previous.value += current.value; // <-- MUTATION HERE
return result;
}
}
return [...result, current];
},
[] as Paragraph['value'],
);
New code:
for (let index = 0; index < flattenedValues.length; index++) {
const current = joinEmoji(...);
const previous = result[result.length - 1];
if (previous && current.type === 'PLAIN_TEXT' && previous.type === 'PLAIN_TEXT') {
previous.value += current.value; // <-- SAME MUTATION
continue;
}
result.push(current);
}
Conclusion: The mutation pattern `previous.value += current.value` exists in BOTH versions.
EOF
🏁 Script executed:
#!/bin/bash
# Verify the old code's mutation behavior by examining the diff more carefully
cat << 'EOF'
Old code analysis:
Length of output: 110
@Shreyas2004wagh You're absolutely right—I apologize for the confusion. Looking at the original code, previous.value += current.value was indeed present and mutating the accumulator in the same way. Your refactor correctly preserves that existing behavior while improving performance by eliminating the array spreads.
Your approach is sound: keep this PR focused on the performance improvement, and if immutability becomes a requirement, address it in a separate PR with appropriate tests. Thank you for the clarification! 👍
🧬 Code Graph Analysis Results
[packages/message-parser/src/utils.ts: joinEmoji]
Docstring-style summary:
- Purpose: Decide how to normalize/adjust emoji inline nodes when adjacent to other inlines.
- Parameters:
- current: Inlines — the current inline node being examined (expected type EMOJI for this function to operate meaningfully).
- previous: Inlines | undefined — the preceding inline node, if any.
- next: Inlines | undefined — the following inline node, if any.
- Returns: Inlines — possibly transformed emoji inline.
- Behavior:
- If current.type is not 'EMOJI' or current.value is falsy, returns current unchanged.
- Detects whether neighboring inlines qualify as emoji neighbors or plain text neighbors.
- If current is an emoji and has an emoji or plain-text neighbor, may convert shortCode to a colon-wrapped emoji code (e.g., 😄) unless it’s an emoticon (where shortCode equals value).
- If emoticon (shortCode equals current.value.value), returns current.value as a plain emoji value instead of re-wrapping.
- Notes: This function mutates none of the input objects; it returns either the original current inline or a new object containing a transformed value when needed.
[packages/message-parser/src/utils.ts: reducePlainTexts]
Docstring-style summary:
- Purpose: Merge adjacent plain text inlines within a paragraph, while preserving non-plain inlines intact.
- Parameters:
- values: Paragraph['value'] — an array of Inlines (excluding Paragraph types) that make up a paragraph.
- Returns: Paragraph['value'] — a new array with consecutive plain-text inlines merged.
- Behavior:
- Flattens the input value array.
- Iterates through flattened inlines, using joinEmoji to possibly normalize emoji neighbors.
- If the current and previous inlines are both Plain, concatenates their text into the previous element and skips adding the current.
- Otherwise, pushes the current inline (possibly transformed by joinEmoji) into the result.
- Notes:
- Relies on joinEmoji for emoji/plain-text handling across neighbors.
- Intended to reduce fragmentation of plain text nodes, potentially affecting downstream mutation behavior if the current implementation mutates objects during accumulation (the described behavior in joinEmoji affects only current/value transformations, not external mutations).
🧠 Learnings used
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38901 +/- ##
===========================================
+ Coverage 70.55% 70.60% +0.04%
===========================================
Files 3189 3189
Lines 112703 112705 +2
Branches 20429 20412 -17
===========================================
+ Hits 79519 79570 +51
+ Misses 31123 31074 -49
Partials 2061 2061
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
2 participants
Summary
reducePlainTextsto use a single mutable accumulator[...result, current]) in a hot parsing pathWhy
reducePlainTextsis used frequently while parsing inline-rich messages. Rebuilding arrays on every iteration causes avoidable allocations and GC pressure.Implementation
values.flat())reduce+ spread with a linearforloop +pushjoinEmojineighbor logic and plain-text merge semanticsVerification
yarn workspace "@rocket.chat/message-parser" buildCloses #38900
Summary by CodeRabbit