Public: anyone can read this; the crew can change it.
Act from /review: Merge, Close, Request changes, Reply, by button or key, then the next item #5182
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/3258-act-from-review-merge-close-request-chan → main 2 commits fork 3 threads · 3 unresolved
8 files
changed +406
−20
README.md +2 −2
| @@ -449,8 +449,8 @@ 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); 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 | to it, and signed out it sends you to sign-in. The terminal has no page of its | |
| 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. Each row also carries its actions under it, as plain form POSTs to `/review/act` (same-origin checked, `rw` scope): on a pull **Merge** (through the same landing gate as anywhere, so a red or unreviewed head is refused with the gate's sentence), **Close**, **Request changes** (with a text box) and **Reply** (with a text box); on an issue **Close** and **Reply**. After an action the page answers 303 to `/review#<the next ask>` (the first that followed it and still waits, else the first left, else `/review`), and the acted ask is gone; a refusal comes back to the same row with its reason. Keys: `j`/`k` move between rows, and on the focused row `m` merges, `c` closes and `r` jumps to the Request-changes box; keys are ignored while a text box has focus, and `?` lists them. `/asks` links to it, and signed out it sends you to sign-in. The terminal has no page of its | |
| 453 | own for it: the same asks are the dashboard's On you card (acting from the terminal is tracked in #5189). The terminal has no page of its | |
| 454 | 454 | own for it: the same asks are the dashboard's On you card. |
| 455 | 455 | |
| 456 | 456 | A client's forge: `Ctrl+K`, then **New forge** (any signed-in person's, as the |
docs/design/areas/review.md +1
| @@ -1,3 +1,4 @@ | ||
| 1 | 1 | # Review |
| 2 | 2 | |
| 3 | 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). |
| 4 | - A row on `/review` carries its actions as `<li data-act-forms>` in the row's children: a `.form-actions` line with the Merge and Close buttons (each its own form) and one or two `.input` textareas (Request changes, Reply) with their own buttons. No new class. `m`, `c` and `r` act on the row the cursor is on (`j`/`k`); the key line under the list carries them as `data-shortcut` so the `?` help lists them. |
src/http/server.ts +2
| @@ -257,6 +257,7 @@ import { forgeLinkSubmit, forgesPage } from "../web/pages/forges.ts"; | ||
| 257 | 257 | import { newForgePage, newForgeSubmit } from "../web/pages/new-forge.ts"; |
| 258 | 258 | import { askedPage } from "../web/studio/asked_page.ts"; |
| 259 | 259 | import { reviewPage } from "../web/studio/review_page.ts"; |
| 260 | import { reviewAct } from "../web/studio/review_act.ts"; | |
| 260 | 261 | import { |
| 261 | 262 | asksBulkSubmit, |
| 262 | 263 | asksBulkUndo, |
| @@ -742,6 +743,7 @@ export function buildRouter(): Router<Handler> { | ||
| 742 | 743 | // What people asked of the crew, and what became of it (core/asked.ts). |
| 743 | 744 | router.add("GET", "/asked", askedPage); |
| 744 | 745 | router.add("GET", "/review", reviewPage); |
| 746 | router.add("POST", "/review/act", reviewAct); | |
| 745 | 747 | // The big view, folded into the board: every old address lands on its wall mode. |
| 746 | 748 | router.add("GET", "/big", bigRedirect); |
| 747 | 749 | router.add("GET", "/search", searchPage); |
src/web/pages/pull.ts +1 −1
| @@ -1070,7 +1070,7 @@ export async function pullReviewSubmit(ctx: Ctx): Promise<Response> { | ||
| 1070 | 1070 | * or lifecycle code, or git's own structured `MergeError` — both are answers |
| 1071 | 1071 | * the person pressing the button deserves to see. |
| 1072 | 1072 | */ |
| 1073 | function mergeFailure(error: AccessError | MergePullError): string { | |
| 1073 | export function mergeFailure(error: AccessError | MergePullError): string { | |
| 1074 | 1074 | if (typeof error !== "string") { |
| 1075 | 1075 | if (error.kind === "conflict") |
| 1076 | 1076 | return `The branches conflict in ${error.files.length} file${error.files.length === 1 ? "" : "s"} — merge the base branch in and push again.`; |
src/web/studio/review_act.ts added +146
| @@ -0,0 +1,146 @@ | ||
| 1 | /** | |
| 2 | * Acting from `/review`: Merge, Close, Request changes and Reply on the ask | |
| 3 | * a row shows, each a plain form POST to `/review/act`. Nothing here is a | |
| 4 | * second road: a merge is `mergePull` (the landing gate), a close the same | |
| 5 | * `setIssueStateThrough` the pull page uses, a verdict `submitReview`, a | |
| 6 | * reply `addCommentThrough` — with the same access checks, behind `readForm` | |
| 7 | * (same-origin, `rw` scope). A person who acted lands on the next ask. | |
| 8 | */ | |
| 9 | ||
| 10 | import type { Database } from "bun:sqlite"; | |
| 11 | import { mergePull, submitReview } from "../../actions/pulls.ts"; | |
| 12 | import { addCommentThrough, setIssueStateThrough } from "../../actions/issues.ts"; | |
| 13 | import { getIssue } from "../../core/issues.ts"; | |
| 14 | import { getPull } from "../../core/pulls.ts"; | |
| 15 | import { canRead, getRepo, listReposVisibleTo } from "../../core/repos.ts"; | |
| 16 | import type { User } from "../../core/users.ts"; | |
| 17 | import type { Ctx } from "../../http/context.ts"; | |
| 18 | import { notFoundPage, signInFirst } from "../chrome.ts"; | |
| 19 | import { field, readForm } from "../form.ts"; | |
| 20 | import { html, type Html } from "../html.ts"; | |
| 21 | import { flashTo, redirect } from "../layout.ts"; | |
| 22 | import { btn } from "../ui.ts"; | |
| 23 | import { originError } from "../pages/thread.ts"; | |
| 24 | import { mergeFailure } from "../pages/pull.ts"; | |
| 25 | import { askAnchor, waitingOn } from "./asks.ts"; | |
| 26 | ||
| 27 | type Ask = ReturnType<typeof waitingOn>[number]; | |
| 28 | ||
| 29 | const VERBS = ["merge", "close", "changes", "reply"] as const; | |
| 30 | type Verb = (typeof VERBS)[number]; | |
| 31 | ||
| 32 | /** Every ask waiting on `user`, newest first: the order `/review` draws and acts in. */ | |
| 33 | export function orderedAsks(db: Database, user: User): Ask[] { | |
| 34 | const repos = listReposVisibleTo(db, user, { limit: 200, withoutArchived: true }); | |
| 35 | return waitingOn(db, user, repos).sort((a, b) => Date.parse(b.since) - Date.parse(a.since)); | |
| 36 | } | |
| 37 | ||
| 38 | /** The row's anchor, so a redirect can land on the ask after the one acted on. */ | |
| 39 | export const rowId = (a: Ask): string => `ask-${askAnchor(a.id)}`; | |
| 40 | ||
| 41 | /** Which of the four a row offers: a pull all of them, an issue close and reply, anything else none. */ | |
| 42 | export function verbsFor(a: Ask): readonly Verb[] { | |
| 43 | return a.pull !== undefined ? VERBS : a.issue !== undefined ? ["close", "reply"] : []; | |
| 44 | } | |
| 45 | ||
| 46 | /** The buttons and boxes under a row, a sibling of its link; `undefined` when it offers none. */ | |
| 47 | export function actRow(a: Ask): Html | undefined { | |
| 48 | const verbs = verbsFor(a); | |
| 49 | const number = a.pull?.number ?? a.issue?.number; | |
| 50 | if (verbs.length === 0 || number === undefined) return undefined; | |
| 51 | const of = `${a.repo.ownerName}/${a.repo.name}`; | |
| 52 | const form = (verb: Verb, inner: Html) => | |
| 53 | html`<form method="post" action="/review/act" data-verb="${verb}"> | |
| 54 | <input type="hidden" name="repo" value="${of}" /> | |
| 55 | <input type="hidden" name="number" value="${String(number)}" /> | |
| 56 | <input type="hidden" name="verb" value="${verb}" />${inner} | |
| 57 | </form>`; | |
| 58 | const box = (verb: Verb, label: string, send: string) => | |
| 59 | form( | |
| 60 | verb, | |
| 61 | html`<label class="sr-only" for="${rowId(a)}-${verb}">${label}</label> | |
| 62 | <textarea class="input" id="${rowId(a)}-${verb}" name="body" rows="2" maxlength="65536" required placeholder="${label}"></textarea> | |
| 63 | <div class="form-actions"><span class="spacer"></span>${btn(send)}</div>`, | |
| 64 | ); | |
| 65 | const quick = verbs.filter((v) => v === "merge" || v === "close"); | |
| 66 | return html`<li data-act-forms> | |
| 67 | <div class="form-actions">${quick.map((v) => | |
| 68 | form(v, btn(v === "merge" ? "Merge" : "Close")), | |
outdated
Each pull row gets |
||
| 69 | )}</div> | |
| 70 | ${verbs.includes("changes") ? box("changes", "Request changes: what must change", "Request changes") : null} | |
| 71 | ${box("reply", "Reply", "Reply")} | |
| 72 | </li>`; | |
| 73 | } | |
| 74 | ||
| 75 | const back = (a: Ask | undefined): string => (a === undefined ? "/review" : `/review#${rowId(a)}`); | |
| 76 | ||
| 77 | /** POST /review/act { repo, number, verb, body? } — one action, then the next ask. */ | |
| 78 | export async function reviewAct(ctx: Ctx): Promise<Response> { | |
| 79 | const user = ctx.auth.user; | |
| 80 | if (!user) return signInFirst(ctx); | |
| 81 | const form = await readForm(ctx, "rw"); | |
| 82 | if (form instanceof Response) return form; | |
| 83 | ||
| 84 | const [owner = "", name = ""] = field(form, "repo").split("/"); | |
| 85 | const verb = field(form, "verb") as Verb; | |
| 86 | const numberText = field(form, "number"); | |
| 87 | if (!VERBS.includes(verb) || !/^\d{1,9}$/.test(numberText) || owner === "" || name === "" || name.includes("/")) | |
| 88 | return flashTo(ctx, "/review", { tone: "danger", message: "That action was not understood." }); | |
| 89 | const repo = getRepo(ctx.app.db, owner, name); | |
| 90 | if (repo === undefined || !canRead(ctx.app.db, repo, user)) return notFoundPage(ctx); | |
| 91 | const number = Number(numberText); | |
| 92 | const pull = getPull(ctx.app.db, repo.id, number); | |
| 93 | const issue = pull ?? getIssue(ctx.app.db, repo.id, number); | |
| 94 | if (issue === undefined || ((verb === "merge" || verb === "changes") && pull === undefined)) return notFoundPage(ctx); | |
| 95 | ||
| 96 | const before = orderedAsks(ctx.app.db, user); | |
| 97 | const acted = (a: Ask) => a.repo.id === repo.id && (a.pull?.number ?? a.issue?.number) === number; | |
| 98 | const at = before.findIndex(acted); | |
| 99 | const here = at < 0 ? undefined : before[at]; | |
| 100 | const refuse = (message: string) => flashTo(ctx, back(here), { tone: "danger", message }); | |
| 101 | ||
| 102 | const text = field(form, "body").trim(); | |
| 103 | if ((verb === "changes" || verb === "reply") && text === "") | |
| 104 | return refuse(verb === "reply" ? "A reply needs something in it." : "Say what must change."); | |
| 105 | ||
| 106 | let done: string; | |
| 107 | if (verb === "merge") { | |
| 108 | const merged = await mergePull(ctx.app, user, repo, pull!, "merge"); | |
| 109 | if (!merged.ok) return refuse(mergeFailure(merged.error)); | |
| 110 | done = "Merged."; | |
| 111 | } else if (verb === "close") { | |
| 112 | const closed = await setIssueStateThrough(ctx.app, user, repo, issue, "closed"); | |
| 113 | if (!closed.ok) | |
| 114 | return refuse( | |
| 115 | originError(closed.error) ?? | |
| 116 | (closed.error === "forbidden" | |
| 117 | ? "Only the author or someone with write access can do that." | |
| 118 | : "That could not be closed."), | |
| 119 | ); | |
| 120 | done = "Closed."; | |
| 121 | } else if (verb === "changes") { | |
| 122 | const reviewed = await submitReview(ctx.app, user, repo, pull!, { state: "changes_requested", body: text }); | |
| 123 | if (!reviewed.ok) | |
| 124 | return refuse( | |
| 125 | originError(reviewed.error) ?? | |
| 126 | (reviewed.error === "forbidden" | |
| 127 | ? "A verdict that gates a merge needs write access." | |
| 128 | : reviewed.error === "body_too_long" | |
| 129 | ? "That review is too long (64 KB at most)." | |
| 130 | : "That review could not be recorded."), | |
| 131 | ); | |
| 132 | done = "Changes requested."; | |
| 133 | } else { | |
| 134 | const posted = await addCommentThrough(ctx.app, user, repo, issue, text); | |
| 135 | if (!posted.ok) | |
| 136 | return refuse(originError(posted.error) ?? (posted.error === "body_too_long" ? "That reply is too long." : "That reply could not be posted.")); | |
| 137 | done = "Reply posted."; | |
| 138 | } | |
| 139 | ||
| 140 | // The next ask: the first one that followed this row and still waits; if | |
| 141 | // this was the last, the first that remains. | |
| 142 | const after = orderedAsks(ctx.app.db, user).filter((a) => !acted(a)); | |
| 143 | const later = new Set(before.slice(at + 1).map((a) => a.id)); | |
| 144 | const next = at < 0 ? after[0] : (after.find((a) => later.has(a.id)) ?? after[0]); | |
| 145 | return redirect(back(next), { flash: { message: done }, url: ctx.url }); | |
| 146 | } |
src/web/studio/review_page.ts +41 −16
| @@ -11,14 +11,13 @@ import { recommendationsFor } from "../../core/pull_wait.ts"; | ||
| 11 | 11 | import { appsForRepos } from "../../core/apps.ts"; |
| 12 | 12 | import { lookUrl, looksForApps } from "../../core/looks.ts"; |
| 13 | 13 | import { latestRunsForIssues, type Run } from "../../core/runs.ts"; |
| 14 | import { listReposVisibleTo } from "../../core/repos.ts"; | |
| 15 | 14 | import type { Ctx } from "../../http/context.ts"; |
| 16 | 15 | import { repoBase, signInFirst } from "../chrome.ts"; |
| 17 | 16 | import { html, type Html } from "../html.ts"; |
| 18 | 17 | import { page } from "../layout.ts"; |
| 19 | 18 | import { badge, empty, listCard, row, runStateBadge, time } from "../ui.ts"; |
| 20 | 19 | import { lookThumb, lookThumbPlain, thumbLook } from "../pages/looks.ts"; |
| 21 | import { waitingOn } from "./asks.ts"; | |
| 20 | import { actRow, orderedAsks, rowId } from "./review_act.ts"; | |
| 22 | 21 | import { diffStatFor, sizeLine } from "./review_evidence.ts"; |
| 23 | 22 | |
| 24 | 23 | /** The check on a pull's head, as the pull page wears it; only a run of this very head counts. */ |
| @@ -28,14 +27,39 @@ function verdict(run: Run | undefined, headOid: string): Html { | ||
| 28 | 27 | : runStateBadge(run.state, "check"); |
| 29 | 28 | } |
| 30 | 29 | |
| 30 | /** | |
| 31 | * m / c / r act on the row the cursor is on (j / k, the shell's own); the | |
| 32 | * buttons are the forms' own, so a key is a click, never a second road. The | |
| 33 | * shell ignores keys typed in a box and so does this. Each key is also a | |
| 34 | * \`data-shortcut\` on the row's control, which is what the help lists. | |
| 35 | */ | |
| 36 | const REVIEW_KEYS_JS = ` | |
| 37 | var rowOf = function () { var a = document.activeElement; return a && a.closest ? a.closest('li[data-ask]') : null; }; | |
| 38 | document.addEventListener('keydown', function (e) { | |
| 39 | var t = e.target; | |
| 40 | if (e.metaKey || e.ctrlKey || e.altKey || (t && t.matches && t.matches('input, textarea, select, [contenteditable]'))) return; | |
| 41 | if (t && t.closest && t.closest('dialog[open]')) return; | |
| 42 | var verb = { m: 'merge', c: 'close', r: 'changes' }[e.key]; | |
| 43 | var li = verb && rowOf(); | |
| 44 | var f = li && li.querySelector('form[data-verb="' + verb + '"]'); | |
| 45 | if (!f) return; | |
| 46 | e.preventDefault(); e.stopPropagation(); | |
| 47 | if (verb === 'changes') f.querySelector('textarea').focus(); | |
| 48 | else f.querySelector('button').click(); | |
| 49 | }, true); | |
| 50 | var hit = location.hash && document.querySelector('li[data-ask]' + location.hash.replace(/[^#a-z0-9-]/g, '')); | |
| 51 | if (hit) { var link = hit.querySelector('[data-nav]'); if (link) link.focus(); } | |
| 52 | `; | |
| 53 | ||
| 54 | /** A key the page answers to; the shell's help lists it from the attribute. */ | |
| 55 | const key = (k: string, label: string, word: string): Html => | |
| 56 | html`<span data-shortcut="${k}" aria-label="${label}"><kbd>${k}</kbd> ${word}</span>`; | |
| 57 | ||
| 31 | 58 | export function reviewPage(ctx: Ctx): Response { |
| 32 | 59 | notePhase(ctx.request, "session", takeSessionTime(ctx.request)); |
| 33 | 60 | const user = ctx.auth.user; |
| 34 | 61 | if (!user) return signInFirst(ctx); |
| 35 | const asks = timePhase(ctx.request, "asks", () => { | |
| 36 | const repos = listReposVisibleTo(ctx.app.db, user, { limit: 200, withoutArchived: true }); | |
| 37 | return waitingOn(ctx.app.db, user, repos).sort((a, b) => Date.parse(b.since) - Date.parse(a.since)); | |
| 38 | }); | |
| 62 | const asks = timePhase(ctx.request, "asks", () => orderedAsks(ctx.app.db, user)); | |
| 39 | 63 | const rendering = performance.now(); |
| 40 | 64 | const advice = recommendationsFor( |
| 41 | 65 | ctx.app.db, |
| @@ -84,9 +108,12 @@ export function reviewPage(ctx: Ctx): Response { | ||
| 84 | 108 | : stat === "failed" |
| 85 | 109 | ? " · Diff size unavailable — open the pull to see it" |
| 86 | 110 | : ""; |
| 111 | const act = actRow(a); | |
| 87 | 112 | return row({ |
| 88 | 113 | href, |
| 89 | 114 | nav: true, |
| 115 | id: rowId(a), | |
| 116 | data: { ask: "" }, | |
| 90 | 117 | title: a.pull?.title ?? a.issue?.title ?? a.proposal?.title ?? `${a.repo.name} is waiting on you`, |
| 91 | 118 | titleAttr: `${a.repo.ownerName}/${a.repo.name}: ${a.why ?? a.consequence.why}`, |
| 92 | 119 | meta: html`${a.repo.ownerName}/${a.repo.name} · ${time(a.since)} · ${a.why ?? a.consequence.why} · ${advised}${pending}`, |
| @@ -97,14 +124,12 @@ export function reviewPage(ctx: Ctx): Response { | ||
| 97 | 124 | trail: html`${typeof stat === "object" ? html`<span class="small muted">${sizeLine(stat)}</span> ` : ""}${verdict(checks.get(a.pull.id), a.pull.headOid)}`, |
| 98 | 125 | }), |
| 99 | 126 | // A sibling of the row's link, never inside it: a link in a link is not a link. |
| 100 | ...(a.pull !== undefined && apps.has(a.repo.id) | |
| 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 | } | |
| 107 | : {}), | |
| 127 | children: [ | |
| 128 | ...(a.pull !== undefined && apps.has(a.repo.id) | |
| 129 | ? [row({ href: `${base}/app/${previewEnv(a.pull.number)}/open`, title: "Open the preview", nav: true }), ...(look === undefined ? [] : [look])] | |
| 130 | : []), | |
| 131 | ...(act === undefined ? [] : [act]), | |
| 132 | ], | |
| 108 | 133 | }); |
| 109 | 134 | }); |
| 110 | 135 | const body = html`<h1>Review</h1> |
| @@ -116,8 +141,8 @@ export function reviewPage(ctx: Ctx): Response { | ||
| 116 | 141 | note: "When the crew needs a person, it shows up here.", |
| 117 | 142 | action: html`<a class="btn" href="/asks">All asks</a>`, |
| 118 | 143 | }) |
The keys are listed here, but the README still says |
||
| 119 | : html`${listCard(rows)}<p class="small muted"><a href="/asks">All asks, with their buttons</a></p>`}`; | |
| 120 | const response = page({ ctx, title: "Review", body }); | |
| 144 | : html`${listCard(rows)}<p class="small muted">Keys: j / k move, ${key("m", "Merge the focused row", "merge")}, ${key("c", "Close the focused row", "close")}, ${key("r", "Request changes on the focused row", "request changes")} · <a href="/asks">All asks, with their buttons</a></p>`}`; | |
| 145 | const response = page({ ctx, title: "Review", body, scriptJs: asks.length === 0 ? "" : REVIEW_KEYS_JS }); | |
| 121 | 146 | notePhase(ctx.request, "render", performance.now() - rendering); |
| 122 | 147 | return response; |
| 123 | 148 | } |
src/web/ui.ts +3 −1
| @@ -925,6 +925,8 @@ export interface RowOptions { | ||
| 925 | 925 | state?: "open" | "closed" | "merged" | "draft"; |
| 926 | 926 | /** The row joins `j`/`k` keyboard traversal. */ |
| 927 | 927 | nav?: boolean; |
| 928 | /** An anchor for the row's `<li>`, so a redirect can land on it. */ | |
| 929 | id?: string; | |
| 928 | 930 | titleAttr?: string; |
| 929 | 931 | /** `data-*` hooks on the row's `<li>` (a script's filter, a test's |
| 930 | 932 | * handle). Keys are lower-case words and dashes; others are dropped. */ |
| @@ -963,7 +965,7 @@ export function row(o: RowOptions): Html { | ||
| 963 | 965 | const data = Object.entries(o.data ?? {}) |
| 964 | 966 | .filter(([key]) => /^[a-z][a-z-]*$/.test(key)) |
| 965 | 967 | .map(([key, value]) => html` data-${raw(key)}="${value}"`); |
| 966 | return html`<li${data}${o.current ? raw(' aria-current="true"') : null}> | |
| 968 | return html`<li${o.id === undefined ? null : html` id="${o.id}"`}${data}${o.current ? raw(' aria-current="true"') : null}> | |
| 967 | 969 | ${o.fold !== undefined |
| 968 | 970 | ? html`<details id="${o.fold.id}" ${o.fold.open ? raw("open") : null}> |
| 969 | 971 | <summary ${attrs}>${inner}</summary> |
test/review-act.test.ts added +210
| @@ -0,0 +1,210 @@ | ||
| 1 | /** `/review` acts: Merge, Close, Request changes and Reply, each by form POST, then on to the next ask. */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtempSync, rmSync } from "node:fs"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { createApp, type App } from "../src/app.ts"; | |
| 7 | import { openPull } from "../src/actions/pulls.ts"; | |
| 8 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; | |
| 9 | import { listComments } from "../src/core/issues.ts"; | |
| 10 | import { getPull } from "../src/core/pulls.ts"; | |
| 11 | import { listReviews } from "../src/core/reviews.ts"; | |
| 12 | import { claimRun, createRun, finishRun } from "../src/core/runs.ts"; | |
| 13 | import { createUser, type User } from "../src/core/users.ts"; | |
| 14 | import type { Repo } from "../src/core/repos.ts"; | |
| 15 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 16 | import { id } from "../src/kernel/id.ts"; | |
| 17 | import { orderedAsks, rowId } from "../src/web/studio/review_act.ts"; | |
| 18 | import { quietLogs } from "./helpers/globals.ts"; | |
| 19 | ||
| 20 | quietLogs(); | |
| 21 | ||
| 22 | const ZERO = "0".repeat(40); | |
| 23 | const hoursAgo = (h: number) => new Date(Date.now() - h * 3_600_000).toISOString(); | |
| 24 | ||
| 25 | let root = ""; | |
| 26 | let app: App; | |
| 27 | let servers: Servers; | |
| 28 | let cookie = ""; | |
| 29 | let alice: User; | |
| 30 | let repo: Repo; | |
| 31 | let age = 100; | |
| 32 | ||
| 33 | /** A pull alice authored with a green check on its head: it asks her, `age` hours old (older asks come last). */ | |
| 34 | function seeded(title: string): number { | |
| 35 | const issueId = id(); | |
| 36 | const number = app.db | |
| 37 | .query<{ issue_seq: number }, [string]>("UPDATE repo SET issue_seq = issue_seq + 1 WHERE id = ?1 RETURNING issue_seq") | |
| 38 | .get(repo.id)!.issue_seq; | |
| 39 | app.db | |
| 40 | .query(`INSERT INTO issue (id, repo_id, number, kind, title, body, author_id, state, created_at) VALUES (?1, ?2, ?3, 'pull', ?4, '', ?5, 'open', ?6)`) | |
| 41 | .run(issueId, repo.id, number, title, alice.id, hoursAgo(age--)); | |
| 42 | app.db | |
| 43 | .query(`INSERT INTO pull (issue_id, base_branch, head_branch, base_oid, head_oid, head_repo_id) VALUES (?1, 'main', ?2, ?3, ?3, ?4)`) | |
| 44 | .run(issueId, `b/${number}`, ZERO, repo.id); | |
| 45 | app.db | |
| 46 | .query(`INSERT INTO run (id, repo_id, kind, issue_id, commit_oid, state) VALUES (?1, ?2, 'check', ?3, ?4, 'ok')`) | |
| 47 | .run(id(), repo.id, issueId, ZERO); | |
| 48 | return number; | |
| 49 | } | |
| 50 | ||
| 51 | /** A real branch with one commit, opened as a pull, its head checked green or red. */ | |
| 52 | async function real(name: string, state: "ok" | "failed"): Promise<number> { | |
| 53 | const made = await commitToBranch(app, alice, repo, { | |
| 54 | branch: `feature/${name}`, | |
| 55 | base: "main", | |
| 56 | message: name, | |
| 57 | files: [{ path: `${name}.ts`, content: `export const n = "${name}";\n` }], | |
| 58 | }); | |
| 59 | if (!made.ok) throw new Error(JSON.stringify(made.error)); | |
| 60 | const opened = await openPull(app, alice, repo, { title: name, baseBranch: "main", headBranch: `feature/${name}` }); | |
| 61 | if (!opened.ok) throw new Error(String(opened.error)); | |
| 62 | const pull = getPull(app.db, repo.id, opened.value.number)!; | |
| 63 | const run = createRun(app.db, { repoId: repo.id, kind: "check", issueId: pull.id, env: `pull-${pull.number}`, commitOid: pull.headOid, reason: "pull" }); | |
| 64 | claimRun(app.db, run.id); | |
| 65 | finishRun(app.db, run.id, { state, summary: state }); | |
| 66 | return pull.number; | |
| 67 | } | |
| 68 | ||
| 69 | const act = (fields: Record<string, string>, headers: Record<string, string> = {}) => | |
| 70 | fetch(`${servers.url}/review/act`, { | |
| 71 | method: "POST", | |
| 72 | headers: { cookie, origin: servers.url, ...headers }, | |
| 73 | body: new URLSearchParams({ repo: "alice/work", ...fields }), | |
| 74 | redirect: "manual", | |
| 75 | }); | |
| 76 | ||
| 77 | const numberOf = (a: ReturnType<typeof orderedAsks>[number]) => a.pull?.number ?? a.issue?.number; | |
| 78 | const anchorOf = (n: number) => rowId(orderedAsks(app.db, alice).find((a) => numberOf(a) === n)!); | |
| 79 | ||
| 80 | beforeAll(async () => { | |
| 81 | root = mkdtempSync(join(tmpdir(), "forge-review-act-")); | |
| 82 | app = createApp({ dataDir: root, port: 0 }); | |
| 83 | servers = await serve(app); | |
| 84 | const made = await createUser(app.db, { | |
| 85 | username: "alice", | |
| 86 | email: "[email protected]", | |
| 87 | password: "a-good-passphrase", | |
| 88 | origin: "seed", | |
| 89 | trusted: true, | |
| 90 | adminIfFirst: true, | |
| 91 | }); | |
| 92 | if (!made.ok) throw new Error(made.error); | |
| 93 | alice = made.value; | |
| 94 | const r = await createRepo(app, alice, { name: "work", visibility: "private" }); | |
| 95 | if (!r.ok) throw new Error(String(r.error)); | |
| 96 | repo = r.value; | |
| 97 | const first = await commitToBranch(app, alice, repo, { message: "Start", files: [{ path: "README.md", content: "# work\n" }] }); | |
| 98 | if (!first.ok) throw new Error(JSON.stringify(first.error)); | |
| 99 | const signin = await fetch(`${servers.url}/signin`, { | |
| 100 | method: "POST", | |
| 101 | headers: { origin: servers.url }, | |
| 102 | body: new URLSearchParams({ username: "alice", password: "a-good-passphrase" }), | |
| 103 | redirect: "manual", | |
| 104 | }); | |
| 105 | cookie = (signin.headers.get("set-cookie") ?? "").split(";")[0]!; | |
| 106 | }, 30_000); | |
| 107 | ||
| 108 | afterAll(async () => { | |
| 109 | await servers.stop(); | |
| 110 | app.close(); | |
| 111 | rmSync(root, { recursive: true, force: true }); | |
| 112 | }); | |
| 113 | ||
| 114 | test("each row offers the four actions as plain forms, and the keys are listed", async () => { | |
| 115 | const n = seeded("Offered"); | |
| 116 | const page = await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text(); | |
| 117 | for (const verb of ["merge", "close", "changes", "reply"]) expect(page).toContain(`data-verb="${verb}"`); | |
| 118 | expect(page).toContain('action="/review/act"'); | |
| 119 | expect(page).toContain(`id="${anchorOf(n)}"`); | |
| 120 | for (const k of ["m", "c", "r"]) expect(page).toContain(`data-shortcut="${k}"`); | |
| 121 | await act({ number: String(n), verb: "close" }); | |
| 122 | }); | |
| 123 | ||
| 124 | test("close closes the pull and points at the next ask, which is gone from the page for the acted one", async () => { | |
| 125 | const a = seeded("First to go"); | |
| 126 | const b = seeded("Second in line"); | |
| 127 | const res = await act({ number: String(a), verb: "close" }); | |
| 128 | expect(res.status).toBe(303); | |
| 129 | expect(getPull(app.db, repo.id, a)!.state).toBe("closed"); | |
| 130 | expect(res.headers.get("location")).toBe(`/review#${anchorOf(b)}`); | |
| 131 | const page = await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text(); | |
| 132 | expect(page).not.toContain("First to go"); | |
| 133 | expect(page).toContain("Second in line"); | |
| 134 | await act({ number: String(b), verb: "close" }); | |
| 135 | }); | |
| 136 | ||
| 137 | test("the last ask closed sends the person back to /review", async () => { | |
| 138 | const only = seeded("Alone"); | |
| 139 | const res = await act({ number: String(only), verb: "close" }); | |
| 140 | expect(res.headers.get("location")).toBe("/review"); | |
| 141 | }); | |
| 142 | ||
| 143 | test("request changes records the review and moves on", async () => { | |
| 144 | const a = seeded("Needs work"); | |
| 145 | const b = seeded("After it"); | |
| 146 | const res = await act({ number: String(a), verb: "changes", body: "Rename the flag." }); | |
| 147 | expect(res.status).toBe(303); | |
| 148 | const pull = getPull(app.db, repo.id, a)!; | |
| 149 | const reviews = listReviews(app.db, pull.id); | |
| 150 | expect(reviews.some((r) => r.state === "changes_requested" && r.body === "Rename the flag.")).toBe(true); | |
| 151 | expect(res.headers.get("location")).toBe(`/review#${anchorOf(b)}`); | |
| 152 | await act({ number: String(b), verb: "close" }); | |
| 153 | }); | |
| 154 | ||
| 155 | test("reply posts a comment, and an empty one is refused where it was", async () => { | |
| 156 | const a = seeded("Talk to me"); | |
| 157 | const empty = await act({ number: String(a), verb: "reply", body: " " }); | |
| 158 | expect(empty.status).toBe(303); | |
| 159 | expect(empty.headers.get("location")).toBe(`/review#${anchorOf(a)}`); | |
| 160 | expect(decodeURIComponent(empty.headers.get("set-cookie") ?? "")).toContain("needs something"); | |
| 161 | const res = await act({ number: String(a), verb: "reply", body: "Looks fine to me." }); | |
| 162 | expect(res.status).toBe(303); | |
| 163 | const pull = getPull(app.db, repo.id, a)!; | |
| 164 | expect(listComments(app.db, pull.id).some((c) => c.body === "Looks fine to me.")).toBe(true); | |
| 165 | await act({ number: String(a), verb: "close" }); | |
| 166 | }); | |
| 167 | ||
| 168 | test("merge lands a green pull through the gate", async () => { | |
| 169 | const n = await real("lands", "ok"); | |
| 170 | const res = await act({ number: String(n), verb: "merge" }); | |
| 171 | expect(res.status).toBe(303); | |
| 172 | expect(getPull(app.db, repo.id, n)!.mergedAt).not.toBeNull(); | |
| 173 | expect(res.headers.get("location")).toMatch(/^\/review(#.*)?$/); | |
| 174 | }); | |
| 175 | ||
| 176 | test("merge on a red check is refused with the gate's sentence, and nothing lands", async () => { | |
| 177 | app.db.query("UPDATE repo SET require_checks = 1 WHERE id = ?1").run(repo.id); | |
| 178 | const n = await real("red", "failed"); | |
| 179 | const res = await act({ number: String(n), verb: "merge" }); | |
| 180 | expect(res.status).toBe(303); | |
| 181 | expect(getPull(app.db, repo.id, n)!.mergedAt).toBeNull(); | |
| 182 | expect(decodeURIComponent(res.headers.get("set-cookie") ?? "").replace(/\+/g, " ")).toContain("needs a passing deploy of its current head"); | |
| 183 | app.db.query("UPDATE repo SET require_checks = 0 WHERE id = ?1").run(repo.id); | |
| 184 | }); | |
| 185 | ||
| 186 | test("a cross-origin post, a stranger, and a bad verb change nothing", async () => { | |
| 187 | const n = seeded("Untouchable"); | |
| 188 | expect((await act({ number: String(n), verb: "close" }, { origin: "https://evil.example" })).status).toBe(403); | |
| 189 | const anon = await fetch(`${servers.url}/review/act`, { | |
| 190 | method: "POST", | |
| 191 | headers: { origin: servers.url }, | |
| 192 | body: new URLSearchParams({ repo: "alice/work", number: String(n), verb: "close" }), | |
| 193 | redirect: "manual", | |
| 194 | }); | |
| 195 | expect(anon.headers.get("location") ?? "").toContain("/signin"); | |
| 196 | const bad = await act({ number: String(n), verb: "delete" }); | |
| 197 | expect(bad.status).toBe(303); | |
| 198 | expect(getPull(app.db, repo.id, n)!.state).toBe("open"); | |
| 199 | }); | |
| 200 | ||
| 201 | test("the key script ships only when there are asks, and sanitises the hash it focuses", async () => { | |
| 202 | const n = seeded("Keyed"); | |
| 203 | const withAsks = await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text(); | |
| 204 | expect(withAsks).toContain("m: 'merge'"); | |
| 205 | expect(withAsks).toContain("location.hash.replace(/[^#a-z0-9-]/g, '')"); | |
| 206 | for (const a of orderedAsks(app.db, alice)) await act({ number: String(numberOf(a)), verb: "close" }); | |
| 207 | expect(n).toBeGreaterThan(0); | |
| 208 | const empty = await (await fetch(`${servers.url}/review`, { headers: { cookie } })).text(); | |
| 209 | expect(empty).not.toContain("m: 'merge'"); | |
| 210 | }); |
new above
offline
This sentence is duplicated. The new text ends with "(acting from the terminal is tracked in #5189). The terminal has no page of its", and the old line "own for it: the same asks are the dashboard's On you card." follows it. Drop the trailing "The terminal has no page of its" and the old next line, so the sentence appears once.