Public: anyone can read this; the crew can change it.
A pull's /review row shows its before/after look thumbnail, when the viewer may fetch it #4883
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/4791-a-pull-s-review-row-shows-its-before-aft → main 4 commits fork 1 thread · 1 unresolved
6 files
changed +92
−7
README.md +1 −1
| @@ -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. 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 | |
| 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); and, when the pull has a look the viewer may fetch, its preview thumbnail beside production's under that (sized, lazy, never inside the row's link; the pull's is the only link, named for a screen reader, and production's is a named picture; a viewer who may not fetch it sees none); all of it comes 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 |
docs/design/areas/review.md added +3
| @@ -0,0 +1,3 @@ | ||
| 1 | # Review | |
| 2 | ||
| 3 | - A pull's row on `/review` carries its look under it as `<li data-look>` in the row's children: a `.looks` strip of `.look[data-slot=thumb]` frames (production, then the pull's preview). It reuses the look component; no new class. The strip stays side by side under 640px (the stacking rule excludes `[data-look] .looks`), the item is inset like a row (8px 14px), and the preview thumb's link has a 44px hit area (a `::after` outset by 7px). |
src/web/pages/looks.ts +10 −3
| @@ -367,9 +367,16 @@ export function lookNow(ctx: Ctx, repo: Repo, record: AppRecord): Html { | ||
| 367 | 367 | </a>`; |
| 368 | 368 | } |
| 369 | 369 | |
| 370 | /** A small picture as a thumbnail that opens the pull. */ | |
| 371 | export function lookThumb(href: string, src: string): Html { | |
| 372 | return html`<a class="look" data-slot="thumb" href="${href}" | |
| 370 | /** A small picture as a thumbnail that opens the pull; `label` names the link for a screen reader. */ | |
| 371 | export function lookThumb(href: string, src: string, label?: string): Html { | |
| 372 | return html`<a class="look" data-slot="thumb" href="${href}" ${label === undefined ? html`` : html`aria-label="${label}"`} | |
| 373 | 373 | ><img src="${src}" width="30" height="40" alt="" loading="lazy" |
| 374 | 374 | /></a>`; |
| 375 | 375 | } |
| 376 | ||
| 377 | /** A small picture that is only a picture: named, not a link. */ | |
| 378 | export function lookThumbPlain(src: string, label: string): Html { | |
| 379 | return html`<span class="look" data-slot="thumb" | |
| 380 | ><img src="${src}" width="30" height="40" alt="${label}" loading="lazy" | |
| 381 | /></span>`; | |
| 382 | } |
src/web/studio/review_page.ts +22 −1
| @@ -9,6 +9,7 @@ import { notePhase, takeSessionTime, timePhase } from "../../core/route_timing.t | ||
| 9 | 9 | import { previewEnv } from "../../apps/data.ts"; |
| 10 | 10 | import { recommendationsFor } from "../../core/pull_wait.ts"; |
| 11 | 11 | import { appsForRepos } from "../../core/apps.ts"; |
| 12 | import { lookUrl, looksForApps } from "../../core/looks.ts"; | |
| 12 | 13 | import { latestRunsForIssues, type Run } from "../../core/runs.ts"; |
| 13 | 14 | import { listReposVisibleTo } from "../../core/repos.ts"; |
| 14 | 15 | import type { Ctx } from "../../http/context.ts"; |
| @@ -16,6 +17,7 @@ import { repoBase, signInFirst } from "../chrome.ts"; | ||
| 16 | 17 | import { html, type Html } from "../html.ts"; |
| 17 | 18 | import { page } from "../layout.ts"; |
| 18 | 19 | import { badge, empty, listCard, row, runStateBadge, time } from "../ui.ts"; |
| 20 | import { lookThumb, lookThumbPlain, thumbLook } from "../pages/looks.ts"; | |
| 19 | 21 | import { waitingOn } from "./asks.ts"; |
| 20 | 22 | import { diffStatFor, sizeLine } from "./review_evidence.ts"; |
| 21 | 23 | |
| @@ -48,8 +50,22 @@ export function reviewPage(ctx: Ctx): Response { | ||
| 48 | 50 | ["check"], |
| 49 | 51 | ); |
| 50 | 52 | const apps = appsForRepos(ctx.app.db, asks.map((a) => a.repo.id)); |
| 53 | const looks = looksForApps(ctx.app.db, [...apps.values()].map((r) => r.id)); | |
| 54 | /** Production beside the pull's preview, each only if this viewer may fetch it. */ | |
| 55 | const lookOf = (a: (typeof asks)[number], base: string): Html | undefined => { | |
| 56 | const record = a.pull === undefined ? undefined : apps.get(a.repo.id); | |
| 57 | if (a.pull === undefined || record === undefined) return undefined; | |
| 58 | const lookIn = (env: string) => thumbLook(ctx, a.repo, record, looks.filter((l) => l.appId === record.id && l.env === env)); | |
| 59 | const after = lookIn(previewEnv(a.pull.number)); | |
| 60 | if (after === undefined) return undefined; | |
| 61 | const before = lookIn("production"); | |
| 62 | // Only the after picture links (to the pull); the before one is a named picture, so | |
| 63 | // a keyboard user meets one stop, not two to the same place. | |
| 64 | return html`<li data-look><div class="looks">${before === undefined ? "" : lookThumbPlain(lookUrl(base, before), "Production look, before")}${lookThumb(`${base}/pulls/${a.pull.number}`, lookUrl(base, after), "Preview look, after: opens the pull")}</div></li>`; | |
| 65 | }; | |
| 51 | 66 | const rows = asks.map((a) => { |
| 52 | 67 | const base = repoBase(a.repo); |
| 68 | const look = lookOf(a, base); | |
| 53 | 69 | const href = |
| 54 | 70 | a.pull !== undefined |
| 55 | 71 | ? `${base}/pulls/${a.pull.number}` |
| @@ -82,7 +98,12 @@ export function reviewPage(ctx: Ctx): Response { | ||
| 82 | 98 | }), |
| 83 | 99 | // A sibling of the row's link, never inside it: a link in a link is not a link. |
| 84 | 100 | ...(a.pull !== undefined && apps.has(a.repo.id) |
| 85 | ? { children: [row({ href: `${base}/app/${previewEnv(a.pull.number)}/open`, title: "Open the preview", nav: true })] } | |
| 101 | ? { | |
| 102 | children: [ | |
| 103 | row({ href: `${base}/app/${previewEnv(a.pull.number)}/open`, title: "Open the preview", nav: true }), | |
| 104 | ...(look === undefined ? [] : [look]), | |
| 105 | ], | |
| 106 | } | |
| 86 | 107 | : {}), |
| 87 | 108 | }); |
| 88 | 109 | }); |
src/web/style/components.ts +6 −1
| @@ -375,7 +375,7 @@ a.sha:hover { color: var(--accent); } | ||
| 375 | 375 | .looks[data-compare] { gap: 20px; } |
| 376 | 376 | .looks[data-compare] > * { flex: 1 1 320px; min-width: 0; display: flex; flex-direction: column; gap: 6px; } |
| 377 | 377 | @media (max-width: 640px) { |
| 378 | .looks:not([data-compare]) { flex-direction: column; } | |
| 378 | .looks:not([data-compare]):not([data-look] .looks) { flex-direction: column; } | |
| 379 | 379 | .looks > [data-slot="phone"] { flex-basis: auto; width: 200px; } |
| 380 | 380 | } |
| 381 | 381 | |
| @@ -393,6 +393,11 @@ a.row:hover, .row[data-nav]:hover { background: var(--muted); color: inherit; } | ||
| 393 | 393 | .row-children { list-style: none; margin: 0 0 0 22px; padding: 0; border-left: 1px solid var(--border); } |
| 394 | 394 | .row-children > li { border-top: 1px solid var(--border); } |
| 395 | 395 | @media (max-width: 719px) { .row-children { margin-left: 12px; } } |
| 396 | /* A row's look thumbs (/review): inset like a row, side by side at any width; the link's hit area grows to 44px. */ | |
| 397 | .row-children > [data-look] { padding: 8px 14px; } | |
| 398 | [data-look] .looks { flex-wrap: nowrap; gap: 10px; align-items: center; } | |
| 399 | [data-look] a.look { position: relative; overflow: visible; } | |
| 400 | [data-look] a.look::after { content: ""; position: absolute; inset: -7px; } | |
| 396 | 401 | a.row:hover .row-title { color: var(--accent); } |
| 397 | 402 | .row-main { flex: 1; min-width: 0; display: flex; flex-direction: column; gap: 2px; } |
| 398 | 403 | .row-title { |
test/review-page.test.ts +50 −1
| @@ -7,6 +7,8 @@ import { createApp, type App } from "../src/app.ts"; | ||
| 7 | 7 | import { addComment, openIssue } from "../src/actions/issues.ts"; |
| 8 | 8 | import { addComment as addRawComment } from "../src/core/issues.ts"; |
| 9 | 9 | import { addCrew, createAgentRow, setCrewRole } from "../src/core/agents.ts"; |
| 10 | import { recordLook } from "../src/core/looks.ts"; | |
| 11 | import { previewEnv } from "../src/apps/data.ts"; | |
| 10 | 12 | import { createApp as createAppRow } from "../src/core/apps.ts"; |
| 11 | 13 | import { createRepo as createRepoRow } from "../src/core/repos.ts"; |
| 12 | 14 | import { submitReview } from "../src/core/reviews.ts"; |
| @@ -26,6 +28,7 @@ let root = ""; | ||
| 26 | 28 | let app: App; |
| 27 | 29 | let servers: Servers; |
| 28 | 30 | let cookie = ""; |
| 31 | let bobCookie = ""; | |
| 29 | 32 | |
| 30 | 33 | beforeAll(async () => { |
| 31 | 34 | root = mkdtempSync(join(tmpdir(), "forge-review-")); |
| @@ -90,7 +93,19 @@ beforeAll(async () => { | ||
| 90 | 93 | .query(`INSERT INTO run (id, repo_id, kind, issue_id, commit_oid, state) VALUES (?1, ?2, 'check', ?3, ?4, ?5)`) |
| 91 | 94 | .run(id(), repo.id, fresh, ZERO, state); |
| 92 | 95 | } |
| 93 | createAppRow(app.db, { repoId: repo.id, slug: "wait", access: "public" }); | |
| 96 | const made2 = createAppRow(app.db, { repoId: repo.id, slug: "wait", access: "public" }); | |
| 97 | if (!made2.ok) throw new Error(String(made2.error)); | |
| 98 | const record = made2.value; | |
| 99 | const numberOf = (issueId: string) => | |
| 100 | app.db.query<{ number: number }, [string]>("SELECT number FROM issue WHERE id = ?1").get(issueId)!.number; | |
| 101 | const shot = (issueId: string, commitOid: string, takenAs: "owner" | "nobody") => | |
| 102 | recordLook(app.db, { appId: record.id, env: previewEnv(numberOf(issueId)), commitOid, kind: "thumb", path: "looks/x.png", takenAs }); | |
| 103 | shot(stale, ZERO, "nobody"); | |
| 104 | recordLook(app.db, { appId: record.id, env: "production", commitOid: "1".repeat(40), kind: "thumb", path: "looks/m.png", takenAs: "nobody" }); | |
| 105 | const bob = await mk("bob", false); | |
| 106 | const mine = pull(builder.id, "Assigned to bob", 2); | |
| 107 | app.db.query("UPDATE issue SET assignee_id = ?2 WHERE id = ?1").run(mine, bob.id); | |
| 108 | shot(mine, ZERO, "owner"); | |
| 94 | 109 | pull(builder.id, "Young crew pull", 2); |
| 95 | 110 | const issue = openIssue(app, alice, repo, { title: "Crew asked about this" }); |
| 96 | 111 | if (!issue.ok) throw new Error(String(issue.error)); |
| @@ -102,6 +117,13 @@ beforeAll(async () => { | ||
| 102 | 117 | redirect: "manual", |
| 103 | 118 | }); |
| 104 | 119 | cookie = (signin.headers.get("set-cookie") ?? "").split(";")[0]!; |
| 120 | const bobIn = await fetch(`${servers.url}/signin`, { | |
| 121 | method: "POST", | |
| 122 | headers: { origin: servers.url }, | |
| 123 | body: new URLSearchParams({ username: "bob", password: "a-good-passphrase" }), | |
| 124 | redirect: "manual", | |
| 125 | }); | |
| 126 | bobCookie = (bobIn.headers.get("set-cookie") ?? "").split(";")[0]!; | |
| 105 | 127 | }, 30_000); |
| 106 | 128 | |
| 107 | 129 | afterAll(async () => { |
| @@ -174,6 +196,33 @@ test("a pull of an app links its preview beside the row, not inside it; an issue | ||
| 174 | 196 | expect(issue).not.toContain("No check"); |
| 175 | 197 | }); |
| 176 | 198 | |
| 199 | test("a pull with a look shows production beside its preview, outside the row's link; one without shows nothing", async () => { | |
| 200 | const body = await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text(); | |
| 201 | const block = (t: string) => { | |
| 202 | const at = body.indexOf(t); | |
| 203 | return body.slice(at, body.indexOf("</ul>", at)); | |
| 204 | }; | |
| 205 | const stale = block("Stale pull"); | |
| 206 | expect(stale.match(/data-slot="thumb"/g)?.length).toBe(2); | |
| 207 | expect(stale).toContain('alt="Production look, before"'); | |
| 208 | expect(stale).toContain('aria-label="Preview look, after: opens the pull"'); | |
| 209 | expect(stale.match(/<a /g)?.length).toBe(2); // the preview link and the after thumb; the before one is no link | |
| 210 | expect(stale).toContain("/alice/wait/looks/production/"); | |
| 211 | expect(stale).toContain('width="30" height="40"'); | |
| 212 | expect(stale).toContain('loading="lazy"'); | |
| 213 | expect(stale.indexOf('data-slot="thumb"')).toBeGreaterThan(stale.indexOf("</a>")); | |
| 214 | expect(block("Fresh ok pull")).not.toContain("data-slot"); | |
| 215 | }); | |
| 216 | ||
| 217 | test("a viewer who may not fetch the look sees no image and no link to it", async () => { | |
| 218 | const mineBody = await (await fetch(`${servers.url}/review`, { headers: { cookie: bobCookie } })).text(); | |
| 219 | expect(mineBody).toContain("Assigned to bob"); | |
| 220 | expect(mineBody).not.toContain("/looks/"); | |
| 221 | expect(mineBody).not.toContain("data-slot"); | |
| 222 | const owner = await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text(); | |
| 223 | expect(owner).not.toContain("Assigned to bob"); | |
| 224 | }); | |
| 225 | ||
| 177 | 226 | test("a pull's row says its diff is loading or unavailable, and unavailable once git failed; an issue row says neither", async () => { |
| 178 | 227 | const rowsOf = async () => |
| 179 | 228 | (await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text()) |
new above
offline
Both thumbs render as links with alt="" and no label: unnamed links for a screen reader, and before/after is indistinguishable. Pass a label (Production / Preview) through to the link.