Public: anyone can read this; the crew can change it.
A preview's toolbar page lists the files its pull changed, each a link to that file's diff #7850
Check · Review · Land · Live done — Landed and live in production.
torque/forge:torque/7848-a-preview-s-toolbar-page-lists-the-files → main 2 commits fork
5 files
changed +65
−8
docs/design/areas/toolbar.md +1
| @@ -1,3 +1,4 @@ | ||
| 1 | 1 | # Toolbar page |
| 2 | 2 | |
| 3 | 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. |
| 4 | - `.change-files` — under `.change-line`, the first ten changed paths of the pull, one per row: a mono link to that file's diff and its `.add`/`.del` counts, the last row "and K more" to the files page. Rows are 40px tall (12px link padding around a 16px line-height; 44px under `@media (pointer: coarse)`, 14px padding). It reuses the `.add`/`.del` colour rules of `.diff-summary` but is a list, not a line of inline links. |
src/web/diff-view.ts +6 −2
| @@ -161,9 +161,13 @@ function statusLabel(status: FileDiff["status"]): string { | ||
| 161 | 161 | } |
| 162 | 162 | } |
| 163 | 163 | |
| 164 | function fileAnchor(file: FileDiff): string { | |
| 164 | export function pathAnchor(path: string): string { | |
| 165 | 165 | // Stable per-path id for deep-linking; not security-sensitive. |
| 166 | return "d-" + escapeHtml(file.path).replace(/[^a-zA-Z0-9]+/g, "-"); | |
| 166 | return "d-" + escapeHtml(path).replace(/[^a-zA-Z0-9]+/g, "-"); | |
| 167 | } | |
| 168 | ||
| 169 | function fileAnchor(file: FileDiff): string { | |
| 170 | return pathAnchor(file.path); | |
| 167 | 171 | } |
| 168 | 172 | |
| 169 | 173 | /** Per-character changed mask for a line, from its word-diff segments. */ |
src/web/studio/toolbar.ts +25 −3
| @@ -34,7 +34,7 @@ import { repoDir } from "../../git/repos.ts"; | ||
| 34 | 34 | import { notePhase, takeSessionTime, timePhase, timePhaseAsync } from "../../core/route_timing.ts"; |
| 35 | 35 | import type { Ctx } from "../../http/context.ts"; |
| 36 | 36 | import { notFoundPage, pageHead, loadRepo, repoBase } from "../chrome.ts"; |
| 37 | import { diffTotals } from "../diff-view.ts"; | |
| 37 | import { diffTotals, pathAnchor } from "../diff-view.ts"; | |
| 38 | 38 | import { field, readForm } from "../form.ts"; |
| 39 | 39 | import { html, when, type Html } from "../html.ts"; |
| 40 | 40 | import { page } from "../layout.ts"; |
| @@ -69,8 +69,13 @@ interface Stat { | ||
| 69 | 69 | readonly files: number; |
| 70 | 70 | readonly additions: number; |
| 71 | 71 | readonly deletions: number; |
| 72 | /** The first FILES_SHOWN files: the cache keeps no more than the page shows. */ | |
| 73 | readonly top: readonly { path: string; additions: number; deletions: number }[]; | |
| 72 | 74 | } |
| 73 | 75 | |
| 76 | /** How many changed paths the page lists; the rest are "and K more". */ | |
| 77 | export const FILES_SHOWN = 10; | |
| 78 | ||
| 74 | 79 | /** The diff stat per pull, head oid and base oid: git is asked once per pair. */ |
| 75 | 80 | const stats = new Map<string, Stat>(); |
| 76 | 81 | const STATS_MAX = 500; |
| @@ -89,7 +94,11 @@ async function statOf(ctx: Ctx, repo: Repo, pull: Pull): Promise<Stat | undefine | ||
| 89 | 94 | statReads.count++; |
| 90 | 95 | const { diff, failed } = await diffBetween(dir, baseOid, pull.headOid); |
| 91 | 96 | if (failed) return undefined; |
| 92 | const found = { files: diff.files.length, additions: diff.additions, deletions: diff.deletions }; | |
| 97 | const found = { files: diff.files.length, additions: diff.additions, deletions: diff.deletions, | |
| 98 | top: diff.files | |
| 99 | .slice(0, FILES_SHOWN) | |
| 100 | .map((f) => ({ path: f.path, additions: f.additions, deletions: f.deletions })), | |
| 101 | }; | |
| 93 | 102 | if (stats.size >= STATS_MAX) stats.delete(stats.keys().next().value!); |
| 94 | 103 | stats.set(key, found); |
| 95 | 104 | return found; |
| @@ -98,6 +107,19 @@ async function statOf(ctx: Ctx, repo: Repo, pull: Pull): Promise<Stat | undefine | ||
| 98 | 107 | } |
| 99 | 108 | } |
| 100 | 109 | |
| 110 | /** The first changed paths, each linking to its diff on the files page. */ | |
| 111 | function changedFiles(stat: Stat, number: number, base: string): Html { | |
| 112 | const files = `${base}/pulls/${number}/files`; | |
| 113 | const more = stat.files - stat.top.length; | |
| 114 | return html`<ul class="change-files" aria-label="Files this pull changed"> | |
| 115 | ${stat.top.map( | |
| 116 | (f) => html`<li><a href="${files}#${pathAnchor(f.path)}" class="mono">${f.path}</a> | |
| 117 | <span class="add" aria-label="${f.additions} added">+${f.additions}</span> <span class="del" aria-label="${f.deletions} removed">−${f.deletions}</span></li>`, | |
| 118 | )} | |
| 119 | ${when(more > 0, () => html`<li><a href="${files}">and ${more} more</a></li>`)} | |
| 120 | </ul>`; | |
| 121 | } | |
| 122 | ||
| 101 | 123 | /** What the pull changed and where its latest check stands, each a link. */ |
| 102 | 124 | async function changeLine(ctx: Ctx, repo: Repo, pull: Pull, base: string) { |
| 103 | 125 | const stat = await statOf(ctx, repo, pull); |
| @@ -113,7 +135,7 @@ async function changeLine(ctx: Ctx, repo: Repo, pull: Pull, base: string) { | ||
| 113 | 135 | ${check.run |
| 114 | 136 | ? html`<a href="${base}/runs/${check.run.id}">${runStateBadge(check.run.state, check.run.kind)}</a>` |
| 115 | 137 | : badge("No check", { icon: "clock", variant: "outline" })} |
| 116 | </p>`; | |
| 138 | </p>${stat && stat.top.length > 0 ? changedFiles(stat, pull.number, base) : html``}`; | |
| 117 | 139 | } |
| 118 | 140 | |
| 119 | 141 | /** The toolbar page's own path, for forms to come back to. */ |
src/web/style/content.ts +6 −3
| @@ -372,10 +372,13 @@ form.inline { display: inline; margin-left: auto; } | ||
| 372 | 372 | .diff { display: flex; flex-direction: column; gap: 12px; } |
| 373 | 373 | .change-line { display: flex; flex-wrap: wrap; align-items: center; gap: 4px 12px; margin: 0 0 12px; font-size: 13px; } |
| 374 | 374 | .change-line a { display: inline-flex; align-items: center; gap: 6px; padding: 12px 0; line-height: 16px; color: inherit; } |
| 375 | @media (pointer: coarse) { .change-line a { padding: 14px 0; } } | |
| 375 | .change-files { list-style: none; margin: 0 0 12px; padding: 0; font-size: 13px; } | |
| 376 | .change-files li { display: flex; flex-wrap: wrap; gap: 0 8px; align-items: center; } | |
| 377 | .change-files a { padding: 12px 0; line-height: 16px; color: inherit; overflow-wrap: anywhere; } | |
| 378 | @media (pointer: coarse) { .change-line a, .change-files a { padding: 14px 0; } } | |
| 376 | 379 | .diff-summary { padding: 10px 14px; font-size: 13px; } |
| 377 | .diff-summary .add, .change-line .add { color: var(--add-fg); font-weight: 600; font-variant-numeric: tabular-nums; } | |
| 378 | .diff-summary .del, .change-line .del { color: var(--del-fg); font-weight: 600; font-variant-numeric: tabular-nums; } | |
| 380 | .diff-summary .add, .change-line .add, .change-files .add { color: var(--add-fg); font-weight: 600; font-variant-numeric: tabular-nums; } | |
| 381 | .diff-summary .del, .change-line .del, .change-files .del { color: var(--del-fg); font-weight: 600; font-variant-numeric: tabular-nums; } | |
| 379 | 382 | |
| 380 | 383 | .diff-file { |
| 381 | 384 | /* Pure-CSS windowing: off-screen files cost nothing until scrolled to. */ |
test/toolbar-change-line.test.ts +27
| @@ -154,3 +154,30 @@ test("signed out sees it too; production is unchanged", async () => { | ||
| 154 | 154 | test("a pull the viewer may not see is a 404", async () => { |
| 155 | 155 | expect((await read("pull-9999")).status).toBe(404); |
| 156 | 156 | }); |
| 157 | ||
| 158 | test("lists changed files with anchors, counts, escaping, and a cut-off", async () => { | |
| 159 | const files = Array.from({ length: 12 }, (_, i) => ({ path: `d/f${String(i).padStart(2, "0")}.txt`, content: "a\n" })); | |
| 160 | files.push({ path: "d/<b>x.txt", content: "a\nb\n" }); | |
| 161 | const done = await commitToBranch(app, aliceUser, repoRow, { branch: "many", message: "many", files }); | |
| 162 | if (!done.ok) throw new Error(JSON.stringify(done.error)); | |
| 163 | const pull = await openPull(app, aliceUser, repoRow, { title: "many", body: "", baseBranch: "main", headBranch: "many" }); | |
| 164 | if (!pull.ok) throw new Error(JSON.stringify(pull.error)); | |
| 165 | const n = pull.value.number; | |
| 166 | const text = await (await read(`pull-${n}`)).text(); | |
| 167 | expect(text).toContain('class="change-files"'); | |
| 168 | expect(text).toContain(`href="/alice/notes/pulls/${n}/files#d-d-f00-txt"`); | |
| 169 | expect(text).toContain('<span class="add" aria-label="1 added">+1</span>'); | |
| 170 | expect(text).not.toContain("<b>x"); | |
| 171 | expect(text).toContain("d/<b>x.txt"); | |
| 172 | const block = /<ul class="change-files"[\s\S]*?<\/ul>/.exec(text)![0]; | |
| 173 | expect(block).toContain('aria-label="Files this pull changed"'); | |
| 174 | expect(block).toContain('aria-label="2 added"'); | |
| 175 | const listed = block.match(/<li>/g)!.length; | |
| 176 | expect(listed).toBe(11); | |
| 177 | expect(text).toContain("and 3 more"); | |
| 178 | expect(text).toContain(`href="/alice/notes/pulls/${n}/files">and 3 more`); | |
| 179 | const before = statReads.count; | |
| 180 | await (await read(`pull-${n}`)).text(); | |
| 181 | expect(statReads.count).toBe(before); | |
| 182 | expect(await (await read("production")).text()).not.toContain("change-files"); | |
| 183 | }); |
new above
offline