Skip to content

Register the toolbox tools from one list both callers share (BL-16799) - #8296

Merged
hatton merged 2 commits into
masterfrom
BL-16799-toolbox-registration
Sep 3, 2026
Merged

Register the toolbox tools from one list both callers share (BL-16799)#8296
hatton merged 2 commits into
masterfrom
BL-16799-toolbox-registration

Conversation

@hatton

@hatton hatton commented Sep 3, 2026

Copy link
Copy Markdown
Member

ToolboxRoot renders a section only for a tool that is in the master tool list,
and a tool gets there by being registered. That happened as a side effect of
loading toolboxBootstrap.ts, which also renders its own toolbox root and
assigns window.toolboxBundle. A test harness cannot afford those side effects,
so react_components/ToolboxRootTestHarness duplicated all eleven
ToolBox.registerTool calls with a "keep this in sync" comment.

bookEdit/toolbox/registerAllToolboxTools.ts is now that list and nothing else:
importing it registers nothing, and both toolboxBootstrap.ts and the harness
call registerAllToolboxTools().

Calling it twice registers nothing the second time. The check is per tool
rather than "is the list empty", because a list holding some other tool is no
evidence that these eleven are registered, and skipping them all on that
evidence would leave the toolbox with no sections at all. It reads the shared
master list rather than a flag in the module, because a caller that is a
React-Refresh boundary re-executes its own module during pnpm dev while
masterToolList keeps its entries.

The caller passes an id and a factory rather than a tool, so a tool the list
already has is never constructed: CanvasTool and GameTool each point a static
field at the instance being constructed, so building one only to discard it as
a duplicate would leave every reader of that field holding a tool the toolbox
does not know about.

Each id is checked against the tool's own id() at registration, because some of
these tools name their id in a constant and some in a string literal, and
nothing else ties the two together. That check earns its keep: it caught
SignLanguageTool.kToolID, which does not exist, the static belonging to
SignLanguageToolControls.

The AUTOMATION-DEBT.md entry becomes "One toolbox harness test asserts on
classes that do not exist", which is the half of it this does not fix.

Verified: the ToolboxRootTestHarness Playwright suite passes, 7 passed and 1
skipped, which is the test.fixme in that entry. Type check and lint clean.

This is one of eleven stacked pull requests (BL-16799)

Each one pays down one entry of src/BloomE2E/AUTOMATION-DEBT.md, and each branches off the one before it. Base: BL-16799-page-screenshot. Review only this pull request's own commit; the ones below it are reviewed in their own pull requests. The first six change test and tooling code only; the last five also change product code.

  1. BL-16799-automation-scripts — Make the bloom-automation scripts safe to ask for help
  2. BL-16799-vr-collect-failures — Report every failed image comparison in a visual-regression case, not the first
  3. BL-16799-component-tests-in-ci — Run the component-tester Playwright suites nightly
  4. BL-16799-vite-port — Let an e2e run test the working tree's front end
  5. BL-16799-type-in-one-call — Type into a text box in one call, not one key press per character
  6. BL-16799-page-screenshot — Capture a whole book page from a test
  7. BL-16799-toolbox-registration — Register the toolbox tools from one list both callers share
  8. BL-16799-shell-document — Stop a test attaching to a shell document Bloom does not drive
  9. BL-16799-tab-test-ids — Click a workspace tab by a test id, not by its localized label
  10. BL-16799-page-change — Refuse a page change the Edit tab cannot do, and wait before asking
  11. BL-16799-collection-languages — Set a collection's languages through an e2e hook, not by writing XML

Replaces #8276, which did all of this in one pull request.

Verification of the whole stack, at its tip: the C# suite passes (3338 passed, 13 skipped), the front-end vitest suite passes (781 passed, 5 skipped), and the src/BloomE2E suite passes against a Vite dev server on the working tree (36 passed, 0 skipped, 8.2 minutes). Each pull request also has its own type check and lint.

🤖 Generated with Claude Code

Devin review


This change is Reviewable

ToolboxRoot renders a section only for a tool that is in the master tool list,
and a tool gets there by being registered. That happened as a side effect of
loading toolboxBootstrap.ts, which also renders its own toolbox root and
assigns window.toolboxBundle. A test harness cannot afford those side effects,
so react_components/ToolboxRootTestHarness duplicated all eleven
ToolBox.registerTool calls with a "keep this in sync" comment.

bookEdit/toolbox/registerAllToolboxTools.ts is now that list and nothing else:
importing it registers nothing, and both toolboxBootstrap.ts and the harness
call registerAllToolboxTools().

Calling it twice registers nothing the second time. The check is per tool
rather than "is the list empty", because a list holding some other tool is no
evidence that these eleven are registered, and skipping them all on that
evidence would leave the toolbox with no sections at all. It reads the shared
master list rather than a flag in the module, because a caller that is a
React-Refresh boundary re-executes its own module during pnpm dev while
masterToolList keeps its entries.

The caller passes an id and a factory rather than a tool, so a tool the list
already has is never constructed: CanvasTool and GameTool each point a static
field at the instance being constructed, so building one only to discard it as
a duplicate would leave every reader of that field holding a tool the toolbox
does not know about.

Each id is checked against the tool's own id() at registration, because some of
these tools name their id in a constant and some in a string literal, and
nothing else ties the two together. That check earns its keep: it caught
SignLanguageTool.kToolID, which does not exist, the static belonging to
SignLanguageToolControls.

The AUTOMATION-DEBT.md entry becomes "One toolbox harness test asserts on
classes that do not exist", which is the half of it this does not fix.

Verified: the ToolboxRootTestHarness Playwright suite passes, 7 passed and 1
skipped, which is the test.fixme in that entry. Type check and lint clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hatton
hatton force-pushed the BL-16799-toolbox-registration branch from 49fa429 to 722f413 Compare September 3, 2026 19:40
Master moved while this stack was open, so each branch merges the one below it.
The conflicts were all in src/BloomE2E/AUTOMATION-DEBT.md: master gave the table
of this stack a pull-request column and annotated each promise with its number,
while this branch deletes its own row and replaces or deletes its own promise.
The merge keeps both, taking this branch text where the two describe the same
entry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BaWw4EHWrbFF2fNJvMJVC8
@hatton

hatton commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

[Claude Opus 5 from Hatton's machine during devin-review]

Consulted Devin on 2026-09-03 22:45 UTC up to commit cf4ea27c67929f453c0710f11dcf10ca20e70bc4.

Base automatically changed from BL-16799-page-screenshot to master September 3, 2026 23:04
@hatton
hatton merged commit 40670fe into master Sep 3, 2026
1 check passed
@hatton
hatton deleted the BL-16799-toolbox-registration branch September 3, 2026 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant