feat(fetch): add runtime-backed adapters and native fetch primitives#12
Merged
feat(fetch): add runtime-backed adapters and native fetch primitives#12
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors the fetch layer into platform-targeted modules: introduces a first-class Body type, RequestInput/RequestInit and Request/Response native/io/js implementations, replaces in-file Headers/Blob/FormData/URLSearchParams with conditional/platform variants, adds web JS-interop and stream utilities, and updates examples/tests to the new APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant RequestCtor as Request(factory)
participant Native as Request.native
participant Headers as Headers (platform)
participant Body as Body
participant Host as Platform Host
App->>RequestCtor: Request(RequestInput.uri(uri), RequestInit(...))
RequestCtor->>Native: normalize RequestInput & RequestInit
Native->>Headers: Headers.from(init.headers)
Native->>Body: Body.from(init.body)
Native->>Host: retain runtime-backed host if provided
App->>Native: access headers/text/json
Native->>Headers: delegate reads/writes
Headers->>Host: forward to runtime backing (if present)
Host-->>Headers: header values
Headers-->>Native: entries/result
Native-->>App: return headers/body/metadata
sequenceDiagram
participant App as Application
participant FormDataAPI as FormData
participant Parser as Multipart Parser
participant Multipart as Multipart (Text/Blob)
participant Encoder as Multipart Encoder
participant Encoded as EncodedFormData
App->>FormDataAPI: FormData.parse(body, contentType)
FormDataAPI->>Parser: choose multipart or urlencoded
Parser->>Multipart: produce entries (TextMultipart/BlobMultipart)
Parser-->>FormDataAPI: entries built
App->>FormDataAPI: encodeMultipart()
FormDataAPI->>Encoder: serialize parts into stream
Encoder-->>Encoded: stream + contentLength
Encoded-->>App: EncodedFormData (stream + headers)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (12)
lib/src/fetch/blob.js.dart (1)
23-29: Avoid per-part Blob wrapping in the normalization hot path.Line 28 allocates
native.Blob([part])for every non-Blob value. Adding primitive fast paths keeps small payload construction cheaper.♻️ Suggested refactor
+import 'dart:typed_data'; + import 'package:block/block.dart' as block; import 'package:web/web.dart' as web; import 'blob.native.dart' as native; @@ static Object _normalizePart(native.BlobPart part) { return switch (part) { final Blob blob => blob, final native.Blob blob => blob, final web.Blob blob => blob, + final String text => text, + final ByteBuffer buffer => ByteData.sublistView(buffer.asUint8List()), + final ByteData data => ByteData.sublistView( + data.buffer.asUint8List(data.offsetInBytes, data.lengthInBytes), + ), + final Uint8List bytes => Uint8List.fromList(bytes), + final List<int> bytes => Uint8List.fromList(bytes), _ => native.Blob([part]), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/blob.js.dart` around lines 23 - 29, The _normalizePart hot path currently allocates native.Blob([part]) for every non-Blob value; change _normalizePart to handle common primitive/byte types explicitly (e.g., String, Uint8List/List<int>, ByteBuffer/ByteData, and Iterable<int>) and return them in their native representations (or convert to a Uint8List/ByteBuffer) instead of wrapping each item in a new native.Blob; only fall back to native.Blob([part]) for truly unknown types. Update the switch in _normalizePart to add cases for String, Uint8List (and List<int>), ByteBuffer/ByteData, and Iterable<int>, performing minimal conversions (avoid per-part Blob allocations) and keep the existing Blob/native.Blob/web.Blob branches unchanged.lib/src/fetch/headers.native.dart (1)
21-22: Copying from upstream bypasses validation.Line 22 directly adds entries from
upstream._hostwithout going throughappend(), which means validation/normalization is bypassed. Whileupstreamshould already contain valid entries, this creates an inconsistency where the constructor path for existingHeadersdiffers from other init forms.If
Headersinstances can be subclassed or the internal representation ever changes, this could propagate invalid data.♻️ Use append for consistency
case final Headers upstream: - headers._host.addAll(upstream._host); + for (final MapEntry(:key, :value) in upstream._host) { + headers.append(key, value); + }This adds overhead but ensures all paths validate consistently. Given that
upstreamshould already be valid, the current approach is acceptable for performance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/headers.native.dart` around lines 21 - 22, The constructor path copies entries directly via headers._host.addAll(upstream._host), bypassing validation; change this to iterate the upstream headers and call Headers.append(key, value) (or the instance method append) for each entry so values are normalized/validated consistently with other init paths—update the block handling case Headers upstream to append each header from upstream instead of using _host.addAll.lib/src/fetch/headers.io.dart (2)
106-114:has()for HttpHeaders may miss multi-value headers with null first value.The
has()implementation forHttpHeadersHost(line 110) useshost.value.value(name) != null. TheHttpHeaders.value()method returns the first value or null. If a header has multiple values but the first is null (unusual but possible), this could incorrectly return false.Consider using
host.value[name]?.isNotEmpty ?? falsefor consistency with how headers are actually stored.♻️ More robust has() check
case final HttpHeadersHost host: - return host.value.value(name) != null; + final values = host.value[name]; + return values != null && values.isNotEmpty;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/headers.io.dart` around lines 106 - 114, The has() method's HttpHeadersHost branch uses host.value.value(name) != null which can miss multi-value headers when the first value is null; update the HttpHeadersHost case in has() (the switch on _host) to check the list for non-emptiness instead, e.g. use host.value[name]?.isNotEmpty ?? false (leave the NativeHeadersHost branch unchanged) so the existence check correctly handles multi-value headers.
58-72: Consider streaming entries directly instead of collecting to list.For
HttpHeadersHost, entries are collected into a list (lines 62-67) before being yielded. Since this is async*generator, you could yield directly within theforEachcallback:♻️ Alternative streaming implementation
case final HttpHeadersHost host: - final entries = <MapEntry<String, String>>[]; - host.value.forEach((name, values) { - for (final value in values) { - entries.add(MapEntry(name, value)); - } - }); - yield* entries; + for (final name in host.value.map((n, _) => MapEntry(n, null)).keys) { + final values = host.value[name]; + if (values != null) { + for (final value in values) { + yield MapEntry(name, value); + } + } + }Note:
HttpHeaders.forEachuses a callback which can't directly yield, so the current approach may be necessary. This is a minor point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/headers.io.dart` around lines 58 - 72, In the entries() generator, avoid collecting HttpHeadersHost values into a temporary list; instead iterate the header map and yield MapEntry items directly from the HttpHeadersHost branch (replace the host.value.forEach(...) + entries list with a direct iteration over host.value or host.value.entries and yield each MapEntry), leaving the NativeHeadersHost branch unchanged; update the entries() method to stream entries without building the intermediate list.test/request_native_test.dart (1)
3-8: Direct import of internal module may be fragile.Line 5 imports
form_data.native.dartdirectly rather than through a barrel file (likeheaders.dartandblob.darton lines 4 and 6). Ifform_data.native.dartisn't part of the public API surface, this test could break when internal structure changes.Consider adding a
form_data.dartbarrel file consistent with the other fetch primitives, or verify this is intentionally part of the public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/request_native_test.dart` around lines 3 - 8, The test directly imports the internal module form_data.native.dart which is fragile; update the test to import the public barrel (create or use a form_data.dart) so the fetch primitives are imported consistently like headers.dart and blob.dart; either add a new barrel file export (e.g., export 'form_data.native.dart';) and update the test import to 'package:ht/src/fetch/form_data.dart', or confirm form_data.native.dart is intentionally public and instead re-export it from an existing public barrel to avoid brittle internal imports.lib/src/fetch/form_data.native.dart (2)
25-41: Duplicate filename computation in constructor.The same
switchexpression for derivingfilenameappears twice: once for thefilenamefield initializer (lines 27-30) and again in thesupercall (lines 33-36). This duplication could lead to inconsistency if one is modified without the other.♻️ Extract to static helper or use initializer list
final class BlobMultipart extends File implements Multipart { - BlobMultipart(Blob value, [String? filename]) - : filename = switch (value) { - final File file => filename ?? file.name, - _ => filename ?? 'blob', - }, - super( - <BlobPart>[value], - switch (value) { - final File file => filename ?? file.name, - _ => filename ?? 'blob', - }, - type: value.type, - ); + BlobMultipart(Blob value, [String? filename]) + : this._internal( + value, + switch (value) { + final File file => filename ?? file.name, + _ => filename ?? 'blob', + }, + ); + + BlobMultipart._internal(Blob value, this.filename) + : super(<BlobPart>[value], filename, type: value.type); final String filename; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/form_data.native.dart` around lines 25 - 41, BlobMultipart's constructor duplicates the filename computation; compute filename once and reuse it to avoid divergence — either extract the logic into a small static helper (e.g., a private static String _computeFilename(Blob value, String? filename)) and call it for both the filename field and the super call, or compute a final local in the initializer list (e.g., final computedFilename = switch(...) ) and pass that to both the filename field and super; update references to use that single computed value and keep symbols BlobMultipart, filename, constructor, super, File, and BlobPart intact.
214-217: Incorrect offset advancement after finding boundary.On line 216,
offsetis incremented by 2 (offset = nextBoundary + 2), butnextBoundarypoints to the start ofboundaryPrefixwhich is'\r\n--$boundary'. The loop then addsboundaryMarker.lengthon line 181, expecting to be at the start of--$boundary.The current logic advances past the
\r\nprefix (2 bytes) which is correct for positioning at the--boundarymarker. However, this is subtle and could be clearer with a named constant or comment.📝 Add clarifying comment
- offset = nextBoundary + 2; + // Skip past \r\n prefix of boundaryPrefix to position at --$boundary + offset = nextBoundary + 2; // 2 = '\r\n'.length🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/form_data.native.dart` around lines 214 - 217, The offset advancement after finding a boundary is unclear: replace the magic "+ 2" in the assignment "offset = nextBoundary + 2" with a self-descriptive constant (e.g. CRLF_LENGTH = 2) or add an inline comment explaining that the +2 skips the "\r\n" prefix so the next addition of "boundaryMarker.length" positions the pointer at the start of the "--boundary" marker; update the code around the loop that uses nextBoundary, boundaryPrefix and boundaryMarker to use that constant or comment for clarity.lib/src/fetch/url_search_params.js.dart (1)
45-53: JS array destructuring relies on runtime type assumption.Line 50 casts
result.valuetoJSArray<JSString>and destructures with[name, value]. This assumes the JS URLSearchParams iterator always yields two-element string arrays, which is correct per spec but could throw at runtime if the underlying JS object is malformed.Consider adding a brief comment or defensive check for clarity:
// URLSearchParams.entries() yields [key, value] tuples per spec. final [name, value] = (result.value as JSArray<JSString>).toDart;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/url_search_params.js.dart` around lines 45 - 53, The JS array destructuring in _entries() assumes result.value is a two-element JSArray<JSString> (cast to JSArray<JSString> and deconstructed), which can throw if the JS object is malformed; add a short comment clarifying the assumption (e.g., "URLSearchParams.entries() yields [key, value] tuples per spec") and add a defensive check before destructuring: verify result.value is a JSArray with length >= 2 (or fallback to treating missing elements as empty strings) so _entries() and the cast of result.value and use of name/value are safe; reference _entries(), _host.entries(), result.value, and JSArray<JSString> when making the change.lib/src/fetch/url_search_params.native.dart (1)
93-96: Consider atomicity ofset()operation.The
set()method callsdelete(name)followed byappend(name, value). While functionally correct, if an exception occurred between these calls (unlikely here), the entry would be lost. This is fine for this implementation but worth noting for consistency with MDN semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/url_search_params.native.dart` around lines 93 - 96, The set() method currently calls delete(name) then append(name, value), which is not atomic; change set(String name, String value) to perform the replace in a single update so the entry cannot be lost between operations: locate existing entries for name in the internal storage (or build a new list of pairs filtering out the name) and then insert the single (name,value) pair into that list before assigning it back, or replace the first matching pair in-place and remove any additional duplicates — update the logic in set to use the class's internal pair/list representation rather than separate delete/append calls.test/public_api_surface_test.dart (1)
29-29: Type change fromBodyInittoObjectis intentional.The change from
final BodyInit init = 'x'tofinal Object init = 'x'aligns withBodyInitnot being re-exported throughRequestInit's public surface. However,BodyInitis still exported frombody.dartperlib/ht.dart. Consider whether this test should verifyBodyInitusability separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/public_api_surface_test.dart` at line 29, The test changed the variable type from BodyInit to Object to reflect that BodyInit is no longer part of RequestInit's public surface; update the test to explicitly assert whichever public surface you want to guarantee: either revert the declaration to "final BodyInit init = 'x';" if BodyInit should remain publicly usable (and ensure BodyInit is exported where the test imports from), or keep "final Object init = 'x';" and add a separate test that imports BodyInit directly and verifies it is available (e.g., declare a BodyInit-typed variable) so the public API surface for BodyInit is validated independently of RequestInit.lib/src/fetch/blob.native.dart (1)
77-80: ByteData normalization may be unnecessary.Since
block.BlockacceptsByteDatanatively, theByteData.sublistView()wrapper on line 78-80 appears redundant. The code currently normalizesByteDatawith non-zero offsets by extracting aUint8Listview and re-wrapping it, but this may not be required. Consider passingByteDatadirectly, or if offset normalization is necessary, clarify that intent with a comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/blob.native.dart` around lines 77 - 80, The ByteData normalization using ByteData.sublistView in the getters (the expressions returning ByteBuffer via ByteData.sublistView(buffer.asUint8List()) and ByteData via ByteData.sublistView(data.buffer.asUint8List(...))) is likely redundant because block.Block accepts ByteData directly; either remove the sublistView wrappers and return the existing ByteBuffer/ByteData values directly, or if normalization for non-zero offsets is required, keep the logic but add a clarifying comment above the data getter explaining why you're re-wrapping (and ensure you return a contiguous ByteData with correct offset/length), referencing the getters named buffer and data so reviewers can find the change.lib/src/_internal/web_utils.dart (1)
44-59: Inefficient triple iteration over lazy iterables.
kv.lengthiterates the entire iterable, thenkv.elementAt(0)andkv.elementAt(1)each iterate from the start. For lazy or single-pass iterables, this causes three separate iterations and could even fail if the iterable isn't replayable.♻️ Proposed fix using iterator directly
factory Headers.fromStringPairs(Iterable<Iterable<String>> pairs) { final headers = Headers(); for (final kv in pairs) { - if (kv.length != 2) { + final iter = kv.iterator; + if (!iter.moveNext()) { throw ArgumentError.value( kv, 'pairs', 'Header pairs must contain exactly two string items.', ); } - - headers.append(kv.elementAt(0), kv.elementAt(1)); + final name = iter.current; + if (!iter.moveNext()) { + throw ArgumentError.value( + kv, + 'pairs', + 'Header pairs must contain exactly two string items.', + ); + } + final value = iter.current; + if (iter.moveNext()) { + throw ArgumentError.value( + kv, + 'pairs', + 'Header pairs must contain exactly two string items.', + ); + } + headers.append(name, value); } return headers; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/_internal/web_utils.dart` around lines 44 - 59, In Headers.fromStringPairs replace the triple-iteration over each kv by obtaining an iterator from kv and pulling the first two items via iterator.moveNext()/current (or equivalent) to validate exactly two elements and avoid multiple traversals; if the iterator has fewer than two elements throw the same ArgumentError, after reading the two values call headers.append(name, value), and finally check iterator.moveNext() to detect and reject extra elements so lazy or single-pass iterables are handled correctly (update function Headers.fromStringPairs and references to kv/headers.append accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/fetch/blob.io.dart`:
- Around line 36-38: The constructor for _FileBlock currently calls synchronous
I/O via _file.lengthSync() (in _FileBlock(this._file, {int start = 0, int?
length, this.type = ''}) : _start = start, _length = length ??
_file.lengthSync();), which can block the event loop; change this to avoid
synchronous file access by making _length nullable and resolving file size
lazily or asynchronously: either accept a required length parameter, provide a
factory/async initializer that awaits _file.length(), or keep _length null and
implement a getter (e.g., size) that caches and returns _file.lengthSync() only
when accessed (or uses asynchronous length resolution), and update any callers
that rely on an immediate length to use the async factory or the lazy getter.
In `@lib/src/fetch/body.dart`:
- Around line 47-50: The current factory/getter creating a new Body via
Body._(blockHost: body._blockHost, streamHost: body._streamHost) copies the
_streamHost reference which can cause stream aliasing and inconsistent _used
state; update this construction to use clone semantics instead of copying the
stream reference (e.g., call body.clone() or create a new stream-host instance
from body.clone()) so the new Body gets an independent streamHost and _used
state; ensure the change targets the Body._ usage in the Body construction path
and that Body.clone() (or equivalent) returns an independent streamHost and
preserves blockHost as needed.
- Around line 88-105: The stream getter currently declares Stream<Uint8List>?
but uses an async* body (involving _blockHost, _streamHost and
_startConsumption), which always returns a non-null Stream; change the signature
of the getter named stream to return Stream<Uint8List> (non-nullable), replace
the early "return" path with yielding an empty stream (e.g., use yield break or
just let the function fall through) so the async* produces an empty Stream when
both _blockHost and _streamHost are null, and then update all callers (for
example bytes() and listen()) to remove nullable checks and handling around
stream being null. Ensure references to _blockHost.stream() and yield*
streamHost remain unchanged except for the non-nullable stream type.
In `@lib/src/fetch/headers.js.dart`:
- Around line 62-63: The JS implementation of Headers.get currently delegates to
host.get(name) in Headers.get, causing get('set-cookie') to return a value
unlike native/IO; change Headers.get to special-case the header name
'set-cookie' (case-insensitive) and return null for that name (so callers must
use getSetCookie()) to match native/IO behavior, or if you prefer to keep the
current behavior add a clear platform note in the header class docs; locate the
get method in headers.js.dart and adjust the logic around host.get(name) and the
getSetCookie helper accordingly.
- Around line 47-60: The entries() iterator currently skips values when
web.Array.isArray(result.value!) is true, which wrongly filters out the valid
[name, value] arrays; update the check in entries() to continue only when
result.value is null/undefined or NOT an array (i.e., invert the
web.Array.isArray check) so you only attempt to destructure into
JSArray<JSString> when result.value is an array, keeping the rest of the loop
(using host.entries(), iterator.next(), result.value, JSArray<JSString>.toDart
and yielding MapEntry(name.toDart, value.toDart)) unchanged.
In `@lib/src/fetch/request.io.dart`:
- Around line 54-63: The body getter inconsistently caches only for
HttpRequestHost causing varying instances for NativeRequestHost; modify the
NativeRequestHost arm in the body getter to assign the resolved body into the
_body field before returning (e.g., obtain host.value.body into a local, set
_body = that value, then return _body) so both HttpRequestHost and
NativeRequestHost populate and reuse _body consistently; update the switch arms
for HttpRequestHost and NativeRequestHost in the body getter to store the result
into _body (handling nullable Body? appropriately).
In `@lib/src/fetch/request.native.dart`:
- Around line 364-370: The helper _urlFromInput currently calls Uri.parse(value)
which can throw FormatException for invalid strings; update _urlFromInput to use
Uri.tryParse(value) and if the result is null either return a safe fallback
(e.g., rethrow a new, descriptive FormatException mentioning the invalid URL and
the Request input) or throw an explicit error before it escapes the Request
constructor so callers get a clear message; ensure you update the branches
handling StringRequestInput and reference the function name _urlFromInput and
the RequestRequestInput/StringRequestInput/UriRequestInput variants when making
the change.
---
Nitpick comments:
In `@lib/src/_internal/web_utils.dart`:
- Around line 44-59: In Headers.fromStringPairs replace the triple-iteration
over each kv by obtaining an iterator from kv and pulling the first two items
via iterator.moveNext()/current (or equivalent) to validate exactly two elements
and avoid multiple traversals; if the iterator has fewer than two elements throw
the same ArgumentError, after reading the two values call headers.append(name,
value), and finally check iterator.moveNext() to detect and reject extra
elements so lazy or single-pass iterables are handled correctly (update function
Headers.fromStringPairs and references to kv/headers.append accordingly).
In `@lib/src/fetch/blob.js.dart`:
- Around line 23-29: The _normalizePart hot path currently allocates
native.Blob([part]) for every non-Blob value; change _normalizePart to handle
common primitive/byte types explicitly (e.g., String, Uint8List/List<int>,
ByteBuffer/ByteData, and Iterable<int>) and return them in their native
representations (or convert to a Uint8List/ByteBuffer) instead of wrapping each
item in a new native.Blob; only fall back to native.Blob([part]) for truly
unknown types. Update the switch in _normalizePart to add cases for String,
Uint8List (and List<int>), ByteBuffer/ByteData, and Iterable<int>, performing
minimal conversions (avoid per-part Blob allocations) and keep the existing
Blob/native.Blob/web.Blob branches unchanged.
In `@lib/src/fetch/blob.native.dart`:
- Around line 77-80: The ByteData normalization using ByteData.sublistView in
the getters (the expressions returning ByteBuffer via
ByteData.sublistView(buffer.asUint8List()) and ByteData via
ByteData.sublistView(data.buffer.asUint8List(...))) is likely redundant because
block.Block accepts ByteData directly; either remove the sublistView wrappers
and return the existing ByteBuffer/ByteData values directly, or if normalization
for non-zero offsets is required, keep the logic but add a clarifying comment
above the data getter explaining why you're re-wrapping (and ensure you return a
contiguous ByteData with correct offset/length), referencing the getters named
buffer and data so reviewers can find the change.
In `@lib/src/fetch/form_data.native.dart`:
- Around line 25-41: BlobMultipart's constructor duplicates the filename
computation; compute filename once and reuse it to avoid divergence — either
extract the logic into a small static helper (e.g., a private static String
_computeFilename(Blob value, String? filename)) and call it for both the
filename field and the super call, or compute a final local in the initializer
list (e.g., final computedFilename = switch(...) ) and pass that to both the
filename field and super; update references to use that single computed value
and keep symbols BlobMultipart, filename, constructor, super, File, and BlobPart
intact.
- Around line 214-217: The offset advancement after finding a boundary is
unclear: replace the magic "+ 2" in the assignment "offset = nextBoundary + 2"
with a self-descriptive constant (e.g. CRLF_LENGTH = 2) or add an inline comment
explaining that the +2 skips the "\r\n" prefix so the next addition of
"boundaryMarker.length" positions the pointer at the start of the "--boundary"
marker; update the code around the loop that uses nextBoundary, boundaryPrefix
and boundaryMarker to use that constant or comment for clarity.
In `@lib/src/fetch/headers.io.dart`:
- Around line 106-114: The has() method's HttpHeadersHost branch uses
host.value.value(name) != null which can miss multi-value headers when the first
value is null; update the HttpHeadersHost case in has() (the switch on _host) to
check the list for non-emptiness instead, e.g. use host.value[name]?.isNotEmpty
?? false (leave the NativeHeadersHost branch unchanged) so the existence check
correctly handles multi-value headers.
- Around line 58-72: In the entries() generator, avoid collecting
HttpHeadersHost values into a temporary list; instead iterate the header map and
yield MapEntry items directly from the HttpHeadersHost branch (replace the
host.value.forEach(...) + entries list with a direct iteration over host.value
or host.value.entries and yield each MapEntry), leaving the NativeHeadersHost
branch unchanged; update the entries() method to stream entries without building
the intermediate list.
In `@lib/src/fetch/headers.native.dart`:
- Around line 21-22: The constructor path copies entries directly via
headers._host.addAll(upstream._host), bypassing validation; change this to
iterate the upstream headers and call Headers.append(key, value) (or the
instance method append) for each entry so values are normalized/validated
consistently with other init paths—update the block handling case Headers
upstream to append each header from upstream instead of using _host.addAll.
In `@lib/src/fetch/url_search_params.js.dart`:
- Around line 45-53: The JS array destructuring in _entries() assumes
result.value is a two-element JSArray<JSString> (cast to JSArray<JSString> and
deconstructed), which can throw if the JS object is malformed; add a short
comment clarifying the assumption (e.g., "URLSearchParams.entries() yields [key,
value] tuples per spec") and add a defensive check before destructuring: verify
result.value is a JSArray with length >= 2 (or fallback to treating missing
elements as empty strings) so _entries() and the cast of result.value and use of
name/value are safe; reference _entries(), _host.entries(), result.value, and
JSArray<JSString> when making the change.
In `@lib/src/fetch/url_search_params.native.dart`:
- Around line 93-96: The set() method currently calls delete(name) then
append(name, value), which is not atomic; change set(String name, String value)
to perform the replace in a single update so the entry cannot be lost between
operations: locate existing entries for name in the internal storage (or build a
new list of pairs filtering out the name) and then insert the single
(name,value) pair into that list before assigning it back, or replace the first
matching pair in-place and remove any additional duplicates — update the logic
in set to use the class's internal pair/list representation rather than separate
delete/append calls.
In `@test/public_api_surface_test.dart`:
- Line 29: The test changed the variable type from BodyInit to Object to reflect
that BodyInit is no longer part of RequestInit's public surface; update the test
to explicitly assert whichever public surface you want to guarantee: either
revert the declaration to "final BodyInit init = 'x';" if BodyInit should remain
publicly usable (and ensure BodyInit is exported where the test imports from),
or keep "final Object init = 'x';" and add a separate test that imports BodyInit
directly and verifies it is available (e.g., declare a BodyInit-typed variable)
so the public API surface for BodyInit is validated independently of
RequestInit.
In `@test/request_native_test.dart`:
- Around line 3-8: The test directly imports the internal module
form_data.native.dart which is fragile; update the test to import the public
barrel (create or use a form_data.dart) so the fetch primitives are imported
consistently like headers.dart and blob.dart; either add a new barrel file
export (e.g., export 'form_data.native.dart';) and update the test import to
'package:ht/src/fetch/form_data.dart', or confirm form_data.native.dart is
intentionally public and instead re-export it from an existing public barrel to
avoid brittle internal imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 075615f0-a535-476d-91c1-79becede5a9c
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
example/main.dartlib/ht.dartlib/src/_internal/stream_tee.dartlib/src/_internal/web_utils.dartlib/src/fetch/blob.dartlib/src/fetch/blob.io.dartlib/src/fetch/blob.js.dartlib/src/fetch/blob.native.dartlib/src/fetch/body.dartlib/src/fetch/file.dartlib/src/fetch/form_data.dartlib/src/fetch/form_data.native.dartlib/src/fetch/headers.dartlib/src/fetch/headers.io.dartlib/src/fetch/headers.js.dartlib/src/fetch/headers.native.dartlib/src/fetch/request.dartlib/src/fetch/request.io.dartlib/src/fetch/request.native.dartlib/src/fetch/response.dartlib/src/fetch/response.native.dartlib/src/fetch/url_search_params.dartlib/src/fetch/url_search_params.js.dartlib/src/fetch/url_search_params.native.dartpubspec.yamltest/_internal/stream_tee_test.darttest/blob_io_test.darttest/blob_js_test.darttest/body_test.darttest/form_data_native_test.darttest/form_data_test.darttest/headers_test.darttest/public_api_surface_test.darttest/request_io_test.darttest/request_native_test.darttest/request_response_test.darttest/response_native_test.darttest/url_search_params_js_test.darttest/url_search_params_test.dart
💤 Files with no reviewable changes (4)
- test/form_data_test.dart
- lib/src/fetch/response.dart
- test/request_response_test.dart
- lib/src/fetch/form_data.dart
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/src/fetch/body.dart (1)
47-50:⚠️ Potential issue | 🟠 MajorAvoid shallow-copying internal hosts in
Body(Body)construction.This still aliases
_streamHostand bypasses consumed-state enforcement. One instance can consume the stream while the sibling instance appears unused.🔧 Proposed fix
- final Body body => Body._( - blockHost: body._blockHost, - streamHost: body._streamHost, - ), + final Body body => body.clone(),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/body.dart` around lines 47 - 50, The Body copy currently shallow-copies internal hosts (Body._ with blockHost: body._blockHost, streamHost: body._streamHost), which aliases _streamHost and allows one instance to consume the stream for both; fix by ensuring we do not alias internal hosts when constructing from another Body: either clone/rewind the stream host (call a clone/rewind method on body._streamHost, e.g., _streamHost.clone() or create a new StreamHost wrapper) or transfer ownership by moving the stream host and marking the source consumed (e.g., body.markConsumed() or set body._streamHost = null) so the new Body gets an independent streamHost; update Body._ construction to use the cloned/new host instead of reusing body._streamHost and handle blockHost similarly if it has mutable/consumable state.
🧹 Nitpick comments (2)
lib/src/fetch/request.js.dart (2)
61-75: Body getter has asymmetric caching behavior.For
WebRequestHost, the body is converted from a web stream and cached in_body. ForNativeRequestHost, the body is returned directly fromhost.value.bodywithout caching. This is likely intentional since the native body is already aBodyobject, but the asymmetry means_bodyis never populated for native hosts.This could cause subtle issues if code later checks
_body != nullto determine if a body wrapper was created. Currently, the early-return check at line 63-64 handles this correctly by checking_bodyfirst, so this is not a bug—just worth noting for future maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/request.js.dart` around lines 61 - 75, The Body getter currently caches the converted web stream into _body for WebRequestHost but returns host.value.body directly for NativeRequestHost, leaving _body unset; update the NativeRequestHost branch in the body getter (the switch handling NativeRequestHost / host.value.body) to assign host.value.body to _body when non-null and then return it (i.e., set _body = host.value.body before returning) so _body is populated consistently for both WebRequestHost and NativeRequestHost.
250-258: Consider using web host's native json() for consistency.The
json()method delegates tobody.json<T>()for all hosts, whiletext(),bytes(), andblob()haveWebRequestHost-specific optimizations that call the web host's native methods directly (e.g.,web_fetch.textFromWebPromise(host.value.text())).For consistency and potential performance benefits, consider adding a web-native path:
♻️ Suggested refactor
`@override` Future<T> json<T>() { - return switch (body) { - final Body body => body.json<T>(), - null => Future<T>.error( - const FormatException('Cannot decode JSON from an empty body.'), - ), + return switch (_host) { + final WebRequestHost host => web_fetch.jsonFromWebPromise<T>( + host.value.json(), + ), + _ => switch (body) { + final Body body => body.json<T>(), + null => Future<T>.error( + const FormatException('Cannot decode JSON from an empty body.'), + ), + }, }; }This would require a corresponding
jsonFromWebPromisehelper inweb_fetch_utils.dart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/request.js.dart` around lines 250 - 258, The json() override always delegates to Body.json<T>() which skips the WebRequestHost optimization used by text()/bytes()/blob(); add a branch to the switch in json<T>() to detect WebRequestHost and call a new web-native helper (e.g., web_fetch_utils.dart::jsonFromWebPromise(host.value.json())) before falling back to body.json<T>() or the FormatException case, and implement the corresponding jsonFromWebPromise helper to parse the web Promise into a Dart Future; ensure imports are updated and keep the existing null => FormatException path untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/_internal/web_fetch_utils.dart`:
- Around line 22-27: The current blobFromWebPromise function hardcodes an empty
string for the new Blob's MIME type, dropping the host blob's type; change it to
await promise.toDart into a local variable (e.g., hostBlob), then construct and
return Blob(<Object>[hostBlob], type ?? hostBlob.type) so that when the caller
doesn't pass type the original host web.Blob MIME type is preserved; ensure you
reference JSPromise, web.Blob and the blobFromWebPromise function when making
the change.
In `@lib/src/fetch/response.io.dart`:
- Around line 190-201: The Response.clone() implementation rebuilds a new
native.Response from only status/statusText/headers which can drop important
fields like type, url, redirected and mishandle error responses (status 0);
update clone() in Response to use the underlying native.Response.clone() when
available (i.e. call this.body?.clone() on the native response or invoke
native.Response.clone()) or otherwise construct the native.Response while
copying all metadata fields (type, url, redirected, and any error semantics)
from the original native response into the native.ResponseInit so error
responses and native clone semantics are preserved (refer to the Response class,
clone(), native.Response and native.ResponseInit).
---
Duplicate comments:
In `@lib/src/fetch/body.dart`:
- Around line 47-50: The Body copy currently shallow-copies internal hosts
(Body._ with blockHost: body._blockHost, streamHost: body._streamHost), which
aliases _streamHost and allows one instance to consume the stream for both; fix
by ensuring we do not alias internal hosts when constructing from another Body:
either clone/rewind the stream host (call a clone/rewind method on
body._streamHost, e.g., _streamHost.clone() or create a new StreamHost wrapper)
or transfer ownership by moving the stream host and marking the source consumed
(e.g., body.markConsumed() or set body._streamHost = null) so the new Body gets
an independent streamHost; update Body._ construction to use the cloned/new host
instead of reusing body._streamHost and handle blockHost similarly if it has
mutable/consumable state.
---
Nitpick comments:
In `@lib/src/fetch/request.js.dart`:
- Around line 61-75: The Body getter currently caches the converted web stream
into _body for WebRequestHost but returns host.value.body directly for
NativeRequestHost, leaving _body unset; update the NativeRequestHost branch in
the body getter (the switch handling NativeRequestHost / host.value.body) to
assign host.value.body to _body when non-null and then return it (i.e., set
_body = host.value.body before returning) so _body is populated consistently for
both WebRequestHost and NativeRequestHost.
- Around line 250-258: The json() override always delegates to Body.json<T>()
which skips the WebRequestHost optimization used by text()/bytes()/blob(); add a
branch to the switch in json<T>() to detect WebRequestHost and call a new
web-native helper (e.g.,
web_fetch_utils.dart::jsonFromWebPromise(host.value.json())) before falling back
to body.json<T>() or the FormatException case, and implement the corresponding
jsonFromWebPromise helper to parse the web Promise into a Dart Future; ensure
imports are updated and keep the existing null => FormatException path
untouched.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9ac561f1-4356-4e58-b022-8bff0e12eeaa
📒 Files selected for processing (20)
README.mdexample/main.dartlib/ht.dartlib/src/_internal/web_fetch_utils.dartlib/src/_internal/web_stream_bridge.dartlib/src/fetch/body.dartlib/src/fetch/headers.js.dartlib/src/fetch/request.dartlib/src/fetch/request.js.dartlib/src/fetch/response.dartlib/src/fetch/response.io.dartlib/src/fetch/response.js.darttest/blob_io_test.darttest/body_test.darttest/form_data_native_test.darttest/request_js_test.darttest/request_native_test.darttest/response_io_test.darttest/response_js_test.darttest/response_native_test.dart
🚧 Files skipped from review as they are similar to previous changes (6)
- test/request_native_test.dart
- lib/src/fetch/headers.js.dart
- test/body_test.dart
- example/main.dart
- test/blob_io_test.dart
- test/form_data_native_test.dart
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
lib/src/fetch/response.io.dart (1)
193-203:⚠️ Potential issue | 🟠 Major
clone()still drops IO-host metadata (redirected/type/url).On Line 193, rebuilding from
native.ResponseInitpreserves status/statusText/headers/body, but it resets other observable metadata. A redirectedHttpClientResponsecan clone into a response whereredirectedbecomesfalse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/response.io.dart` around lines 193 - 203, The clone path for HttpClientResponseHost (the HttpClientResponseHost => Response(native.Response(...)) block) only preserves status/statusText/headers/body and thus loses IO-host metadata like redirected, type, and url; update the reconstruction to propagate those fields into the rebuilt Response (e.g., include redirected, type and url from the original HttpClientResponseHost into native.ResponseInit or otherwise set them on the returned Response) so a cloned HttpClientResponse preserves redirected/type/url while keeping body?.clone().
🧹 Nitpick comments (3)
test/headers_js_test.dart (1)
23-31: Add regression assertions forset-cookiename normalization.Please include mixed-case and whitespace-padded variants (for example,
Set-Cookieand' set-cookie ') so parity bugs inget()are caught early.✅ Suggested test additions
test('does not expose set-cookie through get()', () { final headers = Headers( web.Headers() ..append('set-cookie', 'a=1') ..append('set-cookie', 'b=2'), ); expect(headers.get('set-cookie'), isNull); + expect(headers.get('Set-Cookie'), isNull); + expect(headers.get(' set-cookie '), isNull); expect(headers.getSetCookie(), ['a=1', 'b=2']); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/headers_js_test.dart` around lines 23 - 31, Add regression assertions to verify `Headers.get()` normalizes header names so it does not expose `set-cookie` regardless of case or surrounding whitespace: update the test for the Headers class (the test block using Headers and calls to headers.get and headers.getSetCookie) to also call headers.get with mixed-case and whitespace-padded variants like "Set-Cookie" and " set-cookie " and assert they return null, while headers.getSetCookie() still returns the full list ['a=1','b=2']; this ensures name normalization in Headers.get() is covered.test/request_io_test.dart (1)
10-10: Rename this test to avoid implying wrapper-level caching.Line 10 currently suggests the IO wrapper caches native-backed bodies, but this check is really about stable identity. A clearer name would reduce confusion in future maintenance.
Based on learnings: In
lib/src/fetch/request.io.dart,NativeRequestHostintentionally does not assign_body; identity stability comes fromnative.Request.body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/request_io_test.dart` at line 10, Rename the test titled "caches body for wrapped native requests" to avoid implying wrapper-level caching; update its description to reflect that the test verifies stable identity (e.g., "preserves body identity for wrapped native requests" or "maintains native request body identity") so it's clear the behavior comes from native.Request.body and not an assigned _body in NativeRequestHost; locate the test in request_io_test.dart and change only the test name string, leaving the assertions intact.lib/src/fetch/body.dart (1)
147-149:streamTeeclone path can buffer unbounded data when branches are consumed imbalance.Lines 147–149 use
streamTee(streamHost)which splits the stream into two branches viaStreamSplitter.splitFrom(). Each branch is backed by aStreamController; if one branch's listener never attaches or lags significantly, events accumulate in that controller until consumed or the stream is dropped. For large streaming bodies, this creates memory pressure.Document the consumption contract clearly (both branches should be consumed promptly) and/or consider a bounded buffering strategy for large streams.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/body.dart` around lines 147 - 149, The current use of streamTee(streamHost) (which calls StreamSplitter.splitFrom()) can cause unbounded buffering if one branch (_streamHost or the returned Body._ branch) is not consumed promptly; update the implementation and docs: add a clear consumption contract on Body/streamTee stating both branches must be consumed promptly, and modify streamTee/StreamSplitter.splitFrom to use a bounded buffer (or expose a maxBuffer parameter) and a fallback behavior (drop/throw/cancel) when the buffer limit is exceeded or when the alternate branch is not listened within a short timeout; also ensure callers of Body._ and _streamHost are updated or annotated so consumers know they must attach listeners immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/_internal/web_fetch_utils.dart`:
- Around line 53-59: The conversion from web.File to the native File in
web_fetch_utils.dart is dropping the file.lastModified metadata; update the
block that matches "entry case final web.File file" so the File constructor
receives the lastModified value (e.g., File(<Object>[file], file.name, type:
file.type, lastModified: file.lastModified)) before wrapping it in
Multipart.blob, ensuring the web.File.lastModified is forwarded and preserved.
In `@lib/src/fetch/headers.js.dart`:
- Around line 67-69: In the get method, normalize the incoming header name (trim
whitespace and lowercase) before checking against 'set-cookie' so inputs like '
set-cookie ' are handled the same as native behavior; update the guard in
String? get(String name) to use the trimmed-and-lowercased name for the
comparison and then call host.get(name) with the original or normalized name as
appropriate (reference: get method and host.get).
---
Duplicate comments:
In `@lib/src/fetch/response.io.dart`:
- Around line 193-203: The clone path for HttpClientResponseHost (the
HttpClientResponseHost => Response(native.Response(...)) block) only preserves
status/statusText/headers/body and thus loses IO-host metadata like redirected,
type, and url; update the reconstruction to propagate those fields into the
rebuilt Response (e.g., include redirected, type and url from the original
HttpClientResponseHost into native.ResponseInit or otherwise set them on the
returned Response) so a cloned HttpClientResponse preserves redirected/type/url
while keeping body?.clone().
---
Nitpick comments:
In `@lib/src/fetch/body.dart`:
- Around line 147-149: The current use of streamTee(streamHost) (which calls
StreamSplitter.splitFrom()) can cause unbounded buffering if one branch
(_streamHost or the returned Body._ branch) is not consumed promptly; update the
implementation and docs: add a clear consumption contract on Body/streamTee
stating both branches must be consumed promptly, and modify
streamTee/StreamSplitter.splitFrom to use a bounded buffer (or expose a
maxBuffer parameter) and a fallback behavior (drop/throw/cancel) when the buffer
limit is exceeded or when the alternate branch is not listened within a short
timeout; also ensure callers of Body._ and _streamHost are updated or annotated
so consumers know they must attach listeners immediately.
In `@test/headers_js_test.dart`:
- Around line 23-31: Add regression assertions to verify `Headers.get()`
normalizes header names so it does not expose `set-cookie` regardless of case or
surrounding whitespace: update the test for the Headers class (the test block
using Headers and calls to headers.get and headers.getSetCookie) to also call
headers.get with mixed-case and whitespace-padded variants like "Set-Cookie" and
" set-cookie " and assert they return null, while headers.getSetCookie() still
returns the full list ['a=1','b=2']; this ensures name normalization in
Headers.get() is covered.
In `@test/request_io_test.dart`:
- Line 10: Rename the test titled "caches body for wrapped native requests" to
avoid implying wrapper-level caching; update its description to reflect that the
test verifies stable identity (e.g., "preserves body identity for wrapped native
requests" or "maintains native request body identity") so it's clear the
behavior comes from native.Request.body and not an assigned _body in
NativeRequestHost; locate the test in request_io_test.dart and change only the
test name string, leaving the assertions intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c866b822-b5b6-44fe-a92a-1a147fd8b84c
📒 Files selected for processing (11)
lib/src/_internal/web_fetch_utils.dartlib/src/fetch/body.dartlib/src/fetch/headers.js.dartlib/src/fetch/response.io.dartlib/src/fetch/response.native.darttest/blob_js_test.darttest/body_test.darttest/headers_js_test.darttest/request_io_test.darttest/response_io_test.darttest/response_native_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- test/blob_js_test.dart
- test/response_io_test.dart
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/src/fetch/headers.js.dart (1)
66-70: Minor normalization inconsistency with native implementation.The
set-cookieguard correctly normalizes withtrim().toLowerCase()(line 68), but the actual lookup on line 69 passes the rawnametohost.get().The native implementation normalizes the name before all operations (including lookup). While browser's
Headers.get()is case-insensitive, it may not trim whitespace. This could cause subtle behavioral differences for edge cases likeget(" Content-Type ").Consider normalizing the name before delegation for cross-platform consistency:
💡 Suggested alignment with native behavior
`@override` String? get(String name) { - if (name.trim().toLowerCase() == 'set-cookie') return null; - return host.get(name); + final normalized = name.trim(); + if (normalized.toLowerCase() == 'set-cookie') return null; + return host.get(normalized); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/fetch/headers.js.dart` around lines 66 - 70, The get method currently normalizes the header name only for the set-cookie check but passes the raw name to host.get, causing inconsistency; update the get(String name) implementation (the get method that calls host.get) to normalize the header name once (e.g., final normalized = name.trim().toLowerCase()) and use that normalized value for both the set-cookie guard and the host.get call so lookups like get(" Content-Type ") behave consistently with native behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/fetch/form_data.native.dart`:
- Around line 135-138: The current set(String name, Multipart value) blindly
calls delete(name) then append(name, value), which reorders existing fields;
instead implement in-place replacement: scan the internal entries for the first
occurrence of name, replace that entry's value with value, then remove any
subsequent entries with the same name; if no existing entry is found, call
append(name, value). Do not call delete(name) at the start. Update the set
method (and any helper list of entries used by delete/append) to perform this
logic so insertion order of other fields remains unchanged.
- Around line 338-344: _unescapeHeaderValue currently converts the literal
sequences `\r` and `\n` into actual CR/LF control characters which can corrupt
header values like filenames; update the _unescapeHeaderValue function so it no
longer replaces r'\r' or r'\n' with control characters and only performs safe
unescaping (e.g. unescape backslash and escaped quote sequences like r'\\' and
r'\"'), removing the replaceAll calls for r'\r' and r'\n' (or equivalently
retain them as literal backslash-char sequences) and add/adjust tests for header
filename and field value handling to ensure control characters are not
introduced.
- Around line 316-333: _parseHeaderParameters currently splits the header string
by ';' which breaks quoted parameter values containing semicolons (e.g.,
filename="a;b.txt"); fix by replacing the naive split with a small state-machine
that iterates over the input string character-by-character, tracking whether
you're inside double quotes and honoring escapes, collecting segments and only
treating a ';' as a separator when not inside quotes, then parsing each segment
as before and passing values to _unescapeHeaderValue; update
_parseHeaderParameters to use this quoted-aware splitting logic and ensure
quoted-value trimming/unquoting remains intact.
---
Nitpick comments:
In `@lib/src/fetch/headers.js.dart`:
- Around line 66-70: The get method currently normalizes the header name only
for the set-cookie check but passes the raw name to host.get, causing
inconsistency; update the get(String name) implementation (the get method that
calls host.get) to normalize the header name once (e.g., final normalized =
name.trim().toLowerCase()) and use that normalized value for both the set-cookie
guard and the host.get call so lookups like get(" Content-Type ") behave
consistently with native behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad924035-5dfb-4365-afc1-1d9f65a665e8
📒 Files selected for processing (7)
lib/src/_internal/web_fetch_utils.dartlib/src/fetch/form_data.native.dartlib/src/fetch/headers.js.darttest/_internal/web_fetch_utils_test.darttest/blob_io_test.darttest/blob_js_test.darttest/headers_js_test.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- test/headers_js_test.dart
- test/blob_io_test.dart
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/form_data_native_test.dart (1)
159-165: AssertcontentLengthagainst the actual payload bytes.This currently proves that
applyTo()forwardsencoded.contentLength, but it would not catch_calculateMultipartLength()being wrong because both assertions read the same source. Sincebytesis already materialized here, comparebytes.lengthdirectly as well.✅ Suggested assertion
final bytes = await encoded.bytes(); + expect(bytes.length, encoded.contentLength); final fromStream = BytesBuilder(copy: false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/form_data_native_test.dart` around lines 159 - 165, The test currently compares the streamed bytes to bytes() but doesn't assert encoded.contentLength against the actual materialized payload; add an assertion that encoded.contentLength (the value produced by _calculateMultipartLength()/applyTo()) equals bytes.length to catch miscalculation: after finalizing bytes = await encoded.bytes(), assert expect(encoded.contentLength, bytes.length) (or the test framework equivalent) before or after the existing expect(fromStream.takeBytes(), bytes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/fetch/form_data.native.dart`:
- Around line 388-389: The current _unescapeHeaderValue implementation uses
chained replaceAll calls that are order-dependent and drop a literal backslash
before a quote (e.g., "\\\""). Replace the two replaceAlls with a single-pass
parser inside _unescapeHeaderValue: iterate over the string, and when you see a
backslash and there is a next character, consume the backslash and append the
next character (so "\\\"" becomes backslash + quote), otherwise append the
current char; return the built result. Also add a regression test next to the
quoted-parameter tests that decodes a value containing a literal backslash
immediately before a quote to assert the backslash is preserved.
---
Nitpick comments:
In `@test/form_data_native_test.dart`:
- Around line 159-165: The test currently compares the streamed bytes to bytes()
but doesn't assert encoded.contentLength against the actual materialized
payload; add an assertion that encoded.contentLength (the value produced by
_calculateMultipartLength()/applyTo()) equals bytes.length to catch
miscalculation: after finalizing bytes = await encoded.bytes(), assert
expect(encoded.contentLength, bytes.length) (or the test framework equivalent)
before or after the existing expect(fromStream.takeBytes(), bytes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ada5e27d-2b4c-4ee2-878c-88336bcae39b
📒 Files selected for processing (2)
lib/src/fetch/form_data.native.darttest/form_data_native_test.dart
Comment on lines
+388
to
+389
| static String _unescapeHeaderValue(String value) { | ||
| return value.replaceAll(r'\\', '\\').replaceAll(r'\"', '"'); |
There was a problem hiding this comment.
Unescape quoted parameters in one pass.
The chained replaceAll calls are order-dependent. When the encoder emits \\\" for a literal backslash followed by a quote, this decoder collapses it to " and drops the backslash, so valid filenames and field names no longer round-trip.
🔧 Proposed fix
static String _unescapeHeaderValue(String value) {
- return value.replaceAll(r'\\', '\\').replaceAll(r'\"', '"');
+ return value.replaceAllMapped(
+ RegExp(r'\\(["\\])'),
+ (match) => match.group(1)!,
+ );
}Please add a regression case next to the quoted-parameter tests for a value containing a literal backslash before a quote.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/src/fetch/form_data.native.dart` around lines 388 - 389, The current
_unescapeHeaderValue implementation uses chained replaceAll calls that are
order-dependent and drop a literal backslash before a quote (e.g., "\\\"").
Replace the two replaceAlls with a single-pass parser inside
_unescapeHeaderValue: iterate over the string, and when you see a backslash and
there is a next character, consume the backslash and append the next character
(so "\\\"" becomes backslash + quote), otherwise append the current char; return
the built result. Also add a regression test next to the quoted-parameter tests
that decodes a value containing a literal backslash immediately before a quote
to assert the backslash is preserved.
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
1 participant
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.
Resolves #7
Resolves #8
Resolves #10
Summary by CodeRabbit
New Features
Bug Fixes & Improvements
Tests