fix(fetch): urgently correct broken request implementation#13
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChanged Request construction to accept plain Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
medz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/src/fetch/request.js.dart (1)
285-292: Defensive case at line 286 is unreachable but harmless.The
native.Request request when init == nullcase is already handled by the factory at line 39, so this branch in_toNativeRequestwill never execute. The switch remains exhaustive and the code is correct, but this is effectively dead code.Consider removing it for clarity, or keep it as defensive coding if you prefer robustness against future refactoring.
♻️ Optional: Remove unreachable case
static native.Request _toNativeRequest( Object? input, native.RequestInit? init, ) { return switch (input) { - final native.Request request when init == null => request, final native.Request request => native.Request(request, init), final String _ => native.Request(input, init), final Uri _ => native.Request(input, init), final web.Request request => _nativeRequestFromWebRequest(request, init), _ => throw ArgumentError.value(input, 'input'), }; }🤖 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 285 - 292, Remove the unreachable defensive pattern in the _toNativeRequest switch: delete the branch matching "final native.Request request when init == null" since the factory (line ~39) already guarantees that case; keep the other cases as-is so the switch remains exhaustive and behavior unchanged.
🤖 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/request.io.dart`:
- Around line 28-33: The factory Request currently aliases existing request
wrappers (in the switch arms handling Request, io.HttpRequest, and
native.Request) which leads to duplicate Body instances; change those fast-path
arms in the Request factory to always rebuild a fresh native request via
native.Request(request, init) (or via the existing _toNativeRequest helper)
instead of returning the original wrapper/host (affecting Request(Object? input,
[native.RequestInit? init]), HttpRequestHost usage and NativeRequestHost usage),
and add a regression test that constructs Request(upstream) with null init to
assert only a single Body/stream is used (i.e., bodyUsed and reads remain
consistent).
In `@lib/src/fetch/request.native.dart`:
- Around line 370-381: Change the nullable parameter of _coerceInput from
Object? to non-null Object so the compile-time signature matches the Fetch API's
RequestInfo (which excludes null); update the declaration of
_coerceInput(Object? input) to _coerceInput(Object input) and ensure call sites
(e.g., any calls constructing Request that forward into _coerceInput) do not
pass null (or perform explicit null checks before calling) so the existing
ArgumentError path remains unnecessary for null values but the runtime behavior
for unsupported types is preserved.
---
Nitpick comments:
In `@lib/src/fetch/request.js.dart`:
- Around line 285-292: Remove the unreachable defensive pattern in the
_toNativeRequest switch: delete the branch matching "final native.Request
request when init == null" since the factory (line ~39) already guarantees that
case; keep the other cases as-is so the switch remains exhaustive and behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3f58bcd-aee4-479c-bf42-f2fc357f830f
📒 Files selected for processing (12)
example/main.dartlib/src/fetch/request.dartlib/src/fetch/request.io.dartlib/src/fetch/request.js.dartlib/src/fetch/request.native.darttest/headers_test.darttest/public_api_surface_test.darttest/request_io_test.darttest/request_js_test.darttest/request_native_test.darttest/response_io_test.darttest/url_search_params_test.dart
💤 Files with no reviewable changes (1)
- lib/src/fetch/request.dart
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/request.io.dart`:
- Line 254: The current construction final Request request =>
native.Request(request.clone(), init) always clones the original request before
native.Request sees init, causing unnecessary cloning when init.body is
provided; change this so we only call request.clone() when init.body is null
(i.e., pass the original request when init.body != null and pass request.clone()
when init.body == null) so native.Request short-circuits correctly; update the
code path around the Request getter/constructor that references request.clone(),
native.Request, and init.body accordingly, and add a regression test that
rebuilds a consumed Request with RequestInit(body: ...) to verify the
replacement body is independent of the upstream request state.
In `@lib/src/fetch/request.js.dart`:
- Line 282: The current getter builds the wrapped Request by always calling
request.clone() which causes a body-override bug when init.body is provided;
change the logic in the Request getter so it only calls request.clone() when
init?.body == null and otherwise pass the original upstream request (or a
non-cloned reference) into native.Request along with the RequestInit(body: ...);
update the code around the Request getter that returns
native.Request(request.clone(), init) to conditionally use request.clone() only
when init?.body == null, ensuring RequestInit(body: ...) is applied without
requiring the upstream body to be cloneable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 12e84c34-c592-4fab-815e-d8a841a5e098
📒 Files selected for processing (5)
lib/src/fetch/request.io.dartlib/src/fetch/request.js.dartlib/src/fetch/request.native.darttest/request_io_test.darttest/request_js_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- test/request_js_test.dart
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.
Summary by CodeRabbit
Refactor
Tests