Public: anyone can read this; the crew can change it.
A pull's check summary carries the phone look of /preview-admin/sample/settings#move #10678
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/10336-a-pull-s-check-summary-carries-the-phone → main 2 commits fork 1 thread · 1 unresolved
8 files
changed +100
−22
docs/features/pull-looks.md +14
| @@ -31,3 +31,17 @@ they may enter, and not those of a sealed pull. A look that failed (no | ||
| 31 | 31 | picture) is never listed. Only the pull's own preview environment is read, so |
| 32 | 32 | another pull's or app's looks never appear. A pull with no app or no looks |
| 33 | 33 | answers `items: []`. |
| 34 | ||
| 35 | ## The check summary's looks | |
| 36 | ||
| 37 | `GET …/pulls/:number/checks` (and `…/commits/:oid/check`) answers `looks`: | |
| 38 | one `{ look, screens, sideways, size? }` per look the looks service recorded | |
| 39 | for that commit, `size` being the measured `{ scrollWidth, scrollHeight, | |
| 40 | innerWidth, innerHeight }` — so `phone /preview-admin/sample/settings#move` | |
| 41 | can be quoted as scrollWidth vs innerWidth. They are taken only for a preview of the forge's own repository | |
| 42 | (`ownerLooksFor`), at the pull's head once its preview serves. Each look is | |
| 43 | filed under the commit the moment it is taken (`addCommitLook`), not when the | |
| 44 | round of about fifty ends: a round cut short by a newer head or a restart | |
| 45 | used to file nothing, and the next round filed only what it still lacked, so | |
| 46 | the check's list stayed `[]` or partial. The list now fills as the round goes, | |
| 47 | and is complete when `…/looks` lists every page. |
src/actions/runs.ts +9 −2
| @@ -10,7 +10,7 @@ | ||
| 10 | 10 | |
| 11 | 11 | import { commitVerdicts, isCommitId, releaseCheck, type CommitCheck } from "../checks/commit_check.ts"; |
| 12 | 12 | import { readLooks } from "../checks/looks.ts"; |
| 13 | import { vitalsRows } from "../checks/summary.ts"; | |
| 13 | import { vitalsRows, type PageSize } from "../checks/summary.ts"; | |
| 14 | 14 | import { hasCommit, isAncestor } from "../git/local.ts"; |
| 15 | 15 | import { repoDir } from "../git/repos.ts"; |
| 16 | 16 | import { mayViewId } from "../core/sealed.ts"; |
| @@ -248,6 +248,8 @@ export interface LookFigures { | ||
| 248 | 248 | readonly look: string; |
| 249 | 249 | readonly screens: string; |
| 250 | 250 | readonly sideways: boolean; |
| 251 | /** The window and page as measured (`scrollWidth` vs `innerWidth` says whether it fits); absent when none was recorded. */ | |
| 252 | readonly size?: PageSize; | |
| 251 | 253 | } |
| 252 | 254 | |
| 253 | 255 | /** |
| @@ -262,7 +264,12 @@ export function looksOfCheck( | ||
| 262 | 264 | ): LookFigures[] { |
| 263 | 265 | if (!mayReadRuns(app, actor, repo)) return []; |
| 264 | 266 | const looks = readLooks(app.config.dataDir, { id: run.id, repoId: repo.id, commitOid: run.commitOid }); |
| 265 | return vitalsRows(looks).map((g) => ({ look: g.look, screens: g.screens, sideways: g.sideways })); | |
| 267 | return vitalsRows(looks).map((g) => ({ | |
| 268 | look: g.look, | |
| 269 | screens: g.screens, | |
| 270 | sideways: g.sideways, | |
| 271 | ...(g.size === undefined ? {} : { size: g.size }), | |
| 272 | })); | |
| 266 | 273 | } |
| 267 | 274 | |
| 268 | 275 | /** The own green or red verdicts of a page of commits, for whoever may read the runs; empty otherwise. */ |
src/checks/looks.ts +19 −1
| @@ -30,7 +30,8 @@ export const commitLooksPath = ( | ||
| 30 | 30 | const SAFE = /^[0-9A-Za-z]{1,64}$/; |
| 31 | 31 | |
| 32 | 32 | const MAX_BYTES = 1024 * 1024; |
| 33 | const MAX_LOOKS = 20; | |
| 33 | // The forge's own preview takes ~55 looks (`PREVIEW_OWNER_LOOKS`); every one must fit. | |
| 34 | const MAX_LOOKS = 64; | |
| 34 | 35 | const MAX_WORDS = 80; |
| 35 | 36 | |
| 36 | 37 | const words = (v: unknown): string | null => |
| @@ -117,3 +118,20 @@ export function writeCommitLooks( | ||
| 117 | 118 | writeFileSync(side, JSON.stringify(kept)); |
| 118 | 119 | renameSync(side, path); |
| 119 | 120 | } |
| 121 | ||
| 122 | /** | |
| 123 | * File one look under its commit as it is taken, keeping the others already | |
| 124 | * filed and replacing a look of the same name. A set written only when the | |
| 125 | * whole round ends is lost to any round that is interrupted (a newer head, | |
| 126 | * a restart) and, on the next round, covers only what was still missing. | |
| 127 | */ | |
| 128 | export function addCommitLook( | |
| 129 | dataDir: string, | |
| 130 | repoId: string, | |
| 131 | commitOid: string, | |
| 132 | look: LookVitals, | |
| 133 | ): void { | |
| 134 | if (!SAFE.test(commitOid) || !SAFE.test(repoId)) return; | |
| 135 | const kept = readFile(commitLooksPath(dataDir, repoId, commitOid)).filter((l) => l.look !== look.look); | |
| 136 | writeCommitLooks(dataDir, repoId, commitOid, [...kept, look]); | |
| 137 | } |
src/checks/summary.ts +3 −1
| @@ -243,6 +243,8 @@ export interface LookRows { | ||
| 243 | 243 | readonly screens: string; |
| 244 | 244 | /** A phone look whose page is wider than its window; false without a size. */ |
| 245 | 245 | readonly sideways: boolean; |
| 246 | /** The page and window as measured; absent without a size. */ | |
| 247 | readonly size?: PageSize; | |
| 246 | 248 | } |
| 247 | 249 | |
| 248 | 250 | const isPhone = (look: string): boolean => look === "phone" || look.startsWith("phone "); |
| @@ -284,6 +286,6 @@ export function vitalsRows(looks: readonly LookVitals[]): readonly LookRows[] { | ||
| 284 | 286 | const tall = z === undefined || z.innerHeight <= 0 ? null : z.scrollHeight / z.innerHeight; |
| 285 | 287 | const screens = tall === null ? "" : (Math.round(tall * 10) / 10).toFixed(1); |
| 286 | 288 | const sideways = z !== undefined && isPhone(l.look) && z.scrollWidth > z.innerWidth; |
| 287 | return { look: l.look, skipped: "", rows, screens, sideways }; | |
| 289 | return { look: l.look, skipped: "", rows, screens, sideways, ...(z === undefined ? {} : { size: z }) }; | |
| 288 | 290 | }); |
| 289 | 291 | } |
src/looks/service.ts +15 −16
| @@ -67,7 +67,7 @@ import { thumbnail } from "../design/image/image.ts"; | ||
| 67 | 67 | import { decodePng, encodePng, pngHeader } from "../design/image/png.ts"; |
| 68 | 68 | import { createLog } from "../kernel/log.ts"; |
| 69 | 69 | import { err, ok, type Result } from "../kernel/result.ts"; |
| 70 | import { writeCommitLooks } from "../checks/looks.ts"; | |
| 70 | import { addCommitLook } from "../checks/looks.ts"; | |
| 71 | 71 | import type { LookVitals } from "../checks/summary.ts"; |
| 72 | 72 | import { keepAsMain } from "./main_looks.ts"; |
| 73 | 73 | import { findChromium, isNoBrowser, takeLook, type Capture } from "./capture.ts"; |
| @@ -408,8 +408,6 @@ export function startLooks(app: App, options: LooksOptions): LooksHandle { | ||
| 408 | 408 | : { id: owner.id, name: owner.username, role: "admin" }; |
| 409 | 409 | const host = runtime.hostFor(record.slug, item.env); |
| 410 | 410 | let allOk = true; |
| 411 | // What each look's page recorded, for the check summary's vitals table. | |
| 412 | const vitals: LookVitals[] = []; | |
| 413 | 411 | for (const { page, kind, theme } of wanted) { |
| 414 | 412 | if (stopped) return; |
| 415 | 413 | const token = signGrant( |
| @@ -439,13 +437,22 @@ export function startLooks(app: App, options: LooksOptions): LooksHandle { | ||
| 439 | 437 | timeoutMs: CAPTURE_TIMEOUT_MS, |
| 440 | 438 | }); |
| 441 | 439 | // The thumb is the desktop page again, as a stranger: it adds no row. |
| 440 | // What the page recorded is filed under the commit at once, for the | |
| 441 | // check summary's vitals table: a round that ends early keeps it. | |
| 442 | 442 | if (kind !== "thumb" && theme === "light") { |
| 443 | 443 | const look = page === "/" ? kind : `${kind} ${page}`; |
| 444 | if (shot.ok) { | |
| 445 | const size = shot.value.size === undefined ? {} : { size: shot.value.size }; | |
| 446 | vitals.push({ look, entries: shot.value.entries, ...size }); | |
| 447 | } else if (isNoBrowser(shot.error)) | |
| 448 | vitals.push({ look, skipped: "no browser" }); | |
| 444 | const filed: LookVitals | undefined = shot.ok | |
| 445 | ? { look, entries: shot.value.entries, ...(shot.value.size === undefined ? {} : { size: shot.value.size }) } | |
| 446 | : isNoBrowser(shot.error) | |
| 447 | ? { look, skipped: "no browser" } | |
| 448 | : undefined; | |
| 449 | if (filed !== undefined) { | |
| 450 | try { | |
| 451 | addCommitLook(dataDir, record.repoId, item.commitOid, filed); | |
| 452 | } catch (error) { | |
| 453 | log.warn("vitals not recorded", { commit: item.commitOid, error: String(error) }); | |
| 454 | } | |
| 455 | } | |
| 449 | 456 | } |
| 450 | 457 | const taken = |
| 451 | 458 | shot.ok && kind === "thumb" ? reduceToThumb(staged) : shot; |
| @@ -491,14 +498,6 @@ export function startLooks(app: App, options: LooksOptions): LooksHandle { | ||
| 491 | 498 | recordLook(app.db, { ...row, kind, path: "", error: taken.error }); |
| 492 | 499 | } |
| 493 | 500 | } |
| 494 | // Filed under the commit: the check of it shows them whenever it exists. | |
| 495 | if (vitals.length > 0) { | |
| 496 | try { | |
| 497 | writeCommitLooks(dataDir, record.repoId, item.commitOid, vitals); | |
| 498 | } catch (error) { | |
| 499 | log.warn("vitals not recorded", { commit: item.commitOid, error: String(error) }); | |
| 500 | } | |
| 501 | } | |
| 502 | 501 | // A full set at this head is the environment's look now; the pictures |
| 503 | 502 | // of heads before it have nothing left to say. |
| 504 | 503 | if (allOk) |
test/check-looks-api.test.ts +2 −2
| @@ -93,7 +93,7 @@ test("the commit's check carries screens and sideways; [] without a file", async | ||
| 93 | 93 | const body = await get(`/api/v1/repos/alice/r/commits/${OID}/check`); |
| 94 | 94 | expect(body.runId).toBe(run.id); |
| 95 | 95 | expect(body.looks).toEqual([ |
| 96 | { look: "phone /x", screens: "2.5", sideways: true }, | |
| 96 | { look: "phone /x", screens: "2.5", sideways: true, size: { scrollWidth: 500, scrollHeight: 2000, innerWidth: 390, innerHeight: 800 } }, | |
| 97 | 97 | { look: "desktop", screens: "", sideways: false }, |
| 98 | 98 | ]); |
| 99 | 99 | }); |
| @@ -107,7 +107,7 @@ test("the pull's checks carry the same looks beside run", async () => { | ||
| 107 | 107 | const body = await get(`/api/v1/repos/alice/r/pulls/${number}/checks`); |
| 108 | 108 | expect(body.run.id).toBe(run.id); |
| 109 | 109 | expect(body.looks).toEqual([ |
| 110 | { look: "phone /x", screens: "1.3", sideways: false }, | |
| 110 | { look: "phone /x", screens: "1.3", sideways: false, size: { scrollWidth: 390, scrollHeight: 1000, innerWidth: 390, innerHeight: 800 } }, | |
| 111 | 111 | { look: "desktop", screens: "", sideways: false }, |
| 112 | 112 | ]); |
| 113 | 113 | }); |
test/check-looks-filed.test.ts added +37
| @@ -0,0 +1,37 @@ | ||
| 1 | /** A commit's looks are filed as each is taken, so an interrupted round loses none (#10336). */ | |
| 2 | ||
| 3 | import { afterAll, expect, test } from "bun:test"; | |
| 4 | import { mkdtemp, rm } from "node:fs/promises"; | |
| 5 | import { tmpdir } from "node:os"; | |
| 6 | import { join } from "node:path"; | |
| 7 | import { addCommitLook, commitLooksPath, readLooks } from "../src/checks/looks.ts"; | |
| 8 | import { vitalsRows } from "../src/checks/summary.ts"; | |
| 9 | import { PREVIEW_MOVE_LOOK, PREVIEW_OWNER_LOOKS } from "../src/spawn/preview-looks.ts"; | |
| 10 | ||
| 11 | const dir = await mkdtemp(join(tmpdir(), "forge-looks-filed-")); | |
| 12 | afterAll(() => rm(dir, { recursive: true, force: true })); | |
| 13 | ||
| 14 | const OID = "a".repeat(40); | |
| 15 | const run = { id: "r1", repoId: "repo1", commitOid: OID }; | |
| 16 | const size = { scrollWidth: 390, scrollHeight: 900, innerWidth: 390, innerHeight: 844 }; | |
| 17 | ||
| 18 | test("looks filed in separate rounds are all there, and a retaken one replaces its earlier self", () => { | |
| 19 | addCommitLook(dir, "repo1", OID, { look: "phone /a", entries: {} }); | |
| 20 | // A later round (the first was cut short) files only what it takes. | |
| 21 | addCommitLook(dir, "repo1", OID, { look: `phone ${PREVIEW_MOVE_LOOK}`, entries: {}, size }); | |
| 22 | addCommitLook(dir, "repo1", OID, { look: "phone /a", skipped: "no browser" }); | |
| 23 | const read = readLooks(dir, run); | |
| 24 | expect(read.map((l) => l.look).sort()).toEqual(["phone /a", `phone ${PREVIEW_MOVE_LOOK}`]); | |
| 25 | const move = vitalsRows(read).find((g) => g.look === `phone ${PREVIEW_MOVE_LOOK}`); | |
| 26 | expect(move?.size).toEqual(size); | |
| 27 | expect(commitLooksPath(dir, "repo1", OID)).toContain(OID); | |
| 28 | }); | |
| 29 | ||
| 30 | test("every look of the forge's own preview fits in the set", () => { | |
| 31 | const all = PREVIEW_OWNER_LOOKS.filter((l) => l.kind !== "thumb"); | |
| 32 | for (const l of all) | |
| 33 | addCommitLook(dir, "repo2", OID, { look: `${l.kind} ${l.page}`, entries: {}, size }); | |
| 34 | const read = readLooks(dir, { id: "r2", repoId: "repo2", commitOid: OID }); | |
| 35 | expect(read).toHaveLength(all.length); | |
| 36 | expect(read.some((l) => l.look === `phone ${PREVIEW_MOVE_LOOK}`)).toBe(true); | |
| 37 | }); |
test/check-looks-held.test.ts +1
| @@ -89,6 +89,7 @@ test("a forge preview's check lists the held page at desktop and phone with scre | ||
| 89 | 89 | look: `${kind} ${PREVIEW_HELD_PAGE}`, |
| 90 | 90 | screens: "2.5", |
| 91 | 91 | sideways: false, |
| 92 | size, | |
| 92 | 93 | }); |
| 93 | 94 | } |
| 94 | 95 | }); |
Pairing looks[i] with the vitalsRows row by index is fragile. Put
sizeon the LookRows that vitalsRows already builds froml.size, and read it from there.