diff --git a/src/features/contributor-card.test.ts b/src/features/contributor-card.test.ts index c796c45..a040bc0 100644 --- a/src/features/contributor-card.test.ts +++ b/src/features/contributor-card.test.ts @@ -222,6 +222,74 @@ 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("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 eb21eb3..c76791f 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; } @@ -286,17 +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) => { + if (!data) { + // null = transient failure (see failedFetchAt note) — never cache it as + // permanent "no data". + onFailure(); + return; + } + // Success → cache and render. Clear any prior failure mark. fetching.delete(cacheKey); - infoCache.set(cacheKey, data ?? null); + failedFetchAt.delete(cacheKey); + infoCache.set(cacheKey, data); sync(); }) - .catch(() => { - fetching.delete(cacheKey); - // Don't leave a skeleton shimmering forever on a failed fetch. - card.container.querySelector(`.${PANEL_CLASS}[data-skeleton]`)?.remove(); - }); + .catch(onFailure); } let observer: MutationObserver | null = null; @@ -338,5 +372,6 @@ export function cleanupContributorCard(): void { } fetching.clear(); infoCache.clear(); + failedFetchAt.clear(); document.querySelectorAll(`.${PANEL_CLASS}`).forEach((el) => el.remove()); }