diff --git a/src/context/AppContext.jsx b/src/context/AppContext.jsx index a013159..66bd3c1 100644 --- a/src/context/AppContext.jsx +++ b/src/context/AppContext.jsx @@ -92,7 +92,7 @@ export function AppProvider({ children }) { try { setLoadMsg('Fetching organization metadata...') const orgRes = await Promise.allSettled(orgNames.map(n => fetchOrg(n, pat))) - const validOrgs = orgRes.filter(r => r.status === 'fulfilled').map(r => r.value) + const validOrgs = orgRes.filter(r => r.status === 'fulfilled' && r.value).map(r => r.value) if (!validOrgs.length) throw new Error('No valid organizations found. Check the names and try again.') setOrgs(validOrgs) diff --git a/src/services/github.js b/src/services/github.js index a4180fa..c14d8da 100644 --- a/src/services/github.js +++ b/src/services/github.js @@ -77,61 +77,82 @@ async function fetchWithCache(url, pat) { if (res.status === 404) throw new Error('NOT_FOUND') if (!res.ok) throw new Error(`HTTP_${res.status}`) - const data = await res.json() + // GitHub answers 204 No Content for some endpoints (e.g. /contributors on a + // repo with no commits). res.json() throws on an empty body, so read the text + // first and let callers deal with a null payload. + const text = await res.text() + if (!text) return null + + let data + try { + data = JSON.parse(text) + } catch { + // A truncated or non-JSON body (a proxy error page, a cut-off response) + // should not reject and discard pages already collected. + return null + } + cacheSet(url, data) // write-back, non-blocking return data } -// Public service functions -export const fetchOrg = (org, pat) => - fetchWithCache(`https://api.github.com/orgs/${org}`, pat) - -export async function fetchRepos(org, repoCount, pat) { +/** + * Walks GitHub's page-based pagination, collecting every item until a short + * page arrives or maxPages is reached. + * + * A page that is not an array (204 No Content, an error envelope such as + * `{ message: 'Moved Permanently' }`, or a malformed body) ends the walk and + * whatever was collected so far is returned, rather than throwing and losing + * the earlier pages. + */ +async function fetchPaginated(buildUrl, maxPages, pat) { const all = [] - const maxPages = pat ? Math.ceil(repoCount / 100) : 5 + for (let page = 1; page <= maxPages; page++) { - const url = `https://api.github.com/orgs/${org}/repos?per_page=100&page=${page}&sort=updated` - const data = await fetchWithCache(url, pat) + const data = await fetchWithCache(buildUrl(page), pat) + if (!Array.isArray(data)) break + all.push(...data) if (data.length < 100) break } + return all } +// Public service functions +export const fetchOrg = (org, pat) => + fetchWithCache(`https://api.github.com/orgs/${org}`, pat) + +export async function fetchRepos(org, repoCount, pat) { + return fetchPaginated( + page => `https://api.github.com/orgs/${org}/repos?per_page=100&page=${page}&sort=updated`, + pat ? Math.ceil(repoCount / 100) : 5, + pat + ) +} + export async function fetchContributors(org, repo, pat) { - const all = [] - const maxPages = pat ? 10 : 1 - for(let page = 1; page<=maxPages ; page++) { - const url = `https://api.github.com/repos/${org}/${repo}/contributors?per_page=100&page=${page}` - const data = await fetchWithCache(url, pat) - all.push(...data) - if(data.length < 100) break - } - return all + return fetchPaginated( + page => `https://api.github.com/repos/${org}/${repo}/contributors?per_page=100&page=${page}`, + pat ? 10 : 1, + pat + ) } export async function fetchIssues(org, repo, pat) { - const all = [] - const maxPages = pat ? 10 : 1 - for(let page = 1; page<=maxPages ; page++) { - const url = `https://api.github.com/repos/${org}/${repo}/issues?state=all&per_page=100&page=${page}` - const data = await fetchWithCache(url, pat) - all.push(...data) - if(data.length < 100) break - } - return all + return fetchPaginated( + page => `https://api.github.com/repos/${org}/${repo}/issues?state=all&per_page=100&page=${page}`, + pat ? 10 : 1, + pat + ) } export async function fetchPulls(org, repo, pat) { - const all = [] - const maxPages = pat ? 10 : 1 - for(let page = 1; page<=maxPages ; page++) { - const url = `https://api.github.com/repos/${org}/${repo}/pulls?state=all&per_page=100&page=${page}` - const data = await fetchWithCache(url, pat) - all.push(...data) - if(data.length < 100) break - } - return all + return fetchPaginated( + page => `https://api.github.com/repos/${org}/${repo}/pulls?state=all&per_page=100&page=${page}`, + pat ? 10 : 1, + pat + ) } export async function fetchRateLimit(pat) { diff --git a/src/services/github.test.js b/src/services/github.test.js new file mode 100644 index 0000000..419fcca --- /dev/null +++ b/src/services/github.test.js @@ -0,0 +1,161 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' +import { fetchContributors, fetchIssues, fetchPulls, fetchRepos } from './github' + +// The service writes through to IndexedDB, which jsdom does not implement. +// Every cache helper already swallows its own errors, so a stub that always +// rejects exercises the real cache-miss path without touching storage. +const failingIndexedDB = { + open: () => { + const req = {} + queueMicrotask(() => req.onerror?.()) + return req + }, +} + +function jsonResponse(body, { status = 200, headers = {} } = {}) { + return { + ok: status >= 200 && status < 300, + status, + headers: { get: k => headers[k] ?? null }, + text: async () => JSON.stringify(body), + } +} + +/** A body that is not JSON at all, e.g. a proxy error page or a truncated response. */ +function textResponse(body, { status = 200 } = {}) { + return { + ok: status >= 200 && status < 300, + status, + headers: { get: () => null }, + text: async () => body, + } +} + +/** GitHub answers 204 No Content for repos with no contributors; the body is empty. */ +function noContentResponse() { + return { + ok: true, + status: 204, + headers: { get: () => null }, + text: async () => '', + } +} + +beforeEach(() => { + vi.stubGlobal('indexedDB', failingIndexedDB) + vi.stubGlobal('fetch', vi.fn()) +}) + +afterEach(() => { + vi.unstubAllGlobals() + vi.restoreAllMocks() +}) + +describe('paginated fetchers: response validation', () => { + it('returns an empty list when GitHub answers 204 No Content', async () => { + // A repo with no commits yet returns 204 from /contributors. Before the + // guard, res.json() threw and the whole fetch rejected, so explore()'s + // Promise.allSettled dropped the repo from the contributor map silently. + fetch.mockResolvedValue(noContentResponse()) + + await expect(fetchContributors('AOSSIE-Org', 'EmptyRepo', 'pat')).resolves.toEqual([]) + }) + + it('returns an empty list when the payload is an object rather than an array', async () => { + // Any non-array body used to reach `all.push(...data)` and throw + // "TypeError: data is not iterable". + fetch.mockResolvedValue(jsonResponse({ message: 'Moved Permanently' })) + + await expect(fetchIssues('AOSSIE-Org', 'Renamed', 'pat')).resolves.toEqual([]) + }) + + it('returns an empty list when the payload is null', async () => { + fetch.mockResolvedValue(jsonResponse(null)) + + await expect(fetchPulls('AOSSIE-Org', 'Whatever', 'pat')).resolves.toEqual([]) + }) + + it('returns an empty list when the body is not valid JSON', async () => { + // A proxy error page or a truncated response reaches JSON.parse, which + // threw before the try/catch and rejected the whole fetch. + fetch.mockResolvedValue(textResponse('502 Bad Gateway')) + + await expect(fetchIssues('AOSSIE-Org', 'Proxied', 'pat')).resolves.toEqual([]) + }) + + it('keeps the pages collected before an unparseable page appears', async () => { + // Same guarantee as the malformed-object case, but for a body that cannot + // be parsed at all rather than one that parses to a non-array. + const fullPage = Array.from({ length: 100 }, (_, i) => ({ id: i })) + + fetch + .mockResolvedValueOnce(jsonResponse(fullPage)) + .mockResolvedValueOnce(textResponse('{ truncated')) + + await expect(fetchContributors('AOSSIE-Org', 'Cut', 'pat')).resolves.toHaveLength(100) + // Without this the test would also pass if pagination stopped after page 1 + // and the unparseable page was never requested at all. + expect(fetch).toHaveBeenCalledTimes(2) + }) + + it('keeps the pages collected before a malformed page appears', async () => { + // A full first page must still count even if page 2 comes back malformed. + const fullPage = Array.from({ length: 100 }, (_, i) => ({ id: i })) + + fetch + .mockResolvedValueOnce(jsonResponse(fullPage)) + .mockResolvedValueOnce(jsonResponse({ message: 'Server Error' })) + + await expect(fetchContributors('AOSSIE-Org', 'Big', 'pat')).resolves.toHaveLength(100) + expect(fetch).toHaveBeenCalledTimes(2) + }) + + it('stops paginating as soon as a malformed page is returned', async () => { + fetch.mockResolvedValue(jsonResponse({ message: 'Server Error' })) + + await fetchIssues('AOSSIE-Org', 'Broken', 'pat') + + // maxPages is 10 for PAT users; without the break it would burn all ten. + expect(fetch).toHaveBeenCalledTimes(1) + }) +}) + +describe('paginated fetchers: happy path', () => { + it('stops at the first partial page', async () => { + fetch.mockResolvedValueOnce(jsonResponse([{ id: 1 }, { id: 2 }])) + + await expect(fetchContributors('AOSSIE-Org', 'Small', 'pat')).resolves.toHaveLength(2) + expect(fetch).toHaveBeenCalledTimes(1) + }) + + it('stops at the ten page ceiling for a PAT request', async () => { + // Every page comes back full, so only maxPages can end the walk. This is + // the bound issue #103 asks for: without it the loop would follow GitHub's + // pagination indefinitely. + const fullPage = Array.from({ length: 100 }, (_, i) => ({ id: i })) + fetch.mockResolvedValue(jsonResponse(fullPage)) + + await expect(fetchIssues('AOSSIE-Org', 'Huge', 'pat')).resolves.toHaveLength(1000) + expect(fetch).toHaveBeenCalledTimes(10) + }) + + it('stops after a single page when no PAT is supplied', async () => { + // maxPages is `pat ? 10 : 1`, so an anonymous caller must not walk on. + const fullPage = Array.from({ length: 100 }, (_, i) => ({ id: i })) + fetch.mockResolvedValue(jsonResponse(fullPage)) + + await expect(fetchIssues('AOSSIE-Org', 'Huge')).resolves.toHaveLength(100) + expect(fetch).toHaveBeenCalledTimes(1) + }) + + it('follows pagination while pages come back full', async () => { + const fullPage = Array.from({ length: 100 }, (_, i) => ({ id: i })) + + fetch + .mockResolvedValueOnce(jsonResponse(fullPage)) + .mockResolvedValueOnce(jsonResponse([{ id: 100 }])) + + await expect(fetchRepos('AOSSIE-Org', 150, 'pat')).resolves.toHaveLength(101) + expect(fetch).toHaveBeenCalledTimes(2) + }) +})