Skip to content

fix: stop dropping a review thread's fifty-first comment - #66

Merged
kukv merged 5 commits into
mainfrom
worktree-fluttering-cuddling-papert
Sep 8, 2026
Merged

fix: stop dropping a review thread's fifty-first comment#66
kukv merged 5 commits into
mainfrom
worktree-fluttering-cuddling-papert

Conversation

@kukv

@kukv kukv commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Phase 3 の 3 本目、最後。実装計画は docs/superpowers/plans/2026-09-08-phase3-comment-paging.md

塞いだ穴

1 本のレビュースレッドに 51 件目以降のコメントが付いていると、エラーも出ずに黙って落ちていた
review.graphqlcomments(first: 50) を追う手段が無かったため。

comments は nested connection なので、reviewThreads の cursor では追えない。
50 件を超えたと自分で言っているスレッドだけnode(id:) でそのスレッドを名指しして残りを引く
thread_comments.graphql)。超えていないスレッドには 1 リクエストも足さない。
ページ数の上限は置かない(reviewThreads が上限なしに全ページ取るのと同じ流儀)。

internal/tui は 1 行も触っていない。画面の形は変わらず、今まで消えていた行が出るようになる。

実物で 1 度確認した

thread_comments.graphql は録ったスキーマとしか突き合わせていなかったので、実際に叩いた。
PR #59 のスレッド(PRRT_kwDOTVXF-M6fwhR-)で data.node.comments.nodes が返ることを確認した。

全体レビューで見つけて直したもの

繰り越すつもりだった 2 件が、見積もりより悪かったので撤回して直した。

  1. 追加取得のガードが cursor を見ていなかった。 hasNextPage: trueendCursor が空の応答に
    対して空カーソルのリクエストを投げる。GitHub がそれを無効なカーソルとして断ると
    PRReviewContext 全体が error になり、diff のレビュー欄が丸ごと消える
    「無駄な 1 往復」で済む話ではなかった
  2. カーソルの引き継ぎをテストが見ていなかった。 cursor = page.PageInfo.EndCursor を落としても
    テストは通るが、実物では同じページを問い続ける上限の無い無限ループになる。
    ページ数に上限を置かないと決めている以上、これが一番悪い失敗の形

ほかに、追加取得のエラーが包まれていなかった件(隣の parse thread comments: と非対称)も直した。

触らなかったもの

  • 1 ページ目は comments(first: 50) のまま。この 1 ページは全スレッド分まとめて返ってくるので、
    上げると「長いスレッドが 1 本ある PR」ではなく全部の PR の応答が重くなる
  • Work 板の work.graphql、search の first: 50labelsfirst: 100
    spec §5 が「別の機会にする」と書いているもの

Phase 3 完了

checks(#62)、merge(#65)、ページング(この PR)で 3 本すべて。
docs/superpowers/2026-09-08-phase3-checks-followups.md を締めた。

残っているのは設計判断 1 つだけ: GitHub App が作った check run が、直上のワークフローの
一員に見える
問題。「再実行できる run が無い」ことを示す印を spec で決める必要がある。

実端末での確認は 2 つの handoff 文書に残っている(checks / merge)。

🤖 Generated with Claude Code

kukv and others added 5 commits September 8, 2026 07:29
hasNextPage alone can arrive with an empty endCursor; sending that as
after= risks GitHub rejecting the request and PRReviewContext returning
no review comments at all. Match the two page loops already in this
file, wrap the fetch's error with the step it failed at, and assert the
follow-up walk carries the cursor forward past the first extra page.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both handoffs describe work that still needs a real terminal to confirm.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Code Metrics Report

Coverage Test Execution Time
87.5% 38s

Code coverage of files in pull request scope (93.5%, patch 86.6%)

Files Coverage Patch Coverage
internal/gh/cli/review.go 93.5% 86.6%

Reported by octocov

@kukv
kukv merged commit 7993345 into main Sep 8, 2026
3 checks passed
@kukv
kukv deleted the worktree-fluttering-cuddling-papert branch September 8, 2026 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant