From c27a101c10b6fb8512e5392ab2f5e355c8445c41 Mon Sep 17 00:00:00 2001 From: Austen Stone Date: Thu, 20 Aug 2026 10:50:31 -0700 Subject: [PATCH] fix: stop re-requesting bot avatars that GitHub has no record of resolveBotAvatar only cached successful lookups, so a username the API returned 404 for was requested again on every dataset change. The in-flight map deduplicates concurrent calls but is cleared on settle, so nothing prevented the next pass from asking again. These calls are unauthenticated, which means 60 per hour per IP. A report containing bots that no longer resolve would spend that budget on repeats and then fail to load any avatars at all, including the ones that would have worked. Track unresolved usernames in memory and skip them in both resolveBotAvatar and the preloadBotAvatars filter. Deliberately not persisted: a bot created or renamed later still resolves after a reload, which a localStorage entry would prevent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3cf40db0-fafb-4793-92c6-3fe2b92d70bd --- src/lib/formatters.avatars.test.ts | 51 ++++++++++++++++++++++++++++++ src/lib/formatters.ts | 18 +++++++++-- 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/src/lib/formatters.avatars.test.ts b/src/lib/formatters.avatars.test.ts index 6ee2678..ae10c68 100644 --- a/src/lib/formatters.avatars.test.ts +++ b/src/lib/formatters.avatars.test.ts @@ -124,6 +124,57 @@ describe('bot avatar resolution', () => { expect(fetchMock).toHaveBeenCalledTimes(1); }); + it('does not re-request a bot the API had no avatar for', async () => { + const fetchMock = vi.fn().mockResolvedValue({ ok: false } as Response); + vi.stubGlobal('fetch', fetchMock); + const { resolveBotAvatar } = await freshFormatters(); + + await expect(resolveBotAvatar('gone[bot]')).resolves.toBeNull(); + await expect(resolveBotAvatar('gone[bot]')).resolves.toBeNull(); + await expect(resolveBotAvatar('gone[bot]')).resolves.toBeNull(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('does not re-request a bot whose lookup threw', async () => { + const fetchMock = vi.fn().mockRejectedValue(new Error('offline')); + vi.stubGlobal('fetch', fetchMock); + const { resolveBotAvatar } = await freshFormatters(); + + await expect(resolveBotAvatar('flaky[bot]')).resolves.toBeNull(); + await expect(resolveBotAvatar('flaky[bot]')).resolves.toBeNull(); + + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it('skips known-unresolvable bots on later preload passes', async () => { + const fetchMock = vi.fn().mockResolvedValue({ ok: false } as Response); + vi.stubGlobal('fetch', fetchMock); + const { preloadBotAvatars } = await freshFormatters(); + + await preloadBotAvatars(['a[bot]', 'b[bot]']); + expect(fetchMock).toHaveBeenCalledTimes(2); + + await preloadBotAvatars(['a[bot]', 'b[bot]']); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it('keeps failures out of storage so a reload retries them', async () => { + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false } as Response)); + const first = await freshFormatters(); + await first.resolveBotAvatar('later[bot]'); + expect(storage.getItem(AVATAR_STORAGE_KEY)).toBeNull(); + + const fetchMock = vi.fn().mockResolvedValue(okResponse('https://example.test/late.png')); + vi.stubGlobal('fetch', fetchMock); + const second = await freshFormatters(); + + await expect(second.resolveBotAvatar('later[bot]')).resolves.toBe( + 'https://example.test/late.png', + ); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + it('queues requests beyond the concurrency cap but still resolves them all', async () => { const fetchMock = vi.fn().mockResolvedValue(okResponse('https://example.test/c.png')); vi.stubGlobal('fetch', fetchMock); diff --git a/src/lib/formatters.ts b/src/lib/formatters.ts index 86b14aa..787abce 100644 --- a/src/lib/formatters.ts +++ b/src/lib/formatters.ts @@ -368,6 +368,12 @@ const HARDCODED_BOTS = new Set(botAvatarCache.keys()); /** In-flight fetch promises to avoid duplicate API calls */ const pendingFetches = new Map>(); +// Usernames the API had no avatar for. Kept in memory only, never persisted, so +// a reload retries a bot that has since been created or renamed. Without this +// every failed lookup repeats on each dataset change and exhausts the 60/hr +// unauthenticated rate limit, after which no avatars resolve at all. +const unresolvedAvatars = new Set(); + /** Simple throttle: max concurrent GitHub API requests */ const MAX_CONCURRENT_FETCHES = 3; let activeFetchCount = 0; @@ -407,6 +413,7 @@ function throttledFetch(url: string): Promise { */ export async function resolveBotAvatar(username: string): Promise { if (botAvatarCache.has(username)) return botAvatarCache.get(username)!; + if (unresolvedAvatars.has(username)) return null; if (!isBot(username)) return null; // Deduplicate in-flight requests @@ -423,10 +430,15 @@ export async function resolveBotAvatar(username: string): Promise if (url) { botAvatarCache.set(username, url); persistAvatarCache(); + } else { + unresolvedAvatars.add(username); } return url; }) - .catch(() => null) + .catch(() => { + unresolvedAvatars.add(username); + return null; + }) .finally(() => pendingFetches.delete(username)); pendingFetches.set(username, promise); @@ -439,7 +451,9 @@ export async function resolveBotAvatar(username: string): Promise * Caps at 10 API lookups per batch to avoid rate limits. */ export async function preloadBotAvatars(usernames: string[]): Promise { - const bots = usernames.filter((u) => isBot(u) && !botAvatarCache.has(u)); + const bots = usernames.filter( + (u) => isBot(u) && !botAvatarCache.has(u) && !unresolvedAvatars.has(u), + ); if (bots.length === 0) return false; // Cap lookups to avoid hammering the API with many unknown bots const batch = bots.slice(0, 10);