Skip to content

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

Merged
SandroMaglione merged 2 commits into
mainfrom
tools/pixel-review-fixes
Nov 5, 2026
Merged

SandroMaglione merged 2 commits into
mainfrom
tools/pixel-review-fixes

Conversation

@SandroMaglione

@SandroMaglione SandroMaglione commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

Fixes the breaking problems #187 listed as known. Each was reproduced on copies of the real games' art first, then checked again after the fix.

What changes

  • Saving no longer stops after art is renamed. When the agent renamed a file, or pixel fix put X.fixed.png in place of X.png, every later save failed with no such asset until a reload, which lost what was unsaved. Now the server writes the assets it finds and says back the rest. A review of a name no longer found stays in pixel.review.json under that name, and pixel notes lists it under gone.
  • A review can't be overwritten by a page that missed an asset coming back. The review's version now follows which assets are found, so that page gets a 409 and merges. Found while fixing the above: the old code would have deleted that review.
  • A comment being typed is kept. It survives a click on a status and a reply from the agent, with focus and caret in place. Moving to another asset, animation, or view adds it, with a message saying so. Cancel and Esc drop it, and drawing a new area keeps the text.
  • pixel notes no longer misleads. An approved asset with open comments no longer says "Leave it as it is". An asset with a note or comments but no status comes ahead of untouched ones, and the summary counts it apart.
  • A large folder no longer blocks the page. Reads take turns, so the server answers between them, and the page shows "Reading the art… 239 of 2128". Once all is read, the list comes however long its files take to check, so the page can't stay on "2128 of 2128". Past 500 assets, pixel review prints a warning and pixel notes warns on stderr before it starts reading.
  • Holding an arrow key works in Safari. It hit Safari's limit of 100 address changes in 10 seconds. The address now updates once the person stops on an asset.

Decisions to check

  • Moving on adds the comment rather than asking or dropping it, because everything else on the page saves as the person goes. Undo takes it back. The risk is a half-written thought reaching the agent, so the page says it was added.
  • A picture is now served as its file, not decoded and written again. The hash still comes from the decoded pixels. A browser could draw a PNG with a color profile differently from the pixels hashed, which is also how the game's browser would draw it. Re-encoding on demand was tried first: on the whole set, the page's saves waited behind picture requests for a connection, and the page stuck on "Saving…" for over 8 seconds in WebKit.
  • 500 assets is the warning line. The real folders are 85 to 144 assets of art, and spirit-call/art is 2128, mostly 3D renders. It warns and carries on: a large folder may be what the agent meant.

Measured on spirit-call/art (2128 PNGs)

Before After
Page answers while reading no answer for 5 minutes within 1 second
List ready 305 s 225 s
pixel notes 314 s 217 s
Server memory about 2.8 GB about 750 MB

Before and after were single runs, so take the times as rough. The "after" runs had other tests going beside them.

Verification

  • pnpm run check (1401 tests) and pnpm pack:check pass. website/ isn't touched.
  • New tests in test/pixel-review.test.ts cover saving after a rename, a stale page and an asset coming back, the list answering while it reads, next with open comments, and the order and summary.
  • The page's behavior has no repo test, since the repo has no browser test setup. A throwaway Playwright run covered it in Chromium and WebKit, on the built page, over copies of Still Time, Warden, Spirit Call, Dungeon Paint, Pixel Probe, and Style Probe (238 assets):
  • The page was watched in both browsers on all 2128 assets, with the count going up and the list arriving with no errors.
  • The real game folders weren't changed.

Not covered

  • source in the notes. It still only shows when the source sits beside the PNG, so Still Time's notes don't name theirs. Nothing breaks without it, and fixing it needs a decision on how a game declares where its sources are.
  • A comment typed when the page reloads or closes is still lost.
  • The effects page's comment box is built the same way as the one fixed here, so it may lose typed text too. Not tested.
  • WebKit at exactly 1×: a press can land one pixel off. Assets now open larger, so this is rarer, and it's unchanged here.

🤖 Generated with Claude Code

SandroMaglione and others added 2 commits November 5, 2026 10:54
…nt the truth about it

What #187 left, each found by running the tool on the real games' art:

- Saving stopped for good once the agent renamed art the page held a review
  of, or `pixel fix` put X.fixed.png in place of X.png. The server now writes
  what it finds and says back the rest, whose review stays under its old name.
  The review's version also follows which assets are found, so a page that has
  not heard an asset came back cannot write over that asset's review.
- A comment being typed was lost on a click on a status, on the agent's reply,
  or on moving on. Its text now lives in the page's state, and moving to
  another asset, animation, or view adds it. Cancel or Esc drops it.
- `pixel notes` said "Leave it as it is" about an approved asset with open
  comments, and counted a commented asset with no status as not reviewed.
- A folder of thousands blocked the server for minutes: nothing answered, not
  even the page. Reads now take turns, so it answers between them, and the page
  says how far it is. Past 500 assets, review and notes warn. A picture is
  served as its file instead of a copy written up front, which saves a third of
  the time and most of the memory.
- Holding an arrow key in Safari hit its limit of 100 address changes in 10
  seconds, and the page stopped moving. The address now follows once it settles.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…g it takes to check

A look at the list answered with the list only if it beat the wait, even with
every read done. Checking the files of a large folder on a slow disk could lose
that race on every look, leaving the page on "2128 of 2128" for good. CI hit it
through the new test, which never waits.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@SandroMaglione
SandroMaglione merged commit 741fac8 into main Nov 5, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant