diff --git a/src/channels/status-reactions.slack-lifecycle.test.ts b/src/channels/status-reactions.slack-lifecycle.test.ts index f2d60eb919b..7c8441d77fb 100644 --- a/src/channels/status-reactions.slack-lifecycle.test.ts +++ b/src/channels/status-reactions.slack-lifecycle.test.ts @@ -56,16 +56,16 @@ describe("Slack status reaction lifecycle", () => { void ctrl.setThinking(); await vi.advanceTimersByTimeAsync(10); expect(active.has(DEFAULT_EMOJIS.thinking)).toBe(true); - expect(active.has("eyes")).toBe(false); + expect(active.has("eyes")).toBe(true); void ctrl.setTool("web_search"); await vi.advanceTimersByTimeAsync(10); expect(active.has(DEFAULT_EMOJIS.web)).toBe(true); - expect(active.has(DEFAULT_EMOJIS.thinking)).toBe(false); + expect(active.has(DEFAULT_EMOJIS.thinking)).toBe(true); await ctrl.setDone(); expect(active.has(DEFAULT_EMOJIS.done)).toBe(true); - expect(active.has(DEFAULT_EMOJIS.web)).toBe(false); + expect(active.has(DEFAULT_EMOJIS.web)).toBe(true); await ctrl.clear(); expect(active.size).toBe(0); @@ -87,7 +87,7 @@ describe("Slack status reaction lifecycle", () => { await ctrl.setError(); expect(active.has(DEFAULT_EMOJIS.error)).toBe(true); - expect(active.has("eyes")).toBe(false); + expect(active.has("eyes")).toBe(true); await ctrl.restoreInitial(); expect(active.has("eyes")).toBe(true); @@ -117,6 +117,28 @@ describe("Slack status reaction lifecycle", () => { expect(active.has(DEFAULT_EMOJIS.stallHard)).toBe(false); }); + it("restoreInitial removes extra active reactions when current emoji is already initial", async () => { + const { adapter, active } = createSlackMockAdapter(); + const ctrl = createStatusReactionController({ + enabled: true, + adapter, + initialEmoji: "eyes", + timing: { debounceMs: 0, stallSoftMs: 99999, stallHardMs: 99999 }, + }); + + void ctrl.setThinking(); + await vi.advanceTimersByTimeAsync(10); + void ctrl.setQueued(); + await vi.advanceTimersByTimeAsync(10); + expect(active.has(DEFAULT_EMOJIS.thinking)).toBe(true); + expect(active.has("eyes")).toBe(true); + + await ctrl.restoreInitial(); + + expect(active.has("eyes")).toBe(true); + expect(active.has(DEFAULT_EMOJIS.thinking)).toBe(false); + }); + it("restoreInitial still applies initial emoji when it is only debounced", async () => { const { adapter, active } = createSlackMockAdapter(); const ctrl = createStatusReactionController({ @@ -134,14 +156,14 @@ describe("Slack status reaction lifecycle", () => { void ctrl.setTool("web_search"); await vi.advanceTimersByTimeAsync(25); expect(active.has(DEFAULT_EMOJIS.web)).toBe(true); - expect(active.has("eyes")).toBe(false); + expect(active.has("eyes")).toBe(true); void ctrl.setThinking(); await ctrl.restoreInitial(); expect(active.has("eyes")).toBe(true); expect(active.has(DEFAULT_EMOJIS.web)).toBe(false); - expect(adapter.setReaction).toHaveBeenCalledTimes(3); + expect(adapter.setReaction).toHaveBeenCalledTimes(2); }); it("restoreInitial re-applies initial emoji after an in-flight debounced transition", async () => { @@ -212,6 +234,6 @@ describe("Slack status reaction lifecycle", () => { void ctrl.setTool("exec"); await vi.advanceTimersByTimeAsync(10); expect(active.has(DEFAULT_EMOJIS.coding)).toBe(true); - expect(active.has("eyes")).toBe(false); + expect(active.has("eyes")).toBe(true); }); }); diff --git a/src/channels/status-reactions.test.ts b/src/channels/status-reactions.test.ts index 88da29669f5..199ead38767 100644 --- a/src/channels/status-reactions.test.ts +++ b/src/channels/status-reactions.test.ts @@ -277,7 +277,7 @@ describe("createStatusReactionController", () => { expect(setEmojis).toEqual([DEFAULT_EMOJIS.thinking]); }); - it("should call removeReaction when adapter supports it and emoji changes", async () => { + it("should defer removing previous emojis until clear", async () => { const { calls, controller } = createEnabledController(); void controller.setQueued(); @@ -286,9 +286,29 @@ describe("createStatusReactionController", () => { void controller.setThinking(); await vi.advanceTimersByTimeAsync(DEFAULT_TIMING.debounceMs); - // Should set thinking, then remove queued expectSetEmojiCall(calls, DEFAULT_EMOJIS.thinking); + expect(calls).not.toContainEqual({ method: "remove", emoji: "👀" }); + + await controller.clear(); expect(calls).toContainEqual({ method: "remove", emoji: "👀" }); + expect(calls).toContainEqual({ method: "remove", emoji: DEFAULT_EMOJIS.thinking }); + }); + + it("should not re-add an already active reaction when returning to it", async () => { + const { calls, controller } = createEnabledController(); + + void controller.setThinking(); + await vi.advanceTimersByTimeAsync(DEFAULT_TIMING.debounceMs); + void controller.setTool("web_search"); + await vi.advanceTimersByTimeAsync(DEFAULT_TIMING.debounceMs); + void controller.setThinking(); + await vi.advanceTimersByTimeAsync(DEFAULT_TIMING.debounceMs); + + const thinkingSets = calls.filter( + (call) => call.method === "set" && call.emoji === DEFAULT_EMOJIS.thinking, + ); + expect(thinkingSets).toHaveLength(1); + expect(calls).not.toContainEqual({ method: "remove", emoji: DEFAULT_EMOJIS.thinking }); }); it("should only call setReaction when adapter lacks removeReaction", async () => { diff --git a/src/channels/status-reactions.ts b/src/channels/status-reactions.ts index 2b553aa366a..1426d8de708 100644 --- a/src/channels/status-reactions.ts +++ b/src/channels/status-reactions.ts @@ -125,6 +125,8 @@ export function resolveToolEmoji( * - Debouncing (intermediate states debounce, terminal states are immediate) * - Stall timers (soft/hard warnings on inactivity) * - Terminal state protection (done/error mark finished, subsequent updates ignored) + * - Defers reaction removals until final cleanup to avoid visible flicker on + * platforms without atomic reaction replacement */ export function createStatusReactionController(params: { enabled: boolean; @@ -156,6 +158,7 @@ export function createStatusReactionController(params: { let stallHardTimer: NodeJS.Timeout | null = null; let finished = false; let chainPromise = Promise.resolve(); + const activeEmojis = new Set(); // Known emojis for clear operation const knownEmojis = new Set([ @@ -228,8 +231,29 @@ export function createStatusReactionController(params: { }, timing.stallHardMs); } + async function removeKnownEmojis(options: { keepEmoji?: string } = {}): Promise { + if (!adapter.removeReaction) { + return; + } + + for (const emoji of knownEmojis) { + if (emoji === options.keepEmoji) { + continue; + } + try { + await adapter.removeReaction(emoji); + } catch (err) { + if (onError) { + onError(err); + } + } finally { + activeEmojis.delete(emoji); + } + } + } + /** - * Apply an emoji: set new reaction and optionally remove old one. + * Apply an emoji while keeping previous active-loop reactions visible. */ async function applyEmoji(newEmoji: string): Promise { if (!enabled) { @@ -237,14 +261,11 @@ export function createStatusReactionController(params: { } try { - const previousEmoji = currentEmoji; - await adapter.setReaction(newEmoji); - - // If adapter supports removeReaction and there's a different previous emoji, remove it - if (adapter.removeReaction && previousEmoji && previousEmoji !== newEmoji) { - await adapter.removeReaction(previousEmoji); + if (!adapter.removeReaction || !activeEmojis.has(newEmoji)) { + await adapter.setReaction(newEmoji); } + activeEmojis.add(newEmoji); currentEmoji = newEmoji; } catch (err) { if (onError) { @@ -357,17 +378,7 @@ export function createStatusReactionController(params: { await enqueue(async () => { if (adapter.removeReaction) { - // Remove all known emojis (Discord-style) - const emojisToRemove = Array.from(knownEmojis); - for (const emoji of emojisToRemove) { - try { - await adapter.removeReaction(emoji); - } catch (err) { - if (onError) { - onError(err); - } - } - } + await removeKnownEmojis(); } else { // For platforms without removeReaction, set empty or just skip // (Telegram handles this atomically on the next setReaction) @@ -385,8 +396,9 @@ export function createStatusReactionController(params: { const alreadyInitial = currentEmoji === initialEmoji; const pendingBeforeClear = pendingEmoji; const hadDebouncedPending = debounceTimer !== null; + const hasExtraActiveEmoji = Array.from(activeEmojis).some((emoji) => emoji !== initialEmoji); clearAllTimers(); - if (alreadyInitial && (!pendingBeforeClear || hadDebouncedPending)) { + if (alreadyInitial && (!pendingBeforeClear || hadDebouncedPending) && !hasExtraActiveEmoji) { pendingEmoji = ""; return; } @@ -397,6 +409,7 @@ export function createStatusReactionController(params: { await enqueue(async () => { await applyEmoji(initialEmoji); + await removeKnownEmojis({ keepEmoji: initialEmoji }); pendingEmoji = ""; }); }