Extract paginatedFetch and fetchWithRetry into src/api-utils.ts#19
Extract paginatedFetch and fetchWithRetry into src/api-utils.ts#19
Conversation
- Add src/api-utils.ts with two pure-async helpers:
- paginatedFetch<T>: replaces the three duplicated while-pagination loops
- fetchWithRetry: retries on 429/503 with exponential backoff and Retry-After
header support; returns last response after maxRetries exhausted
- Add src/api-utils.test.ts with 14 unit tests (mocked fetch, instant timers)
- Refactor src/api.ts to consume the new helpers:
- searchCode: use fetchWithRetry instead of bare fetch
- fetchAllResults: use fetchWithRetry for raw file content resolution
- fetchRepoTeams (list teams): replace while loop with paginatedFetch
- fetchRepoTeams (repos per team): replace while loop with paginatedFetch
- Fix: silent break on HTTP error for team repos now logs a warning and
returns [], instead of swallowing errors silently
- Extract githubHeaders() helper to deduplicate header objects
Closes #17
|
Coverage after merging refactor/dry-api-pagination-retry into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR extracts shared pagination and retry logic from src/api.ts into a new helper module, adds unit tests for the extracted helpers, and updates the GitHub API client to use the new implementations (including improved handling of team-repo fetch errors).
Changes:
- Add
fetchWithRetry(429/503 retry with backoff + Retry-After support) andpaginatedFetchhelpers in a newsrc/api-utils.ts. - Add
src/api-utils.test.tsto unit test retry and pagination behavior with mockedfetchand instant timers. - Refactor
src/api.tsto use the new helpers and deduplicate request headers via agithubHeaders()helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/api.ts | Refactors API calls to use retry/pagination helpers and consolidates GitHub headers. |
| src/api-utils.ts | Introduces shared retry + pagination helper functions. |
| src/api-utils.test.ts | Adds unit coverage for the new helpers with mocked globals. |
| export async function fetchWithRetry( | ||
| url: string, | ||
| options: RequestInit, | ||
| maxRetries = 3, | ||
| ): Promise<Response> { |
There was a problem hiding this comment.
src/api-utils.ts introduces a new side-effectful module (uses fetch/setTimeout) even though the repo docs state that I/O should be isolated to api.ts, tui.ts, and github-code-search.ts (see AGENTS.md:94-96). To stay within that boundary, either keep these helpers inside api.ts, or make api-utils.ts purely computational (e.g., export backoff/pagination logic that takes injected fetch/sleep functions from api.ts). If the intent is to expand the allowed I/O surface, the architecture docs should be updated accordingly.
| // Pure-async helpers with no side effects beyond network I/O. These are the | ||
| // only place in the codebase that knows about GitHub rate-limit semantics. |
There was a problem hiding this comment.
The file header describes these as “Pure-async helpers with no side effects beyond network I/O”, but the implementation also depends on timers and nondeterminism (setTimeout and Math.random() jitter). Please adjust the comment to match reality (or remove jitter) so future readers don’t treat this module as pure/deterministic.
| // Pure-async helpers with no side effects beyond network I/O. These are the | |
| // only place in the codebase that knows about GitHub rate-limit semantics. | |
| // Async helpers that encapsulate pagination, retry, and GitHub rate-limit | |
| // semantics for network calls. |
| const result = await paginatedFetch(async (page) => pages[page - 1] ?? [], 2); | ||
| void callCount; |
There was a problem hiding this comment.
This test declares callCount but never increments or asserts on it, and void callCount; just masks the unused-variable warning. Either remove callCount entirely or use it to assert how many pages were fetched, so the test meaningfully verifies multi-page behavior.
| const result = await paginatedFetch(async (page) => pages[page - 1] ?? [], 2); | |
| void callCount; | |
| const result = await paginatedFetch(async (page) => { | |
| callCount++; | |
| return pages[page - 1] ?? []; | |
| }, 2); | |
| expect(callCount).toBe(3); |
What does this PR do?
Closes #17
How did you verify your code works?