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

The open pulls list reads every row's next step in one batch: /trevato/forge/pulls renders in under 15 ms (60–82 ms today) #8395

Merged opened this

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

Merged

The pulls list (`/:owner/:name/pulls`) now reads every row's next step in one batch before it renders. Before, the render looked up each row's step with its own database reads. The batch is a new `stepsOfStored(db, repo, pulls)` in `src/flow/path.ts`, timed as its own `steps` phase between `labels` and `render`, so `render` only renders.

shipped production is serving it deploy b2c98db

opened this

The pulls list (/:owner/:name/pulls) now reads every row's next step in one batch before it renders. Before, the render looked up each row's step with its own database reads. The batch is a new stepsOfStored(db, repo, pulls) in src/flow/path.ts, timed as its own steps phase between labels and render, so render only renders.

Speed: with 50 open pulls on my machine, the batch takes 1.8 ms (best of 5); the old per-row reads took 29.1 ms. Whether render drops under 15 ms on production can only be checked after this lands.

What the batch does: it reads each kind of fact once for the whole page, mostly with batch readers that already existed (checks on each pull's head, reviews, holds, wall decisions). Two pieces are new:

  • latestProofRunsFor in src/core/proof_runs.ts reads the latest proof run for every pull in one query.
  • For landed pulls, "live" is now decided from one read of when production last started serving a deploy, instead of one query per pull. The answer is the same.

The wording of the step is unchanged. proofWordsOf (next_actor.ts) and wallWordsOf (wall_wait.ts) are the old sentence-building code split out so the batch can pass in facts it has already read. proofWords and wallWords still call them, so the pull page keeps stepOfStored and says exactly what it said before.

Tests: in the new test/path-steps-batch.test.ts:

  • One test builds 25 pulls (open, merged before and after a production deploy, merged into a branch nothing deploys from, and closed). It varies every fact the step reads and checks each pull's batch step against stepOfStored.
  • Another asserts that 50 open pulls read their steps in under 10 ms.

I changed one existing test: test/pulls-list-timing.test.ts listed the page's phases in order without steps. The issue asks for that phase, so the test was out of date, not the code.

Checks run: bun scripts/gate.ts, bun x tsc --noEmit and bun scripts/design-lint.ts pass. scripts/fast.ts named 1494 test files, which is over its time budget, so I picked the ones that cover this change: the new test plus pipeline-path, pull-step-surfaces, tui-pull-step, next_actor, wall-card, wall-doors, proof-runs, merge-queue-proof and pulls-list-timing. All 71 tests pass.

Not done: the pull page still uses one stepOfStored per pull, as the issue's Risks note asks.

To try it: open /trevato/forge/pulls on the preview and look for the steps phase in its timing.

🤖 Generated with Claude Code

Closes #8387

commented

The pulls list (/:owner/:name/pulls) now reads every row's next step in one batch before it renders. Before, the render looked up each row's step with its own database reads. The batch is a new stepsOfStored(db, repo, pulls) in src/flow/path.ts, timed as its own steps phase between labels and render, so render only renders.

Speed: with 50 open pulls on my machine, the batch takes 1.8 ms (best of 5); the old per-row reads took 29.1 ms. Whether render drops under 15 ms on production can only be checked after this lands.

What the batch does: it reads each kind of fact once for the whole page, mostly with batch readers that already existed (checks on each pull's head, reviews, holds, wall decisions). Two pieces are new:

  • latestProofRunsFor in src/core/proof_runs.ts reads the latest proof run for every pull in one query.
  • For landed pulls, "live" is now decided from one read of when production last started serving a deploy, instead of one query per pull. The answer is the same.

The wording of the step is unchanged. proofWordsOf (next_actor.ts) and wallWordsOf (wall_wait.ts) are the old sentence-building code split out so the batch can pass in facts it has already read. proofWords and wallWords still call them, so the pull page keeps stepOfStored and says exactly what it said before.

Tests: in the new test/path-steps-batch.test.ts:

  • One test builds 25 pulls (open, merged before and after a production deploy, merged into a branch nothing deploys from, and closed). It varies every fact the step reads and checks each pull's batch step against stepOfStored.
  • Another asserts that 50 open pulls read their steps in under 10 ms.

I changed one existing test: test/pulls-list-timing.test.ts listed the page's phases in order without steps. The issue asks for that phase, so the test was out of date, not the code.

Checks run: bun scripts/gate.ts, bun x tsc --noEmit and bun scripts/design-lint.ts pass. scripts/fast.ts named 1494 test files, which is over its time budget, so I picked the ones that cover this change: the new test plus pipeline-path, pull-step-surfaces, tui-pull-step, next_actor, wall-card, wall-doors, proof-runs, merge-queue-proof and pulls-list-timing. All 71 tests pass.

Not done: the pull page still uses one stepOfStored per pull, as the issue's Risks note asks.

To try it: open /trevato/forge/pulls on the preview and look for the steps phase in its timing.

🤖 Generated with Claude Code

commented

The open pulls list now gets the step for every row from one batch, stepsOfStored in src/flow/path.ts. Each fact (reviews, checks for the current head, holds, proof runs, access-wall decisions) is read once for the whole page instead of once per pull. Merged pulls share one check of whether the repository has an app and when production last served a deploy. Each step is worded exactly as stepOfStored words it for a single pull.

The check failed because test/path-steps-batch.test.ts used a stopwatch: "fifty pulls in under 10 ms". The rule from #4429 and #7824 forbids timing assertions in tests, and the test was wrong, not the code. It now counts SQL statements with statements() and expectWork(). Reading steps for fifty open pulls must run exactly as many statements as reading them for five, and no more than six. Six is today's count, so the cap is tight, and a per-row query anywhere in the batch would fail it on every run.

The other test in the file is unchanged. It builds open, merged and closed pulls that cover every branch (check, review, land, live, a hold, an access wall, a waiting proof) and asserts that the batch matches stepOfStored for each one.

The gate, tsc --noEmit, test/path-steps-batch.test.ts and test/flake.test.ts, which carries the rule that failed, all pass locally.

To try it: bun test test/path-steps-batch.test.ts.

🤖 Generated with Claude Code

commented

The open pulls list now gets every row's step from one batch, stepsOfStored in src/flow/path.ts. Reviews, checks for the current head, holds and access-wall decisions are each read once for the whole page instead of once per pull. Merged pulls share one lookup of whether the repository has an app and when production last served a deploy. Each step is worded exactly as stepOfStored words it for a single pull.

Merge with main: main removed the proof step (#8301). I kept main's src/flow/next_actor.ts and its deletion of src/core/proof_runs.ts, which drops the batched proof read this branch had added there. I also took the proof read out of stepsOfStored and the proof fixtures out of its test.

Tests: test/path-steps-batch.test.ts builds open, merged and closed pulls covering every step (check, review, land, live, a hold, an access wall). It checks that the batch matches stepOfStored for each one.

The second test counts SQL statements rather than timing anything: fifty open pulls must cost exactly what five do, and no more than five statements. Five is today's count, so any per-row query fails it on every run. The first version of this test used a stopwatch, which the rule from #4429 refuses; that test was wrong, not the code.

Run locally: bun scripts/gate.ts, bun x tsc --noEmit, and the seven test files that cover the change all pass (path-steps-batch, pipeline-path, pull-step-surfaces, next_actor, tui-pull-step, merge-queue-no-proof, flake). bun scripts/fast.ts named 1616 files because the merge widens the diff, so I picked these by hand.

To try it: bun test test/path-steps-batch.test.ts.

🤖 Generated with Claude Code

Approved

Approved. stepsOfStored reads reviews, checks, holds and wall decisions once per page, and merged pulls share one lastServed read. I compared it with nextOfPull/stepOfPull: the inputs and wording are the same (conflicted:false matches stepOfStored). The test compares the batch with stepOfStored over open, merged and closed pulls, and counts statements instead of timing. The pulls-list timing test now expects the steps phase. No page markup changed, so the design system is untouched. Caveat: the preview has no trevato/forge repo (404), so I could not see the steps phase or the <15 ms figure there. Two things to do next: (1) once this lands, check render on production /trevato/forge/pulls against the 15 ms target; (2) move review_page.ts and the feed step list onto the batch if they show per-row cost.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs