fix(github): bound fetchLiveReviewThreadBlockers' review-thread GraphQL pagination - #7480
Conversation
…QL pagination The reviewThreads(first: 50, after: cursor) loop in fetchLiveReviewThreadBlockers was an unbounded for(;;) that only stopped on no-next-page / no-nodes / a repeated cursor, so a PR with a pathologically large number of review threads could drive an unbounded number of sequential GraphQL calls on every merge-readiness evaluation -- unlike every other paginated list-fetch in src/github/**. Add REVIEW_THREAD_MAX_PAGES=10 (mirroring PR_DETAIL_MAX_PAGES / MAX_WORKFLOW_RUN_LIST_PAGES) and bound the loop; reaching the cap returns the blockers gathered so far (fail-open), never throws. Also documents why the inner comments(first: 20) connection is intentionally not paginated (thread-level resolved/outdated flags gate blocking, and a missed authorizer past JSONbored#20 only fails open). Adds regression tests for the multi-page walk and the page-cap bound. Closes JSONbored#7454
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7480 +/- ##
==========================================
- Coverage 91.36% 91.35% -0.02%
==========================================
Files 717 717
Lines 73022 73023 +1
Branches 21629 21631 +2
==========================================
- Hits 66719 66709 -10
- Misses 5265 5272 +7
- Partials 1038 1042 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-20 12:10:46 UTC
Review summary Nits — 4 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Summary
Fixes #7454.
src/github/backfill.ts'sfetchLiveReviewThreadBlockers— the live GraphQL read that computes unresolved review-thread merge blockers, consumed directly by the merge/close decision path — walkedreviewThreads(first: 50, after: $cursor)in an unboundedfor (;;)loop. It only stopped when GitHub reported no more pages, returned no nodes, or repeated a cursor, so a PR that accumulates a pathologically large number of review threads could drive an unbounded number of sequential GraphQL calls every time merge-readiness was evaluated — unlike every other multi-page loop insrc/github/**(PR_DETAIL_MAX_PAGES,MAX_WORKFLOW_RUN_LIST_PAGES,REVIEW_PAGE_LIMIT,COMMENT_SEARCH_PAGE_LIMIT,MAX_REPO_PAGES), all of which carry an explicit page cap.Fix
REVIEW_THREAD_MAX_PAGES = 10(matching the file's ownPR_DETAIL_MAX_PAGESandapp.ts'sMAX_WORKFLOW_RUN_LIST_PAGES) and converted thefor (;;)loop tofor (let page = 0; page < REVIEW_THREAD_MAX_PAGES; page += 1). All existing early-stop conditions (no next page / no nodes / repeated cursor) are preserved; hitting the cap simply falls through to processing the threads gathered so far — fail-open, matching this function's documented "GraphQL unavailable →[]" posture — and never throws. 10 × 50 = 500 threads is far beyond any real PR.comments(first: 20)connection, the issue's sanctioned lower-cost option: a thread blocks merge only while it's unresolved/non-outdated (a thread-level flag, independent of comment count), and the authorizing comment is the thread-opening review comment (index 0) or an early reply, so 20 covers it with wide margin. Missing an authorizer buried past feat(signals): implement issue quality reports for opportunity ranking #20 only fails open (no blocker), so full nested pagination — N threads × M comment-pages of extra round-trips — isn't worth it for a rare edge with fail-open semantics.Tests
Extended
test/unit/backfill-2.test.ts'sfetchLiveReviewThreadBlockersdescribe block (per the issue) with mocked multi-page GraphQL responses: a 3-page walk that collects a blocker on page 3 and terminates onhasNextPage: false(asserts exactly 3 GraphQL calls, well under the cap), and a pathological always-hasNextPage: true(distinct cursors so the seen-cursor guard never trips first) that stops at exactlyREVIEW_THREAD_MAX_PAGES(10) calls and fails open to[]without throwing. Every changed line + branch is covered (verified via--coverage), socodecov/patchholds; all 134 existingbackfill-2tests pass unchanged.Validation
git diff --checknpm run typecheck(roottsc --noEmit) greennpm run test:coverageon the affected suite: 100% of changed lines + branches covered; all existingbackfill-2tests pass unmodifiedsrc/github/backfill.ts+ its unit test only — an internal GitHub helper, no API/OpenAPI/MCP/UI/DB/wrangler surface, so no generated artifact needs regenerationSafety
site//CNAME/lovablechanges.Closes #7454