chore: Use extraction from AST to infer mentions #38845
Merged
Merged
Conversation
Contributor
|
Looks like this PR is ready to merge! 🎉 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
WalkthroughAdds a depth-first AST mention extractor, integrates it into MentionsServer with two new conversion methods, moves mention extraction to after markdown parsing in the message pipeline, and includes comprehensive unit tests for AST mention extraction. Changes
Sequence DiagramsequenceDiagram
participant MessageService as Message Service
participant MentionsServer
participant Extractor as AST Extractor
participant UserResolver as User Resolver
participant ChannelResolver as Channel Resolver
rect rgba(100, 150, 200, 0.5)
Note over MessageService,ChannelResolver: New Flow (message.md present)
MessageService->>MentionsServer: execute(message with md)
MentionsServer->>Extractor: extractMentionsFromMessageAST(ast)
Extractor-->>MentionsServer: { mentions[], channels[] }
MentionsServer->>UserResolver: convertMentionsToUsers(mentions, rid, sender)
UserResolver-->>MentionsServer: resolved user objects
MentionsServer->>ChannelResolver: convertMentionsToChannels(channels)
ChannelResolver-->>MentionsServer: resolved channel objects
MentionsServer-->>MessageService: set message.mentions, message.channels
end
rect rgba(200, 150, 100, 0.5)
Note over MessageService,ChannelResolver: Legacy Flow (no md)
MessageService->>MentionsServer: execute(message without md)
MentionsServer->>UserResolver: getUsersByMentions(...)
UserResolver-->>MentionsServer: user objects
MentionsServer->>ChannelResolver: getChannelbyMentions(...)
ChannelResolver-->>MentionsServer: channel objects
MentionsServer-->>MessageService: set message.mentions, message.channels
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
Contributor
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "02/12 22:57", "02/13 22:38", "02/16 14:04", "02/18 23:15", "02/19 23:23", "02/20 15:44", "02/20 16:10 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Fri, 20 Feb 2026 16:10:01 GMT |
ggazzo
force-pushed
the
chore/mention-parser
branch
from
94891bc to
ddf5c4f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38845 +/- ##
===========================================
+ Coverage 70.57% 70.60% +0.02%
===========================================
Files 3183 3184 +1
Lines 112499 112538 +39
Branches 20380 20426 +46
===========================================
+ Hits 79401 79458 +57
+ Misses 31044 31027 -17
+ Partials 2054 2053 -1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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 3 files
Prompt for AI agents (all 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="apps/meteor/app/mentions/server/Mentions.ts">
<violation number="1" location="apps/meteor/app/mentions/server/Mentions.ts:107">
P0: Do not extract mentions from the AST for E2EE messages, as the text is encrypted and `e2eMentions` should be used instead.</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: 3
🧹 Nitpick comments (4)
apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.ts (1)
8-8:ListItemis already included inBlocks.From the
@rocket.chat/message-parserdefinitions,Blocks = Code | Heading | Quote | SpoilerBlock | ListItem | Tasks | OrderedList | UnorderedList | LineBreak | KaTeX, soListItemin the union is redundant. TypeScript handles it gracefully, but the redundancy is worth cleaning up.♻️ Proposed cleanup
-type MessageNode = Paragraph | Blocks | Inlines | Task | ListItem | BigEmoji; +type MessageNode = Paragraph | Blocks | Inlines | Task | BigEmoji;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.ts` at line 8, The MessageNode union redundantly includes ListItem even though Blocks already contains ListItem; update the type alias MessageNode to remove the extra ListItem entry (locate the declaration "type MessageNode = Paragraph | Blocks | Inlines | Task | ListItem | BigEmoji" and change it to exclude ListItem) so the union is clean and non-redundant while keeping Blocks, Paragraph, Inlines, Task, and BigEmoji intact.apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.ts (2)
144-174: Consider adding a channel deduplication test.The suite covers user deduplication but has no parallel test for repeated
MENTION_CHANNELnodes. Adding it would round out the deduplication contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.ts` around lines 144 - 174, Add a unit test alongside the existing "should deduplicate repeated mentions" spec that constructs an AST with repeated MENTION_CHANNEL nodes (use the same Root/paragraph structure but nodes with type 'MENTION_CHANNEL' and inner PLAIN_TEXT values like 'general' repeated), call extractMentionsFromMessageAST(ast), and assert that result.channels (or the channels field returned by extractMentionsFromMessageAST) is deduplicated to a single entry (e.g., expect(result.channels).to.deep.equal(['general'])); ensure the test title clearly states it checks channel deduplication.
140-141: Redundant.to.have.lengthOf(2)assertion.
.to.have.members([...])already asserts exact membership (no more, no fewer elements), so the.to.have.lengthOf(2)on the next line is redundant.♻️ Proposed simplification
expect(result.mentions).to.have.members(['user1', 'user2']); - expect(result.mentions).to.have.lengthOf(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.ts` around lines 140 - 141, The test asserts membership with expect(result.mentions).to.have.members(['user1', 'user2']) and then redundantly checks expect(result.mentions).to.have.lengthOf(2); remove the redundant length assertion so the test relies on the single-membership assertion (keep the expect(result.mentions).to.have.members([...]) line and delete the subsequent expect(...).to.have.lengthOf(2) line) to avoid duplicate checks against result.mentions.apps/meteor/app/mentions/server/Mentions.ts (1)
66-66:for await...ofon a synchronousstring[]is unnecessary.
mentionsis a plainstring[]. Usingfor await...ofon a synchronous iterable adds microtask overhead on every iteration for no benefit. A regularfor...ofis sufficient.♻️ Proposed fix
- for await (const m of mentions) { + for (const m of mentions) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/mentions/server/Mentions.ts` at line 66, The loop in Mentions.ts uses "for await (const m of mentions)" even though mentions is a synchronous string[]; remove the unnecessary async iteration and change it to a regular synchronous loop (e.g., "for (const m of mentions)") inside the method where the loop appears so you avoid microtask overhead; ensure any inner awaits remain valid and do not rely on the removed async iteration construct (method signature need not change unless the function has no other awaits).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.tsapps/meteor/app/mentions/server/Mentions.tsapps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.tsapps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.tsapps/meteor/app/mentions/server/Mentions.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38262
File: apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts:24-33
Timestamp: 2026-01-19T18:17:46.433Z
Learning: In the Rocket.Chat repository, usernames (lastMessage.u.username) and display names (lastMessage.u.name) are sanitized/escaped centrally elsewhere in the system, so individual display functions like getMessagePreview do not need to escape these values before interpolating them into strings.
📚 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:
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.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:
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.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:
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.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:
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.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:
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.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:
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.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 clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.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/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.ts
📚 Learning: 2026-01-17T01:51:47.764Z
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.
Applied to files:
apps/meteor/app/mentions/server/Mentions.ts
🧬 Code graph analysis (3)
apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.ts (1)
packages/message-parser/src/definitions.ts (4)
Blocks(243-243)Inlines(227-241)Task(26-30)ListItem(16-20)
apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.ts (1)
apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.ts (1)
extractMentionsFromMessageAST(51-63)
apps/meteor/app/mentions/server/Mentions.ts (1)
apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.ts (1)
extractMentionsFromMessageAST(51-63)
⏰ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.ts`:
- Around line 44-48: The traversal currently skips arrays because
hasArrayValue/hasObjectValue inspect node.value incorrectly, causing mentions
inside Link.label (Markup[]) to be dropped; update the traversal logic in
traverse (and/or adjust hasArrayValue) to detect Array.isArray(node.value) and,
when true, iterate over node.value calling traverse(item, mentions, channels)
for each element (ensure this array branch runs before the object branch), so
Link.label arrays are recursively processed and mentions preserved.
In `@apps/meteor/app/mentions/server/Mentions.ts`:
- Around line 107-112: The new if (message.md) branch in execute bypasses the
E2EE fast path so E2EE messages with message.md set lose mentions; modify
execute to detect E2EE messages (use isE2EEMessage(message)) and, for E2EE
messages, use the e2eMentions path (the same fallback logic used by
getUsersByMentions/getChannelbyMentions) instead of blindly calling
convertMentionsToUsers/convertMentionsToChannels; alternatively, if you keep the
message.md branch, add a guard that if isE2EEMessage(message) is true and
e2eMentions.* exist, populate message.mentions/message.channels from
e2eMentions.e2eUserMentions and e2eMentions.e2eChannelMentions (or call the
existing helper that does this) so
convertMentionsToUsers/convertMentionsToChannels never silently discard E2EE
mention data.
- Around line 58-62: The convertMentionsToUsers signature is incorrect: change
its return type from Promise<Pick<IRoom, '_id' | 'name' | 'fname' |
'federated'>[]> to Promise<MessageMention[]> (MessageMention = { type?:
'user'|'team'; _id: string; username?: string; name?: string; fname?: string })
to match the actual returned shape built from the username array and
this.getUsers(...); also ensure getUsersByMentions()/getUsers() return types
align with IMessage['mentions']/MessageMention if necessary. In execute(), guard
the AST fast-path with isE2EEMessage(message) (skip AST extraction when the
message is E2EE so e2eMentions are used instead). Finally replace the
unnecessary for await...of over a string[] in convertMentionsToUsers with a
regular for...of loop.
---
Nitpick comments:
In `@apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.ts`:
- Line 8: The MessageNode union redundantly includes ListItem even though Blocks
already contains ListItem; update the type alias MessageNode to remove the extra
ListItem entry (locate the declaration "type MessageNode = Paragraph | Blocks |
Inlines | Task | ListItem | BigEmoji" and change it to exclude ListItem) so the
union is clean and non-redundant while keeping Blocks, Paragraph, Inlines, Task,
and BigEmoji intact.
In `@apps/meteor/app/mentions/server/Mentions.ts`:
- Line 66: The loop in Mentions.ts uses "for await (const m of mentions)" even
though mentions is a synchronous string[]; remove the unnecessary async
iteration and change it to a regular synchronous loop (e.g., "for (const m of
mentions)") inside the method where the loop appears so you avoid microtask
overhead; ensure any inner awaits remain valid and do not rely on the removed
async iteration construct (method signature need not change unless the function
has no other awaits).
In
`@apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.ts`:
- Around line 144-174: Add a unit test alongside the existing "should
deduplicate repeated mentions" spec that constructs an AST with repeated
MENTION_CHANNEL nodes (use the same Root/paragraph structure but nodes with type
'MENTION_CHANNEL' and inner PLAIN_TEXT values like 'general' repeated), call
extractMentionsFromMessageAST(ast), and assert that result.channels (or the
channels field returned by extractMentionsFromMessageAST) is deduplicated to a
single entry (e.g., expect(result.channels).to.deep.equal(['general'])); ensure
the test title clearly states it checks channel deduplication.
- Around line 140-141: The test asserts membership with
expect(result.mentions).to.have.members(['user1', 'user2']) and then redundantly
checks expect(result.mentions).to.have.lengthOf(2); remove the redundant length
assertion so the test relies on the single-membership assertion (keep the
expect(result.mentions).to.have.members([...]) line and delete the subsequent
expect(...).to.have.lengthOf(2) line) to avoid duplicate checks against
result.mentions.
Comment on lines
+58
to
+62
| async convertMentionsToUsers( | ||
| mentions: string[], | ||
| rid: string, | ||
| sender: IMessage['u'], | ||
| ): Promise<Pick<IRoom, '_id' | 'name' | 'fname' | 'federated'>[]> { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "mentions\s*\??\s*:" packages/core-typings/src/IMessage.ts -A 3 -B 1Repository: RocketChat/Rocket.Chat
Length of output: 143
🏁 Script executed:
cat -n apps/meteor/app/mentions/server/Mentions.ts | head -120Repository: RocketChat/Rocket.Chat
Length of output: 5025
🏁 Script executed:
# Find the IMessage type definition file
fd -t f "IMessage.ts" packages/Repository: RocketChat/Rocket.Chat
Length of output: 169
🏁 Script executed:
cat -n packages/core-typings/src/IMessage/IMessage.ts | grep -A 5 "mentions"Repository: RocketChat/Rocket.Chat
Length of output: 625
🏁 Script executed:
# Let's also check the full type definition around mentions
sed -n '1,200p' packages/core-typings/src/IMessage/IMessage.ts | tail -100Repository: RocketChat/Rocket.Chat
Length of output: 2281
🏁 Script executed:
# Check if MessageMention has the fname field and confirm the type structure
rg -n "export type MessageMention" packages/core-typings/src/IMessage/IMessage.ts -A 7Repository: RocketChat/Rocket.Chat
Length of output: 305
🏁 Script executed:
# Verify getUsersByMentions return type to understand the contract
rg -n "getUsersByMentions" apps/meteor/app/mentions/server/Mentions.ts -A 1 -B 1Repository: RocketChat/Rocket.Chat
Length of output: 408
🏁 Script executed:
# Check how E2EE messages are created and whether message.md is populated for them
rg -n "isE2EEMessage" apps/meteor/app/mentions/server/Mentions.ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 758
🏁 Script executed:
# Check if E2EE messages can have message.md populated
rg -n "message\.md.*=\|md.*=" apps/meteor/app/ packages/ --type ts -B 2 -A 2 | head -60Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Find where E2EE messages are created/processed to see if md gets populated
rg -n "e2e.*=.*'done'" apps/meteor/app/ --type ts -A 5 | head -40Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check the convertMentionsToChannels return type for comparison
rg -n "convertMentionsToChannels" apps/meteor/app/mentions/server/Mentions.ts -A 2Repository: RocketChat/Rocket.Chat
Length of output: 463
🏁 Script executed:
# Verify if the getUsers return can be assigned to MessageMention[]
rg -n "getUsers.*Promise" apps/meteor/app/mentions/server/Mentions.ts -A 1 -B 1Repository: RocketChat/Rocket.Chat
Length of output: 536
🏁 Script executed:
# Check the actual return statement to confirm the type mismatch
sed -n '58,90p' apps/meteor/app/mentions/server/Mentions.tsRepository: RocketChat/Rocket.Chat
Length of output: 1003
Wrong return type on convertMentionsToUsers — copy-paste from convertMentionsToChannels.
The declared return type Promise<Pick<IRoom, '_id' | 'name' | 'fname' | 'federated'>[]> is for room/channel mentions. This method actually returns user mentions: Promise<MessageMention[]> where MessageMention = { type?: 'user'|'team'; _id: string; username?: string; name?: string; fname?: string }. At line 89, the method combines { _id: string; username: string }[] with the result of this.getUsers(...) which returns { type: 'team'|'user'; _id: string; username?: string; name?: string }[]—both conform to MessageMention shape, not IRoom shape. Since getUsersByMentions() (line 47) declares its return as Promise<IMessage['mentions']>, the contract is violated.
Proposed fix
async convertMentionsToUsers(
mentions: string[],
rid: string,
sender: IMessage['u'],
-): Promise<Pick<IRoom, '_id' | 'name' | 'fname' | 'federated'>[]> {
+): Promise<MessageMention[]> {Additionally, the execute() method's AST path (lines 107–112) does not guard for E2EE messages. If an E2EE message has message.md populated, the fast path will extract mentions from the AST of encrypted content, potentially bypassing the E2EE mention data stored in e2eMentions. Confirm whether E2EE messages can have md populated; if so, add isE2EEMessage() check before using the AST path.
Minor: Line 66 uses for await...of on a synchronous string[] iterable—use regular for...of instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/mentions/server/Mentions.ts` around lines 58 - 62, The
convertMentionsToUsers signature is incorrect: change its return type from
Promise<Pick<IRoom, '_id' | 'name' | 'fname' | 'federated'>[]> to
Promise<MessageMention[]> (MessageMention = { type?: 'user'|'team'; _id: string;
username?: string; name?: string; fname?: string }) to match the actual returned
shape built from the username array and this.getUsers(...); also ensure
getUsersByMentions()/getUsers() return types align with
IMessage['mentions']/MessageMention if necessary. In execute(), guard the AST
fast-path with isE2EEMessage(message) (skip AST extraction when the message is
E2EE so e2eMentions are used instead). Finally replace the unnecessary for
await...of over a string[] in convertMentionsToUsers with a regular for...of
loop.
Comment on lines
+107
to
+112
| if (message.md) { | ||
| const { mentions, channels } = extractMentionsFromMessageAST(message.md); | ||
| message.mentions = await this.convertMentionsToUsers(mentions, message.rid, message.u); | ||
| message.channels = await this.convertMentionsToChannels(channels); | ||
| return message; | ||
| } |
Contributor
There was a problem hiding this comment.
E2EE messages bypass the e2eMentions fast path when message.md is present.
getUsersByMentions and getChannelbyMentions both guard against E2EE messages via isE2EEMessage(message) and fall back to e2eMentions.e2eUserMentions / e2eMentions.e2eChannelMentions. The new if (message.md) branch added to execute has no such guard. If an E2EE message has message.md set (the parsed encrypted text yields no usable AST nodes), convertMentionsToUsers will receive an empty mentions array, and the correct e2eMentions data is silently discarded — causing E2EE message mentions to be lost.
🐛 Proposed fix
async execute(message: IMessage) {
- if (message.md) {
+ if (message.md && !isE2EEMessage(message)) {
const { mentions, channels } = extractMentionsFromMessageAST(message.md);
message.mentions = await this.convertMentionsToUsers(mentions, message.rid, message.u);
message.channels = await this.convertMentionsToChannels(channels);
return message;
}#!/bin/bash
# Check whether E2EE messages can have message.md populated at the point execute() is called
rg -n 'isE2EEMessage\|e2eMentions\|message\.md\s*=' --type=ts -C 4 apps/meteor/app🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/mentions/server/Mentions.ts` around lines 107 - 112, The new
if (message.md) branch in execute bypasses the E2EE fast path so E2EE messages
with message.md set lose mentions; modify execute to detect E2EE messages (use
isE2EEMessage(message)) and, for E2EE messages, use the e2eMentions path (the
same fallback logic used by getUsersByMentions/getChannelbyMentions) instead of
blindly calling convertMentionsToUsers/convertMentionsToChannels; alternatively,
if you keep the message.md branch, add a guard that if isE2EEMessage(message) is
true and e2eMentions.* exist, populate message.mentions/message.channels from
e2eMentions.e2eUserMentions and e2eMentions.e2eChannelMentions (or call the
existing helper that does this) so
convertMentionsToUsers/convertMentionsToChannels never silently discard E2EE
mention data.
…hannel mentions from message AST - Implemented a new function to traverse the message AST and extract user and channel mentions. - Updated MentionsServer to utilize the new extraction function for processing mentions in messages. - Added comprehensive unit tests to ensure correct extraction of mentions from various AST structures.
ggazzo
force-pushed
the
chore/mention-parser
branch
from
ddf5c4f to
2e602ed
Compare
…per mention execution
ggazzo
force-pushed
the
chore/mention-parser
branch
from
2e602ed to
7f9213d
Compare
Contributor
There was a problem hiding this comment.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.worktrees/replies-refactorapps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.tsapps/meteor/app/mentions/server/Mentions.tsapps/meteor/server/services/messages/service.tsapps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.ts
✅ Files skipped from review due to trivial changes (1)
- .worktrees/replies-refactor
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.ts
- apps/meteor/tests/unit/app/lib/server/functions/extractMentionsFromMessageAST.spec.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:
apps/meteor/app/mentions/server/Mentions.tsapps/meteor/server/services/messages/service.ts
🧠 Learnings (5)
📚 Learning: 2026-01-17T01:51:47.764Z
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.
Applied to files:
apps/meteor/app/mentions/server/Mentions.tsapps/meteor/server/services/messages/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/mentions/server/Mentions.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/mentions/server/Mentions.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/mentions/server/Mentions.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/mentions/server/Mentions.ts
🧬 Code graph analysis (1)
apps/meteor/app/mentions/server/Mentions.ts (1)
apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.ts (1)
extractMentionsFromMessageAST(51-63)
⏰ 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: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/server/services/messages/service.ts (1)
237-243: Correct reordering of mention processing.Moving
mentionServer.executeto aftermarkdownParser.parseMarkdownand beforeparseUrlsInMessageis the right sequence:message.mdis now guaranteed to be populated whenexecuteis called, enabling the AST extraction fast-path inMentionsServer.execute.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/mentions/server/Mentions.ts`:
- Around line 58-62: The return type of convertMentionsToUsers is incorrect: it
currently declares Promise<Pick<IRoom, '_id' | 'name' | 'fname' |
'federated'>[]> but actually returns arrays of mention objects (e.g.
[...mentionsAll, ...getUsers(...)]) matching MessageMention /
IMessage['mentions']; update convertMentionsToUsers' signature to return
Promise<MessageMention[]> (or Promise<IMessage['mentions']>) and adjust any
callers (e.g., getUsersByMentions) to use that type so the propagated types
align with the actual { _id, username } | { type, _id, username?, name? } shape.
- Around line 107-112: The fast-path that uses extractMentionsFromMessageAST
overwrites E2EE mention data; update the early branch in the message.md handling
to first check isE2EEMessage(message) and, if true and message.e2eMentions
exists, populate message.mentions from message.e2eMentions.e2eUserMentions and
message.channels from message.e2eMentions.e2eChannelMentions (or defer to
getUsersByMentions/getChannelByMentions logic) instead of calling
extractMentionsFromMessageAST; otherwise (non-E2EE) continue to call
extractMentionsFromMessageAST and then
convertMentionsToUsers/convertMentionsToChannels, and ensure you do not
overwrite non-empty mention arrays with empty results.
- Line 66: The loop uses "for await (const m of mentions)" but mentions is a
synchronous string[]; change the loop to a plain synchronous "for (const m of
mentions)" in Mentions.ts (where the mentions variable and the loop are
declared) to avoid unnecessary async iteration overhead—keep any await
expressions inside the loop body as-is.
KevLehman
requested changes
Feb 20, 2026
Co-authored-by: Kevin Aleman <kaleman960@gmail.com>
KevLehman
previously approved these changes
Feb 20, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/mentions/server/Mentions.ts (1)
102-104:asynconconvertMentionsToChannelsis redundant — noawaitin body.
this.getChannels(...)already returns aPromise; theasyncwrapper just adds an unnecessary microtask tick. Drop it and return the Promise directly.♻️ Proposed fix
- async convertMentionsToChannels(channels: string[]): Promise<Pick<IRoom, '_id' | 'name' | 'fname' | 'federated'>[]> { + convertMentionsToChannels(channels: string[]): Promise<Pick<IRoom, '_id' | 'name' | 'fname' | 'federated'>[]> { return this.getChannels(channels.map((c) => (c.startsWith('#') ? c.substring(1) : c))); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/mentions/server/Mentions.ts` around lines 102 - 104, Remove the unnecessary async wrapper from convertMentionsToChannels: change the function signature for convertMentionsToChannels to a non-async function and have it directly return the Promise from this.getChannels(...) (still map channels to strip a leading '#' as before). Reference: convertMentionsToChannels and getChannels.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/mentions/server/Mentions.tsapps/meteor/server/services/messages/service.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:
apps/meteor/app/mentions/server/Mentions.tsapps/meteor/server/services/messages/service.ts
🧠 Learnings (6)
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/mentions/server/Mentions.ts
📚 Learning: 2026-01-17T01:51:47.764Z
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.
Applied to files:
apps/meteor/app/mentions/server/Mentions.tsapps/meteor/server/services/messages/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/mentions/server/Mentions.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/mentions/server/Mentions.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/mentions/server/Mentions.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/mentions/server/Mentions.ts
🧬 Code graph analysis (1)
apps/meteor/app/mentions/server/Mentions.ts (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(12-85)apps/meteor/app/lib/server/functions/extractMentionsFromMessageAST.ts (1)
extractMentionsFromMessageAST(51-63)
⏰ 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: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/server/services/messages/service.ts (1)
239-241: ReorderingmentionServer.executeto after markdown parsing is correct.
BeforeSaveMarkdownParsersetsmessage.md; executing mention extraction afterward ensures the newif (message.md)AST path inMentionsServer.executecan actually fire. The fallback (regex-based extraction) remains intact whendisableMarkdownParser=true.apps/meteor/app/mentions/server/Mentions.ts (1)
54-56: LGTM — delegation toconvertMentionsToUsers/convertMentionsToChannelsis clean.The E2EE guards in
getUsersByMentions(lines 50–53) andgetChannelbyMentions(lines 95–98) remain intact before delegating, preserving the existing behavior for the fallback path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/mentions/server/Mentions.ts`:
- Line 66: The loop is using "for await (const m of mentions)" but mentions is a
plain string[] (not an async iterable); change the loop to a synchronous
iteration: use "for (const m of mentions)" in Mentions.ts where the loop is
defined so you don't await non-async values (look for the variable mentions and
the loop that currently reads "for await (const m of mentions)"); no further
async handling is required inside that loop.
- Around line 107-112: The fast-path that handles message.md currently extracts
mentions via extractMentionsFromMessageAST and converts them with
convertMentionsToUsers/convertMentionsToChannels, but it ignores any existing
E2EE mention data; update the block so that if message.e2eMentions (or its
e2eUserMentions/e2eChannelMentions) is present you use those arrays when calling
convertMentionsToUsers and convertMentionsToChannels instead of discarding them,
otherwise fall back to extractMentionsFromMessageAST; ensure you reference
e2eMentions.e2eUserMentions and e2eMentions.e2eChannelMentions and keep the
return of message unchanged.
---
Nitpick comments:
In `@apps/meteor/app/mentions/server/Mentions.ts`:
- Around line 102-104: Remove the unnecessary async wrapper from
convertMentionsToChannels: change the function signature for
convertMentionsToChannels to a non-async function and have it directly return
the Promise from this.getChannels(...) (still map channels to strip a leading
'#' as before). Reference: convertMentionsToChannels and getChannels.
KevLehman
approved these changes
Feb 20, 2026
ggazzo
added
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
Member
Author
|
/jira ARCH-2021 |
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
2 participants
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.
This pull request introduces a new utility function to extract user and channel mentions from a message's AST and refactors the mentions handling logic in the server to use this function. It also adds comprehensive unit tests for the new extraction logic. The main goal is to improve the accuracy and maintainability of mention detection, especially for messages with rich formatting or nested structures.
The most important changes are:
Core functionality:
extractMentionsFromMessageASTfunction inextractMentionsFromMessageAST.tsto recursively traverse the message AST and extract unique user and channel mentions, supporting a wide range of message structures and formatting.Mentions server refactor:
MentionsServerclass inMentions.tsto use the new AST-based extraction function when the message contains a parsed AST (message.md). This ensures mentions are accurately detected from complex message content. [1] [2]convertMentionsToUsersandconvertMentionsToChannels) for improved clarity and maintainability. [1] [2] [3]Testing:
extractMentionsFromMessageAST, covering various scenarios including nested mentions, deduplication, special mentions, and different AST node types.Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Task: ARCH-2067