From 594c805f47ac6eb79c21a52434ca4eb63a48669e Mon Sep 17 00:00:00 2001 From: dj-oyu <68707227+dj-oyu@users.noreply.github.com> Date: Thu, 19 Mar 2026 19:26:35 +0900 Subject: [PATCH] fix: update draft with final content in preSend instead of dismissing The draft path in preSend was dismissing drafts with empty text, leaving ghost bubbles visible in Telegram. Now updates the draft with final content (matching handleTaskStatusSend pattern), deletes orphan placeholders, and cleans up placeholder map on messageID edit success. Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/channels/interfaces_ext.go | 5 + pkg/channels/manager.go | 22 ++++- pkg/channels/manager_ext_test.go | 135 ++++++++++++++++++++------ pkg/channels/telegram/telegram_ext.go | 23 +++++ 4 files changed, 150 insertions(+), 35 deletions(-) diff --git a/pkg/channels/interfaces_ext.go b/pkg/channels/interfaces_ext.go index 647d7cc64..929aeded6 100644 --- a/pkg/channels/interfaces_ext.go +++ b/pkg/channels/interfaces_ext.go @@ -14,3 +14,8 @@ type MessageSenderWithID interface { type DraftSender interface { SendDraft(ctx context.Context, chatID string, draftID int, content string) error } + +// MessageDeleter — channels that can delete a previously sent message. +type MessageDeleter interface { + DeleteMessage(ctx context.Context, chatID string, messageID string) error +} diff --git a/pkg/channels/manager.go b/pkg/channels/manager.go index fe22903ca..9ab0a53e5 100644 --- a/pkg/channels/manager.go +++ b/pkg/channels/manager.go @@ -165,20 +165,32 @@ func (m *Manager) preSend(ctx context.Context, name string, msg bus.OutboundMess } // 3. Try editing a tracked status message (from streaming preview). - // 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. + // For draft-based entries, update the draft with the final content so + // the draft bubble becomes the permanent response. If a placeholder + // also exists, delete it to avoid orphan "Thinking…" messages. if v, loaded := m.statusMsgIDs.LoadAndDelete(key); loaded { if entry, ok := v.(statusMsgEntry); ok { if entry.draftID != 0 { if drafter, ok := ch.(DraftSender); ok { - _ = drafter.SendDraft(ctx, msg.ChatID, entry.draftID, "") + if err := drafter.SendDraft(ctx, msg.ChatID, entry.draftID, msg.Content); err == nil { + m.statusEditTimes.Delete(key) + // Draft displays the final content; delete orphan placeholder. + if v, loaded := m.placeholders.LoadAndDelete(key); loaded { + if phEntry, ok := v.(placeholderEntry); ok && phEntry.id != "" { + if deleter, ok := ch.(MessageDeleter); ok { + _ = deleter.DeleteMessage(ctx, msg.ChatID, phEntry.id) + } + } + } + return true + } } m.statusEditTimes.Delete(key) + // Draft update failed → fall through to placeholder path. } else if entry.messageID != "" { if editor, ok := ch.(MessageEditor); ok { if err := editor.EditMessage(ctx, msg.ChatID, entry.messageID, msg.Content); err == nil { + m.placeholders.Delete(key) return true // edited successfully, skip Send } } diff --git a/pkg/channels/manager_ext_test.go b/pkg/channels/manager_ext_test.go index b54c6ec6a..7f7aec7d2 100644 --- a/pkg/channels/manager_ext_test.go +++ b/pkg/channels/manager_ext_test.go @@ -783,29 +783,22 @@ func TestGenerateDraftID_Stable(t *testing.T) { } } -// 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. -// 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) { +// TestPreSend_DraftPath_UpdatesWithFinalContent verifies that preSend updates +// the draft bubble with the final content (instead of dismissing with empty text), +// and returns true so no separate Send is performed. +func TestPreSend_DraftPath_UpdatesWithFinalContent(t *testing.T) { m := newTestManager() - var dismissCalled bool - var dismissContent string + var draftCalled bool + var draftContent 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 + draftCalled = true + draftContent = content return nil }, editFn: func(_ context.Context, _, _, _ string) error { return nil }, @@ -817,14 +810,14 @@ func TestPreSend_DismissesDraftBeforeSend(t *testing.T) { 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 !edited { + t.Fatal("expected preSend to return true when draft is updated with final content") } - if !dismissCalled { - t.Fatal("expected preSend to call SendDraft to dismiss the draft") + if !draftCalled { + t.Fatal("expected preSend to call SendDraft with final content") } - if dismissContent != "" { - t.Fatalf("expected empty dismiss content, got %q", dismissContent) + if draftContent != "final response" { + t.Fatalf("expected draft content 'final response', got %q", draftContent) } } @@ -864,13 +857,10 @@ func TestRecordReactionUndo_CleansUpOldEntry(t *testing.T) { } } -// 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. -// 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) { +// TestPreSend_DraftUpdate_ClearsEditTimes verifies that updating a draft +// with final content in preSend also clears the statusEditTimes entry for +// that key, preventing stale throttle state from affecting the next cycle. +func TestPreSend_DraftUpdate_ClearsEditTimes(t *testing.T) { m := newTestManager() ch := &mockDraftSender{ @@ -887,9 +877,94 @@ func TestPreSend_DraftDismiss_ClearsEditTimes(t *testing.T) { m.statusEditTimes.Store(key, time.Now()) msg := bus.OutboundMessage{Channel: "test", ChatID: "123", Content: "final"} - m.preSend(context.Background(), "test", msg, ch) + edited := m.preSend(context.Background(), "test", msg, ch) + if !edited { + t.Fatal("expected preSend to return true when draft updated") + } if _, loaded := m.statusEditTimes.Load(key); loaded { - t.Fatal("expected statusEditTimes to be cleared after draft dismiss") + t.Fatal("expected statusEditTimes to be cleared after draft update") + } +} + +// mockDraftSenderWithDelete implements DraftSender + MessageDeleter + MessageEditor. +type mockDraftSenderWithDelete struct { + mockDraftSender + deleteFn func(ctx context.Context, chatID, messageID string) error +} + +func (m *mockDraftSenderWithDelete) DeleteMessage(ctx context.Context, chatID, messageID string) error { + return m.deleteFn(ctx, chatID, messageID) +} + +// TestPreSend_DraftPath_DeletesPlaceholder verifies that when the draft path +// succeeds, any orphan placeholder message is deleted via DeleteMessage. +func TestPreSend_DraftPath_DeletesPlaceholder(t *testing.T) { + m := newTestManager() + + var deleteCalled bool + var deletedMsgID string + + ch := &mockDraftSenderWithDelete{ + mockDraftSender: 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 }, + }, + deleteFn: func(_ context.Context, _, messageID string) error { + deleteCalled = true + deletedMsgID = messageID + return nil + }, + } + + key := "test:123" + m.statusMsgIDs.Store(key, statusMsgEntry{draftID: 42, createdAt: time.Now()}) + m.RecordPlaceholder("test", "123", "ph-99") + + msg := bus.OutboundMessage{Channel: "test", ChatID: "123", Content: "final"} + edited := m.preSend(context.Background(), "test", msg, ch) + + if !edited { + t.Fatal("expected preSend to return true") + } + if !deleteCalled { + t.Fatal("expected DeleteMessage to be called for orphan placeholder") + } + if deletedMsgID != "ph-99" { + t.Fatalf("expected deleted message ID ph-99, got %s", deletedMsgID) + } + if _, loaded := m.placeholders.Load(key); loaded { + t.Fatal("expected placeholder to be removed from map") + } +} + +// TestPreSend_MessageIDPath_CleansPlaceholder verifies that when the messageID +// edit path succeeds, the placeholder map entry is also cleaned up. +func TestPreSend_MessageIDPath_CleansPlaceholder(t *testing.T) { + m := newTestManager() + + ch := &mockMessageEditor{ + mockChannel: mockChannel{ + sendFn: func(_ context.Context, _ bus.OutboundMessage) error { return nil }, + }, + editFn: func(_ context.Context, _, _, _ string) error { return nil }, + } + + key := "test:123" + m.statusMsgIDs.Store(key, statusMsgEntry{messageID: "status-77", createdAt: time.Now()}) + m.RecordPlaceholder("test", "123", "ph-leftover") + + 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 true (status message edited)") + } + if _, loaded := m.placeholders.Load(key); loaded { + t.Fatal("expected placeholder to be cleaned up after messageID edit") } } diff --git a/pkg/channels/telegram/telegram_ext.go b/pkg/channels/telegram/telegram_ext.go index 29ec48179..69572dec1 100644 --- a/pkg/channels/telegram/telegram_ext.go +++ b/pkg/channels/telegram/telegram_ext.go @@ -71,6 +71,29 @@ func (c *TelegramChannel) SendDraft(ctx context.Context, chatID string, draftID return nil } +// DeleteMessage implements channels.MessageDeleter. +// It deletes a previously sent message by its platform message ID. +func (c *TelegramChannel) DeleteMessage(ctx context.Context, chatID string, messageID string) error { + if !c.IsRunning() { + return channels.ErrNotRunning + } + + cid, _, err := parseTelegramChatID(chatID) + if err != nil { + return fmt.Errorf("invalid chat ID %s: %w", chatID, channels.ErrSendFailed) + } + + var mid int + if _, scanErr := fmt.Sscanf(messageID, "%d", &mid); scanErr != nil { + return fmt.Errorf("invalid message ID %s: %w", messageID, channels.ErrSendFailed) + } + + return c.bot.DeleteMessage(ctx, &telego.DeleteMessageParams{ + ChatID: telego.ChatID{ID: cid}, + MessageID: mid, + }) +} + // formatChatID formats a chat ID with optional thread ID as "chatID/threadID". func formatChatID(chatID int64, threadID int) string { if threadID != 0 {