Public: anyone can read this; the crew can change it.
A pull's /review row shows its check verdict and its preview link, read without git #4800
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/4789-a-pull-s-review-row-shows-its-check-verd → main 2 commits fork 1 thread · 1 unresolved
3 files
changed +65
−6
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. 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); both come from the database, never git. 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 |
src/web/studio/review_page.ts +24 −2
| @@ -6,15 +6,25 @@ | ||
| 6 | 6 | */ |
| 7 | 7 | |
| 8 | 8 | import { notePhase, takeSessionTime, timePhase } from "../../core/route_timing.ts"; |
| 9 | import { previewEnv } from "../../apps/data.ts"; | |
| 9 | 10 | import { recommendationsFor } from "../../core/pull_wait.ts"; |
| 11 | import { appsForRepos } from "../../core/apps.ts"; | |
| 12 | import { latestRunsForIssues, type Run } from "../../core/runs.ts"; | |
| 10 | 13 | import { listReposVisibleTo } from "../../core/repos.ts"; |
| 11 | 14 | import type { Ctx } from "../../http/context.ts"; |
| 12 | 15 | import { repoBase, signInFirst } from "../chrome.ts"; |
| 13 | import { html } from "../html.ts"; | |
| 16 | import { html, type Html } from "../html.ts"; | |
| 14 | 17 | import { page } from "../layout.ts"; |
| 15 | import { empty, listCard, row, time } from "../ui.ts"; | |
| 18 | import { badge, empty, listCard, row, runStateBadge, time } from "../ui.ts"; | |
| 16 | 19 | import { waitingOn } from "./asks.ts"; |
| 17 | 20 | |
| 21 | /** The check on a pull's head, as the pull page wears it; only a run of this very head counts. */ | |
| 22 | function verdict(run: Run | undefined, headOid: string): Html { | |
| 23 | return run === undefined || run.commitOid !== headOid | |
| 24 | ? badge("No check", { variant: "outline" }) | |
| 25 | : runStateBadge(run.state, "check"); | |
| 26 | } | |
| 27 | ||
| 18 | 28 | export function reviewPage(ctx: Ctx): Response { |
| 19 | 29 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| 20 | 30 | const user = ctx.auth.user; |
| @@ -31,6 +41,12 @@ export function reviewPage(ctx: Ctx): Response { | ||
| 31 | 41 | return id === undefined ? [] : [id]; |
| 32 | 42 | }), |
| 33 | 43 | ); |
| 44 | const checks = latestRunsForIssues( | |
| 45 | ctx.app.db, | |
| 46 | asks.flatMap((a) => (a.pull === undefined ? [] : [a.pull.id])), | |
| 47 | ["check"], | |
| 48 | ); | |
| 49 | const apps = appsForRepos(ctx.app.db, asks.map((a) => a.repo.id)); | |
| 34 | 50 | const rows = asks.map((a) => { |
| 35 | 51 | const base = repoBase(a.repo); |
| 36 | 52 | const href = |
| @@ -50,6 +66,12 @@ export function reviewPage(ctx: Ctx): Response { | ||
| 50 | 66 | title: a.pull?.title ?? a.issue?.title ?? a.proposal?.title ?? `${a.repo.name} is waiting on you`, |
| 51 | 67 | titleAttr: `${a.repo.ownerName}/${a.repo.name}: ${a.why ?? a.consequence.why}`, |
| 52 | 68 | meta: html`${a.repo.ownerName}/${a.repo.name} · ${time(a.since)} · ${a.why ?? a.consequence.why} · ${advised}`, |
| 69 | // The trail survives the meta line's ellipsis: the verdict is the fact this row gives. | |
| 70 | ...(a.pull === undefined ? {} : { trail: verdict(checks.get(a.pull.id), a.pull.headOid) }), | |
| 71 | // A sibling of the row's link, never inside it: a link in a link is not a link. | |
| 72 | ...(a.pull !== undefined && apps.has(a.repo.id) | |
| 73 | ? { children: [row({ href: `${base}/app/${previewEnv(a.pull.number)}/open`, title: "Open the preview", nav: true })] } | |
| 74 | : {}), | |
| 53 | 75 | }); |
| 54 | 76 | }); |
| 55 | 77 | const body = html`<h1>Review</h1> |
test/review-page.test.ts +40 −3
| @@ -7,6 +7,7 @@ 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 { createApp as createAppRow } from "../src/core/apps.ts"; | |
| 10 | 11 | import { createRepo as createRepoRow } from "../src/core/repos.ts"; |
| 11 | 12 | import { submitReview } from "../src/core/reviews.ts"; |
| 12 | 13 | import { createUser } from "../src/core/users.ts"; |
| @@ -79,6 +80,16 @@ beforeAll(async () => { | ||
| 79 | 80 | submitReview(app.db, { issueId: handed, authorId: reviewer.id, state: "changes_requested", body: `Round ${r}.`, commitOid: ZERO }); |
| 80 | 81 | const stale = pull(alice.id, "Stale pull", 30); |
| 81 | 82 | addRawComment(app.db, { issueId: stale, authorId: reviewer.id, body: "Recommendation: change — rename the <flag>." }); |
| 83 | app.db | |
| 84 | .query(`INSERT INTO run (id, repo_id, kind, issue_id, commit_oid, state) VALUES (?1, ?2, 'check', ?3, ?4, 'running')`) | |
| 85 | .run(id(), repo.id, handed, ZERO); | |
| 86 | for (const state of ["ok", "failed", "cancelled"]) { | |
| 87 | const fresh = pull(alice.id, `Fresh ${state} pull`, 30); | |
| 88 | app.db | |
| 89 | .query(`INSERT INTO run (id, repo_id, kind, issue_id, commit_oid, state) VALUES (?1, ?2, 'check', ?3, ?4, ?5)`) | |
| 90 | .run(id(), repo.id, fresh, ZERO, state); | |
| 91 | } | |
| 92 | createAppRow(app.db, { repoId: repo.id, slug: "wait", access: "public" }); | |
| 82 | 93 | pull(builder.id, "Young crew pull", 2); |
| 83 | 94 | const issue = openIssue(app, alice, repo, { title: "Crew asked about this" }); |
| 84 | 95 | if (!issue.ok) throw new Error(String(issue.error)); |
| @@ -98,12 +109,12 @@ afterAll(async () => { | ||
| 98 | 109 | rmSync(root, { recursive: true, force: true }); |
| 99 | 110 | }); |
| 100 | 111 | |
| 101 | test("lists exactly the three asks, each with title, repository and why", async () => { | |
| 112 | test("lists exactly the six asks, each with title, repository and why", async () => { | |
| 102 | 113 | const res = await fetch(`${servers.url}/review`, { headers: { cookie } }); |
| 103 | 114 | const body = await res.text(); |
| 104 | 115 | expect(res.status).toBe(200); |
| 105 | const rows = body.split('<li').filter((r) => r.includes('class="row"')); | |
| 106 | expect(rows.length).toBe(3); | |
| 116 | const rows = body.split('<li').filter((r) => r.includes('class="row"') && !r.includes("Open the preview")); | |
| 117 | expect(rows.length).toBe(6); | |
| 107 | 118 | const find = (t: string) => rows.find((r) => r.includes(t))!; |
| 108 | 119 | expect(find("Handed over pull")).toContain("three reviews asked for changes"); |
| 109 | 120 | expect(find("Stale pull")).toContain("waiting 30 hours"); |
| @@ -135,3 +146,29 @@ test("each row says what the crew advises and why, or that it has not advised ye | ||
| 135 | 146 | expect(find("Handed over pull")).toContain("The crew has not advised yet"); |
| 136 | 147 | expect(find("Crew asked about this")).toContain("The crew has not advised yet"); |
| 137 | 148 | }); |
| 149 | ||
| 150 | test("a pull's row shows its check verdict; one with no check says none", async () => { | |
| 151 | const body = await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text(); | |
| 152 | const rows = body.split("<li").filter((r) => r.includes('class="row"')); | |
| 153 | const find = (t: string) => rows.find((r) => r.includes(t))!; | |
| 154 | expect(find("Handed over pull")).toContain("Running"); | |
| 155 | expect(find("Stale pull")).toContain("No check"); | |
| 156 | expect(find("Fresh ok pull")).toContain("Passed"); | |
| 157 | expect(find("Fresh failed pull")).toContain("Failed"); | |
| 158 | expect(find("Fresh cancelled pull")).toContain("Cancelled"); | |
| 159 | expect(find("Fresh cancelled pull")).not.toContain("No check"); | |
| 160 | }); | |
| 161 | ||
| 162 | test("a pull of an app links its preview beside the row, not inside it; an issue row has neither", async () => { | |
| 163 | const body = await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text(); | |
| 164 | const rows = body.split("<li").filter((r) => r.includes('class="row"')); | |
| 165 | const find = (t: string) => rows.find((r) => r.includes(t))!; | |
| 166 | const at = body.indexOf("Stale pull"); | |
| 167 | const link = body.indexOf("/alice/wait/app/pull-", at); | |
| 168 | expect(link).toBeGreaterThan(at); | |
| 169 | expect(body.slice(at, link)).toContain("</a>"); | |
| 170 | const issue = find("Crew asked about this"); | |
| 171 | expect(issue).not.toContain("/app/pull-"); | |
| 172 | expect(issue).not.toContain("badge"); | |
| 173 | expect(issue).not.toContain("No check"); | |
| 174 | }); |
new above
offline
Replace with runStateBadge(run) when run.commitOid === headOid; render cancelled as its own badge, not No check. Place it where .row-meta ellipsis cannot hide it.