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 22 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 |
postString goes through wrapAxios, which turns a rejected request into a resolved promise carrying nothing. So a failed post is indistinguishable from a successful one except that no response comes back -- and we were reading only `.data`, which took that for an acceptance. The content was recorded as sent and never offered again, and the next save would write what C# still held, losing everything typed since the snapshot before it. Delivery now requires an actual response that is not a refusal. The retry for both cases, and for a gather that throws, moved to a slower timer: nothing the user did causes any of them, so there is nothing to be responsive to, and retrying every 25ms against a server that is not answering would spin. A real change still reschedules at the normal interval and overtakes it. The existing test missed this because it modelled a failed post as a REJECTED promise, which is the one thing postString never gives us; and every other test's post resolved with nothing, which now means "not delivered", so the mock's successful reply had to become what C# really sends. The new test uses the realistic shape. Found by Devin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Retrying a failed post every second was itself a defect: wrapAxios reports every failed request, so a server that had stopped answering would put an error in front of the user once a second for as long as they stayed on the page. A refusal and a failure are now retried differently, because only one of them is loud. A refusal costs nothing and ends by itself the moment the page reports ready, so we keep offering at a steady second. A failure backs off -- 1s, 2s, 4s, 8s -- and then we stop asking on our own. The content is still not recorded as sent, so the next thing the user changes offers it again, which is how a recovering server gets it; and the case that would otherwise be lost, a failure followed by no further typing and then a quit, is by then several retries old. A gather that throws no longer schedules a retry at all. The gather is deterministic: a page that fails to gather fails again immediately, so a timer would only repeat the report we go to some trouble to make just once. Found by Devin, in the fix for its previous finding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit stopped retrying a failed post after a few attempts, to stop the user being shown an error a second. That traded one defect for another: while the browser holds content C# has not got, quitting writes what C# still holds, so a server that recovers after the retries ran out loses whatever was typed before it broke. The two are only in tension because the request layer decides when to speak. So it no longer does: snapshot posts go through a new postStringQuietly, and pageSnapshot reports the failure itself, once per page. Retrying can then go on as long as it needs to -- backing off 1s, 2s, 4s up to 30s and staying there -- and a server that comes back gets the content with no further typing. A gather that throws now takes the same path. It is deterministic and will usually fail again, but that costs nothing now that it is silent after the first report, and it covers a failure that turns out to have depended on something transient in the page. Both halves are pinned by the test: it must keep offering, it must back off, and the user must be told exactly once. Found by Devin, on both sides -- it flagged the flooding, and then flagged the lost edits in the fix for it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two more from Devin, both of them ways the browser could hold content C# never learned about -- which is the one thing this design cannot afford. **Style-only edits could disappear.** The gathered content includes the user's own style definitions, but the watcher is a MutationObserver on the body, and the style editor changes those definitions through the CSSOM -- setProperty, deleteRule, insertRule -- which mutates no DOM node anywhere. So changing a style's size, spacing or colour without touching the text produced no snapshot at all, and leaving the Edit tab or quitting wrote the styles as they were. There is one place every such change passes through, GetRuleForStyle with create set, and it now tells the page frame. Saying so needlessly costs nothing: an unchanged page produces no post, which is what lets the hook be placed generously rather than exactly. **Leaving Play by switching tools disabled saving for the rest of the page.** Snapshots are suspended while the game tool is in its Play tab, and resumed when it leaves -- but the user can leave Play by switching to another tool, which detaches the game tool straight from play mode without going near the tab code. Nothing on that page would then be volunteered again. The resume now lives in removeToolMarkup, where play mode actually ends, and it tells the live page from a save's clone by whether the element is still in the document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Clicking a thumbnail used to post immediately, so two quick clicks reached C# in the order they were made. Gathering the outgoing page's content first put an await in front of that post, and two clicks in quick succession became two overlapping gathers whose posts could arrive either way round -- so C#, which takes the first and declines the second while it navigates, could act on the earlier click rather than the later one. The clicks are now chained, which costs the second one the first's gather: well under a millisecond. A click that somehow fails does not stop later ones. Also, the page-change benchmark takes the two page ids to click between, rather than only ever working on the book it was written against. Both found by Devin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ay mode (BL-13502) Two more from Devin. **A save that could not write reported success.** Book.Save catches the access-denied case, tells the user, and returns -- so SaveCurrentPageAndBook returned true for a write that never happened. That matters because this branch gave callers a return value to act on: the AI image editor opens the book FROM DISK, so it would show an older book and commit its edits over the newer one. Save now says whether it actually wrote. **A game page can open straight into its Play tab**, because the tab is remembered per page, and the once-per-page baseline read happened regardless of the suspension -- so the very read that suspension exists to prevent still undid the live play preparation. The baseline now waits until play mode ends, which is also the state a save should be writing anyway. Nothing is at risk while it waits: with no baseline nothing is posted, and there is nothing a save should write while the user is playing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
[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>
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. 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.
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.
not free: the game tool's cleanup reaches into bloom-player's record of the live page and would
undo the player's drags.
src/BloomExe/Edit/SavingWithoutReloading.mdexplains the design and the measurements.Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-13502
Devin review
This change is