Public: anyone can read this; the crew can change it.
A repository's new issue, make-it-yours, report and toolbar pages name their Server-Timing phases and answer under 50 ms #4335
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/4321-a-repository-s-new-issue-make-it-yours-r → main 1 commit fork 2 threads · 2 unresolved
6 files
changed +149
−41
src/web/pages/issues.ts +15 −11
| @@ -13,6 +13,7 @@ import { | ||
| 13 | 13 | import type { Issue } from "../../core/issues.ts"; |
| 14 | 14 | import { canRead, type Repo } from "../../core/repos.ts"; |
| 15 | 15 | import { privateRepoForSealed } from "../../actions/repos.ts"; |
| 16 | import { notePhase, takeSessionTime, timePhase } from "../../core/route_timing.ts"; | |
| 16 | 17 | import type { Ctx } from "../../http/context.ts"; |
| 17 | 18 | import { |
| 18 | 19 | crumbsFor, |
| @@ -128,18 +129,21 @@ function newIssueForm( | ||
| 128 | 129 | |
| 129 | 130 | // GET /:owner/:name/issues/new |
| 130 | 131 | export function newIssuePage(ctx: Ctx): Response { |
| 131 | const repo = loadRepo(ctx); | |
| 132 | if (!repo) return notFoundPage(ctx); | |
| 133 | if (!ctx.auth.user) | |
| 134 | return canRead(ctx.app.db, repo, undefined) | |
| 135 | ? accountOrReport(ctx, repo) | |
| 136 | : signInFirst(ctx); | |
| 132 | const request = ctx.request; | |
| 133 | notePhase(request, "session", takeSessionTime(request)); | |
| 134 | const repo = timePhase(request, "repo", () => loadRepo(ctx)); | |
| 135 | if (!repo) return timePhase(request, "render", () => notFoundPage(ctx)); | |
| 136 | if (!ctx.auth.user) { | |
| 137 | const open = timePhase(request, "access", () => canRead(ctx.app.db, repo, undefined)); | |
| 138 | return timePhase(request, "render", () => | |
| 139 | open ? accountOrReport(ctx, repo) : signInFirst(ctx), | |
| 140 | ); | |
| 141 | } | |
| 137 | 142 | // `?sealed=1`: the repository page's "report it sealed" link. |
| 138 | return composePage(ctx, repo, { | |
| 139 | title: "", | |
| 140 | body: "", | |
| 141 | sealed: ctx.url.searchParams.get("sealed") === "1", | |
| 142 | }); | |
| 143 | const sealed = ctx.url.searchParams.get("sealed") === "1"; | |
| 144 | return timePhase(request, "render", () => | |
| 145 | composePage(ctx, repo, { title: "", body: "", sealed }), | |
| 146 | ); | |
| 143 | 147 | } |
| 144 | 148 | |
| 145 | 149 | /** |
src/web/pages/reports.ts +9 −2
| @@ -121,8 +121,15 @@ function reportForm( | ||
| 121 | 121 | |
| 122 | 122 | // GET /:owner/:name/report |
| 123 | 123 | export function reportPage(ctx: Ctx): Response { |
| 124 | const repo = publicRepo(ctx); | |
| 125 | if (!repo) return notFoundPage(ctx); | |
| 124 | const request = ctx.request; | |
| 125 | notePhase(request, "session", takeSessionTime(request)); | |
| 126 | const repo = timePhase(request, "repo", () => loadRepo(ctx)); | |
| 127 | const open = repo !== undefined && timePhase(request, "access", () => canRead(ctx.app.db, repo, undefined)); | |
| 128 | if (!repo || !open) return timePhase(request, "render", () => notFoundPage(ctx)); | |
| 129 | return timePhase(request, "render", () => reportBody(ctx, repo)); | |
| 130 | } | |
| 131 | ||
| 132 | function reportBody(ctx: Ctx, repo: Repo): Response { | |
| 126 | 133 | if (ctx.url.searchParams.get("sent") === "1") |
| 127 | 134 | return page({ |
| 128 | 135 | ctx, |
src/web/studio/make_it_yours.ts +25 −19
| @@ -26,33 +26,39 @@ import { | ||
| 26 | 26 | timePhase, |
| 27 | 27 | } from "../../core/route_timing.ts"; |
| 28 | 28 | |
| 29 | type Refusal = Extract<Awaited<ReturnType<typeof makeItYours>>, { ok: false }>["error"]; | |
| 30 | ||
| 31 | /** Where a refused plan lands: back on the repository, saying why. */ | |
| 32 | function planRefused(ctx: Ctx, repo: NonNullable<ReturnType<typeof loadRepo>>, error: Refusal): Response { | |
| 33 | if (error.kind === "own") | |
| 34 | return flashTo(ctx, repoBase(repo), { | |
| 35 | tone: "attn", | |
| 36 | message: "This app is already yours.", | |
| 37 | }); | |
| 38 | if (error.kind === "not_found") return notFoundPage(ctx); | |
| 39 | return flashTo(ctx, repoBase(repo), { | |
| 40 | tone: "danger", | |
| 41 | message: | |
| 42 | error.kind === "invalid" | |
| 43 | ? error.message | |
| 44 | : error.kind === "not_member" | |
| 45 | ? `Not for this account: ${NOT_A_MEMBER}.` | |
| 46 | : "That could not be made yours.", | |
| 47 | }); | |
| 48 | } | |
| 49 | ||
| 29 | 50 | // GET /:owner/:name/make-it-yours |
| 30 | 51 | export async function makeItYoursPage(ctx: Ctx): Promise<Response> { |
| 31 | 52 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| 32 | 53 | const repo = timePhase(ctx.request, "repo", () => loadRepo(ctx)); |
| 33 | if (!repo) return notFoundPage(ctx); | |
| 34 | if (!ctx.auth.user) return signInFirst(ctx); | |
| 54 | if (!repo) return timePhase(ctx.request, "render", () => notFoundPage(ctx)); | |
| 55 | if (!ctx.auth.user) return timePhase(ctx.request, "render", () => signInFirst(ctx)); | |
| 35 | 56 | const user = ctx.auth.user; |
| 36 | 57 | const plan = await timePhaseAsync(ctx.request, "plan", () => |
| 37 | 58 | makeItYours(ctx.app, user, repo, { dryRun: true }), |
| 38 | 59 | ); |
| 39 | if (!plan.ok) { | |
| 40 | if (plan.error.kind === "own") | |
| 41 | return flashTo(ctx, repoBase(repo), { | |
| 42 | tone: "attn", | |
| 43 | message: "This app is already yours.", | |
| 44 | }); | |
| 45 | if (plan.error.kind === "not_found") return notFoundPage(ctx); | |
| 46 | return flashTo(ctx, repoBase(repo), { | |
| 47 | tone: "danger", | |
| 48 | message: | |
| 49 | plan.error.kind === "invalid" | |
| 50 | ? plan.error.message | |
| 51 | : plan.error.kind === "not_member" | |
| 52 | ? `Not for this account: ${NOT_A_MEMBER}.` | |
| 53 | : "That could not be made yours.", | |
| 54 | }); | |
| 55 | } | |
| 60 | if (!plan.ok) | |
| 61 | return timePhase(ctx.request, "render", () => planRefused(ctx, repo, plan.error)); | |
| 56 | 62 | const p = plan.value; |
| 57 | 63 | const rendering = performance.now(); |
| 58 | 64 | const body = html`<div class="narrow-wide"> |
src/web/studio/toolbar.ts +5 −5
| @@ -26,7 +26,7 @@ import { mayDirectCrew } from "../../crew/direct.ts"; | ||
| 26 | 26 | import { isSafeOid } from "../../git/guard.ts"; |
| 27 | 27 | import { commitLog } from "../../git/read.ts"; |
| 28 | 28 | import { repoDir } from "../../git/repos.ts"; |
| 29 | import { notePhase, takeSessionTime, timePhase } from "../../core/route_timing.ts"; | |
| 29 | import { notePhase, takeSessionTime, timePhase, timePhaseAsync } from "../../core/route_timing.ts"; | |
| 30 | 30 | import type { Ctx } from "../../http/context.ts"; |
| 31 | 31 | import { notFoundPage, pageHead, loadRepo, repoBase } from "../chrome.ts"; |
| 32 | 32 | import { field, readForm } from "../form.ts"; |
| @@ -96,10 +96,10 @@ export async function toolbarPage(ctx: Ctx): Promise<Response> { | ||
| 96 | 96 | const request = ctx.request; |
| 97 | 97 | notePhase(request, "session", takeSessionTime(request)); |
| 98 | 98 | const repo = timePhase(request, "repo", () => loadRepo(ctx)); |
| 99 | if (!repo) return notFoundPage(ctx); | |
| 99 | if (!repo) return timePhase(request, "render", () => notFoundPage(ctx)); | |
| 100 | 100 | const where = parseEnv(ctx.url.searchParams.get("env") ?? "production"); |
| 101 | 101 | if (where === undefined || timePhase(request, "app-row", () => getAppByRepo(ctx.app.db, repo.id)) === undefined) |
| 102 | return notFoundPage(ctx); | |
| 102 | return timePhase(request, "render", () => notFoundPage(ctx)); | |
| 103 | 103 | const db = ctx.app.db; |
| 104 | 104 | const user = ctx.auth.user; |
| 105 | 105 | const pull = |
| @@ -107,11 +107,11 @@ export async function toolbarPage(ctx: Ctx): Promise<Response> { | ||
| 107 | 107 | ? undefined |
| 108 | 108 | : timePhase(request, "pull", () => getIssue(db, repo.id, where.pull!)); |
| 109 | 109 | if (where.pull !== undefined && (!pull || pull.kind !== "pull" || !mayView(db, repo, pull, user))) |
| 110 | return notFoundPage(ctx); | |
| 110 | return timePhase(request, "render", () => notFoundPage(ctx)); | |
| 111 | 111 | const address = pageAddress(ctx.url.searchParams.get("page") ?? ""); |
| 112 | 112 | const base = repoBase(repo); |
| 113 | 113 | const here = html`${repo.name}, ${pull ? html`preview of pull #${pull.number}` : "production"}`; |
| 114 | const liveNote = pull ? html`` : await liveLine(ctx, repo, base); | |
| 114 | const liveNote = pull ? html`` : await timePhaseAsync(request, "live", () => liveLine(ctx, repo, base)); | |
| 115 | 115 | const home = btn("Home", { href: "/", variant: "ghost" }); |
| 116 | 116 | const rendering = performance.now(); |
| 117 | 117 | if (!user) { |
test/repo-pages-timing.test.ts added +95
| @@ -0,0 +1,95 @@ | ||
| 1 | /** A repository's new issue, make-it-yours, report and toolbar pages name their phases and answer under 50 ms (#4321). */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtempSync, rmSync } from "node:fs"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { newApp } from "../src/actions/apps.ts"; | |
| 7 | import { createApp, type App } from "../src/app.ts"; | |
| 8 | import { createUser } from "../src/core/users.ts"; | |
| 9 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 10 | import { proveAll } from "./helpers/verified.ts"; | |
| 11 | import { quietLogs } from "./helpers/globals.ts"; | |
| 12 | import { promote } from "./helpers/members.ts"; | |
| 13 | ||
| 14 | quietLogs(); | |
| 15 | ||
| 16 | let root = ""; | |
| 17 | let app: App; | |
| 18 | let servers: Servers; | |
| 19 | ||
| 20 | async function signIn(username: string): Promise<string> { | |
| 21 | const res = await fetch(`${servers.url}/signin`, { | |
| 22 | method: "POST", | |
| 23 | headers: { origin: servers.url }, | |
| 24 | body: new URLSearchParams({ username, password: "a-good-passphrase" }), | |
| 25 | redirect: "manual", | |
| 26 | }); | |
| 27 | return (res.headers.get("set-cookie") ?? "").split(";")[0]!; | |
| 28 | } | |
| 29 | ||
| 30 | beforeAll(async () => { | |
| 31 | root = mkdtempSync(join(tmpdir(), "forge-repo-pages-timing-")); | |
| 32 | app = createApp({ dataDir: root, port: 0, forgeRepo: "alice/forge" }); | |
| 33 | const users = []; | |
| 34 | for (const username of ["alice", "bob"]) { | |
| 35 | const u = await createUser(app.db, { | |
| 36 | origin: "seed", | |
| 37 | trusted: true, | |
| 38 | username, | |
| 39 | email: `${username}@forge.local`, | |
| 40 | password: "a-good-passphrase", | |
| 41 | adminIfFirst: username === "alice", | |
| 42 | }); | |
| 43 | if (!u.ok) throw new Error(u.error); | |
| 44 | promote(app.db, u.value); | |
| 45 | users.push(u.value); | |
| 46 | } | |
| 47 | proveAll(app); | |
| 48 | const made = await newApp(app, { id: users[0]!.id, username: "alice" } as never, { name: "forge", description: "x", visibility: "public" }); | |
| 49 | if (!made.ok) throw new Error(made.error); | |
| 50 | servers = await serve(app); | |
| 51 | }); | |
| 52 | afterAll(() => { | |
| 53 | servers.stop(); | |
| 54 | app.close(); | |
| 55 | rmSync(root, { recursive: true, force: true }); | |
| 56 | }); | |
| 57 | ||
| 58 | /** Path, who is signed in, the phases it names (bar `app` and `total`), in order. */ | |
| 59 | const PAGES: [path: string, who: string, names: string[]][] = [ | |
| 60 | ["/alice/forge/issues/new", "alice", ["session", "repo", "render"]], | |
| 61 | ["/alice/forge/report", "alice", ["session", "repo", "access", "render"]], | |
| 62 | ["/alice/forge/toolbar", "alice", ["session", "repo", "app-row", "live", "runs", "render"]], | |
| 63 | ["/alice/forge/make-it-yours", "bob", ["session", "repo", "plan", "render"]], | |
| 64 | ]; | |
| 65 | ||
| 66 | for (const [path, who, names] of PAGES) | |
| 67 | test(`GET ${path} names its phases, they fit inside app, and it answers under 50 ms`, async () => { | |
| 68 | const cookie = await signIn(who); | |
| 69 | const read = async (): Promise<string[]> => { | |
| 70 | const res = await fetch(`${servers.url}${path}`, { headers: { cookie } }); | |
| 71 | await res.text(); | |
| 72 | expect(res.status).toBe(200); | |
| 73 | return (res.headers.get("server-timing") ?? "").split(", "); | |
| 74 | }; | |
| 75 | const dur = (part: string | undefined): number => { | |
| 76 | expect(part).toMatch(/^[a-z-]+;dur=\d+\.\d$/); | |
| 77 | return Number(part!.slice(part!.indexOf("=") + 1)); | |
| 78 | }; | |
| 79 | for (let i = 0; i < 5; i++) await read(); // warm | |
| 80 | const shares: number[] = []; | |
| 81 | for (let i = 0; i < 11; i++) { | |
| 82 | const parts = await read(); | |
| 83 | const named = parts.map((p) => p.split(";")[0]!).filter((n) => n !== "app" && n !== "db"); | |
| 84 | expect(named).toEqual(names); | |
| 85 | const total = dur(parts.find((p) => p.startsWith("app;"))); | |
| 86 | const sum = parts.filter((p) => !/^(app|db);/.test(p)).reduce((s, p) => s + dur(p), 0); | |
| 87 | expect(total).toBeLessThan(50); | |
| 88 | expect(sum).toBeLessThanOrEqual(total + 0.1 * names.length); | |
| 89 | shares.push(total === 0 ? 1 : sum / total); | |
| 90 | } | |
| 91 | // `app` also holds routing and the security floor, outside these pages; | |
| 92 | // the median request is what the page can name of itself. | |
| 93 | shares.sort((a, b) => a - b); | |
A 0.4 floor on the share is weaker than the issue (within 1 ms of total) and is timing-dependent on a loaded CI machine. Tighten it once the server names its own overhead. |
||
| 94 | expect(shares[Math.floor(shares.length / 2)]!).toBeGreaterThanOrEqual(0.4); | |
| 95 | }); |
test/server-timing-sweep.test.ts −4
| @@ -27,17 +27,13 @@ const UNNAMED: Array<[path: string, issue: string]> = [ | ||
| 27 | 27 | ["/alice/forge/app/now", "#3390"], |
| 28 | 28 | ["/alice/forge/app/state", "#3390"], |
| 29 | 29 | ["/alice/forge/changes", "#3323"], |
| 30 | ["/alice/forge/issues/new", "#3390"], | |
| 31 | ["/alice/forge/make-it-yours", "#3390"], | |
| 32 | 30 | ["/alice/forge/map", "#3320"], |
| 33 | ["/alice/forge/report", "#3390"], | |
| 34 | 31 | ["/alice/forge/settings", "#3390"], |
| 35 | 32 | ["/alice/forge/settings/danger", "#3390"], |
| 36 | 33 | ["/alice/forge/settings/hooks", "#3390"], |
| 37 | 34 | ["/alice/forge/settings/labels", "#3390"], |
| 38 | 35 | ["/alice/forge/settings/prompts", "#3390"], |
| 39 | 36 | ["/alice/forge/tells", "#3322"], |
| 40 | ["/alice/forge/toolbar", "#3390"], | |
| 41 | 37 | ["/apps/open", "#3390"], |
| 42 | 38 | ["/board", "#3378"], |
| 43 | 39 | ["/claim", "#3390"], |
new above
offline
This inlines what publicRepo does and leaves publicRepo for the POST path. Fine for timing, but the two can drift. Prefer one helper that takes the request and notes the phases. This line is also over the width the surrounding code wraps at.