fix(media): use fetch for direct transfers - #278
Conversation
|
Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/media/transfer/WaMediaTransferClient.ts">
<violation number="1" location="src/media/transfer/WaMediaTransferClient.ts:325">
P2: The fetch path follows HTTP redirects by default (redirect: 'follow'), while the retained http/https agent path does not follow redirects. This changes the observable semantics of StreamTransferResponse for 3xx responses: fetch transparently follows the redirect and reports the final status/ok, whereas the old path surfaced the 3xx. It also means the reported `url` on the returned response stays the originally requested URL even when the transfer actually completes against a different host, and cross-origin redirects can strip sensitive headers. If the media endpoints must not be silently redirected (or callers rely on seeing the 3xx), pass an explicit `redirect: 'manual'` init to match the prior behavior.</violation>
</file>
<file name="src/media/__tests__/media.test.ts">
<violation number="1" location="src/media/__tests__/media.test.ts:1009">
P3: The new fetch path's riskiest behavior—uploading a Node `Readable` body through fetch, which relies on the newly added `duplex: 'half'` option in `WaMediaTransferClient.httpRequest`—has no test coverage. The new fetch test only uploads a `Uint8Array`, and the only Readable-upload test executes through the agent (http) path, so a regression in duplex streaming would pass CI. Add a fetch-path upload test that passes a `Readable` body and asserts the round-tripped bytes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| fetchInit.duplex = 'half' | ||
| } | ||
|
|
||
| const response = await fetch(url, fetchInit) |
There was a problem hiding this comment.
P2: The fetch path follows HTTP redirects by default (redirect: 'follow'), while the retained http/https agent path does not follow redirects. This changes the observable semantics of StreamTransferResponse for 3xx responses: fetch transparently follows the redirect and reports the final status/ok, whereas the old path surfaced the 3xx. It also means the reported url on the returned response stays the originally requested URL even when the transfer actually completes against a different host, and cross-origin redirects can strip sensitive headers. If the media endpoints must not be silently redirected (or callers rely on seeing the 3xx), pass an explicit redirect: 'manual' init to match the prior behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/media/transfer/WaMediaTransferClient.ts, line 325:
<comment>The fetch path follows HTTP redirects by default (redirect: 'follow'), while the retained http/https agent path does not follow redirects. This changes the observable semantics of StreamTransferResponse for 3xx responses: fetch transparently follows the redirect and reports the final status/ok, whereas the old path surfaced the 3xx. It also means the reported `url` on the returned response stays the originally requested URL even when the transfer actually completes against a different host, and cross-origin redirects can strip sensitive headers. If the media endpoints must not be silently redirected (or callers rely on seeing the 3xx), pass an explicit `redirect: 'manual'` init to match the prior behavior.</comment>
<file context>
@@ -311,6 +311,35 @@ export class WaMediaTransferClient {
+ fetchInit.duplex = 'half'
+ }
+
+ const response = await fetch(url, fetchInit)
+ const headers: Record<string, string> = {}
+ response.headers.forEach((value, key) => {
</file context>
| const mediaTransfer = new WaMediaTransferClient() | ||
| const base = `http://127.0.0.1:${address.port}` | ||
| const download = await mediaTransfer.downloadBytes({ url: `${base}/download` }) | ||
| const uploadResponse = await mediaTransfer.uploadStream({ |
There was a problem hiding this comment.
P3: The new fetch path's riskiest behavior—uploading a Node Readable body through fetch, which relies on the newly added duplex: 'half' option in WaMediaTransferClient.httpRequest—has no test coverage. The new fetch test only uploads a Uint8Array, and the only Readable-upload test executes through the agent (http) path, so a regression in duplex streaming would pass CI. Add a fetch-path upload test that passes a Readable body and asserts the round-tripped bytes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/media/__tests__/media.test.ts, line 1009:
<comment>The new fetch path's riskiest behavior—uploading a Node `Readable` body through fetch, which relies on the newly added `duplex: 'half'` option in `WaMediaTransferClient.httpRequest`—has no test coverage. The new fetch test only uploads a `Uint8Array`, and the only Readable-upload test executes through the agent (http) path, so a regression in duplex streaming would pass CI. Add a fetch-path upload test that passes a `Readable` body and asserts the round-tripped bytes.</comment>
<file context>
@@ -973,6 +973,59 @@ test('media transfer client applies separate upload/download agents', async () =
+ const mediaTransfer = new WaMediaTransferClient()
+ const base = `http://127.0.0.1:${address.port}`
+ const download = await mediaTransfer.downloadBytes({ url: `${base}/download` })
+ const uploadResponse = await mediaTransfer.uploadStream({
+ url: `${base}/upload`,
+ method: 'POST',
</file context>
1 participant
Summary
http/httpsagent path when a proxy agent is configuredReadablestreams for the current public APIWhy
Some Meta CDN responses close the HTTP/1.1 connection in a way that the native Node request parser rejects with
HPE_CLOSED_CONNECTION. Undici/Fetch accepts those responses while preserving streaming behavior.Validation