fix(audio): support Response streams safely#1960
Closed
HAYDEN-OAI wants to merge 4 commits into
Closed
Conversation
Contributor
Author
|
closing this out since the fix landed through the original author’s PR in #1952. thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Response.bodyWeb streams withReadable.fromWeb()while preserving Node-readable bodies from custom fetch implementationsstream.pipeline()so playback rejects cleanly if the response stream fails and terminatesffplayinstead of leaving the child hangingWhy
playAudio(response)currently assumes every response body has Node's.pipe()method. Native fetch responses expose a WebReadableStream, so playback fails immediately. The original PR fixed that incompatibility and preserved the existing Node-stream path after review feedback.During deeper review, we found that an error from the adapted Web stream would be emitted without a listener, potentially terminating the process instead of rejecting
playAudio(). Routing the selected source throughpipeline()handles that failure and closes the playback pipe safely.Attribution
This integration branch contains the original #1952 commits
fb2dc044andc9e3996aunchanged. A direct maintainer push to the external fork was blocked by local repository policy, so the original history was carried into this upstream branch before adding the review fix.Validation
pnpm exec jest tests/helpers/audio.test.ts --runInBandpnpm exec tsc --noEmitpnpm exec eslint src/helpers/audio.ts tests/helpers/audio.test.tspnpm exec prettier --check src/helpers/audio.ts tests/helpers/audio.test.tspnpm buildSupersedes #1952.