fix(MessagePayload): allow AttachmentBuilder in files payload#11423
fix(MessagePayload): allow AttachmentBuilder in files payload#11423Qjuh wants to merge 2 commits intodiscordjs:mainfrom
Conversation
Qjuh
requested review from
Jiralite,
didinele,
iCrawl,
kyranet and
vladfrangu
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis pull request introduces support for raw-encodable files in Discord.js by adding a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@packages/discord.js/src/structures/MessagePayload.js`:
- Around line 193-206: The attachments mapping assigns id before spreading
file.toJSON(), which allows any id returned by AttachmentBuilder.toJSON() to
overwrite the index-based id; update the map callback used to build attachments
(the callback inside this.options.files?.map and the branches using
isRawFileEncodable(file)) so that the spread of file.toJSON() happens first and
then set id: index.toString() after the spread in both the raw and non-raw
branches, ensuring the generated index id cannot be overwritten by
file.toJSON().
In `@packages/util/src/encodables.ts`:
- Around line 64-83: Update the JSDoc in the RawFileEncodable block to fix the
typo “mutipart” → “multipart”, and harden the isRawFileEncodable type guard so
it not only checks isJSONEncodable(maybeEncodable) and the presence of
'getRawFile' but also verifies that (maybeEncodable as any).getRawFile is a
function before asserting the RawFileEncodable type; target the RawFileEncodable
interface, the isRawFileEncodable function and the getRawFile member when making
these changes.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
packages/discord.js/src/structures/MessagePayload.jspackages/discord.js/src/structures/interfaces/TextBasedChannel.jspackages/discord.js/typings/index.d.tspackages/util/src/encodables.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/util/src/encodables.ts (1)
packages/util/src/RawFile.ts (1)
RawFile(6-34)
packages/discord.js/src/structures/MessagePayload.js (1)
packages/util/src/encodables.ts (1)
isRawFileEncodable(81-83)
packages/discord.js/typings/index.d.ts (1)
packages/util/src/encodables.ts (1)
RawFileEncodable(69-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests
🔇 Additional comments (5)
packages/discord.js/typings/index.d.ts (2)
8-8: LGTM — typing import aligns with new raw-file support.
6724-6724: LGTM — BaseMessageOptions.files now accepts RawFileEncodable.packages/discord.js/src/structures/interfaces/TextBasedChannel.js (1)
91-92: LGTM — JSDoc reflects RawFileEncodable support.packages/discord.js/src/structures/MessagePayload.js (2)
4-4: LGTM!Clean addition of
isRawFileEncodablealongside the existingisJSONEncodableimport.
286-287: LGTM!Good placement of the
isRawFileEncodablebranch — it correctly sits between the primitive/stream check and the generic attachment-object fallback, and the early return avoids unnecessary resolution.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11423 +/- ##
==========================================
+ Coverage 31.56% 31.66% +0.09%
==========================================
Files 386 386
Lines 13944 13967 +23
Branches 1098 1099 +1
==========================================
+ Hits 4401 4422 +21
- Misses 9409 9411 +2
Partials 134 134
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
almeidx
left a comment
almeidx
left a comment
There was a problem hiding this comment.
Another regression caused by the mentioned pull request, since you're touching this area of MessagePayload
It is not currently possible to edit a message's attachments (e.g., removing all the attachments), as it causes:
/home/cyph/discord.js/packages/discord.js/src/structures/MessagePayload.js:210
attachments.push(
^
TypeError: Cannot read properties of undefined (reading 'push')
at MessagePayload.resolveBody (/home/cyph/discord.js/packages/discord.js/src/structures/MessagePayload.js:210:19)
at GuildMessageManager.edit (/home/cyph/discord.js/packages/discord.js/src/managers/MessageManager.js:243:10)
at Message.edit (/home/cyph/discord.js/packages/discord.js/src/structures/Message.js:862:34)
at Client.<anonymous> (/home/cyph/discord.js/packages/discord.js/test/polls.js:77:11)
at process.processTicksAndRejections (node:internal/process/task_queues:104:5)
Node.js v25.7.0| ); | ||
|
|
||
| // Only passable during edits | ||
| if (Array.isArray(this.options.attachments)) { |
There was a problem hiding this comment.
| if (Array.isArray(this.options.attachments)) { | |
| if (Array.isArray(this.options.attachments)) { | |
| attachments ??= []; |
| waveform: file.waveform, | ||
| duration_secs: file.duration, | ||
| })); | ||
| const attachments = this.options.files?.map((file, index) => |
There was a problem hiding this comment.
| const attachments = this.options.files?.map((file, index) => | |
| let attachments = this.options.files?.map((file, index) => |
2 participants
Fixes a regression of #11278 which causes issues when passing AttachmentBuilder in
MessagePayload#options#filesin any send method