0f7985b · c957917 · 955663c
@@ -449,7 +449,7 @@
only, shows a guest nothing, and names nothing of a placement in a repository
you cannot read.
Review: `/review` lists the asks waiting on you, one row each (title, repository,
age, why, and the crew's advice or that it has not advised yet), newest first. A pull's row also carries the check verdict for its head (the pull page's badge: passed, failed, running, cancelled, or none) in the row's trail, and, when its repository is an app, an "Open the preview" link under it (shown for any app, even before a preview is deployed); both come from the database, never git. It only reads; the buttons stay on `/asks`, which links
age, why, and the crew's advice or that it has not advised yet), newest first. A pull's row also carries the check verdict for its head (the pull page's badge: passed, failed, running, cancelled, or none) in the row's trail, and, when its repository is an app, an "Open the preview" link under it (shown for any app, even before a preview is deployed); both come from the database, never git. The row's trail also says the pull's size (`3 files changed, +40 −7`), kept per base and head commit (`src/web/studio/review_evidence.ts`); a request never waits on git: until the stat is kept the row says "Diff loading — reload for the size" while the app computes it in the background (at most 4 at once, stopped by closing the app), and a failed stat says "Diff size unavailable" and is not asked again for a minute, so a bad pull cannot spawn git on every visit. Visits while a stat is running share it. It only reads; the buttons stay on `/asks`, which links
to it, and signed out it sends you to sign-in. The terminal has no page of its
own for it: the same asks are the dashboard's On you card.
@@ -0,0 +1,152 @@
/**
 * A pull's diff stat for the `/review` row: files changed, additions and
 * deletions, kept per (base oid, head oid). A request only reads the cache;
 * a miss is computed in the background, at most `LIMIT` at once, by the app
 * that asked, so `app.close()` (its `closing` signal) kills the git processes
 * and starts no more. Only a successful stat is kept: a failure is asked
 * again, but not before `RETRY_MS` has passed, so a pull whose diff always
 * fails cannot turn each visit into a git process.
 */
import { pullDir, type Pull } from "../../core/pulls.ts";
import type { App } from "../../app.ts";
import { runGit } from "../../git/env.ts";
import { isSafeOid } from "../../git/guard.ts";
export interface DiffStat {
  readonly files: number;
  readonly additions: number;
  readonly deletions: number;
}
const LIMIT = 4;
const KEPT_MAX = 500;
const RETRY_MS = 60_000;
interface Work {
  readonly kept: Map<string, DiffStat>;
  readonly queued: Map<string, () => Promise<void>>;
  readonly running: Set<Promise<void>>;
  /** Keys queued or running: one git per pair, however many visits. */
  readonly inflight: Set<string>;
  /** Keys whose stat failed, until when. */
  readonly failed: Map<string, number>;
}
// Per app, never per module: nothing exists until a request asks, and a
// closed app takes its work with it.
const works = new WeakMap<App, Work>();
/** `git diff --numstat` lines to totals; a binary file counts as a file with no lines. */
export function parseNumstat(text: string): DiffStat {
  let files = 0;
  let additions = 0;
  let deletions = 0;
  for (const line of text.split("\n")) {
    const m = /^(\d+|-)\t(\d+|-)\t/.exec(line);
    if (m === null) continue;
    files++;
    additions += m[1] === "-" ? 0 : Number(m[1]);
    deletions += m[2] === "-" ? 0 : Number(m[2]);
  }
  return { files, additions, deletions };
}
function workOf(app: App): Work {
  let w = works.get(app);
  if (w === undefined) works.set(app, (w = { kept: new Map(), queued: new Map(), running: new Set(), inflight: new Set(), failed: new Map() }));
  return w;
}
function pump(app: App, w: Work): void {
  while (!app.closing.signal.aborted && w.running.size < LIMIT) {
    const next = w.queued.entries().next();
    if (next.done) return;
    const [key, job] = next.value;
    w.queued.delete(key);
    const p: Promise<void> = job()
      .catch(() => {})
      .finally(() => {
        w.running.delete(p);
        pump(app, w);
      });
    w.running.add(p);
  }
}
/**
 * The stat if it is kept; "failed" while a recent attempt failed; otherwise
 * "loading", with the computation started in the background (once per pair).
 * Never waits on git.
 */
export function diffStatFor(
  app: App,
  repo: { ownerName: string; name: string },
      
        junipercrew
        23d ago
        outdated
      
      Only queued is checked. Once pump() has moved the job to running, the same key is queued again by the next request. Check an in-flight set as well.
    
  pull: Pick<Pull, "baseOid" | "headOid" | "sealed" | "crossRepo" | "headRepoId" | "baseBranch">,
): DiffStat | "loading" | "failed" {
  const w = workOf(app);
  const key = `${pull.baseOid}:${pull.headOid}`;
  const kept = w.kept.get(key);
  if (kept !== undefined) return kept;
  if (!isSafeOid(pull.baseOid) || !isSafeOid(pull.headOid)) return "failed";
      
        junipercrew
        23d ago
        outdated
      
      A failure is forgotten at once, so a pull whose diff always fails or times out spawns git on every visit. Keep a short negative entry.
    
  const failedUntil = w.failed.get(key);
  if (failedUntil !== undefined) {
    if (failedUntil > Date.now()) return "failed";
    w.failed.delete(key);
  }
  if (app.closing.signal.aborted || w.inflight.has(key)) return "loading";
  w.inflight.add(key);
  w.queued.set(key, async () => {
    try {
      await compute(app, w, key, repo, pull);
    } catch {
      if (!app.closing.signal.aborted) w.failed.set(key, Date.now() + RETRY_MS);
    } finally {
      w.inflight.delete(key);
    }
  });
  pump(app, w);
  return "loading";
}
async function compute(
      
        junipercrew
        23d ago
        
      
      Bare block with no purpose; remove it and dedent.
    
  app: App,
  w: Work,
  key: string,
  repo: { ownerName: string; name: string },
  pull: Pick<Pull, "baseOid" | "headOid" | "sealed" | "crossRepo" | "headRepoId" | "baseBranch">,
): Promise<void> {
  const dir = await pullDir(app.db, app.reposRoot, repo, pull);
  const out = await runGit(["diff", "--numstat", "--no-color", "--no-renames", `${pull.baseOid}...${pull.headOid}`, "--"], {
    cwd: dir,
    timeoutMs: 30_000,
    signal: app.closing.signal,
  });
  if (app.closing.signal.aborted) return;
  if (!out.ok) {
    w.failed.set(key, Date.now() + RETRY_MS);
    return;
  }
  keep(w, key, parseNumstat(out.value));
}
function keep(w: Work, key: string, stat: DiffStat): void {
  if (w.kept.size >= KEPT_MAX) w.kept.delete(w.kept.keys().next().value!);
  w.kept.set(key, stat);
}
/** Seed the cache for a (base, head) pair: for tests that need a known size without a repository. */
export function rememberStat(app: App, baseOid: string, headOid: string, stat: DiffStat): void {
  keep(workOf(app), `${baseOid}:${headOid}`, stat);
}
/** `3 files changed, +40 −7`. */
export function sizeLine(s: DiffStat): string {
  return `${s.files} ${s.files === 1 ? "file" : "files"} changed, +${s.additions} −${s.deletions}`;
}
/** Resolves when the app's background stats have all settled: for tests. */
export async function settled(app: App): Promise<void> {
  const w = works.get(app);
  while (w !== undefined && w.running.size > 0) await Promise.allSettled([...w.running]);
}
@@ -17,6 +17,7 @@
import { html, type Html } from "../html.ts";
import { page } from "../layout.ts";
import { badge, empty, listCard, row, runStateBadge, time } from "../ui.ts";
import { waitingOn } from "./asks.ts";
import { diffStatFor, sizeLine } from "./review_evidence.ts";
/** The check on a pull's head, as the pull page wears it; only a run of this very head counts. */
function verdict(run: Run | undefined, headOid: string): Html {
@@ -60,14 +61,25 @@
export function reviewPage(ctx: Ctx): Response {
      rec === undefined
        ? "The crew has not advised yet"
        : html`The crew advises: ${rec.verdict}${rec.reason === "" ? "" : html` — ${rec.reason}`}`;
    const stat = a.pull === undefined ? undefined : diffStatFor(ctx.app, a.repo, a.pull);
    const pending =
      stat === "loading"
        ? " · Diff loading — reload for the size"
        : stat === "failed"
      
        junipercrew
        23d ago
        outdated
      
      A failed stat and a pending one both read "Diff loading", and nothing says to reload. Word the failed state differently and tell the reader how to get the number.
    
        ? " · Diff size unavailable — open the pull to see it"
        : "";
    return row({
      href,
      nav: true,
      
        junipercrew
        23d ago
        outdated
      
      Size is the last item of an ellipsised meta line, so it is dropped first on 360px. Move it to the trail or its own line.
    
      title: a.pull?.title ?? a.issue?.title ?? a.proposal?.title ?? `${a.repo.name} is waiting on you`,
      titleAttr: `${a.repo.ownerName}/${a.repo.name}: ${a.why ?? a.consequence.why}`,
      meta: html`${a.repo.ownerName}/${a.repo.name} · ${time(a.since)} · ${a.why ?? a.consequence.why} · ${advised}`,
      // The trail survives the meta line's ellipsis: the verdict is the fact this row gives.
      ...(a.pull === undefined ? {} : { trail: verdict(checks.get(a.pull.id), a.pull.headOid) }),
      meta: html`${a.repo.ownerName}/${a.repo.name} · ${time(a.since)} · ${a.why ?? a.consequence.why} · ${advised}${pending}`,
      // The trail survives the meta line's ellipsis: the verdict and the size are the facts this row gives.
      ...(a.pull === undefined
        ? {}
        : {
            trail: html`${typeof stat === "object" ? html`<span class="small muted">${sizeLine(stat)}</span> ` : ""}${verdict(checks.get(a.pull.id), a.pull.headOid)}`,
          }),
      // A sibling of the row's link, never inside it: a link in a link is not a link.
      ...(a.pull !== undefined && apps.has(a.repo.id)
        ? { children: [row({ href: `${base}/app/${previewEnv(a.pull.number)}/open`, title: "Open the preview", nav: true })] }
@@ -0,0 +1,120 @@
/** `/review`'s diff stat: cached per base and head, never waited on, stopped with the app. */
import { afterAll, beforeAll, expect, test } from "bun:test";
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import type { App } from "../src/app.ts";
import { diffStatFor, parseNumstat, settled } from "../src/web/studio/review_evidence.ts";
let root = "";
let base = "";
let head = "";
const repo = { ownerName: "o", name: "r" };
const pullOf = (b: string, h: string) =>
  ({ baseOid: b, headOid: h, sealed: false, crossRepo: false, headRepoId: "", baseBranch: "main" }) as const;
const fakeApp = () => ({ closing: new AbortController(), reposRoot: root, db: null }) as unknown as App;
const git = (cwd: string, ...args: string[]) => {
  const r = Bun.spawnSync(["git", ...args], { cwd, env: { ...process.env, GIT_AUTHOR_NAME: "t", GIT_AUTHOR_EMAIL: "t@t", GIT_COMMITTER_NAME: "t", GIT_COMMITTER_EMAIL: "t@t" } });
  return r.stdout.toString().trim();
};
beforeAll(() => {
  root = mkdtempSync(join(tmpdir(), "forge-evidence-"));
  const dir = join(root, "o", "r.git");
  mkdirSync(dir, { recursive: true });
  git(dir, "init", "--bare", "-b", "main");
  const work = join(root, "work");
  mkdirSync(work);
  git(work, "init", "-b", "main");
  writeFileSync(join(work, "a.txt"), "one\ntwo\n");
  git(work, "add", ".");
  git(work, "commit", "-m", "base");
  base = git(work, "rev-parse", "HEAD");
  writeFileSync(join(work, "a.txt"), "one\n");
  writeFileSync(join(work, "b.txt"), "x\ny\nz\n");
  git(work, "add", ".");
  git(work, "commit", "-m", "head");
  head = git(work, "rev-parse", "HEAD");
  git(work, "push", dir, "main");
});
afterAll(() => rmSync(root, { recursive: true, force: true }));
function countSpawns<T>(f: () => Promise<T>): Promise<{ value: T; spawns: number }> {
  const real = Bun.spawn;
  let spawns = 0;
  (Bun as { spawn: unknown }).spawn = ((...a: unknown[]) => (spawns++, (real as (...x: unknown[]) => unknown)(...a))) as unknown;
  return f().then((value) => ({ value, spawns })).finally(() => ((Bun as { spawn: unknown }).spawn = real));
}
test("numstat totals count files, lines and binary files", () => {
  expect(parseNumstat("3\t1\ta\n-\t-\tb.png\n")).toEqual({ files: 2, additions: 3, deletions: 1 });
});
test("a cold request is loading and computes in the background; a warm one makes no git reads", async () => {
  const app = fakeApp();
  expect(diffStatFor(app, repo, pullOf(base, head))).toBe("loading");
  await settled(app);
  const warm = await countSpawns(async () => diffStatFor(app, repo, pullOf(base, head)));
  expect(warm.value).toEqual({ files: 2, additions: 3, deletions: 1 });
  expect(warm.spawns).toBe(0);
});
test("calls while the first is running share it: git is spawned once", async () => {
  const app = fakeApp();
  const r = await countSpawns(async () => {
    const seen = [1, 2, 3].map(() => diffStatFor(app, repo, pullOf(base, head)));
    await settled(app);
    seen.push(diffStatFor(app, repo, pullOf(base, head)));
    return seen;
  });
  expect(r.spawns).toBe(1);
  expect(r.value.slice(0, 3)).toEqual(["loading", "loading", "loading"]);
  expect(r.value[3]).toEqual({ files: 2, additions: 3, deletions: 1 });
});
test("a failed stat says failed and is not asked again within the retry window", async () => {
  const app = fakeApp();
  const missing = "1".repeat(40);
  const r = await countSpawns(async () => {
    expect(diffStatFor(app, repo, pullOf(base, missing))).toBe("loading");
    await settled(app);
    return [diffStatFor(app, repo, pullOf(base, missing)), diffStatFor(app, repo, pullOf(base, missing))];
  });
  expect(r.value).toEqual(["failed", "failed"]);
  expect(r.spawns).toBe(1);
});
test("an oid git would not take is failed without a process", async () => {
  const app = fakeApp();
  const r = await countSpawns(async () => diffStatFor(app, repo, pullOf(base, "--output=x")));
  expect(r.value).toBe("failed");
  expect(r.spawns).toBe(0);
});
test("at most four run at once, and closing the app stops the work and starts none", async () => {
  const app = fakeApp();
  const real = Bun.spawn;
  let live = 0;
  let peak = 0;
  (Bun as { spawn: unknown }).spawn = ((...a: unknown[]) => {
    const p = (real as (...x: unknown[]) => { exited: Promise<number> })(...a);
    peak = Math.max(peak, ++live);
    void p.exited.then(() => live--);
    return p;
  }) as unknown;
  try {
    for (let i = 0; i < 10; i++) diffStatFor(app, repo, pullOf(base, i.toString(16).repeat(40).slice(0, 40)));
    app.closing.abort();
    await settled(app);
    expect(peak).toBeLessThanOrEqual(4);
    expect(live).toBe(0);
    const before = live;
    diffStatFor(app, repo, pullOf(base, head));
    await settled(app);
    expect(live).toBe(before);
    expect(diffStatFor(app, repo, pullOf(base, head))).toBe("loading");
  } finally {
    (Bun as { spawn: unknown }).spawn = real;
  }
});
@@ -14,6 +14,7 @@
import { createUser } from "../src/core/users.ts";
import { BREAKER_NOTE } from "../src/crew/breaker.ts";
import { serve, type Servers } from "../src/http/server.ts";
import { id } from "../src/kernel/id.ts";
import { rememberStat, settled } from "../src/web/studio/review_evidence.ts";
import { quietLogs } from "./helpers/globals.ts";
quietLogs();
@@ -172,3 +173,36 @@
test("a pull of an app links its preview beside the row, not inside it; an issue
  expect(issue).not.toContain("badge");
  expect(issue).not.toContain("No check");
});
test("a pull's row says its diff is loading or unavailable, and unavailable once git failed; an issue row says neither", async () => {
  const rowsOf = async () =>
    (await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text())
      .split("<li")
      .filter((r) => r.includes('class="row"'));
  const first = await rowsOf();
  // Earlier tests in this file may already have asked git; either way the page answered at once.
  expect(first.find((r) => r.includes("Stale pull"))!).toMatch(/Diff (loading — reload for the size|size unavailable)/);
  await settled(app);
  const second = await rowsOf();
  // The fixture pulls point at the zero oid, which git cannot diff.
  expect(second.find((r) => r.includes("Stale pull"))!).toContain("Diff size unavailable");
  expect(second.find((r) => r.includes("Stale pull"))!).not.toContain("Diff loading");
  expect(second.find((r) => r.includes("Crew asked about this"))!).not.toContain("Diff");
});
      
        junipercrew
        23d ago
        
      
      Only the loading and unavailable states are covered. Add a case that renders N files changed, +A −D and 1 file changed.
    
test("a kept stat is the sentence in the row's trail, singular for one file", async () => {
  const rowsOf = async () =>
    (await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text())
      .split("<li")
      .filter((r) => r.includes('class="row"'));
  rememberStat(app, ZERO, ZERO, { files: 3, additions: 40, deletions: 7 });
  const many = (await rowsOf()).find((r) => r.includes("Stale pull"))!;
  expect(many).toContain("3 files changed, +40 −7");
  expect(many.slice(many.indexOf("row-trail"))).toContain("3 files changed, +40 −7");
  expect(many).not.toContain("Diff size unavailable");
  rememberStat(app, ZERO, ZERO, { files: 1, additions: 2, deletions: 0 });
  const one = (await rowsOf()).find((r) => r.includes("Stale pull"))!;
  expect(one).toContain("1 file changed, +2 −0");
  expect(one).not.toContain("1 files");
  expect((await rowsOf()).find((r) => r.includes("Crew asked about this"))!).not.toContain("changed");
});
		@@ -449,7 +449,7 @@ only, shows a guest nothing, and names nothing of a placement in a repository
449	449	you cannot read.
450	450
451	451	Review: `/review` lists the asks waiting on you, one row each (title, repository,
452		age, why, and the crew's advice or that it has not advised yet), newest first. A pull's row also carries the check verdict for its head (the pull page's badge: passed, failed, running, cancelled, or none) in the row's trail, and, when its repository is an app, an "Open the preview" link under it (shown for any app, even before a preview is deployed); both come from the database, never git. It only reads; the buttons stay on `/asks`, which links
	452	age, why, and the crew's advice or that it has not advised yet), newest first. A pull's row also carries the check verdict for its head (the pull page's badge: passed, failed, running, cancelled, or none) in the row's trail, and, when its repository is an app, an "Open the preview" link under it (shown for any app, even before a preview is deployed); both come from the database, never git. The row's trail also says the pull's size (`3 files changed, +40 −7`), kept per base and head commit (`src/web/studio/review_evidence.ts`); a request never waits on git: until the stat is kept the row says "Diff loading — reload for the size" while the app computes it in the background (at most 4 at once, stopped by closing the app), and a failed stat says "Diff size unavailable" and is not asked again for a minute, so a bad pull cannot spawn git on every visit. Visits while a stat is running share it. It only reads; the buttons stay on `/asks`, which links
453	453	to it, and signed out it sends you to sign-in. The terminal has no page of its
454	454	own for it: the same asks are the dashboard's On you card.
455	455
		@@ -0,0 +1,152 @@
	1	/**
	2	* A pull's diff stat for the `/review` row: files changed, additions and
	3	* deletions, kept per (base oid, head oid). A request only reads the cache;
	4	* a miss is computed in the background, at most `LIMIT` at once, by the app
	5	* that asked, so `app.close()` (its `closing` signal) kills the git processes
	6	* and starts no more. Only a successful stat is kept: a failure is asked
	7	* again, but not before `RETRY_MS` has passed, so a pull whose diff always
	8	* fails cannot turn each visit into a git process.
	9	*/
	10
	11	import { pullDir, type Pull } from "../../core/pulls.ts";
	12	import type { App } from "../../app.ts";
	13	import { runGit } from "../../git/env.ts";
	14	import { isSafeOid } from "../../git/guard.ts";
	15
	16	export interface DiffStat {
	17	readonly files: number;
	18	readonly additions: number;
	19	readonly deletions: number;
	20	}
	21
	22	const LIMIT = 4;
	23	const KEPT_MAX = 500;
	24	const RETRY_MS = 60_000;
	25
	26	interface Work {
	27	readonly kept: Map<string, DiffStat>;
	28	readonly queued: Map<string, () => Promise<void>>;
	29	readonly running: Set<Promise<void>>;
	30	/** Keys queued or running: one git per pair, however many visits. */
	31	readonly inflight: Set<string>;
	32	/** Keys whose stat failed, until when. */
	33	readonly failed: Map<string, number>;
	34	}
	35
	36	// Per app, never per module: nothing exists until a request asks, and a
	37	// closed app takes its work with it.
	38	const works = new WeakMap<App, Work>();
	39
	40	/** `git diff --numstat` lines to totals; a binary file counts as a file with no lines. */
	41	export function parseNumstat(text: string): DiffStat {
	42	let files = 0;
	43	let additions = 0;
	44	let deletions = 0;
	45	for (const line of text.split("\n")) {
	46	const m = /^(\d+\|-)\t(\d+\|-)\t/.exec(line);
	47	if (m === null) continue;
	48	files++;
	49	additions += m[1] === "-" ? 0 : Number(m[1]);
	50	deletions += m[2] === "-" ? 0 : Number(m[2]);
	51	}
	52	return { files, additions, deletions };
	53	}
	54
	55	function workOf(app: App): Work {
	56	let w = works.get(app);
	57	if (w === undefined) works.set(app, (w = { kept: new Map(), queued: new Map(), running: new Set(), inflight: new Set(), failed: new Map() }));
	58	return w;
	59	}
	60
	61	function pump(app: App, w: Work): void {
	62	while (!app.closing.signal.aborted && w.running.size < LIMIT) {
	63	const next = w.queued.entries().next();
	64	if (next.done) return;
	65	const [key, job] = next.value;
	66	w.queued.delete(key);
	67	const p: Promise<void> = job()
	68	.catch(() => {})
	69	.finally(() => {
	70	w.running.delete(p);
	71	pump(app, w);
	72	});
	73	w.running.add(p);
	74	}
	75	}
	76
	77	/**
	78	* The stat if it is kept; "failed" while a recent attempt failed; otherwise
	79	* "loading", with the computation started in the background (once per pair).
	80	* Never waits on git.
	81	*/
	82	export function diffStatFor(
	83	app: App,
	84	repo: { ownerName: string; name: string },
		junipercrew 23d ago outdated Only `queued` is checked. Once pump() has moved the job to running, the same key is queued again by the next request. Check an in-flight set as well.
	85	pull: Pick<Pull, "baseOid" \| "headOid" \| "sealed" \| "crossRepo" \| "headRepoId" \| "baseBranch">,
	86	): DiffStat \| "loading" \| "failed" {
	87	const w = workOf(app);
	88	const key = `${pull.baseOid}:${pull.headOid}`;
	89	const kept = w.kept.get(key);
	90	if (kept !== undefined) return kept;
	91	if (!isSafeOid(pull.baseOid) \|\| !isSafeOid(pull.headOid)) return "failed";
		junipercrew 23d ago outdated A failure is forgotten at once, so a pull whose diff always fails or times out spawns git on every visit. Keep a short negative entry.
	92	const failedUntil = w.failed.get(key);
	93	if (failedUntil !== undefined) {
	94	if (failedUntil > Date.now()) return "failed";
	95	w.failed.delete(key);
	96	}
	97	if (app.closing.signal.aborted \|\| w.inflight.has(key)) return "loading";
	98	w.inflight.add(key);
	99	w.queued.set(key, async () => {
	100	try {
	101	await compute(app, w, key, repo, pull);
	102	} catch {
	103	if (!app.closing.signal.aborted) w.failed.set(key, Date.now() + RETRY_MS);
	104	} finally {
	105	w.inflight.delete(key);
	106	}
	107	});
	108	pump(app, w);
	109	return "loading";
	110	}
	111
	112	async function compute(
		junipercrew 23d ago Bare block with no purpose; remove it and dedent.
	113	app: App,
	114	w: Work,
	115	key: string,
	116	repo: { ownerName: string; name: string },
	117	pull: Pick<Pull, "baseOid" \| "headOid" \| "sealed" \| "crossRepo" \| "headRepoId" \| "baseBranch">,
	118	): Promise<void> {
	119	const dir = await pullDir(app.db, app.reposRoot, repo, pull);
	120	const out = await runGit(["diff", "--numstat", "--no-color", "--no-renames", `${pull.baseOid}...${pull.headOid}`, "--"], {
	121	cwd: dir,
	122	timeoutMs: 30_000,
	123	signal: app.closing.signal,
	124	});
	125	if (app.closing.signal.aborted) return;
	126	if (!out.ok) {
	127	w.failed.set(key, Date.now() + RETRY_MS);
	128	return;
	129	}
	130	keep(w, key, parseNumstat(out.value));
	131	}
	132
	133	function keep(w: Work, key: string, stat: DiffStat): void {
	134	if (w.kept.size >= KEPT_MAX) w.kept.delete(w.kept.keys().next().value!);
	135	w.kept.set(key, stat);
	136	}
	137
	138	/** Seed the cache for a (base, head) pair: for tests that need a known size without a repository. */
	139	export function rememberStat(app: App, baseOid: string, headOid: string, stat: DiffStat): void {
	140	keep(workOf(app), `${baseOid}:${headOid}`, stat);
	141	}
	142
	143	/** `3 files changed, +40 −7`. */
	144	export function sizeLine(s: DiffStat): string {
	145	return `${s.files} ${s.files === 1 ? "file" : "files"} changed, +${s.additions} −${s.deletions}`;
	146	}
	147
	148	/** Resolves when the app's background stats have all settled: for tests. */
	149	export async function settled(app: App): Promise<void> {
	150	const w = works.get(app);
	151	while (w !== undefined && w.running.size > 0) await Promise.allSettled([...w.running]);
	152	}
		@@ -17,6 +17,7 @@ import { html, type Html } from "../html.ts";
17	17	import { page } from "../layout.ts";
18	18	import { badge, empty, listCard, row, runStateBadge, time } from "../ui.ts";
19	19	import { waitingOn } from "./asks.ts";
	20	import { diffStatFor, sizeLine } from "./review_evidence.ts";
20	21
21	22	/** The check on a pull's head, as the pull page wears it; only a run of this very head counts. */
22	23	function verdict(run: Run \| undefined, headOid: string): Html {
		@@ -60,14 +61,25 @@ export function reviewPage(ctx: Ctx): Response {
60	61	rec === undefined
61	62	? "The crew has not advised yet"
62	63	: html`The crew advises: ${rec.verdict}${rec.reason === "" ? "" : html` — ${rec.reason}`}`;
	64	const stat = a.pull === undefined ? undefined : diffStatFor(ctx.app, a.repo, a.pull);
	65	const pending =
	66	stat === "loading"
	67	? " · Diff loading — reload for the size"
	68	: stat === "failed"
		junipercrew 23d ago outdated A failed stat and a pending one both read "Diff loading", and nothing says to reload. Word the failed state differently and tell the reader how to get the number.
	69	? " · Diff size unavailable — open the pull to see it"
	70	: "";
63	71	return row({
64	72	href,
65	73	nav: true,
		junipercrew 23d ago outdated Size is the last item of an ellipsised meta line, so it is dropped first on 360px. Move it to the trail or its own line.
66	74	title: a.pull?.title ?? a.issue?.title ?? a.proposal?.title ?? `${a.repo.name} is waiting on you`,
67	75	titleAttr: `${a.repo.ownerName}/${a.repo.name}: ${a.why ?? a.consequence.why}`,
68		meta: html`${a.repo.ownerName}/${a.repo.name} · ${time(a.since)} · ${a.why ?? a.consequence.why} · ${advised}`,
69		// The trail survives the meta line's ellipsis: the verdict is the fact this row gives.
70		...(a.pull === undefined ? {} : { trail: verdict(checks.get(a.pull.id), a.pull.headOid) }),
	76	meta: html`${a.repo.ownerName}/${a.repo.name} · ${time(a.since)} · ${a.why ?? a.consequence.why} · ${advised}${pending}`,
	77	// The trail survives the meta line's ellipsis: the verdict and the size are the facts this row gives.
	78	...(a.pull === undefined
	79	? {}
	80	: {
	81	trail: html`${typeof stat === "object" ? html`<span class="small muted">${sizeLine(stat)}</span> ` : ""}${verdict(checks.get(a.pull.id), a.pull.headOid)}`,
	82	}),
71	83	// A sibling of the row's link, never inside it: a link in a link is not a link.
72	84	...(a.pull !== undefined && apps.has(a.repo.id)
73	85	? { children: [row({ href: `${base}/app/${previewEnv(a.pull.number)}/open`, title: "Open the preview", nav: true })] }
		@@ -0,0 +1,120 @@
	1	/** `/review`'s diff stat: cached per base and head, never waited on, stopped with the app. */
	2	import { afterAll, beforeAll, expect, test } from "bun:test";
	3	import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
	4	import { tmpdir } from "node:os";
	5	import { join } from "node:path";
	6	import type { App } from "../src/app.ts";
	7	import { diffStatFor, parseNumstat, settled } from "../src/web/studio/review_evidence.ts";
	8
	9	let root = "";
	10	let base = "";
	11	let head = "";
	12	const repo = { ownerName: "o", name: "r" };
	13	const pullOf = (b: string, h: string) =>
	14	({ baseOid: b, headOid: h, sealed: false, crossRepo: false, headRepoId: "", baseBranch: "main" }) as const;
	15	const fakeApp = () => ({ closing: new AbortController(), reposRoot: root, db: null }) as unknown as App;
	16
	17	const git = (cwd: string, ...args: string[]) => {
	18	const r = Bun.spawnSync(["git", ...args], { cwd, env: { ...process.env, GIT_AUTHOR_NAME: "t", GIT_AUTHOR_EMAIL: "t@t", GIT_COMMITTER_NAME: "t", GIT_COMMITTER_EMAIL: "t@t" } });
	19	return r.stdout.toString().trim();
	20	};
	21
	22	beforeAll(() => {
	23	root = mkdtempSync(join(tmpdir(), "forge-evidence-"));
	24	const dir = join(root, "o", "r.git");
	25	mkdirSync(dir, { recursive: true });
	26	git(dir, "init", "--bare", "-b", "main");
	27	const work = join(root, "work");
	28	mkdirSync(work);
	29	git(work, "init", "-b", "main");
	30	writeFileSync(join(work, "a.txt"), "one\ntwo\n");
	31	git(work, "add", ".");
	32	git(work, "commit", "-m", "base");
	33	base = git(work, "rev-parse", "HEAD");
	34	writeFileSync(join(work, "a.txt"), "one\n");
	35	writeFileSync(join(work, "b.txt"), "x\ny\nz\n");
	36	git(work, "add", ".");
	37	git(work, "commit", "-m", "head");
	38	head = git(work, "rev-parse", "HEAD");
	39	git(work, "push", dir, "main");
	40	});
	41	afterAll(() => rmSync(root, { recursive: true, force: true }));
	42
	43	function countSpawns<T>(f: () => Promise<T>): Promise<{ value: T; spawns: number }> {
	44	const real = Bun.spawn;
	45	let spawns = 0;
	46	(Bun as { spawn: unknown }).spawn = ((...a: unknown[]) => (spawns++, (real as (...x: unknown[]) => unknown)(...a))) as unknown;
	47	return f().then((value) => ({ value, spawns })).finally(() => ((Bun as { spawn: unknown }).spawn = real));
	48	}
	49
	50	test("numstat totals count files, lines and binary files", () => {
	51	expect(parseNumstat("3\t1\ta\n-\t-\tb.png\n")).toEqual({ files: 2, additions: 3, deletions: 1 });
	52	});
	53
	54	test("a cold request is loading and computes in the background; a warm one makes no git reads", async () => {
	55	const app = fakeApp();
	56	expect(diffStatFor(app, repo, pullOf(base, head))).toBe("loading");
	57	await settled(app);
	58	const warm = await countSpawns(async () => diffStatFor(app, repo, pullOf(base, head)));
	59	expect(warm.value).toEqual({ files: 2, additions: 3, deletions: 1 });
	60	expect(warm.spawns).toBe(0);
	61	});
	62
	63	test("calls while the first is running share it: git is spawned once", async () => {
	64	const app = fakeApp();
	65	const r = await countSpawns(async () => {
	66	const seen = [1, 2, 3].map(() => diffStatFor(app, repo, pullOf(base, head)));
	67	await settled(app);
	68	seen.push(diffStatFor(app, repo, pullOf(base, head)));
	69	return seen;
	70	});
	71	expect(r.spawns).toBe(1);
	72	expect(r.value.slice(0, 3)).toEqual(["loading", "loading", "loading"]);
	73	expect(r.value[3]).toEqual({ files: 2, additions: 3, deletions: 1 });
	74	});
	75
	76	test("a failed stat says failed and is not asked again within the retry window", async () => {
	77	const app = fakeApp();
	78	const missing = "1".repeat(40);
	79	const r = await countSpawns(async () => {
	80	expect(diffStatFor(app, repo, pullOf(base, missing))).toBe("loading");
	81	await settled(app);
	82	return [diffStatFor(app, repo, pullOf(base, missing)), diffStatFor(app, repo, pullOf(base, missing))];
	83	});
	84	expect(r.value).toEqual(["failed", "failed"]);
	85	expect(r.spawns).toBe(1);
	86	});
	87
	88	test("an oid git would not take is failed without a process", async () => {
	89	const app = fakeApp();
	90	const r = await countSpawns(async () => diffStatFor(app, repo, pullOf(base, "--output=x")));
	91	expect(r.value).toBe("failed");
	92	expect(r.spawns).toBe(0);
	93	});
	94
	95	test("at most four run at once, and closing the app stops the work and starts none", async () => {
	96	const app = fakeApp();
	97	const real = Bun.spawn;
	98	let live = 0;
	99	let peak = 0;
	100	(Bun as { spawn: unknown }).spawn = ((...a: unknown[]) => {
	101	const p = (real as (...x: unknown[]) => { exited: Promise<number> })(...a);
	102	peak = Math.max(peak, ++live);
	103	void p.exited.then(() => live--);
	104	return p;
	105	}) as unknown;
	106	try {
	107	for (let i = 0; i < 10; i++) diffStatFor(app, repo, pullOf(base, i.toString(16).repeat(40).slice(0, 40)));
	108	app.closing.abort();
	109	await settled(app);
	110	expect(peak).toBeLessThanOrEqual(4);
	111	expect(live).toBe(0);
	112	const before = live;
	113	diffStatFor(app, repo, pullOf(base, head));
	114	await settled(app);
	115	expect(live).toBe(before);
	116	expect(diffStatFor(app, repo, pullOf(base, head))).toBe("loading");
	117	} finally {
	118	(Bun as { spawn: unknown }).spawn = real;
	119	}
	120	});
		@@ -14,6 +14,7 @@ import { createUser } from "../src/core/users.ts";
14	14	import { BREAKER_NOTE } from "../src/crew/breaker.ts";
15	15	import { serve, type Servers } from "../src/http/server.ts";
16	16	import { id } from "../src/kernel/id.ts";
	17	import { rememberStat, settled } from "../src/web/studio/review_evidence.ts";
17	18	import { quietLogs } from "./helpers/globals.ts";
18	19
19	20	quietLogs();
		@@ -172,3 +173,36 @@ test("a pull of an app links its preview beside the row, not inside it; an issue
172	173	expect(issue).not.toContain("badge");
173	174	expect(issue).not.toContain("No check");
174	175	});
	176
	177	test("a pull's row says its diff is loading or unavailable, and unavailable once git failed; an issue row says neither", async () => {
	178	const rowsOf = async () =>
	179	(await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text())
	180	.split("<li")
	181	.filter((r) => r.includes('class="row"'));
	182	const first = await rowsOf();
	183	// Earlier tests in this file may already have asked git; either way the page answered at once.
	184	expect(first.find((r) => r.includes("Stale pull"))!).toMatch(/Diff (loading — reload for the size\|size unavailable)/);
	185	await settled(app);
	186	const second = await rowsOf();
	187	// The fixture pulls point at the zero oid, which git cannot diff.
	188	expect(second.find((r) => r.includes("Stale pull"))!).toContain("Diff size unavailable");
	189	expect(second.find((r) => r.includes("Stale pull"))!).not.toContain("Diff loading");
	190	expect(second.find((r) => r.includes("Crew asked about this"))!).not.toContain("Diff");
	191	});
		junipercrew 23d ago Only the loading and unavailable states are covered. Add a case that renders `N files changed, +A −D` and `1 file changed`.
	192
	193	test("a kept stat is the sentence in the row's trail, singular for one file", async () => {
	194	const rowsOf = async () =>
	195	(await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text())
	196	.split("<li")
	197	.filter((r) => r.includes('class="row"'));
	198	rememberStat(app, ZERO, ZERO, { files: 3, additions: 40, deletions: 7 });
	199	const many = (await rowsOf()).find((r) => r.includes("Stale pull"))!;
	200	expect(many).toContain("3 files changed, +40 −7");
	201	expect(many.slice(many.indexOf("row-trail"))).toContain("3 files changed, +40 −7");
	202	expect(many).not.toContain("Diff size unavailable");
	203	rememberStat(app, ZERO, ZERO, { files: 1, additions: 2, deletions: 0 });
	204	const one = (await rowsOf()).find((r) => r.includes("Stale pull"))!;
	205	expect(one).toContain("1 file changed, +2 −0");
	206	expect(one).not.toContain("1 files");
	207	expect((await rowsOf()).find((r) => r.includes("Crew asked about this"))!).not.toContain("changed");
	208	});