From cb8c8fee9aed42bc3a2091ecdf84a0b9a5e49b14 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 23 Jan 2026 22:29:47 +0000 Subject: [PATCH] refactor: centralize ack reaction removal --- extensions/bluebubbles/src/monitor.test.ts | 3 +- extensions/bluebubbles/src/monitor.ts | 33 ++++++------- src/channels/ack-reactions.test.ts | 49 ++++++++++++++++++- src/channels/ack-reactions.ts | 16 ++++++ .../monitor/message-handler.process.ts | 30 ++++++------ src/plugin-sdk/index.ts | 6 ++- src/plugins/runtime/index.ts | 3 +- src/plugins/runtime/types.ts | 3 ++ src/slack/monitor/message-handler/dispatch.ts | 36 ++++++++------ src/telegram/bot-message-dispatch.ts | 23 +++++---- 10 files changed, 140 insertions(+), 62 deletions(-) diff --git a/extensions/bluebubbles/src/monitor.test.ts b/extensions/bluebubbles/src/monitor.test.ts index 1901af351..c960b5c4e 100644 --- a/extensions/bluebubbles/src/monitor.test.ts +++ b/extensions/bluebubbles/src/monitor.test.ts @@ -2,7 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { IncomingMessage, ServerResponse } from "node:http"; import { EventEmitter } from "node:events"; -import { shouldAckReaction } from "clawdbot/plugin-sdk"; +import { removeAckReactionAfterReply, shouldAckReaction } from "clawdbot/plugin-sdk"; import type { ClawdbotConfig, PluginRuntime } from "clawdbot/plugin-sdk"; import { handleBlueBubblesWebhookRequest, @@ -138,6 +138,7 @@ function createMockRuntime(): PluginRuntime { }, reactions: { shouldAckReaction, + removeAckReactionAfterReply, }, groups: { resolveGroupPolicy: mockResolveGroupPolicy as unknown as PluginRuntime["channel"]["groups"]["resolveGroupPolicy"], diff --git a/extensions/bluebubbles/src/monitor.ts b/extensions/bluebubbles/src/monitor.ts index 325599c97..1a7a68058 100644 --- a/extensions/bluebubbles/src/monitor.ts +++ b/extensions/bluebubbles/src/monitor.ts @@ -1750,29 +1750,26 @@ async function processMessage( }, }); } finally { - if ( - removeAckAfterReply && - sentMessage && - ackReactionPromise && - ackReactionValue && - chatGuidForActions && - ackMessageId - ) { - void ackReactionPromise.then((didAck) => { - if (!didAck) return; - sendBlueBubblesReaction({ - chatGuid: chatGuidForActions, - messageGuid: ackMessageId, - emoji: ackReactionValue, - remove: true, - opts: { cfg: config, accountId: account.accountId }, - }).catch((err) => { + if (sentMessage && chatGuidForActions && ackMessageId) { + core.channel.reactions.removeAckReactionAfterReply({ + removeAfterReply: removeAckAfterReply, + ackReactionPromise, + ackReactionValue: ackReactionValue ?? null, + remove: () => + sendBlueBubblesReaction({ + chatGuid: chatGuidForActions, + messageGuid: ackMessageId, + emoji: ackReactionValue ?? "", + remove: true, + opts: { cfg: config, accountId: account.accountId }, + }), + onError: (err) => { logVerbose( core, runtime, `ack reaction removal failed chatGuid=${chatGuidForActions} msg=${ackMessageId}: ${String(err)}`, ); - }); + }, }); } if (chatGuidForActions && baseUrl && password && !sentMessage) { diff --git a/src/channels/ack-reactions.test.ts b/src/channels/ack-reactions.test.ts index 8be3fc323..ed018ba5a 100644 --- a/src/channels/ack-reactions.test.ts +++ b/src/channels/ack-reactions.test.ts @@ -1,6 +1,10 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; -import { shouldAckReaction, shouldAckReactionForWhatsApp } from "./ack-reactions.js"; +import { + removeAckReactionAfterReply, + shouldAckReaction, + shouldAckReactionForWhatsApp, +} from "./ack-reactions.js"; describe("shouldAckReaction", () => { it("honors direct and group-all scopes", () => { @@ -222,3 +226,44 @@ describe("shouldAckReactionForWhatsApp", () => { ).toBe(false); }); }); + +describe("removeAckReactionAfterReply", () => { + it("removes only when ack succeeded", async () => { + const remove = vi.fn().mockResolvedValue(undefined); + const onError = vi.fn(); + removeAckReactionAfterReply({ + removeAfterReply: true, + ackReactionPromise: Promise.resolve(true), + ackReactionValue: "👀", + remove, + onError, + }); + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(remove).toHaveBeenCalledTimes(1); + expect(onError).not.toHaveBeenCalled(); + }); + + it("skips removal when ack did not happen", async () => { + const remove = vi.fn().mockResolvedValue(undefined); + removeAckReactionAfterReply({ + removeAfterReply: true, + ackReactionPromise: Promise.resolve(false), + ackReactionValue: "👀", + remove, + }); + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(remove).not.toHaveBeenCalled(); + }); + + it("skips when not configured", async () => { + const remove = vi.fn().mockResolvedValue(undefined); + removeAckReactionAfterReply({ + removeAfterReply: false, + ackReactionPromise: Promise.resolve(true), + ackReactionValue: "👀", + remove, + }); + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(remove).not.toHaveBeenCalled(); + }); +}); diff --git a/src/channels/ack-reactions.ts b/src/channels/ack-reactions.ts index beeb34a47..f35ae76d8 100644 --- a/src/channels/ack-reactions.ts +++ b/src/channels/ack-reactions.ts @@ -53,3 +53,19 @@ export function shouldAckReactionForWhatsApp(params: { shouldBypassMention: params.groupActivated, }); } + +export function removeAckReactionAfterReply(params: { + removeAfterReply: boolean; + ackReactionPromise: Promise | null; + ackReactionValue: string | null; + remove: () => Promise; + onError?: (err: unknown) => void; +}) { + if (!params.removeAfterReply) return; + if (!params.ackReactionPromise) return; + if (!params.ackReactionValue) return; + void params.ackReactionPromise.then((didAck) => { + if (!didAck) return; + params.remove().catch((err) => params.onError?.(err)); + }); +} diff --git a/src/discord/monitor/message-handler.process.ts b/src/discord/monitor/message-handler.process.ts index fe26c79d5..138619e6a 100644 --- a/src/discord/monitor/message-handler.process.ts +++ b/src/discord/monitor/message-handler.process.ts @@ -8,7 +8,10 @@ import { extractShortModelName, type ResponsePrefixContext, } from "../../auto-reply/reply/response-prefix-template.js"; -import { shouldAckReaction as shouldAckReactionGate } from "../../channels/ack-reactions.js"; +import { + removeAckReactionAfterReply, + shouldAckReaction as shouldAckReactionGate, +} from "../../channels/ack-reactions.js"; import { formatInboundEnvelope, formatThreadStarterEnvelope, @@ -394,19 +397,18 @@ export async function processDiscordMessage(ctx: DiscordMessagePreflightContext) `discord: delivered ${finalCount} reply${finalCount === 1 ? "" : "ies"} to ${replyTarget}`, ); } - if (removeAckAfterReply && ackReactionPromise && ackReaction) { - const ackReactionValue = ackReaction; - void ackReactionPromise.then((didAck) => { - if (!didAck) return; - removeReactionDiscord(message.channelId, message.id, ackReactionValue, { - rest: client.rest, - }).catch((err) => { - logVerbose( - `discord: failed to remove ack reaction from ${message.channelId}/${message.id}: ${String(err)}`, - ); - }); - }); - } + removeAckReactionAfterReply({ + removeAfterReply: removeAckAfterReply, + ackReactionPromise, + ackReactionValue: ackReaction, + remove: () => + removeReactionDiscord(message.channelId, message.id, ackReaction, { rest: client.rest }), + onError: (err) => { + logVerbose( + `discord: failed to remove ack reaction from ${message.channelId}/${message.id}: ${String(err)}`, + ); + }, + }); if (isGuildMessage && historyLimit > 0) { clearHistoryEntries({ historyMap: guildHistories, diff --git a/src/plugin-sdk/index.ts b/src/plugin-sdk/index.ts index 652657b92..7cb61e2d3 100644 --- a/src/plugin-sdk/index.ts +++ b/src/plugin-sdk/index.ts @@ -122,7 +122,11 @@ export type { AckReactionScope, WhatsAppAckReactionMode, } from "../channels/ack-reactions.js"; -export { shouldAckReaction, shouldAckReactionForWhatsApp } from "../channels/ack-reactions.js"; +export { + removeAckReactionAfterReply, + shouldAckReaction, + shouldAckReactionForWhatsApp, +} from "../channels/ack-reactions.js"; export { resolveChannelMediaMaxBytes } from "../channels/plugins/media-limits.js"; export type { NormalizedLocation } from "../channels/location.js"; export { formatLocationText, toLocationContext } from "../channels/location.js"; diff --git a/src/plugins/runtime/index.ts b/src/plugins/runtime/index.ts index e0ade62ca..504d5f034 100644 --- a/src/plugins/runtime/index.ts +++ b/src/plugins/runtime/index.ts @@ -25,7 +25,7 @@ import { resolveEffectiveMessagesConfig, resolveHumanDelayConfig } from "../../a import { createMemoryGetTool, createMemorySearchTool } from "../../agents/tools/memory-tool.js"; import { handleSlackAction } from "../../agents/tools/slack-actions.js"; import { handleWhatsAppAction } from "../../agents/tools/whatsapp-actions.js"; -import { shouldAckReaction } from "../../channels/ack-reactions.js"; +import { removeAckReactionAfterReply, shouldAckReaction } from "../../channels/ack-reactions.js"; import { resolveCommandAuthorizedFromAuthorizers } from "../../channels/command-gating.js"; import { discordMessageActions } from "../../channels/plugins/actions/discord.js"; import { telegramMessageActions } from "../../channels/plugins/actions/telegram.js"; @@ -201,6 +201,7 @@ export function createPluginRuntime(): PluginRuntime { }, reactions: { shouldAckReaction, + removeAckReactionAfterReply, }, groups: { resolveGroupPolicy: resolveChannelGroupPolicy, diff --git a/src/plugins/runtime/types.ts b/src/plugins/runtime/types.ts index 86c7caa55..7351bc8da 100644 --- a/src/plugins/runtime/types.ts +++ b/src/plugins/runtime/types.ts @@ -20,6 +20,8 @@ type BuildMentionRegexes = typeof import("../../auto-reply/reply/mentions.js").b type MatchesMentionPatterns = typeof import("../../auto-reply/reply/mentions.js").matchesMentionPatterns; type ShouldAckReaction = typeof import("../../channels/ack-reactions.js").shouldAckReaction; +type RemoveAckReactionAfterReply = + typeof import("../../channels/ack-reactions.js").removeAckReactionAfterReply; type ResolveChannelGroupPolicy = typeof import("../../config/group-policy.js").resolveChannelGroupPolicy; type ResolveChannelGroupRequireMention = @@ -214,6 +216,7 @@ export type PluginRuntime = { }; reactions: { shouldAckReaction: ShouldAckReaction; + removeAckReactionAfterReply: RemoveAckReactionAfterReply; }; groups: { resolveGroupPolicy: ResolveChannelGroupPolicy; diff --git a/src/slack/monitor/message-handler/dispatch.ts b/src/slack/monitor/message-handler/dispatch.ts index 22cd9fcfe..734c7cf49 100644 --- a/src/slack/monitor/message-handler/dispatch.ts +++ b/src/slack/monitor/message-handler/dispatch.ts @@ -9,6 +9,7 @@ import { } from "../../../auto-reply/reply/response-prefix-template.js"; import { dispatchInboundMessage } from "../../../auto-reply/dispatch.js"; import { clearHistoryEntries } from "../../../auto-reply/reply/history.js"; +import { removeAckReactionAfterReply } from "../../../channels/ack-reactions.js"; import { createReplyDispatcherWithTyping } from "../../../auto-reply/reply/reply-dispatcher.js"; import { resolveStorePath, updateLastRoute } from "../../../config/sessions.js"; import { danger, logVerbose, shouldLogVerbose } from "../../../globals.js"; @@ -152,21 +153,26 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag ); } - if (ctx.removeAckAfterReply && prepared.ackReactionPromise && prepared.ackReactionMessageTs) { - const messageTs = prepared.ackReactionMessageTs; - const ackValue = prepared.ackReactionValue; - void prepared.ackReactionPromise.then((didAck) => { - if (!didAck) return; - removeSlackReaction(message.channel, messageTs, ackValue, { - token: ctx.botToken, - client: ctx.app.client, - }).catch((err) => { - logVerbose( - `slack: failed to remove ack reaction from ${message.channel}/${message.ts}: ${String(err)}`, - ); - }); - }); - } + removeAckReactionAfterReply({ + removeAfterReply: ctx.removeAckAfterReply, + ackReactionPromise: prepared.ackReactionPromise, + ackReactionValue: prepared.ackReactionValue, + remove: () => + removeSlackReaction( + message.channel, + prepared.ackReactionMessageTs ?? "", + prepared.ackReactionValue, + { + token: ctx.botToken, + client: ctx.app.client, + }, + ), + onError: (err) => { + logVerbose( + `slack: failed to remove ack reaction from ${message.channel}/${message.ts}: ${String(err)}`, + ); + }, + }); if (prepared.isRoomish && ctx.historyLimit > 0) { clearHistoryEntries({ diff --git a/src/telegram/bot-message-dispatch.ts b/src/telegram/bot-message-dispatch.ts index 4afbaa653..bce5cff82 100644 --- a/src/telegram/bot-message-dispatch.ts +++ b/src/telegram/bot-message-dispatch.ts @@ -7,6 +7,7 @@ import { import { EmbeddedBlockChunker } from "../agents/pi-embedded-block-chunker.js"; import { clearHistoryEntries } from "../auto-reply/reply/history.js"; import { dispatchReplyWithBufferedBlockDispatcher } from "../auto-reply/reply/provider-dispatcher.js"; +import { removeAckReactionAfterReply } from "../channels/ack-reactions.js"; import { danger, logVerbose } from "../globals.js"; import { resolveMarkdownTableMode } from "../config/markdown-tables.js"; import { deliverReplies } from "./bot/delivery.js"; @@ -184,16 +185,18 @@ export const dispatchTelegramMessage = async ({ } return; } - if (removeAckAfterReply && ackReactionPromise && msg.message_id && reactionApi) { - void ackReactionPromise.then((didAck) => { - if (!didAck) return; - reactionApi(chatId, msg.message_id, []).catch((err) => { - logVerbose( - `telegram: failed to remove ack reaction from ${chatId}/${msg.message_id}: ${String(err)}`, - ); - }); - }); - } + removeAckReactionAfterReply({ + removeAfterReply: removeAckAfterReply, + ackReactionPromise, + ackReactionValue: ackReactionPromise ? "ack" : null, + remove: () => reactionApi?.(chatId, msg.message_id ?? 0, []) ?? Promise.resolve(), + onError: (err) => { + if (!msg.message_id) return; + logVerbose( + `telegram: failed to remove ack reaction from ${chatId}/${msg.message_id}: ${String(err)}`, + ); + }, + }); if (isGroup && historyKey && historyLimit > 0) { clearHistoryEntries({ historyMap: groupHistories, historyKey }); }