Defer status reaction cleanup

This commit is contained in:
Peter Steinberger
2026-05-01 11:22:41 +01:00
parent bf7ac8d8c4
commit b3e6f8c5ee
3 changed files with 83 additions and 28 deletions

View File

@@ -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);
});
});

View File

@@ -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 () => {

View File

@@ -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<string>();
// Known emojis for clear operation
const knownEmojis = new Set<string>([
@@ -228,8 +231,29 @@ export function createStatusReactionController(params: {
}, timing.stallHardMs);
}
async function removeKnownEmojis(options: { keepEmoji?: string } = {}): Promise<void> {
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<void> {
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 = "";
});
}