Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(voice): mark stream as ended #10455

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nyapat
Copy link
Contributor

@nyapat nyapat commented Aug 22, 2024

Please describe the changes this PR makes and why it should be merged:

closes #8778

example:

  • subscribe to user.
  • user starts speaking (pipeline is: check if subscription exists, if stream exists, voice packets are decrypted, and pushed to stream)
  • user stops speaking (specifically when endbehavior aftersilence or afterinactivity determines its 'stopped', push null to mark end)
  • user starts speaking (subscription exists: yep, stream exists: yep, push to stream!)
  • error: pushing after stream EOF

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

refactor: prefer destroying the stream
@nyapat nyapat requested a review from a team as a code owner August 22, 2024 12:16
Copy link

vercel bot commented Aug 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
discord-js ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 19, 2024 8:09pm
discord-js-guide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 19, 2024 8:09pm

Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

If possible, can we get a test script for this too? 🙏

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.44%. Comparing base (6cbe248) to head (669104e).

Additional details and impacted files
@@             Coverage Diff             @@
##             main   #10455       +/-   ##
===========================================
+ Coverage   38.01%   50.44%   +12.43%     
===========================================
  Files         239      227       -12     
  Lines       15471    20742     +5271     
  Branches     1353     1305       -48     
===========================================
+ Hits         5881    10463     +4582     
- Misses       9575    10267      +692     
+ Partials       15       12        -3     
Flag Coverage Δ
voice 72.39% <100.00%> (+0.16%) ⬆️

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.

@nyapat nyapat marked this pull request as draft August 23, 2024 10:25
@nyapat
Copy link
Contributor Author

nyapat commented Aug 23, 2024

drafted because putting tests was a good idea and i found undesirable behaviour (particularly annoying because the tests & actual context of the function don't align properly)

test: wait duration ms to check end

chore: eslint

test: end before timeout
@nyapat nyapat marked this pull request as ready for review August 25, 2024 09:53
@nyapat nyapat requested a review from vladfrangu August 31, 2024 03:26
Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

Considering voice is an "if may work" basis, and assuming you've tested this with actual discord behavior, LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

Voice receiver stream error
3 participants