Skip to content

Save a page without reloading it; save from a snapshot the browser volunteers (BL-13502) - #8209

Open
JohnThomson wants to merge 1 commit into
masterfrom
BL-13502-save-without-reload
Open

JohnThomson wants to merge 1 commit into
masterfrom
BL-13502-save-without-reload

Conversation

@JohnThomson

@JohnThomson JohnThomson commented Aug 14, 2026 •

Copy link
Copy Markdown
Contributor

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 page element (nothing else is saved, so nothing
    else is copied), 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, so that visiting every page
    of a book, front and back matter included, writes nothing:
    • 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;
    • a text-measuring scratch element can no longer be written into the book;
    • attributes that come back in a different order no longer count as a change;
    • the cover's editing-only ?transparent=yes image parameter no longer makes the book-wide data
      look changed, which forced a full save on every visit;
    • optional branding images that are not there no longer get the "missing image" text written into
      book-wide data (another full save on every visit to the back cover), and merging classes from the
      data div no longer leaves a stray space.
  • 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, and a gather that throws, are reported to the user rather
    than 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 snapshot of the finished page, and logs the culprit if it has to go ahead anyway.
    The browser sends its snapshots and these notices one at a time, in order, so nothing needs
    numbering or retrying.
  • No request carries the page. Every save uses the snapshot: the page list's clicks, commands and
    moves send only the command, and the few commands that restructure the page and then reload it
    (Change Layout, importing a video) first send the snapshot at once and wait for C# to take it. A Windows
    shutdown is the exception: Windows allows only a few seconds and the browser is going away too,
    so the exit save takes the snapshot as it stands, and Bloom tells Windows it is saving.
  • Features that cleaned their markup off the live page as part of saving do it on the copy instead:
    Bloom Tables (removeTableEditingMarkupFromClone) and Comical's drawing surfaces. The two
    commands of the editable original-copyright sentence (unlock and relock) reload the page, so
    like Change Layout they first send the page to C#.
  • 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.

Also included, because it kept failing this PR's test runs: Xml_DoesNotProvide_ThreadSafety, a
flaky test from master, is made deterministic (its threads no longer sleep between operations, so
a collision no longer depends on luck).

Risk Evaluation

This changes the path every save takes, so the risk is in shared code, not in a new corner:

  • Every save now comes from the snapshot the browser sent: changing pages, every page-list
    command, Duplicate/Delete, leaving the Edit tab, closing the collection, quitting, and a Windows
    shutdown. If a snapshot is missing or stale, the save writes the wrong thing. The places to look
    hardest are pageSnapshot.ts (when a snapshot is sent, the busy notice, the one-at-a-time queue)
    and PageSnapshot.cs / EditingModel.TakeCurrentPageSnapshot (which load is believed, and the
    bounded wait while the page is busy).

  • The gather now runs after every change (25 ms after typing settles), so
    getPageHtmlForSave and everything it calls must stay fast and must never touch the live page.
    That includes every toolbox tool's removeToolMarkup, Comical's export into the clone, and
    CKEditor's text.

  • C# now skips writing a page that did not change. A command that changes the book itself has
    to say so (actionChangesTheBook), or its change is not written. All current callers have been
    checked; a new caller could get this wrong.

  • The editing state machine is simpler: two states and the ask-the-browser round trip are gone.
    But MergeCurrentPageThenSave now runs the caller's action between the merge and the write,
    and that action may navigate.

  • Table markup is stripped by a copy of bloom-table's list. bloom-table's own cleanup also
    ends Paint Format and Border Brush mode, which a save that runs on every change must not do,
    and it does not export its markup-only part. A unit test checks that Bloom's copy strips what
    the library's does, but a new kind of edit-time markup in a later bloom-table would need adding.

There is one deliberate trade. A click, command or exit within about 25 ms of a keystroke saves the
snapshot from just before that keystroke (see "The freshness window" in
SavingWithoutReloading.md).

Ecosystem Impact

  • Saved books: the format is unchanged; the page saved is the same element as before. Fewer
    editor leftovers now reach the file (qTip attributes, Comical's regenerated SVG ids). A branding
    image that is missing keeps the alt its branding gave it instead of the editor's "missing image"
    text. Older Bloom versions open the books as before.
  • Bloom Player / BloomPUB / ePUB / PDF: these read the saved book, which is unchanged apart
    from those leftovers. This PR needs bloom-player 2.20.3-alpha.1 (bloom-player#441), which is
    already in the lockfile. Otherwise saving a game page being played would undo the tester's drags.
  • Localization: one new string, Shell.SavingBeforeWindowsShutsDown ("Bloom is saving your
    book"), in BloomLowPriority.xlf. Windows shows it while Bloom finishes saving during a shutdown.
    It is marked translate="no", the team's pending state, so it reaches translators the next time
    pending strings are released.
  • Not checked: publishing output was not compared byte for byte before and after.

E2E Coverage

New specs for this PR, all passing on the final build:

  • typing-survives-leaving-the-page.spec.ts: typing reaches the file when the user clicks another
    page, duplicates the page, moves a page, leaves the Edit tab, or quits Bloom (from each tab).
  • saved-book-stays-clean.spec.ts: visiting pages without editing writes nothing, content pages
    and every xmatter page alike; editing with tools open leaves no editor leftovers.
  • change-layout-keeps-typing.spec.ts: leaving Change Layout keeps both the new split and the
    typing, including typing done just before.
  • duplicate-page-many-times.spec.ts: the dialog's OK, Cancel, and the locked tabs.

Existing specs that exercise the changed code, still passing: duplicate-page.spec.ts,
copy-page.spec.ts (it waited for Copy Page to reload the page, which it no longer does; it now
waits for Paste Page to become enabled), workspace-tabs.spec.ts, toolbox-tools.spec.ts,
talking-book-paste.spec.ts, rotate-and-flip-images.spec.ts, capture-book-page.spec.ts.

Specs that came from master and exercise the changed code, also passing: the three table specs
(tables-core, tables-extended, tables-gating) and original-copyright-sentence.spec.ts. Two
needed adjusting: tables-extended waited for Copy Page to reload the page, and now waits for Paste
Page to be enabled; original-copyright-sentence typed and left the page at once, and now waits
until Bloom has the typing, as the other typing tests do.

Supporting changes: a test-only hook to wait until Bloom has the typing, a fixture method that
quits Bloom the way a person does, and a wait for a page to settle.

Not covered by e2e, because they depend on timing the fixture cannot control: a click straight
after a keystroke, a Windows shutdown, the wait while the page is busy, importing a video, and
clicks dropped while a page loads. Unit tests cover them (pageSnapshot.spec.ts,
PageSnapshotTests, EditingStateMachineTests).

Notion Test Suite

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

Devin review


This change is Reviewable

Comment thread src/BloomBrowserUI/bookEdit/toolbox/readers/leveledReader/leveledReaderTool.tsx Outdated
Comment thread src/BloomExe/web/controllers/EditingViewApi.cs
Comment thread src/BloomBrowserUI/bookEdit/js/bloomEditing.ts Outdated
Comment thread src/BloomExe/Edit/EditingStateMachine.cs
Comment thread src/BloomBrowserUI/bookEdit/js/bloomEditing.ts Outdated
Comment thread src/BloomBrowserUI/bookEdit/js/bloomEditing.ts
Comment thread src/BloomBrowserUI/bookEdit/pageThumbnailList/currentPageContent.ts Outdated
Comment thread src/BloomBrowserUI/bookEdit/pageThumbnailList/currentPageContent.ts Outdated
Comment thread src/BloomBrowserUI/bookEdit/js/canvasElementManager/CanvasElementManager.ts Outdated
Comment thread src/BloomBrowserUI/bookEdit/js/bloomEditing.ts Outdated
@JohnThomson

Copy link
Copy Markdown
Contributor Author

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

Comment thread src/BloomExe/Edit/EditingStateMachine.cs Outdated
Comment thread src/BloomBrowserUI/bookEdit/js/bloomEditing.ts Outdated
@JohnThomson
JohnThomson force-pushed the BL-13502-save-without-reload branch from c5d0e0f to 37d206b Compare August 17, 2026 21:58
Comment thread src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.ts Outdated
Comment thread src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts Outdated
Comment thread src/BloomBrowserUI/bookEdit/aiImageEditor/aiEditorOverlay.test.ts
Comment thread src/BloomExe/web/PageListApi.cs
Comment thread src/BloomExe/Edit/EditingModel.cs Outdated
Comment thread .claude/skills/run-bloom/benchPageChange.mjs Outdated
Comment thread src/BloomBrowserUI/bookEdit/js/bloomEditing.ts Outdated
@JohnThomson

Copy link
Copy Markdown
Contributor Author

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

Comment thread src/BloomExe/Edit/EditingModel.cs Outdated
Comment thread src/BloomExe/Edit/EditingView.cs
Comment thread src/BloomBrowserUI/bookEdit/toolbox/talkingBook/audioRecording.ts Outdated
Comment thread src/BloomExe/web/PageListApi.cs Outdated
Comment thread src/BloomBrowserUI/bookEdit/js/bloomEditing.ts Outdated
Comment thread src/BloomExe/Edit/EditingStateMachine.cs Outdated
Comment thread src/BloomExe/Edit/EditingModel.cs Outdated
@JohnThomson

Copy link
Copy Markdown
Contributor Author

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

Comment thread src/BloomBrowserUI/pageChooser/PageChooserDialog.tsx Outdated
Comment thread src/BloomExe/Edit/EditingModel.cs
Comment thread src/BloomExe/web/controllers/AddOrChangePageApi.cs Outdated
@JohnThomson

Copy link
Copy Markdown
Contributor Author

[Claude Opus 5.5 from John Thomson's machine during preflight] Consulted Devin on the 5th, up to 7d6bc3c.

This run carried out ten of John's review points, each a simplification (see the preflight report). The largest moved the page list's Duplicate Many Times and Choose Different Layout dialogs into the browser, so every right-click command runs inside its request and stays in order without the old 100 ms deferral.

Devin ran four times on that change and its follow-ups:

  • The in-browser duplicate dialog did not lock the workspace (thread), and a page click still queued when a dialog opened could change the page it acted on (thread). Fixed in ddc33cd: the dialog locks the workspace like the others, and each dialog tells C# which page it was opened for.
  • The chooser still set up a template's tool when C# had ignored the change (thread), and an e2e helper sent no page (thread). Fixed in 3335c55.
  • A declined save still reported the layout as changed (thread). Fixed in 7d6bc3c.
  • The round on 7d6bc3c raised nothing.

On the final head: C# 3678 passed, 1 skipped; front-end 1021 passed on 3335c55 (the last commit is C# only). Xml_DoesNotProvide_ThreadSafety, master's test, failed in two of five full C# runs today; it is untouched by this branch and is reported rather than worked around. All 108 review threads are resolved. pr-automation passed on every push.

@JohnThomson

Copy link
Copy Markdown
Contributor Author

[Claude Opus 5.5 from John Thomson's machine during preflight] Consulted Devin on the 6th, up to b0b3dd1.

Driving Bloom on the 6th found a regression from the master merge of 1 October: this branch's page gather (copyCkEditorDataToClone) still called removeCkEditorFillingChars, which master's BL-16843 removed, so every gather of a page with a text box threw ("A JavaScript error occurred" on opening the Edit tab) and typing was not saved. b0b3dd1 uses getData() as it comes, as master does, with two tests that fail on the old code. The repo's typecheck gate tolerates missing-member errors, which is how it got through; a full tsc found one more in this branch's files (an interface declaration), also fixed.

Devin's round on b0b3dd1 raised nothing new. The automatic trigger did not fire, because the branch now conflicts with master (BL-16608's toolbox cleanup), so the round was started by re-running an earlier pr-automation run; for the same reason CI has not run on b0b3dd1.

The hand test of the page-list dialogs change is complete in a running Bloom (dialogs, workspace lock, layout change, dropped click and drag, typing saved). All 108 review threads are resolved.

Comment thread src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts Outdated
Comment thread src/BloomBrowserUI/bookEdit/editablePage.ts
Comment thread src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts
Comment thread src/BloomBrowserUI/bookEdit/pageThumbnailList/pageThumbnailList.tsx
@JohnThomson

Copy link
Copy Markdown
Contributor Author

[Claude Opus 5.5 from John Thomson's machine during preflight] Consulted Devin on the 6th (about 16:10 UTC), up to 9e40ce5.

The automatic trigger still does not fire while the branch conflicts with master, so the round was started by re-running an earlier pr-automation run. Devin reviewed the whole PR afresh and raised four new things, all about the new snapshot-only saving; none assessed yet:

  • A snapshot post that fails is never resent, so a later save can write the older page (thread).
  • If the "page loaded" notice fails, snapshots start anyway and C# refuses them all (thread).
  • A "page busy" notice can arrive after a page click that should have waited for it (thread).
  • Page clicks and commands may be carried out in a different order from the user's (thread).

Every earlier finding is marked fixed by Devin, and all earlier threads were already resolved.

Comment thread src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts
Comment thread src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts Outdated
Comment thread src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts
@JohnThomson

Copy link
Copy Markdown
Contributor Author

[Claude Opus 5.5 from John Thomson's machine during preflight] Consulted Devin on the 6th, up to 29a5b94.

  • 9e40ce5 (saves use the snapshot alone): four findings. One was real and is fixed in b8c8c72: after a failed post, the next change now resends the page. The other three were answered on their threads. They covered a failure that predates this PR, a window no longer than the debounce we already accept, and page-list ordering that matches master.
  • b8c8c72: nothing new.
  • The merge with master (6656547): two real problems with sending the snapshot before a reload, fixed in 82160a3.
  • 82160a3: one more, a moment in which a snapshot read while busy looked idle. Fixed in 29a5b94.
  • 29a5b94: nothing new.

Every thread is resolved. pr-automation passed on each push.

Comment thread src/BloomBrowserUI/bookEdit/js/bloomEditing.ts
@JohnThomson

Copy link
Copy Markdown
Contributor Author

[Claude Opus 5.5 from John Thomson's machine during pr-ready-for-human] Consulted Devin up to 35b2f17 (the browser now sends only the page element). No bugs; its one flag, that the PR description still said the body was cloned, is corrected and its thread resolved. pr-automation passed.

@JohnThomson
JohnThomson force-pushed the BL-13502-save-without-reload branch from 35b2f17 to ba27292 Compare October 6, 2026 19:26

@JohnThomson JohnThomson left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnThomson reviewed 114 files and all commit messages.
Reviewable status: 0 of 114 files reviewed, 116 unresolved discussions.

@JohnThomson
JohnThomson marked this pull request as ready for review October 6, 2026 19:26
@JohnThomson JohnThomson changed the title Save a page without wrecking it, and spend that on the page list's commands (BL-13502) Save a page without reloading it; save from a snapshot the browser volunteers (BL-13502) Oct 6, 2026
@JohnThomson

JohnThomson commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor Author

[Claude Opus 5.5 from John Thomson's machine, at John's request]

Plan: e2e tests to keep this PR's saving working

Nothing here is decided yet. John will choose whether these tests go into this PR or onto a card of their own. The plan is written so that an agent without this PR's history can carry it out.

What the PR changed, in the terms the tests care about

  • Saving no longer reloads the page. The browser builds the saved page from a copy of the .bloom-page element (getPageHtmlForSave in src/BloomBrowserUI/bookEdit/js/bloomEditing.ts) and never touches the live page.

  • The browser volunteers the page. src/BloomBrowserUI/bookEdit/js/pageSnapshot.ts watches the page and posts the copy to editView/pageSnapshot:

    • once as the page loads;
    • 25 ms after each change settles.

    C# keeps the latest one (src/BloomExe/Edit/PageSnapshot.cs), and every save uses it: changing pages, page-list commands, leaving the Edit tab, closing the collection, quitting. No request carries the page.

  • Commands that restructure the page and then reload it send the snapshot immediately first: Change Layout (origami), importing a video, and making a field derived (sendSnapshotNow, used by saveChangesAndRethinkPage).

  • Untouched pages write nothing. C# compares the merged page with the book (Book.UpdateDomFromEditedPage). A page nobody changed is not written, and leaving the tab or quitting writes only what changed.

  • Each toolbox tool cleans the copy, not the live page. It takes its editing markup off whichever element it is given, through removeToolMarkup(pageOrClone) (ITool in bookEdit/toolbox/toolbox.ts): the copy for a save, or the live page when the tool is detached.

  • Duplicate Many Times and Choose Different Layout open their dialogs inside the Edit tab, not as separate windows (bookEdit/duplicateManyDialog.tsx).

  • Further reading: the design and its known limits are in src/BloomExe/Edit/SavingWithoutReloading.md. The deliberate trade is that something done within ~25 ms of a keystroke saves the snapshot from just before that keystroke.

None of the following is covered by e2e today. Seven specs touch the changed code (duplicate-page, copy-page, toolbox-tools, talking-book-paste, workspace-tabs, rotate-and-flip-images, capture-book-page), but none checks typed text against the saved file, quitting, untouched pages, Change Layout or the dialog.

Before starting

  • Read .claude/skills/add-e2e-test/SKILL.md (the procedure) and src/BloomE2E/README.md (fixture, helpers, how to run).
  • Notion. Every test must be tied to a card in the Notion "Test Case Runs" inventory, with the card's Test Case ID in the test title. Reaching the inventory needs the BLOOM_TESTCASE_NOTION token (User env scope). The section "Existing Notion cards" below maps each test to the cards that exist.
  • Running locally:
    • Quit any dev Bloom first; it stops the test's Bloom from opening its collection.
    • Fetch the test inputs once: node build/get-testing-inputs.mjs.
    • Build Bloom into output/Debug. build/agent-dotnet.sh builds to an isolated folder that the fixture does not use; the dev launcher's --restart does build to output/Debug.
    • Run with BLOOM_E2E_VITE_PORT=<a running Vite port> and BLOOM_AUTOMATION_MONITOR=headless.
    • Don't edit C# during a run, or the fixture refuses the build as stale.
  • Pause after typing. In every test that types and then acts, pause a few hundred ms, as a person would. Acting within 25 ms is the accepted window, not a bug.
  • Helpers that already exist:
    • Making and opening books: makeBookFromTemplate, editBook, goToPage, getContentPages, addPage (helpers/bookMaking.ts).
    • Typing: clickInGroup, typeInGroup (helpers/bookMaking.ts), typeWithKeys (helpers/keys.ts).
    • Pages and the page menu: selectPage, runPageMenuCommand, waitForPageCount (helpers/pageThumbnails.ts).
    • Page-list commands: duplicateCurrentPage, movePageToSlotOf (helpers/pageList.ts).
    • Tabs: switchTab (helpers/workspace.ts).
    • Reading the saved file: readBook, waitForBookWithPageCount (helpers/bookHtml.ts).
    • Change Layout: setChangeLayoutMode, splitSection, chooseSectionType (helpers/origami.ts).
    • Tools: openTool (helpers/toolbox.ts); openToolboxWithTalkingBook, importNarration (helpers/talkingBook.ts).

Existing Notion cards

Checked against the 6.5 suite run, the full inventory; 6.6 had only 15 cards when checked. The procedure for each case is in .claude/skills/add-e2e-test/SKILL.md, "Tie the test to the Notion test inventory":

  • a card the test covers fully becomes Automated;
  • a card it covers partly is split into an [Automated portion] that keeps the ID and a [Manual portion] with the next free ID;
  • a test with no card gets a new row.

Set Automation to Building when you start a card, and to PR Pending when the PR is opened.

Test in this plan Card Automation now What to do
1, quitting #659 Quit Without Losing Changes ("Quit Bloom from each tab. Make sure no changes are lost quitting from Edit tab.") not set (manual) Covered fully by test 1's quit case, if that case also quits from the Collections and Publish tabs. Mark it Automated with the test.
1, the other ways of leaving the page none found — New row.
2 #663 Clean Saved Book HTML ("Look at the html of a saved book. There should be no mention of 'ckeditor' or other user interface leftovers. Note: ignore BL-9992.") not set (manual) Test 2's leftovers check covers it. Mark it Automated with the test. Read BL-9992 first: the card says to ignore it, so the assertions must allow for it. "Untouched pages write nothing" goes in a new row, or in the same test under #663 if you put that in its title.
3 #366 Change Layout Splits (add and delete splits; choose picture, text, video, widget or nothing) Planned Test 3 covers only part of this card: one split, plus typing that survives it. Either automate the whole card, using the origami.ts helpers, or split it.
4 none found specifically — New row. (#504/#505, Talking Book segment markers, are related but not the same.)
5 #810 Duplicate Page [Manual portion]: its "Duplicate Page Many Times" step Manual Split #810 again: move the Many Times step into an automated portion with a new ID, and leave the audio and video steps manual. #349 is the automated portion that already exists for Duplicate Page.
(new) Choose Different Layout #352 Choose Different Layout ("does what it should, gives warnings, and preserves what data it can") Planned This PR moved this dialog into the browser too, and made it act only on the page it was opened for. Worth automating alongside test 5.

Already automated and touching this PR: #348 Copy Page Preserves Everything (copy-page.spec.ts, adjusted in this PR) and #349 Duplicate Page [Automated portion].

1. Typing survives every way of leaving the page (highest value)

This is the core promise of the PR.

Steps. Make a book from a template with a few content pages. Type a unique marker into a text box on page 2, then pause. Then leave the page by one of these routes, one test or test.step each:

  • click another page's thumbnail (selectPage);
  • run Duplicate Page from the page menu on the page being edited;
  • delete a different page from the page menu;
  • drag a page to a new position (movePageToSlotOf);
  • leave the Edit tab (switchTab(page, "collection"));
  • quit Bloom (see below).

Assertion. readBook shows the marker on page 2. For Duplicate, it is also on the copy.

Quitting needs new work. bloomApp.restart() kills Bloom with taskkill /f, so the exit save never runs. Add a fixture helper that closes Bloom the way a user does and waits for the process to exit, and only then restarts. Options:

  • taskkill /pid <bloomPid> without /f, which sends WM_CLOSE to the main window;
  • clicking the window's close button through UI Automation.

Put the helper in fixtures/ and give it a doc comment. If it can't be made reliable, record that in src/BloomE2E/AUTOMATION-DEBT.md and mark the quit case fixme.

2. Looking at pages without editing changes nothing

This guards two things:

  • the decision to write nothing for an untouched page;
  • the cleanup that keeps editor leftovers out of the saved page: qTip attributes, ids on Comical's SVG, cke_ classes, tool markup.

Steps. Open a book. Visit every page, open and close two or three tools (Talking Book, Image Description, a reader tool), then leave the Edit tab. Compare the book's .htm with a copy taken before.

Known obstacle. Visiting pages already rewrites the file today, with or without this PR, but only by reordering things:

  • attributes on the xmatter bloom-editables come out in a different order (for example data-languagetipcontent);
  • data-div entries swap places, including branding <img>s whose alt text differs, which makes alt look as if it changed.

The order alternates on each visit: two visits in a row return the file to exactly where it started. With attributes sorted and element order ignored, the content is identical. It was seen in Temein Books/Testing with much improved save and has no card yet. Padding values do not change: BL-16811, the cover/title-page padding problem, is fixed. (Its fix lets the title page's padding change once, the first time the cover is opened; that is expected, not churn.)

So, either:

  • (a) file a card for the order churn, then compare the whole file byte for byte once it's fixed. That is the strongest version of this test, because it shows nothing was rewritten.
  • (b) Until then, compare a normalized form: attributes sorted within each element, and data-div entries compared as a set. Also compare each numbered page's HTML byte for byte; those are not affected.

Assertion.

  • No numbered page's saved HTML changes.
  • The normalized file is unchanged.
  • Once the order churn is fixed: the file is byte-identical, and ideally its modification time doesn't change either, which shows nothing was written at all.

3. Change Layout keeps both the new layout and the typing

This covers the one path that sends the snapshot immediately and then reloads the page.

Steps. Make a book with a page whose template allows Change Layout. Type a marker into a text box, then pause. Turn Change Layout on with setChangeLayoutMode(page, true), split a section (splitSection), and turn the mode off again; the helper waits for the page to rebuild.

Assertions.

  • The page in the Edit tab shows the marker and the split.
  • After leaving the tab, readBook shows the marker and the extra section (its origami layout is in IPageContents).

Variant. Type and immediately turn the mode off, with no pause. This path is meant to survive that, because sendSnapshotNow sends the page before asking C# to reload.

4. A tool's markup never reaches the saved page

Steps.

  • With Talking Book open, type a sentence and import a narration (importNarration), so the tool has added its markup. Leave the Edit tab.
  • With Image Description open on a page with a picture, leave the tab.

Assertions.

  • The saved page has no bloom-describedImage wrapper and no bloom-ui element.
  • Talking Book's phrase-delimiter spans and highlight classes are absent; check which classes talkingBookTool.removeToolMarkup and audioRecording.ts add.
  • After coming back to the Edit tab, the tool still works on the live page. That is the other half of the clone/live split.

5. Duplicate Many Times

Steps. On a content page, open the page menu, choose Duplicate Many Times, enter 3, and press OK.

Assertions.

  • The page count goes up by 3 (waitForPageCount).
  • Cancel adds nothing.
  • While the dialog is open, the workspace tabs are disabled. Read them through getTabs, which reports each tab's state.

A new helper for the dialog is needed. It is a React dialog in the Edit tab's shell document, not in the page list frame.

Not worth doing in e2e

The wait while the page is busy, a click dropped during a page load, and the 25 ms timing depend on timing a test cannot control reliably. Unit tests pin them deterministically: src/BloomBrowserUI/bookEdit/js/pageSnapshot.spec.ts, src/BloomTests/Edit/PageSnapshotTests.cs and EditingStateMachineTests.cs. Keep them there.

Suggested order

1 and 2 first: between them they cover nearly every way this PR could lose or corrupt work. Then 3. Then 4 and 5.

@JohnThomson
JohnThomson marked this pull request as draft October 6, 2026 22:57
Comment thread src/BloomE2E/tests/saved-book-stays-clean.spec.ts Outdated

@JohnThomson JohnThomson left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JohnThomson reviewed 27 files and all commit messages.
Reviewable status: 0 of 130 files reviewed, 117 unresolved discussions.

Comment thread src/BloomTests/SafeXml/SafeXmlTests.cs
Comment thread src/BloomTests/SafeXml/SafeXmlTests.cs
@JohnThomson

Copy link
Copy Markdown
Contributor Author

[Claude Opus 5.5 from John Thomson's machine during preflight] Consulted Devin up to 1159138. Since the last log it reviewed 4c85e76, bca7f60, d462798, 6043544 and 1159138. No bugs on any of them. It raised two Investigate flags about the thread-safety test, one each way: first that CI excluded it, so it was removed from SkipOnTeamCity; then that it is timing-dependent, which we answered and kept. Both threads are answered and resolved. Nothing else is new.

…lunteers (BL-13502)

Saving used to strip the editing markup out of the live page (CKEditor, the
toolbox tool, the canvas-element machinery), so every save had to end by
reloading the page, and C# had to ask the browser for the page and wait for the
answer on another API, which is why everything that saved first was split
around an asynchronous gap with two extra editing states to wait in.

- The page is gathered from a clone of the .bloom-page element and the live page
  is never touched, so a save leaves the user where they were, still editing.
  Each toolbox tool takes its markup off whichever element it is given (the
  clone, or the live page when it is detached) through one method,
  removeToolMarkup. Editor chrome is stripped in the browser only. Bloom Tables
  (removeTableEditingMarkupFromClone) and Comical's drawing surfaces are
  cleaned on the copy too.
- The browser volunteers the page: a MutationObserver posts the gathered page to
  editView/pageSnapshot 25 ms after each change settles, and once as it loads.
  C# stores it per page load (PageSnapshot) and every save uses it
  synchronously. No request carries the page; the commands that restructure
  the page and reload it (Change Layout, importing a video, unlocking and
  relocking the original-copyright sentence) send the snapshot at once first
  (saveChangesAndRethinkPage, postAfterSendingSnapshot).
- Asynchronous work that belongs in the page registers in the delay register;
  every gather waits for it, and the browser tells C# when the page is busy, so
  a save waits briefly for the snapshot of the finished page. Snapshots and
  busy notices go one at a time, in order; failures are reported, not retried.
- The SavePending / SavedAndStripped states, the ask-the-browser API and the
  shutdown cancel-and-reissue are gone. Leaving the Edit tab and closing the
  collection are straight-line code. MergeCurrentPageThenSave and
  SaveCurrentPageAndBook make the same what-to-write decision, and C# compares
  the merged page with the book, ignoring attribute order
  (SafeXmlNode.GetXmlIgnoringAttributeOrder), so an untouched page writes
  nothing.
- Things that made untouched pages look edited are fixed: qTip attributes,
  Comical's regenerated SVG ids, a text-measuring scratch element, attribute
  order, the cover's editing-only ?transparent=yes parameter, missing-image alt
  text on optional branding images, a stray space when BookData merges classes,
  and the original-copyright sentence compared before being locked again.
- The page list's Duplicate Many Times and Choose Different Layout dialogs open
  in the browser, so context-menu commands run inside their request, in order.
- The Talking Book highlight leaves long runs of spaces unpainted itself,
  instead of changing the DOM.
- Requires bloom-player 2.20.3-alpha.1 (bloom-player#441), so a save leaves a
  game being played alone and writes the authored positions.
- Xml_DoesNotProvide_ThreadSafety no longer depends on luck, and CI runs it.
- E2E tests: typing survives every way of leaving a page, quitting included;
  looking at pages does not rewrite the book; Change Layout keeps layout and
  typing; no editor leftovers in the saved book; Duplicate Page Many Times.
  Master's table and original-copyright specs adjusted to the new saving.

src/BloomExe/Edit/SavingWithoutReloading.md explains the design and the
measurements; docs/save-without-reload/HANDOFF.md is for continuing the work
elsewhere and goes before merging.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@JohnThomson
JohnThomson force-pushed the BL-13502-save-without-reload branch from 1159138 to 6a486af Compare October 9, 2026 20:39
@JohnThomson
JohnThomson marked this pull request as ready for review October 9, 2026 20:39
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.

2 participants