Clean up the toolbox infrastructure now that all tools are React (BL-16608) - #8109
Draft
andrew-polk wants to merge 17 commits into
Draft
Clean up the toolbox infrastructure now that all tools are React (BL-16608)#8109andrew-polk wants to merge 17 commits into
andrew-polk wants to merge 17 commits into
Conversation
Contributor
|
| Filename | Overview |
|---|---|
| src/BloomBrowserUI/bookEdit/toolbox/ToolboxRoot.tsx | Replaces legacy DOM hydration, polling, and event synchronization with directly hosted tool elements and a module adapter. |
| src/BloomBrowserUI/bookEdit/toolbox/toolbox.ts | Removes legacy accordion construction and retains toolbox registration, lifecycle, restoration, and page orchestration. |
| src/BloomBrowserUI/bookEdit/toolbox/toolboxReactAdapter.ts | Provides queued adapter registration so toolbox initialization can safely wait for the React root to mount. |
| src/BloomBrowserUI/bookEdit/toolbox/toolIds.ts | Centralizes canonical IDs and conversions for enabled settings and persisted current-tool names. |
| src/BloomBrowserUI/bookEdit/toolbox/pageEditingMarkup.ts | Moves the existing keypress, paste, CKEditor bookmark, and markup-update implementation out of toolbox.ts without changing its behavior. |
| src/BloomExe/Edit/ToolboxView.cs | Aligns the server-side toolbox configuration and persistence boundary with the consolidated React implementation. |
Reviews (2): Last reviewed commit: "Fix BL-16608 toolbox cleanup: stop switc..." | Re-trigger Greptile
andrew-polk
commented
Jul 27, 2026
andrew-polk
commented
Jul 27, 2026
andrew-polk
commented
Jul 27, 2026
andrew-polk
commented
Jul 27, 2026
Contributor
Author
|
Claude Fable 5: Consulted Devin on 2026-07-27 05:31 UTC up to commit Devin found no bugs. It raised 4 Investigate flags and 4 Informational observations. Each Investigate flag now has its own resolved thread above with the reasoning. In short, three of them were questions Devin could not answer because The Informational items were assessed and need no action; both of the substantive ones (the reader-tools readiness guard being realm-dependent, and tool bodies being built eagerly at startup) are equivalent to what master already did. CI is green and Greptile's review of this commit is clean. |
https://issues.bloomlibrary.org/youtrack/issue/BL-16608 Now that all 11 toolbox tools are React (extending ToolboxToolReactAdaptor), this removes code that is provably dead — zero callers or never-assigned state: - toolbox.ts: loadToolboxToolText (old load-HTML-template-from-server path), getToolIfOffered, ensureToolEnabled and its only helper getActiveToolIdFromCurrentToolboxUi, insertLangAttributesIntoToolboxElements plus its verbatim duplicate scan inside initialize() (only worked for non-React tools), and the ITool members hasRestoredSettings (written, never read), isExperimental() (no caller), and finishToolLocalization() (no-op impls only). - Tools: the isExperimental/toolRequiresEnterprise stubs and the beginRestoreSettings overrides that exactly duplicated the base class (canvas, game, motion, signLanguage), plus now-unused jquery imports. - toolboxBootstrap.ts/toolboxGlobals.d.ts: dead applyToolboxStateToPageLegacy and getToolIfOffered declarations. - ToolboxRoot.tsx: the never-assigned legacyToolHtmlSubPath/legacyToolBodyHtml machinery — extractFirstToolContentDivAsHtml, loadLegacyToolBodyHtml, the unreachable dangerouslySetInnerHTML render branch, the no-op disconnect-clearing effect, an effect whose guard became unconditionally true, and the dead branches of the 250ms polling effect (the load-bearing hydrate retry is preserved). No behavior change intended. Verified: pnpm typecheck passes, eslint on changed files 0 errors, full vitest suite 550 passed / 5 skipped, and build/agent-vite.sh confirms the production bundle compiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… adapter bridge https://issues.bloomlibrary.org/youtrack/issue/BL-16608 ToolboxRoot (the React toolbox UI) used to publish window.toolboxReactAdapter, and legacy code in toolbox.ts checked for it at every call site, falling back to driving the old jQuery accordion when absent. Since ToolboxRoot is always rendered before ToolBox.initialize(), those fallbacks were unreachable. - Removed every no-adapter fallback branch in toolbox.ts: setCurrentTool's entire jQuery-accordion block (header index probing, accordion option calls, the accordionactivate handler), loadToolboxTool's accordion refresh/activate branch, resizeToolbox's accordion refresh, and showToolboxChanged's switchTool fallback. Dropped the now-unused jquery.onSafe import. - Replaced the window-global handshake with a small module, toolboxReactAdapter.ts, holding the IToolboxReactAdapter interface and set/get accessors (a separate module avoids an import cycle, since ToolboxRoot.tsx imports from toolbox.ts). Timing semantics preserved: the accessor returns undefined until ToolboxRoot mounts. - Dropped IToolboxReactAdapter.isEnabled() (always true; it existed only to gate the fallbacks) and getActiveToolId() (no callers). - Removed the Window.toolboxReactAdapter declaration from toolboxGlobals.d.ts. - The Playwright harness now exposes a test-only accessor (window.getToolboxReactAdapterForTests) since page.evaluate cannot import modules; the uitest uses it. The hidden legacy #toolbox DOM and its accordion initialization still exist; they go away in the next commit. Verified: pnpm typecheck passes, pnpm lint 0 errors, full vitest suite 550 passed / 5 skipped, build/agent-vite.sh production bundle compiles. The ToolboxRoot Playwright uitests could not be run: they fail on a pre-existing component-tester issue (React 17 pinned, but the harness imports react-dom/client) that predates this branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…accordion and hidden legacy #toolbox https://issues.bloomlibrary.org/youtrack/issue/BL-16608 The React toolbox (ToolboxRoot) used to adopt its tool bodies out of a hidden legacy jQuery-UI accordion that toolbox.ts still built and kept in sync. Now ToolboxRoot builds each tool body directly from the tool's makeRootElement(), and the hidden accordion is gone entirely. - ToolboxRoot.tsx: sections own their body element; deleted the adoption machinery (ensureReactToolBodyElement, LiveToolBodyHost adoption, getExistingToolboxContentElement, getLiveToolBodyElement), the hidden <div id="toolbox">, the force-hide MutationObserver, the 250ms hydrate poll, the toolbox-tool-added/-removed CustomEvent listeners, and the CSS hacks neutralizing leaked ui-accordion classes (BL-16501/BL-16538). - IToolboxReactAdapter grew the minimal surface the legacy code needs: addTool/removeTool/hasTool/getFirstToolId, plus isToolboxUiReady(). - toolbox.ts: beginAddTool now just announces the tool to React (the hand-built <h3> header, subscription-badge helpers, loadToolboxTool, and the accordion init/localize are deleted); isToolInitialized, isToolActive, adjustToolListForPage, activateToolFromId, setCurrentTool, and showToolboxChanged all run off adapter state; removed the jquery-ui and jquery.i18n.custom imports and resizeToolbox (the root is sized by CSS). - readerToolsModel.saveState/restoreState: replaced the "does the accordion widget exist" guards with isToolboxUiReady(), preserving reader stage and level persistence; removed both accordion("refresh") calls in readerTools.ts. - audioRecording.ts: the busy-cursor element list targeted the hidden #toolbox (so the cursor never showed); now targets .toolboxRoot. - Styles: deleted the ui-accordion/ui-widget rules from toolbox.less and the six per-tool less files, and the jquery-ui theme link from both toolbox pugs. (The .ui-dialog z-index rule stays for the readerSetup dialog.) - toolboxGlobals.d.ts: dropped types that existed only for the removed window.toolboxBundle reach-through. Verified: pnpm typecheck passes (raw tsc error count unchanged at the pre-existing 61, none in touched files), eslint 0 errors, full vitest suite 550 passed / 5 skipped, build/agent-vite.sh bundle compiles. Live smoke test in Bloom over CDP: toolbox renders and fills the window, enabling/disabling Decodable Reader via More... adds/removes the section alphabetically, tool switching works, the decodable stage UI renders, reader state persistence posts fire (state\tdecodableReader\tstage:3, state\tleveledReader\t1), and no page errors occurred. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ruth for tool ids and metadata https://issues.bloomlibrary.org/youtrack/issue/BL-16608 The same tool facts were maintained in three places: toolbox.ts, hardcoded tables in ToolboxRoot.tsx (alwaysOnToolIds, subscriptionToolIds, toolIconPathByToolId, label special-casing), and kToolDefs in SettingsToolControls.tsx. - toolIds.ts now owns the canonical id scheme and every boundary conversion. The in-memory id is ITool.id() without the historical "Tool" suffix; toCanonicalToolId/toPersistedToolName/toEnabledSettingName convert at the meta.json / save-API boundaries (including the "Visualizer" no-suffix rule and the "...Check" fossil), getToolLabelInfo is the one copy of the label/i18n-key convention, and compareToolsByLabel is the shared ordering. ToolBox.addToolToString/checkLeaveOffTool and ToolboxRoot's normalizeToolId/toToolboxToolId are gone. - Per-tool UI metadata now comes from the tool classes: new optional ITool.iconPath() implemented by the nine tools that have icons; always-on and subscription-badge status are asked of the tool (isAlwaysEnabled(), featureName). The More... checkbox list is derived by filtering the registered tools (not always-enabled, not page-required, not settings) and sorting by label - byte-identical to the old hardcoded list. - /bloom/api/toolbox/enabledTools is fetched once, by ToolBox.initialize(); ToolboxRoot no longer fetches it and is populated solely through adapter.addTool. - The adapter methods now require canonical ids. Tightening exposed a latent bug: talkingBookTool.isImageDescriptionToolActive() passed "imageDescriptionTool" and only worked because the adapter was tolerant; it now uses kImageDescriptionToolId (protects BL-8515 behavior). - switchTool's fuzzy startsWith match and getITool's chop-the-suffix match are replaced with exact matches after toCanonicalToolId (equivalent for all 11 ids; no id is a prefix of another). - ToolboxView.cs: updated the stale "how to add a tool" class comment (it referenced a Settings.pug and less icon rules that no longer exist and documented the wrong i18n key convention). Persisted formats are unchanged; every read/write of meta.json tool names, the current-tool setting, and per-tool state strings was traced (all 11 derived i18n keys verified to exist verbatim in the en XLF). Verified: pnpm typecheck passes, eslint 0 errors (0 new warnings), full vitest suite 550 passed / 5 skipped, build/agent-vite.sh bundle compiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…contract https://issues.bloomlibrary.org/youtrack/issue/BL-16608 - Merged IReactTool into ITool: featureName is now an optional readonly member of ITool, and the "tool as unknown as IReactTool" casts are gone. - beginRestoreSettings is now honestly typed: beginRestoreSettings(settings: IToolboxSettings): Promise<void>. The old signature claimed a string but callers passed the settings object through "as unknown as string" and implementations cast it back. IToolboxSettings models what GET toolbox/settings actually returns (current, visibility, and one "<toolId>State" string per tool). The base implementation returns Promise.resolve() instead of a manufactured resolved $.Deferred, and the three real overrides (talkingBook, decodableReader, leveledReader) are now async methods without the double casts. jQuery import dropped from toolboxToolReactAdaptor.tsx and leveledReaderTool.tsx. - Page-access helpers deduplicated into utils/shared.ts (finishing the move the old comments described as in progress): isXmatterPage joins getPageIFrame/getPageIframeBody/getBloomPageElement, and the duplicate statics on ToolBox and ToolboxToolReactAdaptor are deleted, with all callers importing from shared.ts directly. One reconciled difference: the adaptor's isXmatter decoded the class attribute with decodeURIComponent; the merged version reads it raw (class is never URL-encoded, and decoding could throw on a stray %). Known minor timing nuance: tools that don't override beginRestoreSettings now resolve one microtask later, so showTool() runs after currentTool is assigned - strictly more consistent, and the one consumer that cared (audioRecording.doesCurrentToolPlayAudio) tolerates both orders. Verified: pnpm typecheck passes (raw tsgo error set identical to baseline), eslint 0 errors (one warning fewer than baseline, none new), full vitest suite 550 passed / 5 skipped, build/agent-vite.sh bundle compiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
https://issues.bloomlibrary.org/youtrack/issue/BL-16608 - Rewrote bookEdit/toolbox/ReadMe.txt for the current architecture (ToolboxRoot / toolbox.ts / toolboxReactAdapter / toolIds / ToolboxToolReactAdaptor) with an accurate how-to-add-a-tool recipe; the old text described the jQuery accordion and "modifying toolbox.jade". - Deleted TalkingBook-React-Conversion-Plan.md (532 lines): the conversion it plans is done, and its "retire the old non-React toolbox framework" step is this branch. Durable facts already live in AGENTS.md and ReadMe. - ToolboxView.cs: corrected the remaining stale steps in the add-a-tool class comment, and pruned GetToolboxServerDirectories to the directories something still fetches by bare name. The list feeds BloomFileLocator's bare-filename search path (not URL serving; image requests never consult it), so signLanguage, imageDescription, and canvas - whose less is bundled and whose only URL fetches are images - are removed; kept the toolbox root, talkingBook, motion, music (their css files are URL-linked from toolbox.pug / WorkspaceView.cs / editablePage.ts) and readers/readerSetup (real iframe fetch of ReaderSetup.html). - Rewrote the ToolboxRoot Playwright harness/uitest to be faithful to the new design: the harness stands in for toolbox.ts by registering real tools (ImpairmentVisualizer, SettingsTool) plus two lightweight stand-ins carrying motion/canvas ids, icons, and featureNames, then populates through the real adapter (addTool + setActiveToolByToolId) with canonical ids. All 7 test intents kept; the stale .toolbox-react-header-icon and .subscription-badge assertions (classes that never existed on the React UI) are gone. These uitests remain unrunnable due to the pre-existing component-tester React 17 / react-dom/client mismatch - verified by inspection and typecheck only. - Canvas e2e helpers now target the MUI accordion header (.MuiAccordionSummary-root with the canonical data-toolid span) instead of the deleted legacy h3 headers; the subscription-badge probe targets the real badge img. - Comment sweep: toolboxReactAdapter.ts and ToolboxRoot.tsx headers no longer claim tools are "not yet React". Verified: pnpm typecheck passes (both the main and component-tester projects show only their pre-existing unrelated errors), eslint 0 errors, full vitest suite 550 passed / 5 skipped, and build/agent-dotnet.sh build src/BloomExe/BloomExe.csproj succeeds with 0 errors. Canvas e2e and uitests not run (need a live Bloom / blocked by the pre-existing React-version issue). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…diting markup engine from toolbox.ts https://issues.bloomlibrary.org/youtrack/issue/BL-16608 Pure move. toolbox.ts still contained ~460 lines of keypress/paste handling and CKEditor coordination (handlePageEditing, scheduleMarkupUpdateAfterPaste, cleanUpNbsps, removeCommentsFromEditableHtml, setCkeditorBookmarkContent, the nbsp helpers, and the .bloom-editable handler wiring with the BL-3900/BL-12889 commentary) that keeps tool markup intact while the user types - page-editing machinery, not toolbox management. It now lives in bookEdit/toolbox/pageEditingMarkup.ts, moved verbatim. - configureElementsForTools keeps its own duty (each tool's configureElements) and delegates the handler wiring to configurePageEditingHandlers(container, toolbox), which is the engine's single initialization point. - The engine's needs from toolbox.ts (currentTool, toolboxIsShowing) are satisfied by the ToolBox instance passed at wiring time; the only import back is "import type", so the runtime dependency edge is one-way and the bundle has no circular-dependency warnings. - toolboxSpec.ts tested only the moved code, so it is renamed pageEditingMarkupSpec.ts. Stale pointer comments in audioRecording.ts and jquery.longpress.js updated; ReadMe.txt lists the new module. - toolbox.ts stays on the package.json sideEffects list (it still has module-level side effects); pageEditingMarkup.ts is not added - it has none, and its exports are directly imported. toolbox.ts shrinks from 1339 to 878 lines and is now only toolbox orchestration. Verified: pnpm typecheck passes, eslint 0 problems on changed files, full vitest suite 550 passed / 5 skipped with pageEditingMarkupSpec.ts running (5 tests), build/agent-vite.sh bundle compiles with no circular-dependency warnings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… on iconless section headers https://issues.bloomlibrary.org/youtrack/issue/BL-16608 Found during the live smoke test: sections without an iconPath (the More... section) rendered style="background-image: url()", and an empty url() resolves to the document URL, which can trigger a pointless fetch. Now the style is only set when the tool has an icon; the computed style is "none" otherwise. Verified over CDP in a running Bloom: the settings header now computes background-image: none, tool icons still render, and the full smoke pass (enable/disable tools via More..., alphabetical insertion, tool switching, decodable stage restore at Stage 3 and save posts on stage change) succeeded with no page errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ectors in canvas testing docs https://issues.bloomlibrary.org/youtrack/issue/BL-16608 Two agent-facing docs still told readers to click the toolbox tool tab with h3[data-toolid="canvasTool"]. Those legacy jQuery-accordion headers no longer exist; the toolbox is a MUI accordion whose header carries the tool's canonical id on its icon span. Both now document .MuiAccordionSummary-root:has([data-toolid="canvas"]), matching openCanvasToolTab in canvas-e2e-tests/helpers/canvasFrames.ts. Verified live over CDP in a running Bloom (with the Canvas tool enabled in the open book): the selector resolves to exactly one visible header, clicking it makes #canvasToolControls visible, and the spec-14 badge selector finds exactly one subscription badge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…vel newToolId https://issues.bloomlibrary.org/youtrack/issue/BL-16608 Two review findings from this branch's own preflight. 1. A real regression this branch introduced (found by the local review pass). Commit 784f072 renamed switchTool's parameter from newToolName to newToolId, which shadowed the module-level `newToolId` that the function's trailing `newToolId = undefined;` was there to clear. That clearing is the "done switching tools" signal getActiveToolId() depends on, so it silently became a dead store on the parameter. Effect: adjustToolListForPage sets the module variable to a page's data-tool-id (e.g. "game"), and nothing cleared it again, so on any such page getActiveToolId() kept returning "game" instead of falling back to the current tool. AudioRecording.doesCurrentToolPlayAudio() then returned false, so opening Talking Book on a game page quietly stopped setting the ui-audioCurrent highlight and the playback sound. Motion's preview-audio path is gated the same way. The parameter is now requestedToolId, with a comment warning against re-shadowing. Note that `pnpm typecheck` did not catch this: the dead store is a TS2322 ("undefined is not assignable to string"), which that gate deliberately ignores. Raw `tsgo --noEmit` reports it, and now reports zero errors in every file this branch touches. 2. Greptile: the adapter-registration effect in ToolboxRoot now uses the repository's useMountEffect helper, which utils/useMountEffect.ts documents as the preferred way to express a mount-only effect. Behavior is unchanged (its two dependencies were already stable useCallbacks). Verified: pnpm typecheck passes, eslint clean on both files, 370 toolbox tests pass, and raw tsgo confirms the shadowing error is gone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eets https://issues.bloomlibrary.org/youtrack/issue/BL-16608 toolbox.pug linked three per-tool stylesheets by URL. Two of them, motion.less and music.less, were already imported by their own components (motionTool.tsx, musicToolControls.tsx), so they were being loaded twice: once as a <link> and again as the <style> the build injects for such an import. The injected copy already won the cascade, being appended to head after the link, so dropping the links changes nothing. audioRecording.less was the only one that genuinely arrived via the link, so talkingBookTool.tsx now imports it. Its two mask-image urls had to become absolute first: an injected <style> resolves relative urls against the document rather than the stylesheet, which would have broken the current-text indicator. Absolute works for both consumers, and the page frame still links the compiled audioRecording.css (see editablePage.ts). Also removed the dead .subscription-badge rule from toolbox.less, and the @bloomEnterpriseBadgeSize variable that only it used. Nothing has rendered that class since the React header replaced the hand-built one, and its background url was broken anyway ("../../../images/..." resolved above /bloom). Removing it leaves toolbox.less free of relative urls. With motion.css and music.css no longer fetched by name, their folders leave GetToolboxServerDirectories; each remaining entry now says what still needs it. talkingBook stays, because editablePage.ts links audioRecording.css into the page frame, and the toolbox root stays because WorkspaceView injects toolbox.css as well. font-awesome stays: the Decodable Reader's three sort buttons render Font Awesome glyphs by codepoint (U+F15D etc.) with font-family: FontAwesome, so the toolbox document still needs that @font-face. Verified: pnpm typecheck passes, eslint clean, 370 toolbox tests pass, BloomExe builds with 0 errors, and build/agent-vite.sh confirms the built toolboxBundle now carries all three tools' rules inline - including the absolute currentTextIndicator url, with no relative one left. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lbox frame https://issues.bloomlibrary.org/youtrack/issue/BL-16608 The toolbox document loaded the qtip stylesheet, but nothing in that document is ever a qtip. The reader tools' tooltips (sight words, words not decodable in this stage, sentences too long for this level) are created in the page frame: readerToolsModel posts "Qtips" to the page iframe's contentWindow, and the handler there works on .bloom-content1 elements, which only exist in that document. The other qtip user, the source bubbles, likewise lives in the page frame. Both are covered by the page's own stylesheets, which include jquery.qtip.css and qtipOverrides.css (see editablePage.ts). Verified in a running Bloom over CDP rather than by reasoning alone: - The toolbox document contains zero .qtip elements at every point sampled - at startup, with the Decodable Reader active, and after editing text - while the page frame had qtip elements the whole time. - Stripping every qtip stylesheet out of the live toolbox document left it pixel-identical (screenshots byte-for-byte equal) with no console errors, and switching tools afterwards still worked. - With the toolbox's qtip CSS gone, a source-bubble tooltip in the page frame still rendered fully styled, which is the mechanism this change relies on. Note the dev-server path reaches the same document a different way: the vite-dev pug imports the stylesheet as a module, so it arrives as a <style> rather than a <link>. Removed from both, so production and dev stay in sync. Verified: pnpm typecheck passes and the toolbox suite (370 tests) is green; neither is really exercised by a pug change, so the runtime evidence above is what backs this. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hild component https://issues.bloomlibrary.org/youtrack/issue/BL-16608 Until now the toolbox had twelve React roots. Each tool's makeRootElement() created a detached div and called createRoot() on it, and ToolboxRoot hosted that foreign subtree with a ToolBodyHost component that appendChild'd it. So from React's point of view every tool was opaque DOM rather than a child: context could not cross the boundary, lifecycle was driven imperatively, and re-enabling a tool leaked the previous root. ITool.makeRootElement(): HTMLDivElement is replaced by renderPanel(): React.ReactNode, and ToolboxRoot renders {tool.renderPanel()} inside the section's AccordionDetails. There is now exactly one root in the toolbox document. ToolBodyHost and ToolboxToolReactAdaptor.adaptReactElement are gone. Most tools just return their existing element. Three needed real work: - motion used renderForInstance and then synchronously setState'd the initial state. It now keeps a stable callback ref; when a control instance first attaches, the initial state and the image observer are applied, and a separate initializedRootControl field preserves the old re-initialize behavior when a new instance mounts. The four methods that use the control take a local non-null copy first. - the game tool re-rendered itself by calling a private renderRoot() on demand, which composition makes impossible. It is now a small external store: the tool owns tab/pageGeneration and the panel subscribes with useSyncExternalStore, so setActiveTab notifies instead of re-rendering. - sign language rendered through a static setup() helper; that is deleted in favor of an ordinary ref. Each renderPanel keeps the plain wrapper div its makeRootElement used to create (with the same class where there was one), so the rendered DOM and the CSS box model stay as they were - the reader tools and Talking Book have internal height rules that would otherwise collide with the wrapper's fill-the-section rule. The suffixed data-toolid moved onto the wrapper ToolboxRoot renders, so existing stylesheets and test selectors still match. Deliberately not done here: the per-tool ThemeProviders are now redundant (context reaches the tools), but removing them risks visual change and several of those components are used outside the toolbox, so they are left for a follow-up. Behavior change worth knowing: disabling a tool in More... now genuinely unmounts its panel - that is the leak fix - so cleanup effects run and the tool's React state resets when it is re-enabled. Verified: pnpm typecheck passes (zero errors in any changed file under raw tsgo), eslint identical to baseline (0 errors, same 41 pre-existing warnings), full vitest suite 550 passed / 5 skipped, build/agent-vite.sh builds the real bundle. Live in a running Bloom over CDP: all ten enabled tool sections render with correct data-toolid, full height and real content, in alphabetical order with More... last; switching between them works; disabling and re-enabling a tool removes and rebuilds its panel correctly; and no console or page errors occurred throughout. The Game tool is the one panel not exercised live - it needs a game page, which I could not add through the page chooser in automation - so its tab switching wants a manual check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t tree made redundant https://issues.bloomlibrary.org/youtrack/issue/BL-16608 Now that ToolboxRoot renders each tool's panel as an ordinary child of its own tree, the MUI theme reaches the panels through context, so the eleven `<ThemeProvider theme={toolboxTheme}>` wrappers the tools carried - each of them there only because the tool used to be its own createRoot - are redundant and are removed, along with the imports that became unused. Removed from CanvasToolControls, GameTool's DragActivityControls, ImpairmentVisualizerControls, MotionControl, MusicToolControls, the Decodable and Leveled reader controls, ReaderToolSwitch, SettingsToolControls, TalkingBookToolControls, and talkingBookAdvancedSection. Deliberately kept: - All ten `toolboxMenuPopupTheme` providers. Those are a deliberate override for menus and popups, not a re-statement of the toolbox theme. - DragActivityTabControl's provider. That component is rendered into the page document by AbovePageControls (via editablePage.ts), so it is its own root outside ToolboxRoot's tree and still needs its own theme. Since ThemeProvider renders no DOM, the rendered markup is unchanged in shape; only the nesting of React context differs. Most of the diff is prettier re-wrapping text and expressions that dedented - `git diff -w` reduces it to the eleven provider pairs, the import edits, and one added fragment (the Talking Book provider had two children). Verified: pnpm typecheck passes, eslint identical to baseline on the changed files (0 errors, same 36 pre-existing warnings), full vitest suite 550 passed / 5 skipped, build/agent-vite.sh builds the real bundle. Also checked statically that nothing between ToolboxRoot and these panels provides a different theme, and that no code under bookEdit/toolbox reads the theme programmatically. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lpers https://issues.bloomlibrary.org/youtrack/issue/BL-16608 An earlier commit made utils/shared.ts the single home for getting at the page being edited, but left three near-duplicates alone because each returned something slightly different. They are now gone: - motionTool's private getPageFrame()/getPage(). getPage() returned the page *document*, which is why it survived before; shared.ts gains a named getPageIframeDocument() for that, and getPageIframeBody() now derives from it. Ten call sites rewired. - audioRecording's public getPageFrame(). No external callers, so deleted; four call sites rewired. Its getPageDocBody() stays as a one-line delegation, because it is declared on IAudioRecorder and used by two spec files. - editableDivUtils' getPageFrame()/getPage(), plus two inline lookups in the same file. I had believed the editableDivUtils pair was dead; it was not. getPageScale() calls getPage() as this.getPage(), which my search for qualified calls missed, and getPageScale has 13 callers across the edit code. Rather than delete a live path, getPageScale now builds the same jQuery from getPageIframeBody(), keeping the empty-jQuery fallback so all of its existing jQuery logic behaves identically. One real semantic difference had to be settled: editableDivUtils looked the frame up in window.top while everything else uses parent. Standardized on parent - what shared.ts, audioRecording and workspaceFrames already use - and recorded why in a comment on getPageIFrame() and at the getPageScale call site. For the frames involved (page and toolbox, both direct children of the workspace) the two resolve to the same document, and parent has the better failure mode: a caller in a deeper frame gets null rather than silently reaching the outermost document. No behavior change intended: every early return and guard is preserved, and no call site was pushed onto a different type. Verified: pnpm typecheck passes (raw tsgo reports zero errors in the changed files), eslint byte-identical to baseline (0 errors, 78 pre-existing warnings), full vitest suite 550 passed / 5 skipped, build/agent-vite.sh builds the real bundle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…based toolbox https://issues.bloomlibrary.org/youtrack/issue/BL-16608 Rebasing onto master brought in 597fa34, which fixed BL-16602: when the toolbox removes the section that is currently open - which is what happens on leaving a game page, since the Game tool is only offered on game pages - it has to tell toolbox.ts, because toolbox.ts owns each tool's showTool()/hideTool() lifecycle. Without that, it went on believing the removed tool was current and never activated the tool that replaced it, so Talking Book stopped highlighting and playing audio. That fix lived in the code this branch rewrote, so replaying the branch on top of it dropped it. This puts the behaviour back in the new design: - ToolboxRoot now mirrors the expanded section in a ref, the same way it already mirrors the section list, so removeTool() can tell synchronously whether it is removing the open one without the previous functional-update dance. - Removing the open section routes its replacement through makeToolActive(), which notifies toolbox.ts. Removing any other section still leaves the open one alone, and when nothing remains we deliberately don't notify, since toolbox.ts has no way to represent "no current tool". Note this branch notifies with the CANONICAL tool id ("game"), where master notified with the suffixed one ("gameTool"): since commit 4/7 the conversion to the persisted spelling happens in switchTool itself, so copying master's line verbatim would have broken it. Master's new ToolboxRoot.spec.tsx is adapted rather than dropped. It was written against window.toolboxReactAdapter, the CustomEvent pairs and the enabledTools fetch, all of which this branch replaced, so it now drives the module-level adapter and adds sections through addTool(); its axios mock now rejects everything, asserting that ToolboxRoot makes no requests of its own. Its two intents are unchanged, and I confirmed it still earns its keep by reverting the fix and watching it fail with "expected [ 'game' ] to deeply equal [ 'game', 'talkingBook' ]" - the canonical-id twin of the failure master's author reported. Verified after the rebase: pnpm typecheck passes, eslint clean on the changed files, the full vitest suite is 568 passed / 5 skipped, and build/agent-vite.sh builds the real bundle. Also checked that master's other work in the files this branch touches survived: the four BL-15300 audio-highlight commits in audioRecording.ts, the @font-face declaration in toolbox.less, and the BL-16574 null-guard in leveledReaderTool, which is merged with this branch's typed async beginRestoreSettings rather than replaced by it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…l tools https://issues.bloomlibrary.org/youtrack/issue/BL-16608 This rebase picked up master's PR #8113, which got the Playwright component-test harness running again - the pre-existing React 17 / react-dom/client mismatch that had made every one of these suites unexecutable, and the reason this branch's rewritten toolbox uitests had only ever been checked by inspection. They run now, so this adopts master's approach in the toolbox harness: register all eleven real tools, with master's guard keyed off the shared master list (a module-local flag would be reset by React Refresh while masterToolList kept its entries, giving duplicate sections). That replaces the two stand-in tools this branch had been using for Motion and Canvas, which existed only because their real implementations were assumed too heavy to host outside the real toolbox. With every tool now rendering through renderPanel() they are not, so the harness exercises the real panels. Kept from this branch: the test-only window.getToolboxReactAdapterForTests accessor (Playwright runs in the page and cannot import the module the way toolbox.ts does), and populating the toolbox through the real adapter's addTool(), since ToolboxRoot no longer fetches the enabled-tools list itself. Result: the suite runs and all 8 tests pass. There is console noise from ImpairmentVisualizerControls.componentDidMount doing a get() that 404s, because the component harness has no Bloom server behind it; it is harmless and does not fail anything. Verified on the rebased branch: pnpm typecheck passes, the full vitest suite is 568 passed / 5 skipped, build/agent-vite.sh builds the real bundle, BloomExe builds with 0 errors, and the toolbox uitests are 8 passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
andrew-polk
force-pushed
the
BL-16608-toolbox-cleanup
branch
from
July 27, 2026 23:28
94daac6 to
c9c368d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All 11 toolbox tools have been React for a while, but the toolbox infrastructure still ran the old jQuery-UI accordion in parallel:
toolbox.tsbuilt tool DOM into a hidden#toolboxdiv and initialized a jQuery accordion on it, whileToolboxRoot.tsxrendered the real (MUI) UI by adopting those DOM nodes back out through a bridge ofwindow.toolboxReactAdapter, CustomEvents, a MutationObserver, and a 250 ms polling loop. This removes the legacy half and the bridge.Net effect: about 2,100 lines removed,
toolbox.tsgoes from 1,948 to 878 lines and is now only toolbox orchestration.Reviewing commit-by-commit is much easier than reviewing the squashed diff — each commit is one self-contained phase and each was verified green before the next.
IToolmembers (isExperimental,finishToolLocalization,hasRestoredSettings), andToolboxRoot's never-assigned legacy-HTML machinery.ToolboxRootalways mounts first);window.toolboxReactAdapterbecame a small module accessor.#toolboxToolboxRootbuilds tool bodies directly frommakeRootElement(). Gone: the adoption machinery, MutationObserver, CustomEvent plumbing, accordion init, and all.ui-accordionCSS.toolIds.tsowns canonical unsuffixed ids plus the boundary shims for the persisted formats; icons/always-on/badges now come from the tool classes; the More… checkbox list is derived, not hardcoded;enabledToolsis fetched once instead of twice.IReactToolfolded intoITool;beginRestoreSettings(settings: IToolboxSettings): Promise<void>(it previously claimed astringwhile callers passed the settings object through a double cast, and used jQuery Deferreds); page-access helpers consolidated intoutils/shared.ts.ToolboxView.csrecipe corrected andGetToolboxServerDirectoriespruned; Playwright harness/uitest rewritten; canvas e2e selectors moved to MUI.toolbox.tsintopageEditingMarkup.ts— it was page-editing code, not toolbox code.Two small follow-ups found during live testing are at the end: an iconless header no longer renders
background-image: url(""), and two agent-facing docs that still documented the deletedh3[data-toolid=…]selector.Latent bugs fixed along the way
#toolboxdiv, so it never actually appeared; it now targets.toolboxRoot.talkingBookTool.isImageDescriptionToolActive()passed"imageDescriptionTool"and only worked because the adapter was tolerant of either spelling. Tightening the adapter surfaced it (protects the BL-8515 behavior).Verification
Per phase:
pnpm typecheck, eslint (0 errors), the full Vitest suite (550 passed / 5 skipped), and a production bundle build viabuild/agent-vite.sh; the C# change also builds viabuild/agent-dotnet.sh.Live smoke testing in a running Bloom over CDP covered: startup/restore, tool switching, enabling and disabling tools from More… (alphabetical insertion and removal), the derived checkbox list and its subscription badges, the decodable-reader stage UI,
data-tool-id="game"pages adding and removing the Game section, and the canvas e2e header/badge selectors.Persisted formats were verified end-to-end on disk:
active\tcanvasCheck\t1still lands inmeta.jsonas{"name": "canvas", "enabled": true}(canonical name,Checkstripped by the C# side) andcurrent\tcanvasToolascurrentTool: canvasTool(suffixed), with per-tool state strings preserved.Two test-infrastructure limitations, both independent of this branch and detailed on the YouTrack card: the ToolboxRoot Playwright uitests cannot run at all (the component-tester pins React 17 but imports
react-dom/client— pre-existing, verified by stashing), and the canvas e2e suite cannot complete locally because the Vite dev server exhausts its heap under that suite's load.Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16608
Devin review
This change is