trevato/forge

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

More
Forks1g fMapg aRuns14g rEventsFlows

A pull page with a generated file in it answers in milliseconds, not seconds #31

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/27-a-pull-page-with-a-generated-file-in-it → main 2 commits fork

  • Merged. scout/27-a-pull-page-with-a-generated-file-in-it was merged into main as a7cce5a.

Review

Sign in to review this change.
8 files changed +254 −19
src/web/diff-syntax.ts +5 −5
@@ -11,12 +11,12 @@ import { blobForRender } from "../git/read.ts";
1111 import { detectLanguage, highlightLines } from "./highlight.ts";
1212 import {
1313 MAX_DIFF_FILES,
14 stubFor,
1415 type FileTokens,
1516 type SyntaxMap,
1617 } from "./diff-view.ts";
1718
1819 const CAP_BYTES = 1_000_000;
19 const MAX_CHANGED = 5000;
2020
2121 export async function diffSyntax(
2222 dir: string,
@@ -25,11 +25,11 @@ export async function diffSyntax(
2525 newRev: string,
2626 ): Promise<SyntaxMap> {
2727 const map: SyntaxMap = new Map();
28 // Only the files the viewer will actually render are worth highlighting;
29 // this is the bound that keeps a huge diff from spawning thousands of
30 // git subprocesses.
28 // Only the files the viewer will actually render as a table are worth
29 // highlighting — a stubbed file shows no hunks, so tokenising it would
30 // burn the same cost the stub exists to avoid.
3131 for (const file of diff.files.slice(0, MAX_DIFF_FILES)) {
32 if (file.binary || file.additions + file.deletions > MAX_CHANGED) continue;
32 if (file.binary || stubFor(file) !== null) continue;
3333 const entry: FileTokens = {};
3434 if (file.oldPath) {
3535 const blob = await blobForRender(dir, oldRev, file.oldPath, CAP_BYTES);
src/web/diff-view.ts +91 −7
@@ -61,6 +61,10 @@ export interface DiffViewOptions {
6161 /** Base path for comment forms and "+" links, e.g. `/owner/repo/pulls/3`.
6262 * When set, lines become commentable. */
6363 reviewBase?: string;
64 /** Base path for a stubbed file's "View file" link, e.g.
65 * `/owner/repo/blob/<oid>` — the file's path is appended. Omit to render
66 * the stub without a link (a live/origin diff has no local blob route). */
67 fileLinkBase?: string;
6468 }
6569
6670 /** Per-file render context, resolved once per file. */
@@ -79,9 +83,63 @@ const MAX_FILE_LINES = 5000;
7983 // commit is a DoS, not something anyone reads top to bottom.
8084 export const MAX_DIFF_FILES = 100;
8185
86 // A file's diff renders as a stub, not a table, once it holds this many
87 // bytes — a bound on the diff's own content, independent of MAX_FILE_LINES,
88 // because a committed bundle can carry megabytes in only a handful of lines.
89 export const MAX_FILE_BYTES = 100_000;
90
91 // ...or once any single line is this wide. Catches a minified file even when
92 // MAX_FILE_LINES and MAX_FILE_BYTES both pass — a page must never grow wider
93 // than its viewport because one line happened to be short in count.
94 export const MAX_LINE_WIDTH = 2000;
95
8296 const GENERATED =
8397 /(^|\/)(package-lock\.json|bun\.lock|yarn\.lock|pnpm-lock\.yaml|go\.sum|Cargo\.lock)$/;
8498
99 /** Why a file's diff was replaced with a stub, in one sentence fragment. */
100 export interface FileStub {
101 readonly reason: string;
102 }
103
104 /** Total bytes and the widest single line across a file's hunks. The hunks
105 * are already parsed in memory, so this costs nothing extra to compute. */
106 function diffSize(file: FileDiff): { bytes: number; longestLine: number } {
107 let bytes = 0;
108 let longestLine = 0;
109 for (const hunk of file.hunks) {
110 for (const line of hunk.lines) {
111 bytes += line.content.length;
112 if (line.content.length > longestLine) longestLine = line.content.length;
113 }
114 }
115 return { bytes, longestLine };
116 }
117
118 /** Whether a file's diff exceeds the render budget, and why — checked before
119 * building rows so an oversized file never reaches the syntax highlighter
120 * (diff-syntax.ts) or the page (fileBody below). Binary files have their own
121 * short-circuit and are never stubbed. */
122 export function stubFor(file: FileDiff): FileStub | null {
123 const changed = file.additions + file.deletions;
124 if (changed > MAX_FILE_LINES) {
125 return {
126 reason: `${changed.toLocaleString()} changed lines (limit ${MAX_FILE_LINES.toLocaleString()})`,
127 };
128 }
129 const { bytes, longestLine } = diffSize(file);
130 if (bytes > MAX_FILE_BYTES) {
131 return {
132 reason: `${bytes.toLocaleString()} bytes of diff (limit ${MAX_FILE_BYTES.toLocaleString()})`,
133 };
134 }
135 if (longestLine > MAX_LINE_WIDTH) {
136 return {
137 reason: `a line ${longestLine.toLocaleString()} characters wide (limit ${MAX_LINE_WIDTH.toLocaleString()})`,
138 };
139 }
140 return null;
141 }
142
85143 function statusLabel(status: FileDiff["status"]): string {
86144 switch (status) {
87145 case "added":
@@ -368,20 +426,46 @@ function hunkHeader(hunk: Hunk): Html {
368426 </tr>`;
369427 }
370428
429 /** A stub in place of hunks: the path is already in the header above this,
430 * inside the same `<details>`; this adds why, the real counts, and a way to
431 * read the file without paying for it here. No hunks reach the HTML. */
432 function stubBody(file: FileDiff, stub: FileStub, opts: DiffViewOptions): Html {
433 const href = opts.fileLinkBase ? `${opts.fileLinkBase}/${file.path}` : null;
434 return html`<div class="diff-note diff-stub">
435 <p>
436 Diff not shown — ${stub.reason}.
437 <span class="diff-stat"
438 >${when(
439 file.additions > 0,
440 () => html`<span class="add">+${file.additions}</span>`,
441 )}
442 ${when(
443 file.deletions > 0,
444 () => html`<span class="del">−${file.deletions}</span>`,
445 )}</span
446 >
447 </p>
448 ${when(
449 href,
450 (h) =>
451 html`<a class="btn" data-size="sm" data-variant="outline" href="${h}"
452 >${icon("file")} View file</a
453 >`,
454 )}
455 </div>`;
456 }
457
371458 function fileBody(file: FileDiff, ctx: RowCtx): Html {
372459 if (file.binary) {
373460 return html`<div class="diff-note">Binary file not shown.</div>`;
374461 }
375 const changed = file.additions + file.deletions;
376 if (changed > MAX_FILE_LINES) {
377 return html`<div class="diff-note">
378 Large diff not shown — ${file.additions.toLocaleString()} additions,
379 ${file.deletions.toLocaleString()} deletions.
380 </div>`;
381 }
382462 if (file.hunks.length === 0) {
383463 return html`<div class="diff-note">No content changes.</div>`;
384464 }
465 const stub = stubFor(file);
466 if (stub && !fileHasReview(file, ctx.opts)) {
467 return stubBody(file, stub, ctx.opts);
468 }
385469 return html`<table class="diff-table">
386470 <tbody>
387471 ${file.hunks.map(
src/web/pages/commits.ts +6 −1
@@ -134,7 +134,12 @@ export function renderCommit(
134134 { extra: "commit-head" },
135135 );
136136 return html`<div class="mt">${head}</div>
137 <div class="mt">${renderDiff(diff, syntax)}</div>`;
137 <div class="mt">
138 ${renderDiff(diff, {
139 syntax,
140 fileLinkBase: `${scope.base}/blob/${commit.oid}`,
141 })}
142 </div>`;
138143 }
139144
140145 /** The ref switcher (and path crumbs) above a history list. */
src/web/pages/pull.ts +1
@@ -692,6 +692,7 @@ export async function pullPage(ctx: Ctx): Promise<Response> {
692692 syntax,
693693 comments: reviewComments,
694694 threads: threadViews,
695 fileLinkBase: `${base}/blob/${status.headOid}`,
695696 ...(ctx.auth.user ? { reviewBase: thread } : {}),
696697 ...(active ? { active } : {}),
697698 };
src/web/style/content.ts +2
@@ -424,6 +424,8 @@ a.origin-chip:hover { border-color: var(--border-strong); color: var(--fg); }
424424 .diff-stat .add { color: var(--add-fg); }
425425 .diff-stat .del { color: var(--del-fg); }
426426 .diff-note { padding: 20px 16px; text-align: center; color: var(--muted-fg); font-size: 13px; }
427 .diff-stub p { margin: 0 0 10px; }
428 .diff-stub .diff-stat { display: inline-flex; }
427429 .diff-viewed {
428430 display: inline-flex; align-items: center; gap: 5px; flex: none;
429431 font-size: 12px; color: var(--muted-fg); cursor: pointer; margin: 0;
test/apps.test.ts +10
@@ -496,6 +496,16 @@ describe("apps: production, previews, data, gates, crew", () => {
496496 r.commitOid === committed.value.commitOid,
497497 );
498498 expect(done.state).toBe("ok");
499 // Close it out: a later test counts open previews and expects only the
500 // one it opens itself.
501 const merged = await mergePull(
502 app,
503 alice,
504 repo,
505 getPull(app.db, repo.id, opened.value.number)!,
506 "merge",
507 );
508 expect(merged.ok).toBe(true);
499509 }, 30_000);
500510
501511 test("the crew: an agent reads and writes its repo, never the default branch, never a merge", async () => {
test/diff-edge-cases.test.ts +13 −5
@@ -340,7 +340,9 @@ describe("diff engine + resource edge cases", () => {
340340 `[perf] 210k-line file diff took ${elapsed.toFixed(0)}ms, ${text.length} bytes`,
341341 );
342342 expect(resp.status).toBe(200);
343 expect(text).toContain("Large diff not shown");
343 expect(text).toContain("huge.txt");
344 expect(text).toContain("Diff not shown");
345 expect(text).toContain("210,000 changed lines");
344346 // Confirmed clean: the cap makes this fast and small, not a resource sink.
345347 expect(elapsed).toBeLessThan(5_000);
346348 }, 60_000);
@@ -367,9 +369,15 @@ describe("diff engine + resource edge cases", () => {
367369 `[perf] 2MB-single-line modify diff took ${elapsed.toFixed(0)}ms, ${text.length} bytes`,
368370 );
369371 expect(resp.status).toBe(200);
370 // Confirmed clean: CAP_BYTES in diff-syntax.ts bounds this independent of
371 // line count, so this stays fast even though the changed-line count (1)
372 // is far under MAX_CHANGED.
373 expect(elapsed).toBeLessThan(5_000);
372 expect(text).toContain("hugeline.txt");
373 expect(text).toContain("Diff not shown");
374 expect(text).toContain("bytes of diff");
375 // The byte ceiling this issue asks for: a 4MB-of-diff single line must
376 // not put anywhere near that on the wire, and must not creep back up.
377 expect(text.length).toBeLessThan(500_000);
378 // Confirmed clean: MAX_FILE_BYTES/MAX_LINE_WIDTH in diff-view.ts stub the
379 // file before it is ever tokenized or rendered as rows, so this stays
380 // fast even though the changed-line count (1) is far under MAX_FILE_LINES.
381 expect(elapsed).toBeLessThan(200);
374382 }, 60_000);
375383 });
test/diff.test.ts +126 −1
@@ -2,9 +2,25 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test";
22 import { mkdtemp, rm } from "node:fs/promises";
33 import { tmpdir } from "node:os";
44 import { join } from "node:path";
5 import { diffCommit, diffRange, parseUnifiedDiff } from "../src/git/diff.ts";
5 import {
6 diffCommit,
7 diffRange,
8 parseUnifiedDiff,
9 type Diff,
10 type DiffLine,
11 type FileDiff,
12 type Hunk,
13 } from "../src/git/diff.ts";
614 import { initBareRepo, repoDir } from "../src/git/repos.ts";
715 import { unwrap } from "../src/kernel/result.ts";
16 import { anchorKey, type ReviewComment } from "../src/core/reviews.ts";
17 import {
18 MAX_FILE_BYTES,
19 MAX_LINE_WIDTH,
20 renderDiff,
21 stubFor,
22 type DiffViewOptions,
23 } from "../src/web/diff-view.ts";
824
925 const env = {
1026 ...process.env,
@@ -191,3 +207,112 @@ describe("diff against real git", () => {
191207 expect(bad.ok).toBe(false);
192208 });
193209 });
210
211 // ---- the diff view's render budget (src/web/diff-view.ts), pure and fast:
212 // no git, no server, just FileDiff fixtures built by hand.
213 describe("renderDiff stub budget (the view, not the engine)", () => {
214 function row(
215 kind: DiffLine["kind"],
216 oldNumber: number | null,
217 newNumber: number | null,
218 content: string,
219 ): DiffLine {
220 return { kind, oldNumber, newNumber, content, noNewline: false };
221 }
222
223 function hunk(lines: DiffLine[]): Hunk {
224 return {
225 oldStart: 1,
226 oldLines: lines.filter((l) => l.kind !== "add").length,
227 newStart: 1,
228 newLines: lines.filter((l) => l.kind !== "del").length,
229 section: "",
230 lines,
231 };
232 }
233
234 function file(path: string, hunks: Hunk[]): FileDiff {
235 const lines = hunks.flatMap((h) => h.lines);
236 return {
237 status: "modified",
238 oldPath: path,
239 newPath: path,
240 path,
241 binary: false,
242 additions: lines.filter((l) => l.kind === "add").length,
243 deletions: lines.filter((l) => l.kind === "del").length,
244 similarity: null,
245 hunks,
246 };
247 }
248
249 function oneFile(f: FileDiff): Diff {
250 return { files: [f], additions: f.additions, deletions: f.deletions };
251 }
252
253 test("a normal small file renders its hunk, not a stub", () => {
254 const f = file("small.txt", [
255 hunk([
256 row("context", 1, 1, "one"),
257 row("del", 2, null, "two"),
258 row("add", null, 2, "TWO"),
259 ]),
260 ]);
261 expect(stubFor(f)).toBeNull();
262 const out = renderDiff(oneFile(f)).toString();
263 expect(out).toContain("small.txt");
264 expect(out).toContain("diff-table");
265 expect(out).not.toContain("Diff not shown");
266 });
267
268 test("one line wider than MAX_LINE_WIDTH is stubbed even though the file is tiny", () => {
269 const wide = "x".repeat(MAX_LINE_WIDTH + 500);
270 const f = file("bundle.min.js", [hunk([row("add", null, 1, wide)])]);
271 expect(stubFor(f)?.reason).toContain("characters wide");
272 const out = renderDiff(oneFile(f)).toString();
273 expect(out).toContain("bundle.min.js"); // path
274 expect(out).toContain("Diff not shown"); // why
275 expect(out).toContain("characters wide");
276 expect(out).toContain("+1"); // the real counts, not hidden
277 expect(out).not.toContain(wide); // no hunks in the HTML
278 });
279
280 test("a file over MAX_FILE_BYTES is stubbed even with only a handful of lines", () => {
281 const lines = Array.from({ length: 10 }, (_, i) =>
282 row("add", null, i + 1, "y".repeat(Math.ceil(MAX_FILE_BYTES / 5))),
283 );
284 const f = file("data.json", [hunk(lines)]);
285 expect(stubFor(f)?.reason).toContain("bytes of diff");
286 const out = renderDiff(oneFile(f)).toString();
287 expect(out).toContain("Diff not shown");
288 expect(out).toContain("bytes of diff");
289 });
290
291 test("a stubbed file with an existing review comment renders in full instead", () => {
292 const wide = "x".repeat(MAX_LINE_WIDTH + 500);
293 const f = file("bundle.min.js", [hunk([row("add", null, 1, wide)])]);
294 const comments = new Map<string, ReviewComment[]>([
295 [anchorKey("bundle.min.js", "new", 1), []],
296 ]);
297 const opts: DiffViewOptions = { comments };
298 const out = renderDiff(oneFile(f), opts).toString();
299 expect(out).toContain("diff-table");
300 expect(out).not.toContain("Diff not shown");
301 });
302
303 test("the stub links to the file at that commit when fileLinkBase is set", () => {
304 const wide = "x".repeat(MAX_LINE_WIDTH + 500);
305 const f = file("bundle.min.js", [hunk([row("add", null, 1, wide)])]);
306 const opts: DiffViewOptions = { fileLinkBase: "/o/r/blob/deadbeef" };
307 const out = renderDiff(oneFile(f), opts).toString();
308 expect(out).toContain('href="/o/r/blob/deadbeef/bundle.min.js"');
309 });
310
311 test("no fileLinkBase means no link, not a crash", () => {
312 const wide = "x".repeat(MAX_LINE_WIDTH + 500);
313 const f = file("bundle.min.js", [hunk([row("add", null, 1, wide)])]);
314 const out = renderDiff(oneFile(f)).toString();
315 expect(out).toContain("Diff not shown");
316 expect(out).not.toContain("View file");
317 });
318 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs