Fix BL-16682 Save page before invoking Bloom AI Image Tools

[andrew-polk]

Fixes BL-16682: "Edit with AI…" opened the AI Image Tools with an empty "Image to Edit" slot when the user had just added the image.

Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16682

Why it happened

Everything Bloom tells the AI editor about the book — the whole-book image list, and on commit each slot's current src — is read from the saved book DOM. An image the user has just added lives only in the live page, because changeImage/changeImageByElement deliberately don't save (BL-16330). So the clicked image matched nothing in the list and no edit target was sent.

Saving first also closes two latent hazards on the same path: a current-page commit result would describe an image the live page no longer showed, and DeleteSupersededAiImageFiles, which judges orphanhood from the saved DOM, could delete a file the live page still used.

What this does

The menu command posts aiImageEditor/saveThenLaunch; C# saves the page with EditingModel.SaveThen and then opens the editor. Because saving always ends in a navigation, two decisions follow, each for its own reason:

Where the overlay lives — the top window. An ordinary page save replaces the page iframe, so an overlay hosted there would be torn down. This is what the three other image commands in the same menu already do; the comment on missingMetadata in canvasControlRegistry.ts says so outright. aiEditorLauncher.ts therefore splits along the frame boundary:

• aiEditorOverlay.ts (top window, on workspaceBundle) — the overlay, the postMessage handshake with the editor's iframe, and the commit.
• aiEditorPageCommands.ts (page frame, on editablePageBundle) — the only two things that must run where the live page is: the menu command, and applying a commit's current-page swaps.
• aiEditorShared.ts — the types and the predicates both halves must agree on.

When it opens — after the page has come back, via the new EditingModel.RunAfterNextPageLoad. SaveThen's existing doAfterSaveToDisk looks like the right hook, but it runs immediately before the navigation, and that navigation is not always confined to the page iframe: EditingView.StartNavigationToEditPage reloads the whole workspace root once MemoryUtils.SystemIsShortOfMemory() — Bloom's own private bytes past ~2GB, i.e. the ordinary state of a long editing session on a big book. Opening from doAfterSaveToDisk there meant the page saved correctly and the editor never appeared, with no message.

Consequences worth knowing:

• The request to open is queued up front, but acted on only if the save reached disk (doAfterSaveToDisk) or was never attempted (doIfNotInRightStateToSave). Queuing up front matters because OnTabAboutToChange discards the queue when the user leaves the Edit tab — queuing later let that discard run first on an in-flight save, so the editor sprang open on return. Gating on the outcome is what stops a failed save from opening the editor on a book DOM that is still stale; failureAction would not do, since it isn't called when _saveBook() itself throws (the disk-full case) nor on the deliberate discard path.
• Opening the overlay waits for window.workspaceBundle rather than assuming it. C# is called the instant the page iframe reports loaded, and on the whole-workspace-reload route the root document loads in parallel, assigning that global at the very end of workspaceRoot.ts; calling straight in would throw inside a fire-and-forget script, i.e. silently do nothing on precisely the route this design exists to survive.
• The deferred-save workaround in the commit handler is gone. It existed only because saving reloaded the frame owning the overlay's close button. That deferral also meant a second commit in one session read its oldSrc from a still-stale saved page and matched nothing — so this fixes that too.
• The page frame is asked to apply current-page swaps only when the commit actually contains one, so an all-off-page commit is no longer failed just because that frame happens to be mid-reload.

Commits

1. ca0f97d — the fix.
2. 2814f90 — moves the AI editor front end out of bookEdit/toolbox/canvas/ (whose own AGENTS.md describes it as canvas UI and utilities) into bookEdit/aiImageEditor/, beside copyrightAndLicense/. Pure relocation plus import paths — git show -M shows 11 renames, four with no content change — plus an AGENTS.md recording the frame rule, which bundling does not enforce on its own.

A smaller alternative that kept the overlay in the page iframe was written and compared as #8193, and closed in favour of this one.

Verification

• Front-end: full vitest run --no-file-parallelism — 687 passed, 5 skipped, 62 files. The launcher's single suite became two, tracking the split; aiEditorOverlay.test.ts needs no page DOM at all.
• C#: full BloomTests suite — 3063 passed, 0 failed, 12 skipped.
• pnpm typecheck, eslint, and the isolated production bundle build are clean.
• The 5 bloom-exe AI-editor Playwright tests pass against a running Bloom.
• Verified live, including the awkward case: with ShouldDoFullReload() forced true — the state that made the first attempt at this fix silently show no editor — the overlay appears.

---

Devin review

🤖 Generated with Claude Code

---

This change is 

Devin review

[andrew-polk]

[Claude Opus 5 (1M context) from Andrew Polk's machine during preflight]

Consulted Devin twice on 2026-08-10, up to commit bcab9a5bfb647b6c9af7994d6d702f90c0f256b3.

First pass (fcec7b2) raised 1 Bug and 3 Investigate flags. Three are dealt with and their threads are resolved:

• the new helper functions used const foo = () => where src/BloomBrowserUI/AGENTS.md asks for function syntax — a fair catch, fixed;
• the real one: "Edit with AI…" could silently do nothing. The editor was only opened once the save had succeeded, so a save that was attempted and failed left the user with no editor and no message. Fixed by also opening it on the failure path;
• it suspected the driveAiImageEditor.mjs CDP helper still assumed the overlay appears instantly after the click. Checked and it does not need changing — it already waits up to 20s for the editor, and a successful commit already saved immediately before this PR. Reasoning is on that thread.

Second pass (bcab9a5) raised one new flag worth keeping: the overlay is created before the post-save page navigation, so the design depends on that navigation reloading only the page iframe and never the whole window. That holds when you read the navigation code, but it has not been confirmed in a running Bloom for this variant, so that thread is left open.

Two threads are deliberately still open for the developer: the one just described, and the question of what should happen when someone re-commits after a partially-applied commit (today's retry redoes the already-saved off-page slots and orphans their files). Both are in the preflight decision report.

Devin also raised 6 informational observations, not mirrored here as threads. Two were worth a second look and neither needs action: saveThenLaunch runs with requiresSync:false, which is fine because it only starts a save — the work itself lands in editView/pageContent, which does take the lock; and its "the editor could open twice in rare save paths" idea doesn't reproduce, because the failure path returns before the after-save action can run.

Note that its second pass re-reported the arrow-const Bug and the silent-failure flag with their original pre-fix text (still describing applyAiImageEditorReplacements as an arrow-const, and saying no failureAction is supplied when it is, at AiImageEditorApi.cs:298). Those are re-review staleness, not new findings; the fixes are in bcab9a5 and the threads stay resolved.

CI: pr-automation passed. No other review bot runs on this repo — CodeRabbit is configured but disabled (auto_review.enabled: false).

[andrew-polk]

[Claude Opus 5 (1M context)] This branch has been rewritten as a single self-contained commit (f9cd8ba) so it no longer stacks on approach A's commit — the two approaches are now independently comparable, with approach A at #8193. The tree is byte-identical to what was reviewed as bcab9a5; only the history changed. The Devin threads above anchor to the pre-rewrite commits and so show as outdated, but every finding and its outcome still stands as recorded.

[andrew-polk]

[Claude Opus 5 (1M context)] Squashed to a single commit b3b9311 at Andrew's request. The tree is byte-identical to 8c4c4b7, which is what Devin reviewed and what both full suites ran against (687 front-end, 3063 C#) — only the history changed. The Devin threads above therefore anchor to commits no longer in the branch and show as outdated; every finding and its outcome still stands as recorded, and the two threads left open are still open.

[andrew-polk]

[Claude Opus 5 (1M context) from Andrew Polk's machine during preflight]

Consulted Devin five times across this branch, most recently up to commit b3b9311d581fbc315a1fc8bd79f096303ec24540.

What it found that was real, and is fixed:

• "Edit with AI…" could silently do nothing. The editor was only opened once the save had succeeded, so a save that was attempted and failed left the user with no editor and no message.
• A wholly successful commit could be reported as an error. The overlay asked the page frame for its exports before checking whether the commit had anything to do on the open page, and that frame is briefly unreachable while it reloads — a window this feature's own post-commit save opens. Users would have seen a failure for pictures that had in fact been replaced and saved.
• An over-defensive payload parse, against AGENTS.md's explicit "don't be overly defensive / let it throw" rule.
• A duplicated, self-contradicting paragraph in the CDP driving guide.
• The PR description no longer matched the code — the comparison table still claimed this approach left EditingModel untouched. That one mattered: someone could have chosen between Fix BL-16682 Save page before invoking Bloom AI Image Tools #8191 and Fix BL-16682 Save page before invoking Bloom AI Image Tools (approach A: minimal) #8193 on a table that was wrong about the main difference between them. Corrected on the PR.

Two threads are deliberately left open for the developer, both design questions rather than defects, and both applying to #8193 equally now that the two approaches share the RunAfterNextPageLoad hook:

• what a retry should do after a partially-applied commit (today it redoes the already-saved off-page slots and orphans their files);
• whether a queued launch that is never consumed should be dropped. Devin's suggested fix trades that surprise back for the silent-do-nothing failure fixed above, so it needs a human call.

A note for anyone reading Devin's own pages here: its later passes re-report findings that were already fixed, using their original pre-fix text — still describing helpers as arrow functions after they were converted, still saying no failureAction is supplied when the code deliberately no longer needs one, still describing the off-page-commit bug after it was fixed. Each was checked against the working tree rather than taken at face value; the resolved threads stay resolved. Treat a re-review's findings as describing the diff, not necessarily the current code.

CI: pr-automation passed. No other review bot runs on this repo (CodeRabbit is configured but disabled via auto_review.enabled: false).

[andrew-polk]

@andrew-polk partially reviewed 12 files and all commit messages, and resolved 4 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions.

[andrew-polk]

@andrew-polk partially reviewed 17 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions.

[andrew-polk]

@andrew-polk reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions.

[andrew-polk]

You probably want to review as two separate commits. The second one is just moving files.

[JohnThomson]

@JohnThomson reviewed all commit messages, made 3 comments, and resolved 2 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on andrew-polk).

---

a discussion (no related file):

Previously, andrew-polk wrote…

You probably want to review as two separate commits. The second one is just moving files.

I found the control for requesting this, but couldn't make it work. What it showed me at first was just the changes to editing model. Reviewing as a whole makes it a bit overwhelming, but I've tried to focus on changes that looked significant. Just a couple of questions.

---

src/BloomExe/web/controllers/AiImageEditorApi.cs line 299 at r7 (raw file):

            // Queue BEFORE the save: with no page loaded, SaveThen completes synchronously.
            model.RunAfterNextPageLoad(loadedPageId =>

Things are probably just being defensive here, but the comments seem confused. A few lines above, we aborted completely if there isn't a current page being edited. Now the comment immediately above (and I think I've seen this in a few places) claims we're trying to continue even if no page is loaded.

---

src/BloomExe/web/controllers/AiImageEditorApi.cs line 318 at r7 (raw file):

            // save can fail still ends in EditingStateMachine navigating back to the page, so the
            // queued launch fires anyway. That the hook covers the failure paths for free is one of
            // the reasons for preferring it over doAfterSaveToDisk.

This is (hopefully) a very unlikely corner case, but are we sure we still WANT to open the AI image editor if something went so badly wrong that we can't even save the current page? I'm not sure what could cause such a failure, but things that come to mind are not enough disk space to update the HTML file, a badly corrupted image file, out of memory,...

[andrew-polk]

[Claude Opus 5 (1M context) from Andrew Polk's machine during preflight]

Thanks John — both of your questions were right, and the second one changed the behaviour. Fixed in e9179df (force-pushed, so the PR is back in draft while it re-runs the checks).

On the confusing comment at AiImageEditorApi.cs:299 — you read it exactly as written and it was wrong to write it that way. I had conflated two different things called "page": the guard a few lines up tests model.CurrentPage, the book page selection, whereas the comment meant EditingStateMachine's NoPage state, i.e. no page loaded in the browser (the Edit tab hidden). Those are independent — you can have a CurrentPage while the machine sits in NoPage, and in that state ToSavePending runs synchronously, which was the real reason the request had to be made before SaveThen. Rather than reword it, the restructuring below removes the ordering subtlety altogether, so the comment is simply gone. (You wondered whether you'd seen the same thing elsewhere — I checked, and that was the only occurrence.)

On whether we should open the editor at all after a save that failed — you've convinced me, and for a reason that strengthens your case: if the page could not be saved then the saved book DOM is still stale, so the editor would open on exactly the out-of-date data this whole change exists to prevent, and a commit from there would call book.Save() again on top of whatever went wrong. Your examples are apt: disk full, a corrupt image, out of memory.

The earlier reasoning for opening anyway was to avoid a menu item that silently does nothing. That concern doesn't apply here, though: EditingStateMachine has already shown the user "Bloom had trouble saving a page… Then quit Bloom, run it again" before any of this, so they are told — they just shouldn't also get an image editor on top of it.

So the request to open is now made only from the two paths where the book DOM is sound:

• doAfterSaveToDisk — runs only after _saveBook() succeeded;
• doIfNotInRightStateToSave — no save was attempted, nothing failed, and a page load is coming from whatever refused it.

Every failing path reaches neither, so the editor stays closed. Worth noting that this is broader than the obvious fix: my first attempt was to cancel via failureAction, and checking it against your disk-full example showed it wouldn't have covered it — failureAction isn't called when _saveBook() itself throws. Inverting it to "only ask when we know the save landed" also covers the _discardInFlightSave path, where Bloom has deliberately thrown the page away and an image editor would have been the wrong response.

On reviewing as two commits — sorry that fought you. Noted that the tooling didn't cooperate; the split is still worth having in the history (the second commit is renames only, four files with no content change at all), but I won't claim it made your review easier this time.

[andrew-polk]

[Claude Opus 5 (1M context)] Sorry the two-commit split fought you rather than helping. Noted that the control didn't work — the second commit is renames only (11 files, four with no content change), so git show -M on it is the quickest way to skip past it if you look again.

Also my fault that you had to read one long summary instead of answers under the lines you asked about: I replied with a single top-level comment because your comments have no GitHub mirror and I had wrongly ruled out using the Reviewable CLI. Both of your code comments now have their own replies in place, so this thread doesn't need the detail.

@andrew-polk+AI made 3 comments.
Reviewable status: 19 of 20 files reviewed, 3 unresolved discussions (waiting on andrew-polk and JohnThomson).

---

src/BloomExe/web/controllers/AiImageEditorApi.cs line 299 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Things are probably just being defensive here, but the comments seem confused. A few lines above, we aborted completely if there isn't a current page being edited. Now the comment immediately above (and I think I've seen this in a few places) claims we're trying to continue even if no page is loaded.

[Claude Opus 5 (1M context)] You read it exactly as written, and it was wrong to write it that way. I had conflated two different things called "page": the guard a few lines above tests model.CurrentPage, the book page selection, whereas this comment meant EditingStateMachine's NoPage state — no page loaded in the browser, e.g. the Edit tab hidden. Those are independent: you can have a CurrentPage while the machine sits in NoPage, and in that state ToSavePending runs DoPostSaveAction synchronously, which was the real reason the request had to be made before SaveThen.

Rather than reword it, the restructuring for your other comment removed the ordering subtlety altogether, so this comment is simply gone in 140387d.

On your parenthetical — you wondered whether you'd seen the same thing in a few places. I grepped: that was the only occurrence in this change. The two other hits for similar wording are pre-existing and unrelated (EditingStateMachine.cs describing its own state, and a comment in ImageApi.cs).

---

src/BloomExe/web/controllers/AiImageEditorApi.cs line 318 at r7 (raw file):

Previously, JohnThomson (John Thomson) wrote…

This is (hopefully) a very unlikely corner case, but are we sure we still WANT to open the AI image editor if something went so badly wrong that we can't even save the current page? I'm not sure what could cause such a failure, but things that come to mind are not enough disk space to update the HTML file, a badly corrupted image file, out of memory,...

[Claude Opus 5 (1M context)] You've convinced me, and for a reason that strengthens your case: if the page could not be saved then the saved book DOM is still stale, so the editor would open on exactly the out-of-date data this whole change exists to prevent — and a commit from there would call book.Save() again on top of whatever went wrong. Your examples are apt.

The earlier reasoning for opening anyway was to avoid a menu item that silently does nothing. That doesn't apply here: EditingStateMachine has already shown the user "Bloom had trouble saving a page… Then quit Bloom, run it again" before this point, so they are told — they just shouldn't also get an image editor on top of it.

Fixed in 140387d. The request to open is now queued up front but acted on only if a captured bookDomIsSound flag was set, from either doAfterSaveToDisk (the save reached disk) or doIfNotInRightStateToSave (no save was attempted, nothing failed, and a page load is coming anyway). Every failing path reaches neither, so the editor stays closed.

Two things I got wrong on the way there, in case the shape looks roundabout:

1. My first attempt was to cancel via failureAction. Checking it against your disk-full example showed it wouldn't have covered it — failureAction isn't called when _saveBook() itself throws, nor on the deliberate _discardInFlightSave path. Gating on "did the save land" covers all of them.
2. My second attempt only queued from those two callbacks rather than queueing up front and gating. Devin caught that this broke a different ordering: OnTabAboutToChange discards the queue when you leave the Edit tab, so on a save still in flight that discard ran first and the completing save re-armed behind it — the editor then sprang open when you returned to the page. Hence queue-early-but-gate.