fix: paginate discussion comments so MAX_COMMENTS_EVAL is honored - #803
fix: paginate discussion comments so MAX_COMMENTS_EVAL is honored#803Sanjays2402 wants to merge 2 commits into
Conversation
get_discussions fetched comments with a single comments(first: 100) sub-list and never paginated it, while the outer search connection was paginated. With MAX_COMMENTS_EVAL set above 100, count_comments_per_user could never see more than those 100 nodes and silently stopped, diverging from the issue and pull request branches which keep paging until the limit is satisfied. The search query now also selects the discussion id and the comments pageInfo, and _fetch_remaining_comments pages the remaining comments for each discussion until hasNextPage is false or max_comments nodes are collected. issue_metrics passes the configured max_comments_eval through. Adds two regression tests covering the multi-page case and the case where the limit is already reached so no extra request is made.
There was a problem hiding this comment.
🟢 Ready to approve
The pagination logic and regression tests address the reported 100-comment cap and appear low-risk, with only a minor documentation nit remaining.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR fixes discussion mentor-counting accuracy when MAX_COMMENTS_EVAL is set above 100 by adding cursor-based pagination for discussion comments, bringing discussion behavior in line with issues and pull requests.
Changes:
- Add
_fetch_remaining_commentsand a follow-up GraphQL query to paginate a discussion’scommentsconnection untilmax_commentsis reached or no more pages remain. - Thread
max_comments_evalfromissue_metrics.pyintoget_discussionsso the configured limit is honored. - Add regression tests covering multi-page discussion comments and ensuring no extra request is made when the limit is already satisfied.
File summaries
| File | Description |
|---|---|
| discussions.py | Adds comment pagination support (including pageInfo selection and a helper query) to honor max_comments. |
| issue_metrics.py | Passes the configured max_comments_eval through to get_discussions. |
| test_discussions.py | Adds regression coverage for comment pagination and early-stop behavior at max_comments. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| COMMENTS_PAGE_SIZE = 100 | ||
|
|
||
| # Fetches further pages of a single discussion's comments once the first page | ||
| # returned by the search query is exhausted. | ||
| COMMENTS_QUERY = """ |
|
@Sanjays2402 we currently have our test coverage thresshold to 100%. Can you please take a look at the python 3.11 check failure. Looks like 14 lines need coverage in discussions.py. |
|
The uncovered line was the early
|
There was a problem hiding this comment.
🟢 Ready to approve
The pagination fix is narrowly scoped, aligns with the stated issue, and is covered by targeted regression tests.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
discussions.py:87
- The
max_commentsparameter docstring currently says it is the maximum number of comments to collect per discussion, butget_discussionsalways fetches the first page withcomments(first: 100), so it may still return up to 100 comments even whenmax_commentsis smaller (e.g., default 20). Please clarify the docstring to reflect thatmax_commentsonly controls additional pagination beyond the first page.
max_comments (int): Maximum number of comments to collect per discussion.
Values above the GraphQL page size trigger extra requests, so that
discussions match the issue and pull request branches, which keep
paginating until the limit is satisfied.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Pull Request
Proposed Changes
Closes #798
get_discussionspaginated the outersearchconnection but fetched each discussion's comments with a singlecomments(first: 100)sub-list. WithMAX_COMMENTS_EVALset above 100,count_comments_per_usercould never see more than those 100 nodes and stopped silently, unlike the issue and pull request branches, which keep paging until the limit is satisfied.The search query now also selects the discussion
idand the commentspageInfo, and a new_fetch_remaining_commentshelper pages the rest of a discussion's comments untilhasNextPageis false ormax_commentsnodes are collected.issue_metrics.pypasses the configuredmax_comments_evalthrough, so the default of 20 makes no extra request.Two regression tests were added to
test_discussions.py: one covering a discussion whose comments span two pages, and one asserting no follow-up request is made once the limit is already reached. Both fail without the change.This change was prepared with AI assistance; the regression tests were run locally and fail without the fix.
Readiness Checklist
Author/Contributor
make lintand fix any issues that you have introducedmake testand ensure you have test coverage for the lines you are introducing