Fetch all GitHub issues with pagination#21
Conversation
When fetching issues with per_page=100 and there are more results, now fetches all follow-up pages up to the GitHub Search API limit of 1000 results (10 pages). Uses total_count from API response to determine when all items have been fetched.
There was a problem hiding this comment.
Pull request overview
This PR adds pagination support for fetching GitHub issues and pull requests, allowing the application to retrieve up to 1000 results (the GitHub Search API limit) across multiple pages instead of being limited to the first 100 results.
Changes:
- Introduced a new
fetchAllSearchItemsfunction that handles paginated fetching of issues/PRs - Refactored the existing single-page search logic to use the new pagination function
- Added progress reporting for each page fetch and improved user feedback with item counts
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Log if we hit the pagination limit | ||
| if (page > maxPages) { |
There was a problem hiding this comment.
This condition will never be true because the while loop terminates when page <= maxPages, meaning page can at most equal maxPages + 1 after the loop. The warning should check page > maxPages before entering the loop or use page === maxPages + 1 after the loop, but more importantly, this should check if we exited due to hitting maxPages rather than fetching all items. Consider checking allItems.length >= totalCount instead to determine if pagination was incomplete.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
- Changed condition from `page > maxPages` (which would never be true) to `allItems.length < totalCount` - Moved totalCount variable outside the loop to be accessible after loop completion - Now properly detects when pagination limit is hit with more items remaining - Updated warning message to show actual vs total count Co-authored-by: kertal <463851+kertal@users.noreply.github.com>
Fix pagination limit warning condition in fetchAllSearchItems
Deploying git-vegas with
|
| Latest commit: |
bf803ce
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://53d03a67.git-vegas.pages.dev |
| Branch Preview URL: | https://claude-github-issues-paginat.git-vegas.pages.dev |
Tests cover: - Fetching multiple pages when total_count exceeds per_page - Stopping when all items are retrieved (items.length < perPage) - Respecting max pages limit (10 pages / 1000 results) - Preserving assignee data and original property on items
- Add 422 pagination limit error handling (consistent with fetchAllEvents) - Return partial results on error instead of throwing when items exist - Add tests for 422 handling and partial results on network error
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const maxPages = 10; // GitHub Search API allows up to 1000 results (10 pages * 100 per page) | ||
| let totalCount = 0; | ||
|
|
||
| const searchQuery = `(author:${username} OR assignee:${username}) AND updated:${startDate}..${endDate} AND (is:issue OR is:pr)`; |
There was a problem hiding this comment.
The search query construction is duplicated from the code that was removed (line 304 in the old code had an extra space before '(is:issue OR is:pr)'). While the extra space has been correctly removed in the new implementation, consider extracting this query construction into a separate helper function to avoid future inconsistencies and improve maintainability.
| const responseJSON = await response.json(); | ||
|
|
||
| // Handle pagination limit error (422) - return what we have so far | ||
| if (response.status === 422 && responseJSON.message?.includes('pagination')) { |
There was a problem hiding this comment.
The error message check uses a generic substring 'pagination', while the similar check in fetchAllEvents at line 133 uses the more specific 'pagination is limited'. Consider using a consistent and more specific error message pattern across both functions to ensure reliable error detection.
| if (response.status === 422 && responseJSON.message?.includes('pagination')) { | |
| if (response.status === 422 && responseJSON.message?.includes('pagination is limited')) { |
GitHub Apps with user access tokens cannot combine is:issue and is:pull-request in a single query. Now makes separate API calls for issues and PRs, then combines the results. Updated tests to account for the separate queries.
GitHub search API doesn't support parentheses for grouping. Now makes 4 separate queries per user: - author:username is:issue - assignee:username is:issue - author:username is:pull-request - assignee:username is:pull-request Results are deduplicated by id to handle items matching multiple queries. Added test for deduplication.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: kertal <463851+kertal@users.noreply.github.com>
Fix pagination termination logic and extract max pages constant
Use "involves:username" instead of separate author/assignee queries. This reduces API calls from 4 to 2 per user (50% reduction). involves: matches author, assignee, commenter, and mentions in one query.
Instead of making 2 API calls per user, now combines all users into just 2 total API calls (one for issues, one for PRs). Query format uses OR with repeated qualifiers: is:issue updated:... involves:user1 OR is:issue updated:... involves:user2 This works because AND has higher precedence than OR in GitHub search. For 5 users, this reduces API calls from 10 to 2 (80% reduction). Events API still requires per-user queries (no OR support).
Combined OR queries were rejected by GitHub API with "search contains only logical operators without search terms" error. Reverted to simple per-user queries using involves: qualifier (2 API calls per user). The involves: qualifier still provides optimization by matching author, assignee, commenter, and mentions in a single query instead of separate author/assignee queries.
Combines multiple users into single queries using OR between involves: Query: "is:issue updated:... involves:user1 OR involves:user2" This reduces N users from 2*N API calls to just 2 calls total for issues/PRs. Events API still requires per-user calls.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
|
|
||
| if (items.length < perPage || itemsFetchedForQuery >= totalCount) { |
There was a problem hiding this comment.
The condition itemsFetchedForQuery >= totalCount may cause premature pagination termination. Since itemsFetchedForQuery accumulates the count of items added to allItems after deduplication, but items can be deduplicated (skipped if already in seenIds), the actual number of unique items fetched could be less than totalCount even when more pages are available. This should compare against the raw count before deduplication or use a different termination condition.
| const totalCount = searchData.total_count; | ||
| const items = searchData.items; | ||
|
|
||
| itemsFetchedForQuery += items.length; |
There was a problem hiding this comment.
The counter itemsFetchedForQuery is incremented by the full page size before deduplication occurs. This means it counts items that may be filtered out by the seenIds check on lines 192-200. For accurate tracking against totalCount, this should be incremented inside the deduplication loop only for items that are actually added, or the comparison on line 203 should account for this discrepancy.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes implement batched GitHub search API querying with pagination support. A new Changes
Sequence DiagramsequenceDiagram
participant Hook as useGitHubDataFetching
participant BatchFunc as fetchAllSearchItems
participant API as GitHub Search API
participant Storage as IndexedDB
Hook->>BatchFunc: handleSearch(usernames)
activate BatchFunc
BatchFunc->>BatchFunc: Build OR-combined query<br/>(multiple usernames)
BatchFunc->>API: Query issues (page 1)
activate API
API-->>BatchFunc: Results + total_count
deactivate API
loop While more pages available<br/>and pages < MAX_PAGES
BatchFunc->>API: Query next page
activate API
API-->>BatchFunc: Results
deactivate API
end
BatchFunc->>API: Query pull-requests (paginated)
activate API
API-->>BatchFunc: PR Results
deactivate API
BatchFunc->>BatchFunc: Deduplicate by ID<br/>Normalize fields
alt Success with results
BatchFunc->>Storage: Store combined items
activate Storage
Storage-->>BatchFunc: Stored
deactivate Storage
else Error mid-fetch
BatchFunc->>Storage: Store partial results
activate Storage
Storage-->>BatchFunc: Stored
deactivate Storage
end
BatchFunc-->>Hook: Items collected
deactivate BatchFunc
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useGitHubDataFetching.ts (1)
111-116:⚠️ Potential issue | 🟡 MinorDead code after
throwstatement.Lines 114-115 are unreachable because line 113 throws an error unconditionally. The comment
// Continue with what we have so farsuggests the original intent was to break gracefully, but thethrowmakes this impossible.Proposed fix: Either remove dead code or change behavior to match comment
Option 1 - Remove dead code (keep throwing):
} catch (error) { console.error(`Error fetching events page ${page} for ${username}:`, error); throw error; - // Continue with what we have so far - hasMorePages = false; }Option 2 - Continue with partial results (match the comment):
} catch (error) { console.error(`Error fetching events page ${page} for ${username}:`, error); - throw error; // Continue with what we have so far hasMorePages = false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useGitHubDataFetching.ts` around lines 111 - 116, The catch block inside useGitHubDataFetching currently throws the caught error and then contains unreachable code setting hasMorePages = false and a comment about continuing; either remove the dead code and keep the throw (so the function fails fast), or change the behavior to match the comment by not re-throwing: log the error (using console.error or the module logger), set hasMorePages = false (or break/return partial results) and allow the fetch loop to exit gracefully; update the catch in useGitHubDataFetching to use the chosen approach consistently and reference the caught error variable when logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/hooks/useGitHubDataFetching.ts`:
- Around line 111-116: The catch block inside useGitHubDataFetching currently
throws the caught error and then contains unreachable code setting hasMorePages
= false and a comment about continuing; either remove the dead code and keep the
throw (so the function fails fast), or change the behavior to match the comment
by not re-throwing: log the error (using console.error or the module logger),
set hasMorePages = false (or break/return partial results) and allow the fetch
loop to exit gracefully; update the catch in useGitHubDataFetching to use the
chosen approach consistently and reference the caught error variable when
logging.
When fetching issues with per_page=100 and there are more results, now fetches all follow-up pages up to the GitHub Search API limit of 1000 results (10 pages). Uses total_count from API response to determine when all items have been fetched.
Summary by CodeRabbit
New Features
Bug Fixes
Tests