Repository navigation
✨ feat: warn about CORS preflight requests on Web - #2556
Conversation
Detect when a request is not a CORS "simple request" and will trigger a preflight (OPTIONS) request, then emit a warning log and enrich the XMLHttpRequest.onError message with CORS guidance. Closes #2201 Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
AlexV525
left a comment
There was a problem hiding this comment.
Thanks for driving this — the extraction of corsPreflightReason into a pure, testable helper is a nice touch, the CHANGELOG entry is clean, and feat/web-cors-preflight-warning already matches the branch-naming convention that just landed in AGENTS.md. Left three inline comments on the substantive points; a few AGENTS.md formalities below.
AGENTS.md formalities (fixup, or a follow-up)
- Commit / PR title should be gitmoji + Conventional per AGENTS.md §8.2 —
✨ feat: warn about CORS preflight requests on Web. - AI attribution disclosure per §8.3: Devin's
Co-Authored-Bytrailer is present ✓, but if GLM-5.2 also produced code / tests / docs (as noted on #2549), it should have its own trailer. Please also add one line to the PR description stating which agent did which stage — design / implementation / tests / review. - Test-plan item 4 — “existing
corsTestsstill cover runtime behavior” isn't quite right: those tests predate this PR and can't cover the newly-added warning + enriched-error paths. Either run browser CI and check the box, or replace the note with an honest “browser paths not exercised locally”. (Related: AGENTS.md §8.6 now prefers a prose description of what was actually verified over a boilerplate test-plan checklist — no need to restructure this PR, just heads-up for the next one.)
- Remove the public export of corsPreflightReason to avoid adding permanent public API surface for testing only. - Extract collectCorsPreflightReasons and corsEnrichedErrorReason into pure functions in cors.dart so the upload-listener, withCredentials, and enriched-error branches are unit-testable without a browser. - Add enableCORSWarning opt-out flag (defaults to true) so apps that handle CORS correctly can silence the per-request warning. - Add 8 unit tests covering the new pure functions. Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-Authored-By: GLM-5.2 <[email protected]>
|
Thanks for the detailed review — all three inline comments and the AGENTS.md formalities have been addressed in e69c194:
AGENTS.md formalities:
|
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is |
Introduces `AGENTS.md` (+ Simplified Chinese mirror) as this repo's rules for AI-agent-assisted contributions, along with pointers from `CONTRIBUTING` and a PR-template checklist item. `AGENTS.md` is the de-facto standard filename that agent tools (Claude Code, Codex, Cursor, Devin, etc.) pick up automatically, so agents on external forks get these rules without extra setup. ### What the guidelines cover 1. **Motivation first** — every non-trivial change must answer four questions (what can't be done today, who needs it, why in core rather than an interceptor/adapter/transformer/plugin, cost). `Closes #NNNN` alone does not count as prior discussion — the referenced issue must show maintainer buy-in. One PR = one concern. 2. **Mandatory, effective, non-duplicated tests** — tests must fail without the change; coverage minimum is a floor, not a target; no test-theater for coverage numbers. 3. **Compatibility is sacred** — additive-by-default; deprecations with `Use XXX instead. This will be removed in X.0.0` targeting the next major (X is a placeholder — pick the actual next major from the affected package's `pubspec.yaml`); no SDK-lower-bound bumps outside the Compatibility Policy; watch behavioral breaks. Explicit **sensitive-area** list (SSL/TLS + cert pinning, redirects, cookies, headers, timeout/cancellation/connection pooling, interceptor pipeline, request-body encoding) and **dependency-change** rules (no drive-by bumps; state motivation; narrowest constraint; disclose new transitives). 4. **Understand before you change** — no API guessing; cross-platform pub-cache paths + `PUB_CACHE` env var. 5. **Production quality only** — no `TODO`/mock/simplified-and-called-done code. 6. **When to stop and ask** — general rule for ambiguity and out-of-scope work, distinct from §3's breaking-change escalation. 7. **Repository layout** — Melos mono-repo, per-package SDK lower bounds. 8. **Commits, changelog, and PR hygiene** - §8.1 **Branch naming** — `category/ticket-id-or-short-description` - §8.2 **Commit format** — gitmoji **or** Conventional Commits (either is acceptable) with an emoji reference table matched to the repo's actual usage - §8.3 **AI attribution mandatory** — every contributing agent gets a `Co-Authored-By:` trailer; the PR body discloses agent + stage (design / implementation / tests / review) - §8.4 CHANGELOG rules - §8.5 **Self-review your diff and commit message** — remove debug prints, commented code, unrelated formatting; `git add .` / `-A` forbidden; re-read the commit message against the staged diff so a message from a prior attempt doesn't land verbatim - §8.6 **Opening the PR** — draft PRs for large/exploratory changes; **describe verification honestly** in prose rather than a boilerplate "Test plan" checklist; anything genuinely un-verifiable goes under an `Unverified` paragraph - §8.7 **Review iteration** — append commits (no squash-force-push mid-review); no close/reopen to reset state; design feedback is conversation, not instruction; CI failures require log-reading, not retry-until-green 9. **Patterns that may lead to closure** — cross-reference table linking each anti-pattern back to the rule it violates, softened wording (`may` / `might`). ### Iteration notes Follow-ups after the initial commit addressed: - **@AmosHuKe** (first round) — cross-platform pub-cache paths and `PUB_CACHE`, `「包」 → package` in the Chinese version, mainland-accessible Melos URL, softened minor-version breaking-change wording per dio's own CHANGELOG preamble. - **@AmosHuKe** (second round) — the concrete `6.0.0` in the deprecation example could mislead agents that don't check `pubspec.yaml`; replaced with an `X.0.0` placeholder plus a note to derive the real next major from the affected package. - **@CaiJingLong via GLM-5.2** (first round) — expanded the workflow gaps GLM flagged (commit format, self-review, review iteration, when-to-ask, sensitive areas, dependency rules, branch naming, draft PRs); collapsed the "patterns that may lead to closure" section from a full restatement of §§1–8 into a cross-reference table. - **@AlexV525** direct edit — softened §8.2 from "gitmoji **and** Conventional Commits" to "gitmoji **or** Conventional Commits", matching the actual mix in `git log` (recent commits use one or the other, not both). - **@CaiJingLong via GLM-5.2** (re-review) — flagged a duplicated §8.7 block (removed), a garbled §8.2 sentence in the English version (rewritten to match the ZH), the `🥅` gitmoji being frequent in the repo but missing from the reference table (added), and a missing §5 row in the closure cross-reference table (added). - **Self-application** — the "re-read the commit message against the staged diff" clause in §8.5 was added after an actual near-miss on this branch, where a stale message (`Fix deprecation hints`) was almost applied to a different fix. Documenting the source in the commit and the doc keeps the rule from looking arbitrary. - **Stress-tested against #2556** (opened by @CaiJingLong via GLM-5.2 + Devin right after this PR) — that PR validated the branch-naming convention and motivated the "describe verification honestly" rule in §8.6, after its Test-plan section carried a well-intentioned but misleading "existing tests still cover" note on an untested new code path. ### Files - `AGENTS.md` — English rules - `AGENTS-ZH.md` — Simplified Chinese mirror - `CONTRIBUTING.md` / `CONTRIBUTING-ZH.md` — link to the new guidelines - `.github/PULL_REQUEST_TEMPLATE.md` — checklist item pointing to `AGENTS.md` ### Verification Documentation change; no code paths to exercise. All internal `§`-references were grep-verified for consistency after each renumbering pass. The gitmoji reference table in §8.2 was cross-checked against the last 300 commits (`git log --pretty=format:'%s' -300 | grep -oE '^[^A-Za-z0-9 (]+' | sort | uniq -c`) to ensure the emojis it endorses match the repo's real usage frequency. --------- Co-authored-by: Claude <[email protected]>
|
|
@Waxo Could you elaborate? The flag still seems to be alive on the latest code: |
|
My bad got the wrong info my colleague said it was removed. Library wasn't up to date. |
Summary
warningLogexplaining why (non-safelisted method, custom headers, non-simple Content-Type, upload progress listener, orwithCredentials).XMLHttpRequest.onErrorerror message with CORS guidance when the request was preflighted, so users see an actionable hint instead of a generic network error.enableCORSWarningopt-out flag (defaults totrue) so apps that handle CORS correctly can silence the per-request warning. The enriched error message is always emitted regardless of this setting.lib/src/cors.dart(corsPreflightReason,collectCorsPreflightReasons,corsEnrichedErrorReason) so they can be unit-tested on the VM without a browser.Closes #2201.
AI attribution
Implementation, tests, and PR workflow by Devin (GLM-5.2); local review pass by the human operator.
Verification
Added 23 unit tests covering
corsPreflightReason(method / content-type / custom-header combinations),collectCorsPreflightReasons(upload-listener, withCredentials, combined cases), andcorsEnrichedErrorReason(empty / non-empty / multi-reason).dart analyze --fatal-infosanddart test -p vmpass locally ondio_web_adapter.Unverified: The browser-side runtime paths (actual
warningLogemission,XMLHttpRequest.onErrorenrichment in a real browser) are not exercised locally — they require browser CI. The existingcorsTestsindio_testcover CORS preflight classification behavior but do not assert on the new warning or enriched error message.