ritmo-tools pixel review: keep what the person says, and tell the agent the truth about it

.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.

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.
 

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<str
   return yield* promised("cannot look through the paths", () => 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<PixelAsset>): 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) {

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<Frame>
       readonly animations: ReadonlyArray<SheetAnimationRead>
       /** The sheet's JSON, when it is a sheet. */
@@ -208,7 +206,7 @@ const read = async (asset: PixelAsset): Promise<AssetRead> => {
       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<Frame> = [{ 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<AssetRead> => {
             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<unknown> = 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 = <A>(run: () => Promise<A>): Promise<A> => {
+  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<string, { readonly version: string; readonly read: Promise<AssetRead> }>()
+const reads = new Map<string, { readonly version: string; readonly read: Promise<AssetRead>; done: boolean }>()
 export const readAsset = async (asset: PixelAsset): Promise<AssetRead> => {
   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<PixelAsset>): Map<string, Array<PixelAsse
   return files
 }
 
-/** Changes when any of the assets' review files does: a hash of what they hold, so the page knows its own write from another. */
+/**
+ * Changes when what the page holds would: the reviews of these assets as their
+ * files say them, so the page knows its own write from another. An asset going
+ * away or coming back changes it too: a page that has not heard must not write
+ * over a review it cannot see.
+ */
 export const reviewVersion = async (assets: ReadonlyArray<PixelAsset>): Promise<string> => {
   const parts: Array<string> = []
-  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<Entry> => {
 /** The page's list, read again only where files changed. */
 export const libraryOf = (assets: ReadonlyArray<PixelAsset>): Promise<ReadonlyArray<Entry>> => 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<PixelAsset>): { 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<Detail> => {
   const got = await readAsset(asset)

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<string>): Handler => {
+export const api = (paths: ReadonlyArray<string>, listWait = LIST_WAIT_MS): Handler => {
   const named = (request: IncomingMessage, assets: ReadonlyArray<PixelAsset>): 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<string>): 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<undefined>((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,24 +55,25 @@ export const api = (paths: ReadonlyArray<string>): 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}` })
       if (route === "GET /asset") return send(response, 200, await detailOf(asset))
       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}` })