Skip to content

fix: paginate GitHub issue-comment lookup to avoid duplicate comments#7

Merged
jongjesse merged 1 commit into
mainfrom
fix/github-comment-pagination
Jun 22, 2026
Merged

fix: paginate GitHub issue-comment lookup to avoid duplicate comments#7
jongjesse merged 1 commit into
mainfrom
fix/github-comment-pagination

Conversation

@jongjesse

@jongjesse jongjesse commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

What

GitHubPlatform.findExistingComment (src/platforms/github.ts) fetched only the first 100 issue comments via octokit.rest.issues.listComments with no pagination. On a PR that accumulates more than 100 issue comments, the existing Polder Drift comment (identified by the marker) could be missed, causing a duplicate comment to be posted on the next run.

This is the GitHub parallel of the bug class fixed for Azure DevOps in #6 (AzdoPlatform.findExistingComment, which paginates via the x-ms-continuationtoken header).

How

  • Walk every page of listComments (per_page: 100, incrementing page) until a page comes back short, capped at MAX_COMMENT_PAGES = 50 so a misbehaving API can't loop forever — same loop-guard shape as AzDO's MAX_THREAD_PAGES.
  • Accumulate all comments, then match the marker (still returning the last match, preserving prior behavior).
  • Read-failure-throws is kept consistent with the AzDO path: octokit throws on a non-2xx response, so a transient list error propagates to the caller rather than masquerading as "no existing comment" (which would duplicate-post).

Tests

Added a GitHubPlatform.findExistingComment pagination block in tests/platforms.test.ts, mirroring the AzDO fetch-stub tests (here stubbing octokit + the Action context):

  • marker comment on a later page → walks to page 2, updates (does not duplicate)
  • a single sub-100 page → stops without over-fetching
  • a list failure → propagates and does not fall through to a create

npm run typecheck clean; npx vitest run green (201 tests). Rebuilt the bundle with npm run build:all since the action runs from dist/.

Context

CodeRabbit flagged this as a non-blocking nitpick on #6 ("GitHub comment lookup lacks pagination, risking duplicates on busy PRs"); it was left out of #6 to keep that PR scoped to error-surfacing.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability when updating comments on GitHub pull requests with high comment volume. The system now properly searches through multiple comment pages to locate and update existing bot comments without duplicating posts.
  • Tests

    • Added comprehensive test coverage for comment pagination and update scenarios on GitHub.

findExistingComment fetched only the first 100 issue comments, so on a PR
with >100 comments the existing Polder Drift comment could be missed and a
duplicate posted on the next run.

Walk every page of listComments (per_page: 100, incrementing page) until a
short page, capped at MAX_COMMENT_PAGES so a misbehaving API can't loop
forever. Octokit throws on a non-2xx response, so a transient list error
propagates rather than masquerading as "no existing comment" — matching the
AzDO findExistingComment behavior fixed in #6.

Adds GitHub pagination tests mirroring the AzDO fetch-stub tests, and
rebuilds dist/ since the action runs from the bundle.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

findExistingComment in GitHubPlatform is updated from a single-page listComments call to a bounded pagination loop using two new constants (COMMENTS_PER_PAGE, MAX_COMMENT_PAGES). It collects matching comment IDs across pages, stops early on a short page, and returns the last match. Three tests are added covering multi-page discovery, early-stop on a short page, and failure propagation.

Changes

Paginated comment lookup

Layer / File(s) Summary
Pagination constants and loop implementation
src/platforms/github.ts
Adds COMMENTS_PER_PAGE and MAX_COMMENT_PAGES constants; rewrites findExistingComment to iterate pages, aggregate marker-matching IDs, stop early on a short page or after MAX_COMMENT_PAGES, and return the last match or null.
Pagination test suite
tests/platforms.test.ts
Adds imports for GitHubPlatform, @actions/core, and @actions/github; introduces describe('GitHubPlatform.findExistingComment pagination') with three cases: found on a later page, stop on short page, and list failure rejects.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: implementing pagination in GitHub issue-comment lookup to prevent duplicate comments when PR has >100 comments.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/github-comment-pagination

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/platforms.test.ts (1)

166-234: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win

Add an explicit max-page guard test.

The suite validates later-page discovery, short-page stop, and failure propagation, but it doesn’t assert termination at the hard cap when every page is “full” and marker-free.

As per coding guidelines, tests/**/*.ts: “Vitest. Prefer behaviour + edge cases over smoke tests.”

Suggested test case
+  it('stops at the max page cap when all scanned pages are full and marker is absent', async () => {
+    const pages = Array.from({ length: 50 }, (_, i) => fullPage(i * 100 + 1));
+    const { platform, listComments, updateComment, createComment } = platformWith(pages);
+
+    await expect(platform.upsertComment('body', '<!--polder-drift-->', false)).resolves.toBe(false);
+
+    expect(listComments).toHaveBeenCalledTimes(50);
+    expect(updateComment).not.toHaveBeenCalled();
+    expect(createComment).not.toHaveBeenCalled();
+  });
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/platforms.test.ts` around lines 166 - 234, Add a new test case within
the 'GitHubPlatform.findExistingComment pagination' describe block that
validates the max-page termination guard. Create a scenario using platformWith()
where you provide multiple full pages (using the fullPage() helper) that all
lack the marker comment, ensuring each page returns exactly 100 comments to
trigger pagination. Then verify that the listComments spy is called a specific
maximum number of times and that the pagination stops rather than continuing
infinitely, protecting against unbounded iteration when a marker is never found.
This edge case ensures the findExistingComment method respects a configured
maximum page limit during its search loop.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/platforms.test.ts`:
- Around line 166-234: Add a new test case within the
'GitHubPlatform.findExistingComment pagination' describe block that validates
the max-page termination guard. Create a scenario using platformWith() where you
provide multiple full pages (using the fullPage() helper) that all lack the
marker comment, ensuring each page returns exactly 100 comments to trigger
pagination. Then verify that the listComments spy is called a specific maximum
number of times and that the pagination stops rather than continuing infinitely,
protecting against unbounded iteration when a marker is never found. This edge
case ensures the findExistingComment method respects a configured maximum page
limit during its search loop.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 288734ba-8845-48c3-bfb4-890c0035ce9e

📥 Commits

Reviewing files that changed from the base of the PR and between 1f31671 and 9edf84b.

⛔ Files ignored due to path filters (1)
  • dist/index.js is excluded by !**/dist/**, !dist/**
📒 Files selected for processing (2)
  • src/platforms/github.ts
  • tests/platforms.test.ts

@jongjesse jongjesse merged commit 5723e4f into main Jun 22, 2026
2 checks passed
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