fix: centralize draft preview finalization

2026-05-06 19:20:43 +00:00 · 2026-04-22 02:28:58 +01:00
parent ffef84dea7
commit fb9a21ae8f
33 changed files with 824 additions and 195 deletions
--- a/extensions/slack/src/draft-stream.test.ts
+++ b/extensions/slack/src/draft-stream.test.ts
@@ -132,6 +132,22 @@ describe("createSlackDraftStream", () => {
    expect(stream.channelId()).toBeUndefined();
  });

+  it("discardPending stops late updates without deleting the visible preview", async () => {
+    const { stream, send, edit, remove } = createDraftStreamHarness();
+
+    stream.update("hello");
+    await stream.flush();
+    await stream.discardPending();
+    stream.update("late");
+    await stream.flush();
+
+    expect(send).toHaveBeenCalledTimes(1);
+    expect(edit).not.toHaveBeenCalled();
+    expect(remove).not.toHaveBeenCalled();
+    expect(stream.messageId()).toBe("111.222");
+    expect(stream.channelId()).toBe("C123");
+  });
+
  it("clear is a no-op when no preview message exists", async () => {
    const { stream, remove } = createDraftStreamHarness();

--- a/extensions/slack/src/draft-stream.ts
+++ b/extensions/slack/src/draft-stream.ts
@@ -10,6 +10,8 @@ export type SlackDraftStream = {
  update: (text: string) => void;
  flush: () => Promise<void>;
  clear: () => Promise<void>;
+  discardPending: () => Promise<void>;
+  seal: () => Promise<void>;
  stop: () => void;
  forceNewMessage: () => void;
  messageId: () => string | undefined;
@@ -95,9 +97,13 @@ export function createSlackDraftStream(params: {
    loop.stop();
  };

-  const clear = async () => {
+  const discardPending = async () => {
    stop();
    await loop.waitForInFlight();
+  };
+
+  const clear = async () => {
+    await discardPending();
    const channelId = streamChannelId;
    const messageId = streamMessageId;
    streamChannelId = undefined;
@@ -129,6 +135,8 @@ export function createSlackDraftStream(params: {
    update: loop.update,
    flush: loop.flush,
    clear,
+    discardPending,
+    seal: discardPending,
    stop,
    forceNewMessage,
    messageId: () => streamMessageId,
--- a/extensions/slack/src/monitor/message-handler/dispatch.preview-fallback.test.ts
+++ b/extensions/slack/src/monitor/message-handler/dispatch.preview-fallback.test.ts
@@ -9,7 +9,7 @@ const deliverRepliesMock = vi.fn(async () => {});
 const finalizeSlackPreviewEditMock = vi.fn(async () => {});
 let mockedDispatchSequence: Array<{
  kind: "tool" | "block" | "final";
-  payload: { text: string };
+  payload: { text: string; isError?: boolean; mediaUrl?: string; mediaUrls?: string[] };
 }> = [];

 const noop = () => {};
@@ -20,6 +20,8 @@ function createDraftStreamStub() {
    update: noop,
    flush: noopAsync,
    clear: noopAsync,
+    discardPending: noopAsync,
+    seal: noopAsync,
    stop: noop,
    forceNewMessage: noop,
    messageId: () => "171234.567",
@@ -213,6 +215,7 @@ vi.mock("../config.runtime.js", () => ({

 vi.mock("../replies.js", () => ({
  createSlackReplyDeliveryPlan: () => ({
+    peekThreadTs: () => THREAD_TS,
    nextThreadTs: () => THREAD_TS,
    markSent: () => {},
  }),
@@ -234,7 +237,7 @@ vi.mock("../reply.runtime.js", () => ({
  dispatchInboundMessage: async (params: {
    dispatcher: {
      deliver: (
-        payload: { text: string },
+        payload: { text: string; isError?: boolean; mediaUrl?: string; mediaUrls?: string[] },
        info: { kind: "tool" | "block" | "final" },
      ) => Promise<void>;
    };
@@ -293,7 +296,7 @@ describe("dispatchPreparedSlackMessage preview fallback", () => {

    await dispatchPreparedSlackMessage(createPreparedSlackMessage());

-    expect(finalizeSlackPreviewEditMock).toHaveBeenCalledTimes(2);
+    expect(finalizeSlackPreviewEditMock).toHaveBeenCalledTimes(1);
    expect(deliverRepliesMock).toHaveBeenCalledTimes(2);
    expect(deliverRepliesMock).toHaveBeenNthCalledWith(
      1,
@@ -310,4 +313,54 @@ describe("dispatchPreparedSlackMessage preview fallback", () => {
      }),
    );
  });
+
+  it("does not flush draft previews for media finals before normal delivery", async () => {
+    const draftStream = {
+      ...createDraftStreamStub(),
+      flush: vi.fn(noopAsync),
+      clear: vi.fn(noopAsync),
+      discardPending: vi.fn(noopAsync),
+      seal: vi.fn(noopAsync),
+    };
+    createSlackDraftStreamMock.mockReturnValueOnce(draftStream);
+    mockedDispatchSequence = [
+      {
+        kind: "final",
+        payload: { text: "Photo", mediaUrl: "https://example.com/a.png" },
+      },
+    ];
+
+    await dispatchPreparedSlackMessage(createPreparedSlackMessage());
+
+    expect(draftStream.flush).not.toHaveBeenCalled();
+    expect(draftStream.discardPending).toHaveBeenCalled();
+    expect(draftStream.clear).toHaveBeenCalledTimes(1);
+    expect(finalizeSlackPreviewEditMock).not.toHaveBeenCalled();
+    expect(deliverRepliesMock).toHaveBeenCalledTimes(1);
+  });
+
+  it("does not flush draft previews for error finals before normal delivery", async () => {
+    const draftStream = {
+      ...createDraftStreamStub(),
+      flush: vi.fn(noopAsync),
+      clear: vi.fn(noopAsync),
+      discardPending: vi.fn(noopAsync),
+      seal: vi.fn(noopAsync),
+    };
+    createSlackDraftStreamMock.mockReturnValueOnce(draftStream);
+    mockedDispatchSequence = [
+      {
+        kind: "final",
+        payload: { text: "Something failed", isError: true },
+      },
+    ];
+
+    await dispatchPreparedSlackMessage(createPreparedSlackMessage());
+
+    expect(draftStream.flush).not.toHaveBeenCalled();
+    expect(draftStream.discardPending).toHaveBeenCalled();
+    expect(draftStream.clear).toHaveBeenCalledTimes(1);
+    expect(finalizeSlackPreviewEditMock).not.toHaveBeenCalled();
+    expect(deliverRepliesMock).toHaveBeenCalledTimes(1);
+  });
 });
--- a/extensions/slack/src/monitor/message-handler/dispatch.ts
+++ b/extensions/slack/src/monitor/message-handler/dispatch.ts
@@ -7,6 +7,7 @@ import {
  removeAckReactionAfterReply,
  type StatusReactionAdapter,
 } from "openclaw/plugin-sdk/channel-feedback";
+import { deliverFinalizableDraftPreview } from "openclaw/plugin-sdk/channel-lifecycle";
 import { createChannelReplyPipeline } from "openclaw/plugin-sdk/channel-reply-pipeline";
 import {
  resolveChannelStreamingBlockEnabled,
@@ -580,45 +581,9 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag

      const reply = resolveSendableOutboundReplyParts(payload);
      const slackBlocks = readSlackReplyBlocks(payload);
-      const draftMessageId = draftStream?.messageId();
-      const draftChannelId = draftStream?.channelId();
      const trimmedFinalText = reply.trimmedText;
-      const canFinalizeViaPreviewEdit =
-        previewStreamingEnabled &&
-        streamMode !== "status_final" &&
-        !reply.hasMedia &&
-        !payload.isError &&
-        (trimmedFinalText.length > 0 || Boolean(slackBlocks?.length)) &&
-        typeof draftMessageId === "string" &&
-        typeof draftChannelId === "string";

-      if (canFinalizeViaPreviewEdit) {
-        const finalThreadTs = usedReplyThreadTs ?? statusThreadTs;
-        if (deliveryTracker.hasDelivered({ kind: info.kind, payload, threadTs: finalThreadTs })) {
-          observedReplyDelivery = true;
-          return;
-        }
-        draftStream?.stop();
-        try {
-          await finalizeSlackPreviewEdit({
-            client: ctx.app.client,
-            token: ctx.botToken,
-            accountId: account.accountId,
-            channelId: draftChannelId,
-            messageId: draftMessageId,
-            text: normalizeSlackOutboundText(trimmedFinalText),
-            ...(slackBlocks?.length ? { blocks: slackBlocks } : {}),
-            threadTs: finalThreadTs,
-          });
-          observedReplyDelivery = true;
-          deliveryTracker.markDelivered({ kind: info.kind, payload, threadTs: finalThreadTs });
-          return;
-        } catch (err) {
-          logVerbose(
-            `slack: preview final edit failed; falling back to standard send (${formatErrorMessage(err)})`,
-          );
-        }
-      } else if (previewStreamingEnabled && streamMode === "status_final" && hasStreamedMessage) {
+      if (previewStreamingEnabled && streamMode === "status_final" && hasStreamedMessage) {
        try {
          const statusChannelId = draftStream?.channelId();
          const statusMessageId = draftStream?.messageId();
@@ -633,12 +598,75 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag
        } catch (err) {
          logVerbose(`slack: status_final completion update failed (${formatErrorMessage(err)})`);
        }
-      } else if (reply.hasMedia) {
-        await draftStream?.clear();
        hasStreamedMessage = false;
      }

-      await deliverNormally({ payload, kind: info.kind });
+      const result = await deliverFinalizableDraftPreview({
+        kind: info.kind,
+        payload,
+        draft: draftStream
+          ? {
+              flush: draftStream.flush,
+              clear: draftStream.clear,
+              discardPending: draftStream.discardPending,
+              seal: draftStream.seal,
+              id: () => {
+                const channelId = draftStream.channelId();
+                const messageId = draftStream.messageId();
+                return channelId && messageId ? { channelId, messageId } : undefined;
+              },
+            }
+          : undefined,
+        buildFinalEdit: () => {
+          if (
+            !previewStreamingEnabled ||
+            streamMode === "status_final" ||
+            reply.hasMedia ||
+            payload.isError ||
+            (trimmedFinalText.length === 0 && !slackBlocks?.length)
+          ) {
+            return undefined;
+          }
+          return {
+            text: normalizeSlackOutboundText(trimmedFinalText),
+            blocks: slackBlocks,
+            threadTs: usedReplyThreadTs ?? statusThreadTs,
+          };
+        },
+        editFinal: async (preview, edit) => {
+          if (deliveryTracker.hasDelivered({ kind: info.kind, payload, threadTs: edit.threadTs })) {
+            return;
+          }
+          await finalizeSlackPreviewEdit({
+            client: ctx.app.client,
+            token: ctx.botToken,
+            accountId: account.accountId,
+            channelId: preview.channelId,
+            messageId: preview.messageId,
+            text: edit.text,
+            ...(edit.blocks?.length ? { blocks: edit.blocks } : {}),
+            threadTs: edit.threadTs,
+          });
+        },
+        deliverNormally: async () => {
+          await deliverNormally({ payload, kind: info.kind });
+        },
+        onPreviewFinalized: (_preview) => {
+          const finalThreadTs = usedReplyThreadTs ?? statusThreadTs;
+          observedReplyDelivery = true;
+          replyPlan.markSent();
+          deliveryTracker.markDelivered({ kind: info.kind, payload, threadTs: finalThreadTs });
+        },
+        logPreviewEditFailure: (err) => {
+          logVerbose(
+            `slack: preview final edit failed; falling back to standard send (${formatErrorMessage(err)})`,
+          );
+        },
+      });
+
+      if (result === "preview-finalized") {
+        return;
+      }
    },
    onError: (err, info) => {
      runtime.error?.(danger(`slack ${info.kind} reply failed: ${formatErrorMessage(err)}`));
@@ -653,13 +681,12 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag
        accountId: account.accountId,
        maxChars: Math.min(ctx.textLimit, SLACK_TEXT_LIMIT),
        resolveThreadTs: () => {
-          const ts = replyPlan.nextThreadTs();
+          const ts = replyPlan.peekThreadTs();
          if (ts) {
            usedReplyThreadTs ??= ts;
          }
          return ts;
        },
-        onMessageSent: () => replyPlan.markSent(),
        log: logVerbose,
        warn: logVerbose,
      })
@@ -826,8 +853,7 @@ export async function dispatchPreparedSlackMessage(prepared: PreparedSlackMessag
  } catch (err) {
    dispatchError = err;
  } finally {
-    await draftStream?.flush();
-    draftStream?.stop();
+    await draftStream?.discardPending();
    markDispatchIdle();
  }

--- a/extensions/slack/src/monitor/replies.test.ts
+++ b/extensions/slack/src/monitor/replies.test.ts
@@ -6,6 +6,7 @@ vi.mock("../send.js", () => ({
 }));

 let deliverReplies: typeof import("./replies.js").deliverReplies;
+let createSlackReplyDeliveryPlan: typeof import("./replies.js").createSlackReplyDeliveryPlan;
 let resolveSlackThreadTs: typeof import("./replies.js").resolveSlackThreadTs;
 import { deliverSlackSlashReplies } from "./replies.js";

@@ -23,7 +24,8 @@ function baseParams(overrides?: Record<string, unknown>) {

 describe("deliverReplies identity passthrough", () => {
  beforeAll(async () => {
-    ({ deliverReplies, resolveSlackThreadTs } = await import("./replies.js"));
+    ({ createSlackReplyDeliveryPlan, deliverReplies, resolveSlackThreadTs } =
+      await import("./replies.js"));
  });

  beforeEach(() => {
@@ -211,6 +213,29 @@ describe("resolveSlackThreadTs fallback classification", () => {
  });
 });

+describe("createSlackReplyDeliveryPlan", () => {
+  it("lets draft previews inspect first thread targets without consuming them", () => {
+    const hasRepliedRef = { value: false };
+    const plan = createSlackReplyDeliveryPlan({
+      replyToMode: "first",
+      incomingThreadTs: undefined,
+      messageTs: "9999999999.999999",
+      hasRepliedRef,
+      isThreadReply: false,
+    });
+
+    expect(plan.peekThreadTs()).toBe("9999999999.999999");
+    expect(plan.peekThreadTs()).toBe("9999999999.999999");
+    expect(hasRepliedRef.value).toBe(false);
+
+    plan.markSent();
+
+    expect(hasRepliedRef.value).toBe(true);
+    expect(plan.peekThreadTs()).toBeUndefined();
+    expect(plan.nextThreadTs()).toBeUndefined();
+  });
+});
+
 describe("deliverSlackSlashReplies chunking", () => {
  it("keeps a 4205-character reply in a single slash response by default", async () => {
    const respond = vi.fn(async () => undefined);
--- a/extensions/slack/src/monitor/replies.ts
+++ b/extensions/slack/src/monitor/replies.ts
@@ -127,6 +127,7 @@ export function resolveSlackThreadTs(params: {
 }

 type SlackReplyDeliveryPlan = {
+  peekThreadTs: () => string | undefined;
  nextThreadTs: () => string | undefined;
  markSent: () => void;
 };
@@ -168,6 +169,7 @@ export function createSlackReplyDeliveryPlan(params: {
    isThreadReply: params.isThreadReply,
  });
  return {
+    peekThreadTs: () => replyReference.peek(),
    nextThreadTs: () => replyReference.use(),
    markSent: () => {
      replyReference.markSent();