trevato/forge

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

More
Forks1g fMapg aRuns14g rEventsFlows

A pull's Files page reuses its rendered diff for the same head, and nothing viewer-specific is shared #4093

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/4086-a-pull-s-files-page-reuses-its-rendered → main 2 commits fork 3 threads · 3 unresolved

  • Merged. scout/4086-a-pull-s-files-page-reuses-its-rendered was merged into main as 34a52d6.

Review

Sign in to review this change.
4 files changed +281 −9
src/core/pulls.ts +17 −6
@@ -601,15 +601,26 @@ export async function pullDiff(
601601 gate?: ReviewGate,
602602 ): Promise<PullDiff> {
603603 const status = await pullStatus(dir, pull, gate);
604 const from =
605 (await mergeBase(dir, status.baseOid, status.headOid)) ?? status.baseOid;
606 const result = await diffRange(dir, from, status.headOid);
607 const diff = result.ok
608 ? result.value
609 : { files: [], additions: 0, deletions: 0 };
604 const { from, diff } = await diffBetween(dir, status.baseOid, status.headOid);
610605 return { status, from, diff };
611606 }
612607
608 /**
609 * The diff of a head against its merge base with `baseOid`. `failed` says git
610 * could not produce it, so the empty diff is a stand-in no one should keep.
611 */
612 export async function diffBetween(
613 dir: string,
614 baseOid: string,
615 headOid: string,
616 ): Promise<{ from: string; diff: Diff; failed: boolean }> {
617 const from = (await mergeBase(dir, baseOid, headOid)) ?? baseOid;
618 const result = await diffRange(dir, from, headOid);
619 return result.ok
620 ? { from, diff: result.value, failed: false }
621 : { from, diff: { files: [], additions: 0, deletions: 0 }, failed: true };
622 }
623
613624 /**
614625 * Where the review gate stands on this pull, at this head. `mergeable` here
615626 * is the *review* verdict alone — git may still refuse the merge — and it is
src/web/diff-cache.ts added +92
@@ -0,0 +1,92 @@
1 /**
2 * The Files page's diff work, kept for the next reader of the same head.
3 *
4 * What costs time on `/pulls/N/files` is reading git and highlighting whole
5 * blobs; both are pure functions of two commit oids. So that is what is kept:
6 * the parsed diff and its syntax map, keyed by base oid, head oid and the
7 * view's options. Nothing viewer-specific enters the key or the value —
8 * review links, forms, threads and marks are drawn after it, per request.
9 * In memory only, bounded by entries and by estimated bytes, least recently
10 * used out first.
11 */
12
13 import type { Diff } from "../git/diff.ts";
14 import type { SyntaxMap } from "./diff-view.ts";
15
16 export interface KeptDiff {
17 /** The merge base the diff was taken from. */
18 readonly from: string;
19 readonly diff: Diff;
20 readonly syntax: SyntaxMap;
21 /** Git failed and this is a stand-in: serve it, never keep it. */
22 readonly failed?: boolean;
junipercrew

Nothing passes a DiffView yet. Keep it only when the whitespace and layout controls land.
23 }
24
25 /** The view options that change the diff itself. */
26 export interface DiffView {
27 readonly whitespace?: "keep" | "ignore";
28 readonly layout?: "unified" | "split";
29 }
30
31 export const MAX_ENTRIES = 32;
32 export const MAX_BYTES = 32 * 1024 * 1024;
33
34 const kept = new Map<string, { value: KeptDiff; bytes: number }>();
35 let total = 0;
36 let hits = 0;
37 let misses = 0;
38
39 /** What has been counted so far, for a Server-Timing phase and for tests. */
40 export function diffCacheCounts(): { hits: number; misses: number; entries: number; bytes: number } {
41 return { hits, misses, entries: kept.size, bytes: total };
42 }
43
44 export function diffKey(baseOid: string, headOid: string, view: DiffView = {}): string {
45 return `${baseOid}:${headOid}:${view.whitespace ?? "keep"}:${view.layout ?? "unified"}`;
46 }
47
48 /** A rough size: the text the diff and its tokens hold, doubled for overhead. */
49 export function sizeOf(value: KeptDiff): number {
50 let n = 0;
51 for (const file of value.diff.files)
52 for (const hunk of file.hunks) for (const line of hunk.lines) n += line.content.length + 32;
53 for (const tokens of value.syntax.values())
54 for (const side of [tokens.old, tokens.new])
55 for (const line of side ?? []) for (const t of line) n += t.text.length + 48;
56 return n * 2;
57 }
58
59 /**
60 * The kept diff for `key`, or what `make` builds, which is kept when it fits.
61 * Callers must treat the value as read-only.
62 */
63 export async function keptDiff(key: string, make: () => Promise<KeptDiff>): Promise<KeptDiff> {
64 const known = kept.get(key);
65 if (known !== undefined) {
66 kept.delete(key);
67 kept.set(key, known);
68 hits++;
69 return known.value;
70 }
71 misses++;
72 const value = await make();
73 const bytes = sizeOf(value);
74 if (value.failed !== true && bytes <= MAX_BYTES && !kept.has(key)) {
75 kept.set(key, { value, bytes });
76 total += bytes;
77 while (kept.size > MAX_ENTRIES || total > MAX_BYTES) {
78 const oldest = kept.keys().next().value as string;
79 total -= kept.get(oldest)!.bytes;
80 kept.delete(oldest);
81 }
82 }
83 return value;
84 }
85
86 /** Forget everything (tests). */
87 export function clearDiffCache(): void {
88 kept.clear();
89 total = 0;
90 hits = 0;
91 misses = 0;
92 }
src/web/pages/pull.ts +16 −3
@@ -26,7 +26,7 @@ import { getAppByRepo } from "../../core/apps.ts";
2626 import { labelsFor, listLabels } from "../../core/issues.ts";
2727 import { marksByName } from "../../core/marks.ts";
2828 import {
29 pullDiff,
29 diffBetween,
3030 pullDir,
3131 pullStatus,
3232 type MergePullError,
@@ -52,6 +52,7 @@ import {
5252 import { getUsersByIds } from "../../core/users.ts";
5353 import { canWrite, getRepoById, type Repo } from "../../core/repos.ts";
5454 import { resolveOid } from "../../git/merge.ts";
55 import { timePhaseAsync } from "../../core/route_timing.ts";
5556 import { commitLog } from "../../git/read.ts";
5657 import { repoDir } from "../../git/repos.ts";
5758 import { wallOf } from "../../contract/policy.ts";
@@ -64,6 +65,7 @@ import {
6465 repoPalette,
6566 signInFirst,
6667 } from "../chrome.ts";
68 import { diffCacheCounts, diffKey, keptDiff } from "../diff-cache.ts";
6769 import { diffSyntax } from "../diff-syntax.ts";
6870 import {
6971 renderDiff,
@@ -805,11 +807,22 @@ export async function pullFilesPage(ctx: Ctx): Promise<Response> {
805807
806808 // The review gate travels with the status, so the merge box and
807809 // `mergePull` are answering the same question from the same numbers.
808 const { status, from, diff } = await pullDiff(dir, pull, {
810 const status = await pullStatus(dir, pull, {
809811 db: ctx.app.db,
810812 requireReview: repo.requireReview,
811813 });
812 const syntax = await diffSyntax(dir, diff, from, status.headOid);
814 // The diff and its highlighting depend on two oids and nothing else, so
815 // 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);
junipercrew outdated

This repeats pullDiff's logic from core/pulls.ts. Extract one shared helper.
822 return { from, diff, failed, syntax: await diffSyntax(dir, diff, from, status.headOid) };
823 }),
824 diffCacheCounts,
junipercrew outdated

The failure fallback is cached along with real diffs. Return it uncached, and test that.
825 );
813826 // A merged or closed pull is history; the wall is a question about landing.
814827 const wall =
815828 pull.mergedAt === null && pull.state === "open"
test/pull-files-cache.test.ts added +156
@@ -0,0 +1,156 @@
1 /** The Files page keeps its diff work per head and never shares a viewer's markup (#4086). */
2 import { afterAll, beforeAll, beforeEach, 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 { diffBetween } from "../src/core/pulls.ts";
13 import { repoDir } from "../src/git/repos.ts";
14 import { clearDiffCache, diffCacheCounts, diffKey, keptDiff, MAX_ENTRIES } from "../src/web/diff-cache.ts";
15 import { quietLogs } from "./helpers/globals.ts";
16
17 quietLogs();
18
19 let dataDir: string;
20 let app: App;
21 let servers: Servers;
22 let alice: Parameters<typeof commitToBranch>[1] & {};
23 let repo: Parameters<typeof openPull>[2];
24 let aliceCookie = "";
25 let bobCookie = "";
26 let number = 0;
27 const cells = process.env["FORGE_CELLS"];
28
29 const files = (path: string, content: string, branch?: string) =>
30 commitToBranch(app, alice, repo, {
31 ...(branch === undefined ? {} : { branch }),
32 message: "change",
33 files: [{ path, content }],
34 });
35
36 beforeAll(async () => {
37 process.env["FORGE_CELLS"] = "off";
38 dataDir = await mkdtemp(join(tmpdir(), "forge-files-cache-"));
39 app = createApp({ dataDir, port: 0 });
40 servers = await serve(app);
41 const made = await createUser(app.db, {
42 origin: "seed", trusted: true, username: "alice", email: "[email protected]",
43 password: "a-good-passphrase", adminIfFirst: true,
44 });
45 if (!made.ok) throw new Error(String(made.error));
46 alice = made.value;
47 aliceCookie = `forge_session=${createSession(app.db, alice.id).token}`;
48 const bob = await createUser(app.db, {
49 origin: "seed", trusted: true, username: "bob", email: "[email protected]",
50 password: "a-good-passphrase",
51 });
52 if (!bob.ok) throw new Error(String(bob.error));
53 bobCookie = `forge_session=${createSession(app.db, bob.value.id).token}`;
54 const made2 = await createRepo(app, alice, { name: "r", visibility: "public" });
55 if (!made2.ok) throw new Error(String(made2.error));
56 repo = made2.value;
57 for (const r of [files("forge.json", "{}"), files("t.js", "const a = 1;\n", "topic")]) {
58 const done = await r;
59 if (!done.ok) throw new Error(JSON.stringify(done.error));
60 }
61 const pull = await openPull(app, alice, repo, { title: "t", body: "", baseBranch: "main", headBranch: "topic" });
62 if (!pull.ok) throw new Error(JSON.stringify(pull.error));
63 number = pull.value.number;
64 });
65
66 beforeEach(() => clearDiffCache());
67
68 afterAll(async () => {
69 await servers.stop();
70 app.close();
71 if (cells === undefined) delete process.env["FORGE_CELLS"];
72 else process.env["FORGE_CELLS"] = cells;
73 await rm(dataDir, { recursive: true, force: true });
74 });
75
76 const view = async (cookie?: string, query = "") => {
77 const res = await fetch(`${servers.url}/alice/r/pulls/${number}/files${query}`, {
78 headers: cookie ? { cookie } : {},
79 });
80 const text = await res.text();
81 return { res, text };
82 };
83 const diffPhase = (res: Response) =>
84 (res.headers.get("server-timing") ?? "").split(", ").find((p) => p.startsWith("diff;"));
85 const rc = "?rc=t.js~new~1";
86
87 test("a second view of the same head reads the cache", async () => {
88 const first = await view(aliceCookie);
89 expect(first.res.status).toBe(200);
90 expect(diffPhase(first.res)).not.toContain("hit");
91 expect(diffCacheCounts()).toMatchObject({ hits: 0, misses: 1, entries: 1 });
92 const second = await view(aliceCookie);
93 expect(diffPhase(second.res)).toContain('desc="hit"');
94 expect(diffCacheCounts()).toMatchObject({ hits: 1, misses: 1 });
95 expect(second.text).toContain("const");
96 });
97
98 test("viewers with different permissions each see only their own forms", async () => {
99 // The anonymous view warms the cache; the signed-in ones must not inherit it.
100 const anon = await view(undefined, rc);
101 const mine = await view(aliceCookie, rc);
102 const other = await view(bobCookie, rc);
103 expect(diffPhase(mine.res)).toContain('desc="hit"');
104 expect(anon.text).not.toContain(`/pulls/${number}/review"`);
105 expect(anon.text).not.toContain("?rc=");
106 expect(mine.text).toContain(`/pulls/${number}/review"`);
107 expect(other.text).toContain(`/pulls/${number}/review"`);
108 // The writer is offered strictly more forms than a reader, and a reader more than a guest.
109 const actions = (t: string) => new Set([...t.matchAll(/action="([^"]*)"/g)].map((m) => m[1]!));
110 const [forAlice, forBob, forAnon] = [actions(mine.text), actions(other.text), actions(anon.text)];
111 expect(forAlice.size).toBeGreaterThan(forBob.size);
112 expect([...forBob].every((a) => forAlice.has(a))).toBe(true);
113 expect(forBob.size).toBeGreaterThan(forAnon.size);
114 // And no viewer's session or token leaks into another's page.
115 expect(anon.text).not.toContain(aliceCookie.split("=")[1]!);
116 expect(other.text).not.toContain(aliceCookie.split("=")[1]!);
117 });
118
119 test("a new head on the same pull renders fresh", async () => {
120 await view(aliceCookie);
121 const done = await files("u.js", "const b = 2;\n", "topic");
122 if (!done.ok) throw new Error(JSON.stringify(done.error));
123 const after = await view(aliceCookie);
124 expect(diffPhase(after.res)).not.toContain("hit");
125 expect(after.text).toContain("u.js");
126 expect(diffCacheCounts().entries).toBe(2);
127 });
128
129 test("the cache is bounded by entries", async () => {
130 const make = async () => ({ from: "f", diff: { files: [], additions: 0, deletions: 0 }, syntax: new Map() });
131 for (let i = 0; i < MAX_ENTRIES + 5; i++) await keptDiff(diffKey("b", `h${i}`), make);
132 expect(diffCacheCounts().entries).toBe(MAX_ENTRIES);
133 await keptDiff(diffKey("b", "h0"), make);
134 expect(diffCacheCounts().misses).toBe(MAX_ENTRIES + 6);
135 });
136
137 test("whitespace and layout options key separately", () => {
138 expect(diffKey("b", "h")).not.toBe(diffKey("b", "h", { whitespace: "ignore" }));
139 expect(diffKey("b", "h")).not.toBe(diffKey("b", "h", { layout: "split" }));
140 });
141
142 test("a failed diff is served but never kept", async () => {
143 const stand = { from: "f", diff: { files: [], additions: 0, deletions: 0 }, syntax: new Map(), failed: true };
144 let made = 0;
145 const make = async () => (made++, stand);
146 await keptDiff("k", make);
147 await keptDiff("k", make);
148 expect(made).toBe(2);
149 expect(diffCacheCounts()).toMatchObject({ entries: 0, hits: 0 });
150 });
151
152 test("diffBetween flags a git failure", async () => {
153 const dir = repoDir(app.reposRoot, "alice", "r");
154 const bad = "0".repeat(40);
155 expect((await diffBetween(dir, bad, bad)).failed).toBe(true);
156 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs