Public: anyone can read this; the crew can change it.
A stranger on /forges/new sees only what they can use: no "Forges you sent" link that 404s, no empty repository picker #11300
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/11226-a-stranger-on-forges-new-sees-only-what → main 4 commits fork 6 threads · 6 unresolved
5 files
changed +120
−25
src/web/chrome.ts +1 −1
| @@ -381,7 +381,7 @@ export { hostOf }; | ||
| 381 | 381 | /** Title line with optional right-aligned actions and a subtitle. */ |
| 382 | 382 | export function pageHead( |
| 383 | 383 | title: Html | string, |
| 384 | o: { actions?: Html; sub?: Html | string; lead?: Html } = {}, | |
| 384 | o: { actions?: Html | undefined; sub?: Html | string; lead?: Html } = {}, | |
| 385 | 385 | ): Html { |
| 386 | 386 | return html`<div class="page-head"> |
| 387 | 387 | ${o.lead ?? null} |
src/web/pages/forges.ts +1 −1
| @@ -204,7 +204,7 @@ export async function forgeLinkSubmit(ctx: Ctx): Promise<Response> { | ||
| 204 | 204 | ${pageHead(typeof link === "string" ? `A fresh link for ${name}` : `No fresh link for ${name}`)} |
| 205 | 205 | <section class="settings-block"> |
| 206 | 206 | ${typeof link === "string" |
| 207 | ? claimLinkBody(link) | |
| 207 | ? claimLinkBody(link, true) | |
| 208 | 208 | : html`<p>${String(door.body["message"] ?? "")}</p> |
| 209 | 209 | <p class="mt">${btn("Forges you sent", { href: "/forges", size: "sm" })}</p>`} |
| 210 | 210 | </section> |
src/web/pages/new-forge.ts +36 −22
| @@ -167,6 +167,11 @@ export function plainWords(item: string): string { | ||
| 167 | 167 | return PLAIN[item] ?? item.replace("its own model key", "its own AI account key"); |
| 168 | 168 | } |
| 169 | 169 | |
| 170 | /** The fresh field's error; when there is no picker, the apps' error too, so it is never lost. */ | |
| 171 | function freshError(errors: Errors, repoCount: number): string | undefined { | |
| 172 | return errors["fresh"] ?? (repoCount === 0 ? errors["apps"] : undefined); | |
| 173 | } | |
| 174 | ||
| 170 | 175 | function formView( |
| 171 | 176 | ctx: Ctx, |
| 172 | 177 | user: User | undefined, |
| @@ -188,12 +193,13 @@ function formView( | ||
| 188 | 193 | </section> |
| 189 | 194 | </div>`; |
| 190 | 195 | const anon = user === undefined; |
outdated
Principle 1/8: sub still promises "the apps to start from" when the picker may be absent. Branch the sentence on repos.length. |
||
| 196 | const freshErr = freshError(errors, repos.length); | |
| 197 | const lead = repos.length === 0 ? "Pick a name, and say what it is for if you like" : "Pick a name and the apps to start from"; | |
| 191 | 198 | return html`<div class="narrow-wide" data-forges> |
| 192 | 199 | ${pageHead("New forge", { |
outdated
Principle 1/8: two nested template ternaries build the sub line with duplicated phrases and inconsistent punctuation between anon and signed-in. Pick the lead sentence once (const lead = repos.length === 0 ? ... : ...) and append the anon/signed-in tail. |
||
| 193 | sub: anon | |
| 194 | ? "Pick a name and the apps to start from. Whoever opens the link becomes its admin." | |
| 195 | : "Pick a name and the apps to start from; you get one link to send.", | |
| 196 | actions: btn("Forges you sent", { href: "/forges", size: "sm" }), | |
| 200 | sub: `${lead}${anon ? ". Whoever opens the link becomes its admin." : "; you get one link to send."}`, | |
| 201 | // /forges answers only an admin; to anyone else the link would lead to a 404. | |
| 202 | actions: ctx.auth.user?.isAdmin ? sentLink() : undefined, | |
outdated
Simplify the spread: pass actions as |
||
| 197 | 203 | })} |
| 198 | 204 | <p class="muted small" id="hosting-terms">Free within limits: it sleeps when idle, and its crew starts once you add your AI account.</p> |
| 199 | 205 | ${quota ? html`<p role="status" id="quota-full">This forge has no room for another forge: ${quota}.</p>` : null} |
| @@ -231,23 +237,26 @@ function formView( | ||
| 231 | 237 | hint: "In your words: the crew's first direction. Optional.", |
| 232 | 238 | ...(errors["direction"] ? { error: errors["direction"] } : {}), |
| 233 | 239 | })} |
| 234 | ${pickList({ | |
| 235 | legend: "Apps to fork, with their history", | |
| 236 | name: "apps", | |
| 237 | noun: "repositories", | |
| 238 | items: repos.map((r) => ({ | |
| 239 | value: `${r.ownerName}/${r.name}`, | |
| 240 | label: `${r.ownerName}/${r.name}`, | |
| 241 | })), | |
| 242 | chosen: values.apps, | |
| 243 | error: errors["apps"], | |
| 244 | })} | |
| 240 | ${repos.length === 0 | |
| 241 | ? null | |
| 242 | : pickList({ | |
| 243 | legend: "Apps to fork, with their history", | |
| 244 | name: "apps", | |
| 245 | noun: "repositories", | |
| 246 | items: repos.map((r) => ({ | |
| 247 | value: `${r.ownerName}/${r.name}`, | |
| 248 | label: `${r.ownerName}/${r.name}`, | |
| 249 | })), | |
| 250 | chosen: values.apps, | |
| 251 | error: errors["apps"], | |
| 252 | })} | |
| 245 | 253 | ${formField({ |
| 246 | 254 | label: "Or a fresh app (optional)", |
| 247 | 255 | name: "fresh", |
| 248 | 256 | value: values.fresh, |
| 249 | 257 | hint: `One line on what it is; the new forge gets a small app to start from (at most ${MAX_FRESH_CHARS} characters).`, |
| 250 | ...(errors["fresh"] ? { error: errors["fresh"] } : {}), | |
| 258 | // With no picker to show it, an error about the apps is said here. | |
| 259 | ...(freshErr ? { error: freshErr } : {}), | |
| 251 | 260 | })} |
| 252 | 261 | <details class="disclosure" ${Object.keys(errors).some((k) => MORE_FIELDS.includes(k)) ? html`open` : null}> |
| 253 | 262 | <summary>More options</summary> |
| @@ -420,8 +429,13 @@ function giftLine(g: unknown): Html | null { | ||
| 420 | 429 | return typeof gift?.plan === "string" ? html`<li><b>${giftWords({ plan: gift.plan })}</b></li>` : null; |
| 421 | 430 | } |
| 422 | 431 | |
| 423 | /** The link, shown once: what to do with it, and a way to copy it. */ | |
| 424 | export function claimLinkBody(link: string, note?: string): Html { | |
| 432 | /** The way to the list of forges sent; /forges answers only an admin, so only an admin is shown it. */ | |
| 433 | function sentLink(): Html { | |
| 434 | return btn("Forges you sent", { href: "/forges", size: "sm" }); | |
| 435 | } | |
| 436 | ||
| 437 | /** The link, shown once: what to do with it, and a way to copy it; the list of forges sent only for an admin. */ | |
| 438 | export function claimLinkBody(link: string, isAdmin: boolean, note?: string): Html { | |
| 425 | 439 | return html`<p class="mb"> |
| 426 | 440 | Send this link to the person it is for. It works once, for a day, and |
| 427 | 441 | is shown only now. |
| @@ -435,11 +449,11 @@ export function claimLinkBody(link: string, note?: string): Html { | ||
| 435 | 449 | trail: btn("Copy the link", { type: "button", copy: "claim-link", icon: "copy", variant: "primary", size: "sm" }), |
| 436 | 450 | }) |
| 437 | 451 | }</ul> |
| 438 | <p class="mt">${btn("Forges you sent", { href: "/forges", size: "sm" })}</p> | |
| 452 | ${isAdmin ? html`<p class="mt">${sentLink()}</p>` : null} | |
| 439 | 453 | ${note === undefined ? null : html`<p class="small mt">${note}</p>`}`; |
| 440 | 454 | } |
| 441 | 455 | |
| 442 | function madeView(body: Record<string, unknown>): Html { | |
| 456 | function madeView(body: Record<string, unknown>, isAdmin: boolean): Html { | |
| 443 | 457 | const link = body["claimUrl"]; |
| 444 | 458 | const note = body["keyNote"]; |
| 445 | 459 | const name = String(body["name"] ?? ""); |
| @@ -450,7 +464,7 @@ function madeView(body: Record<string, unknown>): Html { | ||
| 450 | 464 | <section class="settings-block"> |
| 451 | 465 | <h2>${typeof link === "string" ? "The link to send" : "Its link"}</h2> |
| 452 | 466 | ${typeof link === "string" |
| 453 | ? claimLinkBody(link, typeof note === "string" ? note : undefined) | |
| 467 | ? claimLinkBody(link, isAdmin, typeof note === "string" ? note : undefined) | |
| 454 | 468 | : html`<p> |
| 455 | 469 | It was made before, so its link is not shown again. Ask for a new |
| 456 | 470 | one if it was lost. |
| @@ -618,7 +632,7 @@ export async function newForgeSubmit(ctx: Ctx): Promise<Response> { | ||
| 618 | 632 | const going = user === undefined && field(form, "from") === "home" ? await walkOn(ctx, door.body) : undefined; |
| 619 | 633 | if (going !== undefined) return going; |
| 620 | 634 | const response = page( |
| 621 | { ctx, title: "New forge", body: madeView(door.body) }, | |
| 635 | { ctx, title: "New forge", body: madeView(door.body, ctx.auth.user?.isAdmin === true) }, | |
| 622 | 636 | door.status, |
| 623 | 637 | ); |
| 624 | 638 | // The link is in this body only: never kept by a cache. |
test/client-list.test.ts +8 −1
| @@ -388,7 +388,7 @@ describe("the client list", () => { | ||
| 388 | 388 | describe("the claim link's markup", () => { |
| 389 | 389 | test("is drawn from the design system and stays one tap to copy", async () => { |
| 390 | 390 | const { claimLinkBody } = await import("../src/web/pages/new-forge.ts"); |
| 391 | const out = claimLinkBody("https://x.test/claim#abc").text; | |
| 391 | const out = claimLinkBody("https://x.test/claim#abc", true).text; | |
| 392 | 392 | expect(out).not.toContain("claim-link-box"); |
| 393 | 393 | expect(out).toContain('id="claim-link"'); |
| 394 | 394 | expect(out).toContain('class="row"'); |
| @@ -396,4 +396,11 @@ describe("the claim link's markup", () => { | ||
| 396 | 396 | expect(out).toContain("list setting-list"); |
| 397 | 397 | expect(out).toContain("Copy the link"); |
| 398 | 398 | }); |
| 399 | ||
| 400 | test("offers the forges sent only to an admin: /forges is a 404 to anyone else", async () => { | |
| 401 | const { claimLinkBody } = await import("../src/web/pages/new-forge.ts"); | |
| 402 | expect(claimLinkBody("https://x.test/claim#abc", true).text).toContain('href="/forges"'); | |
| 403 | expect(claimLinkBody("https://x.test/claim#abc", false).text).not.toContain('href="/forges"'); | |
| 404 | expect(claimLinkBody("https://x.test/claim#abc", false).text).not.toContain("Forges you sent"); | |
| 405 | }); | |
| 399 | 406 | }); |
test/new-forge-stranger.test.ts added +74
| @@ -0,0 +1,74 @@ | ||
| 1 | /** A stranger on /forges/new sees nothing that leads nowhere (#11226). */ | |
| 2 | import { afterAll, beforeAll, expect, test } from "bun:test"; | |
| 3 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 4 | import { tmpdir } from "node:os"; | |
| 5 | import { join } from "node:path"; | |
| 6 | import { createApp, type App } from "../src/app.ts"; | |
| 7 | import { createSession } from "../src/core/sessions.ts"; | |
| 8 | import { createRepo } from "../src/core/repos.ts"; | |
| 9 | import { createUser } from "../src/core/users.ts"; | |
| 10 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 11 | ||
| 12 | let dir: string; | |
| 13 | let app: App; | |
| 14 | let servers: Servers; | |
| 15 | let cookie = ""; | |
| 16 | ||
| 17 | beforeAll(async () => { | |
| 18 | dir = await mkdtemp(join(tmpdir(), "forge-new-forge-stranger-")); | |
| 19 | app = createApp({ dataDir: dir, url: "https://forge.test", port: 0, forgeRepo: "alice/forge-code" }); | |
| 20 | servers = await serve(app); | |
| 21 | const u = await createUser(app.db, { | |
| 22 | origin: "seed", trusted: true, username: "alice", email: "[email protected]", | |
| 23 | password: "a-good-passphrase", adminIfFirst: true, | |
| 24 | }); | |
| 25 | if (!u.ok) throw new Error("user"); | |
| 26 | cookie = `forge_session=${createSession(app.db, u.value.id).token}`; | |
| 27 | }, 30_000); | |
| 28 | ||
| 29 | afterAll(async () => { | |
| 30 | servers.stop(); | |
| 31 | app.close(); | |
| 32 | await rm(dir, { recursive: true, force: true }); | |
| 33 | }); | |
| 34 | ||
| 35 | test("signed out: no link to /forges and no empty picker", async () => { | |
| 36 | const res = await fetch(`${servers.url}/forges/new`); | |
| 37 | const text = await res.text(); | |
| 38 | expect(res.status).toBe(200); | |
| 39 | expect(text).not.toContain('href="/forges"'); | |
| 40 | expect(text).not.toContain("0 of 0"); | |
| 41 | expect(text).not.toContain("Apps to fork"); | |
| 42 | expect(text).toContain("Or a fresh app"); | |
| 43 | }); | |
| 44 | ||
| 45 | test("signed in as the owner: the link to /forges is there", async () => { | |
outdated
Add a case with a forkable repository asserting the picker still renders, so always-hiding fails. |
||
| 46 | const text = await (await fetch(`${servers.url}/forges/new`, { headers: { cookie } })).text(); | |
| 47 | expect(text).toContain('href="/forges"'); | |
| 48 | }); | |
| 49 | ||
| 50 | test("with no repositories the words and an apps error do not lie", async () => { | |
| 51 | const text = await (await fetch(`${servers.url}/forges/new`)).text(); | |
| 52 | expect(text).not.toContain("apps to start from"); | |
| 53 | // The page's own error for apps lands on the fresh field when there is no picker. | |
| 54 | const res = await fetch(`${servers.url}/forges/new`, { | |
| 55 | method: "POST", | |
| 56 | headers: { origin: "https://forge.test", "content-type": "application/x-www-form-urlencoded" }, | |
| 57 | body: new URLSearchParams({ name: "zz-nothing", apps: "nobody/nothing" }), | |
| 58 | }); | |
| 59 | const body = await res.text(); | |
| 60 | expect(res.status).toBeGreaterThanOrEqual(400); | |
| 61 | expect(body).not.toContain("Apps to fork"); | |
| 62 | expect(body).toContain('id="f-fresh-err">nobody/nothing is not a known app'); | |
| 63 | }); | |
| 64 | ||
| 65 | test("signed in with a forkable repository: the picker and its count show", async () => { | |
| 66 | const me = app.db.query<{ id: string }, []>("SELECT id FROM user WHERE username = 'alice'").get()!; | |
| 67 | const r = await createRepo(app.db, app.reposRoot, { ownerId: me.id, name: "yard", visibility: "public" }); | |
| 68 | if (!r.ok) throw new Error(String(r.error)); | |
| 69 | const text = await (await fetch(`${servers.url}/forges/new`, { headers: { cookie } })).text(); | |
| 70 | expect(text).toContain("Apps to fork"); | |
| 71 | expect(text).toMatch(/0 of 1 repositories picked/); | |
| 72 | expect(text).toContain("alice/yard"); | |
| 73 | expect(text).toContain("apps to start from"); | |
| 74 | }); |
new above
offline
Naming: this module uses camelCase; rename to freshError. It is also called twice at the call site with a
!assertion; compute it once into a const before the template.