fix(extensions): handle marketplace outages safely#743
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMarketplace HTTP failures now use a shared formatter that hides non-JSON bodies, extracts short JSON error details, standardizes 5xx messages, and is covered by tests for HTML, JSON, and plain-text responses. ChangesMarketplace error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
electron/extensions/errorUtils.test.ts (1)
5-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests cover the primary paths; consider adding coverage for
messagefallback and truncation.The suite validates the three main scenarios (5xx HTML suppression, JSON
errorextraction, non-JSON suppression). Two gaps worth covering:
messagefield fallback — whenerroris absent butmessageis present, the formatter should extractmessage. This path (typeof error === "string" ? error : message) is untested.- Truncation — a JSON error detail longer than 200 characters should be bounded. No test verifies the truncation boundary.
🤖 Prompt for 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. In `@electron/extensions/errorUtils.test.ts` around lines 5 - 40, Extend the formatMarketplaceHttpError test suite with a case where JSON omits error but includes a string message, asserting that message is used; add another case with a JSON error detail longer than 200 characters, asserting the returned detail is truncated to the formatter’s 200-character limit.electron/extensions/errorUtils.ts (1)
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a truncation indicator.
When
detailexceeds 200 characters,.slice(0, MAX_MARKETPLACE_ERROR_DETAIL_LENGTH)silently cuts the string mid-word (or mid-character for surrogate pairs), and the user has no signal that content was elided. A trailing ellipsis when truncation occurs would improve clarity.♻️ Optional refactor
if (typeof value === "string" && value.trim()) { - detail = value - .trim() - .replace(/\s+/g, " ") - .slice(0, MAX_MARKETPLACE_ERROR_DETAIL_LENGTH); + const normalized = value.trim().replace(/\s+/g, " "); + detail = + normalized.length > MAX_MARKETPLACE_ERROR_DETAIL_LENGTH + ? normalized.slice(0, MAX_MARKETPLACE_ERROR_DETAIL_LENGTH - 1) + "…" + : normalized; }🤖 Prompt for 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. In `@electron/extensions/errorUtils.ts` around lines 28 - 31, Update the detail normalization logic to detect when the trimmed, whitespace-collapsed value exceeds MAX_MARKETPLACE_ERROR_DETAIL_LENGTH, then truncate it while reserving space for a trailing ellipsis so the final value stays within the limit. Preserve the existing full value when no truncation is needed and avoid splitting surrogate pairs if possible.
🤖 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.
Nitpick comments:
In `@electron/extensions/errorUtils.test.ts`:
- Around line 5-40: Extend the formatMarketplaceHttpError test suite with a case
where JSON omits error but includes a string message, asserting that message is
used; add another case with a JSON error detail longer than 200 characters,
asserting the returned detail is truncated to the formatter’s 200-character
limit.
In `@electron/extensions/errorUtils.ts`:
- Around line 28-31: Update the detail normalization logic to detect when the
trimmed, whitespace-collapsed value exceeds MAX_MARKETPLACE_ERROR_DETAIL_LENGTH,
then truncate it while reserving space for a trailing ellipsis so the final
value stays within the limit. Preserve the existing full value when no
truncation is needed and avoid splitting surrogate pairs if possible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8befa6f4-a2a1-4bc0-820c-e538cc717e02
📒 Files selected for processing (3)
electron/extensions/errorUtils.test.tselectron/extensions/errorUtils.tselectron/extensions/extensionMarketplace.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
electron/extensions/errorUtils.test.ts (1)
31-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTests correctly validate the JSON fallback and Unicode-safe truncation contract.
The new
message-fallback and truncation tests align precisely with the upstream formatter logic inerrorUtils.ts—errortakes string precedence,messageis the fallback, and truncation slices byArray.fromcodepoints before appending the ellipsis. The length invariant check at line 50 is a nice touch.Two optional edge cases that would strengthen coverage:
- Malformed JSON with
application/jsoncontent type — exercises thecatchbranch so a regression that leaks raw body text is caught.- Both
errorandmessagepresent — confirmserrorwins when it's a string (currently implicit).🤖 Prompt for 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. In `@electron/extensions/errorUtils.test.ts` around lines 31 - 51, Strengthen the tests around formatMarketplaceHttpError by adding coverage for malformed JSON with an application/json content type, asserting the catch-path fallback does not expose raw body text, and for JSON containing both error and message fields, asserting the string error value takes precedence.
🤖 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.
Nitpick comments:
In `@electron/extensions/errorUtils.test.ts`:
- Around line 31-51: Strengthen the tests around formatMarketplaceHttpError by
adding coverage for malformed JSON with an application/json content type,
asserting the catch-path fallback does not expose raw body text, and for JSON
containing both error and message fields, asserting the string error value takes
precedence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bbb3ad75-f0d1-465b-9489-a6c700453b0f
📒 Files selected for processing (2)
electron/extensions/errorUtils.test.tselectron/extensions/errorUtils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- electron/extensions/errorUtils.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Description
Handle failed Marketplace responses without sending an upstream HTML error page into the Extensions UI. Server-side 5xx responses now produce a concise retry message; 4xx JSON errors retain only a bounded
errorormessagedetail, while non-JSON bodies are suppressed.Motivation
Related to #735.
The production Marketplace currently returns Cloudflare HTTP 525 with a 7,122-byte HTML page. The main process interpolated that page verbatim into the IPC error, producing the overflowing panel shown in the report. Node and Electron reproduce the same response, so this client symptom is unrelated to GPU availability.
Cloudflare 525 means its TLS handshake with the origin failed. This PR improves client behavior but intentionally does not claim to restore the service; the Marketplace operator must repair the origin TLS configuration separately.
Type of Change
Testing Guide
npx vitest --run electron/extensions/errorUtils.test.ts— 3/3 passed after a 3/3 red phase.npx vitest --run --silent=passed-only— 97 files, 844 tests passed.Marketplace is temporarily unavailable (HTTP 525). Please try again later.Strict TypeScript and repository-wide formatting retain current
mainbaseline failures already addressed by draft #737; the changed files pass their focused gates.Checklist
Summary by CodeRabbit
errorovermessage), with whitespace normalized and long details truncated safely.