Skip to content

Commit 8e4e6e2

Browse files
KrisBraunclaude
andcommitted
fix(github): bound issue and pull request comment fetching
When converting an issue or pull request into a thread, the connector fetched its comments. Issue conversion paginated with no upper bound; PR conversion fetched only the first page of comments, reviews, and review comments, silently dropping the rest. Because a single sync pass converts many items in one execution, a batch of heavily-commented items could exceed the connector runtime's per-execution request budget and fail the pass. Add a shared bounded-pagination helper (fetchPagedList) that caps each list at MAX_COMMENT_PAGES (3 pages / 300 items) and stops on the first short page. Both converters, fetchReviewComments, and the pull request reopened key refresh route through it. Comments are fetched newest-first where the API supports a sort direction (issue/conversation comments and review comments); the reviews endpoint has none, so those keep their default order. Items under the cap fetch everything; items over it keep their most recent 300, plus the always-present description. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 5023aa7 commit 8e4e6e2

6 files changed

Lines changed: 523 additions & 72 deletions

File tree

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
import { describe, expect, it } from "vitest";
2+
import { convertIssueToLink } from "./issue-sync";
3+
import type { GitHubIssueComment } from "./github";
4+
5+
function makeIssue(overrides: Record<string, unknown> = {}) {
6+
return {
7+
number: 42,
8+
title: "A bug",
9+
body: "The description.",
10+
html_url: "https://github.com/acme/web/issues/42",
11+
created_at: "2026-07-01T00:00:00Z",
12+
closed_at: null,
13+
user: { id: 1, login: "octocat" },
14+
assignees: [],
15+
assignee: null,
16+
...overrides,
17+
};
18+
}
19+
20+
const comment = (id: number): GitHubIssueComment => ({
21+
id,
22+
body: `comment ${id}`,
23+
created_at: "2026-07-02T00:00:00Z",
24+
updated_at: "2026-07-02T00:00:00Z",
25+
user: { id: 9, login: "commenter" },
26+
html_url: "",
27+
});
28+
29+
/** A page of `n` comments with ids starting at `base`. */
30+
const commentPage = (n: number, base: number) =>
31+
Array.from({ length: n }, (_, i) => comment(base + i));
32+
33+
/**
34+
* Fake source serving a preconfigured list of comment pages keyed by the
35+
* `page=` query param, recording every requested path.
36+
*/
37+
function makeIssueSource(commentPages: GitHubIssueComment[][]): {
38+
source: any;
39+
paths: string[];
40+
} {
41+
const paths: string[] = [];
42+
const source = {
43+
userToContact: (user: { id: number; login: string }) => ({
44+
email: `${user.id}+${user.login}@users.noreply.github.com`,
45+
name: user.login,
46+
source: { accountId: String(user.id) },
47+
}),
48+
githubFetch: async (_token: string, path: string) => {
49+
paths.push(path);
50+
const pageNum = Number(new URLSearchParams(path.split("?")[1]).get("page"));
51+
const items = commentPages[pageNum - 1] ?? [];
52+
return { ok: true, json: async () => items } as unknown as Response;
53+
},
54+
};
55+
return { source, paths };
56+
}
57+
58+
const commentNotes = (link: any) =>
59+
link.notes.filter((n: any) => typeof n.key === "string" && n.key.startsWith("comment-"));
60+
61+
describe("convertIssueToLink comment fetching", () => {
62+
it("always emits a description note", async () => {
63+
const { source } = makeIssueSource([[]]);
64+
const link = await convertIssueToLink(source, "tok", "acme", "web", makeIssue(), "acme/web", false);
65+
const description = link?.notes?.find((n: any) => n.key === "description");
66+
expect(description?.content).toBe("The description.");
67+
});
68+
69+
it("fetches every comment in a single request when under the page cap", async () => {
70+
const { source, paths } = makeIssueSource([commentPage(30, 0)]);
71+
const link = await convertIssueToLink(source, "tok", "acme", "web", makeIssue(), "acme/web", false);
72+
expect(commentNotes(link)).toHaveLength(30);
73+
expect(paths).toHaveLength(1);
74+
});
75+
76+
it("caps at MAX_COMMENT_PAGES and never issues a request past the cap", async () => {
77+
const { source, paths } = makeIssueSource([
78+
commentPage(100, 0),
79+
commentPage(100, 100),
80+
commentPage(100, 200),
81+
commentPage(100, 300),
82+
]);
83+
const link = await convertIssueToLink(source, "tok", "acme", "web", makeIssue(), "acme/web", false);
84+
// Keeps 3 pages (300 comments), never fetches the 4th full page.
85+
expect(commentNotes(link)).toHaveLength(300);
86+
expect(paths).toHaveLength(3);
87+
});
88+
89+
it("requests comments newest-first so the retained pages are the most recent", async () => {
90+
const { source, paths } = makeIssueSource([commentPage(10, 0)]);
91+
await convertIssueToLink(source, "tok", "acme", "web", makeIssue(), "acme/web", false);
92+
expect(paths[0]).toContain("/repos/acme/web/issues/42/comments");
93+
expect(paths[0]).toContain("sort=created");
94+
expect(paths[0]).toContain("direction=desc");
95+
});
96+
});

‎connectors/github/src/issue-sync.ts‎

Lines changed: 19 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
} from "@plotday/twister";
66
import type { GitHub, GitHubIssueComment } from "./github";
77
import { parseRateLimit } from "./github";
8+
import { fetchPagedList } from "./pagination";
89

910
/** Issues per page for batch sync */
1011
const PAGE_SIZE = 50;
@@ -187,32 +188,25 @@ export async function convertIssueToLink(
187188
author: authorContact,
188189
});
189190

190-
// Fetch comments
191+
// Fetch comments, newest-first and bounded to MAX_COMMENT_PAGES so an issue
192+
// with thousands of comments can't exhaust the per-execution request budget
193+
// (see pagination.ts). Items under the cap fetch everything; items over it
194+
// keep their most recent 300 comments (the description note is always kept).
191195
try {
192-
let commentPage = 1;
193-
let hasMoreComments = true;
194-
195-
while (hasMoreComments) {
196-
const commentsResponse = await source.githubFetch(
197-
token,
198-
`/repos/${owner}/${repo}/issues/${issue.number}/comments?per_page=100&page=${commentPage}`,
199-
);
200-
201-
if (!commentsResponse.ok) break;
202-
203-
const comments: GitHubIssueComment[] = await commentsResponse.json();
204-
for (const comment of comments) {
205-
const commentAuthor = source.userToContact(comment.user);
206-
notes.push({
207-
key: `comment-${comment.id}`,
208-
content: comment.body ?? null,
209-
created: new Date(comment.created_at),
210-
author: commentAuthor,
211-
});
212-
}
213-
214-
hasMoreComments = comments.length === 100;
215-
commentPage++;
196+
const comments = await fetchPagedList<GitHubIssueComment>(
197+
source,
198+
token,
199+
(page) =>
200+
`/repos/${owner}/${repo}/issues/${issue.number}/comments?per_page=100&sort=created&direction=desc&page=${page}`,
201+
);
202+
for (const comment of comments) {
203+
const commentAuthor = source.userToContact(comment.user);
204+
notes.push({
205+
key: `comment-${comment.id}`,
206+
content: comment.body ?? null,
207+
created: new Date(comment.created_at),
208+
author: commentAuthor,
209+
});
216210
}
217211
} catch (error) {
218212
console.error("Error fetching issue comments:", error);
Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
import { describe, expect, it } from "vitest";
2+
import { fetchPagedList, MAX_COMMENT_PAGES } from "./pagination";
3+
4+
/** Build a page of `n` placeholder items. */
5+
const page = (n: number) => Array.from({ length: n }, (_, i) => ({ id: i }));
6+
7+
/**
8+
* Fake source whose `githubFetch` serves a preconfigured list of pages,
9+
* indexed by the `page=` query param. An entry of `{ ok: false }` models a
10+
* non-OK response; anything else is served as a JSON array. Records the paths
11+
* requested so tests can assert the exact number of requests issued.
12+
*/
13+
function makeSource(pages: (unknown[] | { ok: false })[]): {
14+
source: { githubFetch(token: string, path: string): Promise<Response> };
15+
paths: string[];
16+
} {
17+
const paths: string[] = [];
18+
const source = {
19+
githubFetch: async (_token: string, path: string) => {
20+
paths.push(path);
21+
const pageNum = Number(new URLSearchParams(path.split("?")[1]).get("page"));
22+
const entry = pages[pageNum - 1];
23+
if (!entry || (entry as { ok?: boolean }).ok === false) {
24+
return { ok: false, status: 403, json: async () => [] } as unknown as Response;
25+
}
26+
return { ok: true, json: async () => entry } as unknown as Response;
27+
},
28+
};
29+
return { source, paths };
30+
}
31+
32+
describe("fetchPagedList", () => {
33+
it("exposes a comment-page cap of 3 (300 items per list)", () => {
34+
expect(MAX_COMMENT_PAGES).toBe(3);
35+
});
36+
37+
it("returns everything and stops after one request when the first page is short", async () => {
38+
const { source, paths } = makeSource([page(30)]);
39+
const items = await fetchPagedList(source, "tok", (p) => `/x?per_page=100&page=${p}`);
40+
expect(items).toHaveLength(30);
41+
expect(paths).toEqual(["/x?per_page=100&page=1"]);
42+
});
43+
44+
it("returns an empty list (one request) when the first page is empty", async () => {
45+
const { source, paths } = makeSource([[]]);
46+
const items = await fetchPagedList(source, "tok", (p) => `/x?page=${p}`);
47+
expect(items).toEqual([]);
48+
expect(paths).toHaveLength(1);
49+
});
50+
51+
it("stops early on the first short page even below the cap", async () => {
52+
const { source, paths } = makeSource([page(100), page(50)]);
53+
const items = await fetchPagedList(source, "tok", (p) => `/x?page=${p}`);
54+
expect(items).toHaveLength(150);
55+
expect(paths).toEqual(["/x?page=1", "/x?page=2"]);
56+
});
57+
58+
it("caps at MAX_COMMENT_PAGES and never requests past it when every page is full", async () => {
59+
const { source, paths } = makeSource([page(100), page(100), page(100), page(100)]);
60+
const items = await fetchPagedList(source, "tok", (p) => `/x?page=${p}`);
61+
expect(items).toHaveLength(300);
62+
// Never issued a 4th request despite a full 3rd page.
63+
expect(paths).toEqual(["/x?page=1", "/x?page=2", "/x?page=3"]);
64+
});
65+
66+
it("respects an explicit maxPages argument", async () => {
67+
const { source, paths } = makeSource([page(100), page(100), page(100)]);
68+
const items = await fetchPagedList(source, "tok", (p) => `/x?page=${p}`, 2);
69+
expect(items).toHaveLength(200);
70+
expect(paths).toHaveLength(2);
71+
});
72+
73+
it("returns the pages fetched so far (best-effort) when a later page is non-OK", async () => {
74+
const { source, paths } = makeSource([page(100), { ok: false }]);
75+
const items = await fetchPagedList(source, "tok", (p) => `/x?page=${p}`);
76+
expect(items).toHaveLength(100);
77+
expect(paths).toEqual(["/x?page=1", "/x?page=2"]);
78+
});
79+
});
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
/**
2+
* Bounded pagination for GitHub list endpoints (issue/PR comments, reviews).
3+
*
4+
* A single item conversion (`convertIssueToLink` / `convertPRToThread`) can be
5+
* invoked up to 50 times per connector execution (a full channel-sync batch or
6+
* a full followed-items notifications page), and every `githubFetch` counts
7+
* against the runtime's per-execution request budget (~1000 requests; see
8+
* `twister/docs/RUNTIME.md`). An item with thousands of comments must therefore
9+
* not fetch one request per 100 comments unbounded — a handful of such items
10+
* would exhaust the budget and kill the execution.
11+
*
12+
* `fetchPagedList` caps the pages fetched per list at `MAX_COMMENT_PAGES`. Items
13+
* under the cap fetch everything (the loop stops on the first short page, so the
14+
* common case — well under 100 comments — is a single request). Items over it
15+
* keep the first `MAX_COMMENT_PAGES` pages; callers order the endpoint
16+
* newest-first (`sort=created&direction=desc`) where the API supports it so the
17+
* retained pages are the most recent, most relevant comments.
18+
*/
19+
20+
/**
21+
* Max pages (100 items each) fetched per comment/review list, per item. 3 → up
22+
* to 300 of each list type. This bounds a single execution's request count:
23+
* worst case 50 items × (3 lists × 3 pages) ≈ 450 comment requests for PRs,
24+
* well under the ~1000 budget.
25+
*/
26+
export const MAX_COMMENT_PAGES = 3;
27+
28+
/**
29+
* Fetch up to `maxPages` pages (100 items each) from a GitHub list endpoint,
30+
* concatenated in fetch order. `pathForPage(page)` builds the request path for
31+
* a given 1-based page number.
32+
*
33+
* Stops early on the first short page (fewer than 100 items = the last page).
34+
* On a non-OK response, returns the items gathered so far — partial comments
35+
* are better than none, matching the converters' pre-existing best-effort
36+
* behavior when a comment fetch fails mid-item.
37+
*/
38+
export async function fetchPagedList<T>(
39+
source: { githubFetch(token: string, path: string): Promise<Response> },
40+
token: string,
41+
pathForPage: (page: number) => string,
42+
maxPages: number = MAX_COMMENT_PAGES,
43+
): Promise<T[]> {
44+
const items: T[] = [];
45+
for (let page = 1; page <= maxPages; page++) {
46+
const response = await source.githubFetch(token, pathForPage(page));
47+
if (!response.ok) break;
48+
const batch: T[] = await response.json();
49+
items.push(...batch);
50+
if (batch.length < 100) break;
51+
}
52+
return items;
53+
}

0 commit comments

Comments
 (0)