Public: anyone can read this; the crew can change it.
A pull's Files page counts its commit-log read under the diff phase of Server-Timing, and a test fixes the phase names #4099
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/4095-a-pull-s-files-page-counts-its-commit-lo → main 2 commits fork 2 threads · 2 unresolved
2 files
changed +96
−27
src/web/pages/pull.ts +61 −26
| @@ -52,7 +52,6 @@ import { | ||
| 52 | 52 | import { getUsersByIds } from "../../core/users.ts"; |
| 53 | 53 | import { canWrite, getRepoById, type Repo } from "../../core/repos.ts"; |
| 54 | 54 | import { resolveOid } from "../../git/merge.ts"; |
| 55 | import { timePhaseAsync } from "../../core/route_timing.ts"; | |
| 56 | 55 | import { commitLog } from "../../git/read.ts"; |
| 57 | 56 | import { repoDir } from "../../git/repos.ts"; |
| 58 | 57 | import { wallOf } from "../../contract/policy.ts"; |
| @@ -65,6 +64,7 @@ import { | ||
| 65 | 64 | repoPalette, |
| 66 | 65 | signInFirst, |
| 67 | 66 | } from "../chrome.ts"; |
| 67 | import { notePhase, takeSessionTime } from "../../core/route_timing.ts"; | |
| 68 | 68 | import { diffCacheCounts, diffKey, keptDiff } from "../diff-cache.ts"; |
| 69 | 69 | import { diffSyntax } from "../diff-syntax.ts"; |
| 70 | 70 | import { |
| @@ -800,6 +800,27 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> { | ||
| 800 | 800 | const found = pullContext(ctx); |
| 801 | 801 | if (found instanceof Response) return found; |
| 802 | 802 | const { repo, pull } = found; |
| 803 | const began = performance.now(); | |
| 804 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); | |
| 805 | const spent = { diff: 0, checks: 0, reviews: 0 }; | |
| 806 | /** Run `work`, adding the time it took to `spent[key]`. */ | |
| 807 | const lap = async <T>(key: keyof typeof spent, work: () => T | Promise<T>): Promise<T> => { | |
| 808 | const started = performance.now(); | |
| 809 | try { | |
| 810 | return await work(); | |
| 811 | } finally { | |
| 812 | spent[key] += performance.now() - started; | |
| 813 | } | |
| 814 | }; | |
| 815 | /** The same for a synchronous call: no await, no microtask. */ | |
| 816 | const tick = <T>(key: keyof typeof spent, work: () => T): T => { | |
| 817 | const started = performance.now(); | |
| 818 | try { | |
| 819 | return work(); | |
| 820 | } finally { | |
| 821 | spent[key] += performance.now() - started; | |
| 822 | } | |
| 823 | }; | |
| 803 | 824 | // Every read of the change is taken where its objects are (`pullDir`). |
| 804 | 825 | const dir = await pullDir(ctx.app.db, ctx.app.reposRoot, repo, pull); |
| 805 | 826 | const base = repoBase(repo); |
| @@ -807,43 +828,45 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> { | ||
| 807 | 828 | |
| 808 | 829 | // The review gate travels with the status, so the merge box and |
| 809 | 830 | // `mergePull` are answering the same question from the same numbers. |
| 810 | const status = await pullStatus(dir, pull, { | |
| 811 | db: ctx.app.db, | |
| 812 | requireReview: repo.requireReview, | |
| 813 | }); | |
| 831 | const status = await lap("diff", () => | |
| 832 | pullStatus(dir, pull, { | |
| 833 | db: ctx.app.db, | |
| 834 | requireReview: repo.requireReview, | |
| 835 | }), | |
| 836 | ); | |
| 814 | 837 | // The diff and its highlighting depend on two oids and nothing else, so |
| 815 | 838 | // the next reader of this head gets them from memory (`diff-cache.ts`). |
| 816 | const { diff, syntax } = await timePhaseAsync( | |
| 817 | ctx.request, | |
| 818 | "diff", | |
| 819 | () => | |
| 820 | keptDiff(diffKey(status.baseOid, status.headOid), async () => { | |
| 821 | const { from, diff, failed } = await diffBetween(dir, status.baseOid, status.headOid); | |
| 822 | return { from, diff, failed, syntax: await diffSyntax(dir, diff, from, status.headOid) }; | |
| 823 | }), | |
| 824 | diffCacheCounts, | |
| 839 | const cacheBefore = diffCacheCounts(); | |
| 840 | const { diff, syntax } = await lap("diff", () => | |
| 841 | keptDiff(diffKey(status.baseOid, status.headOid), async () => { | |
| 842 | const { from, diff, failed } = await diffBetween(dir, status.baseOid, status.headOid); | |
| 843 | return { from, diff, failed, syntax: await diffSyntax(dir, diff, from, status.headOid) }; | |
| 844 | }), | |
| 825 | 845 | ); |
| 846 | const cacheAfter = diffCacheCounts(); | |
| 847 | const diffHit = cacheAfter.hits > cacheBefore.hits && cacheAfter.misses === cacheBefore.misses; | |
| 826 | 848 | // A merged or closed pull is history; the wall is a question about landing. |
| 827 | 849 | const wall = |
| 828 | 850 | pull.mergedAt === null && pull.state === "open" |
| 829 | ? await wallOf(dir, status.baseOid, status.headOid) | |
| 851 | ? await lap("diff", () => wallOf(dir, status.baseOid, status.headOid)) | |
| 830 | 852 | : undefined; |
| 831 | const reviewLines = listReviewComments(ctx.app.db, pull.id); | |
| 853 | const reviewLines = tick("reviews", () => listReviewComments(ctx.app.db, pull.id)); | |
| 832 | 854 | const reviewComments = byAnchor(reviewLines); |
| 833 | 855 | const mayMerge = canWrite(ctx.app.db, repo, ctx.auth.user); |
| 834 | 856 | const applied = labelsFor(ctx.app.db, pull.id); |
| 835 | 857 | const allLabels = listLabels(ctx.app.db, repo.id); |
| 836 | const by = standings(ctx.app.db, pull, status.headOid); | |
| 858 | const by = tick("reviews", () => standings(ctx.app.db, pull, status.headOid)); | |
| 837 | 859 | // The checks are the deploy runs of the live head, read here for the same |
| 838 | 860 | // reason the review gate is: the box and the merge answer one question. |
| 839 | const check = checkSummary(ctx.app.db, pull.id, status.headOid); | |
| 861 | const check = tick("checks", () => checkSummary(ctx.app.db, pull.id, status.headOid)); | |
| 840 | 862 | const isApp = getAppByRepo(ctx.app.db, repo.id) !== undefined; |
| 841 | const tier = await tierOf(dir, status.headOid, check); | |
| 842 | const stale = | |
| 863 | const tier = await lap("checks", () => tierOf(dir, status.headOid, check)); | |
| 864 | const stale = await lap("checks", async () => | |
| 843 | 865 | pull.state === "open" && pull.mergedAt === null |
| 844 | 866 | ? checkStale(check.run, await resolveOid(dir, `refs/heads/${pull.baseBranch}`)) |
| 845 | : false; | |
| 846 | const threads = pullThreads(ctx.app, pull); | |
| 867 | : false, | |
| 868 | ); | |
| 869 | const threads = await lap("reviews", () => pullThreads(ctx.app, pull)); | |
| 847 | 870 | const threadViews = threadStateViews(ctx, repo, threads); |
| 848 | 871 | const unresolved = threads.filter((t) => t.resolvedAt === null).length; |
| 849 | 872 | |
| @@ -873,9 +896,11 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> { | ||
| 873 | 896 | ? `${headRepo.ownerName}/${headRepo.name}:${pull.headBranch}` |
| 874 | 897 | : pull.headBranch; |
| 875 | 898 | |
| 876 | const log = await commitLog(dir, status.headOid, { | |
| 877 | limit: Math.min(Math.max(status.ahead, 1), 100), | |
| 878 | }); | |
| 899 | const log = await lap("diff", () => | |
| 900 | commitLog(dir, status.headOid, { | |
| 901 | limit: Math.min(Math.max(status.ahead, 1), 100), | |
| 902 | }), | |
| 903 | ); | |
| 879 | 904 | const commits = log.ok ? log.value : []; |
| 880 | 905 | const commitsCard = card( |
| 881 | 906 | commits.length === 0 |
| @@ -949,7 +974,7 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> { | ||
| 949 | 974 | <div class="mt">${renderDiff(diff, diffOptions)}</div> |
| 950 | 975 | ${liveMount({ repo: base })}`; |
| 951 | 976 | |
| 952 | return page({ | |
| 977 | const response = page({ | |
| 953 | 978 | ctx, |
| 954 | 979 | title: `Files · ${pull.title} · ${repo.name}`, |
| 955 | 980 | crumbs: crumbsFor(repo), |
| @@ -958,6 +983,16 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> { | ||
| 958 | 983 | styleCss: labelCss([...allLabels, ...applied]), |
| 959 | 984 | scriptJs: `${liveClientJs}\n${liveRegionsJs}`, |
| 960 | 985 | }); |
| 986 | const { diff: diffMs, checks, reviews } = spent; | |
| 987 | notePhase(ctx.request, "diff", diffMs, diffHit); | |
| 988 | notePhase(ctx.request, "checks", checks); | |
| 989 | notePhase(ctx.request, "reviews", reviews); | |
| 990 | notePhase( | |
| 991 | ctx.request, | |
| 992 | "render", | |
| 993 | Math.max(0, performance.now() - began - diffMs - checks - reviews), | |
| 994 | ); | |
| 995 | return response; | |
| 961 | 996 | } |
| 962 | 997 | |
| 963 | 998 | // GET /:owner/:name/pulls/:number/thread?after=<id> |
test/pull-timing.test.ts +35 −1
| @@ -1,4 +1,4 @@ | ||
| 1 | /** `/:owner/:name/pulls/:n`'s Server-Timing names its phases (#3192). */ | |
| 1 | /** `/:owner/:name/pulls/:n`'s Server-Timing names its phases (#3192), and its Files page's (#3359). */ | |
| 2 | 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; |
| 3 | 3 | import { mkdtemp, rm } from "node:fs/promises"; |
| 4 | 4 | import { tmpdir } from "node:os"; |
| @@ -21,6 +21,7 @@ let app: App; | ||
| 21 | 21 | let servers: Servers; |
| 22 | 22 | let cookie = ""; |
| 23 | 23 | let number = 0; |
| 24 | let bigNumber = 0; | |
| 24 | 25 | let owner: User; |
| 25 | 26 | let repoRow: Repo; |
| 26 | 27 | const cells = process.env["FORGE_CELLS"]; |
| @@ -61,6 +62,27 @@ beforeAll(async () => { | ||
| 61 | 62 | }); |
| 62 | 63 | if (!pull.ok) throw new Error(JSON.stringify(pull.error)); |
| 63 | 64 | number = pull.value.number; |
| 65 | // A 40-file, 2,000-line change, for the Files page. | |
| 66 | const big = await commitToBranch(app, alice, repo.value, { | |
| 67 | branch: "big", | |
| 68 | message: "big", | |
| 69 | files: Array.from({ length: 40 }, (_, f) => ({ | |
| 70 | path: `src/m${f}.ts`, | |
| 71 | content: Array.from( | |
| 72 | { length: 50 }, | |
| 73 | (_, i) => `export const v${f}_${i} = "<b>${i}</b>" + (${i} < 3 ? 'x' : \`y\${${i}}\`); // note & ${f}`, | |
| 74 | ).join("\n") + "\n", | |
| 75 | })), | |
| 76 | }); | |
| 77 | if (!big.ok) throw new Error(JSON.stringify(big.error)); | |
| 78 | const bigPull = await openPull(app, alice, repo.value, { | |
| 79 | title: "big", | |
| 80 | body: "", | |
| 81 | baseBranch: "main", | |
| 82 | headBranch: "big", | |
| 83 | }); | |
| 84 | if (!bigPull.ok) throw new Error(JSON.stringify(bigPull.error)); | |
| 85 | bigNumber = bigPull.value.number; | |
| 64 | 86 | createRun(app.db, { |
| 65 | 87 | repoId: repo.value.id, |
| 66 | 88 | kind: "check", |
| @@ -94,6 +116,18 @@ test("a pull names session, diff, checks, reviews and render after app, summing | ||
| 94 | 116 | expect(ms("checks")).toBeGreaterThan(0); |
| 95 | 117 | }); |
| 96 | 118 | |
| 119 | test("a pull's Files page names session, diff, checks, reviews and render, in that order, on the 40-file seed", async () => { | |
| 120 | const url = `${servers.url}/alice/r/pulls/${bigNumber}/files`; | |
| 121 | const res = await fetch(url, { headers: { cookie } }); | |
| 122 | expect(res.status).toBe(200); | |
| 123 | const html = await res.text(); | |
outdated
This pins the order of the names only. Nothing fails if the commitLog read leaves the diff phase. Add an assertion that ties diff to the log read. |
||
| 124 | expect(html).toContain("m39.ts"); | |
| 125 | expect(html).not.toContain("<b>1</b>"); | |
| 126 | const parts = (res.headers.get("server-timing") ?? "").split(", "); | |
| 127 | const names = parts.map((p) => p.split(";")[0]!); | |
| 128 | expect(names.slice(names.indexOf("app") + 1)).toEqual(["session", "diff", "checks", "reviews", "render"]); | |
| 129 | }); | |
| 130 | ||
| 97 | 131 | /** The `app` duration of one Files-page view, in ms. */ |
| 98 | 132 | async function filesApp(): Promise<number> { |
| 99 | 133 | const res = await fetch(`${servers.url}/alice/r/pulls/${number}/files`, { headers: { cookie } }); |
new above
offline
lap and tick duplicate timePhase and timePhaseAsync in core/route_timing.ts, with the difference that they accumulate. Move the accumulating form into route_timing.ts so other pages can reuse it.