From ec9f92f432c4a91d275915ae30d72f8a69ca5467 Mon Sep 17 00:00:00 2001 From: Test Date: Mon, 12 Oct 2026 22:37:57 +0200 Subject: [PATCH 1/2] fix(website): retry skill detail fetch and show a clear error (#737) A network-level failure while loading the per-skill detail JSON ("Failed to fetch") left the detail page stuck on a raw error with no way to recover short of navigating away. SkillDetail now retries network errors and 5xx/408/429 responses with a short backoff, aborts in-flight work on unmount, and on a final failure shows a plain-language message with a Try again button while the summary and install command stay usable. --- .../src/__tests__/skill-detail.test.jsx | 134 +++++++++++++++++- website-src/src/components/SkillDetail.jsx | 80 +++++++++-- 2 files changed, 203 insertions(+), 11 deletions(-) diff --git a/website-src/src/__tests__/skill-detail.test.jsx b/website-src/src/__tests__/skill-detail.test.jsx index de970693..44e1af64 100644 --- a/website-src/src/__tests__/skill-detail.test.jsx +++ b/website-src/src/__tests__/skill-detail.test.jsx @@ -1,6 +1,12 @@ /** @vitest-environment jsdom */ -import { afterEach, describe, expect, it } from "vitest"; -import { cleanup, render, screen } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + act, + cleanup, + fireEvent, + render, + screen, +} from "@testing-library/react"; import { HashRouter } from "react-router-dom"; import SkillDetail from "../components/SkillDetail.jsx"; @@ -183,3 +189,127 @@ describe("SkillDetail — repo detail link (issue #623)", () => { expect(external.getAttribute("href")).toBe("https://github.com/test/repo"); }); }); + +describe("SkillDetail — full-details fetch resilience (issue #737)", () => { + // detailCache is module-level, so every test uses its own detailPath. + let seq = 0; + const nextSkill = () => ({ + ...baseSkill, + detailPath: `skills/issue-737-${++seq}.json`, + }); + const fullDetail = (slim) => ({ + ...slim, + description: "Full description loaded from the detail file", + }); + const okResponse = (body) => ({ + ok: true, + status: 200, + json: async () => body, + }); + const flush = (ms = 0) => act(() => vi.advanceTimersByTimeAsync(ms)); + + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + cleanup(); + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + it("retries a transient network failure and then shows the full details", async () => { + const slim = nextSkill(); + const fetchMock = vi + .fn() + .mockRejectedValueOnce(new TypeError("Failed to fetch")) + .mockResolvedValueOnce(okResponse(fullDetail(slim))); + vi.stubGlobal("fetch", fetchMock); + + renderDetail({ slim }); + await flush(5000); + + expect(fetchMock).toHaveBeenCalledTimes(2); + expect( + screen.getByText("Full description loaded from the detail file"), + ).toBeTruthy(); + expect(screen.queryByText(/Could not load full details/)).toBeNull(); + expect(screen.queryByText(/Failed to fetch/)).toBeNull(); + }); + + it("shows a clear message and a retry button when the network stays down", async () => { + const slim = nextSkill(); + const fetchMock = vi + .fn() + .mockRejectedValue(new TypeError("Failed to fetch")); + vi.stubGlobal("fetch", fetchMock); + + renderDetail({ slim }); + await flush(10000); + + const alert = screen.getByRole("alert"); + expect(alert.textContent).toMatch(/Could not load full details/); + expect(alert.textContent).toMatch(/connection/i); + expect(alert.textContent).not.toMatch(/Failed to fetch/); + // Summary and install command stay usable. + expect(screen.getByText("A test skill for unit testing")).toBeTruthy(); + expect( + screen.getByText("asm install github:test/repo:skills/test-skill"), + ).toBeTruthy(); + + const callsBefore = fetchMock.mock.calls.length; + fetchMock.mockResolvedValue(okResponse(fullDetail(slim))); + fireEvent.click(screen.getByRole("button", { name: /try again/i })); + await flush(5000); + + expect(fetchMock.mock.calls.length).toBe(callsBefore + 1); + expect(screen.queryByRole("alert")).toBeNull(); + expect( + screen.getByText("Full description loaded from the detail file"), + ).toBeTruthy(); + }); + + it("does not retry a 404", async () => { + const slim = nextSkill(); + const fetchMock = vi + .fn() + .mockResolvedValue({ ok: false, status: 404, json: async () => ({}) }); + vi.stubGlobal("fetch", fetchMock); + + renderDetail({ slim }); + await flush(10000); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(screen.getByRole("alert").textContent).toMatch(/HTTP 404/); + }); + + it("retries a 503 before giving up", async () => { + const slim = nextSkill(); + const fetchMock = vi + .fn() + .mockResolvedValueOnce({ ok: false, status: 503, json: async () => ({}) }) + .mockResolvedValueOnce(okResponse(fullDetail(slim))); + vi.stubGlobal("fetch", fetchMock); + + renderDetail({ slim }); + await flush(5000); + + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(screen.queryByRole("alert")).toBeNull(); + }); + + it("stops retrying once the component unmounts", async () => { + const slim = nextSkill(); + const fetchMock = vi + .fn() + .mockRejectedValue(new TypeError("Failed to fetch")); + vi.stubGlobal("fetch", fetchMock); + + const { unmount } = renderDetail({ slim }); + await flush(0); + unmount(); + await flush(10000); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/website-src/src/components/SkillDetail.jsx b/website-src/src/components/SkillDetail.jsx index a8deea74..905b1ca4 100644 --- a/website-src/src/components/SkillDetail.jsx +++ b/website-src/src/components/SkillDetail.jsx @@ -18,17 +18,54 @@ import { Card } from "./ui/card.jsx"; * detailPath eliminates redundant fetches when navigating between * skills and back. * + * A network failure ("Failed to fetch") or a 5xx/408/429 response is + * retried with a short backoff before giving up (issue #737). If the + * details still cannot be loaded, a plain-language message with a + * "Try again" button is shown; the slim row keeps the summary and the + * install command usable either way. + * * Props: * - slim: the slim row from catalog.skills (required) */ const detailCache = new Map(); +/** Backoff before each automatic retry; its length is the retry count. */ +const RETRY_DELAYS_MS = [500, 1500]; + +/** Error raised for a non-2xx response, flagged when worth retrying. */ +class DetailHttpError extends Error { + constructor(status) { + super("HTTP " + status); + this.retryable = status >= 500 || status === 408 || status === 429; + } +} + +/** + * `fetch` rejects with a TypeError ("Failed to fetch" in Chromium) for + * any network-level failure: offline, DNS, reset connection, or a + * service worker answering with Response.error(). + */ +function isRetryable(err) { + return err instanceof TypeError || err?.retryable === true; +} + +function describeDetailError(err) { + if (err instanceof TypeError) { + return "The network request failed. Check your connection and try again."; + } + if (err instanceof DetailHttpError) { + return `The server responded with ${err.message}.`; + } + return "The details file could not be read."; +} + export default function SkillDetail({ slim }) { const [detail, setDetail] = useState({ data: null, loading: true, error: null, }); + const [reloadKey, setReloadKey] = useState(0); useEffect(() => { if (!slim?.detailPath) { @@ -46,11 +83,13 @@ export default function SkillDetail({ slim }) { } let cancelled = false; + let retryTimer = null; + const controller = new AbortController(); setDetail({ data: null, loading: true, error: null }); - (async () => { + const load = async (attempt) => { try { - const res = await fetch(path); - if (!res.ok) throw new Error("HTTP " + res.status); + const res = await fetch(path, { signal: controller.signal }); + if (!res.ok) throw new DetailHttpError(res.status); const data = await res.json(); if (cancelled) return; // Store in cache @@ -58,17 +97,27 @@ export default function SkillDetail({ slim }) { setDetail({ data, loading: false, error: null }); } catch (err) { if (cancelled) return; + if (isRetryable(err) && attempt < RETRY_DELAYS_MS.length) { + retryTimer = setTimeout( + () => load(attempt + 1), + RETRY_DELAYS_MS[attempt], + ); + return; + } setDetail({ data: null, loading: false, - error: err instanceof Error ? err.message : String(err), + error: describeDetailError(err), }); } - })(); + }; + load(0); return () => { cancelled = true; + clearTimeout(retryTimer); + controller.abort(); }; - }, [slim?.detailPath]); + }, [slim?.detailPath, reloadKey]); const skill = useMemo(() => detail.data || slim, [detail.data, slim]); if (!skill) return null; @@ -224,9 +273,22 @@ export default function SkillDetail({ slim }) { {detail.loading &&

Loading details…

} {detail.error && ( -

- ⚠ Could not load full details: {detail.error} -

+
+ + ⚠ Could not load full details. {detail.error} The summary and + install command above still work. + + +
)} ); From 21ad112082d2fd5468268b308ef909980d2e931c Mon Sep 17 00:00:00 2001 From: Test Date: Mon, 12 Oct 2026 22:42:28 +0200 Subject: [PATCH 2/2] fix(website): address review feedback (#737) Give the Try again button a 44px hit area (min-h-11) and render the alert text in --fg inside the warn-bg/warn-border box, keeping --warn only for the icon and border so light-theme contrast passes WCAG AA. --- website-src/src/components/SkillDetail.jsx | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/website-src/src/components/SkillDetail.jsx b/website-src/src/components/SkillDetail.jsx index 905b1ca4..b078808d 100644 --- a/website-src/src/components/SkillDetail.jsx +++ b/website-src/src/components/SkillDetail.jsx @@ -275,15 +275,18 @@ export default function SkillDetail({ slim }) { {detail.error && (
- ⚠ Could not load full details. {detail.error} The summary and - install command above still work. + {" "} + Could not load full details. {detail.error} The summary and install + command above still work.