Skip to content

Pay down the automation debt in the e2e suite (BL-16799) - #8276

Closed
hatton wants to merge 17 commits into
masterfrom
BL-16799-headless-e2e
Closed

Pay down the automation debt in the e2e suite (BL-16799)#8276
hatton wants to merge 17 commits into
masterfrom
BL-16799-headless-e2e

Conversation

@hatton

@hatton hatton commented Sep 2, 2026

Copy link
Copy Markdown
Member

BL-16799

Split, 2026-09-02. The off-screen window this branch started with now lives on its own card and PR: BL-16804 / #8285. This PR keeps the automation debt. Both branch off master, neither depends on the other, and they can merge in either order.

Problem

Making the src/BloomE2E suite run off-screen exposed a row of entries in src/BloomE2E/AUTOMATION-DEBT.md that made the suite unreliable or hard to extend. The worst let a test pass while Bloom drove a document the test never looked at, so the test asserted on its own typing and every page Bloom loaded went somewhere invisible.

Fix

  • A test can no longer attach to the wrong document. More than one document in a run carries the top bar, so the fixture could attach to a shell Bloom does not drive. An e2e run now shares one WebView2 environment for browsers built on the UI thread, and a new e2e/shellUrl hook names the document Bloom drives, which the fixture confirms. This was the cause of the flake recorded above publish-text-languages.spec.ts.
  • A run can test the working tree. BLOOM_E2E_VITE_PORT=<n> passes --vite-port to the launched Bloom, so a front-end edit reaches the suite without the full front-end build.
  • A page change is no longer lost while the Edit tab loads a page. editView/jumpToPage refuses such a jump and says so, instead of replying success and dropping it, and every helper that changes the page waits for the Edit tab to settle first. The three-attempt loop in goToPage no longer hides a real failure. A jump asked for while the Edit tab is not showing is remembered and shown when that tab opens, which is what the Book Settings style links do from the Collection tab; before, Bloom showed the last edited page instead.
  • No test composes collection XML any more. A new e2e/setCollectionLanguages hook does the work of the Collection Settings dialog's OK button.
  • The suite works in any UI language. The workspace tabs carry data-testid attributes, so no test matches a localized label. The component tester's dead #main-tabs button selector is fixed too.
  • Typing is one call, not one key press per character.
  • Other debt paid. A visual-regression case collects every failed image comparison and fails once at the end. Toolbox tool registration is a single registerAllToolboxTools() that both the bootstrap and the test harness call, and it skips a tool the toolbox already has, because the Canvas and Games constructors each assign themselves to a static field and a discarded duplicate would corrupt it. The component-tester Playwright suites have a nightly job. The bloom-automation scripts answer --help without killing anything and reject an unknown flag. A screenshot helper captures a whole book page.
  • Suite maintainability. The add-e2e-test skill now states that every step of a test is a helper call: a test says what happens, and src/BloomE2E/helpers/ says how. The suite is heading for a few thousand tests.

One expectation changed on purpose

publish-text-languages.spec.ts (Notion Test Case ID 169) expected a dropped language to appear in the publish list under its own name, "español". That held only because the old test discarded the language by rewriting the collection file. The code behind the Settings dialog's OK button moves a displaced language to the end of the collection's list, keeping its name, so the list shows "Spanish". The test now expects that, and what the test exercises is unchanged.

That also retires a flake: the old assertion returned "espagnol", French for Spanish, about one run in seven. AUTOMATION-DEBT.md records the Bloom nondeterminism behind it, since no test covers it any more.

Verification

  • The full src/BloomE2E Playwright suite: 29 passed, 7.8 minutes, against a dev server on the
    working tree.
  • The whole C# suite: 3314 passed, 0 failed, 13 skipped.
  • The front-end suite: 781 passed, 5 skipped. Type check and lint clean.

Not verified: the visual-regression change, by type check and reading only, because that suite cannot go green on a developer machine; and the new nightly job, because CI has not run it.


This change is Reviewable

Devin review

hatton and others added 2 commits September 2, 2026 07:31
… it (BL-16799)

Every run of the src/BloomE2E suite took over the developer's desktop, and a test
could pass while Bloom drove a document the test never looked at.

A new --headless flag puts the shell and the splash screen far left of every
monitor, out of the taskbar, without touching the saved window placement. The
window is off-screen rather than minimized because WebView2 stops painting a
minimized window and screenshots come back blank; BLOOM_E2E_HEADED=1 or
Playwright's --debug shows it.

Browsers built on the UI thread share one WebView2 environment under --e2e, so a
run has one browser process and one remote-debugging listener, and a new
e2e/shellUrl hook names the document Bloom drives for the fixture to confirm.
That sharing is limited to the UI thread: an environment belongs to the thread
that created it, and publishing a BloomPUB builds its browsers on the thread
serving the API call.

editView/jumpToPage now queues a jump that arrives while the Edit tab is not
showing, is navigating, or is saving, and reports a failure when it can do
neither, instead of replying success to a jump it dropped. A new
e2e/setCollectionLanguages hook does the work of the Collection Settings
dialog's OK button, so no test composes .bloomCollection XML. The workspace tabs
carry data-testid attributes, so the suite no longer matches on localized
labels. BLOOM_E2E_VITE_PORT points a launched Bloom at a dev server, so a
front-end edit reaches the suite with no build.

Also: a visual-regression case collects every failed image comparison and fails
once at the end; toolbox tool registration is a side-effect-free
registerAllToolboxTools() that both the bootstrap and the test harness call; the
component-tester Playwright suites have a nightly job; the bloom-automation
scripts answer --help without killing anything and reject an unknown flag; a
screenshot helper captures a whole book page the safe way; and the add-e2e-test
skill states that every step of a test is a helper call.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR moves Bloom’s end-to-end runs off-screen and makes their CDP attachment deterministic while expanding and simplifying the automation APIs. It also improves page navigation sequencing, screenshot capture, toolbox registration, visual-regression diagnostics, automation CLI safety, and nightly component-test coverage.

  • Adds off-screen shell and splash placement without persisting automation window bounds.
  • Shares the e2e UI-thread WebView2 environment and verifies the shell document through an e2e-only endpoint.
  • Adds collection-language, page-jump, workspace-tab, text-entry, and full-page screenshot automation support.
  • Aggregates visual-regression comparison failures and schedules component Playwright tests nightly.

Important Files Changed

Filename Overview
src/BloomExe/Edit/EditingModel.cs Queues page jumps through navigation and save transitions; no publishable current lifecycle failure was established.
src/BloomExe/Edit/EditingStateMachine.cs Adds one-slot deferred work for navigation completion while preserving the existing save-completion mechanism.
src/BloomExe/WebView2Browser.cs Shares UI-thread WebView2 environments during e2e runs and disables occlusion throttling for off-screen rendering.
src/BloomE2E/fixtures/bloomTest.ts Identifies the shell target by stable marker, HTTP port, and Bloom’s reported driven URL.
src/BloomE2E/helpers/screenshot.ts Captures oversized elements through bounded device-metrics overrides with CDP deadlines and cleanup.
src/BloomExe/web/controllers/E2eTestingApi.cs Adds e2e-only endpoints for deterministic shell discovery and collection-language updates.
src/BloomVisualRegressionTests/index.spec.ts Accumulates image-comparison failures so all images in a case are captured before the test fails.
.github/workflows/nightly.yml Adds installation, execution, reporting, and artifact handling for component-tester Playwright suites.
src/BloomBrowserUI/react_components/LinkTargetChooser/component-tests/error-handling.uitest.ts Enlarges four explicit waits with justification but without recording the approval required by repository guidance.
src/BloomBrowserUI/react_components/registration/component-tests/test-helpers.ts Enlarges the registration opt-out wait buffer without recording the required approval.

Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mas..." | Re-trigger Greptile

Comment thread .github/skills/bloom-automation/killBloomProcess.mjs
// loaded machine: two of these tests failed about once per full-suite run and passed when run on
// their own. These are waits for a state rather than sleeps, so the larger number costs a passing
// run nothing.
const kErrorAppearsTimeoutMs = 10000;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Devin] Investigate: Explicit test timeouts need approval

src/BloomBrowserUI/AGENTS.md says: "Don't use timeouts in tests, that slows things down and is
fragile. If a timeout is justified, get my approval and add a comment explaining it."

This PR raises two waits and explains both in a comment, but nobody has approved either:

  1. kErrorAppearsTimeoutMs = 10000 here, up from 1000 ms. Two of these tests failed about
    once per full-suite run and passed when run alone. This is a wait for the error message to
    appear or clear, not a sleep, so a passing run still finishes as fast as it ever did; only a
    failing one waits the extra time.
  2. A 120-second per-test limit on the new component-tester job in .github/workflows/nightly.yml,
    because a CI runner starting a browser is slower than a developer's machine.

Both are held for John to accept or reject.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Opus 5 from Hatton's machine during preflight] Greptile raises the same point, and names a
third place, so the decision covers all three:

  1. kTestOptOutTimeoutMs in react_components/registration/component-tests/test-helpers.ts,
    buffer raised from 2000 ms to 8000 ms on top of the component's own 2-second delay. The last
    worker to start missed the opt-out button by a fraction of a second, so one test failed about
    once per full-suite run and passed on its own.

All three are waits for a state to arrive, so a passing run finishes as fast as it ever did. All
three carry a comment saying why. What none of them has is your approval, which is what the rule
asks for. This thread stays open until you decide.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Opus 5 (1M context) from Hatton's machine during preflight]

Devin (Investigate): explicit test timeouts need approval.

This branch raises two waits in the front-end component tests: the wait for an error message to appear here (1000 ms to 10000 ms), and the wait for the registration opt-out button in registration/component-tests/test-helpers.ts (the delay plus 2000 ms, now plus 8000 ms). Both were raised because the tests failed about once per full-suite run on a loaded machine and passed when run alone. Both are waits for a state, not sleeps, so a passing run costs nothing extra.

Devin is right that this repo has a rule about it. src/BloomBrowserUI/AGENTS.md says: "Don't use timeouts in tests, that slows things down and is fragile. If a timeout is justified, get my approval and add a comment explaining it." The comment is there. The approval is not.

Outcome: this goes to the developer as a decision. The thread stays open until that answer comes back.

killBloomProcess.mjs read --pid and --watch-pid with Number(), so "--pid abc",
"--pid" with no value, and "--pid 0" all produced NaN. NaN is falsy, so the
script decided no target had been named and fell through to its no-target
behavior, which is to kill every Bloom this worktree owns. A mistyped process
id therefore threw away unsaved edits in every Bloom on the machine.

A new requireProcessIdOption in bloomProcessCommon.mjs validates all four
spellings of the two options at parse time and exits non-zero before the script
looks at any process. The "did the caller name a target?" test now asks whether
the option was supplied rather than whether its value is truthy, so another
target option cannot reintroduce this.

Also, captureElement now says on standard error when it cannot clear the window
size override, rather than swallowing the failure: the module promises the
override is always cleared, and a run that silently kept an 8000-pixel window
would make every later test in that worker see the wrong Bloom.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/BloomE2E/helpers/screenshot.ts Outdated
captureElement enlarges Bloom's window so a whole book page fits, then puts it
back in a finally. A failed put-back only wrote a line to standard error, so the
test passed and every later test in that Playwright worker drove a Bloom several
thousand pixels wide. Whatever those tests measured or clicked was measured
against a window no user has.

The failure now throws, so the test that made the window big is the test that
fails. The exception is a clear failure that arrives while a capture error is
already on its way out: that error is what the test needs to read, so throwing
over it would hide why the capture failed. The code tells the two apart by
whether an image was produced.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hatton

hatton commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

[Claude Opus 5 (1M context) from Hatton's machine during preflight]

Consulted Devin on 2026-09-02, up to commit 7bee9a64449b6149d832928b2e483480e59e987d. Nine rounds, one per pushed commit. This replaces the earlier version of this note, which named an older head.

Against the current head Devin marks all six of its bugs resolved. It raises six Investigate flags, unchanged from the previous head, and each has a thread:

Devin's nine informational notes read as confirmations of the design and are not mirrored here.

Also for the record, at commit c9908c2d0b, whose only difference from the head is one Markdown file: 29 end-to-end tests pass (7.8 minutes), the C# suite passes (3314 passed, 13 skipped, 3m46s), the front-end suite passes (781 passed, 5 skipped), the typecheck is clean, and CI is green. Greptile last reviewed 726db74635 and raised the same point about the enlarged waits; it has not spoken about the commits since. CodeRabbit is switched off for this repository by .coderabbit.yml.

hatton and others added 2 commits September 2, 2026 08:58
Bloom's Edit tab loses a request that changes the page when that request
arrives while the tab is still loading a page. The suite hit this twice in
publish-text-languages.spec.ts, once through goToPage and once through
setContentLanguages, each time as a 60-second wait for something that had
already been asked for.

Bloom's own log shows the mechanism. A page announces that its DOM has loaded
more than once. The first announcement releases the request that was queued
while the tab navigated; that request asks the browser for the page content so
it can save the page being left. The second announcement then arrives, Bloom
refuses it because a save is in flight, and the browser never answers the save
request. The tab stays in SavePending, so nothing more happens: the page never
changes, and the tab switch that follows is held as well.

    Navigating(f45a2ef8) --> editing(f45a2ef8)
    Editing(f45a2ef8) --> savePending()
    Ignoring edit() request while in SavePending(f45a2ef8)

Worked around for the suite by keeping every request out of that queue:

- e2e/editState reports what the Edit tab is doing, which page it is about,
  whether it is showing, and how many times the page it shows has announced
  itself. Read-only, off the UI thread, and registered only under --e2e.
- waitForEditTabSettled waits until the tab reads Editing twice, 1500 ms apart,
  on one page, with the announcement count unchanged. The state alone reads
  Editing between the two announcements, when a request would still be lost, and
  the gap between them has been seen to reach a second.
- goToPage, setContentLanguages, addPage and duplicateCurrentPage wait for it
  before they ask for anything. No test can see any of this from the DOM: Bloom
  leaves the previous page in the frame while it loads the next one.

goToPage's failure message now names the state the tab is stuck in.

AUTOMATION-DEBT.md records the Bloom defect itself, which remains: a user hits
it whenever something asks for a page change in that window, and the Edit tab
then stops accepting page changes altogether. Both fix directions change
production save behavior, so that is a decision rather than a quiet fix.

Also in this commit, the fixes for the remaining review findings on this branch:

- captureElement refuses an element that needs a window larger than the cap,
  rather than returning a truncated image nothing downstream could detect, and
  fails rather than warns when it cannot clear the window size override.
- goToPage reads the driven shell URL forgivingly, so a failing request cannot
  replace the diagnostic the message exists to give.
- Only an environment that carries a debugging port is kept for later
  UI-thread browsers, so a browser built before BloomServer had its port
  cannot leave the whole run unable to listen.
- findShellPage re-checks a marker-only match before returning it; it may have
  been found ninety seconds earlier, and Bloom navigates the shell while it
  starts up.
- typeInGroup says what keyboard.insertText does not do, and
  AUTOMATION-DEBT.md records that no test that types exercises anything in
  Bloom that listens for a key.
- Under --headless the problem-report screenshot renders the window rather
  than copying from screen coordinates that sit outside every monitor.

Validated: e2e 9/9 twice, 3313 C# tests, e2e typecheck clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Master's duplicate-page work (PR 8277) touched the same three files as this
branch. Resolved as follows:

- EditingStateMachine: master's Editing property and this branch's CurrentState
  and CurrentPageId sit side by side.
- bookMaking.ts: kept this branch's single-request goToPage, which master did
  not have yet, and accepted master's move of duplicateCurrentPage into
  helpers/pageList.ts.
- AUTOMATION-DEBT.md: kept the paragraph master added about the native dialogs
  the duplicate-page manual test needs.

The helpers master added to pageList.ts change the page too, so
duplicatePageWithButton, duplicatePageWithContextMenu, movePageToSlotOf and
duplicateCurrentPage now wait for waitForEditTabSettled before they ask for
anything, and leave the tab settled afterwards. goToPage does the same on the
way out: the page being in the frame is not the whole of arriving, and Bloom
re-navigates the page list as well, so a test that grabbed the page list right
after goToPage returned could be holding a frame about to be replaced.

Also corrected the dev-server instructions in README.md and the add-e2e-test
skill, which recommended port 5199 and cost this branch two failed suite runs.
The port is not free to choose: pageThumbnailList.vite-dev.pug and
toolbox.vite-dev.pug write http://localhost:5173 into every import they emit, so
on any other port the page list and the toolbox load nothing and come up empty.
That reads as the feature being missing, and it is what failed
duplicate-page.spec.ts with "waiting for getByTestId('duplicate-page-button')".
Compounding it, an unset BLOOM_E2E_VITE_PORT does not mean "no dev server":
Bloom probes 5173 itself, finds nothing when the server is elsewhere, and serves
the built output/browser, so the run tests whatever bundle was last built.
Both halves are now an AUTOMATION-DEBT entry with the fix direction.

Validated: e2e 15/15 with the dev server on 5173, C# build clean, e2e typecheck
clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@hatton hatton left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Claude Opus 5 from Hatton's machine during preflight]

Devin (Bug, severe): "Deferred page jumps can freeze editing"

Devin is right, and this is the most important thing it found. The version of this branch it
reviewed had JumpToPage remember a jump that arrived while the Edit tab was still loading a
page, and act on it as soon as that page said its DOM had loaded. A page says that more than
once. So the jump ran on the first announcement and started saving the page being left; the
second announcement was then refused because a save was in flight, and the browser never
answered the save request. The Edit tab stopped accepting page changes from then on.

That is worse than what it replaced. Before, the jump was silently dropped and the person
clicked the page thumbnail again. After, one click at the wrong moment left them unable to
change pages at all until they left the tab.

Fixed by removing the queue: JumpToPage now either shows the page at once or refuses and says
so, and editView/jumpToPage answers with an error. That restores what a person sees today,
keeps the honest answer this branch set out to give (the old code reported success for a jump it
had dropped), and takes the queue out of the picture. DeferUntilPageIsLoaded and
TakeWorkDeferredUntilPageIsLoaded are gone with it.

The underlying Bloom defect is older than this branch and remains: any page change that reaches
the Edit tab between two announcements of one page load wedges it the same way. It is written up
in src/BloomE2E/AUTOMATION-DEBT.md with two fix directions, both of which change how saving
behaves, so it goes to the developer rather than into this PR.

Comment thread src/BloomExe/web/controllers/E2eTestingApi.cs
hatton and others added 3 commits September 2, 2026 13:04
Devin found that the queue this branch added made things worse rather than
better. JumpToPage remembered a jump that arrived while the Edit tab was
loading a page, and acted on it when that page announced its DOM had loaded.
A page announces that more than once: the jump ran on the first announcement
and started saving the page being left, the second announcement was refused
because a save was in flight, and the browser never answered the save
request. The Edit tab then refused every page change until it was left.

That is worse than what it replaced. Before, a person's click on a page
thumbnail at that moment was dropped and they clicked again. After, it left
them unable to change pages at all.

So JumpToPage now either shows the page at once or refuses and says so, and
editView/jumpToPage answers with an error. Both front-end callers already
pass report:false, so a refusal raises no problem report. This restores what
a person sees today and keeps the honest answer the branch set out to give:
the old code reported success for a jump it had dropped.
DeferUntilPageIsLoaded and TakeWorkDeferredUntilPageIsLoaded go with it.

The Bloom defect underneath is older than this suite and remains: any page
change reaching the Edit tab between two announcements of one page load
wedges it. AUTOMATION-DEBT.md records it with two fix directions, both of
which change how saving behaves.

Also: goToPage asks up to three times, waiting for a settled Edit tab before
each, since a refusal is now an error rather than a silent drop; and
E2eTestingApi says why the editState reply reads the announcement count
last, which is what stops a mixed reply letting a test stop waiting early.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts:
#	.github/skills/add-e2e-test/SKILL.md
#	src/BloomVisualRegressionTests/index.spec.ts
…t, tool registration

Four separate defects, all raised in review of this branch.

`--repo-root` in killBloomProcess.mjs and bloomProcessStatus.mjs took the next
argument as its value without checking it. `--repo-root --pid 123` therefore
consumed `--pid` as the repository path, named no target at all, and reached
killBloomProcess.mjs's default, which kills every Bloom this worktree owns. It
now uses requireOptionValue, the same check every other option already had, so
the malformed command exits non-zero and kills nothing.

captureElement warned on standard error, rather than failing, when it could not
clear the window size override after the capture itself had failed. A warning
in a long run is easy to miss, and every later test in that worker then drives a
Bloom of the wrong size. It now always throws, and carries the capture's own
error as the cause so neither error is lost.

GetHeadlessBounds could ask Windows for an x coordinate further left than the
-32000 its own comment names as the limit, because Math.Min took whichever of
the two candidates was further left. No monitor layout reaches that far left, so
there is nothing to compute from the layout: it returns the constant.

registerAllToolboxTools skipped all eleven tools whenever the master list held
any entry at all. It now checks each tool by id, so a list holding some other
tool cannot leave the toolbox with no sections.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread .github/skills/bloom-automation/killBloomProcess.mjs
Comment thread src/BloomE2E/helpers/screenshot.ts
Comment thread src/BloomExe/Shell.cs Outdated
Comment thread src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts Outdated
Comment thread src/BloomE2E/helpers/bookMaking.ts
GetHeadlessBounds has two constraints and the earlier versions each honoured
only one. Taking whichever of the two candidates was further left could ask
Windows for a coordinate past the -32000 that the comment itself names as the
limit. Returning the constant instead could put the window on a monitor, on a
leftward run of monitors wide enough to reach that far.

It now computes the position that clears the leftmost monitor, keeping the
1000-pixel cushion for the case where Windows and this process disagree about
how wide a monitor is, and clamps that to -32000. On any real layout the first
bound lands a few thousand pixels to the left, well inside the limit. A leftward
run of monitors more than about 30000 pixels wide satisfies neither bound, and
there the limit wins: a coordinate Windows will not honour is worse than an
overlap.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/BloomExe/Shell.cs Outdated
hatton and others added 2 commits September 2, 2026 16:10
# Conflicts:
#	.github/workflows/nightly.yml
#	src/BloomE2E/AUTOMATION-DEBT.md
#	src/BloomE2E/README.md
#	src/BloomE2E/tests/publish-text-languages.spec.ts
BL-16799 asked for two things: an e2e suite that does not open a window on the
developer's desktop, and the automation debt that fixing it exposed. Those are
now two cards and two pull requests, at the developer's request, so that the
off-screen change can be reviewed on its own.

This branch keeps the debt. Everything about the off-screen window moves to
BL-16804: the --headless flag and its unit tests, the window and splash-screen
placement, the WebView2 occlusion settings that an off-screen window needs, the
problem-report screenshot path that must render the window rather than copy the
screen, the fixture that passes the flag, and the documentation of it.

Neither branch depends on the other. Until BL-16804 merges, a run of this
suite opens a Bloom window again.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hatton hatton changed the title Run the e2e suite off-screen, and pay down the automation debt behind it (BL-16799) Pay down the automation debt in the e2e suite (BL-16799) Sep 2, 2026
registerAllToolboxTools built all eleven tools and then discarded the ones the
master list already held. Two of them point a static field at the instance being
constructed: CanvasTool.theOneCanvasTool and GameTool.theOneDragActivityTool. So
a second call left those fields holding a tool the toolbox does not know about,
and a canvas refresh or a game's state then went nowhere.

Each entry now names its id and passes a factory, so a tool the list already has
is never made. Where a tool names its id in a constant, the entry uses that
constant; where it uses a string literal, registerOnce checks the constructed
tool's own id against the name it was given and throws if they differ, because
nothing else ties the two together.

Found by Devin on PR 8276.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread src/BloomBrowserUI/bookEdit/toolbox/registerAllToolboxTools.ts
hatton and others added 2 commits September 2, 2026 16:57
Master's e1ca994 points that flake at BL-16806 and corrects the cause: LibPalaso's
localized-name call memoizes into a process-wide static dictionary and can return a name
that does not correspond to what was asked. The earlier guess, that Bloom was naming the
dropped language in the collection's own French, was wrong.

The conflict is in publish-text-languages.spec.ts, whose flake comment master rewrote and
this branch's test no longer needs: the test now drops a language through
e2e/setCollectionLanguages, the code the Settings dialog's OK button runs, so it never
reaches the lookup that wavers. Kept this branch's test, and carried master's diagnosis
into the AUTOMATION-DEBT.md entry that records the coverage this branch gives up.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Master's diagnosis of the "espagnol" / "español" difference in the Text
Languages test changed: it is not a flake and it does not depend on the run,
it depends on the machine. LibPalaso honors a request for a language name "in"
another language only where it can find a native ICU library, and Bloom ships
icu.net without icuuc.dll, so the CI runner answers "espagnol" every time and
a developer machine answers "español" every time.

AUTOMATION-DEBT.md now says that. The entry also records the coverage this
branch gives up: the test drops a language through e2e/setCollectionLanguages,
which keeps the language's collection name, so it never reaches the lookup
that differs.

The merge conflicted in publish-text-languages.spec.ts. Kept this branch's
test, which expects "Spanish", for the reason above.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hatton and others added 2 commits September 3, 2026 06:24
Master added an e2e test for the items a book needs before upload (Notion test
case 606), which brings a new e2e/loginState endpoint, and it tightened the
comment on the Text Languages test again. Three files conflicted.

E2eTestingApi.cs: both sides register new endpoints in the same place and add
their handlers in the same place. Kept both. This branch's shellUrl and
editState endpoints stand beside master's loginState, and all three handlers
stay.

publish-text-languages.spec.ts: kept this branch's test, which drops the
language through e2e/setCollectionLanguages and expects "Spanish". Master's
version rewrites the .bloomCollection and reaches the LibPalaso lookup that
answers differently on CI and on a developer machine. The comment now says
which version meets that difference and which does not, so nobody copies
master's comment back here.

AUTOMATION-DEBT.md: kept master's new entry about the Bloom Library login,
which no test can do for real, and this branch's entry about which front end a
run tests. Master's "Visual-regression cases stop at the first failed
comparison" entry stays deleted: this branch is what fixed it, so the entry
described behavior that no longer exists.

Verified after the merge: the C# build succeeds, the front-end type check
passes, and the e2e suite type-checks.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SignLanguageTool.kToolID does not exist. The static kToolID = "signLanguage"
belongs to SignLanguageToolControls, which is a different class in the same
file. So the id passed to registerOnce was undefined, and the check that the
id matches the tool's own id() threw:

  registerAllToolboxTools names the tool "undefined", but the tool calls
  itself "signLanguage".

toolboxBootstrap.ts calls registerAllToolboxTools() at module load, so the
throw stopped that module after six tools. The Edit tab toolbox lost every
later tool, and window.toolboxBundle never got assigned. It also failed the
seven component tests in
react_components/ToolboxRootTestHarness/component-tests/toolbox-root-react.uitest.ts.

The typecheck does not catch a missing static, so only running the
component-tester suite found it. That suite now passes: 144 passed, 25
skipped, both warm and with the Vite dependency cache deleted.

The other ten ids in the list are checked against each tool's id() and are
correct.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@hatton

hatton commented Sep 3, 2026

Copy link
Copy Markdown
Member Author

[Claude Opus 5 following a prompt from Hatton]

Closing this in favor of eleven stacked pull requests, one per improvement. This one did
too many things at once to review.

Each branches off the one before it, so #8290 is the only one based on master. Review
them bottom up. The first six change test and tooling code only; the last five also change
product code.

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

Two things changed on the way, both because this pull request's own work was never run:

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). Each pull
request also carries its own type check and lint. src/BloomE2E/AUTOMATION-DEBT.md on
master names the pull request paying down each entry.

@hatton hatton closed this Sep 3, 2026
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