Save a page without wrecking it, and spend that on the page list's commands (BL-13502) by JohnThomson · Pull Request #8209 · BloomBooks/BloomDesktop

JohnThomson · 2026-08-14T22:33:19Z

The problem. Saving the page you were editing used to destroy it. To read the page's content,
Bloom stripped the editing markup out of the live DOM — took CKEditor down, detached the
toolbox tool, unwound the canvas-element machinery — leaving a page that could be saved but no
longer typed in. Every save therefore had to end by navigating away and reloading it, which is why
saving flashed, threw away the cursor position, and could not be done casually. Separately, C#
could not read the page itself: it had to ask the browser and wait for the answer on another
API call. So everything that needed a save first — duplicate, delete, reorder, changing layout,
leaving the Edit tab, quitting — was split into a "before" and an "after" around that wait, with
two extra states in the editing state machine to sit in meanwhile.

What this PR changes.

The page is gathered from a clone of the body, and the live page is never touched. A save
leaves the user exactly where they were, still editing. A tool takes its markup off whichever it
is given — the clone for a save, the live page when it is detached — through one method, so the
two can't drift apart.
The browser volunteers the page rather than being asked for it: it watches the page and posts
the current content to C#, which stores it under the id of that page load. A save then takes that
content synchronously.
With nothing to wait for, the asynchronous machinery goes: the ask-the-browser API and its
browser half, two state-machine states and everything that served them, and the shutdown kludge
that cancelled the user's quit and re-issued it once the save came back. Leaving the Edit tab and
closing the collection are now straight-line code.
C# decides whether a save would change anything, comparing the page it is given, after its own
processing, against the page it already has. Opening a page and touching nothing now writes
nothing, and neither does leaving the Edit tab or quitting with nothing changed. A command that
changes the book itself — a new layout, a copyright, a levelled-reader level — says so, so that
skipping the write can never skip its change. Every save, whether it goes on to another page or
stays put, makes that decision in one place and clears the same flags afterwards.
Several things that made an untouched page look edited are fixed: editor chrome (CKEditor's
toolbars, qTip's bubbles and attributes) no longer travels in the gathered content; the SVG
Comical draws no longer carries freshly generated ids; and a text-measuring scratch element can no
longer be written into the book.
Because the whole design rests on "no snapshot means nothing to save", the ways that could quietly
be untrue are closed: a post that fails, or that C# refuses, is offered again rather than counted
as delivered, and a gather that throws is reported instead of leaving C# believing there was
nothing to save. And when the page is mid-way through asynchronous work that belongs in it (an
image being sized, a paste settling), the browser says so and what the work is; a save that
would otherwise use a snapshot from before that work waits, briefly, for the browser to say the
finished page has been sent, and logs the culprit if it has to go ahead anyway.
A game page can be played in the editor while all this goes on. Gathering a copy of such a page
used to reach into bloom-player's record of the live page -- undoing the tester's drags, and
saving the draggables where they had dragged them rather than where the author put them. That is
fixed in bloom-player itself (bloom-player#441, in 2.20.3-alpha.1, which this PR requires), so a
save now leaves the game alone and writes the authored positions.

src/BloomExe/Edit/SavingWithoutReloading.md explains the design and the measurements.

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

Devin review

This change is

JohnThomson · 2026-08-14T23:36:10Z

[Claude Opus 5 from John Thomson's machine during preflight] Consulted Devin through three review rounds during this preflight, most recently up to e10b109.

It found three real bugs, all now fixed and each with its own resolved thread above:

the reader tools' editing highlight was being saved into the book, because their cleanup only ever worked on the live page and the save now works from a clone;
editView/savePageInPlace reported success even when the save was refused;
removeToolboxMarkup() had lost every caller, so the open toolbox tool was no longer shut down when the user left a page.

It also raised a set of "is this intended?" flags. Four turned out to be worth acting on and are fixed and resolved (a missing disk write from the NoPage state, a brittle dependency on the reply's content type, and a page-list command that could vanish silently if the page frame navigated mid-wait). Four are deliberate design decisions or genuine trade-offs and are left open for John: the clone-cleanup ordering (which no single order gets right for both the reader tools and Talking Book), whether canvas-element alternates should still be conditional on canvas editing being on, and the click-time content snapshot.

Devin's own re-review of 38e8647 reported no unresolved bugs remaining. CI is green; CodeRabbit does not review this repo (auto_review is disabled in .coderabbit.yml).

JohnThomson · 2026-08-17T22:57:22Z

[Claude Opus 5 from John Thomson's machine during preflight] Consulted Devin through four review rounds during this second preflight, most recently up to 5834032.

It found five more real defects, all fixed, each with a resolved thread above: an id that isn't a valid CSS identifier would have aborted the whole save and posted an error string instead of the page; the AI image editor stayed silent on two of the three ways its save can fail; a test kept a comment claiming an assertion it had lost; a failed save request was reported as though Bloom had refused; and the committed benchmark scripts only ran on my machine.

Two flags are left open for John, both about visible behaviour rather than defects: whether dropping the 100ms delay before a context-menu command is worth the small scheduling race it leaves (the delay was, undocumented, what made the ordering safe), and whether Copy Page should still move you to the page it copied when that isn't the page you are on.

Also worth recording from this run: preflight's own local review caught a deadlock risk I had introduced myself — dropping that deferral had left the command running inline in the API handler, while two of those commands open modal dialogs this same server must serve and the handler still holds the API lock.

Across both preflight runs Devin has found ten real defects in this work. None were caught by the tests or by driving the real UI. CI is green; CodeRabbit does not review this repo (auto_review is disabled in .coderabbit.yml).

JohnThomson · 2026-08-19T17:42:24Z

[Claude Opus 5 from John Thomson's machine during preflight] Consulted Devin through two more review rounds in this third preflight, most recently up to 62117e6.

Seven findings now have their own threads above; six are fixed and resolved, one is left open for John.

Fixed this round:

the refusal to save a book another program had just replaced was undone by the fallback path, so the other program's work could still be overwritten — there is now a distinct Refused outcome that SaveThen will not fall back from (b42b63d);
leaving the Edit tab only shut the toolbox down, leaving the controls above the page and the canvas machinery mounted — it now runs the page frame's whole pageUnloading() teardown (b42b63d);
a save could quietly delete a book's own highlight markup nested inside a narrated sentence; the Talking Book tool now marks the spans it creates and unwraps only those (b42b63d);
a save requested from inside a save-in-place action stranded the editor in SavePending, so the page the command promised to go to was never shown (62117e6).

Also given threads, having been fixed in earlier rounds without one: the "Duplicate Page Many Times" deadlock (the 100 ms deferral is what releases the API lock before a modal dialog opens — it is back, with the reason recorded at the call site), and balloon data being rewritten on saves of pages where balloon editing is suspended.

Left open for John: after an outside program rewrites the book, a thumbnail click or page command is now dropped silently. That is strictly safer than the old behaviour, which overwrote the other program's file, and the state is largely designed away — setting the flag immediately kicks the user to the Collection tab. But "skip the save and navigate anyway" is only safe for callers whose action is pure, so it needs a human call. See the thread on EditingModel.SaveThen.

Full suites green at 62117e6: C# 3145 passed / 0 failed, front-end 755 passed / 0 failed. CI (pr-automation) passing. No other comment-posting review bots are configured on this repo.

Not yet done: the live smoke test. The Windows session on this machine is locked, so Bloom's WebView2 has no visible page and CDP sees only about:blank — the UI cannot be driven. Everything on that list still needs to be exercised by hand or on an unlocked desktop.

The same fault as the last two, one layer deeper. BookStorage.SaveForPageChanged has its own safety check -- it refuses a page whose marginBox has come out empty, which Bloom has seen happen and does not understand (BL-13078 and friends) -- and it returned silently. Book.SavePageToDisk's per-page fast path then reported success, SaveBookToDisk cleared the flags that say the book still needs writing, and the refused page was never written or retried. The full-save path already guarded this; only the fast path did not. So the refusal now travels: BookStorage.SaveForPageChanged and Book.SaveForPageChanged both say whether they wrote, and the fast path passes that on. With the previous commit, a refusal now also makes the next save a full one, which is what the page needs -- the per-page path names one page, and the next edit names a different one. Also corrects the AI-image-editor driving notes, which still told the reader to expect the page frame to be replaced by the save and any handle taken before the click to be stale. That stopped being true when saves stopped navigating; the advice to wait for the overlay stands, the reason for it has changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JohnThomson · 2026-09-08T16:54:37Z

[Claude Opus 5 from John Thomson's machine during preflight] Consulted Devin again on 2026-09-08, through five more review rounds, up to commit 472d4091d4. The last round raised nothing new, and every thread on this PR is now resolved.

This run existed to land the dependency the previous one was waiting on. bloom-player#441 is merged and published as 2.20.3-alpha.1, so Bloom's play-mode workaround is gone and the fault is fixed at its cause: undoPrepareActivity now works on the page it is given and only that one, so a copy taken for a save comes out with the draggables where the author put them, and the live page goes on being played.

Verified in the running app, both halves. With an item dragged to 333/444 in a game's Play tab: a gather now leaves the live item where it is (it used to snap back to the authored 60/240) and returns the authored position with no bloom-draggedToTarget (it used to return the dragged one). Driven through the real save path, the book on disk came out with the authored positions.

Seven more real defects this run, all fixed:

a stale baseline taken on resuming would have adopted anything typed while the watcher was suspended;
a failed disk write cleared the flags that say the book still needs writing;
chaining the page clicks released the queue when the request was sent rather than accepted;
an explicit save during play wrote the draggables where the tester dragged them, over the book's authored positions -- the half the workaround never covered, and the reason it had to be fixed upstream;
keeping the dirty flags after a failed write still pointed at the page that failed, so the next edit's save wrote the wrong page and abandoned the first;
the per-page write's own empty-page guard refused silently and was reported as success;
and the bloom-player contract test's skip guard had outlived the window it was written for, so it could only have hidden the regression it was meant to announce.

Four of those seven were defects in fixes made earlier in the same run. That is the argument for re-reviewing after fixing rather than once at the end.

Not fixed here, and worth knowing: bloom-player's prepareActivity sets touch-action: none on every draggable (BL-14502) and undoPrepareActivity never removes it, so entering a game's Play tab once and then saving writes that into the book permanently. Pre-existing rather than caused by this PR -- the old destructive save read the live page after the same undo -- and raised with the developer as a possible second bloom-player change.

Gates on this HEAD: C# 3374 passed, front-end 854 passed (including the four bloom-player contract cases, which now run rather than skip), typecheck and lint clean, pr-automation green. One C# run failed SafeXmlTests.Xml_DoesNotProvide_ThreadSafety, which deliberately races raw XmlDocument to prove it is not thread-safe and fails when the race does not trip; it is already marked SkipOnTeamCity as flaky and passes three for three on its own.

…gs (BL-13502) Review of the whole branch turned up several things worth tidying before it goes to a human reviewer: - SaveCurrentPageAndBook wrote through Book.Save() directly, bypassing SaveBookToDisk, so after leaving the tab or quitting the dirty flags stayed set and the next save wrote the whole book again. It now goes through the same merge + SaveBookToDisk pair as every other save, which returns bool and clears the flags it acted on. It also absorbs SavePageInPlace (Copy Page and the AI image editor were the only users) and requires the Editing state, as the state-machine route always did. - Null content now means what it says. A null snapshot no longer forces a full write; SaveBookToDisk writes only if the merge found a change, a caller said its action changes the book, or a data-div change / forced full save is waiting. Leaving the tab or quitting with an untouched book writes nothing. Copy Page no longer forces a full save either, since copying changes nothing. - The Refused outcome was unreachable: ReloadCurrentBookDiscardingEdits clears _havePageToSave in the same breath as setting the reload flag, and that test came first. Gone, with the guards that produced it. - savePageWithoutReloading and the editView/savePageInPlace endpoint had no production caller. Gone. - "InPlace" no longer distinguished anything: ToSavedInPlaceThenNavigating is SaveThenNavigate, InPlaceSaveOutcome is SaveOutcome, and SavePageInPlaceThen is folded into MergeCurrentPageThenSave, its only caller. The page-content string is pageContent at every layer. PageSelectedChanged is a typed event and always carries its args. The page-loaded notification is JSON like the other APIs. - The duplicate <summary> blocks on SavePageToDisk and SaveForPageChanged, two stray BOMs, three copies of the could-not-save message, and two identical catch blocks in the state machine are each one thing now. - Comments: stale references to the ask-the-browser path and to SaveThen are fixed; the removed closing protocol, the late-load-id hazard, the actionChangesTheBook trap and the snapshot-clearing rationale are each told once, with pointers; discovery narrative and measurement tables that the design doc already holds are cut. The doc's "What changed" section now describes the code as it is. - The two CDP bench scripts measured a round trip that no longer exists, so they are removed; the numbers they produced stay in the design doc. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The comment named the removed editView/pageContent callback; the live editor now posts to editView/pageSnapshot, and the reason for not reusing it is the same. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ut-reload

…ecord the created event (BL-13502) The review of the previous commit found two narrow regressions in making SaveCurrentPageAndBook require the Editing state: leaving the tab or quitting while a page was still loading no longer retried a write that the navigation's own save had failed, and a brand-new book quit at that moment lost its "created" history entry. Mid-navigation there is nothing to merge -- the page we left was saved before the navigation began -- but whatever that write left behind is now written, and the history entry no longer depends on the save having happened. Also says, where the state machine writes the book, why a failed write still navigates: the action has already changed the book in memory and the page list already shows it, the user has been told, and the flags keep the retry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…y sent (BL-13502) The baseline gather waits for the page's load-time work to finish, and the user can start typing before it does. The baseline then already contained that typing, and treating it as "already sent" meant nothing ever posted it: the follow-up snapshot matched the baseline and stayed quiet, so quitting wrote what C# held. Now a baseline taken while the page was changing counts for nothing, and the follow-up posts whatever is there -- one redundant post on an untouched page, which C# finds unchanged. Devin found this. Also brings the AI image editor folder's AGENTS.md up to date: it still told the reader that the editor waits for a post-save page load, which this branch removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JohnThomson · 2026-09-08T20:25:38Z

[Claude Opus 5 from John Thomson's machine during preflight] Consulted Devin on this run's three pushes, up to 74938b6.

This run applied the branch review John asked for (one save path, no unreachable outcomes, shorter names, fewer retellings), merged master, and re-verified. Devin ran on each push:

On the tidy-up commit it raised one bug: a failed disk write still navigates. That is deliberate and long-standing (the action has already changed the book in memory and the page list shows it; the flags keep the retry), now said in the code and on its thread.
On the merged head it raised two bugs and two flags. One bug was real and is fixed: a page-snapshot baseline read while the user had already started typing counted that typing as sent (thread). The other is the accepted freshness trade at exit (thread). Of the flags, the stale AI-editor folder guidance is fixed (thread) and the shutdown edit-loss window is the same accepted trade (thread).
On the final head it raised nothing new.

All 77 review threads are resolved. pr-automation passed on every push. Full suites on the tidied tree: C# 3370 passed, front-end 854 passed; the Edit and Book fixtures and the snapshot spec were re-run after the later commits.

…ve (BL-13502) Work whose result belongs in the saved page -- sizing an image, settling a paste -- registers in the browser's delay register, and every gather in the browser waits for the register to empty. A save made from the snapshot could not: the snapshot C# held simply predated the work, so leaving the Edit tab or quitting while an image was still being sized wrote the page without it. The browser now tells C# when the register goes busy, naming the work (editView/pageBusy), and when it is idle again (editView/pageIdle) -- and says idle only after it has posted the finished page, so that "idle" also means "you already have it". A refused busy notice is offered again while the work goes on, like a refused snapshot. On the C# side a snapshot-based save waits for the idle notice by sleeping the UI thread, for at most four seconds (the browser's own cap on waiting for the register); the two notices, like the snapshot itself, arrive on server threads, so the sleep does not stop them. No new asynchronous protocol. If the wait runs out the save goes ahead and the log records what the page was still busy with, so a report of a lost change can be read against it. Tests: PageSnapshot waits, gives up naming the work, is released by the idle notice, ignores notices from other loads and forgets busy on navigation; the delay register reports only its transitions; the snapshot module posts busy with the work's id, posts the finished page before idle, and re-offers a refused busy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…13502) The idle notice promised that C# already held the page as it is after the work, but kept that promise only when the snapshot run under way had been parked behind the register. A run that had read the page BEFORE the work began and was sitting in its post satisfied the wait too, and what it sent predated the work; the corrected snapshot then arrived after the save, or after the navigation had cleared it. Now the idle notice always gathers once more after any run in flight, which posts only if the page differs from what was last sent. A test pins the ordering with a post already in flight. The wait also has a limit worth stating: work that calls a C# API on the UI thread cannot finish while the UI thread sleeps, and when the save is inside an API handler holding the sync lock (leaving the tab from the tab bar), neither can work that calls any synchronised API. Then the wait runs out and the save behaves exactly as before the wait existed, with the culprit logged. The cap is two seconds rather than four, so that case costs a pause rather than a freeze; the comments and the design doc say so. Found by the light review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JohnThomson · 2026-09-09T22:01:05Z

[Claude Opus 5 from John Thomson's machine during preflight] Consulted Devin for the busy/idle wait, up to e3c607c.

This run's second part added the wait for in-flight page work John asked for: the browser tells C# what the page is busy with and when it is idle again (saying idle only after posting the finished page), and a snapshot-based save sleeps briefly for that, logging the culprit if it gives up.

Devin's jobs for both commits (0ce8e13, e3c607c) errored within seconds of starting, marked retryable with no message; the first was triggered by pr-automation, the second by loading the review page by hand, and a second load did not produce a new job. So these two commits are not Devin-reviewed. A later preflight run will fold in its round when the service takes the PR again.
The light review covered them instead. It found one real ordering hole, fixed in e3c607c with a test: a snapshot already in flight when the work finished could satisfy the wait with content from before it. It also described the limit of the sleep (work that needs the UI thread or the sync lock the saver holds cannot finish during it), now documented in the code and the design doc, with the cap shortened to two seconds and the question put to John.

All 77 review threads remain resolved. pr-automation passed on every push it ran for; GitHub did not fire it for the final push. Full suites on the busy/idle commit: front-end 858 passed; C# 3391 passed with 6 failures in the subscription-tier tests, whose fake codes expired on 8 September and which fail identically on master today.

…ut-reload # Conflicts: # src/BloomExe/Edit/PageThumbnailList.cs

Devin found three ways the busy/idle notices could fail to do their job. The snapshot module subscribes to the delay register after bootstrap(), when the load-time work has usually already registered, and a listener that heard only transitions missed all of it; the register now tells a new subscriber its state at once. A busy notice whose post failed outright (no reply, which is what a failed post looks like) was never offered again, so a save made meanwhile did not know to wait; it is now retried like a refused one. And an idle notice that was not taken left C# believing the page busy until the next navigation, so every snapshot-based save in between sat out the whole wait; it too is offered again while the page stays idle. Three tests, one for each. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…e (BL-13502) The busy and idle notices are separate, unsynchronised requests, so nothing guarantees C# processes them in the order they were sent. Devin pointed out the consequence for the idle notice's retry -- it could land after the busy notice for work that began in the meantime, and C# would then save in the middle of that work -- but the same is true of any pair. The browser now numbers every notice, and C# ignores one whose number is below the latest it has acted on, answering it as taken so that it is not offered again. The numbering restarts with each page load, as C#'s record of it does. Tests on both sides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JohnThomson · 2026-09-10T14:40:42Z

[Claude Opus 5 from John Thomson's machine during preflight] Consulted Devin on the 10th, up to 95286d4.

John asked for the PR to be brought up to date and re-tested, and for Devin to be tried again after yesterday's errored jobs. Master was merged (150 commits; one conflicting line, where master removed the page list's Enabled gate for BL-16809 and this branch changed the same call's signature). Both full suites are green on the merged tree: C# 3435 passed, including the subscription-tier tests that failed yesterday on expired fake codes, which master renewed; front-end 859 passed.

Devin ran three times and finished each time:

On the merge commit: three real bugs in the busy/idle notices, all fixed in 64a3241 with tests (already-registered work went unreported, a failed busy post was never re-offered, a lost idle notice left C# believing the page busy), and one flag restating the sleep's known limit (thread).
On that fix: one more real bug, an idle notice processed after the busy notice for newer work, fixed in 95286d4 by numbering the notices (thread).
On the final head: nothing new. One bug and one flag are re-titlings of settled threads (the deliberate 100 ms window in the context-menu handler; the accepted freshness window at shutdown), answered as such.

All 84 review threads are resolved. pr-automation passed on every push today.