Throw FetchError with HTTP status from fetchJson

[piyalbasu]

Summary

Fixes a Sentry grouping bug where every fetchJson failure collapses into one issue (FREIGHTER-DMQ), and addresses adjacent error-reporting hygiene gaps surfaced during review. Closes #2747.

What was broken (the original bug)

fetchJson did throw new Error(res.statusText) on non-2xx responses. Over HTTP/2 (Cloudflare + most modern backends), statusText is empty in Chrome, producing a message-less Error. With no message, Sentry's stack-trace fingerprint is the only grouping signal — distinct API failures (429s, 400s, 431s, etc.) all collapsed into a single "No error message" issue.

What this PR changes

This PR is broader than the minimum bug fix. Beyond adding the status code to the thrown error, it:

1. Throws a structured FetchError carrying status, statusText, url (sanitized — query string dropped), method, body, and a kind discriminator (http-error | non-json). Produces a stable, status-coded error.message for Sentry grouping.
2. Fixes the non-JSON content-type branch, which previously interpolated arbitrary HTML/text bodies into error.message — the inverse grouping bug, where every Cloudflare interstitial / gateway page would have been its own fingerprint.
3. Adds captureFetchError — a Sentry helper that unpacks FetchError fields into setTag / setContext so http.status:429, http.method:POST, etc. become filterable in Sentry. (Sentry's default captureException does not enumerate own-properties of Error subclasses, so without this the new fields would be inert.)
4. Caps body reads — Content-Length-aware skip plus a 4 KB read cap and explicit …[truncated] marker. Prevents a multi-MB error payload from being slurped just to produce a 200-char snippet, and makes truncation visible to future readers.
5. Migrates scanSite, scanAsset, and scanAssetBulk to go through fetchJson — they were using raw fetch with the same shape of bad error capture (raw strings passed to Sentry.captureException).
6. Replaces string-based Sentry.captureException("...") calls in blockaid.ts with new Error(...) wraps so Sentry always receives a real Error (improves stack capture and grouping for body-signaled errors).
7. Preserves body-read failures via cause — super(msg, { cause }) so the chained underlying error appears in Sentry's exception detail panel.

Why the expanded scope

The original ticket (#2747) only required item 1 (status in the message). Reviewer (@aristidesstaffieri) flagged items 2, 3, 5, 6 as legitimate adjacent gaps in the same files — fixing them in one PR avoids a string of follow-up PRs through the same surface area. Items 4 and 7 are bonus hardening (memory safety, debuggability) that came up during implementation.

What this PR does not fix (deliberately)

The actual production bugs that have been hiding inside FREIGHTER-DMQ are still present. Once this lands and Sentry starts splitting events by status code, separate issues will need to be triaged:

• HTTP 429 on /scan-tx — rate-limit thrash; client-side retry/backoff likely also needed
• HTTP 400 on /scan-tx — likely a payload-shape regression in 5.40.0
• HTTP 431 on an earlier non-scan-tx fetch — header bloat (Cookie / Authorization / baggage)

These remain on the issue's follow-up list (#2747).

Test plan

• 8 jest unit tests in extension/src/popup/helpers/__tests__/fetch.test.ts:
  • 2xx success
  • 429 with empty statusText (the FREIGHTER-DMQ regression case)
  • 400 with status / sanitized URL / method / body in message
  • 431 with empty statusText (still produces a meaningful message)
  • non-JSON 2xx → kind: "non-json", body NOT in message (stable grouping)
  • body truncation marker
  • Content-Length-based read skip
  • res.text() rejection chained as cause
• Existing blockaid.test.tsx (27 tests) continues to pass.
• tsc --noEmit -p extension/tsconfig.json clean.
• After merge, watch FREIGHTER-DMQ in Sentry to confirm new events stop landing there and split into status-specific issues.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR improves observability for failed fetchJson calls by replacing message-less Error(res.statusText) throws (common under HTTP/2) with a dedicated FetchError that includes HTTP status context, improving Sentry grouping and triage for distinct failure modes.

Changes:

• Introduced a FetchError subclass that carries status, statusText, and optional body, and formats a more informative error message.
• Updated fetchJson to throw FetchError on non-2xx responses and attempt to read the response body for added context.
• Added Jest coverage for success and several non-2xx regression cases (including empty statusText).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
extension/src/popup/helpers/fetch.ts	Adds FetchError and updates fetchJson to throw it with HTTP status context on non-OK responses.
extension/src/popup/helpers/tests/fetch.test.ts	Adds unit tests covering 2xx success and several error-status scenarios (incl. empty statusText regression).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[aristidesstaffieri]

Nice fix on the !res.ok path. A few adjacent error-reporting gaps worth considering before / after merge:

1. The non-JSON content-type branch is still bare Error (same FREIGHTER-DMQ shape)

fetch.ts:9 still does:

throw new Error(`Did not receive json error:${content}`);

The body is interpolated into error.message, so each unique HTML/text payload (Cloudflare interstitials, gateway pages, etc.) becomes its own Sentry fingerprint — the inverse grouping bug. Worth giving this branch the FetchError treatment too (status 200-but-wrong-content-type, body in a property, stable message). The new test file has no case for it.

2. status / body properties are dead weight in Sentry as currently wired

Callers use plain Sentry.captureException(err) (e.g. blockaid.ts:132, :211, :656, :692). Sentry's default capture only reads message + stack; it does not enumerate own-properties on Error subclasses into tags/contexts. So you'll see "429" in the title but you can't filter http.status:429, alert on 5xx-only, or pivot by body.

To actually unlock the new fields, route through a small helper:

export function captureFetchError(err: unknown) {
  if (err instanceof FetchError) {
    Sentry.withScope((scope) => {
      scope.setTag("http.status", String(err.status));
      scope.setContext("http", { statusText: err.statusText, body: err.body });
      Sentry.captureException(err);
    });
    return;
  }
  Sentry.captureException(err);
}

…and swap the four fetchJson callers to it. Bonus: routing body through setContext runs Sentry's data scrubbers on it (see #4).

3. No URL / method on the error

"Request failed with status 429" doesn't say which endpoint. The stack helps in Sentry but log search and titles benefit from the URL. Suggest new FetchError(url, method, status, statusText, body?) with a sanitized URL (drop query string — report-asset-warning?address=…&details=… and the others contain user addresses / signed XDR).

4. body.slice(0, 200) is silent truncation + a PII vector

• No … / [truncated] marker, so future readers can't tell content was cut.
• 200 chars of arbitrary response body flowing into Sentry error.value (which Sentry indexes/displays) can include user addresses or signed XDR depending on endpoint. Prefer parsing JSON and surfacing a structured code/message, or keep message status-only and put body in scope.setContext so Sentry's scrubbers apply.

5. scanAsset and scanSite don't go through fetchJson — they're left out of the fix

• blockaid.ts:188 (scanAsset) uses raw fetch and on failure calls Sentry.captureException(response.error || "Failed to scan asset") — passing a string, which Sentry groups by exact text.
• Same shape at :542 / :582 (Failed to call scan site: ${error}) and :608.

If the goal is "fix Sentry grouping for indexer calls," these are the next items on the list — either migrate them to fetchJson or wrap their failures in FetchError.

6. Body-read failure swallows the underlying error

The empty catch {} around await res.text() discards the real cause. super(msg, { cause: e }) (supported at the ES2019 target) preserves it for Sentry's chained-exception display.

7. Test gaps worth adding

• The non-JSON content-type branch (Run eslint during build, minor adjustments #1).
• Truncation behavior (verifies cutoff and any marker added).
• The res.text()-rejects path — easy to mock, today nothing pins down "stays status-only."

---

The single highest-leverage follow-up is #2 + #3: a captureFetchError helper plus URL on the error converts the new error-shape investment into actually-queryable Sentry data. Then #1 closes the loop so FREIGHTER-DMQ doesn't just migrate to a sibling issue on the JSON-content-type branch.

[aristidesstaffieri]

Nice fix on the !res.ok path. A few adjacent error-reporting gaps worth considering before / after merge:

1. The non-JSON content-type branch is still bare Error (same FREIGHTER-DMQ shape)

fetch.ts:9 still does:

throw new Error(`Did not receive json error:${content}`);

The body is interpolated into error.message, so each unique HTML/text payload (Cloudflare interstitials, gateway pages, etc.) becomes its own Sentry fingerprint — the inverse grouping bug. Worth giving this branch the FetchError treatment too (status 200-but-wrong-content-type, body in a property, stable message). The new test file has no case for it.

2. status / body properties are dead weight in Sentry as currently wired

Callers use plain Sentry.captureException(err) (e.g. blockaid.ts:132, :211, :656, :692). Sentry's default capture only reads message + stack; it does not enumerate own-properties on Error subclasses into tags/contexts. So you'll see "429" in the title but you can't filter http.status:429, alert on 5xx-only, or pivot by body.

To actually unlock the new fields, route through a small helper:

export function captureFetchError(err: unknown) {
  if (err instanceof FetchError) {
    Sentry.withScope((scope) => {
      scope.setTag("http.status", String(err.status));
      scope.setContext("http", { statusText: err.statusText, body: err.body });
      Sentry.captureException(err);
    });
    return;
  }
  Sentry.captureException(err);
}

…and swap the four fetchJson callers to it. Bonus: routing body through setContext runs Sentry's data scrubbers on it (see #4).

3. No URL / method on the error

"Request failed with status 429" doesn't say which endpoint. The stack helps in Sentry but log search and titles benefit from the URL. Suggest new FetchError(url, method, status, statusText, body?) with a sanitized URL (drop query string — report-asset-warning?address=…&details=… and the others contain user addresses / signed XDR).

4. body.slice(0, 200) is silent truncation + a PII vector

• No … / [truncated] marker, so future readers can't tell content was cut.
• 200 chars of arbitrary response body flowing into Sentry error.value (which Sentry indexes/displays) can include user addresses or signed XDR depending on endpoint. Prefer parsing JSON and surfacing a structured code/message, or keep message status-only and put body in scope.setContext so Sentry's scrubbers apply.

5. scanAsset and scanSite don't go through fetchJson — they're left out of the fix

• blockaid.ts:188 (scanAsset) uses raw fetch and on failure calls Sentry.captureException(response.error || "Failed to scan asset") — passing a string, which Sentry groups by exact text.
• Same shape at :542 / :582 (Failed to call scan site: ${error}) and :608.

If the goal is "fix Sentry grouping for indexer calls," these are the next items on the list — either migrate them to fetchJson or wrap their failures in FetchError.

6. Body-read failure swallows the underlying error

The empty catch {} around await res.text() discards the real cause. super(msg, { cause: e }) (supported at the ES2019 target) preserves it for Sentry's chained-exception display.

7. Test gaps worth adding

• The non-JSON content-type branch (Run eslint during build, minor adjustments #1).
• Truncation behavior (verifies cutoff and any marker added).
• The res.text()-rejects path — easy to mock, today nothing pins down "stays status-only."

The single highest-leverage follow-up is #2 + #3: a captureFetchError helper plus URL on the error converts the new error-shape investment into actually-queryable Sentry data. Then #1 closes the loop so FREIGHTER-DMQ doesn't just migrate to a sibling issue on the JSON-content-type branch.

I think we can ignore 4 here but the others are legit imo

[piyalbasu]

Pushed 24cada4a addressing the review. Quick map of comments → response:

1. Non-JSON branch — now throws FetchError with kind: "non-json". Body lives only on the property, not in error.message, so each body variant doesn't fragment grouping. Test added.
2. status / body were dead weight — added captureFetchError helper using Sentry.withScope + setTag("http.status", …) / setTag("http.method", …) / setTag("http.kind", …) + setContext("http", { url, method, status, statusText, body, kind }). Routing body through setContext also runs Sentry's scrubbers on it. Wired into all 7 blockaid.ts fetch-error sites.
3. URL / method on the error — FetchError carries both. URL is sanitized (origin + pathname; query string dropped) so signed XDR / addresses in report-asset-warning?address=… etc. don't leak into error.value. Sanitized URL also goes to the property and to setContext.
4. Body PII vector — skipped per your call. Mitigated indirectly via router fix #2 (scrubbers) and Piyal dev #3 (sanitized URL).
5. scanAsset / scanSite / scanAssetBulk outside fetchJson — all migrated to fetchJson. Plus the useAsyncSiteScan and fetchSiteScanData Sentry.captureException(\Failed to call scan site: ${error}`)string-captures replaced withcaptureFetchError(error). The res.errorJSON-body captures inreportAssetWarning/reportTransactionWarningare now wrapped innew Error(...)` (still plain Errors since they're not HTTP-level failures).
6. catch {} swallows underlying error — readBoundedText returns the rejection as readError, which is passed to super(msg, { cause }). New test asserts err.cause === readError.
7. Test gaps — added cases for: non-JSON branch (no body in message), truncation marker, Content-Length skip, and res.text() rejection with cause.

The PR description has been updated to reflect the expanded scope and the production bugs that are not fixed here (the actual 429/400/431 root causes — those are now in #2747's follow-up list and will become triageable as separate Sentry issues once the new error shapes start splitting).

Note: the failed test check on the prior commit was unrelated playwright flakes in accountHistory.test.ts (4 muxed-address / claimable-balance cases). Jest unit tests are green.

[aristidesstaffieri]

nice 👏

[piyalbasu]

@copilot resolve the merge conflicts in this pull request