fix(bluebubbles): accept updated-message events carrying attachments#66120
fix(bluebubbles): accept updated-message events carrying attachments#66120coletebou wants to merge 1 commit intoopenclaw:mainfrom
Conversation
BlueBubbles fires new-message first (text only), then updated-message ~2-3s later once attachment data is ready. The webhook filter unconditionally discards updated-message events that are not reactions, silently dropping all inbound images, PDFs, and files. Let updated-message events through when data.attachments is non-empty. Text is preserved (not stripped) so that mention gating in group chats continues to work correctly. For text+attachment sends this means the text body is delivered twice (once from new-message, once from updated-message); a messageId-based seen-set in the debouncer would close this gap but is left as a follow-up to keep this fix minimal.
openclaw-barnacle
Bot
added
channel: bluebubbles
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 348aba1baa
ℹ️ 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 dataAttachments = asRecord(payload.data)?.attachments; | ||
| const isAttachmentUpdate = | ||
| eventType === "updated-message" && Array.isArray(dataAttachments) && dataAttachments.length > 0; |
There was a problem hiding this comment.
The new isAttachmentUpdate gate only checks asRecord(payload.data)?.attachments, but this webhook path already supports non-record wrappers (for example array- or string-wrapped payloads) via extractMessagePayload in normalizeWebhookMessage. For updated-message events where attachments are present but payload.data is not a plain object, isAttachmentUpdate stays false and the event is still dropped by the updated-message/!reaction filter, so inbound attachments remain silently lost in those supported payload shapes.
Useful? React with 👍 / 👎.
Greptile SummaryFixes silent attachment loss in the BlueBubbles webhook handler by allowing Confidence Score: 5/5Safe to merge; all findings are P2 style/test suggestions that do not block correctness. The logic change is small and well-reasoned: the extensions/bluebubbles/src/monitor.ts — minor log string and no test coverage for the new
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Allow updated-message events through when they carry attachments. | ||
| // BB fires new-message (text only), then updated-message ~2-3s later once | ||
| // attachment data is ready. Without this, inbound images/PDFs/files are | ||
| // silently dropped. Only check data.attachments (normalizeWebhookMessage | ||
| // requires a message container in payload.data, so root-only attachment | ||
| // payloads would pass the filter but fail normalization). | ||
| const dataAttachments = asRecord(payload.data)?.attachments; | ||
| const isAttachmentUpdate = | ||
| eventType === "updated-message" && Array.isArray(dataAttachments) && dataAttachments.length > 0; |
There was a problem hiding this comment.
The fix introduces a meaningful new branch — updated-message events carrying data.attachments now pass through the filter — but monitor.test.ts has no test cases for this scenario. Given that the old behavior silently dropped attachments and updated-message filtering is easy to regress, a few focused tests would be valuable: one asserting that updated-message with non-empty data.attachments is accepted, one asserting that updated-message without attachments is still discarded, and one confirming new-message with attachments continues to work normally.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/bluebubbles/src/monitor.ts
Line: 251-259
Comment:
**No automated test coverage for the new path**
The fix introduces a meaningful new branch — `updated-message` events carrying `data.attachments` now pass through the filter — but `monitor.test.ts` has no test cases for this scenario. Given that the old behavior silently dropped attachments and `updated-message` filtering is easy to regress, a few focused tests would be valuable: one asserting that `updated-message` with non-empty `data.attachments` is accepted, one asserting that `updated-message` without attachments is still discarded, and one confirming `new-message` with attachments continues to work normally.
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Summary
BlueBubbles fires
new-messagefirst (text only), thenupdated-message~2-3s later once attachment data is ready. The webhook filter unconditionally discardsupdated-messageevents that are not reactions, silently dropping all inbound images, PDFs, and files.Lets
updated-messageevents through whendata.attachmentsis non-empty. Text is preserved (not stripped) so that mention gating in group chats works correctly.Supersedes #66105 and #63983.
Changes
extensions/bluebubbles/src/monitor.ts:updated-messageviaasRecord(payload.data)?.attachmentsKnown limitation
For text+attachment sends, the text body is delivered twice — once from
new-messageand once from theupdated-messagethat carries the attachment. The 500ms debounce window is shorter than the ~2-3s BB delay, so they aren't coalesced. AmessageId-based seen-set in the debouncer would close this gap but is left as a follow-up to keep this fix minimal. Dropping attachments (the current behavior) is a worse outcome than a duplicate text line.Test plan
updated-messageevents without attachments are still filtered out