Preflight report — toolbox cleanup 4/4

BloomDesktop · BL-16608-toolbox-3-react · head b09b15fb55 · 2026-10-08

Ready for review Mergeable All reviewers complete All decisions answered

PR #8447 Files changed Commits BL-16608 Devin review

What this PR is about

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 before restoring the book’s saved settings, but it applied settings read before that wait, so anything the user did meanwhile was undone — a toolbox just opened was shut again. People click again and move on; seven e2e tests could not, and were skipped.

What the PR does

One React root. Each tool is an ordinary child rendered through ITool.renderPanel(), so React context reaches tools normally and the per-tool ThemeProviders go.
toolboxState.ts is the single source of truth for what is offered, open, enabled and shown. React subscribes with useSyncExternalStore; toolbox.ts reads and writes it.
Which tool is running is derived, not stored. The two copies can no longer disagree, which makes the BL-16602 class of bug unrepresentable rather than fixed.
Each tool’s lifecycle runs from a React effect, so a tool runs because it is the current tool of a showing toolbox, not because something remembered to activate it.
Neither late restore overwrites a decision made after its settings were read, and a reader stage or level the book had saved is no longer silently discarded when those settings are slow or fail to load (the hold, the replay) — a long-standing bug, 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).

Quality gate

Check	Result
Typecheck (tsc)	No new errors. 81 exist at HEAD and all pre-date this branch; the 3 added by the base merge are in master’s own files (`bloomEditing.ts`, `CanvasElementKeyboardProvider.test.ts`), not in anything this branch touches.
Lint (eslint)	0 errors across the 34 changed files (142 warnings, all long-standing style warnings in files this PR did not introduce).
TypeScript tests (vitest)	1213 passed in 98 files — the full suite, on the merged result.
C# tests (dotnet)	Not run — the only C# in the diff is a comment in `ToolboxView.cs`, confirmed by diffing out comment lines. Nothing in the diff can reach the C# suite.
End-to-end (Playwright)	8 passed across both changed specs, headless at the nightly CI window size. See the first decision about an earlier contended run.
Merge with master	Merged. One conflict, in `CanvasToolControls.tsx`, where master’s Bloom Tables work and this branch’s removal of the per-tool `ThemeProvider` touched the same JSX. Resolved by taking master’s file and re-applying only the wrapper removal, then diffing against master to confirm nothing from Tables was lost.

What changed this run

Fixed a long-standing silent drop of a book’s saved reader stage or level, and the cross-book leak that holding the value introduced — raised by the local review and, independently, by Devin.
Hardened the panel-clipping e2e check so it can no longer pass without examining anything.
Merged master (Bloom Tables), resolving one toolbox conflict.

All commits · 3640c1d0 cross-book guard · b09b15fb clipping check

Reviewer outcomes

Reviewer	Outcome
Local review light sub-agent pass	Complete. One finding fixed: `clearForTest()` did not reset the flag that gates `restoreState()`, which could have let a later test pass for the wrong reason. One open question it could not settle — whether a held stage or level could cross into the next book — which I confirmed is reachable and fixed with a guard plus a regression test that fails without it.
Devin	Complete on this HEAD (4 min). Four bugs: three it marks resolved, one assessed and resolved on its thread in an earlier run. Five investigate flags: one fixed this run, one assessed as already covered by a sibling spec, two raised as decisions here, one handled previously. Devin independently found the cross-book leak and marks it resolved at this HEAD. Threads
Greptile	Has not reviewed this PR — it has never posted here, including after an explicit `@greptile-apps review` bypass on 2026-10-07. Recorded as absent rather than waited out.
CodeRabbit	Has not reviewed this PR. The repo carries a `.coderabbit.yml`, but it has posted nothing here.
CI	Complete. `pr-automation` passed. `code-review/reviewable` reads pending because it tracks open review discussions — the two left open are the decisions opposite — not because a build is running.
Repo check e2e at nightly window size	Passed on the final code — all 8 tests in the two changed specs. An earlier run of the same check failed two of them; that is the first decision.

Worth knowing

The game-page path still has not been confirmed by hand and has no e2e test — visiting a game page offers the Game tool and leaving withdraws it, the original BL-16602 scenario. It has unit coverage only.
Typecheck is not clean on master either. Running tsc on this branch shows 81 errors and none of them are this branch’s — worth knowing before chasing one.
Running e2e locally needs pnpm --dir src/content run build:pageSizes first after taking current master, or every test fails with “The Edit tab never showed a page”. The file it generates is git-ignored and read by C#, so neither a dev server nor dotnet build supplies it.

Decisions

All three were answered on 2026-10-08 and are no longer open. Each one's record lives where the next person will be standing — on its review thread, or in the code — rather than here.

The two e2e tests that failed in one run and passed in the next: left as machine contention, nothing recorded.
The clipping assertion: dropped, in 2f805f35. Removing it exposed a race it had been masking — openTool and clickToolHeader decided a tool was absent from a single check — which is fixed in the same commit.
The unexplained nightly failure: the deferral in docs/nightly-failures/README.md stands.

Next step

Nothing is waiting. The PR is a draft with every reviewer complete, every suite green and no open review threads. After your own review, /pr-ready-for-human squashes it and hands it to a colleague.