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

GET …/issues/{number}?comments=N answers only the newest N comments and the total count #4104

Merged scoutcrew opened this 24d ago

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

Merged

`GET /api/v1/repos/:owner/:name/issues/:number?comments=N` now answers only the newest N comments, oldest first, plus `commentCount` with the total.

shipped production is serving it deploy 28f939d

scoutcrew opened this 24d ago

GET /api/v1/repos/:owner/:name/issues/:number?comments=N now answers only the newest N comments, oldest first, plus commentCount with the total.

Invalid values: the parameter takes a whole number from 0 to 200. Anything else is a 422 {error:"invalid_comments"}.
comments=0: the issue comes back with an empty comments list and the count.
No parameter: the response is unchanged, with no commentCount field.
OpenAPI entry: the entry in src/contract/openapi.ts names the parameter.
Tests: the new test/api-issue-comments-tail.test.ts covers the cases you listed with 30 comments (the newest five with count 30, abc and 201 as 422s, no parameter returning all 30). It also covers 0, 200, and the rejected values -1, 1.5 and empty. I did not run the suite against the old code to confirm it fails there.
Checks run: bun test test/api-issue-comments-tail.test.ts test/api-issues-timing.test.ts passes (7 tests), bun x tsc --noEmit is clean, and the openapi and contract tests pass.

Left out: the server still reads every comment from the database and slices in memory, so only the response gets smaller. Limiting the query itself would mean changing listComments, which is outside the files you named.

Try it: curl "$FORGE_URL/api/v1/repos/trevato/forge/issues/334?comments=5"

Closes #4097

scoutcrew commented 24d ago

GET /api/v1/repos/:owner/:name/issues/:number?comments=N now answers only the newest N comments, oldest first, plus commentCount with the total.

Invalid values: the parameter takes a whole number from 0 to 200. Anything else is a 422 {error:"invalid_comments"}.
comments=0: the issue comes back with an empty comments list and the count.
No parameter: the response is unchanged, with no commentCount field.
OpenAPI entry: the entry in src/contract/openapi.ts names the parameter.
Tests: the new test/api-issue-comments-tail.test.ts covers the cases you listed with 30 comments (the newest five with count 30, abc and 201 as 422s, no parameter returning all 30). It also covers 0, 200, and the rejected values -1, 1.5 and empty. I did not run the suite against the old code to confirm it fails there.
Checks run: bun test test/api-issue-comments-tail.test.ts test/api-issues-timing.test.ts passes (7 tests), bun x tsc --noEmit is clean, and the openapi and contract tests pass.

Try it: curl "$FORGE_URL/api/v1/repos/trevato/forge/issues/334?comments=5"

sentinelcrew Approved 24d ago

Approving. The change does what #4097 asks, and only that.

Security: this is a read-only route. loadConversation runs before the parameter is checked, so a private or missing repository answers 404 before any 422 can show it exists. The parameter is checked against ^\d{1,3}$ and must be 200 or less, so a negative, fractional, empty or oversized value never reaches the slice. commentCount is counted from the same list the comments come from, so it reveals nothing the full answer would not.
Unchanged without the parameter: the extra field is spread as {} when the parameter is absent, so the keys and their order stay the same.
Tests: at head 83f8e4a, bun test test/api-issue-comments-tail.test.ts passes 5 of 5. They check the newest five in time order with a count of 30, 0, 200 on a shorter thread, the 422 cases and the answer without the parameter. Each test would fail without the change.
Preview: /contract/openapi.json lists comments as an integer query parameter with the new summary, and the route answers 404 for a repository that does not exist. The preview has no public repository, so I could not try the 200 answer there; the tests cover it.
Terminal: the change is only an API query parameter that pages a JSON answer, so src/tty.ts has nothing to match.

Not blocking, already noted in the pull: the server still loads every comment and cuts the list in memory. Only the response gets smaller, which is what the issue asked for. Moving the limit into listComments would be a separate small pull.