fix: multiple message handling improvements#1
fix: multiple message handling improvements#12-A-M wants to merge 1 commit intoelizaos-plugins:nextfrom
Conversation
1. Entity creation: create user entity before createMemory() to prevent foreign key violations in handleMessage() and handleAppMention() 2. Race condition: skip @mentions in handleMessage() for non-DM channels since handleAppMention() already processes those, preventing duplicate responses that get discarded 3. Empty response guard: check response text before calling sendMessage() to prevent Slack no_text API errors when model returns empty 4. mentionContext: set isMention flag in buildMemoryFromMention() so the runtime auto-responds to @mentions without LLM shouldRespond evaluation 5. Markdown conversion: call markdownToSlackMrkdwn() in sendMessage() to properly convert **bold**, *italic*, ~~strike~~, [links](url) and ## headings to Slack mrkdwn format
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
standujar
left a comment
standujar
left a comment
There was a problem hiding this comment.
Hello!
Thanks for this contribution :)
![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
🧹 Nitpick comments (2)
typescript/src/service.ts (2)
354-364: Extract duplicated entity-bootstrap code into a helper.The same entity creation block appears in both handlers; centralizing it reduces drift and keeps metadata shape consistent.
♻️ Proposed refactor
+ private async ensureSlackEntityExists(entityId: UUID, slackUserId: string): Promise<void> { + const existingEntity = await this.runtime.getEntityById(entityId); + if (existingEntity) return; + + const user = await this.getUser(slackUserId); + const displayName = user ? getSlackUserDisplayName(user) : slackUserId; + + await this.runtime.createEntity({ + id: entityId, + names: [displayName], + metadata: { slack: { id: slackUserId, name: displayName, userName: user?.name || slackUserId } }, + agentId: this.runtime.agentId, + }); + }- const existingEntity = await this.runtime.getEntityById(memory.entityId); - if (!existingEntity) { - const user = await this.getUser(message.user); - const displayName = user ? getSlackUserDisplayName(user) : message.user; - await this.runtime.createEntity({ - id: memory.entityId, - names: [displayName], - metadata: { slack: { id: message.user, name: displayName, userName: user?.name || message.user } }, - agentId: this.runtime.agentId, - }); - } + if (!message.user) return; + await this.ensureSlackEntityExists(memory.entityId, message.user);- const existingEntity = await this.runtime.getEntityById(memory.entityId); - if (!existingEntity) { - const user = await this.getUser(event.user); - const displayName = user ? getSlackUserDisplayName(user) : event.user; - await this.runtime.createEntity({ - id: memory.entityId, - names: [displayName], - metadata: { slack: { id: event.user, name: displayName, userName: user?.name || event.user } }, - agentId: this.runtime.agentId, - }); - } + await this.ensureSlackEntityExists(memory.entityId, event.user);Also applies to: 407-417
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/src/service.ts` around lines 354 - 364, Extract the duplicated entity-bootstrap logic into a single helper (e.g., ensureEntityExists or bootstrapEntity) on the same class that currently calls runtime.getEntityById and runtime.createEntity; the helper should accept the memory.entityId and message.user (or the message object) and perform: await this.runtime.getEntityById(id), if missing call await this.getUser(message.user), compute displayName via getSlackUserDisplayName(user) or fallback to message.user, then call this.runtime.createEntity with the same metadata shape (id, names, metadata: { slack: { id: message.user, name: displayName, userName: user?.name || message.user } }, agentId: this.runtime.agentId). Replace the duplicate blocks in both handlers (the block using memory.entityId/message.user at runtime.getEntityById/createEntity) with a call to the new helper to ensure a single source of truth for entity bootstrapping.
920-921: Add an outbound empty-text check insendMessageitself.Guarding only in
processAgentMessageleaves othersendMessagecallers unprotected; a boundary-level check is safer.🛡️ Proposed hardening
- const convertedText = markdownToSlackMrkdwn(text); - const messages = this.splitMessage(convertedText); + const convertedText = markdownToSlackMrkdwn(text).trim(); + if (!convertedText) { + throw new Error("Cannot send an empty Slack message"); + } + const messages = this.splitMessage(convertedText).filter( + (msg) => msg.trim().length > 0, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/src/service.ts` around lines 920 - 921, Add a boundary-level guard in sendMessage to early-return when there is no outbound content: inside the sendMessage function (not just processAgentMessage), check the incoming text and the convertedText (after calling markdownToSlackMrkdwn) for null/undefined or text.trim() === "" and return (or no-op) before calling this.splitMessage or any downstream sending logic; update sendMessage (and references to markdownToSlackMrkdwn and splitMessage within it) so other callers are protected by this single-source check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@typescript/src/service.ts`:
- Around line 332-335: The current check skips mentions whenever isMentioned is
true even if message.channel_type is undefined; update the condition so we only
skip when channel_type exists and is not 'im' (e.g., change to: if (isMentioned
&& message.channel_type != null && message.channel_type !== 'im') return;),
referencing the isMentioned flag and message.channel_type to ensure RTM-style
messages (no channel_type) are not treated as channel mentions.
---
Nitpick comments:
In `@typescript/src/service.ts`:
- Around line 354-364: Extract the duplicated entity-bootstrap logic into a
single helper (e.g., ensureEntityExists or bootstrapEntity) on the same class
that currently calls runtime.getEntityById and runtime.createEntity; the helper
should accept the memory.entityId and message.user (or the message object) and
perform: await this.runtime.getEntityById(id), if missing call await
this.getUser(message.user), compute displayName via
getSlackUserDisplayName(user) or fallback to message.user, then call
this.runtime.createEntity with the same metadata shape (id, names, metadata: {
slack: { id: message.user, name: displayName, userName: user?.name ||
message.user } }, agentId: this.runtime.agentId). Replace the duplicate blocks
in both handlers (the block using memory.entityId/message.user at
runtime.getEntityById/createEntity) with a call to the new helper to ensure a
single source of truth for entity bootstrapping.
- Around line 920-921: Add a boundary-level guard in sendMessage to early-return
when there is no outbound content: inside the sendMessage function (not just
processAgentMessage), check the incoming text and the convertedText (after
calling markdownToSlackMrkdwn) for null/undefined or text.trim() === "" and
return (or no-op) before calling this.splitMessage or any downstream sending
logic; update sendMessage (and references to markdownToSlackMrkdwn and
splitMessage within it) so other callers are protected by this single-source
check.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
typescript/src/service.ts
| // Skip @mentions in channels — handleAppMention handles those | ||
| if (isMentioned && message.channel_type !== 'im') { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the code context around lines 332-335
head -340 typescript/src/service.ts | tail -20Repository: elizaos-plugins/plugin-slack
Length of output: 675
🏁 Script executed:
# Let's also check the type definitions for the message object
find . -name "*.ts" -o -name "*.d.ts" | head -20Repository: elizaos-plugins/plugin-slack
Length of output: 813
🏁 Script executed:
# Find message type definitions
rg "interface.*message|type.*message" -t ts -A 5 | head -100Repository: elizaos-plugins/plugin-slack
Length of output: 6906
🌐 Web query:
Slack Bolt 4.1.0 message event channel_type property optional required
💡 Result:
In Bolt for JavaScript v4.1.0, the TypeScript event types come from @slack/types, where channel_type is defined as a required (non-optional) field on message events such as GenericMessageEvent (and also BotMessageEvent). [2]
From Slack’s Events API docs, message.* events (e.g., message.channels) include channel_type in the event payload and Slack explicitly tells you to use it to differentiate the message.* variants. [1]
However, the RTM “message” event (and message objects returned by some Web API methods like history) may not include channel_type—Slack’s RTM message event example omits it. [3]
So:
- Events API / Bolt event subscriptions (
message.channels,message.im, etc.):channel_typeis expected/present (treated as required by typings). [1][2] - RTM-style
messageevents / other message objects:channel_typecan be absent. [3]
Line 333 drops valid mentions when channel_type is missing.
The type definition shows channel_type?: string; is optional. Since undefined !== 'im' evaluates to true, mentions are skipped unintentionally when channel_type is absent (e.g., in RTM-style messages), even though the code intent is to skip only mentions in channels, not DMs.
Proposed fix
- if (isMentioned && message.channel_type !== 'im') {
+ if (isMentioned && message.channel_type && message.channel_type !== 'im') {
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@typescript/src/service.ts` around lines 332 - 335, The current check skips
mentions whenever isMentioned is true even if message.channel_type is undefined;
update the condition so we only skip when channel_type exists and is not 'im'
(e.g., change to: if (isMentioned && message.channel_type != null &&
message.channel_type !== 'im') return;), referencing the isMentioned flag and
message.channel_type to ensure RTM-style messages (no channel_type) are not
treated as channel mentions.
2 participants
Summary
Fixes 5 bugs found during Slack connector testing:
Test plan
Summary by CodeRabbit
Bug Fixes
Improvements