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

Gathering a page's content for a save used to wreck the live page: the browser stripped the
editing markup out of the real DOM (detached the toolbox tool, removed origami layout mode and
text-box labels, killed the niceScroll bars, rewrote every bloom-editable with CKEditor's cleaned
data). What was left could be saved but not edited — which is exactly what the SavedAndStripped
state records, and why every save had to end by navigating somewhere.

This makes the gather non-destructive, and then starts spending that.

The core change

getBodyContentForSavePage() now clones the body and cleans the clone. Nothing at all happens
to the live page. Getting there needed:

comicaljs 0.4.1 for Comical.exportSvgToCopiesOfParents — the non-destructive counterpart of
stopEditing(), added for this in comical-js#120. (Moving off 0.3.106 also surfaced four
pre-existing type errors: 0.3.106's declarations used a bare from "bubbleSpec" specifier, so
BubbleSpec had silently been any.)
Our own niceScroll cleanup (niceScrollCleanup.ts) for the clone, instead of asking the live
scrollbars to remove themselves.
One ITool.removeToolMarkup(pageOrClone) used two ways rather than a duplicated pair, with a
console error if a detachFromPage override forgets its super call — because the symptom
otherwise shows up much later, as tool markup saved into a book.
CKEditor's data is read from the live editors and written into the clone.
We no longer blur the active element on save, so the cursor stays put.

What that buys, so far

SaveThen now takes the current page's content when the caller can send it, and does the whole
save in one step instead of asking the browser and waiting for the answer on a separate API. Every
command the page list initiates does this: page click, duplicate, delete, paste, reorder, plus
Change Layout / import video / convert-to-derived-field.

Copy Page no longer reloads at all — it was reloading the very page it was copying, purely to
recover from the destructive save.

Removing the round trip also closes a real hole: while C# sat in SavePending waiting for the
browser, a second page click was silently discarded.

Safety

Everything that gathers page content now waits on one gate, whenNoActiveDelays()
(pageContentDelays.ts), so a save can't read a page mid-change. The synchronous gather is no
longer exported anywhere, so there is no way around it.
SaveThen is the only way in; the rule that only a Declined outcome may fall back lives in one
place. Getting that wrong deletes a page twice — which it did, once, during development.

Testing

Driven against a real book in a running Bloom: every command saves the page's unsaved typing,
lands on a fully set-up page (CKEditor attached, canvas elements present), reaches disk, and never
fires editView/pageContent. Holding a delay stops a command from starting at all; a delay that
never clears lets it through after the 4s cap with a warning.

New unit tests: 21 for the state machine, 9 for the delay gate, 9 for niceScroll cleanup.

Note for reviewers

This is deliberately not merging into the current release. It is written to be cheap to merge
later — new behaviour in new files, and no reshaping of existing code just to add to it — because
what conflicts is a changed line, not an added one. src/BloomExe/Edit/SavingWithoutReloading.md
explains that, what has been converted, and what is left.

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

…13502) Gathering a page's content for a save used to **wreck the live page**. The browser stripped the editing markup out of the real DOM: it detached the toolbox tool, removed origami layout mode and the text-box labels, killed the niceScroll bars, and rewrote every bloom-editable's innerHTML with CKEditor's cleaned-up data. What was left could be saved but not edited -- which is exactly what the SavedAndStripped state records, and why EVERY save had to end by navigating somewhere. getBodyContentForSavePage() now CLONES the body and cleans the clone. Nothing at all happens to the live page. Getting there needed: - comicaljs 0.4.1, for Comical.exportSvgToCopiesOfParents -- the non-destructive counterpart of stopEditing(), added for this in comical-js#120. Moving off 0.3.106 also surfaced four pre-existing type errors: its declarations used a bare `from "bubbleSpec"` specifier, so BubbleSpec had silently been `any`. - Our own niceScroll cleanup for the clone, instead of asking the live scrollbars to remove themselves. It handles the rails and cursors, the alignment classes addScrollbarsToPage() moves aside (the part that would have been real data loss), and the three inline styles niceScroll sets without recording. - One ITool.removeToolMarkup(pageOrClone), used two ways rather than duplicated: the save path calls it on a clone, and detachFromPage() calls it on the live page. detachCurrentTool() logs an error if an override forgets its super call, because the symptom otherwise shows up much later as tool markup saved into a book. - CKEditor's data read from the live editors and written INTO the clone. - No blurring of the active element, so the cursor stays where the user left it. On top of that, SaveThen now takes the current page's content when the caller can send it, and does the whole save in one step instead of asking the browser and waiting for the answer on a separate API. Every command the page list initiates does this -- page click, duplicate, delete, paste, reorder -- as do Change Layout, importing a video, and converting a field to a derived one. Those three keep their reload, which is doing a second job for them: they have restructured the page in ways that have never been through SetupElements. Copy Page no longer reloads at all. It was reloading the very page it was copying, purely to recover from the destructive save. Removing the round trip also closes a real hole: while C# sat in SavePending waiting for the browser, a second page click was silently discarded. Safety: - Everything that gathers page content waits on one gate, whenNoActiveDelays(), so a save cannot read a page mid-change. The synchronous gather is not exported anywhere, so there is no way around it. That gate also means the *command* does not start mid-change: C# is not asked to duplicate or delete anything until the page has settled. - SaveThen is the only way in, so the rule that only a Declined outcome may fall back lives in one place. Getting that wrong deletes a page twice -- which it did, once, during development. Reviewed by Devin over several rounds, which found six real defects in this work that neither the tests nor driving the UI had caught: the reader tools' editing highlight being saved into the book; a save that never happened reported as successful; toolbox tools no longer shut down when leaving a page (and, later, when leaving the tab); a page-list command that could vanish silently if the page frame navigated; "leave the editor blank" ignored; and balloon data rewritten on pages where it used to be left alone. All are fixed, each with a documented and resolved thread on the PR. Also from review: the Talking Book tool's cleanup now undoes the audio highlighting structurally rather than restoring a snapshot taken when playback started, so typing done during playback is no longer thrown away -- and the phrase-delimiter enshrouding survives it, which the snapshot restore had been destroying. New tests: 30 for the editing state machine, 9 for the delay gate, 9 for niceScroll cleanup, 6 for the audio highlighting undo, 4 for reader-markup removal, 1 for a refused save. src/BloomExe/Edit/SavingWithoutReloading.md explains the design, what has been converted, what has not and why, and the risks to watch when converting more. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Dropping the 100ms deferral before a thumbnail context-menu command went too far: I removed the deferral itself, not just the delay, and ran the command inline in the API handler. That is unsafe. "Duplicate Many Times" and "Choose Different Layout" open MODAL dialogs, and those dialogs' content is served by this same Bloom server -- while the handler still holds the API sync lock, because it has not returned. Running them inline invites a deadlock. So the command is queued again, with BeginInvoke rather than a delayed Task: we are already on the UI thread, so that just puts it after the current message. It returns at once, we reply, the lock is released, and then the command runs. The 100ms window that could have left the user's latest typing out of the snapshot is still gone, which was the actual point. Found in this run's own local review, before it reached anyone else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

**An id that is not a valid CSS identifier would abort the whole save.** undoHighlightingFixes looked its elements up with `querySelector("#" + id)`, which THROWS for an id that is not a valid CSS identifier -- a legacy one starting with a digit, say. That pattern is older than this branch, but its blast radius is not: it now runs inside the save's clone cleanup, so a throw would abort gathering the page and we would post an error string instead of the user's content. It now compares the id property, which cannot throw whatever the id looks like. **The AI image editor stayed silent on two of the three ways its save can fail.** The check added earlier covers C# refusing, but not the page frame having gone away (the optional call then yields nothing at all) and not the request itself failing. All three now say the same thing, which is the part that matters to the user: the book on disk still has the old image. Two tests. **A test kept a comment claiming an assertion it no longer had.** The partial-failure case in the AI editor tests still said "What did land still gets saved" but had lost the assertion when the save moved off postThatMightNavigate. Restored, against the current API. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The message for a save that did not happen asserted that Bloom had declined. It cannot know that: a failed request does not reject, because bloomApi's wrapAxios catches it (and reports the network error itself), so it arrives looking exactly like a refusal. The message now says what is true -- the page was not saved -- and leaves the cause open. Found by Devin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

They resolved playwright through a hard-coded C:/github/BloomDesktop, so they threw immediately for anyone whose repo lives elsewhere -- which is everyone but me. They now locate the repo from their own file position, and take the CDP port from BLOOM_CDP_PORT when the launcher picked a different one. Found by Devin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

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

Both of John's calls on the preflight report. **The 100ms deferral is back**, with its real reason written down this time. Returning from the handler is not enough on its own: the server thread releases the API sync lock a moment AFTER we return, while the UI thread is already free to pump whatever we queued -- so a modal dialog could ask this same server for its content while the lock is still held. The delay makes that ordering certain rather than merely likely. Nobody had recorded that; removing it is what brought it to light. **Copy Page selects the page it copied again** -- but only when it isn't already selected, which is the case that actually matters. Selection is what a later Paste inserts after (DeterminePageWhichWouldPrecedeNextInsertion starts from the current selection), so leaving the selection elsewhere would have put the pasted copy somewhere the user did not ask for. That is a better reason than the one I gave for dropping the navigation, and I had missed it. Copying the page you are already on -- the overwhelmingly common case, since a right-click does not select -- still does not navigate, so that keeps the win: copying a page no longer reloads it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ut-reload

My comment said a right-click does not select the page, so Copy Page had to navigate in that case. John pointed out that is wrong: the page list opens its context menu only on the selected page, and I had read handleContextMenu without following the call into openContextMenu, where the guard lives. So the copied page is always the selected one, the in-place branch is what runs, and copying a page does not reload it. The navigating branch stays as a safety net -- the copied page must end up selected for a later Paste to land where the user expects -- and the comment now names the guarantee it depends on and where that guarantee is enforced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

All three are mine, from the in-place-save work on this branch. 1. An external process replacing the book on disk could still get its work clobbered. SavePageInPlaceThen returned Declined for that case, and SaveThen treats Declined as "nothing happened, so do it the long way" -- and the long way, asking the browser for the page content, has no such guard and would write the page anyway. Added a distinct InPlaceSaveOutcome.Refused so the guard survives the fallback. Declined and Refused look alike (nothing was written, doBeforeSaveToDisk did not run) and mean opposite things to the caller, which is why they are separate values. 2. Leaving the Edit tab only shut the toolbox tool down. Changing pages runs the page frame's whole pageUnloading() teardown, but leaving the tab does not unload or re-navigate that frame, so the page kept the controls above it and the canvas-element machinery as well. Call pageUnloading() instead of just removeToolboxMarkup(). 3. undoHighlightingFixes stripped a book's OWN ui-enableHighlight spans and ui-disableHighlight classes, not just the temporary ones the Talking Book tool adds -- a book can legitimately carry them, even nested inside a sentence the tool has touched, and now that this runs on every save that was silent data loss. Our spans are stamped data-bloom-temp-highlight and only those are unwrapped; the no-highlight class comes off only where we were the ones who put it on (tracked per element, keeping the first answer so Play -> Pause -> Play does not misread our own class as the book's). An attribute rather than a JS-side record of the elements because the undo also runs against a CLONE of the page, whose elements are different objects. Tests: two new ones covering a book's own highlight span nested inside a fixed sentence, and a book that already had the no-highlight class. The existing fixHighlighting expectations gain the marker attribute, which is what they now produce. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Devin found the one hole left in _runningSaveInPlaceAction. An action passed to ToSavedInPlaceThenNavigating runs while the machine is still in Editing, and ToSavePending's Editing branch accepted a nested request: it flipped the state to SavePending and asked the browser for the page again. When the action returned, the ToNavigating that follows it is ignored from SavePending, so the page the action promised to go to was never shown, and the browser's eventual reply merged and saved the page a second time. ToSavePending now refuses while _runningSaveInPlaceAction is set. That is what the old flow got for free by running the action in SavedAndStripped, where this method already returns false. Refusing rather than deferring is right: inside the action the browser's content is already merged into the book DOM and _saveBook() is about to run, so a second save has nothing to do, and both callers on this path pass an empty "wrong state to save" handler. Reachable from PageThumbnailList.PageMoved, whose action raises RelocatePageEvent and then changes the page selection, which goes to PageListController.OnPageSelectedChanged and calls SaveThen with no content. Narrower than it first looks: on a successful move OnRelocatePage calls RefreshDisplayOfCurrentPage, which navigates, so the state is already Navigating and the nested save was being ignored anyway. The window is the info.Cancel case, a relocation the book refuses. ToNavigating and ToNoPage already had their matching rules; this was the only one of the three without. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

JohnThomson · 2026-08-19T17:41:56Z

+                if (
+                    SavePageInPlaceThen(pageContentFromBrowser, doBeforeSaveToDisk, forceFullSave)
+                    != InPlaceSaveOutcome.Declined
+                )


[Devin] Bug: Clicking a page or using a page command does nothing at all after another program has changed the book

A page command is abandoned without doing anything and without telling the caller when the book on disk has been replaced by another program, so the user's click appears to be ignored with no message and no page change.

Impact: after an outside program rewrites the book, clicking a thumbnail (or Duplicate/Delete/Paste/reorder) silently does nothing until the user leaves the Edit tab, with no explanation.

Why the Refused outcome is dropped rather than handled

SavePageInPlaceThen returns InPlaceSaveOutcome.Refused when _reloadFromDiskOnLeavingEditTab is set, deliberately distinguishing it from Declined so the caller will not fall back to the ask-the-browser route (which would clobber the other program's file). But SaveThen treats every non-Declined outcome the same: it simply returns. So for Refused (and for Failed) neither the save, the action, the navigation, nor doIfNotInRightStateToSave happens.

For a plain page click (PageListController.OnPageSelectedChanged, whose action is just () => pageId) there is nothing dangerous about navigating — only about saving — yet the navigation is dropped too. Previously the same click went through ToSavePending and always ended in a navigation.

A reasonable fix is to let SaveThen distinguish the outcomes: on Refused, skip the save but still honour the caller's intent (e.g. navigate).

Accurate about the code path, and I am leaving it open for the developer rather than acting on it, because the fix Devin proposes is a user-facing behaviour choice in an already-degraded state.

Two things narrow it a lot. First, the premise is largely designed away: the only place _reloadFromDiskOnLeavingEditTab is set (EditingModel, in the external-change handler) immediately follows it with _view.WorkspaceView.ChangeTab(Workspace.WorkspaceTab.collection) — Bloom deliberately kicks the user out of the Edit tab, precisely because reloading under a live editor proved fragile. So the user is not normally sitting in the Edit tab clicking thumbnails in this state; the window is whatever gap exists before the tab actually changes.

Second, the suggested "skip the save but still navigate" is only safe for the callers whose action is pure. For a plain page click the action is () => pageId and navigating would be harmless. But Duplicate, Delete and Paste express their whole effect in that action — running it is what changes the book — and that is exactly what must not happen when the book on disk is about to replace ours. So there is no single "honour the caller's intent" rule; it would need to distinguish page-changing callers from book-changing ones.

Worth noting the previous behaviour was not a better one to return to: before this fix the same click fell through to the ask-the-browser path and overwrote the other program's file, which is the bug this Refused outcome exists to stop. Doing nothing is strictly safer than that.

The cheap middle option, if the developer wants one, is to call doIfNotInRightStateToSave() on Refused so the caller at least learns the command was dropped. Every current caller passes an empty handler, so it would change nothing visible today, but it would stop the silence being structural.

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.

JohnThomson · 2026-08-20T18:56:10Z

[Claude Opus 5 from John Thomson's machine] Manual verification: reordering pages preserves unsaved changes.

John tested this by hand, which closes the one functional gap left open in the consultation log above.

It matters because page move is the path the last fix on this branch touches. PageThumbnailList.PageMoved passes an action that changes the page selection, which reaches PageListController.OnPageSelectedChanged and asks for another save from inside the save-in-place action. ToSavePending now refuses while that action is running (62117e6), so the navigation that follows it is no longer dropped — and the edits gathered before the move are not lost.

Why it had to be tested by hand: page reorder cannot be driven over CDP. Synthesized mouse events do not engage react-grid-layout's react-draggable, so the drag either does nothing or onDragStop sees unchanged grid coordinates and treats it as an ordinary click. Two attempts with three numbered pages present moved nothing and reported no error; an earlier attempt with two pages instead hit the "front matter and back matter pages must remain where they are" dialog, because the target slot sat in the column next to the Credits Page. This is now recorded in the bloom-automation skill so the next agent does not repeat it.

Still untested anywhere: typing during Talking Book playback. It needs a book with recorded audio, and every such book in this collection lives in a different Bloom collection. The change itself is covered by four unit tests in audioRecordingSpec.ts, including two added for the cases that were previously untested (a book's own highlight span nested inside a sentence the tool fixed, and a book that already carried the no-highlight class).