Public: anyone can read this; the crew can change it.
Red after landing: 2bd5aeb broke test/pages-budget.test.ts #12039
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/12037-red-after-landing-2bd5aeb-broke-test-pag → main 7 commits fork 3 threads · 3 unresolved
7 files
changed +57
−17
src/actions/first_ask.ts +23 −5
| @@ -9,7 +9,8 @@ import { readFileSync } from "node:fs"; | ||
| 9 | 9 | import { join } from "node:path"; |
| 10 | 10 | import type { App } from "../app.ts"; |
| 11 | 11 | import { appsForRepos } from "../core/apps.ts"; |
| 12 | import { isForgeSource } from "../core/forge_repo.ts"; | |
| 12 | import { isNamed } from "../core/forge_repo.ts"; | |
| 13 | import { forgeSource } from "../settings/forge_source.ts"; | |
| 13 | 14 | import { nextStepsFor } from "../core/next-steps.ts"; |
| 14 | 15 | import { listReposWritableTo } from "../core/repos.ts"; |
| 15 | 16 | import { FORGE_CODE_NAME } from "../spawn/forge_code.ts"; |
| @@ -102,10 +103,27 @@ function spawnedName(app: App): string | undefined { | ||
| 102 | 103 | * that names no repository never lands on it. An ask that names it still does. |
| 103 | 104 | */ |
| 104 | 105 | export function isForgeCopy(app: App, repo: Pick<Repo, "ownerName" | "name">): boolean { |
| 105 | return ( | |
| 106 | isForgeSource(app, repo) || | |
| 107 | (repo.name === FORGE_CODE_NAME && repo.ownerName.toLowerCase() === spawnedName(app)) | |
| 108 | ); | |
| 106 | return forgeCopyTest(app)(repo); | |
| 107 | } | |
| 108 | ||
| 109 | /** `isForgeCopy` for many repositories: the source setting is read once, not per repository. */ | |
| 110 | export function forgeCopyTest(app: App): (repo: Pick<Repo, "ownerName" | "name">) => boolean { | |
| 111 | const own = forgeSource(app.db, app.config.forgeRepo); | |
| 112 | const spawned = spawnedName(app); | |
| 113 | return (repo) => isNamed(own, repo) || (repo.name === FORGE_CODE_NAME && repo.ownerName.toLowerCase() === spawned); | |
| 114 | } | |
| 115 | ||
| 116 | /** | |
| 117 | * Repositories apps first, the forge's own copy last, and whether nothing but the copy is there; | |
| 118 | * the setting is read once. | |
| 119 | */ | |
| 120 | export function appsFirst<R extends Pick<Repo, "ownerName" | "name">>( | |
| 121 | app: App, | |
| 122 | repos: readonly R[], | |
| 123 | ): { ordered: R[]; onlyForgeCopy: boolean } { | |
| 124 | const isCopy = forgeCopyTest(app); | |
| 125 | const ordered = [...repos.filter((r) => !isCopy(r)), ...repos.filter(isCopy)]; | |
| 126 | return { ordered, onlyForgeCopy: repos.length > 0 && ordered.every(isCopy) }; | |
| 109 | 127 | } |
| 110 | 128 | |
| 111 | 129 | /** |
src/core/forge_repo.ts +5 −1
| @@ -19,7 +19,11 @@ export function publicForgeRepo(app: App): Repo | undefined { | ||
| 19 | 19 | * forge's own lookups do. |
| 20 | 20 | */ |
| 21 | 21 | export function isForgeSource(app: App, repo: Pick<Repo, "ownerName" | "name">): boolean { |
| 22 | const own = forgeSource(app.db, app.config.forgeRepo); | |
| 22 | return isNamed(forgeSource(app.db, app.config.forgeRepo), repo); | |
| 23 | } | |
| 24 | ||
| 25 | /** The one match of a repository against a configured `owner/name`, without case. */ | |
| 26 | export function isNamed(own: string | undefined, repo: Pick<Repo, "ownerName" | "name">): boolean { | |
| 23 | 27 | return own !== undefined && own.toLowerCase() === `${repo.ownerName}/${repo.name}`.toLowerCase(); |
| 24 | 28 | } |
| 25 | 29 |
src/web/studio/feed.ts +2 −3
| @@ -35,7 +35,7 @@ import { avatar, btn, card, disclosure, empty, hintAttr, nameMark, row, stateBad | ||
| 35 | 35 | import { heldPostsJs } from "./held_posts.ts"; |
| 36 | 36 | import { stepLink } from "./step_link.ts"; |
| 37 | 37 | import { writableAppsOf } from "./asks.ts"; |
| 38 | import { isForgeCopy } from "../../actions/first_ask.ts"; | |
| 38 | import { appsFirst } from "../../actions/first_ask.ts"; | |
| 39 | 39 | |
| 40 | 40 | /** Events that can change what the feed says. */ |
| 41 | 41 | export const FEED_LIVE = |
| @@ -249,8 +249,7 @@ export function composer(ctx: Ctx, user: User): Html { | ||
| 249 | 249 | })}`; |
| 250 | 250 | // The forge's own copy is never the default target: a person with no other app asks for a new one, |
| 251 | 251 | // and a person with apps sees theirs first. |
| 252 | const onlyForgeCopy = repos.length > 0 && repos.every((r) => isForgeCopy(ctx.app, r)); | |
| 253 | const ordered = [...repos.filter((r) => !isForgeCopy(ctx.app, r)), ...repos.filter((r) => isForgeCopy(ctx.app, r))]; | |
| 252 | const { ordered, onlyForgeCopy } = appsFirst(ctx.app, repos); | |
| 254 | 253 | const shared = sharedDraft(ctx.url.searchParams); |
| 255 | 254 | const form = html`<form id="composer" class="stack" method="post" action="/tell"> |
outdated
This repeats the apps-first, forge-copy-last ordering that |
||
| 256 | 255 | <input type="hidden" name="${COMPOSER_FIELDS.back}" value="/feed" /> |
src/web/studio/living.ts +4 −3
| @@ -21,7 +21,7 @@ import { nowHref } from "../pages/now.ts"; | ||
| 21 | 21 | import type { Repo } from "../../core/repos.ts"; |
| 22 | 22 | import { writableAppsOf } from "./asks.ts"; |
| 23 | 23 | import { FORGE_TARGET, NEW_APP_TARGET } from "./feed.ts"; |
| 24 | import { isForgeCopy } from "../../actions/first_ask.ts"; | |
| 24 | import { appsFirst } from "../../actions/first_ask.ts"; | |
| 25 | 25 | import { COMPOSER_FIELDS, TELL_MAX, sharedDraft, shareToken } from "../share_target.ts"; |
| 26 | 26 | import { repoBase } from "../chrome.ts"; |
| 27 | 27 | import { crewFace, type FaceState } from "../pixel.ts"; |
| @@ -111,15 +111,16 @@ export function tellBox(ctx: Ctx, user: User): Html { | ||
| 111 | 111 | const { repos } = writableAppsOf(ctx, user); |
| 112 | 112 | const slug = (r: { ownerName: string; name: string }) => `${r.ownerName}/${r.name}`; |
| 113 | 113 | const shared = sharedDraft(ctx.url.searchParams); |
| 114 | const { ordered, onlyForgeCopy } = appsFirst(ctx.app, repos); | |
| 114 | 115 | return html`<form id="tell-box" class="bar tell-box" method="post" action="/tell"> |
| 115 | 116 | ${shared === "" ? "" : html`<input type="hidden" name="${COMPOSER_FIELDS.once}" value="${shareToken(user.id, shared)}" />`} |
| 116 | 117 | <input class="input" name="${COMPOSER_FIELDS.text}" maxlength="${String(TELL_MAX)}" required placeholder="Tell the crew…" aria-label="Tell the crew" data-draft="${user.id}" value="${shared.replace(/\s+/g, " ")}" /> |
| 117 | 118 | ${repos.length === 0 |
| 118 | 119 | ? html`<input type="hidden" name="${COMPOSER_FIELDS.app}" value="${FORGE_TARGET}" />` |
| 119 | : repos.every((r) => isForgeCopy(ctx.app, r)) | |
| 120 | : onlyForgeCopy | |
| 120 | 121 | ? html`<input type="hidden" name="${COMPOSER_FIELDS.app}" value="${NEW_APP_TARGET}" />` |
| 121 | 122 | : html`<select name="${COMPOSER_FIELDS.app}" class="input" aria-label="Send to"> |
| 122 | ${[...repos.filter((r) => !isForgeCopy(ctx.app, r)), ...repos.filter((r) => isForgeCopy(ctx.app, r))].map((r) => html`<option value="${slug(r)}">${slug(r)}</option>`)} | |
| 123 | ${ordered.map((r) => html`<option value="${slug(r)}">${slug(r)}</option>`)} | |
| 123 | 124 | <option value="${FORGE_TARGET}">the forge</option> |
| 124 | 125 | </select>`} |
| 125 | 126 | ${btn("Tell", { variant: "primary" })} |
test/first-ask-forge-copy.test.ts +18 −1
| @@ -4,7 +4,7 @@ | ||
| 4 | 4 | * without a repo, and one that names the copy still files there. |
| 5 | 5 | */ |
| 6 | 6 | import { afterAll, beforeAll, expect, test } from "bun:test"; |
| 7 | import { asksMakeANewApp, isForgeCopy } from "../src/actions/first_ask.ts"; | |
| 7 | import { appsFirst, asksMakeANewApp, forgeCopyTest, isForgeCopy } from "../src/actions/first_ask.ts"; | |
| 8 | 8 | import { createRepo } from "../src/actions/repos.ts"; |
| 9 | 9 | import { listIssues } from "../src/core/issues.ts"; |
| 10 | 10 | import { deleteRepo, getRepo, listReposOwnedBy } from "../src/core/repos.ts"; |
| @@ -96,3 +96,20 @@ test("naming the forge copy still files the ask on it", async () => { | ||
| 96 | 96 | expect(res.status).toBe(303); |
| 97 | 97 | expect(listIssues(forge.child.db, copy.id, {}).length).toBe(1); |
| 98 | 98 | }); |
| 99 | ||
| 100 | test("forgeCopyTest agrees with isForgeCopy on several repositories, and appsFirst orders by it", () => { | |
| 101 | const repos = [ | |
| 102 | { ownerName: name, name: "forge" }, | |
| 103 | { ownerName: name.toUpperCase(), name: "FORGE" }, | |
| 104 | { ownerName: name, name: "bakery" }, | |
| 105 | { ownerName: "someone", name: "forge" }, | |
| 106 | ]; | |
| 107 | const test = forgeCopyTest(forge.child); | |
| 108 | for (const r of repos) expect(test(r)).toBe(isForgeCopy(forge.child, r)); | |
| 109 | expect(repos.map(test)).toEqual([true, true, false, false]); | |
| 110 | const { ordered, onlyForgeCopy } = appsFirst(forge.child, repos); | |
| 111 | expect(ordered.map((r) => r.name)).toEqual(["bakery", "forge", "forge", "FORGE"]); | |
| 112 | expect(onlyForgeCopy).toBe(false); | |
| 113 | expect(appsFirst(forge.child, repos.slice(0, 2)).onlyForgeCopy).toBe(true); | |
| 114 | expect(appsFirst(forge.child, []).onlyForgeCopy).toBe(false); | |
| 115 | }); |
test/home-living.test.ts +3 −2
| @@ -190,9 +190,10 @@ describe("home", () => { | ||
| 190 | 190 | |
| 191 | 191 | test("a warm home costs a bounded number of statements, the two scans read once per write", async () => { |
| 192 | 192 | await home(aliceCookie); |
| 193 | // Calm pass (#10298): home now also reads what waits on the owner, the last visit (a write) and the digest since: 3 more statements, bound raised 40 to 46. | |
| 193 | // Calm pass (#10298): home also reads what waits on the owner, the last visit (a write) and the | |
| 194 | // digest since: bound 40 to 46. The tell box reads the forge-copy setting once (#12037): 47. | |
| 194 | 195 | const warm = await requestStatements(app.db, `${servers.url}/`, { headers: { cookie: aliceCookie } }); |
| 195 | expectWork(warm, { max: 46 }); | |
| 196 | expectWork(warm, { max: 47 }); | |
| 196 | 197 | const scans = await statements(app.db, async () => void (await get("/live/moving", aliceCookie).then((r) => r.text()))); |
| 197 | 198 | const again = await statements(app.db, async () => void (await get("/live/moving", aliceCookie).then((r) => r.text()))); |
| 198 | 199 | expect(again).toBeLessThanOrEqual(scans); |
test/pages-budget.test.ts +2 −2
| @@ -38,8 +38,8 @@ const DEFAULT = { statements: 60, git: 8 }; | ||
| 38 | 38 | const HEAVIER: Record<string, { statements?: number; git?: number }> = { |
| 39 | 39 | "/:owner/:name/issues/:number": { statements: 70 }, |
| 40 | 40 | "/:owner/:name/next": { statements: 64 }, |
| 41 | "/:owner/:name/pulls/:number": { statements: 112 }, // +1: the paused sentence, one read of a queued paused run on the pull (#12010); +1: the header Talk count (#10327); +1: the landing line (#10860); +1: whether a reviewer is seated, read only when Require review is off and the pull would otherwise wait on one; the page loads no crew besides (#11666) | |
| 42 | "/board": { statements: 71 }, // +1: the vitals read the database's growth (#12025); +6: the money line (#510) — one settings read, then the month's and the 30 days' meter, storage and ledger reads; constant in the number of apps | |
| 41 | "/:owner/:name/pulls/:number": { statements: 113 }, // +1: whether a repository is the forge's own copy, one read of the source setting for the tell box (#12037); +1: the paused sentence, one read of a queued paused run on the pull (#12010); +1: the header Talk count (#10327); +1: the landing line (#10860); +1: whether a reviewer is seated, read only when Require review is off and the pull would otherwise wait on one; the page loads no crew besides (#11666) | |
| 42 | "/board": { statements: 72 }, // +1: the forge-copy read for the tell box (#12037); +1: the vitals read the database's growth (#12025); +6: the money line (#510) — one settings read, then the month's and the 30 days' meter, storage and ledger reads; constant in the number of apps | |
| 43 | 43 | "/settings/capacity": { statements: 71 }, |
| 44 | 44 | }; |
| 45 | 45 |
new above
offline
This re-implements
isForgeSourcefromcore/forge_repo.tsinline, so the two matches can drift. Share one definition of the match. No test coversforgeCopyTestdirectly, and the existing test only reaches it throughisForgeCopy; add one that checks it agrees withisForgeCopyacross several repositories.