Public: anyone can read this; the crew can change it.
Absorb: Settings > Features and /api/v1/features read git for every app on every request #9650
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/9543-absorb-settings-features-and-api-v1-feat → main 3 commits fork 2 threads · 2 unresolved
3 files
changed +57
−7
src/actions/features.ts +20 −1
| @@ -80,11 +80,28 @@ export interface FeatureView { | ||
| 80 | 80 | const siteAdmin = (actor: User | undefined): actor is User => |
| 81 | 81 | actor !== undefined && actor.isAdmin && actor.kind !== "agent"; |
| 82 | 82 | |
| 83 | /** | |
| 84 | * The parsed declaration by commit oid. A commit never changes, so what its | |
| 85 | * forge.json declares never does: only an app whose tip moved reads git again. | |
| 86 | * Bounded by clearing whole when full; a refill costs one read per app. | |
| 87 | */ | |
| 88 | const declared = new Map<string, Result<FeatureSpec | undefined, string>>(); | |
| 89 | const DECLARED_MAX = 2000; | |
| 90 | ||
| 83 | 91 | /** The feature an app's default branch declares, as `forge.json` says it at the tip. */ |
| 84 | 92 | export async function declaredFeature(app: App, repo: Repo): Promise<Result<FeatureSpec | undefined, string>> { |
| 85 | 93 | const dir = repoDir(app.reposRoot, repo.ownerName, repo.name); |
| 86 | 94 | const tip = await resolveOid(dir, `refs/heads/${repo.defaultBranch}`); |
| 87 | 95 | if (tip === undefined) return ok(undefined); |
| 96 | const known = declared.get(tip); | |
| 97 | if (known !== undefined) return known; | |
| 98 | const read = await readDeclaration(dir, tip); | |
| 99 | if (declared.size >= DECLARED_MAX) declared.clear(); | |
| 100 | declared.set(tip, read); | |
| 101 | return read; | |
| 102 | } | |
| 103 | ||
| 104 | async function readDeclaration(dir: string, tip: string): Promise<Result<FeatureSpec | undefined, string>> { | |
| 88 | 105 | const text = await readFileAt(dir, tip, "forge.json"); |
| 89 | 106 | if (text === undefined) return ok(undefined); |
| 90 | 107 | let parsed: unknown; |
| @@ -165,6 +182,8 @@ export interface NavFeature { | ||
| 165 | 182 | readonly name: string; |
| 166 | 183 | /** The app path to open: the feature's page. */ |
| 167 | 184 | readonly page: string; |
| 185 | /** The app record already read, so the menu does not fetch it again. */ | |
| 186 | readonly record: AppRecord; | |
| 168 | 187 | } |
| 169 | 188 | |
| 170 | 189 | /** Absorbed apps the actor may read, for the forge's nav. Cheap enough for every page: no git reads. */ |
| @@ -175,7 +194,7 @@ export function absorbedForNav(app: App, actor: User | undefined): NavFeature[] | ||
| 175 | 194 | if (record === undefined) continue; |
| 176 | 195 | const repo = getRepoById(app.db, record.repoId); |
| 177 | 196 | if (repo === undefined || !canRead(app.db, repo, actor)) continue; |
| 178 | out.push({ appId: record.id, slug: record.slug, repo: `${repo.ownerName}/${repo.name}`, name: row.name, page: row.page }); | |
| 197 | out.push({ appId: record.id, slug: record.slug, repo: `${repo.ownerName}/${repo.name}`, name: row.name, page: row.page, record }); | |
| 179 | 198 | } |
| 180 | 199 | return out; |
| 181 | 200 | } |
src/web/layout.ts +2 −4
| @@ -16,7 +16,6 @@ import { appsForRepos } from "../core/apps.ts"; | ||
| 16 | 16 | import { foundryHref } from "../actions/foundry_door.ts"; |
| 17 | 17 | import { appsRuntime } from "../apps/runtime.ts"; |
| 18 | 18 | import { absorbedForNav } from "../actions/features.ts"; |
| 19 | import { getAppById } from "../core/apps.ts"; | |
| 20 | 19 | import { |
| 21 | 20 | enterableApps, |
| 22 | 21 | getRepo, |
| @@ -273,12 +272,11 @@ function newMenu(ctx: Ctx): Html { | ||
| 273 | 272 | function featureItems(ctx: Ctx, user: User): MenuItem[] { |
| 274 | 273 | const runtime = appsRuntime(ctx.app); |
| 275 | 274 | return absorbedForNav(ctx.app, user).map((f) => { |
| 276 | const record = getAppById(ctx.app.db, f.appId); | |
| 277 | 275 | // Without a runtime there is no host to open: the app's own page instead. |
| 278 | 276 | const href = |
| 279 | runtime === undefined || record === undefined | |
| 277 | runtime === undefined | |
| 280 | 278 | ? `/${f.repo}` |
| 281 | : `/apps/open?host=${encodeURIComponent(new URL(runtime.urlFor(record, "production")).host)}&to=${encodeURIComponent(f.page)}`; | |
| 279 | : `/apps/open?host=${encodeURIComponent(new URL(runtime.urlFor(f.record, "production")).host)}&to=${encodeURIComponent(f.page)}`; | |
| 282 | 280 | return { label: f.name, href, icon: "rocket" as const }; |
| 283 | 281 | }); |
| 284 | 282 | } |
test/features.test.ts +35 −2
| @@ -12,15 +12,17 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | ||
| 12 | 12 | import { mkdtemp, rm } from "node:fs/promises"; |
| 13 | 13 | import { tmpdir } from "node:os"; |
| 14 | 14 | import { join } from "node:path"; |
| 15 | import { statements } from "./helpers/statements.ts"; | |
| 16 | import { expectWork } from "./helpers/work.ts"; | |
| 15 | 17 | import { createApp, type App } from "../src/app.ts"; |
| 16 | import { absorbApp, absorbedForNav, changeFeatureSettings, getFeature, unabsorbApp } from "../src/actions/features.ts"; | |
| 18 | import { absorbApp, absorbedForNav, declaredFeature, changeFeatureSettings, getFeature, unabsorbApp } from "../src/actions/features.ts"; | |
| 17 | 19 | import { commitToBranch, createRepo } from "../src/actions/repos.ts"; |
| 18 | 20 | import { checkSetting, featureEnv, parseFeature, settingEnvName } from "../src/apps/feature.ts"; |
| 19 | 21 | import { featureVarsAt } from "../src/apps/feature_env.ts"; |
| 20 | 22 | import { appEnv } from "../src/apps/process.ts"; |
| 21 | 23 | import { createApp as insertApp } from "../src/core/apps.ts"; |
| 22 | 24 | import { listEvents } from "../src/core/events.ts"; |
| 23 | import { featureSettings, getAbsorbed } from "../src/core/features.ts"; | |
| 25 | import { absorb, featureSettings, getAbsorbed } from "../src/core/features.ts"; | |
| 24 | 26 | import type { Repo } from "../src/core/repos.ts"; |
| 25 | 27 | import { createToken } from "../src/core/tokens.ts"; |
| 26 | 28 | import { createUser, type User } from "../src/core/users.ts"; |
| @@ -330,4 +332,35 @@ describe("absorbing an app", () => { | ||
| 330 | 332 | const stranger = await mcpTool(servers.url, bobToken, "features", { slug: "radar" }); |
| 331 | 333 | expect(stranger.error).toBeDefined(); |
| 332 | 334 | }); |
| 335 | ||
| 336 | test("the declaration is cached by tip: the same tip is one read, a moved tip reads again", async () => { | |
| 337 | const first = await declaredFeature(app, repo); | |
| 338 | expect(await declaredFeature(app, repo)).toBe(first); // the very same parsed result | |
| 339 | const moved = JSON.stringify({ app: { run: "bun run src/server.ts" }, feature: { ...FEATURE, name: "Radar 2" } }); | |
| 340 | const done = await commitToBranch(app, cy, repo, { message: "rename", files: [{ path: "forge.json", content: moved }] }); | |
| 341 | if (!done.ok) throw new Error(JSON.stringify(done.error)); | |
| 342 | const after = await declaredFeature(app, repo); | |
| 343 | expect(after).not.toBe(first); | |
| 344 | expect(after.ok && after.value?.name).toBe("Radar 2"); | |
| 345 | }); | |
| 346 | ||
| 347 | test("with 20 more absorbed apps the menu is built from one pass and renders within budget", async () => { | |
| 348 | for (let i = 0; i < 20; i++) { | |
| 349 | const r = await createRepo(app, cy, { name: `many-${i}`, visibility: "public", description: "" }); | |
| 350 | if (!r.ok) throw new Error(String(r.error)); | |
| 351 | const row = insertApp(app.db, { repoId: r.value.id, slug: `many-${i}`, access: "public" }); | |
| 352 | if (!row.ok) throw new Error(row.error); | |
| 353 | absorb(app.db, row.value.id, ada.id, `Many ${i}`, "/"); | |
| 354 | } | |
| 355 | let nav = absorbedForNav(app, ada); | |
| 356 | expect(nav.length).toBeGreaterThanOrEqual(20); | |
| 357 | expect(nav.every((f) => f.record.id === f.appId)).toBe(true); // the record rides along | |
| 358 | const n = nav.length; | |
| 359 | const used = await statements(app.db, async () => { | |
| 360 | nav = absorbedForNav(app, ada); | |
| 361 | }); | |
| 362 | expectWork(used, { max: 2 + 3 * n }); // one list, then at most three reads per absorbed app | |
Next: the bound 2+3n sits one statement above the measured 64 for 21 apps, so it is brittle. Also, it measures absorbedForNav rather than the featureItems render. |
||
| 363 | const page = await (await web(adaCookie, "/settings")).text(); | |
| 364 | expect(page).toContain("Many 19"); | |
| 365 | }); | |
| 333 | 366 | }); |
new above
offline
Next: the cached parsed result is shared by reference with every caller. Freeze it or hand out a copy so a mutation cannot corrupt the cache.