Activate the one undo stack behind the Undo button and Ctrl+Y (BL-6681, Stage 1) by JohnThomson · Pull Request #8317 · BloomBooks/BloomDesktop

JohnThomson · 2026-09-07T16:28:15Z

The problem. Bloom has five separate undo mechanisms — origami's layout stack, the reader
tools' per-box text undo, the image-operation undo, CKEditor's per-editable undo manager, and the
browser's own — with no shared record of what happened in which order. workspaceRoot.handleUndo
arbitrated between four of them with a hand-written if-chain that also had to remember the BL-16558
rule (repaint the highlights after any undo that rewrites a text box), and there was no way at all
to add a new kind of undoable operation (deleting a page, deleting a canvas element) without
growing that chain. This is Stage 1 of the plan to retire CKEditor and unify Undo
(docs/retire-ckeditor/PLAN.md); its whole point is to change no user-visible behaviour.

What this PR changes.

Adds the one undo stack (bookEdit/undo/): an index-based stack with truncate-on-push, O(1)
canUndo/canRedo (C# polls canUndo on a timer), page scoping, lazy redo capture, and
runUndoable for one-gesture-one-entry nesting. Entries are data, not page-frame closures,
because the page iframe dies on every navigation.
Wraps the four existing mechanisms as legacy providers, consulted in exactly the order the old
if-chain used, including the BL-16558 markup update after the reader-tools and CKEditor undos.
handleUndo() and canUndo() become one-line delegations.
Tells the stack about page-frame navigation from the one place C# navigates it
(switchContentPage), using the .bloom-page element's id as page identity.
Binds Ctrl+Y in the page frame as a last resort — it acts only when nothing earlier claimed the
keystroke and the stack has something to redo, so origami's and the reader tools' own Ctrl+Y and
CKEditor's redo keep winning until they are converted. No Redo button, no C# involvement.
Corrects the Stage 0 seam in toolbox.ts for master's BL-16717 (bookmarks are now taken only when
something can rewrite the box), and adds live-check harnesses under
docs/retire-ckeditor/liveChecks/ that attribute each undo gesture to the mechanism that ran.

Verified in a running Bloom: the Undo button reaches the reader-tools undo, CKEditor's undo,
origami's undo and the image undo exactly once each, and Ctrl+Y never double-fires. The docs also
record five pre-existing bugs the harnesses exposed (paste-filter bypass, reader-tools double undo,
stale bookmark spans, handler accumulation, a dead page-id check); none is changed here.

Targets the project's integration branch BL-6681-ckeditor, not master, per PLAN.md §5.

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

Devin review

This change is

…its (BL-6681) Both corrections came from spending the "worth a moment's check" that Stage 1 itself asked for, and both would have caused real damage if left. 1. readerToolsModel.redo() is NOT unreachable. decodableReaderTool.tsx:170 calls it. So there are two existing Redos, not one, and Stage 5's "deleted regardless" would have silently removed a working Ctrl+Y / Ctrl+Shift+Z for reader-tool typing. 2. Section 3's ordering table describes the *button* path only. handleUndo() has exactly one caller -- topBarButtonClick, i.e. the toolbar Undo button. There is no Ctrl+Z handler in the workspace frame at all, and C#'s UndoCommand.Implementer is an empty lambda that exists only so the button's Enabled can be set. Ctrl+Z is claimed in the page frame by origami (layout mode), by the reader tools (whenever a markup type is active -- they preventDefault), then by CKEditor, then by native contenteditable undo. Three things follow, all now written down: - The deliberate reader-tools-before-CKEditor precedence is enforced for the keyboard by that preventDefault, not by handleUndo's ordering. The plan reached the right conclusion (wrap the providers and get it for free) from a wrong argument. - Stage 1 is behaviour-neutral because it changes only the button path, not because it preserves an ordering. So "one consistent Undo stack" arrives for the button now and for the keystroke only in Stages 3-4. Better to be straight about that than to over-claim. - Redo cannot be a workspace-frame keydown handler, which is what Stage 1 assumed: keyboard events inside the page iframe never reach the parent document, and typing is exactly when Redo is wanted. That is why both existing handlers are in the page frame. Adds DEFERRED-EDITS.md, the ledger of edits to existing files that a finished stage of new code is waiting on. The project's defence against rebase pain is that new code goes in new files and edits to existing files land late; the cost is that the reasoning for those edits can go stale in between. Writing each one down at design time -- what it is, why it is safe, and what proves it worked -- makes landing it mechanical. Its trigger is deliberately NOT "after Stage 0 merges": under the no-merging constraint that is months away, and waiting would leave Stage 1 unreachable and so unverifiable for the whole period. Stage 0's commits are the base of BL-6681-ckeditor and Stage 1's PR targets that branch, so the review-independence the deferral was buying is already there. Also makes the project resumable cold, on another machine with none of this session's context. That needs saying explicitly because nothing here exists on master -- not the plan, not the code, not even the resume skill -- so a fresh clone has no /resume-ckeditor at all until it checks out a project branch. PROGRESS.md gains a "How to resume" section with that bootstrap, an authoritative branch table naming the working tip, and a master-sync log; the resume skill is rewritten for the new topology, since its old advice ("you may well be on master with nothing in flight", "don't keep a long-lived branch") is now exactly backwards. Stage 0's four remaining verification items are retargeted off the branch under review, where pushing would restart the review for purely additive work. Also fixes citation drift found while checking: origami.ts:139-146 -> :137, and the origamiCanUndo/origamiUndo range -> :277-294. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Stage 1 of retiring CKEditor: the single undo entry point that the four existing mechanisms will be folded into, plus the transaction wrapper that keeps one user gesture to one undo entry. New files only. Nothing imports them, so this is behaviourally inert -- the edits to workspaceRoot.ts that would activate it are specified in docs/retire-ckeditor/DEFERRED-EDITS.md. - undoTypes.ts: IUndoEntry, ILegacyUndoProvider, kMaxUndoEntries. The long comment on IUndoEntry earns its length: it is the rule that shapes the whole design. The page iframe's JS context dies not only on page change but on same-page reloads (ctrl+wheel zoom, leaving Change Layout mode), so an entry built by page-frame code would later mutate a detached document or throw. Entries are therefore built in the workspace frame out of pure data. - UndoStack.ts: index-based with truncate-on-push, so Redo is possible without a second structure. canUndo/canRedo are O(1) because C# polls canUndo on a timer to set the button's enabled state, and anything that walks entries there makes the button flicker. Bounded by entry count. Page-scoped entries are discarded on page change, while entries with no pageId (deleting a page) survive. Redo state is captured lazily at undo time rather than at commit time, so nothing extra is paid per keystroke -- the same trick origamiUndo already uses. - legacyUndoProviders.ts: wrappers, not conversions, in exactly the order handleUndo used. Each carries the reason its mechanism behaves as it does, so that when it is deleted the reasoning does not go with it. - runUndoable.ts: depth-counted scopes, outermost wins, nested pushes dropped. Not speculative: deleting a canvas element whose content is a background image already records an image undo, so wrapping the delete naively would leave two entries and the first Ctrl+Z would half-undo the gesture. 31 tests. They passed first run, which for new code is a reason for suspicion rather than satisfaction, so I mutation-tested three of the load-bearing behaviours: removing truncate-on-push failed 1 test, disabling nested-push suppression failed 6, and making keepOnly recompute the index unconditionally failed 1 -- each caught by exactly the test meant to catch it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

… (BL-6681) Stage 1's DEFERRED-EDITS 1a-1e. workspaceRoot.handleUndo() and canUndo() become delegations to theOneUndoStack, whose four legacy providers reproduce the old if-chain in the same order. The button path is the only thing this changes; Ctrl+Z is still claimed per-context in the page frame. Three things differ from the deferred-edits text as written, all because master moved or the premise was wrong: - BL-16558 (on master since) made handleUndo call updateMarkupAfterUndoOrRedo() after the reader-tools and CKEditor undos, because both rewrite an editable's innerHTML and so detach the highlights painted over it. The toolbox and ckeditor providers now do the same, and legacyUndoProvidersSpec pins it. - Redo is bound in the PAGE frame (undo/redoKeyBinding.ts, one call from editablePage.ts), as the last resort: it acts only when nothing earlier in the bubble claimed Ctrl+Y and the stack has something to redo, so origami's and the reader tools' handlers, and CKEditor's own redo, keep winning until converted. The workspace bundle grows canRedo()/handleRedo() for it. - Page identity is the .bloom-page element's id, not data-page-id: nothing in Bloom sets that attribute (only ImageUndoManagerSpec does), so the check in ImageUndoManager that reads it never fires. pageFrameUndoHooks.ts clears page-scoped entries whenever switchContentPage runs -- the one route C# uses to navigate the page frame, same page or not -- and records the id on load. Also corrected: ctrl+wheel zoom no longer reloads the page (it is a CSS transform via setZoom), so the comments citing it as a same-page reload are fixed; the real same-page reloads are leaving Change Layout mode, importing a video and changing the topic. 52 undo tests green (21 new), typecheck clean, no new lint warnings.

…ent (BL-6681) PROGRESS.md: the master-sync row, the Version6.5 finding (the merge window may now be open; John's call), the live-verification results, the two pre-existing reader-tools bugs the harness exposed, what BL-13502 changes for the plan, and revised next actions. PLAN.md: the zoom premise and the 5.1 drift table were wrong and are corrected. DEFERRED-EDITS.md: entries 1a-1e are landed; the verification checklist is ticked with what proved each item. docs/retire-ckeditor/liveChecks/: the four CDP harnesses that produced those results, kept so later stages can show a mechanism moved onto the stack and nothing else did. They attribute each gesture by wrapping the cross-frame entry points and listening to CKEditor's afterCommandExec, not by looking at the text.

…-6681) liveChecks/handlerAccumulation.mjs shows the document-level and per-editable edit key handlers each fire once more per extra SetupElements run (1 -> 2 -> 3), confirming PLAN.md 4.10's code-reading finding; it doubles as the inventory's X4 test, failing until the signal-scoped teardown lands. activateTool.mjs switches tools through ToolBox.activateToolFromId, and the harnesses now ask the ToolBox which tool is current instead of trusting the accordion's header classes, which lag. G2 stays open: with the Talking Book tool current, typing produced no audio-sentence markup, so the async path was not exercised.

…ed (BL-6681) PASTE-DROP-BASELINE.md records what today's pasteFilter lets through for inventory rows C1-C7, paste and drop alike, from synthetic ClipboardEvent / DragEvent payloads that go through CKEditor's clipboard plugin and Bloom's paste transforms exactly as real ones do. Every row matches the inventory except C5: spans arrive with every attribute and style, because BloomField.restoreHtmlMarkupIfNecessary (BL-12357) tests dataTransfer's cke/id to detect an internal copy, and CKEditor stamps every transfer with one. When the payload contains a styled span that handler replaces the filtered HTML with the full clipboard HTML, so tables, iframes, images and divs with ids reach the book; the same payload without the span is filtered correctly. liveChecks/pasteFilterBypass.mjs is the repro; the inventory's C5/C7 rows and PROGRESS.md are updated, with the fix (test the transfer type) noted.

…view found (BL-6681) The Undo button never reached workspaceRoot's handleUndo: C# calls topBarButtonClick in the PAGE frame, which imported handleUndo from ../workspaceRoot, so that module -- and "the one" stack, with its own providers -- was executing in the page frame too, and the button undid from that copy. Neutral while both were empty, wrong the moment anything is pushed. topBarButtonClick now calls getWorkspaceBundleExports().handleUndo(), the import is gone, and the provider registration runs only in the top frame (Vite still bundles workspaceRoot into a chunk the page and toolbox bundles import, so its top level runs in every frame). The live checks now press Undo through the page frame's real entry point, which is how the bug was missed. Devin's three findings, all in new code: undo()/redo() now move currentIndex transactionally, so a failing entry stays the next thing to undo instead of being skipped and offered as Redo; runUndoable holds pushes until the outermost scope closes and keeps the gesture's own (depth-1) entry, falling back to the first inner one, so an inner layer recording first no longer wins; and the Ctrl+Y binding stands down in Change Layout mode, where origami redoes without claiming the event. A once-only load listener records the page id even when switchContentPage's 1500 ms fallback ran first. 69 undo/toolbox tests green (11 new), typecheck and lint clean, and all four live checks pass on a freshly launched Bloom. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

…ope (BL-6681) Two edges Devin's second round found in the transactional index and the held pushes, both about navigation racing an asynchronous operation: - A failing async undo restored the numeric index it had before, but clearPageScopedEntries may have dropped and renumbered entries meanwhile, so the index could point past the end and canUndo would advertise an entry that was not there. The rollback is now by identity: the failed entry becomes the next to undo (or redo) if it still exists; if navigation removed it, the index keepOnly computed is already right. - Pushes held by an open runUndoable scope were not filtered by keepOnly, so an async gesture awaiting across a page change would record its old-page entry when the scope closed. They are now filtered by the same predicate. Two tests pin both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Devin's third round: a push that arrives AFTER a page change -- an asynchronous gesture on the old page finishing late -- misses keepOnly, which ran at navigation time, and would be recorded and later undone against the new page. record() now refuses an entry scoped to any page but the current one; entries with no page id (deleting a page) are unaffected. A test drives the late push both bare and inside a scope. Also logs the second round's two fixes in PROGRESS.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

JohnThomson · 2026-09-07T17:08:26Z

[Claude Fable 5.1, via John Thomson] Consulted Devin on 2026-09-07 up to commit ecff075476f025dde31c97cb2a36592bbcbe8b62 (four rounds, one per push). It raised eight distinct findings over the branch — six bugs and two investigate flags — each mirrored as a review thread below with its outcome: the transactional undo index and its identity-based rollback, outermost-wins nesting, dropping held and late pushes on a page change, the Ctrl+Y stand-down in Change Layout mode, and the once-only load listener were all fixed; the async-scope constraint is documented on runUndoable. One informational note (providers re-acquire frame exports per call) was read and needs no change. Round four listed only already-resolved items, so the review is clean at this head. CI (pr-automation) is green; CodeRabbit posted nothing.

Devin review: https://app.devin.ai/review/BloomBooks/BloomDesktop/pull/8317

JohnThomson and others added 7 commits September 7, 2026 09:41

Record the integration-branch nightly result (BL-6681)

495cee7