diff --git a/.changeset/pixel-review-fixes.md b/.changeset/pixel-review-fixes.md new file mode 100644 index 00000000..7c085ee9 --- /dev/null +++ b/.changeset/pixel-review-fixes.md @@ -0,0 +1,12 @@ +--- +"@typeonce/ritmo-tools": patch +--- + +`pixel review` and `pixel notes` fixes, from trying them on real games: + +- The page keeps saving after the agent renames or replaces art it holds a review of (`pixel fix` does, putting `X.fixed.png` in place of `X.png`). Saves used to fail until a reload, which lost what was unsaved. The old review stays in `pixel.review.json` under its old name, and `pixel notes` lists it under `gone`. +- A page that has not heard an asset came back can no longer write over that asset's review. +- A comment being typed survives a click on a status or a reply from the agent, and is added when the person moves to another asset, animation, or view. Cancel or Esc drops it. +- `pixel notes` no longer says "Leave it as it is" about an approved asset with open comments. An asset with a note or comments but no status comes ahead of untouched ones, and the summary counts it apart. +- The server answers while it reads a large folder, and the page says how far it is. Past 500 assets, `pixel review` and `pixel notes` warn to name narrower folders. The page gets a picture's file as it is, rather than a copy written for it up front, so a large folder takes about a third less time and far less memory. +- Holding an arrow key down in Safari no longer stops the page moving through assets. diff --git a/packages/tools/README.md b/packages/tools/README.md index 84be5516..f750cef0 100644 --- a/packages/tools/README.md +++ b/packages/tools/README.md @@ -436,7 +436,8 @@ version; the status stays approved, since only the person changes it. ↑ and ↓ change asset, Space plays, ← and → step frames, + and − zoom, 0 goes back to 1×, F fits each asset again, V switches to the whole set and back, and Ctrl/Cmd+Z undoes. -Every change is saved as it happens. Art the agent rewrites or adds, and +Every change is saved as it happens, and a comment still being typed is added +when the person moves on (Cancel or Esc drops it). Art the agent rewrites or adds, and comments it resolves, show up while the page is open; what the person is typing is kept. diff --git a/packages/tools/src/pixel/cli/commands.ts b/packages/tools/src/pixel/cli/commands.ts index a18ea5af..d15583cf 100644 --- a/packages/tools/src/pixel/cli/commands.ts +++ b/packages/tools/src/pixel/cli/commands.ts @@ -12,7 +12,7 @@ * art's rules: they exit 0, or 2 on a usage problem. Their lint results are * warnings to read, never a failure. */ -import { Data, Effect, FileSystem, Option, Path, Runtime } from "effect" +import { Console, Data, Effect, FileSystem, Option, Path, Runtime } from "effect" import { Argument, Command, Flag } from "effect/cli" import type { Metric } from "../core/color.ts" import { toHex } from "../core/color.ts" @@ -581,6 +581,12 @@ const assetsUnder = Effect.fn("assetsUnder")(function* (paths: ReadonlyArray findAssets(paths)) }) +/** Past this many a review takes minutes to read, and a person cannot go through it: most likely the paths hold much that is not art. */ +const MANY_ASSETS = 500 + +const tooMany = (found: ReadonlyArray): string | undefined => + found.length <= MANY_ASSETS ? undefined : `${found.length} assets under these paths, which take minutes to read. Name the folders the game's art is in (\`public/art\`, \`assets/sprites\`), not ones holding renders or captures.` + const review = Command.make( "review", { @@ -591,12 +597,14 @@ const review = Command.make( Effect.fn("review")(function* (flags) { const path = yield* Path.Path const found = yield* assetsUnder(flags.paths) + const warning = tooMany(found) // A broken review file is said now, not when the page cannot save. yield* promised("cannot read the review", () => reviewsOf(found)) const address = yield* promised("cannot serve the page", () => serve({ paths: flags.paths, port: flags.port, host: flags.host })) yield* printJson({ ...address, assets: found.length, + warning, reviews: [...new Set(found.map((asset) => path.relative(process.cwd(), reviewPlace(asset.file).reviews)))], next: "Send the person the url. What they approve and comment is saved as they go; read it back with `ritmo-tools pixel notes`, with the same paths, and `pixel resolve` the comments you act on." }) @@ -614,6 +622,9 @@ const notes = Command.make( Effect.fn("notes")(function* (flags) { const path = yield* Path.Path const found = yield* assetsUnder(flags.paths) + // Said before the reading, since it takes minutes: an agent that gives up on them still sees why. + const warning = tooMany(found) + if (warning !== undefined) yield* Console.error(`ritmo-tools: ${warning}`) const reviews = yield* promised("cannot read the review", () => reviewsOf(found)) const entries = [] for (const asset of found) { diff --git a/packages/tools/src/pixel/cli/library.ts b/packages/tools/src/pixel/cli/library.ts index 91be4ab9..f9a3c92c 100644 --- a/packages/tools/src/pixel/cli/library.ts +++ b/packages/tools/src/pixel/cli/library.ts @@ -119,8 +119,6 @@ export type AssetRead = /** Its size: the pixels are not kept, so a game's worth of reads stays small. */ readonly width: number readonly height: number - /** The picture as the page draws it: decoded and written again, so it shows exactly the pixels hashed (a text sprite, rendered). */ - readonly png: Uint8Array readonly frames: ReadonlyArray readonly animations: ReadonlyArray /** The sheet's JSON, when it is a sheet. */ @@ -208,7 +206,7 @@ const read = async (asset: PixelAsset): Promise => { if (!parsed.ok) return { ok: false, problem: parsed.problems.map((problem) => (problem.line === undefined ? problem.message : `line ${problem.line}: ${problem.message}`)).join("; ") } const image = toRgba(parsed.art) const frames: ReadonlyArray = [{ name: "", index: 0, x: 0, y: 0, w: image.width, h: image.height, hash: hashFrame(image, 0, 0, image.width, image.height) }] - return { ok: true, width: image.width, height: image.height, png: encodePng(image), frames, animations: [], sheet: undefined, lint: await lintOf(asset, image, frames, parsed.art) } + return { ok: true, width: image.width, height: image.height, frames, animations: [], sheet: undefined, lint: await lintOf(asset, image, frames, parsed.art) } } const bytes = await readFile(asset.file) let image: RgbaImage @@ -231,23 +229,50 @@ const read = async (asset: PixelAsset): Promise => { const animation = animations.find((entry) => entry.frames.includes(i)) return { name: rect.name, animation: animation?.name ?? "frames", index: animation?.frames.indexOf(i) ?? i, x: rect.x, y: rect.y, w: rect.w, h: rect.h, duration: rect.duration, hash: hashFrame(image, rect.x, rect.y, rect.w, rect.h) } }) - return { ok: true, width: image.width, height: image.height, png: encodePng(image), frames, animations, sheet: sheet === undefined ? undefined : sheetFile, lint: await lintOf(asset, image, frames, undefined) } + return { ok: true, width: image.width, height: image.height, frames, animations, sheet: sheet === undefined ? undefined : sheetFile, lint: await lintOf(asset, image, frames, undefined) } } catch (error) { return { ok: false, problem: message(error) } } } +let turns: Promise = Promise.resolve() + +/** One read at a time, letting the server answer between them: decoding and linting a picture block it, and a large folder holds thousands. */ +const inTurn = (run: () => Promise): Promise => { + const turn = turns.then(() => new Promise((next) => setImmediate(next))).then(run) + turns = turn.catch(() => undefined) + return turn +} + /** Each asset's read, by its version: the page's poll reads nothing again until a file changes, and two polls at once share one read. */ -const reads = new Map }>() +const reads = new Map; done: boolean }>() export const readAsset = async (asset: PixelAsset): Promise => { const version = await assetVersion(asset) const known = reads.get(asset.file) if (known?.version === version) return known.read - const fresh = read(asset) - reads.set(asset.file, { version, read: fresh }) + const fresh = inTurn(() => read(asset)) + const entry = { version, read: fresh, done: false } + reads.set(asset.file, entry) + void fresh.finally(() => { + entry.done = true + }) return fresh } +/** + * The picture the page draws: a PNG or JPEG file as it is, which a browser + * reads the way the game's does, and a text sprite rendered. Made when asked + * for and not kept: a page opening the whole set asks for them all, and its + * saves wait behind them for a connection if each takes long. + */ +export const pictureOf = async (asset: PixelAsset): Promise<{ readonly bytes: Uint8Array; readonly type: string }> => { + if (asset.kind === "picture") return { bytes: await readFile(asset.file), type: /\.png$/i.test(asset.file) ? "image/png" : "image/jpeg" } + const text = await readFile(asset.file, "utf8") + const parsed = /\.json$/i.test(asset.file) ? parseJsonMap(text) : parseSprite(text) + if (!parsed.ok) throw new Error(`${asset.id} no longer reads as a sprite`) + return { bytes: encodePng(toRgba(parsed.art)), type: "image/png" } +} + // --------------------------------------------------------------------------- // The review file @@ -278,10 +303,19 @@ const filesOf = (assets: ReadonlyArray): Map): Promise => { const parts: Array = [] - for (const reviews of [...filesOf(assets).keys()].sort()) parts.push(reviews, (await textOf(reviews)) ?? "") + for (const [reviews, inside] of [...filesOf(assets).entries()].sort(([a], [b]) => a.localeCompare(b))) { + const text = await textOf(reviews) + const file = text === null ? ({ ok: true, value: emptyReview } as const) : decodeReview(text) + parts.push(reviews, file.ok ? JSON.stringify(inside.map((asset) => [asset.id, file.value.assets[reviewPlace(asset.file).key] ?? null])) : (text ?? "")) + } return hashText(parts.join("\u0000")) } @@ -372,6 +406,12 @@ export const entryOf = async (asset: PixelAsset): Promise => { /** The page's list, read again only where files changed. */ export const libraryOf = (assets: ReadonlyArray): Promise> => Promise.all(assets.map(entryOf)) +/** How many of the assets have been read so far: what a page waiting for its list shows. */ +export const readingOf = (assets: ReadonlyArray): { readonly done: number; readonly total: number } => ({ + done: assets.filter((asset) => reads.get(asset.file)?.done === true).length, + total: assets.length +}) + /** What the page shows beside one asset: its lint issues, and the files it comes from. */ export const detailOf = async (asset: PixelAsset): Promise => { const got = await readAsset(asset) diff --git a/packages/tools/src/pixel/cli/serve.ts b/packages/tools/src/pixel/cli/serve.ts index 509fcc04..eb2247d0 100644 --- a/packages/tools/src/pixel/cli/serve.ts +++ b/packages/tools/src/pixel/cli/serve.ts @@ -15,7 +15,7 @@ import { fileURLToPath } from "node:url" import { hashText } from "../../internal/hash.ts" import { body, send, servePage, type Address, type Handler } from "../../internal/serve.ts" import { decodeReview, encodeReview } from "../core/review.ts" -import { assetVersion, detailOf, findAssets, libraryOf, readAsset, reviewsOf, reviewVersion, writeReviews, type PixelAsset } from "./library.ts" +import { assetVersion, detailOf, findAssets, libraryOf, pictureOf, readAsset, readingOf, reviewsOf, reviewVersion, writeReviews, type PixelAsset } from "./library.ts" export interface ServeOptions { /** The pictures, text sprites, and folders to review. */ @@ -24,8 +24,11 @@ export interface ServeOptions { readonly host: boolean } +/** How long a look at the list waits before saying how far the reading is: a browser may give up on an answer that takes minutes. */ +const LIST_WAIT_MS = 1000 + /** The API over the assets under `paths`. */ -export const api = (paths: ReadonlyArray): Handler => { +export const api = (paths: ReadonlyArray, listWait = LIST_WAIT_MS): Handler => { const named = (request: IncomingMessage, assets: ReadonlyArray): PixelAsset | undefined => { const id = new URL(request.url ?? "/", "http://localhost").searchParams.get("id") return assets.find((asset) => asset.id === id) @@ -37,7 +40,12 @@ export const api = (paths: ReadonlyArray): Handler => { const route = `${request.method ?? "GET"} ${path}` const assets = await findAssets(paths) if (route === "GET /library") { - const entries = await libraryOf(assets) + const listing = libraryOf(assets) + const listed = await Promise.race([listing, new Promise((done) => setTimeout(done, listWait))]) + // The reading goes on behind this answer, and the next look finds it further along. Once it is all read, the list comes however long its files take to check: a page must not wait for a look that always runs out of time. + const reading = readingOf(assets) + if (listed === undefined && reading.done < reading.total) return send(response, 200, { version: "", review: "", reading }) + const entries = listed ?? (await listing) const version = hashText(JSON.stringify(entries.map((entry) => [entry.id, entry.version]))) const review = await reviewVersion(assets) // Most looks change nothing: then only the versions travel. @@ -47,13 +55,13 @@ export const api = (paths: ReadonlyArray): Handler => { if (route === "PUT /review") { const decoded = decodeReview((await body(request)).toString("utf8")) if (!decoded.ok) return send(response, 400, { error: decoded.error }) - const unknown = Object.keys(decoded.value.assets).filter((id) => !assets.some((asset) => asset.id === id)) - if (unknown.length > 0) return send(response, 400, { error: `no such asset: ${unknown.join(", ")}` }) // Written since the page last read it (the agent resolved a comment): the page merges and tries again. const now = await reviewVersion(assets) if (query(request, "base") !== now) return send(response, 409, { error: "the review changed since the page read it", text: encodeReview(await reviewsOf(assets)), version: now }) await writeReviews(assets, decoded.value) - return send(response, 200, { version: await reviewVersion(assets) }) + // Only what was found is written. An id that is not (the agent renamed that file while the page was open) is said back, and its review stays in the file under its old name, for `pixel notes` to list. + const ignored = Object.keys(decoded.value.assets).filter((id) => !assets.some((asset) => asset.id === id)) + return send(response, 200, { version: await reviewVersion(assets), ignored }) } const asset = named(request, assets) if (asset === undefined) return send(response, 404, { error: `no such asset for ${route}` }) @@ -61,10 +69,11 @@ export const api = (paths: ReadonlyArray): Handler => { if (route === "GET /png") { const got = await readAsset(asset) if (!got.ok) return send(response, 422, { error: got.problem }) - response.setHeader("content-type", "image/png") + const picture = await pictureOf(asset) + response.setHeader("content-type", picture.type) // The page asks by version (`v`): a new version is a new address, so the picture at one can be kept. response.setHeader("cache-control", query(request, "v") === (await assetVersion(asset)) ? "private, max-age=31536000, immutable" : "no-store") - response.end(got.png) + response.end(picture.bytes) return } return send(response, 404, { error: `no ${route}` }) diff --git a/packages/tools/src/pixel/core/review.ts b/packages/tools/src/pixel/core/review.ts index 31b24fde..7f3cd0d5 100644 --- a/packages/tools/src/pixel/core/review.ts +++ b/packages/tools/src/pixel/core/review.ts @@ -318,6 +318,7 @@ export const reviewSummary = (review: AssetReview, frames: ReadonlyArray // Open comments only, each with its number: resolved ones are done. const open = review.comments.flatMap((comment, index) => (comment.resolved === true ? [] : [{ number: index + 1, where: whereOf(comment), ...comment }])) const resolved = review.comments.length - open.length + const worded = open.length > 0 || review.note !== "" return { status: review.status, note: review.note === "" ? undefined : review.note, @@ -326,30 +327,43 @@ export const reviewSummary = (review: AssetReview, frames: ReadonlyArray changedSinceApproved: changed, next: changed !== undefined - ? "It changed since the person approved it. Tell them what changed and ask them to look again; only they approve it." - : review.status === "approved" - ? "The person approved it. Leave it as it is." - : open.length > 0 || review.note !== "" - ? "Act on the note and each comment where it points, then `pixel resolve` the comments with a reply saying what changed, and tell the person." - : review.status === "changes" - ? "The person asked for changes but said nothing yet: ask them what." - : "Nothing said yet. Wait for the person, or ask them to look." + ? `It changed since the person approved it. Tell them what changed and ask them to look again; only they approve it.${open.length > 0 ? " Then `pixel resolve` the comments you acted on, with a reply saying what changed." : ""}` + : review.status === "approved" && open.length > 0 + ? "The person approved it and left comments too. Read each: change only what it asks for, `pixel resolve` it with a reply saying what changed, and if the art changed, ask them to look again." + : review.status === "approved" && worded + ? "The person approved it and left a note. Read it, and leave the art as it is unless the note asks for a change." + : review.status === "approved" + ? "The person approved it. Leave it as it is." + : worded + ? "Act on the note and each comment where it points, then `pixel resolve` the comments with a reply saying what changed, and tell the person." + : review.status === "changes" + ? "The person asked for changes but said nothing yet: ask them what." + : "Nothing said yet. Wait for the person, or ask them to look." } } interface Summarized { readonly status: Status + readonly note?: string | undefined + /** The comments still open. */ + readonly comments?: ReadonlyArray | undefined readonly changedSinceApproved?: string | undefined } -const urgency = (entry: Summarized): number => (entry.status === "changes" ? 0 : entry.changedSinceApproved !== undefined ? 1 : entry.status === "new" ? 2 : 3) +/** Whether the person said something about it without asking for changes: a note or comments, with no status set; or comments on what they approved. */ +const leftWords = (entry: Summarized): boolean => entry.status !== "changes" && (entry.comments !== undefined || (entry.status === "new" && entry.note !== undefined)) -/** What is left to do across the assets, with them in the order to act on them: changes asked, changed since approved, not reviewed yet, approved. */ +const urgency = (entry: Summarized): number => (entry.status === "changes" || leftWords(entry) ? 0 : entry.changedSinceApproved !== undefined ? 1 : entry.status === "new" ? 2 : 3) + +/** What is left to do across the assets, with them in the order to act on them: changes asked or words left, changed since approved, not reviewed yet, approved. */ export const pixelSummary = (entries: ReadonlyArray) => { const total = (status: Status): number => entries.filter((entry) => entry.status === status).length const stale = entries.filter((entry) => entry.changedSinceApproved !== undefined).length + const approvedWords = entries.filter((entry) => entry.status === "approved" && leftWords(entry)).length + const newWords = entries.filter((entry) => entry.status === "new" && leftWords(entry)).length + const about = [...(stale === 0 ? [] : [`${stale} changed since: ask the person to look again`]), ...(approvedWords === 0 ? [] : [`${approvedWords} with open comments: read them`])] return { - summary: `${total("approved")} approved${stale === 0 ? "" : ` (${stale} changed since: ask the person to look again)`}, ${total("changes")} with changes asked, ${total("new")} not reviewed yet (of ${entries.length}).`, + summary: `${total("approved")} approved${about.length === 0 ? "" : ` (${about.join("; ")})`}, ${total("changes")} with changes asked, ${newWords === 0 ? "" : `${newWords} with a note or comments but no status set: act on them, `}${total("new") - newWords} not reviewed yet (of ${entries.length}).`, assets: [...entries].sort((a, b) => urgency(a) - urgency(b)) } } diff --git a/packages/tools/src/pixel/review/lab.ts b/packages/tools/src/pixel/review/lab.ts index fa97d245..6126ad06 100644 --- a/packages/tools/src/pixel/review/lab.ts +++ b/packages/tools/src/pixel/review/lab.ts @@ -21,7 +21,7 @@ import { button, callout, group, hint, icon, mark, prop, section, segmented, swi import { h, replace } from "../ui/dom.ts" import { makeHistory } from "../ui/history.ts" import { parseHex } from "../core/color.ts" -import { approvalOf, changedOf, decodeReview, emptyReview, encodeReview, mergeReviews, newAsset, whereOf, type AssetReview, type Comment, type Status } from "../core/review.ts" +import { approvalOf, changedOf, decodeReview, emptyReview, encodeReview, mergeReviews, newAsset, whereOf, type AssetReview, type Comment, type Review, type Status } from "../core/review.ts" import { SERVER_GONE, type Detail, type Entry, type FrameIssue, type Store } from "./store.ts" const RITMO_URL = "https://ritmo.typeonce.dev/" @@ -85,11 +85,15 @@ const state = { frame: 0, /** A comment being written, on the area the person drew. */ composing: undefined as Area | undefined, + /** What it says so far: kept here, so a redraw of the panel never loses it. */ + composingText: "", /** Pixels ringed from the Rules section: where a lint issue is. */ marks: undefined as { readonly frame: number; readonly at: ReadonlyArray } | undefined, saving: "saved" as "saved" | "saving" | "failed", /** Whether the review server answered the last look. */ online: true, + /** How far the server is, while it reads the art for the first time. */ + reading: undefined as { readonly done: number; readonly total: number } | undefined, loaded: false } @@ -111,6 +115,8 @@ let dirty = false let saveTimer: ReturnType | undefined let flushing: Promise = Promise.resolve() +const without = (review: Review, ids: ReadonlyArray): Review => ({ assets: Object.fromEntries(Object.entries(review.assets).filter(([id]) => !ids.includes(id))) }) + const flush = (): Promise => { flushing = flushing.then(write) return flushing @@ -130,7 +136,11 @@ const write = async (): Promise => { owed = false state.reviewVersion = saved.version const sent = decodeReview(text) - if (sent.ok) state.base = sent.value + if (sent.ok) state.base = without(sent.value, saved.ignored) + if (saved.ignored.length > 0) { + state.review = without(state.review, saved.ignored) + say(`${saved.ignored.join(", ")} is gone: renamed or deleted. What was said about it stays in pixel.review.json under that name, and the agent sees it in \`pixel notes\`.`) + } continue } // The agent wrote the review meanwhile (a resolved comment): keep both, and try again. @@ -183,6 +193,35 @@ const reviewChanged = (): void => { reviewShown() } +/** Adds the comment being written, when it says anything, to the asset open, and ends it. */ +const addComposed = (): void => { + const area = state.composing + const text = state.composingText.trim() + state.composing = undefined + state.composingText = "" + if (area === undefined || text === "") return + const id = state.current + setReview(id, { + comments: [ + ...reviewOf(id).comments, + { ...(area.animation === undefined ? {} : { animation: area.animation }), frame: area.frame, x: area.x, y: area.y, ...(area.width > 1 ? { width: area.width } : {}), ...(area.height > 1 ? { height: area.height } : {}), text } + ] + }) + reviewChanged() +} + +/** Moving on keeps what was typed: everything else the person says is saved as they go. Only Cancel or Esc drops it. */ +const settleComposed = (): void => { + const added = state.composing !== undefined && state.composingText.trim() !== "" + addComposed() + if (added) say("Your comment is added. Delete it there if you were not done with it.") +} + +const dropComposed = (): void => { + state.composing = undefined + state.composingText = "" +} + /** Sets an asset's status; approving keeps what it looks like now, so a later change to it shows. */ const setStatus = (entry: Entry, status: Status): void => { setReview(entry.id, { status, approved: status === "approved" && entry.problem === undefined ? approvalOf(entry.frames) : undefined }) @@ -210,6 +249,13 @@ const pictureOf = (entry: Entry): Promise => { } let opening = 0 +let addressTimer: ReturnType | undefined + +/** The address names the asset open, once the person stops on one: Safari refuses more than 100 changes of it in 10 seconds, and an arrow key held down gets there. */ +const showInAddress = (id: string): void => { + if (addressTimer !== undefined) clearTimeout(addressTimer) + addressTimer = setTimeout(() => window.history.replaceState(null, "", `#${encodeURIComponent(id)}`), 250) +} /** Opens an asset. Nothing is owed first: the review is one file, saved whole. */ const open = async (id: string): Promise => { @@ -219,16 +265,16 @@ const open = async (id: string): Promise => { if (state.view === "set") setView("asset") return } + settleComposed() state.current = id state.view = "asset" state.animation = animationsOf(entry)[0]!.name state.frame = 0 state.playing = true - state.composing = undefined state.marks = undefined state.detail = undefined clock = performance.now() - window.history.replaceState(null, "", `#${encodeURIComponent(id)}`) + showInAddress(id) update() scrollToRow(id) void loadDetail(id) @@ -253,7 +299,16 @@ const reviewShown = (): void => { } const load = async (): Promise => { - const listed = await store.library("") + let listed = await store.library("") + // A folder of thousands takes minutes to read: say how far it is, and look again. + while (listed?.reading !== undefined) { + state.reading = listed.reading + renderRail() + renderStage() + await new Promise((next) => setTimeout(next, 500)) + listed = await store.library("") + } + state.reading = undefined if (listed?.entries === undefined) return say("The review server sent no art.") state.library = listed.entries state.libraryVersion = listed.version @@ -295,7 +350,7 @@ const follow = async (): Promise => { renderTopbar() if (online && state.saving === "failed") void flush() } - if (listed === undefined) return + if (listed === undefined || listed.reading !== undefined) return if (listed.entries !== undefined) { const before = entryOf(state.current)?.version state.library = listed.entries @@ -320,12 +375,15 @@ const follow = async (): Promise => { if (busy() || answer.version === state.reviewVersion) return const review = decodeReview(answer.text) if (!review.ok) return + // An asset going away changes the version too, with nothing new to read on the ones still here. + const onList = (of: Review): string => encodeReview(without(of, Object.keys(of.assets).filter((id) => entryOf(id) === undefined))) + const news = onList(review.value) !== onList(state.review) state.review = review.value state.base = review.value state.reviewVersion = answer.version state.reviewProblem = undefined history.schedule() - say("The review changed in its file (the agent's replies show on the comments).") + if (news) say("The review changed in its file (the agent's replies show on the comments).") reviewShown() } catch { // The CLI was stopped; the page keeps what it has. @@ -617,8 +675,8 @@ const visible = (): ReadonlyArray => { const setView = (view: View): void => { if (state.view === view) return + settleComposed() state.view = view - state.composing = undefined update() } @@ -727,6 +785,8 @@ const flags = (entry: Entry): Array => { ] } +const readingWords = (): string => (state.reading === undefined ? "Reading the art…" : `Reading the art… ${state.reading.done} of ${state.reading.total}`) + const renderRail = (): void => { const scroll = rows.scrollTop const entries = visible() @@ -734,7 +794,7 @@ const renderRail = (): void => { let folder: string | undefined replace( rows, - entries.length === 0 && h("p", { class: "hint empty-list" }, state.loaded ? "Nothing here." : "Reading the art…"), + entries.length === 0 && h("p", { class: "hint empty-list" }, state.loaded ? "Nothing here." : readingWords()), entries.flatMap((entry) => { const heading = folders.size > 1 && entry.folder !== folder ? h("h3", { class: "pixel-folder", title: entry.folder }, entry.folder) : false folder = entry.folder @@ -813,7 +873,7 @@ const renderStage = (): void => { if (state.view === "set") return renderSet() const entry = entryOf(state.current) if (entry === undefined) { - replace(content, h("div", { class: "empty-card lab-empty" }, h("div", { class: "empty-icon" }, mark()), h("h2", {}, state.loaded ? "No art here" : "Reading the art…"))) + replace(content, h("div", { class: "empty-card lab-empty" }, h("div", { class: "empty-icon" }, mark()), h("h2", {}, state.loaded ? "No art here" : readingWords()))) renderPlayback() return } @@ -858,10 +918,10 @@ const renderStage = (): void => { attrs: { type: "button", "aria-pressed": String(animation.name === animationOf(entry).name) }, on: { click: () => { + settleComposed() state.animation = animation.name state.frame = 0 clock = performance.now() - state.composing = undefined state.marks = undefined renderStage() renderComments() @@ -1242,17 +1302,7 @@ const renderComments = (): void => { reviewChanged() } const add = (): void => { - const text = commentsBox.querySelector(".pixel-compose textarea")?.value.trim() ?? "" - if (text === "" || state.composing === undefined) return - const area = state.composing - setReview(id, { - comments: [ - ...reviewOf(id).comments, - { ...(area.animation === undefined ? {} : { animation: area.animation }), frame: area.frame, x: area.x, y: area.y, ...(area.width > 1 ? { width: area.width } : {}), ...(area.height > 1 ? { height: area.height } : {}), text } - ] - }) - state.composing = undefined - reviewChanged() + if (state.composingText.trim() !== "") addComposed() } redraw( commentsBox, @@ -1273,13 +1323,16 @@ const renderComments = (): void => { "label", { class: "input note" }, h("textarea", { - props: { rows: 2 }, + props: { rows: 2, value: state.composingText }, attrs: { placeholder: "What should change here? (\"outline too thick\", \"reads as a smudge at 1×\")", "aria-label": "Comment" }, on: { + input: (event) => { + if (event.currentTarget instanceof HTMLTextAreaElement) state.composingText = event.currentTarget.value + }, keydown: (event) => { if (event.key === "Enter" && (event.metaKey || event.ctrlKey)) add() if (event.key === "Escape") { - state.composing = undefined + dropComposed() renderComments() renderRings() } @@ -1291,7 +1344,7 @@ const renderComments = (): void => { "div", { class: "fx-compose-actions" }, button("Cancel", () => { - state.composing = undefined + dropComposed() renderComments() renderRings() }, { small: true, variant: "ghost" }), @@ -1373,7 +1426,7 @@ const renderChecks = (): void => { state.playing = false state.frame = frame?.index ?? 0 state.marks = { frame: issue.frame ?? 0, at: issue.at } - state.composing = undefined + settleComposed() renderStage() repaint() }, { small: true, variant: "ghost", title: "Pause on its frame with its pixels ringed" }) @@ -1573,7 +1626,7 @@ window.addEventListener("keydown", (event) => { // Not Tab: that key belongs to moving through the page's controls. setView(state.view === "asset" ? "set" : "asset") } else if (event.key === "Escape" && (state.composing !== undefined || state.marks !== undefined)) { - state.composing = undefined + dropComposed() state.marks = undefined renderRings() renderComments() diff --git a/packages/tools/src/pixel/review/store.ts b/packages/tools/src/pixel/review/store.ts index 6d31bac4..5fe2c2f1 100644 --- a/packages/tools/src/pixel/review/store.ts +++ b/packages/tools/src/pixel/review/store.ts @@ -58,10 +58,12 @@ export interface Listed { readonly review: string /** Every asset, unless nothing changed since the version asked about. */ readonly entries?: ReadonlyArray | undefined + /** How far the reading is, while the server is still reading the art: no list yet. */ + readonly reading?: { readonly done: number; readonly total: number } | undefined } -/** What a save says: written, with the review's new version, or written meanwhile by someone else, with what the file holds now. */ -export type Saved = { readonly ok: true; readonly version: string } | { readonly ok: false; readonly text: string; readonly version: string } +/** What a save says: written, with the review's new version and the assets it no longer finds, or written meanwhile by someone else, with what the file holds now. */ +export type Saved = { readonly ok: true; readonly version: string; readonly ignored: ReadonlyArray } | { readonly ok: false; readonly text: string; readonly version: string } export interface Store { /** The list, or `undefined` when it cannot be read (the CLI was stopped). */ @@ -84,6 +86,8 @@ const failure = (answer: unknown, fallback: string): string => (isRecord(answer) const isDetail = (value: unknown): value is Detail => isRecord(value) && typeof value.id === "string" && typeof value.version === "string" && Array.isArray(value.issues) +const isReading = (value: unknown): value is { readonly done: number; readonly total: number } => isRecord(value) && typeof value.done === "number" && typeof value.total === "number" + const query = (id: string): string => `id=${encodeURIComponent(id)}` /** The API `ritmo-tools pixel review` serves beside the page. */ @@ -92,7 +96,7 @@ export const serverStore: Store = { try { const answer: unknown = await (await fetch(`/api/library?since=${encodeURIComponent(since)}`)).json() return isRecord(answer) && typeof answer.version === "string" && typeof answer.review === "string" - ? { version: answer.version, review: answer.review, entries: Array.isArray(answer.entries) ? answer.entries : undefined } + ? { version: answer.version, review: answer.review, entries: Array.isArray(answer.entries) ? answer.entries : undefined, reading: isReading(answer.reading) ? answer.reading : undefined } : undefined } catch { return undefined @@ -111,7 +115,7 @@ export const serverStore: Store = { const answer: unknown = await response.json() if (response.status === 409 && isRecord(answer) && typeof answer.text === "string" && typeof answer.version === "string") return { ok: false, text: answer.text, version: answer.version } if (!response.ok || !isRecord(answer) || typeof answer.version !== "string") throw new Error(failure(answer, response.statusText)) - return { ok: true, version: answer.version } + return { ok: true, version: answer.version, ignored: Array.isArray(answer.ignored) ? answer.ignored.filter((id): id is string => typeof id === "string") : [] } }, detail: async (id) => { try { diff --git a/packages/tools/test/pixel-review.test.ts b/packages/tools/test/pixel-review.test.ts index 317340b8..ef4c9486 100644 --- a/packages/tools/test/pixel-review.test.ts +++ b/packages/tools/test/pixel-review.test.ts @@ -1,5 +1,5 @@ import { spawnSync } from "node:child_process" -import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, utimesSync, writeFileSync } from "node:fs" +import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, renameSync, utimesSync, writeFileSync } from "node:fs" import { createServer, type Server } from "node:http" import type { AddressInfo } from "node:net" import { tmpdir } from "node:os" @@ -321,6 +321,33 @@ describe("comments", () => { expect(order.assets.map((entry) => entry.file)).toEqual(["changes", "stale", "new", "approved"]) expect(order.summary).toBe("2 approved (1 changed since: ask the person to look again), 1 with changes asked, 1 not reviewed yet (of 4).") }) + + it("never tell the agent to leave alone what still has an open comment", () => { + const comment = { frame: 0, x: 0, y: 0, text: "Too dark" } + expect(reviewSummary({ status: "approved", note: "", comments: [] }, undefined).next).toContain("Leave it as it is") + expect(reviewSummary({ status: "approved", note: "", comments: [comment] }, undefined).next).not.toContain("Leave it as it is") + expect(reviewSummary({ status: "approved", note: "", comments: [comment] }, undefined).next).toContain("pixel resolve") + expect(reviewSummary({ status: "approved", note: "But the eyes could be wider", comments: [] }, undefined).next).toContain("note") + expect(reviewSummary({ status: "new", note: "", comments: [comment] }, undefined).next).toContain("Act on") + // Resolved comments ask nothing more. + expect(reviewSummary({ status: "approved", note: "", comments: [{ ...comment, resolved: true }] }, undefined).next).toContain("Leave it as it is") + const frames: ReadonlyArray = [{ name: "", index: 0, x: 0, y: 0, w: 8, h: 8, hash: "new" }] + expect(reviewSummary({ status: "approved", note: "", comments: [comment], approved: { frames: ["old"] } }, frames).next).toContain("pixel resolve") + }) + + it("put words left without a status ahead of what nobody has looked at, and count them apart", () => { + const order = pixelSummary([ + { file: "untouched", status: "new" as const }, + { file: "approved", status: "approved" as const }, + { file: "stale", status: "approved" as const, changedSinceApproved: "it looks different" }, + { file: "commented", status: "new" as const, comments: [{}] }, + { file: "noted", status: "new" as const, note: "Rounder" }, + { file: "approved-commented", status: "approved" as const, comments: [{}] }, + { file: "approved-noted", status: "approved" as const, note: "Lovely" } + ]) + expect(order.assets.map((entry) => entry.file)).toEqual(["commented", "noted", "approved-commented", "stale", "untouched", "approved", "approved-noted"]) + expect(order.summary).toBe("4 approved (1 changed since: ask the person to look again; 1 with open comments: read them), 0 with changes asked, 2 with a note or comments but no status set: act on them, 1 not reviewed yet (of 7).") + }) }) describe("sheet JSON", () => { @@ -340,8 +367,8 @@ describe("sheet JSON", () => { }) describe("the review page's server", () => { - const withServer = async (paths: ReadonlyArray, run: (url: string) => Promise): Promise => { - const server: Server = createServer((request, response) => void api(paths)(request, response)) + const withServer = async (paths: ReadonlyArray, run: (url: string) => Promise, listWait?: number): Promise => { + const server: Server = createServer((request, response) => void api(paths, listWait)(request, response)) await new Promise((done) => server.listen(0, "127.0.0.1", done)) try { await run(`http://127.0.0.1:${(server.address() as AddressInfo).port}`) @@ -380,12 +407,77 @@ describe("the review page's server", () => { expect((await fetch(`${url}/asset?id=${encodeURIComponent(id)}`)).status, id).toBe(404) } expect((await fetch(`${url}/../../etc/passwd`)).status).toBe(404) + // An id in a saved review is never a path to write: nothing found, nothing written, and it is said back. const review = encodeReview({ assets: { "../../etc/passwd": { status: "approved", note: "", comments: [] } } }) const { review: base } = (await (await fetch(`${url}/library?since=`)).json()) as { review: string } - expect((await fetch(`${url}/review?base=${base}`, { method: "PUT", body: review })).status).toBe(400) + const saved = await fetch(`${url}/review?base=${base}`, { method: "PUT", body: review }) + expect(saved.status).toBe(200) + expect(((await saved.json()) as { ignored: ReadonlyArray }).ignored).toEqual(["../../etc/passwd"]) + expect(existsSync(join(folder, "pixel.review.json"))).toBe(false) + }) + }) + + it("keeps saving after the agent renames art the page holds a review of, and keeps that review under its old name", async () => { + const folder = game() + await withServer([join(folder, "public")], async (url) => { + const coin = idOf(join(folder, "public/art/coin.png")) + const atlas = idOf(join(folder, "public/art/atlas.png")) + const said = { status: "changes", note: "Rounder.", comments: [] } as const + const first = (await (await fetch(`${url}/review`)).json()) as { version: string } + const saved = (await (await fetch(`${url}/review?base=${first.version}`, { method: "PUT", body: encodeReview({ assets: { [coin]: said } }) })).json()) as { version: string } + renameSync(join(folder, "public/art/coin.png"), join(folder, "public/art/token.png")) + // The page has not looked since: its version is stale, and it still holds the old name. + const stale = await fetch(`${url}/review?base=${saved.version}`, { method: "PUT", body: encodeReview({ assets: { [coin]: said, [atlas]: { status: "approved", note: "", comments: [] } } }) }) + expect(stale.status).toBe(409) + const now = ((await stale.json()) as { version: string }).version + const merged = await fetch(`${url}/review?base=${now}`, { method: "PUT", body: encodeReview({ assets: { [coin]: said, [atlas]: { status: "approved", note: "", comments: [] } } }) }) + expect(merged.status).toBe(200) + expect(((await merged.json()) as { ignored: ReadonlyArray }).ignored).toEqual([coin]) + const file = decodeReview(readFileSync(join(folder, "pixel.review.json"), "utf8")) + expect(file.ok && [file.value.assets["public/art/coin.png"]?.note, file.value.assets["public/art/atlas.png"]?.status]).toEqual(["Rounder.", "approved"]) + }) + }) + + it("does not let a page that has not seen art come back write over its review", async () => { + const folder = game() + const file = join(folder, "public/art/coin.png") + const coinKey = "public/art/coin.png" + writeFileSync(join(folder, "pixel.review.json"), encodeReview({ assets: { [coinKey]: { status: "changes", note: "Rounder.", comments: [] } } })) + renameSync(file, join(folder, "public/art/token.png")) + await withServer([join(folder, "public")], async (url) => { + // The page reads the review while the file is away. + const { version, text } = (await (await fetch(`${url}/review`)).json()) as { version: string; text: string } + renameSync(join(folder, "public/art/token.png"), file) + expect((await fetch(`${url}/review?base=${version}`, { method: "PUT", body: text })).status).toBe(409) + const saved = decodeReview(readFileSync(join(folder, "pixel.review.json"), "utf8")) + expect(saved.ok && saved.value.assets[coinKey]?.note).toBe("Rounder.") }) }) + it("answers while it reads, and says how far it is", async () => { + const folder = game() + mkdirSync(join(folder, "public/more")) + for (let i = 0; i < 8; i++) writeFileSync(join(folder, `public/more/coin-${i}.png`), encodePng(coin(i))) + await withServer( + [join(folder, "public")], + async (url) => { + const first = (await (await fetch(`${url}/library?since=`)).json()) as { reading?: { done: number; total: number }; entries?: ReadonlyArray } + expect(first.entries).toBeUndefined() + expect(first.reading?.total).toBe(12) + expect(first.reading?.done).toBeLessThan(12) + let listed = first + // A look that never waits still gets the list once all is read: it does not have to beat the clock. + for (let look = 0; listed.entries === undefined && look < 1000; look++) { + await new Promise((next) => setTimeout(next, 20)) + listed = (await (await fetch(`${url}/library?since=`)).json()) as typeof first + } + expect(listed.entries).toHaveLength(12) + expect(listed.reading).toBeUndefined() + }, + 0 + ) + }, 60_000) + it("saves the whole review, and refuses one written over a newer file, with what it holds now", async () => { const folder = game() await withServer([join(folder, "public")], async (url) => {