trevato/forge

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

More
Forks1g fMapg aRuns14g rEventsFlows

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 scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/8387-the-open-pulls-list-reads-every-row-s-ne → main 3 commits fork

  • Merged. scout/8387-the-open-pulls-list-reads-every-row-s-ne was merged into main as 32d2e5c.

Review

Sign in to review this change.
5 files changed +234 −14
src/flow/path.ts +68 −3
@@ -13,11 +13,14 @@
1313
1414 import type { Database } from "bun:sqlite";
1515 import { getAppByRepo } from "../core/apps.ts";
16 import { latestDecisionsFor } from "../core/decisions.ts";
17 import { holdsFor } from "../core/holds.ts";
1618 import { getPull, type Pull } from "../core/pulls.ts";
1719 import type { Repo } from "../core/repos.ts";
18 import { reviewSummary } from "../core/reviews.ts";
19 import { checkSummary, type RunState } from "../core/runs.ts";
20 import { nextOfPull, type Next } from "./next_actor.ts";
20 import { reviewSummariesFor, reviewSummary } from "../core/reviews.ts";
21 import { checksForHeads, checkSummary, type RunState } from "../core/runs.ts";
22 import { nextActor, nextOfPull, type Next } from "./next_actor.ts";
23 import { wallWordsOf } from "./wall_wait.ts";
2124
2225 export const STEPS = ["check", "review", "land", "live"] as const;
2326 export type Step = (typeof STEPS)[number];
@@ -142,3 +145,65 @@ export function stepOfNumber(db: Database, repo: Repo, number: number): PathStep
142145 const pull = getPull(db, repo.id, number);
143146 return pull === undefined ? null : stepOfStored(db, repo, pull);
144147 }
148
149 /** When production for `repo` last started serving a deploy; null when it never has. */
150 function lastServed(db: Database, repoId: string): string | null {
151 return (
152 db
153 .query<{ at: string | null }, [string]>(
154 `SELECT MAX(created_at) AS at FROM run
155 WHERE repo_id = ?1 AND kind = 'deploy' AND env = 'production' AND serving_at IS NOT NULL`,
156 )
157 .get(repoId)?.at ?? null
158 );
159 }
160
161 /**
162 * `stepOfStored` for every pull of a list at once, by pull id: each fact is
163 * read in one query for the whole page rather than once per row, so a list
164 * renders without touching the database (#8387). Same steps, same words.
165 */
166 export function stepsOfStored(db: Database, repo: Repo, pulls: readonly Pull[]): Map<string, PathStep | null> {
167 const out = new Map<string, PathStep | null>();
168 const open = pulls.filter((p) => p.mergedAt === null && p.state === "open");
169 const ids = open.map((p) => p.id);
170 const reviews = reviewSummariesFor(db, open);
171 const checks = checksForHeads(db, new Map(open.map((p) => [p.id, p.headOid])));
172 const holds = holdsFor(db, ids);
173 const walls = latestDecisionsFor(db, ids, "land");
174 for (const pull of open) {
175 const r = reviews.get(pull.id)!;
176 const check = checks.get(pull.id)?.[0]?.state ?? "none";
177 const hold = holds.get(pull.id);
178 const next = nextActor({
179 held: hold === undefined ? null : { by: hold.heldByName, why: hold.why },
180 conflicted: false,
181 check,
182 changesRequested: r.changesRequested,
183 approvals: r.approvals,
184 pushedSince: r.stale,
185 wall: wallWordsOf(walls.get(pull.id), pull.headOid),
186 });
187 out.set(pull.id, pathStep({ state: "open", check, approvals: r.approvals, next }));
188 }
189 const merged = pulls.filter((p) => p.mergedAt !== null);
190 const toDefault = merged.some((p) => p.baseBranch === repo.defaultBranch);
191 const hasApp = toDefault && getAppByRepo(db, repo.id) !== undefined;
192 const served = hasApp ? lastServed(db, repo.id) : null;
193 for (const pull of merged) {
194 const deploys = hasApp && pull.baseBranch === repo.defaultBranch;
195 out.set(
196 pull.id,
197 pathStep({
198 state: "merged",
199 check: "ok",
200 approvals: 0,
201 next: null,
202 deploys,
203 liveSince: deploys && served !== null && served >= pull.mergedAt!,
204 }),
205 );
206 }
207 for (const pull of pulls) if (!out.has(pull.id)) out.set(pull.id, null);
208 return out;
209 }
src/flow/wall_wait.ts +6 −2
@@ -6,14 +6,18 @@
66 */
77
88 import type { Database } from "bun:sqlite";
9 import { latestDecision } from "../core/decisions.ts";
9 import { latestDecision, type Decision } from "../core/decisions.ts";
1010
1111 const strings = (v: unknown): string[] =>
1212 Array.isArray(v) ? v.filter((c): c is string => typeof c === "string") : [];
1313
1414 /** What the wall holds `headOid` for, in plain words; null when it holds nothing there. */
1515 export function wallWords(db: Database, pullId: string, headOid: string): string[] | null {
16 const d = latestDecision(db, pullId, "land");
16 return wallWordsOf(latestDecision(db, pullId, "land"), headOid);
17 }
18
19 /** `wallWords` from the pull's latest `land` decision, already read: a list reads them all at once. */
20 export function wallWordsOf(d: Decision | undefined, headOid: string): string[] | null {
1721 if (d === undefined || d.answer !== "wall" || d.outcome !== null) return null;
1822 const state = d.state as { head?: unknown; plain?: unknown; cells?: unknown } | null;
1923 if (state === null || typeof state !== "object" || state.head !== headOid) return null;
src/web/studio/lists.ts +7 −7
@@ -9,7 +9,6 @@
99 * `/pulls`.
1010 */
1111
12 import type { Database } from "bun:sqlite";
1312 import { sealedScope } from "../../core/sealed.ts";
1413 import { appsRuntime } from "../../apps/runtime.ts";
1514 import { getAppByRepo } from "../../core/apps.ts";
@@ -20,8 +19,8 @@ import {
2019 type Issue,
2120 type Label,
2221 } from "../../core/issues.ts";
23 import { listPulls, openPullsClosing, type Pull } from "../../core/pulls.ts";
24 import { stepOfStored } from "../../flow/path.ts";
22 import { listPulls, openPullsClosing } from "../../core/pulls.ts";
23 import { stepsOfStored, type PathStep } from "../../flow/path.ts";
2524 import { canRead, canWrite, type Repo } from "../../core/repos.ts";
2625 import { FLOW_LABEL } from "../../core/flow_label.ts";
2726 import { flowButton } from "../flow_button.ts";
@@ -490,9 +489,8 @@ export async function tellsList(ctx: Ctx): Promise<Response> {
490489 }
491490
492491 /** The pull's step beside its row, as a sibling of the row's link: `after`. */
493 function stepAfter(db: Database, repo: Repo, pull: Pull): { after?: Html } {
494 const step = stepOfStored(db, repo, pull);
495 return step === null ? {} : { after: stepLink(step) };
492 function stepAfter(step: PathStep | null | undefined): { after?: Html } {
493 return step == null ? {} : { after: stepLink(step) };
496494 }
497495
498496 /**
@@ -534,6 +532,8 @@ export async function changesList(ctx: Ctx): Promise<Response> {
534532 const rows = fetched
535533 .map((pull, i) => ({ pull, info: words[i]! }))
536534 .filter((r) => shown(r.info.word, f));
535 const steps = stepsOfStored(db, repo, rows.map((r) => r.pull));
536 lap("steps");
537537
538538 const list =
539539 rows.length === 0
@@ -561,7 +561,7 @@ export async function changesList(ctx: Ctx): Promise<Response> {
561561 : pull.authorName} · ${info.note} ·
562562 ${when(answered.get(pull.number), (t) => html`${tellLine(base, t)} · `)}${time(pull.updatedAt)}`,
563563 trail: wordBadge(info.word),
564 ...stepAfter(db, repo, pull),
564 ...stepAfter(steps.get(pull.id)),
565565 }),
566566 ),
567567 );
test/path-steps-batch.test.ts added +151
@@ -0,0 +1,151 @@
1 /** The pulls list reads every row's step in one batch (#8387): the same step `stepOfStored` gives each pull, read fast. */
2 import { afterAll, beforeAll, expect, test } from "bun:test";
3 import { mkdtemp, rm } from "node:fs/promises";
4 import { tmpdir } from "node:os";
5 import { join } from "node:path";
6 import { createApp, type App } from "../src/app.ts";
7 import { createRepo } from "../src/actions/repos.ts";
8 import { createApp as createAppRecord } from "../src/core/apps.ts";
9 import { recordDecision } from "../src/core/decisions.ts";
10 import { setHold } from "../src/core/holds.ts";
11 import { createIssue, setIssueState } from "../src/core/issues.ts";
12 import { getPull, type Pull } from "../src/core/pulls.ts";
13 import type { Repo } from "../src/core/repos.ts";
14 import { submitReview } from "../src/core/reviews.ts";
15 import { createRun } from "../src/core/runs.ts";
16 import { createUser, type User } from "../src/core/users.ts";
17 import { stepOfStored, stepsOfStored } from "../src/flow/path.ts";
18 import { quietLogs } from "./helpers/globals.ts";
19 import { statements } from "./helpers/statements.ts";
20 import { expectWork } from "./helpers/work.ts";
21
22 quietLogs();
23
24 let dataDir = "";
25 let app: App;
26 let alice: User, bob: User;
27 let repo: Repo;
28
29 const user = async (username: string): Promise<User> => {
30 const made = await createUser(app.db, {
31 origin: "seed",
32 trusted: true,
33 username,
34 email: `${username}@forge.local`,
35 password: "a-good-passphrase",
36 adminIfFirst: true,
37 });
38 if (!made.ok) throw new Error(String(made.error));
39 return made.value;
40 };
41
42 const oid = (n: number, salt = "a"): string => `${salt}${n.toString(16)}`.padEnd(40, "0");
43
44 /** A pull row written straight to the database: the step reads rows, not the repository. */
45 function pull(n: number, base = "main"): Pull {
46 const issue = createIssue(app.db, { repoId: repo.id, authorId: alice.id, title: `p${n}`, kind: "pull" });
47 if (!issue.ok) throw new Error(String(issue.error));
48 app.db
49 .query(
50 `INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid, head_repo_id)
51 VALUES (?1, ?2, ?3, ?4, ?5, ?6)`,
52 )
53 .run(issue.value.id, base, `b${n}`, oid(0, "b"), oid(n), repo.id);
54 return getPull(app.db, repo.id, issue.value.number)!;
55 }
56
57 const setRun = (id: string, state: string): void => {
58 app.db.query("UPDATE run SET state = ?1 WHERE id = ?2").run(state, id);
59 };
60
61 const reread = (p: Pull): Pull => getPull(app.db, repo.id, p.number)!;
62
63 beforeAll(async () => {
64 dataDir = await mkdtemp(join(tmpdir(), "forge-steps-"));
65 app = createApp({ dataDir, port: 0 });
66 alice = await user("alice");
67 bob = await user("bob");
68 const made = await createRepo(app, alice, { name: "steps", visibility: "public" });
69 if (!made.ok) throw new Error(String(made.error));
70 repo = made.value;
71 const record = createAppRecord(app.db, { repoId: repo.id, slug: "steps", access: "public" });
72 if (!record.ok) throw new Error(record.error);
73 });
74
75 afterAll(async () => {
76 app.close();
77 await rm(dataDir, { recursive: true, force: true, maxRetries: 20, retryDelay: 100 });
78 });
79
80 test("the batch gives each open, merged and closed pull the same step as stepOfStored", () => {
81 const pulls: Pull[] = [];
82 for (let n = 1; n <= 24; n++) {
83 const p = pull(n, n % 12 === 11 ? "other" : "main");
84 const head = p.headOid;
85 // Every fact the step reads, varied across the rows.
86 if (n % 6 !== 0) {
87 const run = createRun(app.db, { repoId: repo.id, kind: "check", issueId: p.id, commitOid: head });
88 setRun(run.id, (["queued", "running", "ok", "failed", "cancelled"] as const)[n % 5]!);
89 }
90 // An older check on another head never counts.
91 if (n % 4 === 0) setRun(createRun(app.db, { repoId: repo.id, kind: "check", issueId: p.id, commitOid: oid(n, "c") }).id, "failed");
92 if (n % 3 === 0)
93 submitReview(app.db, { issueId: p.id, authorId: bob.id, state: "approved", commitOid: head });
94 if (n % 7 === 0)
95 submitReview(app.db, { issueId: p.id, authorId: bob.id, state: "changes_requested", commitOid: n % 2 ? head : oid(n, "c") });
96 if (n % 8 === 0) setHold(app.db, p.id, alice.id, "waiting on a call");
97 if (n % 9 === 0)
98 recordDecision(app.db, {
99 kind: "land",
100 repoId: repo.id,
101 issueId: p.id,
102 state: { head, plain: ["reaches example.com"] },
103 answer: "wall",
104 p: 1,
105 by: "test",
106 ms: 0,
107 });
108 pulls.push(p);
109 }
110 // Some land before production serves a deploy, some after; some close unmerged.
111 const merge = (p: Pull, at: string): void => {
112 app.db.query("UPDATE pull SET merged_at = ?1 WHERE issue_id = ?2").run(at, p.id);
113 setIssueState(app.db, p.id, "closed");
114 };
115 merge(pulls[2]!, "2026-01-01T00:00:00.000Z");
116 merge(pulls[10]!, "2026-01-01T00:00:00.000Z");
117 merge(pulls[13]!, "2099-01-01T00:00:00.000Z");
118 setIssueState(app.db, pulls[4]!.id, "closed");
119 setIssueState(app.db, pulls[5]!.id, "closed");
120 const deploy = createRun(app.db, { repoId: repo.id, kind: "deploy", env: "production", commitOid: oid(1) });
121 app.db.query("UPDATE run SET created_at = '2026-06-01T00:00:00.000Z', serving_at = created_at WHERE id = ?1").run(deploy.id);
122
123 const all = pulls.map(reread);
124 const batch = stepsOfStored(app.db, repo, all);
125 expect(batch.size).toBe(all.length);
126 for (const p of all) expect({ n: p.number, step: batch.get(p.id) }).toEqual({ n: p.number, step: stepOfStored(app.db, repo, p) });
127 // The fixture reaches every branch it means to.
128 const steps = [...batch.values()];
129 expect(steps.filter((s) => s === null).length).toBe(2);
130 expect(new Set(steps.map((s) => s?.step))).toEqual(new Set(["check", "review", "land", "live", undefined]));
131 expect(steps.some((s) => s?.step === "live" && s.done)).toBe(true);
132 expect(steps.some((s) => s?.step === "live" && !s.done)).toBe(true);
133 expect(steps.some((s) => s?.why.includes("holds it"))).toBe(true);
134 expect(steps.some((s) => s?.why.includes("access wall"))).toBe(true);
135 expect(stepsOfStored(app.db, repo, []).size).toBe(0);
136 });
137
138 test("fifty open pulls read their steps in the statements five do: one batch, not one per row", async () => {
139 const pulls: Pull[] = [];
140 for (let n = 100; n < 150; n++) {
141 const p = pull(n);
142 setRun(createRun(app.db, { repoId: repo.id, kind: "check", issueId: p.id, commitOid: p.headOid }).id, n % 2 ? "ok" : "running");
143 if (n % 3 === 0) submitReview(app.db, { issueId: p.id, authorId: bob.id, state: "approved", commitOid: p.headOid });
144 pulls.push(p);
145 }
146 expect(stepsOfStored(app.db, repo, pulls).size).toBe(50);
147 const five = await statements(app.db, () => void stepsOfStored(app.db, repo, pulls.slice(0, 5)));
148 const fifty = await statements(app.db, () => void stepsOfStored(app.db, repo, pulls));
149 expect(fifty).toBe(five);
150 expectWork(fifty, { max: 5 });
151 });
test/pulls-list-timing.test.ts +2 −2
@@ -37,10 +37,10 @@ function names(res: Response): string[] {
3737 return (res.headers.get("server-timing") ?? "").split(", ").map((p) => p.split(";")[0]!);
3838 }
3939
40 test("a repository's pulls list names session, repo, pulls, words, tells, labels and render in order", async () => {
40 test("a repository's pulls list names session, repo, pulls, words, tells, labels, steps and render in order", async () => {
4141 const res = await fetch(`${servers.url}/alice/notes/pulls`, { headers: { authorization: `Bearer ${bearer}` } });
4242 expect(res.status).toBe(200);
4343 await res.text();
4444 expect(res.headers.get("x-content-type-options")).toBe("nosniff");
45 expect(names(res)).toEqual(["app", "session", "repo", "pulls", "words", "tells", "labels", "render"]);
45 expect(names(res)).toEqual(["app", "session", "repo", "pulls", "words", "tells", "labels", "steps", "render"]);
4646 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs