Skip to content

Conversation

@Exodo0
Copy link
Contributor

@Exodo0 Exodo0 commented Dec 4, 2025

Description

Fixes #11314

AttachmentBuilder.getRawFile() was using a falsy check (this.data.id ? ...) to determine whether the attachment ID existed.
Because 0 is a valid ID but is falsy in JavaScript, the method incorrectly treated it as missing, producing an invalid file reference.

This PR updates the condition to properly detect the presence of the id property, allowing 0 to be handled correctly.

Changes

  • Replaced the falsy check this.data.id? ... with a property-existence check: 'id' in this.data? ....
  • Added a test to confirm that an attachment with ID 0 generates the expected raw file key (files[0]).

Testing

  • All existing tests pass.
  • Added test: "AttachmentBuilder handles 0 as a valid ID".
  • Verified that getRawFile() returns 'files[0]' when the ID is 0.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Fixes discordjs#11314

The AttachmentBuilder.getRawFile() method was using a falsy check that treated 0 as invalid. Changed to use the 'in' operator to properly check for the existence of the id property.
@vercel
Copy link

vercel bot commented Dec 4, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
discord-js Skipped Skipped Dec 8, 2025 11:03pm
discord-js-guide Skipped Skipped Dec 8, 2025 11:03pm

@vercel vercel bot temporarily deployed to Preview – discord-js-guide December 4, 2025 19:59 Inactive
@vercel vercel bot temporarily deployed to Preview – discord-js December 4, 2025 19:59 Inactive
@codecov
Copy link

codecov bot commented Dec 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 38.59%. Comparing base (d0745af) to head (3f1c38b).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11330      +/-   ##
==========================================
+ Coverage   32.45%   38.59%   +6.14%     
==========================================
  Files         369      375       +6     
  Lines       13611    17528    +3917     
  Branches     1069     1483     +414     
==========================================
+ Hits         4417     6765    +2348     
- Misses       9059    10747    +1688     
+ Partials      135       16     -119     
Flag Coverage Δ
builders 67.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vercel vercel bot temporarily deployed to Preview – discord-js-guide December 6, 2025 02:02 Inactive
@vercel vercel bot temporarily deployed to Preview – discord-js December 6, 2025 02:02 Inactive
Copy link
Member

@didinele didinele left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almeida approved but I insist on their proposed diff.

@github-project-automation github-project-automation bot moved this from Todo to Review in Progress in discord.js Dec 8, 2025
@vercel
Copy link

vercel bot commented Dec 8, 2025

Deployment failed with the following error:

The provided GitHub repository does not contain the requested branch or commit reference. Please ensure the repository is not empty.

@almeidx almeidx requested a review from didinele December 8, 2025 13:42
@vercel vercel bot temporarily deployed to Preview – discord-js December 8, 2025 16:18 Inactive
@vercel vercel bot temporarily deployed to Preview – discord-js-guide December 8, 2025 16:18 Inactive
@github-project-automation github-project-automation bot moved this from Review in Progress to Review Approved in discord.js Dec 8, 2025
@vercel vercel bot temporarily deployed to Preview – discord-js-guide December 8, 2025 23:03 Inactive
@vercel vercel bot temporarily deployed to Preview – discord-js December 8, 2025 23:03 Inactive
@coderabbitai
Copy link

coderabbitai bot commented Dec 8, 2025

📝 Walkthrough

Walkthrough

The fix corrects how AttachmentBuilder handles id 0, treating it as a valid identifier instead of falsy. Previously, id 0 was treated as falsy, preventing the key field from being included in the RawFile output. The conditional check was updated to explicitly detect undefined values, and a test was added to verify this behavior.

Changes

Cohort / File(s) Summary
Attachment id 0 handling
packages/builders/__tests__/messages/fileBody.test.ts, packages/builders/src/messages/Attachment.ts
Added test case validating AttachmentBuilder treats id 0 as valid; updated attachment key assignment logic to include key whenever id is defined rather than only when truthy, allowing id 0 to produce the correct RawFile output

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Logic change: Single conditional update from truthy check to explicit undefined check
  • Test coverage: Straightforward new test case for id 0 handling
  • Scope: Localized fix affecting only the id field assignment in one component

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title clearly and concisely summarizes the main change: fixing handling of 0 as a valid attachment ID.
Description check ✅ Passed The description is directly related to the changeset, explaining the bug fix, the specific changes made, and testing approach.
Linked Issues check ✅ Passed The changes fully address the linked issue #11314 by replacing the falsy check with a property-existence check and adding a test for ID 0.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the attachment ID handling issue; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d0745af and 3f1c38b.

📒 Files selected for processing (2)
  • packages/builders/__tests__/messages/fileBody.test.ts (1 hunks)
  • packages/builders/src/messages/Attachment.ts (1 hunks)
🔇 Additional comments (2)
packages/builders/src/messages/Attachment.ts (1)

156-156: LGTM! Correct fix for the 0 ID issue.

The explicit undefined check correctly handles 0 as a valid ID while maintaining the intended behavior for missing IDs. This replaces the previous falsy check that incorrectly treated 0 as missing.

packages/builders/__tests__/messages/fileBody.test.ts (1)

27-36: LGTM! Test correctly verifies the fix.

The test appropriately verifies that AttachmentBuilder handles 0 as a valid ID, asserting that getRawFile() returns the expected key: 'files[0]' value.


Comment @coderabbitai help to get the list of available commands and usage tips.

@kodiakhq kodiakhq bot merged commit fa43c40 into discordjs:main Dec 8, 2025
27 checks passed
@github-project-automation github-project-automation bot moved this from Review Approved to Done in discord.js Dec 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Attachment builder does not handle 0 for ids

4 participants