Skip to content

Improve fetch request error reporting#1010

Merged
PeterDaveHello merged 1 commit into
ChatGPTBox-dev:masterfrom
PeterDaveHello:improve-fetch-request-errors
Jul 18, 2026
Merged

Improve fetch request error reporting#1010
PeterDaveHello merged 1 commit into
ChatGPTBox-dev:masterfrom
PeterDaveHello:improve-fetch-request-errors

Conversation

@PeterDaveHello

@PeterDaveHello PeterDaveHello commented Jul 16, 2026

Copy link
Copy Markdown
Member

Summary

  • classify invalid API endpoint URLs separately from request and response-stream transport failures
  • distinguish pre-response fetch failures from interrupted response streams without guessing whether DNS, CORS, TLS, or the provider caused the failure
  • show localized neutral summaries, actionable guidance, the request origin, and the browser diagnostic message
  • isolate background localization per request and route classified content-script failures through the shared error handler
  • preserve silent cancellation behavior, escape dynamic details, and add regression coverage across the transport lifecycle

Validation

  • npm run pretty
  • npm run lint
  • npm test (791 passed)
  • npm run build
  • Chromium 149.0.7827.55 extension smoke test (5 scenarios)
  • Firefox 152.0.6 extension smoke test (5 scenarios)
  • verified diagnostic blocks remain inert and unhighlighted in both browsers while JSON diagnostics retain syntax highlighting

Summary by CodeRabbit

  • Bug Fixes
    • Improved user-facing API/transport failure messaging with localized, multi-line details that include the configured endpoint and underlying browser error.
    • Prevented abort/cancellation from being shown as failures; improved safe escaping/formatting of rich error text.
    • Standardized port error reporting and strengthened WebSocket/network error normalization (including safer error output).
  • Localization
    • Expanded provider/API endpoint failure and retry guidance across supported languages, including %s placeholders for endpoint and browser message.
  • Tests
    • Expanded unit coverage for structured error codes, request-origin capture, translation scoping, and error formatting across network and WebSocket scenarios.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • ✅ Review completed - (🔄 Check again to review again)
📝 Walkthrough

Walkthrough

Fetch failures now receive structured metadata, delegated errors are handled centrally, and localized sanitized messages include endpoint and browser error details. Tests cover classification, propagation, abort handling, escaping, concurrency, and API integrations.

Changes

API request error handling

Layer / File(s) Summary
Fetch error classification and propagation
src/utils/fetch-sse.mjs, tests/unit/utils/fetch-sse.test.mjs, tests/unit/services/apis/*
Endpoints are validated and request or stream failures receive structured codes and request origins; abort behavior remains distinct and API tests assert the metadata.
Port error routing and formatting
src/content-script/*, src/services/wrappers.mjs
Classified errors are delegated to centralized handling, which selects per-request translations and posts sanitized endpoint and browser details.
Endpoint failure localization
src/_locales/*/main.json
Supported locales add messages for request failure, invalid endpoints, interrupted streams, retry guidance, and formatted diagnostic details.
Behavioral validation
tests/unit/content-script/*, tests/unit/services/*
Tests cover delegation, localized concurrent requests, structured classifications, escaping, stream failures, and abort handling.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant APIClient
  participant fetchSSE
  participant ContentScript
  participant handlePortError
  participant Port
  APIClient->>fetchSSE: request API endpoint
  fetchSSE-->>APIClient: classified fetch or stream error
  APIClient->>ContentScript: delegated error
  ContentScript->>handlePortError: pass classified error
  handlePortError->>Port: post localized sanitized message
Loading

Possibly related PRs

Suggested labels: Review effort 4/5

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main change: improved fetch request error reporting.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces robust error handling and localization for API fetch failures across multiple languages. It annotates fetch errors with a custom code and request origin, formats detailed error messages safely, and delegates them appropriately. The review feedback suggests enhancing the HTML escaping logic to prevent XSS vulnerabilities and handling cases where string errors are thrown instead of Error objects.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/services/wrappers.mjs Outdated
Comment thread src/content-script/port-error.mjs
@PeterDaveHello
PeterDaveHello marked this pull request as ready for review July 16, 2026 20:57
@PeterDaveHello
PeterDaveHello requested a review from Copilot July 16, 2026 20:57
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Improve fetch rejection error reporting with origin-aware, localized messages

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

AI Description

• Classify pre-response fetch rejections separately from HTTP/provider errors.
• Show a neutral, localized summary with request origin and browser diagnostics.
• Delegate classified content-script failures to the shared port error handler.
• Add i18n strings and unit coverage for classification, escaping, and abort handling.
Diagram

graph TD
API["API wrappers"] --> SSE["fetchSSE"] --> BF{{"Browser fetch"}}
CS["ChatGPT content script"] --> PE["port-error.mjs"] --> WH["handlePortError"] --> UI["User-visible error"]
WH --> I18N["Locale strings"]
BF -. "rejects" .-> SSE
SSE -. "annotates code+origin" .-> WH
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Send structured error payloads instead of formatted strings
  • ➕ Allows UI to localize and format consistently (no newline joining in the service layer)
  • ➕ Avoids needing HTML-escaping logic in the port error handler
  • ➖ Requires broader UI/consumer changes wherever errors are displayed/parsed
  • ➖ More coordination across components and higher regression risk
2. Introduce a dedicated Error subclass (e.g., FetchRequestFailedError)
  • ➕ More explicit typing than mutating arbitrary Error objects
  • ➕ Can preserve original error as .cause while adding metadata
  • ➖ May not survive cross-realm/cross-bundle boundaries depending on runtime and serialization
  • ➖ Still requires defensive handling for non-Error throws and browser-specific error types

Recommendation: Current approach is a good incremental fit: classify only true fetch rejections close to the fetch boundary, propagate minimal safe metadata (origin only), and keep aborts silent. The added HTML escaping and tests meaningfully reduce UX/security regressions without requiring a UI contract change.

Files changed (22) +336 / -29

Enhancement (4) +56 / -2
index.jsxDelegate classified fetch/abort failures to shared port error handling +3/-1

Delegate classified fetch/abort failures to shared port error handling

• Introduces content-script helpers to decide when to rethrow errors so they are handled by the shared port error handler. Keeps existing behavior for ordinary provider errors by posting a local error message when not delegated.

src/content-script/index.jsx

port-error.mjsAdd content-script port error delegation helpers +9/-0

Add content-script port error delegation helpers

• Adds shouldDelegatePortError() to route classified fetch failures and AbortError to the shared handler. Adds getPortErrorMessage() to preserve prior fallback behavior for non-delegated errors.

src/content-script/port-error.mjs

wrappers.mjsRender localized, escaped summaries for classified fetch rejections +16/-0

Render localized, escaped summaries for classified fetch rejections

• Adds FETCH_REQUEST_FAILED handling in handlePortError() to display a neutral localized summary plus optional origin and browser message. Escapes dynamic values to prevent HTML injection and ensures aborts remain silent.

src/services/wrappers.mjs

fetch-sse.mjsAnnotate fetch rejections with classification code and request origin +28/-1

Annotate fetch rejections with classification code and request origin

• Adds FETCH_REQUEST_FAILED and annotates fetch() rejections (excluding AbortError) with a stable code and the request URL origin (no path/query/credentials). Uses defensive property setting and skips origin when the URL is malformed.

src/utils/fetch-sse.mjs

Tests (5) +228 / -14
port-error.test.mjsAdd unit tests for content-script error delegation rules +37/-0

Add unit tests for content-script error delegation rules

• Covers delegation for classified fetch failures and AbortError, and ensures normal provider errors remain handled within the content script. Verifies fallback message formatting behavior.

tests/unit/content-script/port-error.test.mjs

custom-api.test.mjsAssert fetch rejection errors include classification metadata +7/-1

Assert fetch rejection errors include classification metadata

• Updates the network error test to assert the thrown error includes FETCH_REQUEST_FAILED and a sanitized requestOrigin derived from the endpoint URL.

tests/unit/services/apis/custom-api.test.mjs

openai-api-compat.test.mjsAssert OpenAI-compat network errors include classification metadata +18/-9

Assert OpenAI-compat network errors include classification metadata

• Updates the rejection assertion to validate message, FETCH_REQUEST_FAILED code, and requestOrigin extraction for fetch failures.

tests/unit/services/apis/openai-api-compat.test.mjs

handle-port-error.test.mjsAdd regression tests for localized fetch-failure summaries and escaping +107/-0

Add regression tests for localized fetch-failure summaries and escaping

• Adds coverage for Chromium/Firefox-style fetch rejection messages, HTML escaping (including replacement-token safety), and ensures AbortError remains silent even when classified.

tests/unit/services/handle-port-error.test.mjs

fetch-sse.test.mjsAdd tests for fetchSSE classification, origin parsing, and abort handling +59/-4

Add tests for fetchSSE classification, origin parsing, and abort handling

• Verifies non-OK HTTP responses are not classified, fetch rejections are annotated with code + origin (stripping credentials/path/query), malformed URLs omit origin, and AbortError is not reclassified.

tests/unit/utils/fetch-sse.test.mjs

Documentation (13) +52 / -13
main.jsonAdd German strings for neutral fetch-failure summary +4/-1

Add German strings for neutral fetch-failure summary

• Adds localized strings for the new browser-level fetch failure message and details (API endpoint + browser message). Also fixes JSON trailing comma by making the prior key non-terminal.

src/_locales/de/main.json

main.jsonAdd English strings for neutral fetch-failure summary +4/-1

Add English strings for neutral fetch-failure summary

• Adds the base English strings used to render classified fetch rejection errors, including endpoint and browser diagnostic placeholders.

src/_locales/en/main.json

main.jsonAdd Spanish strings for neutral fetch-failure summary +4/-1

Add Spanish strings for neutral fetch-failure summary

• Adds Spanish translations for the neutral browser fetch failure summary and its endpoint/message detail lines.

src/_locales/es/main.json

main.jsonAdd French strings for neutral fetch-failure summary +4/-1

Add French strings for neutral fetch-failure summary

• Adds French translations for the new fetch rejection summary and detail placeholders.

src/_locales/fr/main.json

main.jsonAdd Indonesian strings for neutral fetch-failure summary +4/-1

Add Indonesian strings for neutral fetch-failure summary

• Adds Indonesian translations for the neutral fetch failure summary and diagnostic details.

src/_locales/in/main.json

main.jsonAdd Italian strings for neutral fetch-failure summary +4/-1

Add Italian strings for neutral fetch-failure summary

• Adds Italian translations for the new browser fetch failure summary and detail placeholders.

src/_locales/it/main.json

main.jsonAdd Japanese strings for neutral fetch-failure summary +4/-1

Add Japanese strings for neutral fetch-failure summary

• Adds Japanese translations for the neutral fetch rejection summary plus endpoint/message detail lines.

src/_locales/ja/main.json

main.jsonAdd Korean strings for neutral fetch-failure summary +4/-1

Add Korean strings for neutral fetch-failure summary

• Adds Korean translations for the new fetch rejection summary and diagnostic placeholders.

src/_locales/ko/main.json

main.jsonAdd Portuguese strings for neutral fetch-failure summary +4/-1

Add Portuguese strings for neutral fetch-failure summary

• Adds Portuguese translations for the neutral browser fetch failure summary and endpoint/message details.

src/_locales/pt/main.json

main.jsonAdd Russian strings for neutral fetch-failure summary +4/-1

Add Russian strings for neutral fetch-failure summary

• Adds Russian translations for the new fetch rejection summary and associated detail strings.

src/_locales/ru/main.json

main.jsonAdd Turkish strings for neutral fetch-failure summary +4/-1

Add Turkish strings for neutral fetch-failure summary

• Adds Turkish translations for the neutral fetch failure summary and detail placeholders.

src/_locales/tr/main.json

main.jsonAdd Simplified Chinese strings for neutral fetch-failure summary +4/-1

Add Simplified Chinese strings for neutral fetch-failure summary

• Adds Simplified Chinese translations for the new browser fetch failure summary and endpoint/message details.

src/_locales/zh-hans/main.json

main.jsonAdd Traditional Chinese strings for neutral fetch-failure summary +4/-1

Add Traditional Chinese strings for neutral fetch-failure summary

• Adds Traditional Chinese translations for the neutral fetch failure summary and diagnostic placeholders.

src/_locales/zh-hant/main.json

Comment thread src/services/wrappers.mjs Outdated
@kilo-code-bot

kilo-code-bot Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (29 files)
  • src/_locales/de/main.json
  • src/_locales/en/main.json
  • src/_locales/es/main.json
  • src/_locales/fr/main.json
  • src/_locales/in/main.json
  • src/_locales/it/main.json
  • src/_locales/ja/main.json
  • src/_locales/ko/main.json
  • src/_locales/pt/main.json
  • src/_locales/ru/main.json
  • src/_locales/tr/main.json
  • src/_locales/zh-hans/main.json
  • src/_locales/zh-hant/main.json
  • src/components/ConversationCard/index.jsx
  • src/components/MarkdownRender/markdown-without-katex.jsx
  • src/components/MarkdownRender/markdown.jsx
  • src/content-script/index.jsx
  • src/content-script/port-error.mjs
  • src/services/clients/bing/index.mjs
  • src/services/wrappers.mjs
  • src/utils/abort-error.mjs
  • src/utils/error-text.mjs
  • src/utils/fetch-sse.mjs
  • tests/unit/content-script/port-error.test.mjs
  • tests/unit/services/apis/custom-api.test.mjs
  • tests/unit/services/apis/openai-api-compat.test.mjs
  • tests/unit/services/clients/bing.test.mjs
  • tests/unit/services/handle-port-error.test.mjs
  • tests/unit/services/wrappers-register.test.mjs
  • tests/unit/utils/error-text.test.mjs
  • tests/unit/utils/fetch-sse.test.mjs

Incremental review note: No new findings on changed lines since the prior review at 2ed5261 (full diff re-reviewed because the incremental git diff against ebd20468 could not be resolved in this sandbox). The transport-error classification, escaping, token redaction, and silent-cancellation behaviors are coherent, security-positive, and covered by regression tests.

Previous Review Summaries (8 snapshots, latest commit ebd2046)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit ebd2046)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (8 files, incremental since 2ed5261)
  • src/components/ConversationCard/index.jsx - no issues
  • src/components/MarkdownRender/markdown.jsx - no issues
  • src/components/MarkdownRender/markdown-without-katex.jsx - no issues
  • src/services/clients/bing/index.mjs - no issues
  • src/utils/error-text.mjs - no issues
  • tests/unit/content-script/port-error.test.mjs - no issues
  • tests/unit/services/clients/bing.test.mjs - no issues
  • tests/unit/utils/error-text.test.mjs - no issues

Incremental review note: No new findings on changed lines since the prior review at 2ed5261. The incremental changes are coherent and verified against current HEAD:

  • error-text.mjs adds a diagnostic/json language label to fenced error blocks and the display-detection regex now also matches legacy unlabeled fences ((?:diagnostic|json)?), preserving backward compatibility (covered by a new test).
  • ConversationCard/index.jsx switches the error path to a functional setConversationItemData updater, fixing a stale-closure bug when updating the last item in place.
  • markdown.jsx and markdown-without-katex.jsx register plainText: ['diagnostic'] with rehype-highlight so the diagnostic-labeled code fences are rendered as inert plain text rather than parsed as HTML/raw markup (security-positive, complements the new label).
  • bing/index.mjs logs the redacted connectionError instead of the raw event; the accompanying test asserts the secret token is absent and the redacted marker is present in the logged error.

Previous review (commit 2ed5261)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (12 files, incremental since c623fe3)
  • src/components/ConversationCard/index.jsx - no issues
  • src/content-script/index.jsx - no issues
  • src/content-script/port-error.mjs - no issues
  • src/services/clients/bing/index.mjs - no issues
  • src/services/wrappers.mjs - no issues
  • src/utils/abort-error.mjs (new) - no issues
  • src/utils/error-text.mjs - no issues
  • src/utils/fetch-sse.mjs - no issues
  • tests/unit/content-script/port-error.test.mjs - no issues
  • tests/unit/services/apis/custom-api.test.mjs - no issues
  • tests/unit/services/apis/openai-api-compat.test.mjs - no issues
  • tests/unit/services/clients/bing.test.mjs - no issues
  • tests/unit/services/handle-port-error.test.mjs - no issues
  • tests/unit/services/wrappers-register.test.mjs - no issues
  • tests/unit/utils/error-text.test.mjs - no issues
  • tests/unit/utils/fetch-sse.test.mjs - no issues

Incremental review note: No new findings on changed lines since the prior review at c623fe3. All previously raised findings across wrappers.mjs, fetch-sse.mjs, port-error.mjs, and bing/index.mjs are confirmed addressed in commits 004a665, e8ce4bb, and 2ed5261: frozen-error classification now wraps into a classified error; the transport lookup uses an own-property guard; isAbortError is shared and used uniformly; formatErrorDetail appends diagnostics when a translation omits %s; non-transport errors pass through formatErrorMessage; and the Bing WebSocket handshake onerror closes the socket only when settlement wins. The shared error-text.mjs helpers correctly fence rich/HTML/JSON content while passing through UNAUTHORIZED/CLOUDFLARE markers, and ConversationCard delegates display to getDisplayErrorText.

Previous review (commit c623fe3)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (12 files, incremental since e8ce4bb)
  • src/components/ConversationCard/index.jsx - no issues
  • src/content-script/port-error.mjs - no issues
  • src/services/clients/bing/index.mjs - no issues
  • src/services/wrappers.mjs - no issues
  • src/utils/error-text.mjs (new) - no issues
  • src/utils/fetch-sse.mjs - no issues
  • tests/unit/content-script/port-error.test.mjs - no issues
  • tests/unit/services/clients/bing.test.mjs (new) - no issues
  • tests/unit/services/handle-port-error.test.mjs - no issues
  • tests/unit/services/wrappers-register.test.mjs - no issues
  • tests/unit/utils/error-text.test.mjs (new) - no issues
  • tests/unit/utils/fetch-sse.test.mjs - no issues

Incremental review note: No new findings on changed lines since the prior review at e8ce4bb. The new shared error-text.mjs helpers (formatErrorText/formatErrorMessage/getDisplayErrorText) correctly fence rich/HTML/JSON error content as inert code blocks while passing through UNAUTHORIZED/CLOUDFLARE protocol markers, and the delimiter-length calc avoids premature fence termination. fetch-sse.mjs now reliably detects ignore-setter assignments via err[key] === value. The Bing WebSocket lifecycle refactor (abort handling, handshake timeout, close diagnostics, token redaction via redactWebSocketErrorMessage) is covered by 13 new tests and uses settled/closeExpected guards to avoid double settlement. ConversationCard correctly delegates display formatting to getDisplayErrorText, and silent cancellation behavior is preserved.

Previous review (commit e8ce4bb)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (23 files)
  • src/_locales/*/main.json (13 locale files) - no issues
  • src/content-script/index.jsx - no issues
  • src/content-script/port-error.mjs - no issues
  • src/services/wrappers.mjs - no new issues
  • src/utils/fetch-sse.mjs - no new issues
  • tests/unit/content-script/port-error.test.mjs - no issues
  • tests/unit/services/apis/custom-api.test.mjs - no issues
  • tests/unit/services/apis/openai-api-compat.test.mjs - no issues
  • tests/unit/services/handle-port-error.test.mjs - no issues
  • tests/unit/services/wrappers-register.test.mjs - no issues
  • tests/unit/utils/fetch-sse.test.mjs - no issues

Incremental review note: No new findings on changed lines since the prior review at 004a6659. The transport-classification logic (INVALID_API_ENDPOINT vs FETCH_REQUEST_FAILED vs FETCH_RESPONSE_STREAM_FAILED, response-stream separation, URL validation with credential stripping, escapeHtml detail escaping via replacement function, Object.prototype.hasOwnProperty guard against prototype pollution, per-request getFixedT translation scoping, and preserved silent AbortError/DOMException cancellation handling) is correct and covered by the expanded test suite. Prior warnings in the discussion thread are already addressed in 004a6659 or are CodeRabbit reply threads.

Previous review (commit 004a665)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (23 files)
  • src/_locales/*/main.json (13 locale files) - no issues
  • src/content-script/index.jsx - no issues
  • src/content-script/port-error.mjs - no issues
  • src/services/wrappers.mjs - no new issues
  • src/utils/fetch-sse.mjs - no new issues
  • tests/unit/content-script/port-error.test.mjs - no issues
  • tests/unit/services/apis/custom-api.test.mjs - no issues
  • tests/unit/services/apis/openai-api-compat.test.mjs - no issues
  • tests/unit/services/handle-port-error.test.mjs - no issues
  • tests/unit/services/wrappers-register.test.mjs - no issues
  • tests/unit/utils/fetch-sse.test.mjs - no issues

Incremental review note: No new findings on the changed lines since the prior review at 5c0709d6. The additional transport-classification logic (INVALID_API_ENDPOINT, response-stream separation, URL validation with credential stripping, escapeHtml detail escaping, per-request translation scoping, and legacy DOMException abort handling) is correct and covered by the expanded test suite. The CodeRabbit hasOwnProperty suggestion on the transportErrorSummaryKeys lookup is a false positive: the object is a plain literal keyed only by exported string constants, so prototype keys cannot match err.code.

Previous review (commit 5c0709d)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (22 files)
  • src/_locales/*/main.json (13 locale files) - no issues
  • src/content-script/index.jsx - no issues
  • src/content-script/port-error.mjs - no issues
  • src/services/wrappers.mjs - no new issues
  • src/utils/fetch-sse.mjs - no issues
  • tests/unit/content-script/port-error.test.mjs - no issues
  • tests/unit/services/apis/custom-api.test.mjs - no issues
  • tests/unit/services/apis/openai-api-compat.test.mjs - no issues
  • tests/unit/services/handle-port-error.test.mjs - no issues
  • tests/unit/services/wrappers-register.test.mjs - no issues
  • tests/unit/utils/fetch-sse.test.mjs - no issues

Note: This is a full review (the prior incremental base SHA 6ff41feed... was not present in the repository history, so the fallback path of gh pr diff 1010 was used). All previously raised inline comments from other reviewers were reviewed and either withdrawn (formatErrorDetail $-token false positive), accepted as pre-existing/out-of-scope (unescaped non-FETCH error branches, escapeHtml quote escaping), or resolved by new tests (per-request translation scoping addresses the background-locale-stuck-on-fallback concern). No new Code Review Findings were raised.

Previous review (commit 6ff41fe)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (13 files)
  • src/_locales/*/main.json (13 locale files) - no issues
  • src/content-script/index.jsx - no issues
  • src/content-script/port-error.mjs - no issues
  • src/services/wrappers.mjs - no new issues
  • src/utils/fetch-sse.mjs - no issues
  • tests/unit/content-script/port-error.test.mjs - no issues
  • tests/unit/services/apis/custom-api.test.mjs - no issues
  • tests/unit/services/apis/openai-api-compat.test.mjs - no issues
  • tests/unit/services/handle-port-error.test.mjs - no issues
  • tests/unit/services/wrappers-register.test.mjs - no issues
  • tests/unit/utils/fetch-sse.test.mjs - no issues

Note: The previous WARNING on src/services/wrappers.mjs:75 (formatErrorDetail) was a false positive. String.prototype.replace only interprets $&/$$ patterns when the replacement is a string; here the replacement is a function, so the returned value is inserted literally. The new "preserves replacement tokens" regression test confirms correct behavior. No action required.

Previous review (commit e82dc96)

Status: 1 Issue Found | Recommendation: Address before merge

Overview

Severity Count
CRITICAL 0
WARNING 1
SUGGESTION 0
Issue Details (click to expand)

WARNING

File Line Issue
src/services/wrappers.mjs 75 formatErrorDetail corrupts error detail strings containing $ patterns (e.g. $&, $$) because .replace('%s', () => escapeHtml(value)) re-applies $ substitution to the replacement. Also makes the new "preserves replacement tokens" regression test logically inconsistent with spec behavior. Fix: use (t(key) ?? key).split('%s').join(escapeHtml(value)).
Files Reviewed (17 files)
  • src/_locales/*/main.json (13 locale files) - no issues
  • src/content-script/index.jsx - no new issues (delegation behavior is intentional per PR)
  • src/content-script/port-error.mjs - no new issues (string-error handling noted by existing gemini comment, not duplicated)
  • src/services/wrappers.mjs - 1 issue (line 75)
  • src/utils/fetch-sse.mjs - no issues
  • tests/unit/content-script/port-error.test.mjs - no issues
  • tests/unit/services/apis/custom-api.test.mjs - no issues
  • tests/unit/services/apis/openai-api-compat.test.mjs - no issues
  • tests/unit/services/handle-port-error.test.mjs - existing token test likely incorrect (see line 75 finding)
  • tests/unit/utils/fetch-sse.test.mjs - no issues

Note: Two existing active comments from gemini-code-assist[bot] (escapeHtml quote-escaping on wrappers.mjs:72, and string-throw handling on port-error.mjs:9) were reviewed and intentionally not duplicated.

Fix these issues in Kilo Cloud


Reviewed by hy3:free · Input: 83.6K · Output: 8.4K · Cached: 359K

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves how pre-response fetch() failures are classified and surfaced to users across API modes and the ChatGPT Web (content-script) path, aiming to provide a neutral, localized, and safer error summary that includes the request origin and the browser’s diagnostic message.

Changes:

  • Classify fetch() rejections in fetchSSE with a dedicated FETCH_REQUEST_FAILED code plus a derived requestOrigin, while excluding AbortError.
  • Update shared port error handling to render a localized, neutral fetch-failure summary (with HTML-escaped details), and delegate certain content-script errors to the shared handler.
  • Add i18n strings and regression tests covering classification, escaping, abort handling, and content-script delegation.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/unit/utils/fetch-sse.test.mjs Adds coverage for fetch rejection classification, origin derivation/omission, and abort behavior in fetchSSE.
tests/unit/services/handle-port-error.test.mjs Adds tests for the new neutral fetch-failure summary, HTML escaping, and abort suppression in handlePortError.
tests/unit/services/apis/openai-api-compat.test.mjs Updates network-error assertions to validate classified fetch rejection metadata (code, requestOrigin).
tests/unit/services/apis/custom-api.test.mjs Updates network-error assertions to validate classified fetch rejection metadata (code, requestOrigin).
tests/unit/content-script/port-error.test.mjs Adds unit tests for content-script error delegation rules and fallback formatting.
src/utils/fetch-sse.mjs Introduces FETCH_REQUEST_FAILED classification and annotates fetch rejections with requestOrigin.
src/services/wrappers.mjs Adds localized neutral messaging for classified fetch failures and HTML-escapes fetch failure details.
src/content-script/port-error.mjs Adds delegation logic to route specific errors (classified fetch failures / aborts) to the shared handler.
src/content-script/index.jsx Uses new delegation + message formatting helpers in the ChatGPT Web port listener error path.
src/_locales/en/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/de/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/es/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/fr/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/in/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/it/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/ja/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/ko/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/pt/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/ru/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/tr/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/zh-hans/main.json Adds localized strings for the neutral fetch-failure summary and details.
src/_locales/zh-hant/main.json Adds localized strings for the neutral fetch-failure summary and details.

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

Comment thread src/services/wrappers.mjs Outdated
@qodo-code-review

qodo-code-review Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Legacy abort misclassified ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New
Description
handlePortError() and the content-script port handler treat cancellations only as errors with name
=== 'AbortError', so legacy DOMException aborts (ABORT_ERR) that fetchSSE intentionally treats as
cancellation can be reported as failures when they reach these handlers via non-fetchSSE paths.
Code

src/services/wrappers.mjs[R90-99]

+export function handlePortError(session, port, err, translate = t) {
+  const transportErrorSummaryKey = Object.prototype.hasOwnProperty.call(
+    transportErrorSummaryKeys,
+    err?.code,
+  )
+    ? transportErrorSummaryKeys[err.code]
+    : undefined
+  if (err?.name === 'AbortError') return
  if (isDisconnectedPortError(err)) {
    console.warn('[handlePortError] Ignoring disconnected port error:', err.message)
Evidence
handlePortError() and shouldDelegatePortError() only check err.name === 'AbortError', while
fetchSSE() explicitly supports legacy aborts by checking DOMException.ABORT_ERR even if the
error’s name is not AbortError (and there is a test demonstrating that exact legacy shape). This
mismatch means legacy aborts are handled silently in the fetch layer but could be surfaced if they
reach the port-level handlers via other paths.

src/services/wrappers.mjs[90-99]
src/content-script/port-error.mjs[14-16]
src/utils/fetch-sse.mjs[3-11]
tests/unit/utils/fetch-sse.test.mjs[393-420]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Cancellation detection is inconsistent across layers:
- `fetchSSE()` treats legacy aborts as cancellation (`DOMException.ABORT_ERR`) even when `err.name` is not `'AbortError'`.
- `handlePortError()` and content-script delegation logic ignore cancellations **only** when `err.name === 'AbortError'`.

If a legacy abort-shaped error reaches `handlePortError()` / `shouldDelegatePortError()` through a path that bypasses `fetchSSE()` (e.g., other transports or callers), it may be surfaced to users instead of remaining silent.

### Issue Context
There is already explicit test coverage asserting that legacy aborts exist and should be treated as cancellation in `fetchSSE()`.

### Fix Focus Areas
- src/services/wrappers.mjs[90-99]
- src/content-script/port-error.mjs[14-16]

### Suggested fix
1. Create a shared `isAbortError(err)` helper (e.g., `src/utils/abort-error.mjs`) that matches the `fetch-sse` semantics:
  - `err?.name === 'AbortError'`, OR
  - `err instanceof DOMException && err.code === DOMException.ABORT_ERR` (when `DOMException` exists)
2. Use that helper in:
  - `handlePortError()` (early return)
  - `shouldDelegatePortError()` (delegate/ignore cancellation consistently)
3. Add/adjust unit tests for `handlePortError()` / `port-error.mjs` to cover the legacy DOMException abort shape (similar to the existing `fetchSSE` legacy-abort test).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Raw error markup injection ✓ Resolved 🐞 Bug ⛨ Security
Description
handlePortError() still posts raw err.message / JSON.stringify(err) for non-transport errors, and
those strings are rendered via ReactMarkdown with rehypeRaw enabled, so HTML inside an error will be
interpreted as markup in the extension UI. This leaves a remaining UI-injection/XSS surface outside
the new transport-error escaping path.
Code

src/services/wrappers.mjs[R132-167]

  if (message) {
    if (
      ['message you submitted was too long', 'maximum context length'].some((m) =>
        message.includes(m),
      )
    )
-      postError(t('Exceeded maximum context length') + '\n\n' + message)
+      postError(translate('Exceeded maximum context length') + '\n\n' + message)
    else if (['CaptchaChallenge', 'CAPTCHA'].some((m) => message.includes(m)))
-      postError(t('Bing CaptchaChallenge') + '\n\n' + message)
+      postError(translate('Bing CaptchaChallenge') + '\n\n' + message)
    else if (['exceeded your current quota'].some((m) => message.includes(m)))
-      postError(t('Exceeded quota') + '\n\n' + message)
+      postError(translate('Exceeded quota') + '\n\n' + message)
    else if (['Rate limit reached'].some((m) => message.includes(m)))
-      postError(t('Rate limit') + '\n\n' + message)
+      postError(translate('Rate limit') + '\n\n' + message)
    else if (['authentication token has expired'].some((m) => message.includes(m)))
      postError('UNAUTHORIZED')
    else if (
      isUsingClaudeWebModel(session) &&
      ['Invalid authorization', 'Session key required'].some((m) => message.includes(m))
    )
-      postError(t('Please login at https://claude.ai first, and then click the retry button'))
+      postError(
+        translate('Please login at https://claude.ai first, and then click the retry button'),
+      )
    else if (
      isUsingBingWebModel(session) &&
      ['/turing/conversation/create: failed to parse response body.'].some((m) =>
        message.includes(m),
      )
    )
-      postError(t('Please login at https://bing.com first'))
+      postError(translate('Please login at https://bing.com first'))
    else postError(message)
  } else {
    const errMsg = JSON.stringify(err) ?? 'unknown error'
    if (isUsingBingWebModel(session) && errMsg.includes('isTrusted'))
-      postError(t('Please login at https://bing.com first'))
+      postError(translate('Please login at https://bing.com first'))
    else postError(errMsg)
  }
Evidence
handlePortError() posts raw message/errMsg in several branches, while ConversationItem renders
type='error' content using MarkdownRender, which enables rehypeRaw to parse/render raw HTML in
markdown.

src/services/wrappers.mjs[100-167]
src/components/ConversationItem/index.jsx[91-123]
src/components/MarkdownRender/markdown.jsx[114-193]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`handlePortError()` sometimes forwards raw `err.message` / `JSON.stringify(err)` to the UI. Error content is rendered using `ReactMarkdown` with `rehypeRaw`, so raw HTML inside these error strings can be interpreted as markup.

## Issue Context
This PR introduces `escapeHtml()` but applies it only to the new classified transport-error detail lines; other branches still emit unescaped strings.

## Fix Focus Areas
- src/services/wrappers.mjs[86-167]
- src/components/ConversationItem/index.jsx[91-123]
- src/components/MarkdownRender/markdown.jsx[114-193]

## What to change
- Ensure **all** strings passed to `postError(...)` are treated as plain text:
 - Either escape `message`/`errMsg` with the same `escapeHtml()` helper in every non-transport branch (including the `Exceeded maximum context length` / `CaptchaChallenge` / `Rate limit` branches).
 - Or (more robust) stop rendering error items with `rehypeRaw` (e.g., render errors with a safe Markdown component/config that does not parse raw HTML), and keep transport escaping as defense-in-depth.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


3. Classification fails on frozen errors ✓ Resolved 🐞 Bug ☼ Reliability
Description
fetchSSE annotates transport failures by mutating the thrown error object, but setErrorProperty()
can fail and its return value is ignored, leaving err.code/requestOrigin unset. When that happens,
shouldDelegatePortError() and handlePortError() won’t take the new classified transport path and
will fall back to the legacy raw-message handling.
Code

src/utils/fetch-sse.mjs[R13-58]

+function setErrorProperty(err, key, value) {
+  try {
+    err[key] = value
+  } catch {
+    return false
+  }
+  return true
+}
+
+function getHttpRequestUrl(resource) {
+  let url
+  try {
+    url = new URL(resource?.url ?? resource)
+  } catch {
+    return null
+  }
+  if (!['http:', 'https:'].includes(url.protocol) || url.username || url.password) return null
+  return url
+}
+
+function createInvalidApiEndpointError() {
+  const err = new TypeError()
+  setErrorProperty(err, 'code', INVALID_API_ENDPOINT)
+  return err
+}
+
+function annotateRequestError(resource, err) {
+  if (!err || typeof err !== 'object' || err.name === 'AbortError') return err
+
+  const url = getHttpRequestUrl(resource)
+  setErrorProperty(err, 'code', url ? FETCH_REQUEST_FAILED : INVALID_API_ENDPOINT)
+  if (!url) return err
+  setErrorProperty(err, 'requestOrigin', url.origin)
+
+  return err
+}
+
+function annotateResponseStreamError(resource, err) {
+  if (!err || typeof err !== 'object' || err.name === 'AbortError') return err
+
+  setErrorProperty(err, 'code', FETCH_RESPONSE_STREAM_FAILED)
+  const url = getHttpRequestUrl(resource)
+  if (url) setErrorProperty(err, 'requestOrigin', url.origin)
+
+  return err
+}
Evidence
setErrorProperty() explicitly returns false when annotation cannot be applied, but both
annotation helpers ignore that and still return the original error object. Downstream logic depends
on err.code to select the new transport error messaging and delegation behavior, so missing code
disables the new path.

src/utils/fetch-sse.mjs[13-58]
src/services/wrappers.mjs[94-126]
src/content-script/port-error.mjs[7-15]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`fetchSSE()` classifies transport failures by adding `code` and `requestOrigin` onto the thrown `Error` object. If the error is non-extensible (or otherwise rejects new properties), `setErrorProperty()` returns `false`, but callers ignore that result, so the classification metadata can be missing.

This breaks the new behavior in two places:
- `handlePortError()` only enters the new transport-summary branch when `err.code` matches.
- The content-script delegation path (`shouldDelegatePortError`) also depends on `err.code`.

### Issue Context
This is an edge case (native browser `TypeError` is usually extensible), but the code explicitly anticipates property-set failure via `setErrorProperty()` returning `false`, so it’s worth making the classification robust.

### Fix Focus Areas
- src/utils/fetch-sse.mjs[13-58]

### Suggested fix
1. In `annotateRequestError()` and `annotateResponseStreamError()`, compute the intended `code` and `requestOrigin`.
2. If setting `code` fails, create a new `Error`/`TypeError` wrapper that:
  - copies `name`, `message`, and `stack` (best-effort)
  - sets `code` and `requestOrigin` on the new object
  - optionally sets `cause` to the original error
3. Return the wrapped error from the annotate function so downstream handlers always see the classification.

This preserves the best-effort behavior while ensuring the new classification/escaping path is consistently applied.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


View more (1)
4. Background locale may stay default ✓ Resolved 🐞 Bug ≡ Correctness
Description
handlePortError() now localizes FETCH_REQUEST_FAILED messages in the background via i18next.t(), but
refreshMenu() sets the background language only after an early return when hideContextMenu is
enabled, leaving error summaries stuck in fallback language for that configuration. This is
user-visible specifically for the new fetch-failure summary/details text.
Code

src/services/wrappers.mjs[R82-87]

+  if (err?.code === FETCH_REQUEST_FAILED) {
+    const details = [t('The browser could not complete the request to the API endpoint.')]
+    if (err.requestOrigin) details.push(formatErrorDetail('API endpoint: %s', err.requestOrigin))
+    if (message) details.push(formatErrorDetail('Browser message: %s', message))
+    port.postMessage({ error: details.join('\n\n') })
+    return
Evidence
The new FETCH_REQUEST_FAILED path in handlePortError() uses t(...) to build the message. In the
background, language selection is performed inside refreshMenu(), but it is skipped entirely when
hideContextMenu is true, and the i18n init specifies fallbackLng: 'en', so background-side
translations can remain at the fallback language.

src/services/wrappers.mjs[78-88]
src/background/menus.mjs[109-118]
src/_locales/i18n.mjs[1-7]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`handlePortError()` formats `FETCH_REQUEST_FAILED` errors using `t(...)` in the background/service layer. In the background script, the i18next language is set in `refreshMenu()` only after the `hideContextMenu` early-return, so when `hideContextMenu` is true the background can remain on `fallbackLng` (en) and emit English error summaries.

## Issue Context
- `handlePortError()` is invoked from `registerPortListener()` in the background when API execution throws.
- `refreshMenu()` skips `changeLanguage(lang)` when `hideContextMenu` is enabled.
- i18next background init uses `fallbackLng: 'en'`.

## Fix Focus Areas
- src/background/menus.mjs[109-118]
- src/services/wrappers.mjs[78-88]
- src/_locales/i18n.mjs[1-7]

## Suggested fix
Initialize i18next language for the background script independently of context-menu rendering (e.g., in `src/background/index.mjs` right after importing `../_locales/i18n`, call `getPreferredLanguageKey()` and `changeLanguage(lang)`; keep `refreshMenu()` as-is). Alternatively, avoid pre-localizing in `handlePortError()` and send stable keys/structured payload to the UI for translation there.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

5. Duplicate fetch-failure logs ✓ Resolved 🐞 Bug ◔ Observability
Description
For delegated fetch failures, the content-script logs the error and then rethrows, and the shared
background handler logs the same error again, producing duplicate console.error entries. This
affects delegated non-abort errors (e.g., FETCH_REQUEST_FAILED), not AbortError (which is returned
early in handlePortError()).
Code

src/content-script/index.jsx[R899-905]

      } catch (e) {
        console.error('[content] Error in port listener callback:', e, 'Session:', session)
+        if (shouldDelegatePortError(e)) throw e
        try {
          port.postMessage({
-            error: e.message || 'An unexpected error occurred in content script port listener.',
+            error: getPortErrorMessage(e),
          })
Evidence
The content script logs errors in its catch block and now rethrows delegated errors. The shared
handler logs again for non-abort errors, so delegated fetch failures are duplicated in logs.

src/content-script/index.jsx[899-905]
src/services/wrappers.mjs[78-83]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Delegated errors are now logged twice: the content script logs in its catch block and then rethrows; `registerPortListener()` catches and calls `handlePortError()`, which logs again for non-abort errors.

## Issue Context
- Content script catch logs and then `throw e` when `shouldDelegatePortError(e)` is true.
- `handlePortError()` logs (`console.error(err)`) for all non-abort errors.

## Fix Focus Areas
- src/content-script/index.jsx[899-905]
- src/services/wrappers.mjs[78-83]

## Suggested fix
In the content script, decide delegation first and skip logging for delegated errors, e.g.:
- `if (shouldDelegatePortError(e)) throw e` before `console.error(...)`, or
- wrap the log in `if (!shouldDelegatePortError(e)) ...`.
This keeps the shared handler as the single logging point for delegated fetch failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Previous review results

Review updated until commit 59c2a93

Results up to commit e82dc96


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Background locale may stay default ✓ Resolved 🐞 Bug ≡ Correctness
Description
handlePortError() now localizes FETCH_REQUEST_FAILED messages in the background via i18next.t(), but
refreshMenu() sets the background language only after an early return when hideContextMenu is
enabled, leaving error summaries stuck in fallback language for that configuration. This is
user-visible specifically for the new fetch-failure summary/details text.
Code

src/services/wrappers.mjs[R82-87]

+  if (err?.code === FETCH_REQUEST_FAILED) {
+    const details = [t('The browser could not complete the request to the API endpoint.')]
+    if (err.requestOrigin) details.push(formatErrorDetail('API endpoint: %s', err.requestOrigin))
+    if (message) details.push(formatErrorDetail('Browser message: %s', message))
+    port.postMessage({ error: details.join('\n\n') })
+    return
Evidence
The new FETCH_REQUEST_FAILED path in handlePortError() uses t(...) to build the message. In the
background, language selection is performed inside refreshMenu(), but it is skipped entirely when
hideContextMenu is true, and the i18n init specifies fallbackLng: 'en', so background-side
translations can remain at the fallback language.

src/services/wrappers.mjs[78-88]
src/background/menus.mjs[109-118]
src/_locales/i18n.mjs[1-7]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`handlePortError()` formats `FETCH_REQUEST_FAILED` errors using `t(...)` in the background/service layer. In the background script, the i18next language is set in `refreshMenu()` only after the `hideContextMenu` early-return, so when `hideContextMenu` is true the background can remain on `fallbackLng` (en) and emit English error summaries.

## Issue Context
- `handlePortError()` is invoked from `registerPortListener()` in the background when API execution throws.
- `refreshMenu()` skips `changeLanguage(lang)` when `hideContextMenu` is enabled.
- i18next background init uses `fallbackLng: 'en'`.

## Fix Focus Areas
- src/background/menus.mjs[109-118]
- src/services/wrappers.mjs[78-88]
- src/_locales/i18n.mjs[1-7]

## Suggested fix
Initialize i18next language for the background script independently of context-menu rendering (e.g., in `src/background/index.mjs` right after importing `../_locales/i18n`, call `getPreferredLanguageKey()` and `changeLanguage(lang)`; keep `refreshMenu()` as-is). Alternatively, avoid pre-localizing in `handlePortError()` and send stable keys/structured payload to the UI for translation there.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational
2. Duplicate fetch-failure logs ✓ Resolved 🐞 Bug ◔ Observability
Description
For delegated fetch failures, the content-script logs the error and then rethrows, and the shared
background handler logs the same error again, producing duplicate console.error entries. This
affects delegated non-abort errors (e.g., FETCH_REQUEST_FAILED), not AbortError (which is returned
early in handlePortError()).
Code

src/content-script/index.jsx[R899-905]

      } catch (e) {
        console.error('[content] Error in port listener callback:', e, 'Session:', session)
+        if (shouldDelegatePortError(e)) throw e
        try {
          port.postMessage({
-            error: e.message || 'An unexpected error occurred in content script port listener.',
+            error: getPortErrorMessage(e),
          })
Evidence
The content script logs errors in its catch block and now rethrows delegated errors. The shared
handler logs again for non-abort errors, so delegated fetch failures are duplicated in logs.

src/content-script/index.jsx[899-905]
src/services/wrappers.mjs[78-83]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Delegated errors are now logged twice: the content script logs in its catch block and then rethrows; `registerPortListener()` catches and calls `handlePortError()`, which logs again for non-abort errors.

## Issue Context
- Content script catch logs and then `throw e` when `shouldDelegatePortError(e)` is true.
- `handlePortError()` logs (`console.error(err)`) for all non-abort errors.

## Fix Focus Areas
- src/content-script/index.jsx[899-905]
- src/services/wrappers.mjs[78-83]

## Suggested fix
In the content script, decide delegation first and skip logging for delegated errors, e.g.:
- `if (shouldDelegatePortError(e)) throw e` before `console.error(...)`, or
- wrap the log in `if (!shouldDelegatePortError(e)) ...`.
This keeps the shared handler as the single logging point for delegated fetch failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit 5c0709d


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Classification fails on frozen errors ✓ Resolved 🐞 Bug ☼ Reliability
Description
fetchSSE annotates transport failures by mutating the thrown error object, but setErrorProperty()
can fail and its return value is ignored, leaving err.code/requestOrigin unset. When that happens,
shouldDelegatePortError() and handlePortError() won’t take the new classified transport path and
will fall back to the legacy raw-message handling.
Code

src/utils/fetch-sse.mjs[R13-58]

+function setErrorProperty(err, key, value) {
+  try {
+    err[key] = value
+  } catch {
+    return false
+  }
+  return true
+}
+
+function getHttpRequestUrl(resource) {
+  let url
+  try {
+    url = new URL(resource?.url ?? resource)
+  } catch {
+    return null
+  }
+  if (!['http:', 'https:'].includes(url.protocol) || url.username || url.password) return null
+  return url
+}
+
+function createInvalidApiEndpointError() {
+  const err = new TypeError()
+  setErrorProperty(err, 'code', INVALID_API_ENDPOINT)
+  return err
+}
+
+function annotateRequestError(resource, err) {
+  if (!err || typeof err !== 'object' || err.name === 'AbortError') return err
+
+  const url = getHttpRequestUrl(resource)
+  setErrorProperty(err, 'code', url ? FETCH_REQUEST_FAILED : INVALID_API_ENDPOINT)
+  if (!url) return err
+  setErrorProperty(err, 'requestOrigin', url.origin)
+
+  return err
+}
+
+function annotateResponseStreamError(resource, err) {
+  if (!err || typeof err !== 'object' || err.name === 'AbortError') return err
+
+  setErrorProperty(err, 'code', FETCH_RESPONSE_STREAM_FAILED)
+  const url = getHttpRequestUrl(resource)
+  if (url) setErrorProperty(err, 'requestOrigin', url.origin)
+
+  return err
+}
Evidence
setErrorProperty() explicitly returns false when annotation cannot be applied, but both
annotation helpers ignore that and still return the original error object. Downstream logic depends
on err.code to select the new transport error messaging and delegation behavior, so missing code
disables the new path.

src/utils/fetch-sse.mjs[13-58]
src/services/wrappers.mjs[94-126]
src/content-script/port-error.mjs[7-15]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`fetchSSE()` classifies transport failures by adding `code` and `requestOrigin` onto the thrown `Error` object. If the error is non-extensible (or otherwise rejects new properties), `setErrorProperty()` returns `false`, but callers ignore that result, so the classification metadata can be missing.

This breaks the new behavior in two places:
- `handlePortError()` only enters the new transport-summary branch when `err.code` matches.
- The content-script delegation path (`shouldDelegatePortError`) also depends on `err.code`.

### Issue Context
This is an edge case (native browser `TypeError` is usually extensible), but the code explicitly anticipates property-set failure via `setErrorProperty()` returning `false`, so it’s worth making the classification robust.

### Fix Focus Areas
- src/utils/fetch-sse.mjs[13-58]

### Suggested fix
1. In `annotateRequestError()` and `annotateResponseStreamError()`, compute the intended `code` and `requestOrigin`.
2. If setting `code` fails, create a new `Error`/`TypeError` wrapper that:
  - copies `name`, `message`, and `stack` (best-effort)
  - sets `code` and `requestOrigin` on the new object
  - optionally sets `cause` to the original error
3. Return the wrapped error from the annotate function so downstream handlers always see the classification.

This preserves the best-effort behavior while ensuring the new classification/escaping path is consistently applied.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Results up to commit e8ce4bb


🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Remediation recommended
1. Raw error markup injection ✓ Resolved 🐞 Bug ⛨ Security
Description
handlePortError() still posts raw err.message / JSON.stringify(err) for non-transport errors, and
those strings are rendered via ReactMarkdown with rehypeRaw enabled, so HTML inside an error will be
interpreted as markup in the extension UI. This leaves a remaining UI-injection/XSS surface outside
the new transport-error escaping path.
Code

src/services/wrappers.mjs[R132-167]

  if (message) {
    if (
      ['message you submitted was too long', 'maximum context length'].some((m) =>
        message.includes(m),
      )
    )
-      postError(t('Exceeded maximum context length') + '\n\n' + message)
+      postError(translate('Exceeded maximum context length') + '\n\n' + message)
    else if (['CaptchaChallenge', 'CAPTCHA'].some((m) => message.includes(m)))
-      postError(t('Bing CaptchaChallenge') + '\n\n' + message)
+      postError(translate('Bing CaptchaChallenge') + '\n\n' + message)
    else if (['exceeded your current quota'].some((m) => message.includes(m)))
-      postError(t('Exceeded quota') + '\n\n' + message)
+      postError(translate('Exceeded quota') + '\n\n' + message)
    else if (['Rate limit reached'].some((m) => message.includes(m)))
-      postError(t('Rate limit') + '\n\n' + message)
+      postError(translate('Rate limit') + '\n\n' + message)
    else if (['authentication token has expired'].some((m) => message.includes(m)))
      postError('UNAUTHORIZED')
    else if (
      isUsingClaudeWebModel(session) &&
      ['Invalid authorization', 'Session key required'].some((m) => message.includes(m))
    )
-      postError(t('Please login at https://claude.ai first, and then click the retry button'))
+      postError(
+        translate('Please login at https://claude.ai first, and then click the retry button'),
+      )
    else if (
      isUsingBingWebModel(session) &&
      ['/turing/conversation/create: failed to parse response body.'].some((m) =>
        message.includes(m),
      )
    )
-      postError(t('Please login at https://bing.com first'))
+      postError(translate('Please login at https://bing.com first'))
    else postError(message)
  } else {
    const errMsg = JSON.stringify(err) ?? 'unknown error'
    if (isUsingBingWebModel(session) && errMsg.includes('isTrusted'))
-      postError(t('Please login at https://bing.com first'))
+      postError(translate('Please login at https://bing.com first'))
    else postError(errMsg)
  }
Evidence
handlePortError() posts raw message/errMsg in several branches, while ConversationItem renders
type='error' content using MarkdownRender, which enables rehypeRaw to parse/render raw HTML in
markdown.

src/services/wrappers.mjs[100-167]
src/components/ConversationItem/index.jsx[91-123]
src/components/MarkdownRender/markdown.jsx[114-193]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`handlePortError()` sometimes forwards raw `err.message` / `JSON.stringify(err)` to the UI. Error content is rendered using `ReactMarkdown` with `rehypeRaw`, so raw HTML inside these error strings can be interpreted as markup.

## Issue Context
This PR introduces `escapeHtml()` but applies it only to the new classified transport-error detail lines; other branches still emit unescaped strings.

## Fix Focus Areas
- src/services/wrappers.mjs[86-167]
- src/components/ConversationItem/index.jsx[91-123]
- src/components/MarkdownRender/markdown.jsx[114-193]

## What to change
- Ensure **all** strings passed to `postError(...)` are treated as plain text:
 - Either escape `message`/`errMsg` with the same `escapeHtml()` helper in every non-transport branch (including the `Exceeded maximum context length` / `CaptchaChallenge` / `Rate limit` branches).
 - Or (more robust) stop rendering error items with `rehypeRaw` (e.g., render errors with a safe Markdown component/config that does not parse raw HTML), and keep transport escaping as defense-in-depth.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Qodo Logo

Comment thread src/services/wrappers.mjs Outdated
Comment thread src/content-script/index.jsx
@PeterDaveHello
PeterDaveHello force-pushed the improve-fetch-request-errors branch 2 times, most recently from 6ff41fe to 5c0709d Compare July 17, 2026 18:49
@PeterDaveHello
PeterDaveHello requested a review from Copilot July 17, 2026 19:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Comment thread src/services/wrappers.mjs Outdated
Comment thread src/utils/fetch-sse.mjs
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 5c0709d

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/services/wrappers.mjs`:
- Around line 94-102: Update the transportErrorSummaryKeys lookup in
handlePortError to accept only own properties, preventing inherited keys such as
constructor or toString from being treated as transport error codes. Preserve
the existing abort handling and translation flow for recognized transport codes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a99cfeb9-6adc-4e0e-b53d-463a2c24f42c

📥 Commits

Reviewing files that changed from the base of the PR and between 5c0709d and 563547f.

📒 Files selected for processing (23)
  • src/_locales/de/main.json
  • src/_locales/en/main.json
  • src/_locales/es/main.json
  • src/_locales/fr/main.json
  • src/_locales/in/main.json
  • src/_locales/it/main.json
  • src/_locales/ja/main.json
  • src/_locales/ko/main.json
  • src/_locales/pt/main.json
  • src/_locales/ru/main.json
  • src/_locales/tr/main.json
  • src/_locales/zh-hans/main.json
  • src/_locales/zh-hant/main.json
  • src/content-script/index.jsx
  • src/content-script/port-error.mjs
  • src/services/wrappers.mjs
  • src/utils/fetch-sse.mjs
  • tests/unit/content-script/port-error.test.mjs
  • tests/unit/services/apis/custom-api.test.mjs
  • tests/unit/services/apis/openai-api-compat.test.mjs
  • tests/unit/services/handle-port-error.test.mjs
  • tests/unit/services/wrappers-register.test.mjs
  • tests/unit/utils/fetch-sse.test.mjs
🚧 Files skipped from review as they are similar to previous changes (19)
  • src/_locales/ja/main.json
  • src/_locales/zh-hant/main.json
  • src/_locales/in/main.json
  • src/_locales/es/main.json
  • src/_locales/ru/main.json
  • src/_locales/fr/main.json
  • src/_locales/it/main.json
  • src/_locales/de/main.json
  • src/content-script/port-error.mjs
  • src/content-script/index.jsx
  • tests/unit/services/apis/openai-api-compat.test.mjs
  • src/_locales/pt/main.json
  • src/_locales/en/main.json
  • src/_locales/tr/main.json
  • tests/unit/content-script/port-error.test.mjs
  • src/utils/fetch-sse.mjs
  • src/_locales/ko/main.json
  • tests/unit/services/wrappers-register.test.mjs
  • tests/unit/services/handle-port-error.test.mjs

Comment thread src/services/wrappers.mjs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Comment thread src/services/wrappers.mjs
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit e8ce4bb

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Comment thread src/services/clients/bing/index.mjs
Comment thread src/services/wrappers.mjs
Comment thread src/services/wrappers.mjs
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit c623fe3

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 2ed5261

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/components/ConversationCard/index.jsx`:
- Around line 257-266: The error-handling update around lastItem must use a
functional setConversationItemData state update so it reads the latest
conversation items when appending the error. Preserve the existing
duplicate/loading check and updateAnswer behavior, but avoid spreading the stale
conversationItemData closure captured by the listener.

In `@src/services/clients/bing/index.mjs`:
- Around line 407-413: Remove the raw console.error call from the ws.onerror
handler and only process the event through createWebSocketError(error). Preserve
the existing conditional assignment to webSocketError and
abortController.abort() based on the sanitized error message, ensuring the
unredacted WebSocket event is never logged.

In `@tests/unit/services/clients/bing.test.mjs`:
- Around line 104-107: Replace the double-quoted string literals at
tests/unit/services/clients/bing.test.mjs:104-107 and 235-238 with single-quoted
strings, escaping embedded double quotes as needed; also update the instruction
string at tests/unit/utils/error-text.test.mjs:49-53 to use single quotes and
escape its contraction.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 040bef6d-f935-4f3c-ba36-32b74b312b46

📥 Commits

Reviewing files that changed from the base of the PR and between 563547f and 2ed5261.

📒 Files selected for processing (29)
  • src/_locales/de/main.json
  • src/_locales/en/main.json
  • src/_locales/es/main.json
  • src/_locales/fr/main.json
  • src/_locales/in/main.json
  • src/_locales/it/main.json
  • src/_locales/ja/main.json
  • src/_locales/ko/main.json
  • src/_locales/pt/main.json
  • src/_locales/ru/main.json
  • src/_locales/tr/main.json
  • src/_locales/zh-hans/main.json
  • src/_locales/zh-hant/main.json
  • src/components/ConversationCard/index.jsx
  • src/content-script/index.jsx
  • src/content-script/port-error.mjs
  • src/services/clients/bing/index.mjs
  • src/services/wrappers.mjs
  • src/utils/abort-error.mjs
  • src/utils/error-text.mjs
  • src/utils/fetch-sse.mjs
  • tests/unit/content-script/port-error.test.mjs
  • tests/unit/services/apis/custom-api.test.mjs
  • tests/unit/services/apis/openai-api-compat.test.mjs
  • tests/unit/services/clients/bing.test.mjs
  • tests/unit/services/handle-port-error.test.mjs
  • tests/unit/services/wrappers-register.test.mjs
  • tests/unit/utils/error-text.test.mjs
  • tests/unit/utils/fetch-sse.test.mjs
🚧 Files skipped from review as they are similar to previous changes (20)
  • src/_locales/es/main.json
  • src/_locales/zh-hant/main.json
  • src/content-script/port-error.mjs
  • src/_locales/it/main.json
  • tests/unit/services/apis/custom-api.test.mjs
  • src/_locales/fr/main.json
  • src/_locales/en/main.json
  • src/_locales/tr/main.json
  • src/_locales/ja/main.json
  • src/_locales/ko/main.json
  • src/_locales/ru/main.json
  • src/content-script/index.jsx
  • src/_locales/pt/main.json
  • src/_locales/zh-hans/main.json
  • tests/unit/services/apis/openai-api-compat.test.mjs
  • src/_locales/de/main.json
  • tests/unit/services/wrappers-register.test.mjs
  • src/utils/fetch-sse.mjs
  • src/services/wrappers.mjs
  • tests/unit/utils/fetch-sse.test.mjs

Comment thread src/components/ConversationCard/index.jsx Outdated
Comment thread src/services/clients/bing/index.mjs
Comment thread tests/unit/services/clients/bing.test.mjs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.

Comment thread src/services/clients/bing/index.mjs
@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit ebd2046

Classify invalid endpoints, pre-response request failures, and response
stream interruptions separately so the UI can provide localized,
actionable guidance without guessing the underlying cause.

Include only request origins and preserve browser diagnostics for
troubleshooting. Keep aborts silent, escape dynamic details, and scope
translations to each request.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.

@qodo-code-review

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 59c2a93

@PeterDaveHello
PeterDaveHello merged commit 570fd43 into ChatGPTBox-dev:master Jul 18, 2026
4 checks passed
@PeterDaveHello
PeterDaveHello deleted the improve-fetch-request-errors branch July 18, 2026 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants