fix(voice): remove player from datastore during buffering - #11598
fix(voice): remove player from datastore during buffering#11598Umit-Ulusoy wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthrough
ChangesAudioPlayer cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/voice/__tests__/AudioPlayer.test.ts`:
- Line 430: Update the test around deleteAudioPlayer to assert that the
DataStore no longer contains player after deletion, while keeping the real
deletion path enabled and retaining the existing deleteAudioPlayer call-count
assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2d62ca8d-f48d-4abc-bab8-cd015c647394
📒 Files selected for processing (2)
packages/voice/__tests__/AudioPlayer.test.tspackages/voice/src/audio/AudioPlayer.ts
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-05-18T13:40:11.014Z
Learnt from: almeidx
Repo: discordjs/discord.js PR: 11530
File: packages/core/src/api/user.ts:190-190
Timestamp: 2026-05-18T13:40:11.014Z
Learning: When reviewing JSDoc comments in the discord.js monorepo (e.g., discord.js and related packages), treat `see` links to Discord developer docs as consistent with the new documentation domain. Prefer `https://docs.discord.com/developers/...` over `https://discord.com/developers...`, and do not flag `see` links that already use `https://docs.discord.com/developers/...` as inconsistent.
Applied to files:
packages/voice/__tests__/AudioPlayer.test.tspackages/voice/src/audio/AudioPlayer.ts
📚 Learning: 2026-05-18T13:40:11.014Z
Learnt from: almeidx
Repo: discordjs/discord.js PR: 11530
File: packages/core/src/api/user.ts:190-190
Timestamp: 2026-05-18T13:40:11.014Z
Learning: When reviewing discord.js (and related) source files, JSDoc `see` links that point to Discord’s documentation on `https://docs.discord.com/developers/...` are correct and should not be flagged as inconsistent. For new/updated links going forward, prefer `https://docs.discord.com/developers` over the legacy `https://discord.com/developers/docs` domain.
Applied to files:
packages/voice/__tests__/AudioPlayer.test.tspackages/voice/src/audio/AudioPlayer.ts
🔇 Additional comments (1)
packages/voice/src/audio/AudioPlayer.ts (1)
494-495: LGTM!
| player.stop(); | ||
|
|
||
| expect(player.state.status).toEqual(AudioPlayerStatus.Idle); | ||
| expect(deleteAudioPlayer).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the DataStore side effect.
Line [430] verifies only that deleteAudioPlayer was called. It does not verify that player was removed from the DataStore; a failed or early-returning deletion would still pass. Assert that the actual store no longer contains player, while keeping the real deletion path enabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/voice/__tests__/AudioPlayer.test.ts` at line 430, Update the test
around deleteAudioPlayer to assert that the DataStore no longer contains player
after deletion, while keeping the real deletion path enabled and retaining the
existing deleteAudioPlayer call-count assertion.
This fix updates the AudioPlayer stop method to allow direct transition to the Idle state when the player is currently in the Buffering status. Previously, calling stop() during Buffering would hang or fail to clear the player due to silence padding constraints, leaving it lingering in the DataStore. This ensures the player is properly cleaned up and removed from the store. Fixes discordjs#11039
e63d296 to
710869a
Compare
|
(How) did you verify that the Player actually hangs/stays in DataStore before this fix? Because looking at the code the AudioResource should keep on playing 5 (by default) silence packets before the Player going to Idle state (and removing the Player from the DataStore in the process). Independent of it the resource had started or is still buffering. If users wanted to force the immediate stop then that parameter already exists. |
|
Hello there @Qjuh, thanks for taking a closer look. The silence-padding mechanism ( private _stepPrepare() { So when the player is in } else if (this.state.resource.silenceRemaining === -1) { this line does set To confirm this isn't just a theoretical reading of the code, I ran the existing test suite against the unpatched FAIL should remove AudioPlayer from DataStore when stopped during Buffering state and against the patched version, the same test (plus a second regression test I added for the "stream never becomes readable" case) passes cleanly, with no other test files affected. On the |
|
Okay, I see your point now. Your test however doesn't actually test for what it should. It would also fail if the 5 silence packets were sucessfully sent and then end the resource and thus the player, because your 2 wait() calls don't take long enough for five cycles of the audio loop (which takes ~100 ms). But I verified that the player does indeed keep staying stuck like you said even if waiting longer and thank you for pointing out the reason being the guard against Idle/Buffering state in _stepPrepare(). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #11598 +/- ##
=======================================
Coverage 32.07% 32.07%
=======================================
Files 390 390
Lines 14100 14100
Branches 1113 1113
=======================================
Hits 4522 4522
Misses 9440 9440
Partials 138 138
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for verifying it and for the feedback, @Qjuh! |
2 participants
This fix updates the AudioPlayer stop method to allow direct transition to the Idle state when the player is currently in the Buffering status.
Previously, calling stop() during Buffering would hang or fail to clear the player due to silence padding constraints, leaving it lingering in the DataStore.
This ensures the player is properly cleaned up and removed from the store.
Fixes #11039