trevato/forge

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

More
Forks1g fMapg aRuns12g rEventsFlows

The API's read of one pull names its Server-Timing phases: session, pull, head, diff, checks, render #3769

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/3765-the-api-s-read-of-one-pull-names-its-ser → main 1 commit fork

  • Merged. scout/3765-the-api-s-read-of-one-pull-names-its-ser was merged into main as 4e0a4f4.

Review

Sign in to review this change.
2 files changed +120 −26
src/http/api.ts +35 −26
@@ -15,6 +15,7 @@ import { listPeople, NOT_A_MEMBER } from "../core/roles.ts";
1515 import { setAccountCeilings, setAccountGrant } from "../actions/model_grant.ts";
1616 import { storageLimitMessage } from "../core/storage.ts";
1717 import { voteVariant } from "../actions/variants.ts";
18 import { notePhase, takeSessionTime, timePhase, timePhaseAsync } from "../core/route_timing.ts";
1819 import { parentForgeFor } from "../core/parent_releases.ts";
1920 import { readFlow } from "../flow/read.ts";
2021 import { listPinned as listPinnedPanels, movePinned, pinPanel, unpinPanel, type PinError } from "../actions/board-pins.ts";
@@ -3672,41 +3673,49 @@ export function apiRoutes(router: Router<Handler>): void {
36723673 });
36733674
36743675 router.add("GET", "/api/v1/repos/:owner/:name/pulls/:number", async (ctx) => {
3675 const found = loadPull(ctx);
3676 notePhase(ctx.request, "session", takeSessionTime(ctx.request));
3677 const found = timePhase(ctx.request, "pull", () => loadPull(ctx));
36763678 if ("error" in found) return found.error;
36773679 // A fork may have pushed since the pull opened; review what is being
36783680 // proposed now, not the snapshot taken when it was.
3679 await refreshPullHead(ctx.app, found.repo, found.pull);
3681 await timePhaseAsync(ctx.request, "head", () =>
3682 refreshPullHead(ctx.app, found.repo, found.pull),
3683 );
36803684 // Include the diff so a reviewer (human or agent) reads the change without
36813685 // a second round trip, and the review gate beside it so a client knows
36823686 // whether merging is even on offer.
3683 const { status, diff } = await pullDiff(
3684 await pullDirOf(ctx, found.repo, found.pull),
3685 found.pull,
3686 { db: ctx.app.db, requireReview: found.repo.requireReview },
3687 const { status, diff } = await timePhaseAsync(ctx.request, "diff", async () =>
3688 pullDiff(await pullDirOf(ctx, found.repo, found.pull), found.pull, {
3689 db: ctx.app.db,
3690 requireReview: found.repo.requireReview,
3691 }),
36873692 );
3688 const hold = getHold(ctx.app.db, found.pull.id);
36893693 const live = found.pull.state === "open" && found.pull.mergedAt === null;
3690 const checkStaleNow =
3691 live &&
3692 checkStale(
3693 checkSummary(ctx.app.db, found.pull.id, status.headOid).run,
3694 await resolveOid(
3695 await pullDirOf(ctx, found.repo, found.pull),
3696 `refs/heads/${found.pull.baseBranch}`,
3694 const checkStaleNow = await timePhaseAsync(
3695 ctx.request,
3696 "checks",
3697 async () =>
3698 live &&
3699 checkStale(
3700 checkSummary(ctx.app.db, found.pull.id, status.headOid).run,
3701 await resolveOid(
3702 await pullDirOf(ctx, found.repo, found.pull),
3703 `refs/heads/${found.pull.baseBranch}`,
3704 ),
36973705 ),
3698 );
3699 return json({
3700 ...withLabels(ctx, found.pull),
3701 checkStale: checkStaleNow,
3702 variantSiblings: variantSiblings(ctx.app.db, [found.pull])[0] ?? [],
3703 ...(found.pull.state === "open" && found.pull.mergedAt === null
3704 ? { mergeable: status.mergeable, conflicts: status.conflicts }
3705 : {}),
3706 proposal: pullProposalOf(ctx.app.db, found.pull.id) ?? null,
3707 status,
3708 diff,
3709 hold: hold === undefined ? null : holdJson(hold),
3706 );
3707 return timePhase(ctx.request, "render", () => {
3708 const hold = getHold(ctx.app.db, found.pull.id);
3709 return json({
3710 ...withLabels(ctx, found.pull),
3711 checkStale: checkStaleNow,
3712 variantSiblings: variantSiblings(ctx.app.db, [found.pull])[0] ?? [],
3713 ...(live ? { mergeable: status.mergeable, conflicts: status.conflicts } : {}),
3714 proposal: pullProposalOf(ctx.app.db, found.pull.id) ?? null,
3715 status,
3716 diff,
3717 hold: hold === undefined ? null : holdJson(hold),
3718 });
37103719 });
37113720 });
37123721
test/api-pull-timing.test.ts added +85
@@ -0,0 +1,85 @@
1 /** The API's read of one pull names its Server-Timing phases (#3765). */
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 { openPull } from "../src/actions/pulls.ts";
8 import { commitToBranch, createRepo } from "../src/actions/repos.ts";
9 import { createSession } from "../src/core/sessions.ts";
10 import { createUser } from "../src/core/users.ts";
11 import { serve, type Servers } from "../src/http/server.ts";
12 import { quietLogs } from "./helpers/globals.ts";
13
14 quietLogs();
15
16 let dataDir: string;
17 let app: App;
18 let servers: Servers;
19 let cookie = "";
20 let number = 0;
21 const cells = process.env["FORGE_CELLS"];
22
23 beforeAll(async () => {
24 process.env["FORGE_CELLS"] = "off";
25 dataDir = await mkdtemp(join(tmpdir(), "forge-api-pull-timing-"));
26 app = createApp({ dataDir, port: 0 });
27 servers = await serve(app);
28 const made = await createUser(app.db, {
29 origin: "seed",
30 trusted: true,
31 username: "alice",
32 email: "[email protected]",
33 password: "a-good-passphrase",
34 adminIfFirst: true,
35 });
36 if (!made.ok) throw new Error(String(made.error));
37 const alice = made.value;
38 cookie = `forge_session=${createSession(app.db, alice.id).token}`;
39 const repo = await createRepo(app, alice, { name: "r", visibility: "public" });
40 if (!repo.ok) throw new Error(String(repo.error));
41 for (const [branch, file] of [[undefined, "forge.json"], ["topic", "t.txt"]] as const) {
42 const done = await commitToBranch(app, alice, repo.value, {
43 ...(branch === undefined ? {} : { branch }),
44 message: "change",
45 files: [{ path: file, content: file === "forge.json" ? "{}" : "x" }],
46 });
47 if (!done.ok) throw new Error(JSON.stringify(done.error));
48 }
49 const pull = await openPull(app, alice, repo.value, {
50 title: "t",
51 body: "",
52 baseBranch: "main",
53 headBranch: "topic",
54 });
55 if (!pull.ok) throw new Error(JSON.stringify(pull.error));
56 number = pull.value.number;
57 });
58
59 afterAll(async () => {
60 await servers.stop();
61 app.close();
62 if (cells === undefined) delete process.env["FORGE_CELLS"];
63 else process.env["FORGE_CELLS"] = cells;
64 await rm(dataDir, { recursive: true, force: true });
65 });
66
67 async function phases(path: string): Promise<{ status: number; names: string[]; body: string }> {
68 const res = await fetch(`${servers.url}${path}`, { headers: { cookie } });
69 const body = await res.text();
70 const names = (res.headers.get("server-timing") ?? "").split(", ").map((p) => p.split(";")[0]!);
71 return { status: res.status, names: names.slice(names.indexOf("app") + 1), body };
72 }
73
74 test("the API read of an open pull names its six phases in order", async () => {
75 const got = await phases(`/api/v1/repos/alice/r/pulls/${number}`);
76 expect(got.status).toBe(200);
77 expect(got.names).toEqual(["session", "pull", "head", "diff", "checks", "render"]);
78 expect(JSON.parse(got.body).number).toBe(number);
79 });
80
81 test("a missing pull still names session and pull", async () => {
82 const got = await phases("/api/v1/repos/alice/r/pulls/9999");
83 expect(got.status).toBe(404);
84 expect(got.names).toEqual(["session", "pull"]);
85 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs