From cd4ea0fbb2e39817d83608d763f490d5247f59ab Mon Sep 17 00:00:00 2001 From: Amrendra Vikram Singh <76041208+AmrendraTheCoder@users.noreply.github.com> Date: Fri, 14 Aug 2026 14:18:46 +0530 Subject: [PATCH 1/4] fix: validate paginated GitHub responses before spreading fetchWithCache() called res.json(), which throws on an empty body, and each paginated fetcher then spread the result directly into an array. Two real payloads broke that path: - 204 No Content, returned by /contributors for a repo with no commits - a non-array body such as { message: "Moved Permanently" } In explore() the failure was invisible: the call sits inside Promise.allSettled, so the rejection was swallowed and the repo was silently dropped from the contributor model, under-counting analytics with no error shown to the user. Read the body as text and return null when empty, then guard each page with Array.isArray() before spreading so a malformed page ends pagination while keeping the pages already collected. The four fetchers now share one fetchPaginated() helper, so the guard lives in one place rather than four copies of the same loop. Also filter falsy values out of validOrgs in explore(), since fetchOrg can now resolve to null instead of rejecting. --- src/context/AppContext.jsx | 2 +- src/services/github.js | 85 ++++++++++++++++------------ src/services/github.test.js | 107 ++++++++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 37 deletions(-) create mode 100644 src/services/github.test.js 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..0a7a782 100644 --- a/src/services/github.js +++ b/src/services/github.js @@ -77,61 +77,74 @@ 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 + + const data = JSON.parse(text) 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..5078242 --- /dev/null +++ b/src/services/github.test.js @@ -0,0 +1,107 @@ +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), + } +} + +/** 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('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) + }) + + 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('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) + }) +}) From 51cc602a3f300709e1dce551d54753b9836342dd Mon Sep 17 00:00:00 2001 From: Amrendra Vikram Singh <76041208+AmrendraTheCoder@users.noreply.github.com> Date: Fri, 14 Aug 2026 14:56:09 +0530 Subject: [PATCH 2/4] fix: return null when a response body is not valid JSON Addresses CodeRabbit review feedback on #168. The Array.isArray() guard covered bodies that parse to a non-array, but a body that cannot be parsed at all (a proxy error page, a truncated response) still threw from JSON.parse. That rejection propagated out of fetchPaginated and discarded the pages already collected, which is the exact failure the guard was meant to prevent. Wrap the parse and return null on failure, so an unparseable page ends pagination the same way a non-array page does. Adds two regression tests, both of which fail without the try/catch. --- src/services/github.js | 10 +++++++++- src/services/github.test.js | 30 ++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/services/github.js b/src/services/github.js index 0a7a782..c14d8da 100644 --- a/src/services/github.js +++ b/src/services/github.js @@ -83,7 +83,15 @@ async function fetchWithCache(url, pat) { const text = await res.text() if (!text) return null - const data = JSON.parse(text) + 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 } diff --git a/src/services/github.test.js b/src/services/github.test.js index 5078242..9932ff0 100644 --- a/src/services/github.test.js +++ b/src/services/github.test.js @@ -21,6 +21,16 @@ function jsonResponse(body, { status = 200, headers = {} } = {}) { } } +/** 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 { @@ -65,6 +75,26 @@ describe('paginated fetchers: response validation', () => { 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) + }) + 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 })) From 4267e2bda5f7cb9cd4bd625c8dc82c24cce6f57b Mon Sep 17 00:00:00 2001 From: Amrendra Vikram Singh <76041208+AmrendraTheCoder@users.noreply.github.com> Date: Fri, 14 Aug 2026 19:08:24 +0530 Subject: [PATCH 3/4] test: assert the bad page was actually requested Addresses CodeRabbit review feedback on #168. Both "keeps the pages collected" tests asserted only the resulting length, so they would have passed even if pagination had stopped after page 1 and the bad page had never been fetched. That made them weaker than they looked, since the behaviour under test is precisely that the second page is requested and then handled. Assert the fetch count in both. Verified by changing the break condition to stop after the first page: both tests fail with the assertion and passed without it. --- src/services/github.test.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/services/github.test.js b/src/services/github.test.js index 9932ff0..ba42f93 100644 --- a/src/services/github.test.js +++ b/src/services/github.test.js @@ -93,6 +93,9 @@ describe('paginated fetchers: response validation', () => { .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 () => { @@ -104,6 +107,7 @@ describe('paginated fetchers: response validation', () => { .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 () => { From 8290d310118015f39f28e0ee4dbea30357fa5b8f Mon Sep 17 00:00:00 2001 From: Amrendra Vikram Singh <76041208+AmrendraTheCoder@users.noreply.github.com> Date: Fri, 14 Aug 2026 21:35:03 +0530 Subject: [PATCH 4/4] test: cover the maximum page ceiling Addresses the remaining open comment from the first CodeRabbit review on #168, which I had left unanswered. Issue #103 is titled around the missing pagination safety guard, but the tests so far only covered the response validation half. Nothing asserted that the walk actually stops at maxPages, which is the bound the issue name points at. Add two tests where every page comes back full, so only the ceiling can end the loop: - with a PAT, fetch runs 10 times and returns 1000 items - without a PAT, fetch runs once and returns 100 items Verified by replacing the maxPages bound with a large constant: both tests fail without the ceiling and pass with it. --- src/services/github.test.js | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/services/github.test.js b/src/services/github.test.js index ba42f93..419fcca 100644 --- a/src/services/github.test.js +++ b/src/services/github.test.js @@ -128,6 +128,26 @@ describe('paginated fetchers: happy path', () => { 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 }))