Public: anyone can read this; the crew can change it.
GET …/issues/{number}/comments pages through a thread: ?after=<id>&limit=50 #4160
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/4150-get-issues-number-comments-pages-through → main 1 commit fork 1 thread · 1 unresolved
4 files
changed +126
−2
src/contract/openapi.ts +1
| @@ -82,6 +82,7 @@ export const ROUTES: readonly Route[] = [ | ||
| 82 | 82 | body: { state: "string", reason: "string", into: "integer" }, |
| 83 | 83 | }, |
| 84 | 84 | { method: "get", path: `${REPO}/pulls/{number}`, summary: "Read one pull with its diff and review gate (diff=0 leaves the diff out)", auth: false, query: { diff: "string" } }, |
| 85 | { method: "get", path: `${REPO}/issues/{number}/comments`, summary: "Page through an issue's or pull's comments, oldest first: {comments, next}; limit 1 to 100 (default 50), after a comment id", auth: false, query: { after: "string", limit: "integer" } }, | |
| 85 | 86 | { |
| 86 | 87 | method: "post", |
| 87 | 88 | path: `${REPO}/issues/{number}/comments`, |
src/core/issues.ts +7 −2
| @@ -633,6 +633,10 @@ export interface ListCommentsOptions { | ||
| 633 | 633 | * the same remark shows up twice on one page. @default "all" |
| 634 | 634 | */ |
| 635 | 635 | kind?: "comment" | "review" | "all"; |
| 636 | /** Only comments after this id (ids sort oldest first). */ | |
| 637 | after?: string; | |
| 638 | /** At most this many. */ | |
| 639 | limit?: number; | |
| 636 | 640 | } |
| 637 | 641 | |
| 638 | 642 | export function listComments( |
| @@ -662,9 +666,10 @@ export function listComments( | ||
| 662 | 666 | `SELECT comment.*, user.username AS author_name, heard.id AS heard_ok |
| 663 | 667 | FROM comment JOIN user ON user.id = comment.author_id |
| 664 | 668 | LEFT JOIN run heard ON heard.id = comment.heard_in AND heard.state = 'ok' |
| 665 | WHERE ${where} ORDER BY comment.id`, | |
| 669 | WHERE ${where}${options.after === undefined ? "" : " AND comment.id > ?2"} | |
| 670 | ORDER BY comment.id${options.limit === undefined ? "" : ` LIMIT ${Math.trunc(options.limit)}`}`, | |
| 666 | 671 | ) |
| 667 | .all(issueId); | |
| 672 | .all(...((options.after === undefined ? [issueId] : [issueId, options.after]) as [string])); | |
| 668 | 673 | return rows.map((row) => ({ |
| 669 | 674 | id: row.id, |
| 670 | 675 | issueId: row.issue_id, |
src/http/api.ts +23
| @@ -3241,6 +3241,29 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 3241 | 3241 | }); |
| 3242 | 3242 | }); |
| 3243 | 3243 | |
| 3244 | // A thread, a page at a time: oldest first, `next` is the `after` of the | |
| 3245 | // following page (#4150). A pull's number answers the same. | |
| 3246 | router.add("GET", "/api/v1/repos/:owner/:name/issues/:number/comments", (ctx) => { | |
| 3247 | const found = loadConversation(ctx); | |
| 3248 | if ("error" in found) return found.error; | |
| 3249 | const rawLimit = ctx.url.searchParams.get("limit"); | |
| 3250 | const limit = rawLimit === null ? 50 : /^\d{1,4}$/.test(rawLimit) ? Number(rawLimit) : 0; | |
| 3251 | if (limit < 1 || limit > 100) | |
| 3252 | return apiError("invalid_limit", 422, "limit is a whole number from 1 to 100"); | |
| 3253 | const after = ctx.url.searchParams.get("after"); | |
| 3254 | if (after !== null && !/^[A-Za-z0-9_-]{1,64}$/.test(after)) | |
| 3255 | return apiError("invalid_after", 422, "after is a comment id"); | |
| 3256 | const rows = listComments(ctx.app.db, found.issue.id, { | |
| 3257 | limit: limit + 1, | |
| 3258 | ...(after === null ? {} : { after }), | |
| 3259 | }); | |
| 3260 | const page = rows.slice(0, limit); | |
| 3261 | return json({ | |
| 3262 | comments: readerMarked(ctx, found.repo.id, page), | |
| 3263 | next: rows.length > limit ? page[page.length - 1]!.id : null, | |
| 3264 | }); | |
| 3265 | }); | |
| 3266 | ||
| 3244 | 3267 | // What an issue waits for, and what waits for it. |
| 3245 | 3268 | // The private repository a writer fixes a sealed report in by hand (#865). |
| 3246 | 3269 | router.add("POST", "/api/v1/repos/:owner/:name/issues/:number/private", async (ctx) => { |
test/issue-comments-paging.test.ts added +95
| @@ -0,0 +1,95 @@ | ||
| 1 | /** GET …/issues/{number}/comments pages a thread: {comments, next} (#4150). */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { createApp, type App } from "../src/app.ts"; | |
| 7 | import { addComment, openIssue } from "../src/actions/issues.ts"; | |
| 8 | import { createRepo } from "../src/actions/repos.ts"; | |
| 9 | import { createToken } from "../src/core/tokens.ts"; | |
| 10 | import { createUser } from "../src/core/users.ts"; | |
| 11 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 12 | import { quietLogs } from "./helpers/globals.ts"; | |
| 13 | ||
| 14 | quietLogs(); | |
| 15 | ||
| 16 | let dataDir: string; | |
| 17 | let app: App; | |
| 18 | let servers: Servers; | |
| 19 | let token = ""; | |
| 20 | let issueNumber = 0; | |
| 21 | ||
| 22 | const ok = <T>(r: { ok: true; value: T } | { ok: false; error: unknown }): T => { | |
| 23 | if (!r.ok) throw new Error(JSON.stringify(r.error)); | |
| 24 | return r.value; | |
| 25 | }; | |
| 26 | const get = (repo: string, number: number, query = "", auth?: string) => | |
| 27 | fetch(`${servers.url}/api/v1/repos/alice/${repo}/issues/${number}/comments${query}`, { | |
| 28 | headers: auth ? { authorization: `Bearer ${auth}` } : {}, | |
| 29 | }); | |
| 30 | ||
| 31 | beforeAll(async () => { | |
| 32 | dataDir = await mkdtemp(join(tmpdir(), "forge-comment-paging-")); | |
| 33 | app = createApp({ dataDir, port: 0 }); | |
| 34 | servers = await serve(app); | |
| 35 | const alice = ok( | |
| 36 | await createUser(app.db, { | |
| 37 | origin: "seed", | |
| 38 | trusted: true, | |
| 39 | username: "alice", | |
| 40 | email: "[email protected]", | |
| 41 | password: "a-good-passphrase", | |
| 42 | adminIfFirst: true, | |
| 43 | }), | |
| 44 | ); | |
| 45 | const repo = ok(await createRepo(app, alice, { name: "pub", visibility: "public" })); | |
| 46 | const issue = ok(await openIssue(app, alice, repo, { title: "Long", body: "" })); | |
| 47 | issueNumber = issue.number; | |
| 48 | for (let i = 0; i < 120; i++) ok(await addComment(app, alice, repo, issue, `c${i}`)); | |
| 49 | const secret = ok(await createRepo(app, alice, { name: "priv", visibility: "private" })); | |
| 50 | ok(await openIssue(app, alice, secret, { title: "Hidden", body: "" })); | |
| 51 | token = createToken(app.db, alice.id, "rw", "rw").token; | |
| 52 | }); | |
| 53 | ||
| 54 | afterAll(async () => { | |
| 55 | await servers.stop(); | |
| 56 | app.db.close(); | |
| 57 | await rm(dataDir, { recursive: true, force: true }); | |
| 58 | }); | |
| 59 | ||
| 60 | test("120 comments page in three calls, each once, in order", async () => { | |
| 61 | const seen: string[] = []; | |
| 62 | let after = ""; | |
| 63 | const sizes: number[] = []; | |
| 64 | for (let calls = 0; calls < 5; calls++) { | |
| 65 | const res = await get("pub", issueNumber, `?limit=50${after}`); | |
| 66 | expect(res.status).toBe(200); | |
| 67 | const page = (await res.json()) as { comments: { id: string; body: string }[]; next: string | null }; | |
| 68 | sizes.push(page.comments.length); | |
| 69 | seen.push(...page.comments.map((c) => c.body)); | |
| 70 | if (page.next === null) break; | |
| 71 | after = `&after=${page.next}`; | |
| 72 | } | |
| 73 | expect(sizes).toEqual([50, 50, 20]); | |
| 74 | expect(seen).toEqual(Array.from({ length: 120 }, (_, i) => `c${i}`)); | |
| 75 | }); | |
| 76 | ||
| 77 | test("the default page is 50", async () => { | |
| 78 | const page = (await (await get("pub", issueNumber)).json()) as { comments: unknown[]; next: string }; | |
| 79 | expect(page.comments.length).toBe(50); | |
| 80 | expect(page.next).toBeTruthy(); | |
| 81 | }); | |
| 82 | ||
| 83 | test("bad limit or after is a 422", async () => { | |
| 84 | for (const q of ["?limit=0", "?limit=101", "?limit=x", "?limit=-1", "?after=a%20b"]) | |
| 85 | expect((await get("pub", issueNumber, q)).status).toBe(422); | |
| 86 | }); | |
| 87 | ||
| 88 | test("a private repository is not found to a stranger", async () => { | |
| 89 | expect((await get("priv", 1)).status).toBe(404); | |
| 90 | expect((await get("priv", 1, "", token)).status).toBe(200); | |
| 91 | }); | |
| 92 | ||
| 93 | test("a number that is not there is not found", async () => { | |
| 94 | expect((await get("pub", 999)).status).toBe(404); | |
| 95 | }); |
new above
offline
Nit, not blocking:
LIMITis built into the SQL string from a number. The handler validates it, so this is safe today. ButlistCommentsis a core function, and a later caller that passes NaN gets an SQL error back instead of a clean failure. Binding it asLIMIT ?3, or clamping withMath.max(1, Math.min(…))here, would keep the core function safe whatever its caller passes.