Skip to content

Fix all code-review findings (2026-10-21) and history.js on Chrome for Mac - #22

Merged
pathanin merged 5 commits into
masterfrom
fix/save-resize-flatten
Oct 24, 2026
Merged

pathanin merged 5 commits into
masterfrom
fix/save-resize-flatten

Conversation

@pathanin

Copy link
Copy Markdown
Owner

Fixes every finding of the 2026-10-21 code review (docs/code-review-2026-10-21.md), plus the 3 history.js checks that failed on Chrome for Mac.

What changed

Work that waits no longer lands on a newer document (findings 1–3, c56df80)

  • A save, resize or Open still running when New or Open replaces the document is cancelled or dropped. A new S.docId tells documents apart.
  • ⌘E on a selection keeps marks that clear (Transparent strokes, patches, fills that replace pixels) as objects. Before, it brought the cleared pixels back.

The rest of the review (findings 4–12)

  • 4: the "nothing seen changes" claim for ⌘E on a selection now notes anti-aliasing shifts on nearby paths.
  • 5: Open after a slow "Discard unsaved changes?" offers the picker from a toast button. It used to fail with "permission was denied".
  • 6: PNG CRCs are computed only for the chunk that is read.
  • 7: a colour pick restyles only what the selected kind draws with. No more undo steps that change nothing.
  • 8: Cut deletes only after the clipboard write succeeds, and only what it copied.
  • 9: box-select and pan release toolbar focus, so Delete and the arrows reach the selection.
  • 10: [ / ] move an odd size above 10 onto the even grid.
  • 11: png-worker.js reads a transferred snapshot back. The page no longer calls getImageData on #image. The worker still accepts the old {raw} message.
  • 12:
    • encodeBlob falls back to toBlob when the snapshot fails.
    • A PNG's scene chunk is validated (Core.validObj). A bad one opens as plain pixels.
    • No Math.max(...) over object lists (Core.unionBox).
    • A brush's frame is its ink.
    • paintLater skips an empty pass.
  • Also: copying the whole image won't put an image edited mid-copy on the clipboard, and ⌘E on a lone image adds no undo step.

history.js on Chrome for Mac

  • Cause: a live pencil stroke left the anti-aliased edges of objects under it (text, scaled images) up to 4 levels off a fresh render. The stroke's rects were drawn in a later GPU batch than those objects.
  • Fix: when the stroke joins, its tiles that hold other objects are composed again (rejoin, scene.js).
  • Cost, measured on the M4 at 4000×3000:
    • ~9 ms more main-thread time on release (pencil over 5k: inpMain 1.2 → 10.2 ms; no interaction over 200 ms).
    • The GPU process settles ~50 MB higher. Over 60 strokes it levels off, so it's not a leak. The bench's after edits memory row shows +120 MB.

Decisions for you

These are the agent's defaults. They're also listed in task.md under Open.

  • An Open superseded by a later Open, drop or New is dropped silently.
  • An edit made while an Open loads, or while its Open… toast waits, brings a second discard prompt.
  • New messages:
    • "Save cancelled: a different image is open now"
    • "Discard confirmed. Choose the file to open" (with an Open… button)
    • "Copied: the selection changed, so nothing was cut"
    • "Copy failed: the image changed while it was copied. Copy again"
  • ⌘E keeps marks that clear as objects, with a toast saying why.
  • ⌘E on a selection that is already one image does nothing, with no message.
  • One test now expects different behaviour. Pressing X on a selected line used to swap both colours, but a line never draws its colour 2 (alt). The check now expects only the line's colour to change.

Verification (this Mac)

  • Unit: 95 passed.
  • Chrome: smoke 332, history 86, preview 40, filledge 13, objects 236, alpha 114, libs 75, pwa 16. All passed.
  • WebKit: smoke 331, history 86, preview 40, filledge 13, objects 233, alpha 114, libs 75. All passed.
  • Each new check was run without its fix and failed.
  • Not done: the fourth review round of c56df80 was interrupted before it reported. Firefox and real Safari remain untested, as before.

…hat clear as objects

Fixes findings 1-3 of the 2026-10-21 code review, and what three review rounds of the fix found.

- Save: S.docId (bumped by New and Open) is checked before the encode (the save is cancelled)
  and after the write (the new document keeps its own handle, name and save point).
  encodeBlob's fallback, which encodes the live canvas, rejects when the document was
  replaced (its `still` callback), so a failing blob worker can't write the new document
  into the old file.
- Resize: a scale still running when New or Open replaces the document is dropped
  (New and Open reuse the base canvas and empty the history, so only S.docId tells).
- Open: requests to replace the document are ordered with New (doc.js requestDoc /
  landDoc). An Open still loading when a later request lands is dropped quietly; a later
  one that never lands leaves it be; an edit made while it loads is asked about again.
- ⌘E on a selection: a mark that clears or replaces what is under it (Core.clearsUnder)
  stays an object and splits the images where it overlaps them, with a toast.

Tests: core.test.js (clearsUnder, flattenRuns), alpha.js 13, libs.js (resize during
New/Open, save during New with a failing blob worker), smoke.js (saves and Opens racing
New and each other). Each new check fails without its fix.
…s on Chrome for Mac

- A pencil or eraser stroke that joins recomposes its tiles that hold other objects: drawn in
  later batches, it left their anti-aliased edges up to 4 levels off a fresh compose on Chrome
  for Mac (history.js's 3 failures here).
- Open after a slow discard prompt offers the picker from a toast button.
- PNG CRCs are computed lazily; png-worker reads a transferred snapshot back instead of the
  page calling getImageData on #image.
- A restyle sets only what the kind draws with; Cut deletes after the clipboard write, and only
  what it copied; box-select and pan release toolbar focus; [ / ] keep to the even grid above 10.
- encodeBlob falls back to toBlob when the snapshot fails; copying the whole image refuses a
  picture changed while it encoded; a second Cmd+E on a lone image adds no step.
- readScene validates objects (Core.validObj); Core.unionBox replaces Math.max(...) over object
  lists; a brush's frame is its ink; paintLater skips an empty pass.
- The "nothing seen changes" claim for Cmd+E on a selection now notes anti-aliasing shifts.
…colour and size, bench row for a pencil over objects

- The Open… toast keeps the document mark from its confirm, so an edit made while it waits is
  asked about again (the file input path too, through takeInputSeen()).
- A line draws with colour and size only: colour 2 on a selected line adds no step.
- encodePng catches a failed snapshot where it is taken (no unhandled rejection).
- bench.js: 'pencil over 5k' measures rejoin's cost; task.md records it and the open decisions.
@pathanin
pathanin merged commit 0405186 into master Oct 24, 2026
3 checks passed
@pathanin
pathanin deleted the fix/save-resize-flatten branch October 24, 2026 11:00
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