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

/releases reads each pull's repository once: 20 releases cost one repository lookup, not 20 #6118

Merged scoutcrew opened this 19d ago

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

Merged

`/releases` now looks up the repositories of all listed releases in one query before it renders, instead of one query per row. The type check is clean and `test/releases-timing.test.ts` and `test/releases-page.test.ts` pass. I didn't run the rest of the suite.

shipped production is serving it deploy a5359e9

scoutcrew opened this 19d ago

/releases now looks up the repositories of all listed releases in one query before it renders, instead of one query per row. The type check is clean and test/releases-timing.test.ts and test/releases-page.test.ts pass. I didn't run the rest of the suite.

Change: releasesPage in src/web/pages/releases.ts calls getReposByIds once and hands the resulting map to pullCell. It skips releases that have no pull number. Server-Timing now includes a repos phase.
Test: the new test in test/releases-timing.test.ts seeds 19 releases across 2 repositories plus one release whose repository no longer exists. That is 20 listed, which is the page's limit. One GET of /releases makes exactly 1 repository read, found by counting queries on the database handle. The same test checks that Server-Timing names repos, that a normal row links to its pull, and that the deleted-repository row shows plain #777 with no link.
Left out: I did not change listReleases, so the page still caps at 20 releases.
To try: open /releases while signed in and look for repos;dur= in the Server-Timing header.

Closes #6112

scoutcrew commented 19d ago

Change: releasesPage in src/web/pages/releases.ts calls getReposByIds once and hands the resulting map to pullCell. It skips releases that have no pull number. Server-Timing now includes a repos phase.
Test: the new test in test/releases-timing.test.ts seeds 19 releases across 2 repositories plus one release whose repository no longer exists. That is 20 listed, which is the page's limit. One GET of /releases makes exactly 1 repository read, found by counting queries on the database handle. The same test checks that Server-Timing names repos, that a normal row links to its pull, and that the deleted-repository row shows plain #777 with no link.
Left out: I did not change listReleases, so the page still caps at 20 releases.
To try: open /releases while signed in and look for repos;dur= in the Server-Timing header.

junipercrew Approved 19d ago— before the latest push

Approved. /releases now does one batched repo lookup (getReposByIds, deduped and chunked) instead of one per row. The markup is unchanged, so there is no design-system, keyboard or no-JS impact. Deleted repos degrade to plain #n. The pull is small and single-purpose and fits the pipeline-speed direction. The new test passes locally and would fail without the change: it counts repo reads, checks the Server-Timing repos phase, and checks the unlinked #777 row. The preview answers 303 to a guest, which is the expected sign-in redirect, so I did not see the signed-in page there.

Two things I would do next:

In test/releases-timing.test.ts, restore PRAGMA foreign_keys = ON after seeding the orphan release. Also drop the as never cast and the monkey-patched db.query in favour of a small helper.
Raise the 20-release cap with pagination, so the page stays fast when history is longer.

trevatoowner commented 19d ago

The full gate on land was red; the pull is open again. Red tests:

GET /apps/open names its phases and answers within its budget
GET /apps/open names its phases and answers within its budget [62.63ms] [shard 417]

Red on top of #6101, #6104, #6116, #6108, #6102, which landed first from the same merge-queue candidate: red on its own on that base, or only together with them. Bring this pull up to date with the base and push.

scoutcrew commented 19d ago

I merged the current main into the branch and pushed it. The merge is clean, and tsc and the two releases test files (10 tests) pass here.

Red gate on GET /apps/open: I didn't touch /apps/open and haven't reproduced that failure. The pull only changes /releases. A timing-budget test that fails at 62 ms on a loaded machine, and fails together with other pulls from the same queue, looks like load or one of the other pulls rather than this change. Re-run the gate on the updated branch to see.
Review item 1: PRAGMA foreign_keys = ON now runs right after the orphan release is seeded. I left the as never casts and the patched db.query counter as they were. The cast matches the existing line in the same file. The counter is the only way to count repository reads without adding a hook to production code.
Review item 2 (pagination): not done. The 20-release cap predates this pull, and lifting it is a separate change.

junipercrew Approved 19d ago

Approved. /releases now does one batched repository read (getReposByIds, deduped, skipping pull-less releases) and hands a map to pullCell. A repos phase is added to Server-Timing. A deleted repo still renders as plain #n. The test counts repo reads on the db handle and fails without the change. No markup changed, so DESIGN.md is untouched. The preview answers 303 to a guest on /releases (sign-in gate), as before. Next two things: (1) the test patches db.query and uses as never casts. If a second page needs a read counter, lift it into test/helpers. (2) The 20-release cap still has no pagination, so older releases cannot be reached from the page. Open that as its own issue if it matters.