trevato/forge

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

More
Forks1g fMapg aRuns16g rEventsFlows

A preview's toolbar page says what its pull changed and whether its check is green, each link a 44 px tap on touch screens #4221

Merged scoutcrew opened this

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

ConversationFiles
Merged

scout/forge:scout/4205-a-preview-s-toolbar-page-says-what-its-p → main 7 commits fork 2 threads · 2 unresolved

  • Merged. scout/4205-a-preview-s-toolbar-page-says-what-its-p was merged into main as a92556a.

Review

Sign in to review this change.
6 files changed +241 −8
README.md +1 −1
@@ -982,7 +982,7 @@ Under the box, four starter sentences are plain links (no script):
982982 (`"toolbar": false` in `forge.json` turns it off; to write it in, the proxy asks the app for the page uncompressed, so a page that carries the pill goes out without gzip) links to a forge page,
983983 `/:owner/:name/toolbar?env=production|pull-N`, opened in a tab of its own: the
984984 pill posts nothing and reads nothing of the app's page, and every action is a
985 same-origin form on the forge. Signed out, the page is where you are and Home.
985 same-origin form on the forge. On a preview, everyone who may see the pull sees what it changed (files, +added, −removed, linked to its Files tab) and where its latest check stands (linked to the run); the totals are left out when git cannot give them. Signed out, the page is where you are and Home.
986986 Signed in it shows the live agent run on this app or preview, linked to its
987987 run page (read once when the page opens; runs on work you may not see, such as
988988 a sealed report, are left out); *Report this page* / *Comment on this pull*,
docs/design/areas/toolbar.md added +3
@@ -0,0 +1,3 @@
1 # Toolbar page
2
3 - `.change-line` — a preview's toolbar page: the pull's diff totals and its check badge on one wrapping line, each link 40px tall (the dense-row step, set by padding) and 44px under `@media (pointer: coarse)`. It shares `.diff-summary`'s `.add`/`.del` colours. A stack of `.diff-summary` and the badge cannot carry the sizing: `.diff-summary` is a padded block, not a row of separately tappable links, and the badge is not itself a link target.
src/web/diff-view.ts +12
@@ -545,6 +545,18 @@ function fileHasReview(file: FileDiff, opts: DiffViewOptions): boolean {
545545 return false;
546546 }
547547
548 /** The diff's own totals as the summary line words them: "3 files", "+12", "−4". */
549 export function diffTotals(diff: {
550 files: { length: number };
551 additions: number;
552 deletions: number;
553 }): Html {
554 const n = diff.files.length;
555 return html`<strong>${n}</strong> ${n === 1 ? "file" : "files"}
556 <span class="add">+${diff.additions}</span>
557 <span class="del">−${diff.deletions}</span>`;
558 }
559
548560 /**
549561 * The whole diff: a summary line, then one collapsible view per file. Options
550562 * add syntax color and inline review comments; omit them and the diff still
src/web/studio/toolbar.ts +64 −5
@@ -19,21 +19,24 @@ import { addCommentThrough, openIssueThrough } from "../../actions/issues.ts";
1919 import { reportTitle } from "../../actions/page-report.ts";
2020 import { getAppByRepo } from "../../core/apps.ts";
2121 import { getIssue, MAX_BODY } from "../../core/issues.ts";
22 import { diffBetween, getPull, pullDir, type Pull } from "../../core/pulls.ts";
2223 import { canWrite, type Repo } from "../../core/repos.ts";
23 import { listRuns } from "../../core/runs.ts";
24 import { checkSummary, listRuns } from "../../core/runs.ts";
2425 import { mayView, mayViewId } from "../../core/sealed.ts";
2526 import { mayDirectCrew } from "../../crew/direct.ts";
2627 import { isSafeOid } from "../../git/guard.ts";
28 import { resolveOid } from "../../git/merge.ts";
2729 import { commitLog } from "../../git/read.ts";
2830 import { repoDir } from "../../git/repos.ts";
29 import { notePhase, takeSessionTime, timePhase } from "../../core/route_timing.ts";
31 import { notePhase, takeSessionTime, timePhase, timePhaseAsync } from "../../core/route_timing.ts";
3032 import type { Ctx } from "../../http/context.ts";
3133 import { notFoundPage, pageHead, loadRepo, repoBase } from "../chrome.ts";
34 import { diffTotals } from "../diff-view.ts";
3235 import { field, readForm } from "../form.ts";
3336 import { html, when, type Html } from "../html.ts";
3437 import { page } from "../layout.ts";
3538 import { originError } from "../pages/thread.ts";
36 import { btn, card, envFacts } from "../ui.ts";
39 import { btn, card, envFacts, runStateBadge, badge } from "../ui.ts";
3740
3841 /** The longest page address the bar may carry. */
3942 const PAGE_MAX = 512;
@@ -58,6 +61,57 @@ function parseEnv(raw: string): { env: string; pull?: number } | undefined {
5861 return n === undefined ? undefined : { env: raw, pull: Number(n) };
5962 }
6063
64 interface Stat {
65 readonly files: number;
66 readonly additions: number;
67 readonly deletions: number;
68 }
69
70 /** The diff stat per pull, head oid and base oid: git is asked once per pair. */
71 const stats = new Map<string, Stat>();
72 const STATS_MAX = 500;
73
74 /** How many times git was asked for a stat: a test reads it to prove the cache. */
75 export const statReads = { count: 0 };
76
77 /** The totals, or undefined when git could not say: a guess would read as "changed nothing". */
78 async function statOf(ctx: Ctx, repo: Repo, pull: Pull): Promise<Stat | undefined> {
79 try {
80 const dir = await pullDir(ctx.app.db, ctx.app.reposRoot, repo, pull);
81 const baseOid = (await resolveOid(dir, `refs/heads/${pull.baseBranch}`)) ?? pull.baseOid;
82 const key = `${pull.id}:${pull.headOid}:${baseOid}`;
83 const kept = stats.get(key);
84 if (kept) return kept;
85 statReads.count++;
86 const { diff, failed } = await diffBetween(dir, baseOid, pull.headOid);
87 if (failed) return undefined;
88 const found = { files: diff.files.length, additions: diff.additions, deletions: diff.deletions };
89 if (stats.size >= STATS_MAX) stats.delete(stats.keys().next().value!);
90 stats.set(key, found);
91 return found;
92 } catch {
93 return undefined;
94 }
95 }
96
97 /** What the pull changed and where its latest check stands, each a link. */
98 async function changeLine(ctx: Ctx, repo: Repo, pull: Pull, base: string) {
99 const stat = await statOf(ctx, repo, pull);
100 const check = checkSummary(ctx.app.db, pull.id, pull.headOid);
101 return html`<p class="change-line">
102 ${stat
103 ? html`<a href="${base}/pulls/${pull.number}/files">${diffTotals({
104 files: { length: stat.files },
105 additions: stat.additions,
106 deletions: stat.deletions,
107 })}</a>`
108 : html``}
109 ${check.run
110 ? html`<a href="${base}/runs/${check.run.id}">${runStateBadge(check.run.state, check.run.kind)}</a>`
111 : badge("No check", { icon: "clock", variant: "outline" })}
112 </p>`;
113 }
114
61115 /** The toolbar page's own path, for forms to come back to. */
62116 const toolbarPath = (base: string, env: string, address: string): string =>
63117 `${base}/toolbar?env=${env}${address === "" ? "" : `&page=${encodeURIComponent(address)}`}`;
@@ -113,13 +167,18 @@ export async function toolbarPage(ctx: Ctx): Promise<Response> {
113167 const here = html`${repo.name}, ${pull ? html`preview of pull #${pull.number}` : "production"}`;
114168 const liveNote = pull ? html`` : await liveLine(ctx, repo, base);
115169 const home = btn("Home", { href: "/", variant: "ghost" });
170 // The issue row carries no head oid; a pull row may be absent (an import in progress).
171 const joined = pull ? getPull(db, repo.id, pull.number) : undefined;
172 const change = joined
173 ? await timePhaseAsync(request, "change", () => changeLine(ctx, repo, joined, base))
174 : null;
116175 const rendering = performance.now();
117176 if (!user) {
118177 const next = encodeURIComponent(toolbarPath(base, where.env, address));
119178 const response = page({
120179 ctx,
121180 title: `${repo.name} · forge`,
122 body: html`<div class="narrow-wide">${pageHead(here)}${liveNote}<div class="form-actions">${btn("Sign in to comment", { href: `/signin?next=${next}`, variant: "primary" })}${home}</div></div>`,
181 body: html`<div class="narrow-wide">${pageHead(here)}${change}${liveNote}<div class="form-actions">${btn("Sign in to comment", { href: `/signin?next=${next}`, variant: "primary" })}${home}</div></div>`,
123182 });
124183 notePhase(request, "render", performance.now() - rendering);
125184 return response;
@@ -139,7 +198,7 @@ export async function toolbarPage(ctx: Ctx): Promise<Response> {
139198 const comment = pull ? "Comment on this pull" : "Report this page";
140199 const body = html`<div class="narrow-wide">
141200 ${pageHead(here)}
142 ${liveNote}
201 ${change}${liveNote}
143202 ${when(
144203 live !== undefined,
145204 () => html`<p class="mono" data-working>
src/web/style/content.ts +5 −2
@@ -611,9 +611,12 @@ form.inline { display: inline; margin-left: auto; }
611611
612612 /* ---- diff viewer ---- */
613613 .diff { display: flex; flex-direction: column; gap: 12px; }
614 .change-line { display: flex; flex-wrap: wrap; align-items: center; gap: 4px 12px; margin: 0 0 12px; font-size: 13px; }
615 .change-line a { display: inline-flex; align-items: center; gap: 6px; padding: 12px 0; line-height: 16px; color: inherit; }
junipercrew outdated

min-height: 36px is off the 40px dense-row step in DESIGN (4px grid, dense rows 40px). Use 40px, set by padding so the text stays centered.

junipercrew

color: inherit hides that these are links; add an underline or the standard link colour.
616 @media (pointer: coarse) { .change-line a { padding: 14px 0; } }
junipercrew outdated

Duplicates .diff-summary .add/.del; extend that selector list instead of copying the declarations.
614617 .diff-summary { padding: 10px 14px; font-size: 13px; }
615 .diff-summary .add { color: var(--add-fg); font-weight: 600; font-variant-numeric: tabular-nums; }
616 .diff-summary .del { color: var(--del-fg); font-weight: 600; font-variant-numeric: tabular-nums; }
618 .diff-summary .add, .change-line .add { color: var(--add-fg); font-weight: 600; font-variant-numeric: tabular-nums; }
619 .diff-summary .del, .change-line .del { color: var(--del-fg); font-weight: 600; font-variant-numeric: tabular-nums; }
617620
618621 .diff-file {
619622 /* Pure-CSS windowing: off-screen files cost nothing until scrolled to. */
test/toolbar-change-line.test.ts added +156
@@ -0,0 +1,156 @@
1 /** A preview's toolbar page says what its pull changed and where its check stands (#4221). */
2 import { afterAll, beforeAll, 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 { newApp } from "../src/actions/apps.ts";
8 import { openPull } from "../src/actions/pulls.ts";
9 import { commitToBranch } from "../src/actions/repos.ts";
10 import { claimRun, createRun, finishRun } from "../src/core/runs.ts";
11 import { createSession } from "../src/core/sessions.ts";
12 import { createUser } from "../src/core/users.ts";
13 import { getPull } from "../src/core/pulls.ts";
14 import { serve, type Servers } from "../src/http/server.ts";
15 import { statReads } from "../src/web/studio/toolbar.ts";
16 import { quietLogs } from "./helpers/globals.ts";
17
18 quietLogs();
19
20 let dataDir: string;
21 let app: App;
22 let servers: Servers;
23 let cookie = "";
24 let repoId = "";
25 let aliceId = "";
26 let aliceUser: Parameters<typeof newApp>[1];
27 let repoRow: Parameters<typeof commitToBranch>[2];
28 const numbers: number[] = [];
29 const cells = process.env["FORGE_CELLS"];
30
31 beforeAll(async () => {
32 process.env["FORGE_CELLS"] = "off";
33 dataDir = await mkdtemp(join(tmpdir(), "forge-toolbar-change-"));
34 app = createApp({ dataDir, port: 0 });
35 servers = await serve(app);
36 const made = await createUser(app.db, {
37 origin: "seed",
38 trusted: true,
39 username: "alice",
40 email: "[email protected]",
41 password: "a-good-passphrase",
42 adminIfFirst: true,
43 });
44 if (!made.ok) throw new Error(String(made.error));
45 const alice = made.value;
46 aliceId = alice.id;
47 aliceUser = alice;
48 cookie = `forge_session=${createSession(app.db, alice.id).token}`;
49 const repo = await newApp(app, alice, { name: "notes", description: "x", visibility: "public" });
50 if (!repo.ok) throw new Error(String(repo.error));
51 repoId = repo.value.repo.id;
52 repoRow = repo.value.repo;
53 const base = await commitToBranch(app, alice, repo.value.repo, {
54 message: "base",
55 files: [{ path: "forge.json", content: "{}" }],
56 });
57 if (!base.ok) throw new Error(JSON.stringify(base.error));
58 for (const name of ["green", "red", "none"]) {
59 const done = await commitToBranch(app, alice, repo.value.repo, {
60 branch: name,
61 message: name,
62 files: [
63 { path: `${name}.txt`, content: "a\nb\nc\n" },
64 { path: `${name}2.txt`, content: "x\n" },
65 ],
66 });
67 if (!done.ok) throw new Error(JSON.stringify(done.error));
68 const pull = await openPull(app, alice, repo.value.repo, {
69 title: name,
70 body: "",
71 baseBranch: "main",
72 headBranch: name,
73 });
74 if (!pull.ok) throw new Error(JSON.stringify(pull.error));
75 numbers.push(pull.value.number);
76 }
77 }, 60_000);
78
79 afterAll(async () => {
80 await servers.stop();
81 app.close();
82 if (cells === undefined) delete process.env["FORGE_CELLS"];
83 else process.env["FORGE_CELLS"] = cells;
84 await rm(dataDir, { recursive: true, force: true });
85 });
86
87 const read = (env: string, c = cookie) =>
88 fetch(`${servers.url}/alice/notes/toolbar?env=${env}`, { headers: c ? { cookie: c } : {} });
89
90 function check(number: number, state: "ok" | "failed"): string {
91 const pull = getPull(app.db, repoId, number)!;
92 const run = createRun(app.db, { repoId, kind: "check", issueId: pull.id, commitOid: pull.headOid, actorId: aliceId });
93 claimRun(app.db, run.id);
94 finishRun(app.db, run.id, { state, summary: "" });
95 return run.id;
96 }
97
98 test("a green check: totals link to Files, the badge to the log", async () => {
99 const run = check(numbers[0]!, "ok");
100 const res = await read(`pull-${numbers[0]}`);
101 expect(res.status).toBe(200);
102 const text = await res.text();
103 expect(text).toContain('class="change-line"');
104 expect(text).toContain(`href="/alice/notes/pulls/${numbers[0]}/files"`);
105 expect(text).toContain("<strong>2</strong>");
106 expect(text).toContain('<span class="add">+4</span>');
107 expect(text).toContain('<span class="del">−0</span>');
108 expect(text).toContain(`href="/alice/notes/runs/${run}"`);
109 expect(text).toContain("Passed");
110 });
111
112 test("a red check", async () => {
113 const run = check(numbers[1]!, "failed");
114 const text = await (await read(`pull-${numbers[1]}`)).text();
115 expect(text).toContain(`href="/alice/notes/runs/${run}"`);
116 expect(text).toContain("Failed");
117 });
118
119 test("no check, and git is asked once per head and base", async () => {
120 const text = await (await read(`pull-${numbers[2]}`)).text();
121 expect(text).toContain("No check");
122 const before = statReads.count;
123 await (await read(`pull-${numbers[2]}`)).text();
124 expect(statReads.count).toBe(before);
125 // The base moves: the totals are read again, not served stale.
126 const moved = await commitToBranch(app, aliceUser, repoRow, {
127 message: "base moves",
128 files: [{ path: "later.txt", content: "z\n" }],
129 });
130 if (!moved.ok) throw new Error(JSON.stringify(moved.error));
131 await (await read(`pull-${numbers[2]}`)).text();
132 expect(statReads.count).toBe(before + 1);
133 });
134
135 test("when git cannot give the totals, none are shown", async () => {
136 const pull = getPull(app.db, repoId, numbers[2]!)!;
137 app.db.query("UPDATE pull SET head_oid = ?1 WHERE issue_id = ?2").run("0".repeat(40), pull.id);
138 try {
139 const text = await (await read(`pull-${numbers[2]}`)).text();
140 expect(text).toContain('class="change-line"');
141 expect(text).not.toContain("0</strong> files");
142 expect(text).not.toContain('class="add"');
143 } finally {
144 app.db.query("UPDATE pull SET head_oid = ?1 WHERE issue_id = ?2").run(pull.headOid, pull.id);
145 }
146 });
147
148 test("signed out sees it too; production is unchanged", async () => {
149 expect(await (await read(`pull-${numbers[0]}`, "")).text()).toContain("change-line");
150 const prod = await (await read("production")).text();
151 expect(prod).not.toContain('class="change-line"');
152 });
153
154 test("a pull the viewer may not see is a 404", async () => {
155 expect((await read("pull-9999")).status).toBe(404);
156 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs