Slack: fix review regressions
This commit is contained in:
parent
749f3e7baa
commit
47e0bf522f
@ -1292,6 +1292,25 @@ describe("slack actions adapter", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("does not attach empty blocks to plain media sends", async () => {
|
||||
handleSlackAction.mockClear();
|
||||
|
||||
await runSlackAction("send", {
|
||||
to: "channel:C1",
|
||||
message: "",
|
||||
media: "https://example.com/image.png",
|
||||
});
|
||||
|
||||
const [params] = handleSlackAction.mock.calls[0] ?? [];
|
||||
expect(params).toMatchObject({
|
||||
action: "sendMessage",
|
||||
to: "channel:C1",
|
||||
content: "",
|
||||
mediaUrl: "https://example.com/image.png",
|
||||
});
|
||||
expect(params).not.toHaveProperty("blocks");
|
||||
});
|
||||
|
||||
it("rejects edit when both message and blocks are missing", async () => {
|
||||
const { cfg, actions } = slackHarness();
|
||||
|
||||
|
||||
@ -100,4 +100,48 @@ describe("slackOutbound sendPayload", () => {
|
||||
await expect(run()).rejects.toThrow(/blocks must be an array/i);
|
||||
expect(sendMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("sends media before a separate interactive blocks message", async () => {
|
||||
const { run, sendMock, to } = createHarness({
|
||||
payload: {
|
||||
text: "Approval required",
|
||||
mediaUrl: "https://example.com/image.png",
|
||||
interactive: {
|
||||
blocks: [
|
||||
{
|
||||
type: "buttons",
|
||||
buttons: [{ label: "Allow", value: "pluginbind:approval-123:o" }],
|
||||
},
|
||||
],
|
||||
},
|
||||
},
|
||||
sendResults: [{ messageId: "sl-media" }, { messageId: "sl-controls" }],
|
||||
});
|
||||
|
||||
const result = await run();
|
||||
|
||||
expect(sendMock).toHaveBeenCalledTimes(2);
|
||||
expect(sendMock).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
to,
|
||||
"",
|
||||
expect.objectContaining({
|
||||
mediaUrl: "https://example.com/image.png",
|
||||
}),
|
||||
);
|
||||
expect(sendMock.mock.calls[0]?.[2]).not.toHaveProperty("blocks");
|
||||
expect(sendMock).toHaveBeenNthCalledWith(
|
||||
2,
|
||||
to,
|
||||
"Approval required",
|
||||
expect.objectContaining({
|
||||
blocks: [
|
||||
expect.objectContaining({
|
||||
type: "actions",
|
||||
}),
|
||||
],
|
||||
}),
|
||||
);
|
||||
expect(result).toMatchObject({ channel: "slack", messageId: "sl-controls" });
|
||||
});
|
||||
});
|
||||
|
||||
@ -20,6 +20,16 @@ import {
|
||||
|
||||
const SLACK_MAX_BLOCKS = 50;
|
||||
|
||||
function resolveRenderedInteractiveBlocks(
|
||||
interactive?: InteractiveReply,
|
||||
): SlackBlock[] | undefined {
|
||||
if (!interactive) {
|
||||
return undefined;
|
||||
}
|
||||
const blocks = buildSlackInteractiveBlocks(interactive);
|
||||
return blocks.length > 0 ? blocks : undefined;
|
||||
}
|
||||
|
||||
function resolveSlackSendIdentity(identity?: OutboundIdentity): SlackSendIdentity | undefined {
|
||||
if (!identity) {
|
||||
return undefined;
|
||||
@ -116,15 +126,15 @@ function resolveSlackBlocks(payload: {
|
||||
interactive?: InteractiveReply;
|
||||
}) {
|
||||
const slackData = payload.channelData?.slack;
|
||||
const renderedInteractive = buildSlackInteractiveBlocks(payload.interactive);
|
||||
const renderedInteractive = resolveRenderedInteractiveBlocks(payload.interactive);
|
||||
if (!slackData || typeof slackData !== "object" || Array.isArray(slackData)) {
|
||||
return renderedInteractive.length > 0 ? renderedInteractive : undefined;
|
||||
return renderedInteractive;
|
||||
}
|
||||
let existingBlocks: SlackBlock[] | undefined;
|
||||
existingBlocks = parseSlackBlocksInput((slackData as { blocks?: unknown }).blocks) as
|
||||
| SlackBlock[]
|
||||
| undefined;
|
||||
const mergedBlocks = [...(existingBlocks ?? []), ...renderedInteractive];
|
||||
const mergedBlocks = [...(existingBlocks ?? []), ...(renderedInteractive ?? [])];
|
||||
if (mergedBlocks.length === 0) {
|
||||
return undefined;
|
||||
}
|
||||
@ -173,17 +183,16 @@ export const slackOutbound: ChannelOutboundAdapter = {
|
||||
identity: ctx.identity,
|
||||
});
|
||||
}
|
||||
const lastResult = await sendPayloadMediaSequence({
|
||||
text: payload.text ?? "",
|
||||
await sendPayloadMediaSequence({
|
||||
text: "",
|
||||
mediaUrls,
|
||||
send: async ({ text, mediaUrl, isFirst }) =>
|
||||
send: async ({ text, mediaUrl }) =>
|
||||
await sendSlackOutboundMessage({
|
||||
cfg: ctx.cfg,
|
||||
to: ctx.to,
|
||||
text,
|
||||
mediaUrl,
|
||||
mediaLocalRoots: ctx.mediaLocalRoots,
|
||||
blocks: isFirst ? blocks : undefined,
|
||||
accountId: ctx.accountId,
|
||||
deps: ctx.deps,
|
||||
replyToId: ctx.replyToId,
|
||||
@ -191,7 +200,18 @@ export const slackOutbound: ChannelOutboundAdapter = {
|
||||
identity: ctx.identity,
|
||||
}),
|
||||
});
|
||||
return lastResult ?? { channel: "slack", messageId: "" };
|
||||
return await sendSlackOutboundMessage({
|
||||
cfg: ctx.cfg,
|
||||
to: ctx.to,
|
||||
text: payload.text ?? "",
|
||||
mediaLocalRoots: ctx.mediaLocalRoots,
|
||||
blocks,
|
||||
accountId: ctx.accountId,
|
||||
deps: ctx.deps,
|
||||
replyToId: ctx.replyToId,
|
||||
threadId: ctx.threadId,
|
||||
identity: ctx.identity,
|
||||
});
|
||||
},
|
||||
sendText: async ({ cfg, to, text, accountId, deps, replyToId, threadId, identity }) => {
|
||||
return await sendSlackOutboundMessage({
|
||||
|
||||
@ -186,6 +186,32 @@ describe("runMessageAction context isolation", () => {
|
||||
).rejects.toThrow(/message required/i);
|
||||
});
|
||||
|
||||
it("requires message when send only includes shared interactive payloads", async () => {
|
||||
await expect(
|
||||
runDrySend({
|
||||
cfg: {
|
||||
channels: {
|
||||
telegram: {
|
||||
botToken: "telegram-test",
|
||||
},
|
||||
},
|
||||
} as OpenClawConfig,
|
||||
actionParams: {
|
||||
channel: "telegram",
|
||||
target: "123456",
|
||||
interactive: {
|
||||
blocks: [
|
||||
{
|
||||
type: "buttons",
|
||||
buttons: [{ label: "Approve", value: "approve" }],
|
||||
},
|
||||
],
|
||||
},
|
||||
},
|
||||
}),
|
||||
).rejects.toThrow(/message required/i);
|
||||
});
|
||||
|
||||
it.each([
|
||||
{
|
||||
name: "structured poll params",
|
||||
|
||||
@ -13,7 +13,6 @@ import type {
|
||||
ChannelThreadingToolContext,
|
||||
} from "../../channels/plugins/types.js";
|
||||
import type { OpenClawConfig } from "../../config/config.js";
|
||||
import { normalizeInteractiveReply } from "../../interactive/payload.js";
|
||||
import { getAgentScopedMediaLocalRoots } from "../../media/local-roots.js";
|
||||
import { hasPollCreationParams, resolveTelegramPollVisibility } from "../../poll-params.js";
|
||||
import { resolvePollMaxSelections } from "../../polls.js";
|
||||
@ -406,11 +405,10 @@ async function handleSendAction(ctx: ResolvedActionContext): Promise<MessageActi
|
||||
readStringParam(params, "filePath", { trim: false });
|
||||
const hasCard = params.card != null && typeof params.card === "object";
|
||||
const hasComponents = params.components != null && typeof params.components === "object";
|
||||
const hasInteractive = normalizeInteractiveReply(params.interactive) != null;
|
||||
const caption = readStringParam(params, "caption", { allowEmpty: true }) ?? "";
|
||||
let message =
|
||||
readStringParam(params, "message", {
|
||||
required: !mediaHint && !hasCard && !hasComponents && !hasInteractive,
|
||||
required: !mediaHint && !hasCard && !hasComponents,
|
||||
allowEmpty: true,
|
||||
}) ?? "";
|
||||
if (message.includes("\\n")) {
|
||||
@ -476,14 +474,7 @@ async function handleSendAction(ctx: ResolvedActionContext): Promise<MessageActi
|
||||
message = "";
|
||||
}
|
||||
}
|
||||
if (
|
||||
!message.trim() &&
|
||||
!mediaUrl &&
|
||||
mergedMediaUrls.length === 0 &&
|
||||
!hasCard &&
|
||||
!hasComponents &&
|
||||
!hasInteractive
|
||||
) {
|
||||
if (!message.trim() && !mediaUrl && mergedMediaUrls.length === 0 && !hasCard && !hasComponents) {
|
||||
throw new Error("send requires text or media");
|
||||
}
|
||||
params.message = message;
|
||||
|
||||
@ -39,9 +39,9 @@ export async function handleSlackMessageAction(params: {
|
||||
allowEmpty: true,
|
||||
});
|
||||
const mediaUrl = readStringParam(actionParams, "media", { trim: false });
|
||||
const blocks =
|
||||
readSlackBlocksParam(actionParams) ??
|
||||
buildSlackInteractiveBlocks(normalizeInteractiveReply(actionParams.interactive));
|
||||
const interactive = normalizeInteractiveReply(actionParams.interactive);
|
||||
const interactiveBlocks = interactive ? buildSlackInteractiveBlocks(interactive) : undefined;
|
||||
const blocks = readSlackBlocksParam(actionParams) ?? interactiveBlocks;
|
||||
if (!content && !mediaUrl && !blocks) {
|
||||
throw new Error("Slack send requires message, blocks, or media.");
|
||||
}
|
||||
@ -56,9 +56,9 @@ export async function handleSlackMessageAction(params: {
|
||||
to,
|
||||
content: content ?? "",
|
||||
mediaUrl: mediaUrl ?? undefined,
|
||||
blocks,
|
||||
accountId,
|
||||
threadTs: threadId ?? replyTo ?? undefined,
|
||||
...(blocks ? { blocks } : {}),
|
||||
},
|
||||
cfg,
|
||||
ctx.toolContext,
|
||||
|
||||
@ -65,6 +65,7 @@ type SlackInteractiveDispatchContext = Omit<
|
||||
PluginInteractiveSlackHandlerContext,
|
||||
| "interaction"
|
||||
| "respond"
|
||||
| "channel"
|
||||
| "requestConversationBinding"
|
||||
| "detachConversationBinding"
|
||||
| "getCurrentConversationBinding"
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user