feat: add onDownloadProgress callback for response progress tracking#558
feat: add onDownloadProgress callback for response progress tracking#558productdevbook wants to merge 2 commits intounjs:mainfrom
Conversation
Add `onDownloadProgress` option that reports download progress via ReadableStream. Provides transferred bytes, total (from Content-Length), and percent. Response is still parsed normally after progress tracking. Resolves unjs#45 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughDownload progress tracking functionality was added to the fetch utility. The implementation includes a Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Fetch
participant Response as Response Stream
participant Reader as Stream Reader
participant Callback as onDownloadProgress
participant Parser as Response Parser
Client->>Response: Request with onDownloadProgress
Response-->>Reader: Create stream reader
Reader->>Reader: Read chunk
Reader->>Callback: { transferred, total, percent }
Note over Callback: Progress update emitted
Reader->>Reader: Accumulate bytes
loop For each chunk
Reader->>Reader: Read next chunk
Reader->>Callback: Update progress
Reader->>Reader: Accumulate
end
Reader-->>Response: Body fully buffered
Response->>Response: Reconstruct with buffered bytes
Response->>Parser: Pass reconstructed response
Parser-->>Client: Return parsed result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
… cap percent - Wrap reader.read() loop in try/catch to properly route errors through onError (produces FetchError, not raw AbortError) - Cancel reader on error to release stream lock - Skip onDownloadProgress when responseType is "stream" to avoid buffering the entire body (defeats purpose of streaming) - Cap percent at 1.0 with Math.min to handle Content-Length mismatches Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fetch.ts`:
- Around line 211-214: The buffering branch that checks
context.options.onDownloadProgress and context.options.responseType !== "stream"
should also skip buffering when the response is auto-detected as a stream; call
detectResponseType(context.response) (or reuse the same detection used
elsewhere) and treat a detected "stream" like an explicit responseType "stream"
so you don't buffer the body; apply the same change to the similar branch around
the 264-268 logic so both places first resolve the effective response type and
only buffer if it is not "stream".
- Around line 257-261: The current code replaces context.response with a newly
constructed Response after consuming the body for onDownloadProgress, which
loses native fields like url; instead, before reading the body for progress
tracking in the onDownloadProgress handling (the code paths manipulating
context.response and reading body), call context.response =
context.response.clone() or clone the original response into a local variable,
consume the clone for progress tracking, and keep the original Response object
(or set context.response to the original) so $fetch.raw returns the intact
native Response with its url and other properties; update the logic around
onDownloadProgress and the places that currently do new Response(...) to use
Response.clone() and avoid reconstructing the response.
In `@test/index.test.ts`:
- Around line 525-541: The test "reports total from content-length header"
currently skips assertions when last.total is undefined; update the test to call
$fetch against an endpoint that always returns a Content-Length (e.g., use a
fixture/getURL variant that sets Content-Length) and remove the conditional so
you unconditionally assert last.total is defined and then assert last.percent is
defined and within (0, 1] using the existing progress/last variables and the
onDownloadProgress handler; ensure you reference the existing test name, the
$fetch call, getURL, and the progress array when making the change.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d14381e0-35d7-4e46-9e13-3c63fbd73159
📒 Files selected for processing (3)
src/fetch.tssrc/types.tstest/index.test.ts
| if ( | ||
| context.options.onDownloadProgress && | ||
| context.response.body && | ||
| context.options.responseType !== "stream" |
There was a problem hiding this comment.
Skip auto-detected stream responses before buffering.
Line 214 only excludes an explicit responseType: "stream". If the caller omits responseType and the server returns text/event-stream, detectResponseType() later resolves that to "stream" in src/utils.ts:55-83, but this branch has already started buffering the body. That breaks streaming semantics and can hang until the server closes.
Suggested fix
- if (hasBody) {
+ if (hasBody) {
+ const responseType =
+ (context.options.parseResponse
+ ? "json"
+ : context.options.responseType) ||
+ detectResponseType(context.response.headers.get("content-type") || "");
// Download progress tracking
// Skip for stream responseType — progress tracking buffers the entire body
if (
context.options.onDownloadProgress &&
context.response.body &&
- context.options.responseType !== "stream"
+ responseType !== "stream"
) {- const responseType =
- (context.options.parseResponse
- ? "json"
- : context.options.responseType) ||
- detectResponseType(context.response.headers.get("content-type") || "");Also applies to: 264-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fetch.ts` around lines 211 - 214, The buffering branch that checks
context.options.onDownloadProgress and context.options.responseType !== "stream"
should also skip buffering when the response is auto-detected as a stream; call
detectResponseType(context.response) (or reuse the same detection used
elsewhere) and treat a detected "stream" like an explicit responseType "stream"
so you don't buffer the body; apply the same change to the similar branch around
the 264-268 logic so both places first resolve the effective response type and
only buffer if it is not "stream".
| context.response = new Response(body, { | ||
| status: context.response.status, | ||
| statusText: context.response.statusText, | ||
| headers: context.response.headers, | ||
| }) as FetchResponse<any>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
const http = require("node:http");
(async () => {
const server = http.createServer((_, res) => {
res.writeHead(200, { "Content-Type": "text/plain" });
res.end("hello");
});
await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve));
const { port } = server.address();
const url = `http://127.0.0.1:${port}/progress`;
try {
const original = await fetch(url);
const rebuilt = new Response(await original.clone().arrayBuffer(), {
status: original.status,
statusText: original.statusText,
headers: original.headers,
});
console.log({
originalUrl: original.url,
rebuiltUrl: rebuilt.url,
});
} finally {
server.close();
}
})();
NODERepository: unjs/ofetch
Length of output: 590
🏁 Script executed:
cat -n src/fetch.ts | head -280 | tail -50Repository: unjs/ofetch
Length of output: 2050
🏁 Script executed:
rg "onDownloadProgress" -A 3 -B 3Repository: unjs/ofetch
Length of output: 2386
🏁 Script executed:
rg "context\.response\s*=" --type ts -A 3 -B 3Repository: unjs/ofetch
Length of output: 642
🏁 Script executed:
rg "response\.url" --type tsRepository: unjs/ofetch
Length of output: 37
🏁 Script executed:
rg "\.raw" -A 5 -B 5 --type ts | head -100Repository: unjs/ofetch
Length of output: 503
🏁 Script executed:
cat -n src/fetch.ts | head -210 | tail -50Repository: unjs/ofetch
Length of output: 1919
🏁 Script executed:
rg "response\.url|\.url" -B 2 -A 2 --type ts | head -50Repository: unjs/ofetch
Length of output: 1667
🏁 Script executed:
git log --oneline -10 -- src/fetch.tsRepository: unjs/ofetch
Length of output: 144
🏁 Script executed:
rg "issues|clone|body" src/fetch.ts -B 2 -A 2 | head -80Repository: unjs/ofetch
Length of output: 2715
🏁 Script executed:
rg "github\.com/unjs/ofetch/issues" -A 1 -B 1Repository: unjs/ofetch
Length of output: 5986
🏁 Script executed:
cat -n src/fetch.ts | sed -n '200,270p'Repository: unjs/ofetch
Length of output: 3025
🏁 Script executed:
rg "clone\(\)" src/fetch.ts -B 3 -A 3Repository: unjs/ofetch
Length of output: 37
🏁 Script executed:
cat -n src/fetch.ts | sed -n '180,220p'Repository: unjs/ofetch
Length of output: 1716
🏁 Script executed:
rg "Response" src/fetch.ts -B 1 -A 1 | head -40Repository: unjs/ofetch
Length of output: 1161
🏁 Script executed:
rg "_data|response\._" src/fetch.ts | head -20Repository: unjs/ofetch
Length of output: 242
🏁 Script executed:
git log -p --all -S "clone()" -- src/fetch.ts | head -100Repository: unjs/ofetch
Length of output: 37
🏁 Script executed:
rg "FetchResponse|FetchContext" src/types.ts -A 5 -B 2 | head -60Repository: unjs/ofetch
Length of output: 1735
🏁 Script executed:
cat -n src/fetch.ts | sed -n '215,235p'Repository: unjs/ofetch
Length of output: 875
🏁 Script executed:
cat -n src/fetch.ts | sed -n '219,262p'Repository: unjs/ofetch
Length of output: 1812
🏁 Script executed:
node - <<'NODE'
// Test if cloning response preserves url
const response = {
url: "http://example.com/api",
status: 200,
statusText: "OK",
headers: new Map(),
body: null
};
console.log("Original response has url:", "url" in response);
// Simulate what happens with new Response()
const reconstructed = {};
Object.assign(reconstructed, {
status: response.status,
statusText: response.statusText,
headers: response.headers
});
console.log("Reconstructed response has url:", "url" in reconstructed);
console.log("Response.url is read-only:", Object.getOwnPropertyDescriptor(Response.prototype, 'url')?.writable === false);
NODERepository: unjs/ofetch
Length of output: 157
🏁 Script executed:
cat -n src/fetch.ts | sed -n '208,225p'Repository: unjs/ofetch
Length of output: 817
🏁 Script executed:
rg "test.*onDownloadProgress" -A 20 test/index.test.ts | head -50Repository: unjs/ofetch
Length of output: 37
🏁 Script executed:
node - <<'NODE'
// Test if response.clone() allows reading body twice
const response = new Response("test body");
// First clone for reading
const clone1 = response.clone();
const text1 = await clone1.text();
console.log("First clone read:", text1);
// Original still readable
const text2 = await response.text();
console.log("Original still readable:", text2);
// Both have same content
console.log("Both readable:", text1 === text2);
NODERepository: unjs/ofetch
Length of output: 137
🏁 Script executed:
cat -n src/fetch.ts | sed -n '85,115p'Repository: unjs/ofetch
Length of output: 1086
🏁 Script executed:
rg "return.*context" src/fetch.ts -B 2 -A 2Repository: unjs/ofetch
Length of output: 471
🏁 Script executed:
cat -n src/fetch.ts | sed -n '310,330p'Repository: unjs/ofetch
Length of output: 718
Preserve the original Response object for $fetch.raw.
When onDownloadProgress is used, the code reconstructs the response with new Response(...) after consuming the body for progress tracking. This loses the url property that the native fetch API automatically sets, causing $fetch.raw(..., { onDownloadProgress }) to expose a response with url = undefined while the same request without progress tracking would have the correct url. Instead of reconstructing the response, clone it before reading the body to preserve the original object with all its properties intact.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fetch.ts` around lines 257 - 261, The current code replaces
context.response with a newly constructed Response after consuming the body for
onDownloadProgress, which loses native fields like url; instead, before reading
the body for progress tracking in the onDownloadProgress handling (the code
paths manipulating context.response and reading body), call context.response =
context.response.clone() or clone the original response into a local variable,
consume the clone for progress tracking, and keep the original Response object
(or set context.response to the original) so $fetch.raw returns the intact
native Response with its url and other properties; update the logic around
onDownloadProgress and the places that currently do new Response(...) to use
Response.clone() and avoid reconstructing the response.
| it("reports total from content-length header", async () => { | ||
| const progress: Array<{ | ||
| transferred: number; | ||
| total?: number; | ||
| percent?: number; | ||
| }> = []; | ||
| await $fetch(getURL("ok"), { | ||
| onDownloadProgress(p) { | ||
| progress.push({ ...p }); | ||
| }, | ||
| }); | ||
| const last = progress.at(-1)!; | ||
| if (last.total) { | ||
| expect(last.percent).toBeDefined(); | ||
| expect(last.percent).toBeGreaterThan(0); | ||
| expect(last.percent).toBeLessThanOrEqual(1); | ||
| } |
There was a problem hiding this comment.
Assert total unconditionally.
Line 537 makes this test a no-op when Content-Length is missing, so it still passes if onDownloadProgress regresses to never reporting total. Prefer a handler with a known Content-Length and assert last.total and last.percent directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/index.test.ts` around lines 525 - 541, The test "reports total from
content-length header" currently skips assertions when last.total is undefined;
update the test to call $fetch against an endpoint that always returns a
Content-Length (e.g., use a fixture/getURL variant that sets Content-Length) and
remove the conditional so you unconditionally assert last.total is defined and
then assert last.percent is defined and within (0, 1] using the existing
progress/last variables and the onDownloadProgress handler; ensure you reference
the existing test name, the $fetch call, getURL, and the progress array when
making the change.
2 participants
Summary
onDownloadProgressoption for tracking download progress{ transferred, total, percent }via callbacktotalfrom Content-Length header (undefined if missing)Usage
New types
Test plan
onDownloadProgresscalled during response body readingResolves #45
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests