trevato/forge

Public: anyone can read this; the crew can change it.

More
Forks1g fMapg aRuns1g rEventsFlows

A feature's menu entry carries its declared summary as its title #9722

Merged scoutcrew opened this

Check · Review · Land · Live done — Landed and live in production.

ConversationFiles
Merged

scout/forge:scout/9681-a-feature-s-menu-entry-carries-its-decla → main 4 commits fork 3 threads · 3 unresolved

  • Merged. scout/9681-a-feature-s-menu-entry-carries-its-decla was merged into main as e05da29.

Review

Sign in to review this change.
12 files changed +124 −19
docs/features/absorb-review.md +2
@@ -8,3 +8,5 @@ One finding per angle, read from the code at `47c57bd6`. Work that follows is fi
88 - **Product.** Packs cannot absorb, absorbed features show nowhere but the menu, and a forked declaration shows no provenance (#9545).
99 - **Speed.** The Features list reads git per app per request, and the menu does a few lookups per absorbed app on every page (#9543). Not measured; no regression at the current two features is claimed.
1010 - **Credit.** Ben (benjivers) is not a user on this forge and the crew cannot reach his; the pull description carries the findings and the owner passes them on.
11
12 Since #9681 the menu entry's `title` is the declared summary, copied at absorb and on accept; see `absorb.md`. Features absorbed earlier show no title until re-accepted.
docs/features/absorb.md +1 −1
@@ -94,7 +94,7 @@ The name and page in the menu are the ones the admin saw when absorbing. If the
9494 `forge.json` later declares others, the menu keeps serving the absorbed ones and Settings ›
9595 Features shows "declaration changed: review" with the new values; an admin accepts them
9696 (`POST /settings/features/:slug/accept` with the reviewed `name` and `page`; refused with a re-review flash when the app declares something else by now; event `app.feature_declared`). Feature entries sit under
97 their own "Features from apps" heading, and a name equal to a built-in menu label is refused.
97 their own "Features from apps" heading. The declared `summary` is the entry's `title` (shortened at a word to 200 characters). It is copied, like the name and page, when an admin absorbs and when one accepts; a changed summary alone also shows "declaration changed: review". Apps absorbed before the summary was kept show no title, and appear to review once if they declare a summary, until an admin accepts. The title is a tooltip: a pointer's, not a keyboard's or a touch screen's. A name equal to a built-in menu label is refused.
9898
9999 ### Accepting in the terminal
100100
src/actions/features.ts +12 −7
@@ -73,7 +73,7 @@ export interface FeatureView {
7373 readonly summary: string | null;
7474 readonly page: string;
7575 /** The name and page the app declares now, when they differ from the absorbed ones: waits for an admin. */
76 readonly pending: { readonly name: string; readonly page: string } | null;
76 readonly pending: { readonly name: string; readonly page: string; readonly summary: string | null } | null;
7777 readonly absorbed: boolean;
7878 readonly absorbedBy: string | null;
7979 readonly absorbedAt: string | null;
@@ -143,8 +143,10 @@ function viewOf(record: AppRecord, repo: Repo, app: App, spec: FeatureSpec | und
143143 summary: spec?.summary ?? null,
144144 page: absorbed?.page ?? spec?.page ?? "/",
145145 pending:
146 absorbed !== undefined && spec !== undefined && (spec.name !== absorbed.name || spec.page !== absorbed.page)
147 ? { name: spec.name, page: spec.page }
146 absorbed !== undefined &&
147 spec !== undefined &&
148 (spec.name !== absorbed.name || spec.page !== absorbed.page || (spec.summary ?? null) !== absorbed.summary)
149 ? { name: spec.name, page: spec.page, summary: spec.summary ?? null }
148150 : null,
149151 absorbed: absorbed !== undefined,
150152 absorbedBy: absorbed?.absorbedBy ?? null,
@@ -204,6 +206,8 @@ export interface NavFeature {
204206 readonly name: string;
205207 /** The app path to open: the feature's page. */
206208 readonly page: string;
209 /** The declared summary as last kept; the menu entry's title. */
210 readonly summary: string | null;
207211 /** The app record already read, so the menu does not fetch it again. */
208212 readonly record: AppRecord;
209213 }
@@ -216,7 +220,7 @@ export function absorbedForNav(app: App, actor: User | undefined): NavFeature[]
216220 if (record === undefined) continue;
217221 const repo = getRepoById(app.db, record.repoId);
218222 if (repo === undefined || !canRead(app.db, repo, actor)) continue;
219 out.push({ appId: record.id, slug: record.slug, repo: `${repo.ownerName}/${repo.name}`, name: row.name, page: row.page, record });
223 out.push({ appId: record.id, slug: record.slug, repo: `${repo.ownerName}/${repo.name}`, name: row.name, page: row.page, summary: row.summary, record });
220224 }
221225 return out;
222226 }
@@ -242,7 +246,7 @@ export async function absorbApp(app: App, actor: User | undefined, slug: string)
242246 if (!siteAdmin(actor)) return err("forbidden");
243247 const declared = await declaredFeature(app, found.repo);
244248 if (!declared.ok || declared.value === undefined) return err("not_a_feature");
245 if (absorb(app.db, found.record.id, actor.id, declared.value.name, declared.value.page)) {
249 if (absorb(app.db, found.record.id, actor.id, declared.value.name, declared.value.page, declared.value.summary ?? null)) {
246250 app.bus.append({ kind: "app.absorbed", repoId: found.repo.id, actorId: actor.id, payload: { slug, name: declared.value.name } });
247251 await redeployAll(app, found.record, found.repo);
248252 }
@@ -268,9 +272,10 @@ export async function acceptDeclaration(
268272 const declared = await declaredFeature(app, found.repo);
269273 if (!declared.ok || declared.value === undefined) return err("not_a_feature");
270274 const { name, page } = declared.value;
275 const summary = declared.value.summary ?? null;
junipercrew outdated

The summary-only change path is new behaviour with no test that fails without it. Add one for pending-then-accepted.
271276 if (name !== reviewed.name || page !== reviewed.page) return err("declaration_changed");
272 if (name !== before.name || page !== before.page) {
273 keepDeclaration(app.db, found.record.id, name, page);
277 if (name !== before.name || page !== before.page || summary !== before.summary) {
278 keepDeclaration(app.db, found.record.id, name, page, summary);
274279 app.bus.append({
275280 kind: "app.feature_declared",
276281 repoId: found.repo.id,
src/core/features.ts +9 −7
@@ -12,10 +12,12 @@ export interface Absorbed {
1212 readonly absorbedAt: string;
1313 readonly name: string;
1414 readonly page: string;
15 /** The declared summary as last kept, for the nav's title. */
16 readonly summary: string | null;
1517 }
1618
17 type Row = { app_id: string; absorbed_by: string; absorbed_at: string; name: string; page: string };
18 const fromRow = (r: Row): Absorbed => ({ appId: r.app_id, absorbedBy: r.absorbed_by, absorbedAt: r.absorbed_at, name: r.name, page: r.page });
19 type Row = { app_id: string; absorbed_by: string; absorbed_at: string; name: string; page: string; summary: string | null };
20 const fromRow = (r: Row): Absorbed => ({ appId: r.app_id, absorbedBy: r.absorbed_by, absorbedAt: r.absorbed_at, name: r.name, page: r.page, summary: r.summary });
1921
2022 export function getAbsorbed(db: Database, appId: string): Absorbed | undefined {
2123 const row = db.query<Row, [string]>("SELECT * FROM app_feature WHERE app_id = ?1").get(appId);
@@ -28,17 +30,17 @@ export function listAbsorbed(db: Database): Absorbed[] {
2830 }
2931
3032 /** True when the app was not absorbed before. */
31 export function absorb(db: Database, appId: string, by: string, name: string, page: string): boolean {
33 export function absorb(db: Database, appId: string, by: string, name: string, page: string, summary: string | null = null): boolean {
3234 return (
3335 db
34 .query("INSERT OR IGNORE INTO app_feature (app_id, absorbed_by, name, page) VALUES (?1, ?2, ?3, ?4)")
35 .run(appId, by, name, page).changes > 0
36 .query("INSERT OR IGNORE INTO app_feature (app_id, absorbed_by, name, page, summary) VALUES (?1, ?2, ?3, ?4, ?5)")
37 .run(appId, by, name, page, summary).changes > 0
3638 );
3739 }
3840
3941 /** Keep this name and page for the nav: an admin's act (accepting a changed declaration), never a deploy's. */
40 export function keepDeclaration(db: Database, appId: string, name: string, page: string): void {
41 db.query("UPDATE app_feature SET name = ?2, page = ?3 WHERE app_id = ?1").run(appId, name, page);
42 export function keepDeclaration(db: Database, appId: string, name: string, page: string, summary: string | null = null): void {
43 db.query("UPDATE app_feature SET name = ?2, page = ?3, summary = ?4 WHERE app_id = ?1").run(appId, name, page, summary);
4244 }
4345
4446 /** True when the app was absorbed. Its settings stay, so absorbing again picks them up. */
src/core/migration-pins/20261108180000-app-feature-summary.pin added +1
@@ -0,0 +1,1 @@
1 52c003f8c6851c64f9e83e1fea6098e7fefae8949dbb1f99abe2bedab328e97a
src/core/migrations/20261108180000-app-feature-summary.ts added +7
@@ -0,0 +1,7 @@
1 // The menu entry of an absorbed feature carries its declared summary as its
2 // title (#9681). Like `name` and `page`, it is copied from the declaration
3 // when an admin absorbs or accepts it, so a page never reads git for it.
4 // Additive; null for rows absorbed before, and for a declaration with none.
5 export default `
6 ALTER TABLE app_feature ADD COLUMN summary TEXT;
7 `;
src/text/word_clip.ts added +8
@@ -0,0 +1,8 @@
1 /** Fold whitespace, then cut to at most `max` characters at a word, ending in "…" when cut. */
junipercrew

New module with no direct test. The edge cases (no space, a cut that lands on a space, trailing parenthesis) are only covered by the one menu case.
2 export function wordClip(text: string, max: number): string {
3 const flat = text.replace(/\s+/g, " ").trim();
4 if (flat.length <= max) return flat;
5 const room = flat.slice(0, Math.max(max - 1, 0));
6 const whole = flat[room.length] === " " ? room : room.slice(0, Math.max(room.lastIndexOf(" "), 0));
7 return `${whole.replace(/[\s(]+$/, "")}…`;
8 }
src/web/layout.ts +6 −1
@@ -15,6 +15,7 @@ import { administersAny } from "../core/admin_repos.ts";
1515 import { appsForRepos } from "../core/apps.ts";
1616 import { foundryHref } from "../actions/foundry_door.ts";
1717 import { appsRuntime } from "../apps/runtime.ts";
18 import { wordClip } from "../text/word_clip.ts";
1819 import { absorbedForNav } from "../actions/features.ts";
1920 import {
2021 enterableApps,
@@ -264,6 +265,9 @@ function newMenu(ctx: Ctx): Html {
264265 });
265266 }
266267
268 /** A summary's longest title: a tooltip, not a paragraph. */
269 const SUMMARY_TITLE_MAX = 200;
270
267271 /**
268272 * The apps absorbed as the forge's own features, for the menu: each
269273 * opens at its declared page through the forge's door, which vouches for the
@@ -277,7 +281,8 @@ function featureItems(ctx: Ctx, user: User): MenuItem[] {
277281 runtime === undefined
278282 ? `/${f.repo}`
279283 : `/apps/open?host=${encodeURIComponent(new URL(runtime.urlFor(f.record, "production")).host)}&to=${encodeURIComponent(f.page)}`;
280 return { label: f.name, href, icon: "rocket" as const };
284 const entry: MenuItem = { label: f.name, href, icon: "rocket" };
285 return f.summary ? { ...entry, title: wordClip(f.summary, SUMMARY_TITLE_MAX) } : entry;
281286 });
282287 // Under a heading of their own, so an app's chosen name cannot pass for a forge item.
283288 return entries.length === 0
src/web/pages/features.ts +3 −1
@@ -60,7 +60,9 @@ function featureRow(f: FeatureView) {
6060 ${f.pending !== null ? badge("declaration changed: review", { tone: "attn" }) : null}`,
6161 meta: html`${f.pending !== null
6262 ? html`Menu: <strong>${f.name}</strong> at <span class="mono">${f.page}</span> → app: <strong>${f.pending.name}</strong> at
63 <span class="mono">${f.pending.page}</span>`
63 <span class="mono">${f.pending.page}</span>${f.pending.summary !== null
64 ? html`<br />Menu title: ${f.pending.summary}`
65 : html`<br />Menu title: none`}`
6466 : f.problem !== null
6567 ? html`<span class="danger">${f.problem}</span>`
6668 : html`${f.summary ?? ""}${when(f.summary !== null, () => html`<br />`)}${f.settings.length} setting${f.settings.length === 1 ? "" : "s"}${f.absorbed
src/web/ui.ts +3 −1
@@ -1107,6 +1107,8 @@ export interface MenuItem {
11071107 disabled?: string;
11081108 /** The key that does the same, drawn beside the label. */
11091109 hint?: string;
1110 /** The link's tooltip; escaped. Absent, no title renders. */
1111 title?: string;
11101112 }
11111113
11121114 /** A dropdown built on `<details>` — the only construct correct with JS off. */
@@ -1161,7 +1163,7 @@ export function menu(o: {
11611163 return html`<a
11621164 class="menu-item"
11631165 href="${i.href ?? "#"}"
1164 ${i.current ? raw('aria-current="true"') : null}
1166 ${i.current ? raw('aria-current="true"') : i.title ? html`title="${i.title}"` : null}
11651167 >${i.icon ? icon(i.icon) : null}<span class="grow">${i.label}</span
11661168 >${i.hint ? keyHint(i.hint) : null}${i.current ? icon("check") : null}</a
11671169 >`;
test/features.test.ts +42 −1
@@ -307,7 +307,7 @@ describe("absorbing an app", () => {
307307 const c = await commitToBranch(app, cy, repo, { message: "rename", files: [{ path: "forge.json", content: renamed }] });
308308 if (!c.ok) throw new Error(JSON.stringify(c.error));
309309 const view = await getFeature(app, ada, "radar");
310 expect(view.ok && view.value.pending).toEqual({ name: "Radar Pro", page: "/pro" });
310 expect(view.ok && view.value.pending).toEqual({ name: "Radar Pro", page: "/pro", summary: FEATURE.summary });
311311 expect(view.ok && view.value.name).toBe("Radar");
312312 const list = await (await web(adaCookie, "/settings/features")).text();
313313 expect(list).toContain("declaration changed: review");
@@ -324,6 +324,24 @@ describe("absorbing an app", () => {
324324 expect(absorbedForNav(app, ada)[0]!.name).toBe("Radar");
325325 });
326326
327 test("a changed summary alone is pending, and accepting stores it for the menu title", async () => {
328 const before = absorbedForNav(app, ada)[0]!;
329 expect(before.summary).toBe(FEATURE.summary);
330 const edit = JSON.stringify({ app: { run: "x" }, feature: { ...FEATURE, summary: "Now says more" } });
331 const c = await commitToBranch(app, cy, repo, { message: "summary", files: [{ path: "forge.json", content: edit }] });
332 if (!c.ok) throw new Error(JSON.stringify(c.error));
333 const view = await getFeature(app, ada, "radar");
334 expect(view.ok && view.value.pending).toEqual({ name: "Radar", page: "/", summary: "Now says more" });
335 expect(absorbedForNav(app, ada)[0]!.summary).toBe(FEATURE.summary); // the menu waits
336 const done = await acceptDeclaration(app, ada, "radar", { name: "Radar", page: "/" });
337 expect(done.ok).toBe(true);
338 expect(absorbedForNav(app, ada)[0]!.summary).toBe("Now says more");
339 const after = await getFeature(app, ada, "radar");
340 expect(after.ok && after.value.pending).toBeNull();
341 await commitToBranch(app, cy, repo, { message: "back", files: [{ path: "forge.json", content: FORGE_JSON }] });
342 await acceptDeclaration(app, ada, "radar", { name: "Radar", page: "/" });
343 });
344
327345 test("accepting what the admin reviewed is refused when production declared something else since", async () => {
328346 const declare = (name: string) =>
329347 commitToBranch(app, cy, repo, { message: name, files: [{ path: "forge.json", content: JSON.stringify({ app: { run: "x" }, feature: { ...FEATURE, name } }) }] });
@@ -510,6 +528,29 @@ describe("absorbing an app", () => {
510528 expect(page).toContain("Many 19");
511529 });
512530
531 test("a menu entry carries the declared summary as its title, escaped and shortened", async () => {
532 const make = async (slug: string, name: string, summary: string | null) => {
533 const r = await createRepo(app, cy, { name: slug, visibility: "public", description: "" });
534 if (!r.ok) throw new Error(String(r.error));
535 const row = insertApp(app.db, { repoId: r.value.id, slug, access: "public" });
536 if (!row.ok) throw new Error(row.error);
537 absorb(app.db, row.value.id, ada.id, name, "/", summary);
538 };
539 await make("with-sum", "Withsum", "Watches the fleet");
540 await make("no-sum", "Nosum", null);
541 await make("evil-sum", "Evilsum", "<script>alert(1)</script>");
542 await make("long-sum", "Longsum", `${"word ".repeat(60)}end`);
543 const page = await (await web(adaCookie, "/settings")).text();
544 const entry = (slug: string) => page.split(/<a\s/).find((c) => c.includes(`href="/cy/${slug}"`)) ?? "";
545 expect(entry("with-sum")).toContain('title="Watches the fleet"');
546 expect(entry("no-sum")).not.toContain("title=");
547 expect(entry("evil-sum")).toContain("title=\"&lt;script&gt;alert(1)&lt;/script&gt;\"");
548 expect(page).not.toContain("<script>alert(1)");
549 const long = /title="([^"]*)"/.exec(entry("long-sum"))?.[1] ?? "";
550 expect(long.length).toBeLessThanOrEqual(200);
551 expect(long.endsWith("word…")).toBe(true);
552 });
553
513554 test("deleting the app takes its feature row and settings with it", () => {
514555 const rows = (table: string) =>
515556 app.db.query(`SELECT 1 FROM ${table} WHERE app_id = ?1`).all(appId).length;
test/word-clip.test.ts added +30
@@ -0,0 +1,30 @@
1 import { expect, test } from "bun:test";
2 import { wordClip } from "../src/text/word_clip.ts";
3
4 test("text that fits is returned, whitespace folded", () => {
5 expect(wordClip(" a b\n c ", 20)).toBe("a b c");
6 });
7
8 test("input of exactly the limit is not cut", () => {
9 const text = "x".repeat(199) + "y";
10 expect(wordClip(text, 200)).toBe(text);
11 expect(wordClip(`${text}z`, 200).length).toBeLessThanOrEqual(200);
12 });
13
14 test("a long text is cut at a word and ends in an ellipsis within the limit", () => {
15 const out = wordClip("alpha beta gamma delta", 14);
16 expect(out).toBe("alpha beta…");
17 expect(out.length).toBeLessThanOrEqual(14);
18 });
19
20 test("a cut that lands on a space keeps the whole word before it", () => {
21 expect(wordClip("alpha beta gamma", 11)).toBe("alpha beta…");
22 });
23
24 test("no space in the first characters gives just the ellipsis", () => {
25 expect(wordClip("x".repeat(300), 200)).toBe("…");
26 });
27
28 test("trailing space and an open parenthesis are not left before the ellipsis", () => {
29 expect(wordClip("alpha (beta gamma", 9)).toBe("alpha…");
30 });

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs