From 3eaf808582a49779ad2224d3a67c778ab8a8c23c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 3 Mar 2026 11:03:31 +0000 Subject: [PATCH] fix: prevent draft ghost bubbles and typing/reaction state leaks Three related fixes for the draft message overwrite issue: 1. preSend: explicitly dismiss draft (SendDraft with empty text) before sending the permanent message. Previously relied on sendMessage to auto-replace the draft, which fails when a user message arrives in between, leaving a ghost bubble. 2. RecordTypingStop: call old stop() before storing the new entry. Without this, preSend for message A consumes message B's typing stop, killing B's indicator prematurely. 3. RecordReactionUndo: call old undo() before storing the new entry. Same issue as typing - prevents the previous cycle's preSend from consuming the next message's reaction undo. Also clears statusEditTimes on draft dismiss to prevent stale throttle state from affecting the next processing cycle. https://claude.ai/code/session_01GeNd28Z7MtXyt2ejP4x2GW --- pkg/channels/manager.go | 31 +++++++++-- pkg/channels/manager_test.go | 102 +++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 3 deletions(-) diff --git a/pkg/channels/manager.go b/pkg/channels/manager.go index 76f207110..bf477b4f1 100644 --- a/pkg/channels/manager.go +++ b/pkg/channels/manager.go @@ -117,15 +117,35 @@ func (m *Manager) RecordPlaceholder(channel, chatID, placeholderID string) { // RecordTypingStop registers a typing stop function for later invocation. // Implements PlaceholderRecorder. +// +// If a previous typing indicator exists for the same chat, it is stopped +// immediately before the new one is recorded. This prevents the next +// preSend (which finalises the *previous* processing cycle) from +// consuming the *new* message's typing entry. func (m *Manager) RecordTypingStop(channel, chatID string, stop func()) { key := channel + ":" + chatID + if v, loaded := m.typingStops.Load(key); loaded { + if entry, ok := v.(typingEntry); ok { + entry.stop() // idempotent + } + } m.typingStops.Store(key, typingEntry{stop: stop, createdAt: time.Now()}) } // RecordReactionUndo registers a reaction undo function for later invocation. // Implements PlaceholderRecorder. +// +// If a previous reaction exists for the same chat, it is undone immediately +// before the new one is recorded. Same rationale as RecordTypingStop: the +// old entry belongs to the previous processing cycle and must not leak into +// the next preSend call. func (m *Manager) RecordReactionUndo(channel, chatID string, undo func()) { key := channel + ":" + chatID + if v, loaded := m.reactionUndos.Load(key); loaded { + if entry, ok := v.(reactionEntry); ok { + entry.undo() // idempotent + } + } m.reactionUndos.Store(key, reactionEntry{undo: undo, createdAt: time.Now()}) } @@ -149,12 +169,17 @@ func (m *Manager) preSend(ctx context.Context, name string, msg bus.OutboundMess } // 3. Try editing a tracked status message (from streaming preview). - // If the status was draft-based (draftID != 0), just clear the entry — - // the final sendMessage will automatically replace the draft bubble. + // For draft-based entries, explicitly dismiss the draft bubble before + // sending the permanent message. Without this, a user message sent + // between the last draft update and sendMessage may prevent the + // platform from auto-replacing the draft, leaving a ghost bubble. if v, loaded := m.statusMsgIDs.LoadAndDelete(key); loaded { if entry, ok := v.(statusMsgEntry); ok { if entry.draftID != 0 { - // Draft-based: sendMessage replaces the draft, no edit needed + if drafter, ok := ch.(DraftSender); ok { + _ = drafter.SendDraft(ctx, msg.ChatID, entry.draftID, "") + } + m.statusEditTimes.Delete(key) } else if entry.messageID != "" { if editor, ok := ch.(MessageEditor); ok { if err := editor.EditMessage(ctx, msg.ChatID, entry.messageID, msg.Content); err == nil { diff --git a/pkg/channels/manager_test.go b/pkg/channels/manager_test.go index 9b441d788..39c796aa3 100644 --- a/pkg/channels/manager_test.go +++ b/pkg/channels/manager_test.go @@ -1433,3 +1433,105 @@ func TestGenerateDraftID_Stable(t *testing.T) { t.Fatalf("expected different draft IDs for different keys, both got %d", id1) } } + +// TestPreSend_DismissesDraftBeforeSend verifies that preSend explicitly +// dismisses a draft-based status bubble (via SendDraft with empty text) +// before proceeding to send the permanent message. This prevents ghost +// draft bubbles when a user message arrives between the last draft update +// and the final sendMessage. +func TestPreSend_DismissesDraftBeforeSend(t *testing.T) { + m := newTestManager() + + var dismissCalled bool + var dismissContent string + + ch := &mockDraftSender{ + mockChannel: mockChannel{ + sendFn: func(_ context.Context, _ bus.OutboundMessage) error { return nil }, + }, + draftFn: func(_ context.Context, _ string, _ int, content string) error { + dismissCalled = true + dismissContent = content + return nil + }, + editFn: func(_ context.Context, _, _, _ string) error { return nil }, + sendWithID: func(_ context.Context, _, _ string) (string, error) { return "", nil }, + } + + // Store a draft-based status entry (simulates active streaming) + m.statusMsgIDs.Store("test:123", statusMsgEntry{draftID: 42, createdAt: time.Now()}) + + msg := bus.OutboundMessage{Channel: "test", ChatID: "123", Content: "final response"} + edited := m.preSend(context.Background(), "test", msg, ch) + + if edited { + t.Fatal("expected preSend to return false for draft-based status") + } + if !dismissCalled { + t.Fatal("expected preSend to call SendDraft to dismiss the draft") + } + if dismissContent != "" { + t.Fatalf("expected empty dismiss content, got %q", dismissContent) + } +} + +// TestRecordTypingStop_CleansUpOldEntry verifies that recording a new +// typing stop function calls the previous stop first. +func TestRecordTypingStop_CleansUpOldEntry(t *testing.T) { + m := newTestManager() + + var oldStopped atomic.Bool + + m.RecordTypingStop("tg", "42", func() { oldStopped.Store(true) }) + + // Record a new one — old stop should fire + m.RecordTypingStop("tg", "42", func() {}) + + if !oldStopped.Load() { + t.Fatal("expected old typing stop to be called when new entry is recorded") + } +} + +// TestRecordReactionUndo_CleansUpOldEntry verifies that recording a new +// reaction undo function calls the previous undo first. +func TestRecordReactionUndo_CleansUpOldEntry(t *testing.T) { + m := newTestManager() + + var oldUndone atomic.Bool + + m.RecordReactionUndo("tg", "42", func() { oldUndone.Store(true) }) + + // Record a new one — old undo should fire + m.RecordReactionUndo("tg", "42", func() {}) + + if !oldUndone.Load() { + t.Fatal("expected old reaction undo to be called when new entry is recorded") + } +} + +// TestPreSend_DraftDismiss_ClearsEditTimes verifies that dismissing a draft +// in preSend also clears the statusEditTimes entry for that key, preventing +// stale throttle state from affecting the next processing cycle. +func TestPreSend_DraftDismiss_ClearsEditTimes(t *testing.T) { + m := newTestManager() + + ch := &mockDraftSender{ + mockChannel: mockChannel{ + sendFn: func(_ context.Context, _ bus.OutboundMessage) error { return nil }, + }, + draftFn: func(_ context.Context, _ string, _ int, _ string) error { return nil }, + editFn: func(_ context.Context, _, _, _ string) error { return nil }, + sendWithID: func(_ context.Context, _, _ string) (string, error) { return "", nil }, + } + + key := "test:123" + m.statusMsgIDs.Store(key, statusMsgEntry{draftID: 42, createdAt: time.Now()}) + m.statusEditTimes.Store(key, time.Now()) + + msg := bus.OutboundMessage{Channel: "test", ChatID: "123", Content: "final"} + m.preSend(context.Background(), "test", msg, ch) + + if _, loaded := m.statusEditTimes.Load(key); loaded { + t.Fatal("expected statusEditTimes to be cleared after draft dismiss") + } +}