trevato/forge

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

More
Forks1g fMapg aRuns12g rEventsFlows

A pull's resolve page's Server-Timing names its phases: session, each read, render #3501

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/3463-a-pull-s-resolve-page-s-server-timing-na → main 1 commit fork

  • Merged. scout/3463-a-pull-s-resolve-page-s-server-timing-na was merged into main as 0e6a6d5.

Review

Sign in to review this change.
2 files changed +103 −8
src/web/pages/conflicts.ts +23 −8
@@ -24,6 +24,7 @@ import {
2424 resolvedCount,
2525 type Choice,
2626 } from "../../git/conflicts.ts";
27 import { notePhase, takeSessionTime, timePhase, timePhaseAsync } from "../../core/route_timing.ts";
2728 import type { Ctx } from "../../http/context.ts";
2829 import { crumbsFor, notFoundPage, repoBase, repoPalette, repoHeader } from "../chrome.ts";
2930 import { FINISH_REFUSALS, finishConflicts } from "../../actions/finish-conflicts.ts";
@@ -143,21 +144,27 @@ const CSS = `
143144 `;
144145
145146 export async function conflictsPage(ctx: Ctx): Promise<Response> {
146 const found = pullContext(ctx);
147 const req = ctx.request;
148 notePhase(req, "session", takeSessionTime(req));
149 const found = timePhase(req, "pull", () => pullContext(ctx));
147150 if (found instanceof Response) return found;
148151 const { repo, pull } = found;
149152 if (!canWrite(ctx.app.db, repo, ctx.auth.user)) return notFoundPage(ctx);
150153 const base = repoBase(repo);
151154 const back = `${base}/pulls/${pull.number}/files`;
152 const dir = await pullDir(ctx.app.db, ctx.app.reposRoot, repo, pull);
153 const status = await pullStatus(dir, pull);
155 const dir = await timePhaseAsync(req, "checkout", () =>
156 pullDir(ctx.app.db, ctx.app.reposRoot, repo, pull),
157 );
158 const status = await timePhaseAsync(req, "status", () => pullStatus(dir, pull));
154159 const files = status.conflicts;
155160 const asked = ctx.url.searchParams.get("file") ?? "";
156161 // Only a path git itself named is read; the rest of the world is a 404.
157162 const file = asked === "" ? files[0] : files.find((f) => f === asked);
158163 const unknown = asked !== "" && file === undefined && files.length > 0;
159164 if (pull.state !== "open" || file === undefined) {
160 return page({
165 const crew = await timePhaseAsync(req, "crew", () => proposalCard(ctx, repo, pull, dir, false));
166 const rendering = performance.now();
167 const response = page({
161168 ctx,
162169 title: `Resolve · ${pull.title}`,
163170 crumbs: crumbsFor(repo),
@@ -170,11 +177,17 @@ export async function conflictsPage(ctx: Ctx): Promise<Response> {
170177 : "Nothing to resolve: this pull has no conflicts."}
171178 <a href="${back}">Back to the files</a>.</p>`,
172179 )}
173 ${await proposalCard(ctx, repo, pull, dir, false)}`,
180 ${crew}`,
174181 });
182 notePhase(req, "render", performance.now() - rendering);
183 return response;
175184 }
176185
177 const text = await readConflictedFile(dir, status.baseOid, status.headOid, file);
186 const text = await timePhaseAsync(req, "file", () =>
187 readConflictedFile(dir, status.baseOid, status.headOid, file),
188 );
189 const crew = await timePhaseAsync(req, "crew", () => proposalCard(ctx, repo, pull, dir, true));
190 const rendering = performance.now();
178191 const parsed = parseConflicts(text ?? "");
179192 const choices = choicesFrom(ctx.url.searchParams, parsed.blocks.length);
180193 const labels = { ours: pull.baseBranch, theirs: pull.headBranch };
@@ -232,7 +245,7 @@ export async function conflictsPage(ctx: Ctx): Promise<Response> {
232245 )}
233246 </p>`
234247 : ""}
235 ${await proposalCard(ctx, repo, pull, dir, true)}
248 ${crew}
236249 ${total === 0
237250 ? card(
238251 html`<p>This file has no text blocks to choose between (it may be
@@ -251,7 +264,7 @@ export async function conflictsPage(ctx: Ctx): Promise<Response> {
251264 <h2 class="mt">Merged file</h2>
252265 <pre class="rs-merged" aria-label="Merged ${file}">${merged}</pre>`;
253266
254 return page({
267 const response = page({
255268 ctx,
256269 title: `Resolve · ${pull.title} · ${repo.name}`,
257270 crumbs: crumbsFor(repo),
@@ -259,6 +272,8 @@ export async function conflictsPage(ctx: Ctx): Promise<Response> {
259272 body,
260273 styleCss: CSS,
261274 });
275 notePhase(req, "render", performance.now() - rendering);
276 return response;
262277 }
263278
264279 // POST /:owner/:name/pulls/:number/resolve
test/conflicts-timing.test.ts added +80
@@ -0,0 +1,80 @@
1 /** `/:owner/:name/pulls/:n/resolve`'s Server-Timing names its phases (#3463). */
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-conflicts-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 const commit = async (branch: string | undefined, content: string) => {
42 const done = await commitToBranch(app, alice, repo.value, {
43 ...(branch === undefined ? {} : { branch }),
44 message: "change",
45 files: [{ path: "a.txt", content }],
46 });
47 if (!done.ok) throw new Error(JSON.stringify(done.error));
48 };
49 await commit(undefined, "one\n");
50 await commit("topic", "theirs\n");
51 await commit(undefined, "ours\n");
52 const pull = await openPull(app, alice, repo.value, {
53 title: "t",
54 body: "",
55 baseBranch: "main",
56 headBranch: "topic",
57 });
58 if (!pull.ok) throw new Error(JSON.stringify(pull.error));
59 number = pull.value.number;
60 });
61
62 afterAll(async () => {
63 await servers.stop();
64 app.close();
65 if (cells === undefined) delete process.env["FORGE_CELLS"];
66 else process.env["FORGE_CELLS"] = cells;
67 await rm(dataDir, { recursive: true, force: true });
68 });
69
70 test("a conflicted pull's resolve page names its phases after app, in order", async () => {
71 const res = await fetch(`${servers.url}/alice/r/pulls/${number}/resolve`, { headers: { cookie } });
72 expect(res.status).toBe(200);
73 expect(await res.text()).toContain("Block 1 of 1");
74 const parts = (res.headers.get("server-timing") ?? "").split(", ");
75 const names = parts.map((p) => p.split(";")[0]!);
76 expect(names.slice(names.indexOf("app") + 1)).toEqual([
77 "session", "pull", "checkout", "status", "file", "crew", "render",
78 ]);
79 for (const p of parts) expect(p).toMatch(/;dur=\d+\.\d/);
80 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs