Save a page without wrecking it, and spend that on the page list's commands (BL-13502) - #8209
Save a page without wrecking it, and spend that on the page list's commands (BL-13502)#8209JohnThomson wants to merge 30 commits 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 from John Thomson's machine during preflight] Consulted Devin on 2026-09-04 through seven review rounds, up to commit The first round cleared a backlog that had never been mirrored here: thirty-three findings are now posted as their own threads, each with a reply recording what was decided and why. Sixty-two threads stand on the PR; sixty-one are resolved. Eleven were real defects, and all are fixed. None was caught by the test suites. In the order they were found:
Four of those were defects in fixes made earlier in the same run, each caught by the next round. That is the argument for re-reviewing after fixing rather than once at the end. One thread is deliberately left open: the failure path of an in-place save. Master has since answered the same question upstream, in BL-16776, and how that carries into this branch's rewritten code is a judgement for a human. Everything else: both suites green against this HEAD (C# 3366 passed, front-end 851 passed), typecheck and lint clean, One finding could not be given a thread: Devin flags that this branch targets |
Master's BL-16776 fixed three faults, the third of which is that a failed page save blocked every exit: leaving the Edit tab and shutting down both ran their real work only after Book.Save() returned, so a save that always threw blocked the tab change and the close for good, leaving Task Manager as the only way out. It fixed that in the shape those paths used to have -- an asynchronous save with postponed work behind it. This branch replaced that shape, so the textual merge conflicts and the fix has to be re-expressed rather than applied. The same fault takes a simpler form here, because the save is synchronous and there is no postponed work: the exception simply comes out of the collection-closing handler or the tab-change handler. So both now catch it, report, and carry on -- quietly at shutdown, where the user is trying to quit and the failure will already have been reported when it happened; and with the usual could-not-save message on leaving the tab, where they are staying in Bloom. The state machine half of BL-16776 -- run the caller's failure action when the post-save action throws, not only when applying the page content does -- is already true here: that catch covers both, and the caller is told twice over, by reportFailure and by the Failed outcome it gets back, because a synchronous call has a return value to carry it. What we deliberately do NOT do is navigate afterwards, as that older path did: it had to, because the page had been stripped to be read and was no longer editable. Nothing strips the page now, so the browser still has an intact one in front of the user, and rebuilding it from an in-memory book we know to be half-updated would be the worse of the two. That is recorded where the decision is made. Both suites green on the result: C# 3374 passed, front-end 854 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three more from Devin, the first of which I opened one commit earlier. **A baseline taken on resuming would swallow the edits it was meant to catch.** When the watcher is suspended -- a game page in its Play tab -- it has no baseline yet, and the previous commit had it go and take one when the suspension lifted. But a baseline means "what the page looked like before the user touched it", and that moment has passed: anything that changed while we were not watching would be folded in and thereby counted as already delivered, and the next save would write the page as it was before. Resuming now declares instead that it has no idea what C# holds, which makes the snapshot it schedules unconditional. The cost is one redundant snapshot per suspension, and not even a redundant save, since C# writes nothing when what it is given matches what the book already says. **A failed write cleared the flags that say the book still needs writing.** SavePageToDisk catches its failures and tells the user, but said nothing to its caller, so SaveBookToDisk cleared everything and the next save believed there was nothing to do. Same fault as Book.Save had, one level up, and the same answer: it returns whether anything reached disk, and the flags survive a failure. **Chaining the page clicks did not actually order them.** The chain released when the request had been sent rather than when C# had accepted it, so the next click's post could still overtake it -- which is the ordering the chain exists to guarantee. It awaits the post now. The cost is that a second click waits for the first's round trip, and only when two clicks land close enough to overlap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…BL-13502) Suspending the page watcher while a game is in its Play tab keeps the snapshot path from undoing the tester's drags. It does not keep an EXPLICIT save from doing it -- a page-list command gathers the page directly rather than through the watcher -- and it does nothing at all about the other half of the same fault, which is that the copy such a save takes records the draggables where the tester dragged them rather than where the author put them, and writes that into the book. Both come from bloom-player restoring positions through the element references it kept rather than in the page it is given, which is bloom-player#441. When that is merged and the dependency bumped, this whole workaround comes out. Demonstrated in a running Bloom: gathering the page with an item dragged to 333/444 left the live element back at its authored 60/240 and returned content carrying `bloom-draggedToTarget` and `left: 333px; top: 444px`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The page watcher stopped watching while a game was in its Play tab, so that gathering the page could not reach into bloom-player's record of the live page and undo the tester's drags. That was always half a fix: an explicit save -- a page-list command -- gathers the page directly rather than through the watcher, so it still undid the drags, and it still wrote the copy with the draggables wherever the tester had left them, over the book's authored ones. bloom-player#441 fixes both, at the cause: undoPrepareActivity now works on the page it is given and only that one, so a copy comes out with the authored positions -- which is what the book should record -- and the live page goes on being played. So all of this comes out: setSnapshotsSuspended and its page-frame export, both calls in GameTool, the deferred baseline, and the GameTool code that had to work out whether it was holding a clone. What replaces it is a contract spec saying what the save path needs from bloom-player, since getting that wrong is invisible from here until a user loses work. It skips, with a message naming the reason, on a bloom-player older than that fix -- so this branch is green either way, and turns into a real test the moment the dependency is bumped rather than needing anyone to remember. REQUIRES that bump before merging. Verified both ways by building bloom-player#441 locally and swapping it into node_modules: with it, all four contract tests pass and the suite is 854 green; with the published 2.20.1-alpha.6, they skip and the suite is 850 green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bloom-player#441 is merged and published as 2.20.3-alpha.1, so the contract spec added with the workaround removal is no longer skipped: all four of its cases run and pass. That is the dependency the previous commit said this branch required before merging. The lockfile is edited in the three places that actually change -- the importer's resolved version, the package's resolution block, and its snapshot entry -- rather than by letting pnpm rewrite it. `pnpm update bloom-player` reflowed the whole file for 30,000 lines of diff, which would have buried the one line that matters and made the PR unreviewable. `pnpm install --frozen-lockfile` accepts the result. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ing (BL-13502) Two from Devin, both on work from the last few rounds. **A failed page write could still lose the edit.** Keeping the dirty flags so the write would be retried was only half of it: _modifiedPageElement still named the page that failed, and the next edit replaces that with whichever page it was on. The retry was then a per-page write of the NEW page, which reached disk while the failed page's change stayed in memory for good -- retrying the wrong page, which is worse than not retrying. A failed write now also forces the next save to be full: once a write has failed we can no longer honestly say the book differs from disk in one page only. **The bloom-player contract test no longer skips.** The skip existed for one window -- between deleting Bloom's play-mode workaround and bloom-player#441 being published -- and that window closed with the dependency bump. Left in, it could only hide the regression it was meant to announce. Also: the page-change benchmark's header claimed it dispatches "a real click". It dispatches a synthetic event straight at the thumbnail, skipping hit testing and any pointer handling above it, so the first mark is when the handler ran rather than when a finger would have landed. Everything downstream of the click is real; the header now says which is which. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
|
[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 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: 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 Seven more real defects this run, all fixed:
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 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, |
…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>
…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>
|
[Claude Opus 5 from John Thomson's machine during preflight] Consulted Devin on this run's three pushes, up to 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:
All 77 review threads are resolved. |
1 participant
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.
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.
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.
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.
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.Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-13502
Devin review
This change is