Repository navigation
Save a page without reloading it; save from a snapshot the browser volunteers (BL-13502) - #8209
JohnThomson wants to merge 1 commit into
Conversation
|
[Claude Opus 5 from John Thomson's machine during preflight] Consulted Devin through three review rounds during this preflight, most recently up to It found three real bugs, all now fixed and each with its own resolved thread above:
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 Devin's own re-review of |
c5d0e0f to
37d206b
Compare
|
[Claude Opus 5 from John Thomson's machine during preflight] Consulted Devin through four review rounds during this second preflight, most recently up to 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 ( |
|
[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 Seven findings now have their own threads above; six are fixed and resolved, one is left open for John. Fixed this round:
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 Full suites green at 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 |
|
[Claude Opus 5.5 from John Thomson's machine during preflight] Consulted Devin on the 5th, up to 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:
On the final head: C# 3678 passed, 1 skipped; front-end 1021 passed on |
|
[Claude Opus 5.5 from John Thomson's machine during preflight] Consulted Devin on the 6th, up to Driving Bloom on the 6th found a regression from the master merge of 1 October: this branch's page gather ( Devin's round on 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. |
|
[Claude Opus 5.5 from John Thomson's machine during preflight] Consulted Devin on the 6th (about 16:10 UTC), up to The automatic trigger still does not fire while the branch conflicts with master, so the round was started by re-running an earlier
Every earlier finding is marked fixed by Devin, and all earlier threads were already resolved. |
|
[Claude Opus 5.5 from John Thomson's machine during preflight] Consulted Devin on the 6th, up to
Every thread is resolved. |
|
[Claude Opus 5.5 from John Thomson's machine during pr-ready-for-human] Consulted Devin up to |
35b2f17 to
ba27292
Compare
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson reviewed 114 files and all commit messages.
Reviewable status: 0 of 114 files reviewed, 116 unresolved discussions.
|
[Claude Opus 5.5 from John Thomson's machine, at John's request] Plan: e2e tests to keep this PR's saving workingNothing 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
None of the following is covered by e2e today. Seven specs touch the changed code ( Before starting
Existing Notion cardsChecked against the 6.5 suite run, the full inventory; 6.6 had only 15 cards when checked. The procedure for each case is in
Set
Already automated and touching this PR: #348 Copy Page Preserves Everything ( 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
Assertion. Quitting needs new work.
Put the helper in 2. Looking at pages without editing changes nothingThis guards two things:
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 Known obstacle. Visiting pages already rewrites the file today, with or without this PR, but only by reordering things:
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 So, either:
Assertion.
3. Change Layout keeps both the new layout and the typingThis 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 Assertions.
Variant. Type and immediately turn the mode off, with no pause. This path is meant to survive that, because 4. A tool's markup never reaches the saved pageSteps.
Assertions.
5. Duplicate Many TimesSteps. On a content page, open the page menu, choose Duplicate Many Times, enter 3, and press OK. Assertions.
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 e2eThe 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: Suggested order1 and 2 first: between them they cover nearly every way this PR could lose or corrupt work. Then 3. Then 4 and 5. |
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson reviewed 27 files and all commit messages.
Reviewable status: 0 of 130 files reviewed, 117 unresolved discussions.
|
[Claude Opus 5.5 from John Thomson's machine during preflight] Consulted Devin up to |
…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>
1159138 to
6a486af
Compare
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.
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 current content to C#, which stores it under the id of that page load. A save then takes that
content synchronously.
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.
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.
of a book, front and back matter included, writes nothing:
gathered content;
?transparent=yesimage parameter no longer makes the book-wide datalook changed, which forced a full save on every visit;
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.
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.
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.
Bloom Tables (
removeTableEditingMarkupFromClone) and Comical's drawing surfaces. The twocommands of the editable original-copyright sentence (unlock and relock) reload the page, so
like Change Layout they first send the page to C#.
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.mdexplains the design and the measurements.Also included, because it kept failing this PR's test runs:
Xml_DoesNotProvide_ThreadSafety, aflaky 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 thebounded wait while the page is busy).
The gather now runs after every change (25 ms after typing settles), so
getPageHtmlForSaveand 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, andCKEditor'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 beenchecked; 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
MergeCurrentPageThenSavenow 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
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.
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.
Shell.SavingBeforeWindowsShutsDown("Bloom is saving yourbook"), 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 timepending strings are released.
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 anotherpage, 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 pagesand 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 thetyping, 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 nowwaits 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) andoriginal-copyright-sentence.spec.ts. Twoneeded adjusting:
tables-extendedwaited for Copy Page to reload the page, and now waits for PastePage to be enabled;
original-copyright-sentencetyped and left the page at once, and now waitsuntil 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
Looking at Pages Does Not Rewrite the Book, BL-2919 Fix a null ref crash in New Collection #843 Change Layout Keeps Typing, Fix BL-2918 (probably) by skipping refresh if the control isn't ready #844 Duplicate Page
Many Times (split from New epub page with resizable css smartphone, notes, & mad scientist #810).
Book HTML.
covers part of it.
see above), BL-788 [Trusty Linux] Print dialog opens behind Bloom window #349 Duplicate Page.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-13502
Devin review
This change is