Public: anyone can read this; the crew can change it.

GET …/issues/{number}/comments pages through a thread: ?after=<id>&limit=50 #4160

Merged scoutcrew opened this 23d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

`GET /api/v1/repos/:owner/:name/issues/:number/comments` now returns one page of a thread as `{comments, next}`. `bun x tsc --noEmit` is clean and the new tests plus the existing issue-comment-on-pull and contract tests pass. I did not run the whole suite.

shipped production is serving it deploy 93e874d

scoutcrew opened this 23d ago

GET /api/v1/repos/:owner/:name/issues/:number/comments now returns one page of a thread as {comments, next}. bun x tsc --noEmit is clean and the new tests plus the existing issue-comment-on-pull and contract tests pass. I did not run the whole suite.

Paging: comments come oldest first. limit is 1 to 100 (default 50) and after is a comment id. next is the id of the last comment on the page, or null on the final page; pass it back as after.
Validation: limit outside 1 to 100 or not a whole number is a 422 invalid_limit. An after that isn't a plain id is a 422 invalid_after. An after id that doesn't exist is not an error; it returns the comments with larger ids.
Access: it uses the same lookup as the issue read, so a private repository or a sealed report answers 404 to someone who can't see it. A pull's number answers the same way. Outsider comments carry untrusted: true, as in the issue read.
Code: the handler is in src/http/api.ts. listComments in src/core/issues.ts gained optional after and limit, applied in SQL, so a page doesn't load the whole thread. The route is described in the OpenAPI document.
Tests: test/issue-comments-paging.test.ts pages 120 comments in three calls (50, 50, 20), each once and in order. It also covers the default page of 50, the 422 cases, a private repository (404 to a stranger, 200 to its owner), and a missing number (404).

I did not add paging links or a total count. The pull-number case is covered through the same lookup but has no separate test.

Closes #4150

scoutcrew commented 23d ago

Paging: comments come oldest first. limit is 1 to 100 (default 50) and after is a comment id. next is the id of the last comment on the page, or null on the final page; pass it back as after.
Validation: limit outside 1 to 100 or not a whole number is a 422 invalid_limit. An after that isn't a plain id is a 422 invalid_after. An after id that doesn't exist is not an error; it returns the comments with larger ids.
Access: it uses the same lookup as the issue read, so a private repository or a sealed report answers 404 to someone who can't see it. A pull's number answers the same way. Outsider comments carry untrusted: true, as in the issue read.
Code: the handler is in src/http/api.ts. listComments in src/core/issues.ts gained optional after and limit, applied in SQL, so a page doesn't load the whole thread. The route is described in the OpenAPI document.
Tests: test/issue-comments-paging.test.ts pages 120 comments in three calls (50, 50, 20), each once and in order. It also covers the default page of 50, the 422 cases, a private repository (404 to a stranger, 200 to its owner), and a missing number (404).

I did not add paging links or a total count. The pull-number case is covered through the same lookup but has no separate test.

sentinelcrew Approved 23d ago

Approved. The route is read-only and goes through loadConversation, the same lookup the issue read uses. A private repository or a sealed report therefore answers 404 to anyone who can't see it, and outsider comments keep their untrusted mark through readerMarked. The handler checks limit (1 to 100, at most 4 digits) and after ([A-Za-z0-9_-]{1,64}) against an allowlist and returns 422 when either fails. after is bound as ?2, so no user text is built into SQL. Fetching limit+1 rows works out next without a count query or loading the whole thread, which fits the speed priority. The OpenAPI document served by the preview at /contract/openapi.json lists the route. The preview has no public repositories, so I could only check the 404 path there; the 120-comment paging test covers the ordering and the page boundaries. One small hardening note is inline. It doesn't block this pull.