From ca72357cc8dba5a8846ffb0825208991535644b8 Mon Sep 17 00:00:00 2001 From: shawn Date: Wed, 24 Jun 2026 00:47:02 +0800 Subject: [PATCH 1/2] fix(contributor-card): retry failed fetches instead of hiding the card forever MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fetchContributorInfo returns null on ANY error (rate limit, network blip, service-worker hiccup). sync() cached that null in infoCache, and the cache-hit branch renders nothing for a null entry and never refetches — so a single transient failure hid the contributor card for that user for the rest of the session (until a full page reload). This looked page-specific (e.g. 'works on the PR detail page, not the PR list') but was really just whichever hover happened to poison the cache first. Now only successful fetches are cached. A failure is recorded in failedFetchAt with a timestamp and retried after a 60s cooldown (and on any fresh load, since the maps are in-memory). The cooldown keeps a busy hovercard from re-fetching on every DOM re-render while still recovering on a later hover. cleanupContributorCard clears the new map too. Verified live: with the cache cleared, the card fetches and renders fine on the PR list page — confirming the fetch itself works and the poisoned null cache was the cause. Added a regression test (retry after cooldown, no refetch storm within it). --- src/features/contributor-card.test.ts | 30 ++++++++++++++++++++++ src/features/contributor-card.ts | 37 +++++++++++++++++++++++---- 2 files changed, 62 insertions(+), 5 deletions(-) diff --git a/src/features/contributor-card.test.ts b/src/features/contributor-card.test.ts index c796c45..574906b 100644 --- a/src/features/contributor-card.test.ts +++ b/src/features/contributor-card.test.ts @@ -222,6 +222,36 @@ describe("injectContributorCard", () => { expect(document.querySelector(BLOCK)).toBeNull(); }); + it("retries a failed fetch on a later hover instead of suppressing the card forever", async () => { + // fetchContributorInfo returns null on ANY error (rate limit, network blip). + // Caching that as permanent "no data" hid the card for the rest of the + // session after a single transient failure. It must retry — but not storm + // the network on every hovercard re-render, so a short cooldown gates it. + fetchContributorInfo.mockResolvedValueOnce(null); // first attempt fails + injectContributorCard(); + const card = hovercard("octocat"); + document.body.appendChild(card); + await flush(); + + expect(document.querySelector(BLOCK)).toBeNull(); // failed → nothing shown + expect(fetchContributorInfo).toHaveBeenCalledTimes(1); + + // Re-hover within the cooldown: no refetch (no storm), still nothing. + card.querySelector(".content")!.appendChild(document.createElement("span")); + await flush(); + expect(fetchContributorInfo).toHaveBeenCalledTimes(1); + expect(document.querySelector(BLOCK)).toBeNull(); + + // Past the cooldown, a hover retries — and this time the fetch succeeds. + fetchContributorInfo.mockResolvedValue(baseInfo()); + vi.setSystemTime(new Date(NOW.getTime() + 61_000)); + card.querySelector(".content")!.appendChild(document.createElement("span")); + await flush(); + + expect(fetchContributorInfo).toHaveBeenCalledTimes(2); // retried, not stuck + expect(document.querySelector(BLOCK)?.textContent).toContain("3 days"); + }); + it("shows a loading skeleton, then fills it in when data arrives", async () => { // The panel is anchored in the stable popover root (not inside the body // GitHub re-renders), so a skeleton→data swap is safe — it lets the facts diff --git a/src/features/contributor-card.ts b/src/features/contributor-card.ts index eb21eb3..3826ce1 100644 --- a/src/features/contributor-card.ts +++ b/src/features/contributor-card.ts @@ -204,8 +204,16 @@ function buildSkeleton(login: string): HTMLElement { return panel; } -const infoCache = new Map(); +// Successful fetches only — keyed by repo#login, persists across navigations. +const infoCache = new Map(); const fetching = new Set(); +// Logins whose last fetch FAILED, with when it failed. A failure must NOT be +// cached as a permanent "no data": fetchContributorInfo returns null on any +// error (rate limit, network blip), and caching that forever hid the card for +// the rest of the session after a single transient failure. Instead we retry +// after a short cooldown (and on any fresh page load, since this is in-memory). +const failedFetchAt = new Map(); +const FAILED_FETCH_RETRY_MS = 60_000; interface Card { container: HTMLElement; @@ -272,8 +280,16 @@ function sync(): void { const cacheKey = `${repo?.owner ?? ""}/${repo?.repo ?? ""}#${login}`; if (infoCache.has(cacheKey)) { existing?.remove(); - const info = infoCache.get(cacheKey); - if (info) placePanel(card, buildPanel(login, info)); + placePanel(card, buildPanel(login, infoCache.get(cacheKey)!)); + return; + } + + // Fetch failed recently → don't re-shimmer or refetch on every hovercard + // re-render (which would storm the network); just show nothing for now. The + // cooldown lets a later hover retry instead of the card being stuck off. + const failedAt = failedFetchAt.get(cacheKey); + if (failedAt !== undefined && Date.now() - failedAt < FAILED_FETCH_RETRY_MS) { + existing?.remove(); return; } @@ -289,11 +305,21 @@ function sync(): void { fetchContributorInfo(login, repo?.owner, repo?.repo) .then((data) => { fetching.delete(cacheKey); - infoCache.set(cacheKey, data ?? null); - sync(); + if (data) { + // Success → cache and render. Clear any prior failure mark. + failedFetchAt.delete(cacheKey); + infoCache.set(cacheKey, data); + sync(); + } else { + // null = transient failure (see failedFetchAt note). Mark for retry and + // drop the skeleton — never cache it as permanent "no data". + failedFetchAt.set(cacheKey, Date.now()); + card.container.querySelector(`.${PANEL_CLASS}[data-skeleton]`)?.remove(); + } }) .catch(() => { fetching.delete(cacheKey); + failedFetchAt.set(cacheKey, Date.now()); // Don't leave a skeleton shimmering forever on a failed fetch. card.container.querySelector(`.${PANEL_CLASS}[data-skeleton]`)?.remove(); }); @@ -338,5 +364,6 @@ export function cleanupContributorCard(): void { } fetching.clear(); infoCache.clear(); + failedFetchAt.clear(); document.querySelectorAll(`.${PANEL_CLASS}`).forEach((el) => el.remove()); } From a511f0c53d03cee302942f12b8f23a66f24e2670 Mon Sep 17 00:00:00 2001 From: shawn Date: Wed, 24 Jun 2026 09:38:21 +0800 Subject: [PATCH 2/2] fix(contributor-card): scope failed-fetch skeleton removal to its login A failed fetch removed the skeleton via a login-unscoped selector against the captured container. If the user hovered past to another user before the slow fetch resolved, the failure callback yanked the *new* user's in-flight skeleton. Scope the removal by data-login so a since-passed fetch can't blank the card now loading a different user. Also collapse the now-identical null-return and thrown-error paths into one onFailure helper, so the two failure branches can't drift. Add a regression test asserting node identity (a later sync() re-adds a different skeleton, masking the bug if you only assert presence). --- src/features/contributor-card.test.ts | 38 ++++++++++++++++++++++++ src/features/contributor-card.ts | 42 ++++++++++++++++----------- 2 files changed, 63 insertions(+), 17 deletions(-) diff --git a/src/features/contributor-card.test.ts b/src/features/contributor-card.test.ts index 574906b..a040bc0 100644 --- a/src/features/contributor-card.test.ts +++ b/src/features/contributor-card.test.ts @@ -252,6 +252,44 @@ describe("injectContributorCard", () => { expect(document.querySelector(BLOCK)?.textContent).toContain("3 days"); }); + it("a stale failed fetch doesn't blank the card now showing a different user", async () => { + // Hovercards are reused: hovering A then B swaps the same container's payload. + // When A's slow fetch finally fails, it must drop only A's skeleton — the + // removal is scoped by login, so a since-passed fetch can't blank the card + // that is now loading B. + const resolvers: Record void> = {}; + fetchContributorInfo.mockImplementation( + (login) => new Promise((r) => (resolvers[login] = r)), + ); + injectContributorCard(); + + const card = hovercard("alice"); + document.body.appendChild(card); + await flush(); + expect(document.querySelector(BLOCK)?.dataset.login).toBe("alice"); + + // Move to bob on the same reused hovercard (GitHub swaps the payload in place, + // which our observer sees as a childList mutation under the content node). + const view = card.querySelector("[data-hydro-view]")!; + view.setAttribute( + "data-hydro-view", + JSON.stringify({ event_type: "user-hovercard-hover", payload: { card_user_login: "bob" } }), + ); + card.querySelector(".content")!.appendChild(document.createElement("span")); + await flush(); + const bobSkeleton = document.querySelector(`${BLOCK}[data-login="bob"]`); + expect(bobSkeleton?.dataset.skeleton).toBe("true"); + + // alice's fetch resolves null last — must NOT touch bob's in-flight skeleton. + // Assert on node identity, not presence: an unscoped removal would yank this + // exact node out (a later sync() re-adds a *different* one, masking the bug), + // so `isConnected` is what proves bob's skeleton was left alone. + resolvers["alice"](null); + await flush(); + + expect(bobSkeleton!.isConnected).toBe(true); // bob's own skeleton untouched + }); + it("shows a loading skeleton, then fills it in when data arrives", async () => { // The panel is anchored in the stable popover root (not inside the body // GitHub re-renders), so a skeleton→data swap is safe — it lets the facts diff --git a/src/features/contributor-card.ts b/src/features/contributor-card.ts index 3826ce1..c76791f 100644 --- a/src/features/contributor-card.ts +++ b/src/features/contributor-card.ts @@ -302,27 +302,35 @@ function sync(): void { } if (fetching.has(cacheKey)) return; fetching.add(cacheKey); + + // A null return and a thrown error are the same outcome — fetchContributorInfo + // returns null on any error (rate limit, network blip). Treat both as a + // transient failure: clear the in-flight mark, schedule a retry after the + // cooldown, and drop *this login's* skeleton. Scope the removal by login so a + // slow fetch for a user we've since hovered past doesn't blank the card that + // now shows a different user. + const onFailure = (): void => { + fetching.delete(cacheKey); + failedFetchAt.set(cacheKey, Date.now()); + card.container + .querySelector(`.${PANEL_CLASS}[data-skeleton][data-login="${login}"]`) + ?.remove(); + }; fetchContributorInfo(login, repo?.owner, repo?.repo) .then((data) => { - fetching.delete(cacheKey); - if (data) { - // Success → cache and render. Clear any prior failure mark. - failedFetchAt.delete(cacheKey); - infoCache.set(cacheKey, data); - sync(); - } else { - // null = transient failure (see failedFetchAt note). Mark for retry and - // drop the skeleton — never cache it as permanent "no data". - failedFetchAt.set(cacheKey, Date.now()); - card.container.querySelector(`.${PANEL_CLASS}[data-skeleton]`)?.remove(); + if (!data) { + // null = transient failure (see failedFetchAt note) — never cache it as + // permanent "no data". + onFailure(); + return; } - }) - .catch(() => { + // Success → cache and render. Clear any prior failure mark. fetching.delete(cacheKey); - failedFetchAt.set(cacheKey, Date.now()); - // Don't leave a skeleton shimmering forever on a failed fetch. - card.container.querySelector(`.${PANEL_CLASS}[data-skeleton]`)?.remove(); - }); + failedFetchAt.delete(cacheKey); + infoCache.set(cacheKey, data); + sync(); + }) + .catch(onFailure); } let observer: MutationObserver | null = null;