Repository navigation
Conversation
🦋 Changeset detectedLatest commit: ecbd40e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
WalkthroughThe middleware now returns multipart 206 responses for multiple byte-range requests when there are at most 200 ranges and their combined coverage is less than the file size. A new helper builds the multipart response body using streams or a cached file buffer. Tests cover GET and HEAD requests, mixed satisfiable ranges, all-unsatisfiable ranges, and requests with more than 200 ranges. The utility also has a TypeScript declaration and a changeset describing the response behavior. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Multi-range requests now return multipart Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly and concisely describes the main change: adding multipart range request support. |
| Docstring Coverage | |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 4 files. (1 skipped: 1 … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches 💡 1
-
🛠️ Fix failing CI checks 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 ast-grep (0.45.3)
-
test/middleware.test.js
-
ast-grep timed out on this file
-
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
1c10354e-7f13-48b5-817f-a1cd62d7dd7f
📒 Files selected for processing (5)
.changeset/multipart-range-requests.mdsrc/middleware.jssrc/utils.jstest/middleware.test.jstypes/utils.d.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| "webpack-dev-middleware": minor | ||
| --- | ||
|
|
||
| Added support for `multipart/byteranges` responses. A `Range` header with several separate ranges now returns a `206` with one part per range instead of the whole file. Requests with more than 200 ranges, or ranges that cover the whole file, still get a normal `200`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the full-coverage status claim.
For a 10-byte file, Range: bytes=0-4,5-9 merges into one range and returns 206, not 200. The new full-coverage check only runs when multiple ranges remain. Remove or qualify the claim so the release note describes the implemented behavior. (raw.githubusercontent.com)
| } | ||
|
|
||
| if (shouldUseMultipart) { | ||
| const contentType = context.mimeTypes.contentType(filename); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use the selected response type for each part.
If headers sets Content-Type, or mime.default supplies it for an unknown extension, the filename-derived value can differ from the type a 200 response would use. Each multipart part then has the wrong type or no type. Read the established response Content-Type before replacing the outer header with multipart/byteranges. RFC 9110 recommends carrying that representation type in each part. (rfc-editor.org)
| cachedFullBuffer = /** @type {Buffer} */ ( | ||
| outputFileSystem.readFileSync(filename) | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle synchronous read failures before creating the response.
If a file disappears after the middleware stats it, readFileSync(filename) can throw. The multipart call in src/middleware.js is outside the ordinary read’s try block. The request then bypasses errorHandler instead of receiving the file-read error response. Catch this read failure at the call site or handle it within createMultipartBody.
| for (let i = 0; i < 201; i++) { | ||
| ranges.push(`bytes=${i * 2}-${i * 2 + 1}`); | ||
| } | ||
| const response = await req | ||
| .get("/bundle.js") | ||
| .set("Range", ranges.join(", ")); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Construct 201 valid, separate ranges for the cap test.
ranges.join(", ") produces bytes=0-1, bytes=2-3, .... range-parser skips the entries with the repeated bytes= prefix, so this request does not reach the cap and takes the single-range 206 path. Removing those prefixes alone is insufficient: the adjacent intervals merge into one range. Use one bytes= prefix and leave gaps between the 201 intervals. (raw.githubusercontent.com)
|
Fully AI generated PR with bugs |
2 participants
Summary
Fixes #
A
Rangeheader with several separate ranges used to be ignored, and the middleware sent the whole file with a200. For very large files (the reporter mentions game data files over 1 GB) that defeats the purpose of range requests.This PR answers such requests with a
206and amultipart/byterangesbody, as described in RFC 9110 section 14.6:Content-TypeandContent-Range;Content-Lengthis calculated up front, so the response is streamed without chunked encoding;Rangeheader is ignored and the whole file is sent with a200, to avoid abuse;HEADreturns the same headers with no body.What kind of change does this PR introduce?
feat
Did you add tests for your changes?
Yes, in
test/middleware.test.js, run across all supported frameworks: a multipart206for two separate ranges,HEADwith multiple ranges, one valid and one unsatisfiable range, all ranges unsatisfiable (416), and more ranges than the cap (200). The existing tests for merged ranges still pass.Does this PR introduce a breaking change?
No API changes. Behavior changes only for requests with several separate ranges: they now get a
206multipart response instead of a200with the full file, which is what the HTTP spec describes. Single-range requests are unchanged.If relevant, what needs to be documented once your changes are merged or what have you already documented?
I added a "Range requests" section to the README, including how
modifyResponseDatabehaves with a multipart body. A changeset (minor) is included.Use of AI
I used Claude (Anthropic) to help analyze the issue, plan the approach and draft the first version of the implementation and tests. I reviewed the changes, ran lint, and ran the tests myself, and I can explain how the code works.
Summary by CodeRabbit
206 Partial Contentresponse with a separate part for each satisfiable range, including forHEADrequests.200response. Requests where all ranges are unsatisfiable receive416.