feat(adapters): add sendPayload to batch-d (Zalo, Zalouser, core outbound plugins)#30144
Conversation
3a2ab74 to
03962fe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03962fe6c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return lastResult; | ||
| } | ||
| return outbound.sendText!({ ...ctx }); |
There was a problem hiding this comment.
Preserve chunking for text-only sendPayload deliveries
When channelData is present, deliverOutboundPayloads routes through sendPayload instead of sendTextChunks (src/infra/outbound/deliver.ts), and this branch sends text with a single sendText call. For adapters created by this helper (textChunkLimit: 4000), a text-only payload that includes metadata in channelData will no longer be chunked, so long messages can exceed channel limits (or bypass Signal’s special chunking/format path) even though the non-sendPayload flow handled them safely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Chunking is handled at the delivery layer in deliver.ts (PR #29997, commit 835566c). When channelData is present, deliverOutboundPayloadsCore chunks text before calling sendPayload — leading chunks go via sendText, the final chunk (with channelData) goes via sendPayload. Adapters do not need to chunk internally.
The text-only fallback now explicitly passes text: ctx.payload.text ?? "" (b07f80e) for defensive clarity.
There was a problem hiding this comment.
Correction: chunking is now handled in the adapter's sendPayload (commit 61b6475), using the factory's own chunker/textChunkLimit. Test coverage added.
There was a problem hiding this comment.
Pull request overview
Adds sendPayload support across several outbound adapters/plugins to support the outbox-based delivery pipeline that prefers a single unified adapter entrypoint.
Changes:
- Added
sendPayloadto core outbound adapters: WhatsApp, Slack, Discord. - Added
sendPayloadto the shared direct text/media outbound factory (affecting iMessage/Signal). - Added
sendPayloadto batch-d extension adapters: Zalo and Zalouser.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/channels/plugins/outbound/whatsapp.ts | Adds sendPayload that delegates to existing sendText / sendMedia for WhatsApp. |
| src/channels/plugins/outbound/slack.ts | Adds sendPayload that delegates to existing sendText / sendMedia for Slack. |
| src/channels/plugins/outbound/discord.ts | Adds sendPayload that delegates to existing sendText / sendMedia for Discord. |
| src/channels/plugins/outbound/direct-text-media.ts | Adds sendPayload to the shared direct adapter factory by sequencing sendMedia / sendText. |
| extensions/zalouser/src/channel.ts | Adds sendPayload to Zalouser outbound adapter delegating to existing methods. |
| extensions/zalo/src/channel.ts | Adds sendPayload to Zalo outbound adapter delegating to existing methods. |
| sendPayload: async (ctx) => { | ||
| const urls = ctx.payload.mediaUrls?.length | ||
| ? ctx.payload.mediaUrls | ||
| : ctx.payload.mediaUrl | ||
| ? [ctx.payload.mediaUrl] | ||
| : []; | ||
| if (urls.length > 0) { | ||
| let lastResult = await outbound.sendMedia!({ | ||
| ...ctx, | ||
| text: ctx.payload.text ?? "", | ||
| mediaUrl: urls[0], | ||
| }); | ||
| for (let i = 1; i < urls.length; i++) { | ||
| lastResult = await outbound.sendMedia!({ | ||
| ...ctx, | ||
| text: "", | ||
| mediaUrl: urls[i], | ||
| }); | ||
| } | ||
| return lastResult; | ||
| } | ||
| return outbound.sendText!({ ...ctx }); | ||
| }, |
There was a problem hiding this comment.
sendPayload is newly added here (and therefore to the iMessage/Signal adapters that are built from this factory), but there are no unit tests exercising the new delegation behavior (text-only, media-only, multi-media ordering/captioning). Consider adding tests similar to the existing telegramOutbound.sendPayload tests to ensure the first media gets the caption and subsequent media are sent with empty text, and that the returned result is the final send.
There was a problem hiding this comment.
Acknowledged — sendPayload test coverage will be added in a follow-up. The delegation logic is straightforward (iterate URLs → sendMedia, fallback → sendText) and is now defensively explicit about the text field (b07f80e).
Greptile SummaryAdds a unified
The changes are additive only—existing Confidence Score: 5/5
Last reviewed commit: 03962fe |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b07f80ee25
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| return lastResult; | ||
| } | ||
| return whatsappOutbound.sendText!({ ...ctx, text: ctx.payload.text ?? "" }); |
There was a problem hiding this comment.
Keep chunking for text-only sendPayload deliveries
deliverOutboundPayloads routes any payload with non-empty channelData through adapter sendPayload, but this fallback sends text via a single sendText call (ctx.payload.text ?? "") instead of the adapter chunker. In whatsappOutbound (and the identical pattern in extensions/zalo/src/channel.ts and extensions/zalouser/src/channel.ts), a long text payload that includes metadata in channelData will bypass textChunkLimit and can exceed provider limits or fail, even though the non-sendPayload path would chunk it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 61b6475. sendPayload now chunks text using the adapter's own chunker/textChunkLimit. Test coverage added.
| } | ||
| return lastResult; | ||
| } | ||
| return outbound.sendText!({ ...ctx, text: ctx.payload.text ?? "" }); |
There was a problem hiding this comment.
Skip empty Signal sends in sendPayload fallback
When payload has channelData but no text/media, this fallback always calls sendText with an empty string. For the Signal adapter (which is built from this helper), that reaches sendMessageSignal, which throws when both text and media are empty; channelData-only payloads are intentionally preserved upstream, so these deliveries now fail instead of no-oping as they did before sendPayload was added.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 61b6475. Empty payloads return a no-op result. Test coverage added.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61b6475a60
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const limit = outbound.textChunkLimit; | ||
| const chunks = limit && outbound.chunker ? outbound.chunker(text, limit) : [text]; |
There was a problem hiding this comment.
Use resolved chunk limits when chunking payload text
When channelData is present, delivery switches to sendPayload, and this implementation chunks using outbound.textChunkLimit directly instead of the delivery-layer resolveTextChunkLimit/resolveChunkMode logic. That means channel/account overrides are ignored for these payloads: for example, a configured smaller Signal/iMessage limit (or newline chunk mode) is bypassed and the hardcoded adapter limit behavior is used instead, which can change chunking semantics and violate configured limits.
Useful? React with 👍 / 👎.
| for (const chunk of chunks) { | ||
| lastResult = await outbound.sendText!({ ...ctx, text: chunk }); |
There was a problem hiding this comment.
Check cancellation between chunk sends
This loop sends all chunks inside sendPayload without any abort checks, so once execution enters this method it will continue sending remaining chunks even if delivery is cancelled mid-flight. Before this commit, channelData payloads on this adapter went through delivery-layer chunk/media loops that call throwIfAborted between sends, so cancellation could stop subsequent messages.
Useful? React with 👍 / 👎.
61b6475 to
0f73e2d
Compare
|
Landed via temp rebase onto main.
Thanks @nohat! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f73e2d3ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const limit = whatsappOutbound.textChunkLimit; | ||
| const chunks = | ||
| limit && whatsappOutbound.chunker ? whatsappOutbound.chunker(text, limit) : [text]; |
There was a problem hiding this comment.
Respect resolved chunk config in WhatsApp sendPayload
This sendPayload implementation chunks text with whatsappOutbound.textChunkLimit directly, which bypasses the delivery-layer resolveTextChunkLimit/resolveChunkMode logic used for normal sends. For payloads that include channelData, account/channel overrides like channels.whatsapp.accounts.<id>.textChunkLimit or chunkMode: "newline" are ignored, so chunking semantics can diverge from configured behavior and long messages may exceed limits that were explicitly configured to prevent that.
Useful? React with 👍 / 👎.
| for (let i = 1; i < urls.length; i++) { | ||
| lastResult = await whatsappOutbound.sendMedia!({ | ||
| ...ctx, |
There was a problem hiding this comment.
Stop sendPayload loops when delivery is aborted
The new media iteration loop does not check cancellation between sends. deliverOutboundPayloadsCore only checks throwIfAborted before calling sendPayload, so for channelData payloads an abort that happens after the first media send will not prevent subsequent media sends in this loop. This can produce unintended extra outbound messages even after callers cancel the delivery.
Useful? React with 👍 / 👎.
Part of Message Reliability: Durable SQLite Outbox, Recovery Worker, and Unified sendPayload (#32063)
Summary
sendPayloadmethod for the delivery pipelinesendPayloadper adaptersendPayloadto batch-d adapters (Zalo, Zalouser) and core outbound plugins (direct-text-media, Discord, Slack, WhatsApp). All iteratemediaUrlsand delegate to existing sendText/sendMediaChange Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
None — additive internal method only.
Security Impact (required)
Repro + Verification
Environment
Steps
Expected
Actual
Evidence
Human Verification (required)
Compatibility / Migration
Failure Recovery (if this breaks)
Risks and Mitigations
None — additive change.