Add pagination support to GraphQL-based tools #683
Conversation
mattdholloway
changed the title
ListDiscussions
|
Pagination works for listing discussions but does not currently provide the caller with the details of which page it is on: 
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR adds pagination support to GraphQL-based tools for GitHub discussions and related functionality, implementing GitHub's GraphQL cursor-based pagination spec while maintaining compatibility with REST API pagination patterns.
- Introduces unified pagination using page/perPage parameters that convert to GraphQL 'first' parameter
- Adds GraphQLPaginationParams type and conversion methods from REST-style pagination
- Updates GraphQL queries to include pagination parameters and PageInfo for proper cursor-based pagination
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/github/server.go | Adds WithUnifiedPagination function, GraphQLPaginationParams type, and ToGraphQLParams conversion method |
| pkg/github/discussions.go | Updates GraphQL discussions tools to use unified pagination with proper PageInfo handling |
| pkg/github/discussions_test.go | Updates test mocks to include PageInfo and fixes variable types for GraphQL compatibility |
| pkg/github/server_test.go | Updates test expectations for capitalized PaginationParams fields |
| Multiple REST files | Updates field references from lowercase to capitalized PaginationParams fields |
| README.md | Updates documentation to reflect new pagination parameters for discussion tools |
| } | ||
|
|
||
| // Use default of 100 if pagination was not explicitly provided | ||
| if !paginationExplicit { |
There was a problem hiding this comment.
The logic for detecting explicit pagination parameters is duplicated across multiple functions. Consider extracting this into a helper function to reduce code duplication.
Copilot uses AI. Check for mistakes.
pkg/github/discussions.go
Outdated
| @@ -102,6 +119,11 @@ func ListDiscussions(getGQLClient GetGQLClientFn, t translations.TranslationHelp | |||
| } | |||
| discussions = append(discussions, di) | |||
| } | |||
|
|
|||
| out, err = json.Marshal(discussions) | |||
There was a problem hiding this comment.
JSON marshaling is duplicated in both conditional branches. Consider moving this logic outside the conditional blocks to reduce duplication.
Copilot uses AI. Check for mistakes.
|
A downside of using cursor-based pagination in GraphQL is that there is no way of 'skipping' pages as the ID of the final entry of the previously - I am investigating falling back to the REST api if the user needs this functionality. |
This PR idiomatically implements pagination for GraphQL based tools using the spec described in https://docs.github.com/en/graphql/reference/objects#connection and based on the standard in https://graphql.org/learn/pagination/
Closes: https://github.com/github/copilot-agent-services/issues/296