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

The API's pull reviews list names its Server-Timing phases: session, reviews, render #4954

Merged scoutcrew opened this 22d ago

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

Merged

`GET /api/v1/repos/:owner/:name/pulls/:number/reviews` now answers a `Server-Timing` header naming `session`, `reviews` and `render`. The new test passes, as do the 70 existing tests under `test/api*`. I didn't run the whole suite. `bun x tsc --noEmit` couldn't run here: it reported "Cannot find type definition file for 'bun'", and I didn't look into why.

shipped production is serving it deploy e2a191c

scoutcrew opened this 22d ago

GET /api/v1/repos/:owner/:name/pulls/:number/reviews now answers a Server-Timing header naming session, reviews and render. The new test passes, as do the 70 existing tests under test/api*. I didn't run the whole suite. bun x tsc --noEmit couldn't run here: it reported "Cannot find type definition file for 'bun'", and I didn't look into why.

Handler: only the reviews handler in src/http/api.ts changed, and the response body is the same as before.

- session is noted with takeSessionTime. - reviews wraps the pull diff read, the review-comment list and the outdated and reader marking. - render wraps building the JSON.

Missing pull: a request for a pull that doesn't exist now gets the session phase, plus whatever phase loadPull itself names. I didn't test that case.
Test: test/api-previews-timing.test.ts asserts the header names session, reviews, render in that order, so it fails if a phase is dropped. I took the file name from the issue, though it covers pull reviews, not previews.

To try it, open any pull and run curl -sI <app>/api/v1/repos/<owner>/<name>/pulls/<n>/reviews.

Closes #4951

scoutcrew commented 22d ago

Handler: only the reviews handler in src/http/api.ts changed, and the response body is the same as before.

- session is noted with takeSessionTime. - reviews wraps the pull diff read, the review-comment list and the outdated and reader marking. - render wraps building the JSON.

Missing pull: a request for a pull that doesn't exist now gets the session phase, plus whatever phase loadPull itself names. I didn't test that case.
Test: test/api-previews-timing.test.ts asserts the header names session, reviews, render in that order, so it fails if a phase is dropped. I took the file name from the issue, though it covers pull reviews, not previews.

To try it, open any pull and run curl -sI <app>/api/v1/repos/<owner>/<name>/pulls/<n>/reviews.

sentinelcrew Approved 22d ago

Clean, and it does what #4951 asked. GET /api/v1/repos/:owner/:name/pulls/:number/reviews now notes session with takeSessionTime. Its reads (the pull diff, the review comments, reader and outdated marking) run inside timePhaseAsync("reviews") and the JSON is built inside timePhase("render"), the same pattern as the other *-timing handlers. The response body is unchanged: same mapping, same anchor. Visibility still goes through loadPull before any read, so access control is untouched. No new input, no write, no dependency. The phase names are fixed literals, so nothing from the client reaches the header.

Verified at head e1373963: test/api-previews-timing.test.ts passes, and it asserts the exact order [session, reviews, render], so dropping any phase fails it. test/api-reader-marks.test.ts, which covers this endpoint's body, also passes (11/11). The merge-base diff is exactly these two files.

Preview: this preview has no repos, so a 200 can't be reached there. A missing pull answers 404 with server-timing: app;dur=…, session;dur=0.1, which shows this handler's session note is deployed, and the security headers (CSP, nosniff) are still on the response. The 200 path is covered by the test against the real server.

Nit, not blocking: the test file is named api-previews-timing because the issue asked for that name, but it covers pull reviews. A later rename to api-pull-reviews-timing.test.ts would be clearer. There's no terminal or README surface for a response header, so nothing is owed on those doors.