Toolbox cleanup 4/4: give the toolbox's state and each tool's lifecycle to React (BL-16608)

[andrew-polk]

Problem

Every tool in the Edit tab's toolbox is a React component, but the toolbox around them was not. toolbox.ts kept which tools were offered, which was open and which was running in its own module variables, and called each tool's lifecycle methods by hand, while React rendered the tool list from a separate copy of the same facts. Keeping two copies in step by hand is what failed in BL-16602: leaving a game page withdrew the Game tool but left the toolbox believing it was still current, so the tool that replaced it was never shown and Talking Book lost its highlighting and audio.

Separately, the toolbox has to wait for the page (CKEditor) before restoring the book's saved settings, but it applied settings read before that wait. Anything the user did meanwhile was undone: a toolbox just opened was shut again, a tool just opened closed back to the saved one. People click again and move on; seven e2e tests could not, and were skipped.

Fix

• One React root. Each tool is an ordinary child rendered through ITool.renderPanel(), so React context (the MUI theme) reaches tools normally and the per-tool ThemeProviders go.
• toolboxState.ts is the single source of truth for which tools are offered, which is open, which tools the book has enabled, whether the toolbox shows, and which page it is on. React subscribes with useSyncExternalStore; toolbox.ts reads and writes it.
• Which tool is running is derived, not stored — the open tool, unless the toolbox has stopped offering it. The two can no longer disagree, which makes the BL-16602 class of bug unrepresentable rather than fixed. With it go the listener round trip through toolbox.ts and the unreachable clearActiveTool().
• useToolLifecycle.ts runs each tool's lifecycle from a React effect, so a tool runs because it is the current tool of a showing toolbox rather than because something remembered to activate it.
• Neither late restore overwrites a decision made after its settings were read. Each snapshots the toolbox before its settings request goes out and imposes a saved fact only if that fact has not changed since; readerToolsModel.restoreState() seeds its default stage and level only into a model nothing has told yet.
• A reader stage or level the book had saved is no longer silently thrown away. setStageNumber/setLevelNumber used to discard the value outright when the Synphony settings had not loaded, so a slow or failed load left the tool on 1 and — because the restore does not save — recorded nothing either. They now hold the request until the settings arrive. This is a long-standing bug, not one this rework introduced; it is fixed here because it is what made the re-enabled reader tests flaky.
• The seven skipped e2e tests are back on (Test Case IDs 830, 441, 442, 460).

Why a store, and not just React state

The obvious objection to toolboxState.ts is that it looks like a layer React should not need. It is there because the code that shares this state with React cannot itself be React.

toolbox.ts is published as window.toolboxBundle and called from the page frame, the workspace shell and the reader-setup dialog — about fifty call sites across those documents — and it starts running before any React has mounted. You cannot hand a hook across an iframe boundary, so it has to stay plain module code, and the state it shares with React has to be reachable from outside React.

Keeping the state inside React and bridging inward is what this PR deletes. That was toolboxReactAdapter.ts: a registration object that only existed once ToolboxRoot had mounted, plus a queue for everything that arrived first — a page enabling the Canvas tool while still loading, for instance. The store points the bridge the other way. Module state exists at import time and React subscribes to it, so there is no mount ordering to get wrong; that is why beginAddTool is now three straight lines instead of a deferred callback. useSyncExternalStore is React's own mechanism for this, and the Game tool already keeps its panel state the same way.

The bridge is transitional, not a destination. It shrinks as the Edit tab stops being several documents — a separate and much larger job, and not a prerequisite for this one.

Screenshots

None: the toolbox is meant to look and behave exactly as before, and does. Verified by driving a real Bloom — all ten tool panels render with their own controls at identical geometry, alphabetical order with "More..." last, subscription badges on exactly Canvas/Motion/Music, and no console errors.

Risk Evaluation

This is a rework of shared machinery, not a new feature behind a flag, so the blast radius is every tool and every edit session. The paths worth the reviewer's attention:

• Toolbox startup and page-change ordering (restoreToolboxSettings, applyToolboxStateToUpdatedPage, restoreToolboxSettingsWhenPageReady). The "only impose a saved fact that has not changed since we read it" rule decides what the user sees when a book or page opens, and it depends on when the snapshot is taken.
• Every tool's lifecycle, now driven by a React effect. Effect ordering is load-bearing: the activation effect is declared first so its cleanup runs before the detach-and-hide cleanup.
• A tool's panel now mounts whenever the tool is offered, not only while it runs. Anything a tool starts from its panel's mount (observers, timers, listeners) must clean itself up on unmount, because detachFromPage() only runs for the running tool. MotionTool's image observer needed exactly this.
• readerToolsModel, where a flag now gates restoreState() and a stage/level asked for before the settings arrive is held rather than dropped. The reader tools save from many places, so the guard's correctness depends on every stage/level setter marking the model as told. The held request is discarded when a restore for another book begins, because the model outlives a book change — the toolbox frame is not reloaded for one.
• Per-keystroke markup is unchanged by this PR but runs through the current tool, so a wrong "current tool" would show up there.

Lower risk: ToolboxRoot.tsx and toolboxState.ts are substantially new code reached only through the toolbox.

Ecosystem Impact

• Published output (Bloom Player, BloomPUB, ePUB, PDF): unaffected. Nothing here changes book content or the files Bloom writes, beyond the existing meta.json currentTool and <tool>Check settings, whose formats are unchanged.
• Older Blooms opening the same books: unaffected, for the same reason — the persisted spellings (the historical "Tool"/"Check" suffixes) are untouched.
• Localization: no XLF files added or changed; no new user-visible strings.
• Dependencies: none added or changed (no package.json, lockfile or .csproj edits).
• Other repos/services: not touched.
• C#: one comment-only change, in ToolboxView.cs.

E2E Coverage

Re-enabled by this PR, after the races above were fixed:

• src/BloomE2E/tests/toolbox-tools.spec.ts — turning a tool on and off under "More...", clicking the open tool's header, and the book remembering which tool was open (Test Case ID 830).
• src/BloomE2E/tests/reader-tool-stage-and-level.spec.ts — each book remembering its own stage and level, and a new book inheriting the last chosen ones (Test Case IDs 441, 442, 460).

Measured locally at the nightly CI window size, headless: both files pass, and reader-tool-stage-and-level.spec.ts passed 20 consecutive runs. That is not proof the remaining flake is cured — at the rate observed, 20 clean runs happen about one time in eight by chance — so the reader-restore fix is backed by readerToolRestore.spec.ts, which reproduces the failure deterministically and fails on the unfixed code with the flake's own signature. The first nightlies are still the real confirmation. The shared helpers now wait for a tool's header rather than deciding from a single check that the toolbox is not offering it, which was an intermittent failure in both specs.

Also covering this change: ToolboxRootTestHarness component tests (the real tools driven through the real store), and unit specs toolboxState.spec.ts, useToolLifecycleSpec.tsx, ToolboxRoot.spec.tsx, and the new readerToolRestore.spec.ts (the restore path when the settings never arrive, including that a pending value does not cross into the next book).

Not covered end to end: the game-page path, where visiting a game page offers the Game tool and leaving withdraws it — the original BL-16602 scenario. It has unit coverage in useToolLifecycleSpec and toolboxState.spec, but no e2e test and no manual confirmation.

Notion Test Suite

No cards added. The four cards covering the re-enabled tests are all on the 6.6 run, all Automation = Automated, all Status = Not started — nothing to change.

ID	Title	Status
830	Toolbox: turning tools on and off, and which tool is open	Not started
441	New Book Inherits Stage [Automated portion]	Not started
442	Book Remembers Its Stage	Not started
460	New Book Inherits Level	Not started

The 6.5 copies of 441, 442 and 460 still read Skipped, which is correct history for that run and is not worth editing: 6.6 is what these tests now run against.

Preflight report: https://bloombooks.github.io/dev-process-artifacts/deciders/bloomdesktop-bl-16608-toolbox-3-react.html

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

Devin review

---

This change is 

[andrew-polk]

[Claude Opus 5 during preflight] Consulted Devin on 2026-10-06 up to commit fd31f42ed7a7522840667a952d43c5ec2408a45d.

It raised 3 Bugs and 1 Investigate flag, 0 Informational. Each is mirrored above as its own thread with its outcome: two were correct and are fixed in 3ab88e6f62 (the settings snapshot taken inside the GET callback instead of before the request; Motion's image observer not being disconnected when an inactive tool is withdrawn and re-offered). Two describe real but pre-existing behavior that master shares and this PR does not change — both on the deferred page-adjustment retry — and are resolved as such rather than silently.

Re-review for 3ab88e6f62 is running; CI green.

[andrew-polk]

[Claude Opus 5 during preflight] @greptile-apps review

[andrew-polk]

@andrew-polk+AI made 1 comment.
Reviewable status: 0 of 41 files reviewed, 4 unresolved discussions.

---

src/BloomBrowserUI/bookEdit/toolbox/readers/readerToolsModel.ts line 215 at r5 (raw file):

            // could spring back to 1 -- and, because the restore passes skipSave, never
            // be recorded either -- whenever the settings load was slow or failed.
            this.stageAwaitingSynphony = { value: stage, skipSave };

[Claude Opus 5] Why a pre-existing bug is being fixed in this PR.

This silent drop is not something this PR introduced -- it has been here a long time. setStageNumber/setLevelNumber discarded the requested value whenever the Synphony settings had not loaded, so a book's saved stage or level sprang back to 1 if that load was slow or failed, and because the restore passes skipSave it was never recorded either.

It is being fixed now for two reasons:

1. It shows up in e2e. It is the cause of an intermittent failure in reader-tool-stage-and-level.spec.ts -- exactly the tests this PR re-enables (Test Case IDs 441, 442, 460). Turning those tests back on while leaving this in place would hand us a flaky suite.
2. This PR is the moment the toolbox gets a full test pass. It is the last of the four, so the whole rework is about to be exercised end to end. Fixing it here means that pass covers the fixed behavior rather than documenting a known flake.

The fix holds the request instead of discarding it, and setSynphony -- the single point where settings arrive, on both the cold-start and refresh paths -- applies it. An out-of-range value is still refused.

The mechanism is proven rather than inferred: readerToolRestore.spec.ts reproduces it deterministically, and those tests fail on unfixed code with the flake's exact signature (expected 1 to be 3).

[andrew-polk]

@andrew-polk reviewed 41 files and all commit messages, and resolved 6 discussions.
Reviewable status: 0 of 41 files reviewed, 3 unresolved discussions.

[andrew-polk]

[Claude Opus 5 during preflight] Consulted Devin on 2026-10-08 up to commit 5b92ca7a8c3144e98330f4b78d25f08bbebf4628.

Its review of that commit completed with no findings against it. Everything it raised across the branch now has a documented outcome on its own thread: three bugs it marks resolved, one assessed and resolved earlier, and the investigate flags either fixed, assessed as already covered, or recorded with the developer's decision. No unresolved review threads remain.

[StephenMcConnel]

Two minor comments is all I found.

@StephenMcConnel reviewed 41 files and all commit messages, made 3 comments, and resolved 3 discussions.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on andrew-polk).

---

src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.spec.tsx line 7 at r8 (raw file):

// The real ./toolbox module drags in the whole legacy toolbox (jQuery, every tool, the
// edit-page frames...). All ToolboxRoot wants from it is the list of tools that exist,

Is this comment still accurate?

---

src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts line 387 at r8 (raw file):

    public getCurrentTool(): ITool | undefined {
        return getCurrentTool();

Is the getCurrentTool() that is called different from the getCurrentTool() being defined? It looks like the method is defined in terms of itself, but with no recursive termination. Javascript/Typescript may be able to distinguish these identical names, but humans have a harder time.

[andrew-polk]

@andrew-polk+AI made 2 comments.
Reviewable status: 39 of 41 files reviewed, 2 unresolved discussions (waiting on StephenMcConnel).

---

src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.spec.tsx line 7 at r8 (raw file):
[Claude Opus 5, on Andrew's behalf] Good catch — it was stale, on one count.

I checked it claim by claim against the current module:

• jQuery — still true (import $ from "jquery", five uses).
• the edit-page frames — still true (parent.window.document, pageFrame.contentWindow).
• "every tool" — no longer true. toolbox.ts imports only GameTool directly now; the rest register themselves through ToolBox.registerTool. That phrase described the module before this rework.

"the whole legacy toolbox" was also the wrong frame of reference, since this PR is what removes the legacy copy.

The comment now reads:

The real ./toolbox module drags in a great deal that a unit test cannot run: jQuery, the edit-page frames it reaches through parent.window, and the tools it imports directly.

The second sentence I left alone — ToolboxRoot really does want only the tool list from this module (it imports getMasterToolList and ITool and nothing else); the two other mocked functions belong to useToolLifecycle, which the comment block just below already explains.

---

src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts line 387 at r8 (raw file):

Previously, StephenMcConnel (Steve McConnel) wrote…

Is the getCurrentTool() that is called different from the getCurrentTool() being defined? It looks like the method is defined in terms of itself, but with no recursive termination. Javascript/Typescript may be able to distinguish these identical names, but humans have a harder time.

[Claude Opus 5, on Andrew's behalf] They are different, and you are right that it reads badly.

There is a file-local function getCurrentTool() further down the module, and an unqualified call inside a method resolves to module scope rather than to this, so it is not recursive. But that is exactly the point — you have to know that rule to read the line, and nothing on the page tells you.

While looking, setCurrentTool two lines below turned out to have the identical shape: a file-local function shadowed by a method that delegates to it. Fixing only the one you asked about would have left the same question for the next reader, so both are renamed for what they actually do:

• getCurrentTool() (module) → findCurrentTool() — it looks up the ITool matching the store's current tool id.
• setCurrentTool() (module) → makeToolCurrent() — "set" undersold it: it canonicalizes the id and falls back to the first offered tool.

The two public methods keep their names, since four cross-frame callers use getCurrentTool() through the toolbox bundle. The rename is file-local, 13 lines, no call site through an object touched; 531 toolbox tests pass and typecheck is unchanged.

[andrew-polk]

@andrew-polk reviewed 2 files and all commit messages.
Reviewable status: 39 of 41 files reviewed, 2 unresolved discussions (waiting on StephenMcConnel).

[andrew-polk]

@andrew-polk reviewed 2 files and all commit messages.
Reviewable status: 37 of 41 files reviewed, 2 unresolved discussions (waiting on StephenMcConnel).

[JohnThomson]

@JohnThomson partially reviewed 10 files and made 13 comments.
Reviewable status: 37 of 41 files reviewed, 15 unresolved discussions (waiting on andrew-polk and StephenMcConnel).

---

src/BloomBrowserUI/bookEdit/js/editableDivUtils.ts line 204 at r8 (raw file):

        // page frame up in window.top rather than parent. For the frames this runs in (the page
        // frame and the toolbox frame) those are the same document; see getPageIFrame() in
        // shared.ts for why parent is the one we standardized on.

I think it's rarely useful to permanently store the fact that code used to be different and why it was changed. What it is now is perfectly reasonable.

---

src/BloomBrowserUI/bookEdit/toolbox/ReadMe.txt line 31 at r8 (raw file):

                         the list exists once rather than being duplicated.
    - markupSelectionPreservation.ts  preserving the user's caret while a tool rewrites the
                         markup around it.

This is going to be working mainly inside the page iframe...would it be better in that bundle? Not necessarily, if most of its clients are in the toolbox bundle, but worth considering.

---

src/BloomBrowserUI/bookEdit/toolbox/ReadMe.txt line 56 at r8 (raw file):

1. Create a folder here whose name is the tool's canonical id (no "Tool" suffix).
2. In it, write a class that extends ToolboxToolReactAdaptor, implementing at least id()
   and renderPanel() (which just returns the tool's React element), plus iconPath() if its

This description (and maybe even the name) could be clearer about whether you mean the real HTML element (from useRef or similar) or the React object that represents it.

---

src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx line 33 at r8 (raw file):

import { useToolLifecycle } from "./useToolLifecycle";

// React host for the toolbox sidebar. It shows an accordion for each tool the toolbox is

There's a confusion of terminology here caused by MUI: in ordinary English (even fairly technical programmer English) Accordion refers to the whole list of expandable sections, but MUI considers each expandable item to be an Accordion. There might be a few places it would be helpful to note that, including here and in the description of OfferedToolAccortion. For example, here we might say "It shows an Accordion (MUI: one expandable item, not the whole list as in common usage) for each tool..."

---

src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx line 266 at r8 (raw file):

                    // Some tool stylesheets, and our automated tests,
                    // still select a tool's body by this attribute, using
                    // the historical "Tool"-suffixed name.

While we're cleaning up, can we fix stylesheets and automated tests that refer to things by obsolete names? Also consider whether sylesheets for tools should be replaced by emotion, though either of these changes might want to be separate cards.

---

src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts line 101 at r8 (raw file):

    // it was opened or closed.
    showTool();
    hideTool(); // called when the tool stops running: another tool takes over, or the toolbox is hidden.

If the new comments are accurate, ShowTool is NOT called when the toolbox is opened (unless perhaps it has not previously been called for the tool that is becoming visible?), but hideTool IS called when the toolbox closes. If that's a real change from the previous behavior, I don't like it; the names strongly suggest more parallel (but inverse) behavior. If the new comment is right and we really want them to be non-parallel in this way, I think an explanation is in order.

---

src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts line 137 at r8 (raw file):

}

// Class that represents the whole toolbox. Gradually we will move more functionality in here.

Will we? Or is the goal now to move it all to the ToolboxRoot.tsx? It could be clearer here why both things even exist.

---

src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts line 226 at r8 (raw file):

        this.runTasksForClosingTool();
        const currentTool = getCurrentTool();
        if (currentTool && isToolOffered(currentTool.id())) {

Is there really some way a tool that is not offered can become current?? If we don't offer it, doesn't that mean this Bloom doesn't implement it, in which case, how does it have an implementation of detachFromPage()?

---

src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts line 287 at r8 (raw file):

                });
            // The checkbox may already be checked, in which case we will never be told it
            // changed, so take its state now.

I think you mean, if it is already on, we will not get an initial message saying it turned on, like we would if it started out off and was checked later. Surely we will be told it changed if the user toggles it.

---

src/BloomBrowserUI/bookEdit/toolbox/toolboxState.ts line 59 at r8 (raw file):

    // Note that the tool does not stop being current when the toolbox is hidden; it just
    // stops running (see toolboxVisible).
    readonly currentToolId: string | undefined;

There are at least four terms used in this code that roughly correspond to the current tool: current, open, running, and active.
I asked Claude what the difference is and it says they are not used consistently. Its best approximation: open means which accordion section is expanded; current: open, plus it must be "offered", that is a valid, known tool; running: current, plus the toolbox is open. That can't be quite right: if a tool isn't offered, I don't think it can have an accordion section at all, so in that sense a tool can't be open if it isn't offered, which would make open and current identical. (There is probably some state where a particular tool id has been requested by the saved state, possibly from a later version of bloom, but can't become current in any sense because it isn't offered by this version of Bloom.)
"Active" means two different things: IToolboxUiState.activeToolId means open; getActiveToolId() means something more like "current".
Right here, "currentToolId" appears to actually mean the one that is "running". and similarly "getCurrentTool() is documented to return "the tool that is running".
While we're redoing all this might be a good time to clean this up: Decide how many names we really need for this concept, define the difference clearly in one place, and make sure all variable names, method names, and comments use them consistently.
(But be careful about changing a name that is saved in meta.json...that would affect backwards compatibility. We might need to make an exception, since forward/backward migration would be messy, if a meta.json name does not correspond to what we now want to call things.)

---

src/BloomBrowserUI/bookEdit/toolbox/toolboxState.ts line 151 at r8 (raw file):

/**
 * Is the toolbox currently offering this tool? (This says nothing about whether
 * it is the active one.)

We could word this better. It sounds as if whether a tool is offered changes from moment to moment, or book to book, or something of that sort. For Bloom to encounter an id for a tool it does not offer is a strange, near-pathological state: the only known, legitimate way for it to happen is that the tool id is added by a later version of Bloom and so not known to this version. Even that is unlikely: a page last edited with a tool this version of Bloom does not have would likely trigger a rule that says this version of Bloom is not allowed to open it. Something like, "Is this tool one that this version of Bloom implements?" might work, though you may be able to do better.

---

src/BloomBrowserUI/bookEdit/toolbox/toolboxState.ts line 159 at r8 (raw file):

/**
 * The id of the first tool offered, or undefined if there are none. The "More..."
 * (settings) tool doesn't count; it is not a tool that can be current.

If it can't be current, what state is it in when it is showing? Do we mean something more like, "it can't be selected as the default tool"? Also: offeredToolIds is defined to offer the More... tool last; is this function really used in a way that would make it undesirable to offer that tool if there are no others? Which I think is impossible unless we throw away most of the toolbox, anyway. Filtering it out seems redundant.

---

src/BloomBrowserUI/bookEdit/toolbox/toolboxState.ts line 202 at r8 (raw file):

        return;
    }
    // remainingToolIds[0] is undefined when nothing is left, which leaves no tool open.

Sounds as though this is a likely state, when in fact it is impossible: many tools are always offered.