Public: anyone can read this; the crew can change it.
The sender reads the crew's replies to their note inside the forge button #10804
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/10562-the-sender-reads-the-crew-s-replies-to-t → main 3 commits fork 4 threads · 4 unresolved
7 files
changed +132
−13
docs/features/notes.md +2
| @@ -28,3 +28,5 @@ The terminal has the same door: the palette's "Notes" (`src/tui/screens/thoughtn | ||
| 28 | 28 | ## Notes from an app's forge button |
| 29 | 29 | |
| 30 | 30 | A note sent from an app's forge button (or the terminal's "Tell the builders") files a from-app issue, and the sender is told its number at once: "Filed as #N". Anyone the repository would show the issue to (`canRead`, so a signed-in reader of a private repository too, not only a public app on a public repository) also gets the issue's link and the repository's; everyone else gets the number and no link. The list shows `#N` beside each note. The browser button links only to the forge's own origin. In the terminal (`src/tui/screens/notes.ts`) the sender is the SSH user: the confirmation reads "Sent as #N, owner/repo/issues/N" and the list and moderation screens carry `#N`; the terminal has no clickable link. |
| 31 | ||
| 32 | The crew's answers come back to the sender: under each note the button's list (and the terminal's "Notes from the app", where picking a note shows them) carries the comments on its issue by the repository's crew and writers, with author and time, oldest first. Anyone else's comments stay on the forge. The button shows the latest five and says how many are earlier. |
src/actions/app_notes.ts +36 −9
| @@ -9,15 +9,15 @@ import { dropHiddenPictures } from "../core/note_pictures.ts"; | ||
| 9 | 9 | import type { App } from "../app.ts"; |
| 10 | 10 | import { banVisitor, bannedNotes, unbanVisitor, markSender, getNote, hideNote, insertNote, isBanned, notesOf, notesOfApp, type AppNote } from "../core/app_notes.ts"; |
| 11 | 11 | import type { AppRecord } from "../core/apps.ts"; |
| 12 | import { createLabel, getIssue, getLabel, type Issue } from "../core/issues.ts"; | |
| 12 | import { createLabel, getIssue, getLabel, listComments, type Issue } from "../core/issues.ts"; | |
| 13 | 13 | import { mergedPullClosing } from "../core/pulls.ts"; |
| 14 | 14 | import { releaseName, releaseOfPull } from "../release/record.ts"; |
| 15 | 15 | import { RateLimiter } from "../core/ratelimit.ts"; |
| 16 | import { canAdmin, canRead, type Repo } from "../core/repos.ts"; | |
| 16 | import { canAdmin, canRead, canWrite, type Repo } from "../core/repos.ts"; | |
| 17 | 17 | import { plainText } from "../core/reports.ts"; |
| 18 | 18 | import { listActiveRuns, workingAgentRuns } from "../core/runs.ts"; |
| 19 | 19 | import { getUserById, isActive, type User } from "../core/users.ts"; |
| 20 | import { crewWithRole } from "../core/agents.ts"; | |
| 20 | import { crewWithRole, isCrew } from "../core/agents.ts"; | |
| 21 | 21 | import { fence } from "../crew/fence.ts"; |
| 22 | 22 | import { unadoptedAmong } from "../crew/direct.ts"; |
| 23 | 23 | import { NOTE_EMPTY } from "../apps/note_words.ts"; |
| @@ -92,6 +92,13 @@ export interface NoteStatus { | ||
| 92 | 92 | readonly label: string; |
| 93 | 93 | } |
| 94 | 94 | |
| 95 | /** One answer to a note: a comment on its issue by the crew or a writer of the repository. */ | |
| 96 | export interface NoteReply { | |
| 97 | readonly by: string; | |
| 98 | readonly at: string; | |
| 99 | readonly text: string; | |
| 100 | } | |
| 101 | ||
| 95 | 102 | export interface NoteView { |
| 96 | 103 | readonly id: string; |
| 97 | 104 | readonly text: string; |
| @@ -100,12 +107,14 @@ export interface NoteView { | ||
| 100 | 107 | readonly status: NoteStatus; |
| 101 | 108 | /** The issue's number: shown to everyone, linked only to a reader of the repository. */ |
| 102 | 109 | readonly number: number; |
| 110 | /** The crew's and writers' comments on the issue, newest last; an outsider's are never here. */ | |
| 111 | readonly replies: readonly NoteReply[]; | |
| 103 | 112 | readonly url?: string; |
| 104 | 113 | readonly repoUrl?: string; |
| 105 | 114 | } |
| 106 | 115 | |
| 107 | 116 | /** Where a note stands, read from the issue as it moves. */ |
| 108 | export function statusOf(app: App, repo: Repo, number: number): { status: NoteStatus; title: string } | undefined { | |
| 117 | export function statusOf(app: App, repo: Repo, number: number): { status: NoteStatus; title: string; issueId: string } | undefined { | |
| 109 | 118 | const issue = getIssue(app.db, repo.id, number); |
| 110 | 119 | if (issue === undefined) return undefined; |
| 111 | 120 | if (issue.state === "closed") { |
| @@ -116,20 +125,37 @@ export function statusOf(app: App, repo: Repo, number: number): { status: NoteSt | ||
| 116 | 125 | const release = pull === undefined ? undefined : releaseOfPull(app.db, repo.id, pull.number); |
| 117 | 126 | const label = |
| 118 | 127 | release !== undefined ? `Shipped in ${releaseName(release)}` : `Shipped on ${(pull?.mergedAt ?? issue.closedAt ?? issue.updatedAt).slice(0, 10)}`; |
| 119 | return { title: issue.title, status: { state: "shipped", label } }; | |
| 128 | return { issueId: issue.id, title: issue.title, status: { state: "shipped", label } }; | |
| 120 | 129 | } |
| 121 | 130 | const label = issue.closeReason === "parked" ? "Set aside for now" : issue.closeReason === "split" ? "Split into smaller pieces" : "Closed without a change"; |
| 122 | return { title: issue.title, status: { state: "closed", label: `${label} (${when})` } }; | |
| 131 | return { issueId: issue.id, title: issue.title, status: { state: "closed", label: `${label} (${when})` } }; | |
| 123 | 132 | } |
| 124 | 133 | // A stranger's words are a suggestion until a writer adopts them. |
| 125 | 134 | if (unadoptedAmong(app.db, repo, [issue]).has(issue.id)) |
| 126 | return { title: issue.title, status: { state: "waiting", label: "Waiting for the builders to look" } }; | |
| 135 | return { issueId: issue.id, title: issue.title, status: { state: "waiting", label: "Waiting for the builders to look" } }; | |
| 127 | 136 | const run = workingAgentRuns(app.db, repo.id, issue.id)[0]; |
| 128 | 137 | if (run !== undefined) { |
| 129 | 138 | const actor = run.actorId === null ? undefined : getUserById(app.db, run.actorId); |
| 130 | return { title: issue.title, status: { state: "building", label: `Being built by ${actor?.username ?? "the crew"}` } }; | |
| 139 | return { issueId: issue.id, title: issue.title, status: { state: "building", label: `Being built by ${actor?.username ?? "the crew"}` } }; | |
| 131 | 140 | } |
| 132 | return { title: issue.title, status: { state: "queued", label: "In the queue for a builder" } }; | |
| 141 | return { issueId: issue.id, title: issue.title, status: { state: "queued", label: "In the queue for a builder" } }; | |
| 142 | } | |
| 143 | ||
| 144 | /** What the crew and the repository's writers said on a note's issue; anyone else's comment stays on the forge. */ | |
| 145 | function repliesTo(app: App, repo: Repo, issueId: string): NoteReply[] { | |
| 146 | const standing = new Map<string, boolean>(); | |
| 147 | const speaks = (id: string): boolean => { | |
| 148 | let yes = standing.get(id); | |
| 149 | if (yes === undefined) { | |
| 150 | const user = getUserById(app.db, id); | |
| 151 | yes = user !== undefined && isActive(user) && (isCrew(app.db, repo.id, id) || canWrite(app.db, repo, user)); | |
| 152 | standing.set(id, yes); | |
| 153 | } | |
| 154 | return yes; | |
| 155 | }; | |
| 156 | return listComments(app.db, issueId, { kind: "comment" }) | |
| 157 | .filter((c) => speaks(c.authorId)) | |
| 158 | .map((c) => ({ by: c.authorName, at: c.createdAt, text: c.body })); | |
| 133 | 159 | } |
| 134 | 160 | |
| 135 | 161 | function view(app: App, repo: Repo, forgeUrl: string | undefined, viewer: User | undefined, note: AppNote): NoteView | undefined { |
| @@ -144,6 +170,7 @@ function view(app: App, repo: Repo, forgeUrl: string | undefined, viewer: User | | ||
| 144 | 170 | at: note.createdAt, |
| 145 | 171 | status: found.status, |
| 146 | 172 | number: note.issueNumber, |
| 173 | replies: repliesTo(app, repo, found.issueId), | |
| 147 | 174 | ...(open |
| 148 | 175 | ? { |
| 149 | 176 | url: new URL(`/${repo.ownerName}/${repo.name}/issues/${note.issueNumber}`, forgeUrl).href, |
src/apps/toolbar.ts +15
| @@ -188,6 +188,21 @@ export const toolbarJs = `(function () { | ||
| 188 | 188 | st.style.cssText = "opacity:.85"; |
| 189 | 189 | row.appendChild(t); |
| 190 | 190 | row.appendChild(st); |
| 191 | var all = Array.isArray(n.replies) ? n.replies : []; | |
| 192 | // Only the latest few, so a long thread cannot push the panel off a phone; the rest is on the issue. | |
| 193 | var shown = all.slice(-5); | |
| 194 | if (all.length > shown.length) { | |
| 195 | var more = document.createElement("div"); | |
| 196 | more.textContent = (all.length - shown.length) + " earlier on the forge"; | |
| 197 | more.style.cssText = "opacity:.85;margin-top:2px"; | |
| 198 | row.appendChild(more); | |
| 199 | } | |
| 200 | shown.forEach(function (r) { | |
| 201 | var rr = document.createElement("div"); | |
| 202 | rr.textContent = String(r.by) + " (" + String(r.at).slice(0, 16).replace("T", " ") + "): " + String(r.text); | |
| 203 | rr.style.cssText = "margin-top:2px;padding-left:6px;border-left:2px solid #444c56;white-space:pre-wrap;max-height:8em;overflow:auto"; | |
| 204 | row.appendChild(rr); | |
| 205 | }); | |
| 191 | 206 | var al = n.url && link(n.url, "See it"); |
| 192 | 207 | if (al) row.appendChild(al); |
| 193 | 208 | mine.appendChild(row); |
src/tui/screens/notes.ts +8 −2
| @@ -15,6 +15,9 @@ import type { Command } from "../app/palette.ts"; | ||
| 15 | 15 | import type { Session } from "../session.ts"; |
| 16 | 16 | import { confirmScreen, pickerScreen, promptScreen } from "./common.ts"; |
| 17 | 17 | |
| 18 | /** "2 replies", "1 reply": what a note's list line says of the answers under it. */ | |
| 19 | export const repliesWord = (n: number): string => `${n} repl${n === 1 ? "y" : "ies"}`; | |
| 20 | ||
| 18 | 21 | export function noteCommands(f: Session, ctx: Ctx, parent: () => Screen, repo: Repo | undefined): Command[] { |
| 19 | 22 | const user = f.user; |
| 20 | 23 | if (repo === undefined || user === undefined || !canRead(f.app.db, repo, user)) return []; |
| @@ -88,15 +91,18 @@ export function noteCommands(f: Session, ctx: Ctx, parent: () => Screen, repo: R | ||
| 88 | 91 | ctx.push( |
| 89 | 92 | pickerScreen(ctx, parent(), { |
| 90 | 93 | title: "Notes from the app", |
| 91 | items: notes.map((n) => ({ id: n.id, label: `#${n.number} ${n.text}`, hint: `${n.status.label} — ${n.by}` })), | |
| 94 | items: notes.map((n) => ({ id: n.id, label: `#${n.number} ${n.text}${n.replies.length > 0 ? ` (${repliesWord(n.replies.length)})` : ""}`, hint: `${n.status.label} — ${n.by}` })), | |
| 92 | 95 | onPick: ([id]) => { |
outdated
The reply dialog for the non-admin path and the admin body have no test. Also the one-line item hint is getting long; extract a small helper for the reply count wording. |
||
| 93 | 96 | const picked = notes.find((n) => n.id === id); |
| 94 | 97 | if (picked === undefined) return; |
| 98 | const said = picked.replies.map((r) => `${r.by} ${r.at.slice(0, 16).replace("T", " ")}: ${r.text}`); | |
| 99 | if (!admin && said.length > 0) | |
| 100 | return ctx.push(confirmScreen(ctx, parent(), { title: `#${picked.number}: ${picked.status.label}`, body: said, actions: [{ label: "Close", variant: "ghost" }], onChoose: () => {} })); | |
| 95 | 101 | if (!admin) return ctx.toast(`#${picked.number}: ${picked.status.label}.`); |
| 96 | 102 | ctx.push( |
| 97 | 103 | confirmScreen(ctx, parent(), { |
| 98 | 104 | title: "Moderate this note", |
| 99 | body: [`#${picked.number} ${picked.by}: ${picked.text}`, picked.status.label], | |
| 105 | body: [`#${picked.number} ${picked.by}: ${picked.text}`, picked.status.label, ...said], | |
| 100 | 106 | actions: [{ label: "Keep", variant: "ghost" }, { label: "Hide" }, { label: "Ban sender", variant: "danger" }], |
| 101 | 107 | onChoose: (i) => { |
| 102 | 108 | if (i < 1) return; |
test/app_notes.test.ts +46 −1
| @@ -10,7 +10,9 @@ import { newApp } from "../src/actions/apps.ts"; | ||
| 10 | 10 | import { notesFor, sendNote, type Sender } from "../src/actions/app_notes.ts"; |
| 11 | 11 | import { createApp, type App } from "../src/app.ts"; |
| 12 | 12 | import { getAppBySlug } from "../src/core/apps.ts"; |
| 13 | import { getRepo } from "../src/core/repos.ts"; | |
| 13 | import { addComment, getIssue } from "../src/core/issues.ts"; | |
| 14 | import { addCrew, createAgentRow } from "../src/core/agents.ts"; | |
| 15 | import { canWrite, getRepo } from "../src/core/repos.ts"; | |
| 14 | 16 | import { createUser, type User } from "../src/core/users.ts"; |
| 15 | 17 | import { quietLogs } from "./helpers/globals.ts"; |
| 16 | 18 | |
| @@ -76,4 +78,47 @@ describe("the link a note's sender gets", () => { | ||
| 76 | 78 | expect(listed[0]?.number).toBe(sent.value.number); |
| 77 | 79 | expect(listed[0]?.url).toBeUndefined(); |
| 78 | 80 | }); |
| 81 | ||
outdated
Only the writer branch is covered. Add a crew member (not a writer) whose comment must appear; otherwise isCrew can be removed with no failing test. Also assert deactivated authors are skipped if that is intended. |
||
| 82 | test("a writer's comment on the issue is a reply; an outsider's is not, and the order is oldest first", async () => { | |
| 83 | const record = getAppBySlug(app.db, "sandbox-game")!; | |
| 84 | const repo = getRepo(app.db, "alice", "sandbox-game")!; | |
| 85 | const made = await createUser(app.db, { origin: "seed", trusted: true, username: "mallory", email: "[email protected]" }); | |
| 86 | if (!made.ok) throw new Error(String(made.error)); | |
| 87 | const who = visitor(); | |
| 88 | const sent = await sendNote(app, record, repo, "production", FORGE, who, input, "10.1.0.3"); | |
| 89 | if (!sent.ok) throw new Error(sent.error.message); | |
| 90 | const issue = getIssue(app.db, repo.id, sent.value.number)!; | |
| 91 | expect(notesFor(app, record, repo, FORGE, who)[0]?.replies).toEqual([]); | |
| 92 | addComment(app.db, { issueId: issue.id, authorId: alice.id, body: "first <b>answer</b>" }); | |
| 93 | addComment(app.db, { issueId: issue.id, authorId: made.value.id, body: "buy my stuff" }); | |
| 94 | addComment(app.db, { issueId: issue.id, authorId: alice.id, body: "second answer" }); | |
| 95 | const replies = notesFor(app, record, repo, FORGE, who)[0]!.replies; | |
| 96 | expect(replies.map((r) => r.text)).toEqual(["first <b>answer</b>", "second answer"]); | |
| 97 | expect(replies[0]?.by).toBe("alice"); | |
| 98 | expect(replies[0]?.at).toBeTruthy(); | |
| 99 | }); | |
| 100 | ||
| 101 | test("a crew agent who is not a writer is heard; a suspended one is not", async () => { | |
| 102 | const record = getAppBySlug(app.db, "sandbox-game")!; | |
| 103 | const repo = getRepo(app.db, "alice", "sandbox-game")!; | |
| 104 | const mk = async (username: string) => { | |
| 105 | const r = await createUser(app.db, { origin: "seed", username, email: `${username}@a.invalid`, agent: true }); | |
| 106 | if (!r.ok) throw new Error("agent"); | |
| 107 | createAgentRow(app.db, { userId: r.value.id, brief: "build", createdBy: alice.id }); | |
| 108 | addCrew(app.db, repo.id, r.value.id, alice.id); | |
| 109 | return r.value; | |
| 110 | }; | |
| 111 | const builder = await mk("builder-bot"); | |
| 112 | const gone = await mk("gone-bot"); | |
| 113 | expect(canWrite(app.db, repo, builder)).toBe(false); | |
| 114 | const who = visitor(); | |
| 115 | const sent = await sendNote(app, record, repo, "production", FORGE, who, input, "10.1.0.4"); | |
| 116 | if (!sent.ok) throw new Error(sent.error.message); | |
| 117 | const issue = getIssue(app.db, repo.id, sent.value.number)!; | |
| 118 | addComment(app.db, { issueId: issue.id, authorId: builder.id, body: "on it" }); | |
| 119 | addComment(app.db, { issueId: issue.id, authorId: gone.id, body: "silenced" }); | |
| 120 | app.db.query("UPDATE user SET suspended_at = ?2 WHERE id = ?1").run(gone.id, new Date().toISOString()); | |
| 121 | const replies = notesFor(app, record, repo, FORGE, who)[0]!.replies; | |
| 122 | expect(replies.map((r) => `${r.by}: ${r.text}`)).toEqual(["builder-bot: on it"]); | |
| 123 | }); | |
| 79 | 124 | }); |
test/apps-toolbar.test.ts +11
| @@ -723,3 +723,14 @@ describe("pill link tap targets (#8462)", () => { | ||
| 723 | 723 | expect(js).toContain("if (hasHome) panel.appendChild(link)"); |
| 724 | 724 | }); |
| 725 | 725 | }); |
| 726 | ||
| 727 | describe("replies under a note in the button's list", () => { | |
| 728 | test("each is set as text, the latest five are shown and the rest counted", async () => { | |
| 729 | const { toolbarJs } = await import("../src/apps/toolbar.ts"); | |
| 730 | expect(toolbarJs).toContain("n.replies"); | |
| 731 | expect(toolbarJs).toContain("all.slice(-5)"); | |
| 732 | expect(toolbarJs).toContain("earlier on the forge"); | |
| 733 | expect(toolbarJs).toContain('rr.textContent = String(r.by) + " ("'); | |
| 734 | expect(toolbarJs).not.toMatch(/rr\.innerHTML/); | |
| 735 | }); | |
| 736 | }); |
test/tui-notes.test.ts +14 −1
| @@ -14,7 +14,7 @@ import { createApp, type App } from "../src/app.ts"; | ||
| 14 | 14 | import { sendNote, type Sender } from "../src/actions/app_notes.ts"; |
| 15 | 15 | import { isBanned } from "../src/core/app_notes.ts"; |
| 16 | 16 | import { getAppBySlug } from "../src/core/apps.ts"; |
| 17 | import { getIssue, labelsFor } from "../src/core/issues.ts"; | |
| 17 | import { addComment, getIssue, labelsFor } from "../src/core/issues.ts"; | |
| 18 | 18 | import { getRepo, type Repo } from "../src/core/repos.ts"; |
| 19 | 19 | import { createUser, type User } from "../src/core/users.ts"; |
| 20 | 20 | import { createForgeApp } from "../src/tui/forge.ts"; |
| @@ -126,4 +126,17 @@ describe("notes from the app, in the terminal", () => { | ||
| 126 | 126 | expect(again.ok).toBe(true); |
| 127 | 127 | expect(await palette(alice, notes, [":", "Banned", "\r"])).not.toContain("Unban this sender"); |
| 128 | 128 | }, 30_000); |
| 129 | ||
| 130 | test("a writer's reply shows to the sender on picking the note, and in the admin's moderation body", async () => { | |
| 131 | for (const row of app.db.query<{ issue_number: number }, []>("SELECT issue_number FROM app_note").all()) | |
| 132 | addComment(app.db, { issueId: getIssue(app.db, notes.id, row.issue_number)!.id, authorId: alice.id, body: "we are on it" }); | |
| 133 | const list = await palette(bob, notes, [":", "Notes from", "\r"]); | |
| 134 | expect(list).toMatch(/#\d+ back again \(\d+ replies?\)/); | |
| 135 | const asBob = await palette(bob, notes, [":", "Notes from", "\r", "\r"]); | |
| 136 | expect(asBob).toContain("alice"); | |
| 137 | expect(asBob).toContain("we are on it"); | |
| 138 | const asAlice = await palette(alice, notes, [":", "Notes from", "\r", "\r"]); | |
| 139 | expect(asAlice).toContain("Moderate this note"); | |
| 140 | expect(asAlice).toContain("we are on it"); | |
| 141 | }, 30_000); | |
| 129 | 142 | }); |
Unbounded replies inside a small panel will push the panel off a 360px screen. Show the latest few and link to the issue for the rest. No test covers this markup; add one.