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

.github/skills/add-e2e-test/SKILL.md

@@ -338,12 +338,29 @@ pnpm exec playwright test tests/workspace-tabs.spec.ts   # one file
 pnpm exec playwright test -g "switching workspace tabs"  # one test by title
 ```
 
-A run opens a real Bloom window; that is expected. It needs a built `Bloom.exe` under
-`output/{Debug,Release}/{x64,AnyCPU,}/` (build it yourself; see "Build Bloom whenever it
-helps") and the inputs at `output/testing-inputs`. Point
+A run needs a built `Bloom.exe` under `output/{Debug,Release}/{x64,AnyCPU,}/` (build it yourself;
+see "Build Bloom whenever it helps") and the inputs at `output/testing-inputs`. Point
 `BLOOM_TESTING_INPUTS_DIR` at a bloom-testing-inputs checkout to use your own in-progress
 collections instead of the pinned ones.
 
+The launched Bloom serves its React UI from the built `output/browser`, so **an edit to a `.tsx`
+file does not reach a run until that bundle is rebuilt.** To test the working tree instead, start
+a dev server and name its port in `BLOOM_E2E_VITE_PORT`; the fixture passes `--vite-port` and
+Bloom loads every React control from it. Set `PORT` as well as `--port`, or the dev server's
+HMR and React-Refresh URLs still point at 5173 and the page fails to load its entry module.
+
+```bash
+PORT=5173 pnpm exec vite --port 5173 --strictPort   # in src/BloomBrowserUI
+BLOOM_E2E_VITE_PORT=5173 pnpm test                  # in src/BloomE2E
+```
+
+**Use 5173, and set the variable.** The page list and the toolbox write `http://localhost:5173`
+into their own imports, so on any other port those two frames load nothing and come up empty,
+which reads as the feature being missing. And leaving the variable unset does not mean "no dev
+server": a dev build of Bloom probes 5173 by itself, so an unset variable and a server elsewhere
+means the run quietly tests the built bundle, however old it is. Stop a Bloom that already holds
+5173 rather than moving the dev server. See AUTOMATION-DEBT.md.
+
 `.github/workflows/nightly.yml` does not run this suite yet. The step it will need is the
 same `pnpm test` in that folder, after the Release build and the testing-inputs fetch that
 the visual-regression job already does.

.github/skills/bloom-automation/bloomProcessCommon.mjs

@@ -42,6 +42,28 @@ export const requireTcpPortOption = (optionName, value) => {
     return port;
 };
 
+/**
+ * A process id given on the command line, as a positive integer.
+ *
+ * This throws rather than returning undefined, because the caller that wants a process id wants
+ * to act on exactly that process. A killer script that read a malformed id as "no id given" would
+ * fall through to whatever its no-target default is, and killBloomProcess.mjs's default is to kill
+ * every Bloom the worktree owns.
+ */
+export const requireProcessIdOption = (optionName, value) => {
+    const normalized = value === undefined ? "" : String(value).trim();
+    const processId = /^\d+$/.test(normalized)
+        ? toPositiveInteger(normalized)
+        : undefined;
+    if (!processId) {
+        throw new Error(
+            `${optionName} must be a positive integer process id. Received: ${value}`,
+        );
+    }
+
+    return processId;
+};
+
 export const requireOptionValue = (args, index, optionName) => {
     const value = args[index + 1];
     if (!value || value.startsWith("--")) {

.github/skills/bloom-automation/bloomProcessStatus.mjs

@@ -13,6 +13,18 @@ import {
     toWorkspaceTabsEndpoint,
 } from "./bloomProcessCommon.mjs";
 
+const usage = `Report the Bloom and launcher processes this machine is running.
+
+  node bloomProcessStatus.mjs [options]
+
+  --help, -h            Print this and exit.
+  --json                Report as JSON.
+  --running-bloom       Also probe each running Bloom's own HTTP server.
+  --repo-root <path>    The worktree to judge instances against (default: this checkout).
+  --http-port <port>    Report the instance whose server answers on this port.
+
+This script only reads; it never kills anything.`;
+
 const parseArgs = () => {
     const args = process.argv.slice(2);
     const options = {
@@ -25,6 +37,11 @@ const parseArgs = () => {
     for (let i = 0; i < args.length; i++) {
         const arg = args[i];
 
+        if (arg === "--help" || arg === "-h") {
+            console.log(usage);
+            process.exit(0);
+        }
+
         if (arg === "--json") {
             options.json = true;
             continue;
@@ -36,7 +53,11 @@ const parseArgs = () => {
         }
 
         if (arg === "--repo-root") {
-            options.repoRoot = args[i + 1] || options.repoRoot;
+            // A required value, checked the same way as every other option's. Taking
+            // `args[i + 1]` and falling back to the default would swallow the next flag as
+            // this option's value. killBloomProcess.mjs has the same parser, where that
+            // mistake is destructive.
+            options.repoRoot = requireOptionValue(args, i, "--repo-root");
             i++;
             continue;
         }
@@ -57,6 +78,10 @@ const parseArgs = () => {
             );
             continue;
         }
+
+        // An unknown flag is a mistake, and silently ignoring it hides it.
+        console.error(`Unknown option ${arg}.\n\n${usage}`);
+        process.exit(2);
     }
 
     return options;

.github/skills/bloom-automation/killBloomProcess.mjs

@@ -7,9 +7,37 @@ import {
     killProcessIds,
     normalizeBloomInstanceInfo,
     requireOptionValue,
+    requireProcessIdOption,
     requireTcpPortOption,
 } from "./bloomProcessCommon.mjs";
 
+const usage = `Kill the Bloom.exe (and dotnet.exe BloomExe.csproj) processes of this worktree.
+
+  node killBloomProcess.mjs [options]
+
+  --help, -h              Print this and exit without killing anything.
+  --json                  Report what was killed as JSON.
+  --only-mismatched       Kill only instances whose repo root is not this worktree.
+  --repo-root <path>      The worktree to judge instances against (default: this checkout).
+  --http-port <port>      Kill the instance whose server answers on this port.
+  --pid <pid>             Kill this process and the Bloom processes in its chain.
+  --watch-pid <pid>       Kill this launcher/watch process and its Bloom processes.
+
+With no --http-port, --pid or --watch-pid, this kills EVERY Bloom this worktree owns.`;
+
+// Print the usage and exit 0 without killing anything, or reject an unknown flag with a non-zero
+// exit. Reading the usage first must never be the dangerous move: this script used to ignore
+// --help and go straight to its destructive default (AUTOMATION-DEBT.md, "Automation helper
+// scripts run destructive defaults on unknown flags").
+const exitWithUsage = (unknownArgument) => {
+    if (unknownArgument) {
+        console.error(`Unknown option ${unknownArgument}.\n\n${usage}`);
+        process.exit(2);
+    }
+    console.log(usage);
+    process.exit(0);
+};
+
 const parseArgs = () => {
     const args = process.argv.slice(2);
     const options = {
@@ -24,6 +52,10 @@ const parseArgs = () => {
     for (let i = 0; i < args.length; i++) {
         const arg = args[i];
 
+        if (arg === "--help" || arg === "-h") {
+            exitWithUsage();
+        }
+
         if (arg === "--json") {
             options.json = true;
             continue;
@@ -35,7 +67,11 @@ const parseArgs = () => {
         }
 
         if (arg === "--repo-root") {
-            options.repoRoot = args[i + 1] || options.repoRoot;
+            // A required value, checked the same way as every other option's. Taking
+            // `args[i + 1]` and falling back to the default would swallow the NEXT FLAG as
+            // this option's value, so `--repo-root --pid 123` would name no target at all and
+            // reach the default that kills every Bloom this worktree owns.
+            options.repoRoot = requireOptionValue(args, i, "--repo-root");
             i++;
             continue;
         }
@@ -58,25 +94,40 @@ const parseArgs = () => {
         }
 
         if (arg === "--pid") {
-            options.pid = Number(args[i + 1]);
+            options.pid = requireProcessIdOption(
+                "--pid",
+                requireOptionValue(args, i, "--pid"),
+            );
             i++;
             continue;
         }
 
         if (arg.startsWith("--pid=")) {
-            options.pid = Number(arg.slice("--pid=".length));
+            options.pid = requireProcessIdOption(
+                "--pid",
+                arg.slice("--pid=".length),
+            );
             continue;
         }
 
         if (arg === "--watch-pid") {
-            options.watchPid = Number(args[i + 1]);
+            options.watchPid = requireProcessIdOption(
+                "--watch-pid",
+                requireOptionValue(args, i, "--watch-pid"),
+            );
             i++;
             continue;
         }
 
         if (arg.startsWith("--watch-pid=")) {
-            options.watchPid = Number(arg.slice("--watch-pid=".length));
+            options.watchPid = requireProcessIdOption(
+                "--watch-pid",
+                arg.slice("--watch-pid=".length),
+            );
+            continue;
         }
+
+        exitWithUsage(arg);
     }
 
     return options;
@@ -85,8 +136,14 @@ const parseArgs = () => {
 const options = parseArgs();
 const processState = classifyProcesses(options.repoRoot);
 const processIds = new Set();
+// Whether the caller named a target, not whether the value we parsed from it is usable. The
+// two are the same now that every target option is validated at parse time, and this says the
+// intended thing: a caller who asked for one process must never reach the default that kills
+// every Bloom this worktree owns.
 const exactTargetRequested =
-    !!options.httpPort || !!options.pid || !!options.watchPid;
+    options.httpPort !== undefined ||
+    options.pid !== undefined ||
+    options.watchPid !== undefined;
 let targetedInstance;
 let exactTargetResolutionError;
 

.github/skills/bloom-automation/launcherControl.mjs

@@ -43,6 +43,17 @@ const actionNames = [
     "--ensure-running",
 ];
 
+const usage = `Command the go.sh launcher for this worktree.
+
+  node launcherControl.mjs <action> [options]
+
+  actions: ${actionNames.join(", ")}
+  options: --json, --wait-ready, --repo-root <path>, --timeout-ms <n>
+           --help, -h   Print this and exit without commanding anything.
+
+  --restart rebuilds and relaunches; --quit-bloom stops Bloom and leaves the launcher;
+  --shutdown stops Bloom, the launcher, and Vite.`;
+
 const parseArgs = () => {
     const args = process.argv.slice(2);
     const options = {
@@ -56,6 +67,13 @@ const parseArgs = () => {
     for (let i = 0; i < args.length; i++) {
         const arg = args[i];
 
+        // Print the usage and stop, before any action can run. Asking a destructive script how to
+        // use it must never be the destructive move.
+        if (arg === "--help" || arg === "-h") {
+            console.log(usage);
+            process.exit(0);
+        }
+
         if (actionNames.includes(arg)) {
             if (options.action) {
                 throw new Error(

.github/workflows/nightly.yml

@@ -1,10 +1,10 @@
 # Nightly build + full test run of everything on master.
 #
 # This is a health check, not a release: it builds the front-end and the C# solution and
-# runs all four test suites — front-end vitest, C# NUnit, the BloomE2E suite and the visual
-# regression suite — but produces no installer, does no signing, and publishes nothing. It
-# exists to catch breakage that the PR checks miss — e.g. tests excluded from PR runs, or rot
-# from dependency/runner drift — on a predictable cadence.
+# runs all five test suites — front-end vitest, C# NUnit, the BloomE2E suite, the visual
+# regression suite, and the component-tester Playwright suite — but produces no installer, does
+# no signing, and publishes nothing. It exists to catch breakage that the PR checks miss — e.g.
+# tests excluded from PR runs, or rot from dependency/runner drift — on a predictable cadence.
 #
 # Each suite publishes its own check run / job-summary section, so the commit shows four
 # independent results rather than one merged total. See the "Test reports" steps at the end.
@@ -54,6 +54,10 @@ on:
                 description: "Visual-regression suite (drives a real Bloom)"
                 type: boolean
                 default: true
+            run_component_tests:
+                description: "Component-tester suite (Playwright against the Vite harness)"
+                type: boolean
+                default: true
 
 # Don't stack nightlies: if a manual run overlaps the scheduled one, let the first finish.
 concurrency:
@@ -81,6 +85,7 @@ jobs:
             RUN_CSHARP_TESTS: ${{ github.event_name != 'workflow_dispatch' || inputs.run_csharp_tests }}
             RUN_E2E_TESTS: ${{ github.event_name != 'workflow_dispatch' || inputs.run_e2e_tests }}
             RUN_VISUAL_REGRESSION: ${{ github.event_name != 'workflow_dispatch' || inputs.run_visual_regression }}
+            RUN_COMPONENT_TESTS: ${{ github.event_name != 'workflow_dispatch' || inputs.run_component_tests }}
 
         steps:
             - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
@@ -110,6 +115,7 @@ jobs:
                       src/content/pnpm-lock.yaml
                       src/BloomE2E/pnpm-lock.yaml
                       src/BloomVisualRegressionTests/pnpm-lock.yaml
+                      src/BloomBrowserUI/react_components/component-tester/pnpm-lock.yaml
 
             # ----- Dependencies (mirrors init.sh) -----
 
@@ -447,6 +453,50 @@ jobs:
                   retention-days: 4
                   if-no-files-found: ignore
 
+            # ----- Component-tester tests -----
+            # src/BloomBrowserUI/react_components/component-tester renders one React component at a
+            # time in a Vite dev server and drives it with Playwright (the *.uitest.ts files beside
+            # each component). Nothing ran these until now, which is how the harness sat broken for
+            # weeks — a React 17 pin plus a config bug — and it would rot again silently
+            # (AUTOMATION-DEBT.md, "The component-tester Playwright suites are not in CI").
+            #
+            # This is the component config only (playwright.config.ts, which the package's own
+            # `pnpm test` uses). The sibling playwright.bloom-exe.config.ts attaches over CDP to a
+            # Bloom the developer already has running, so it needs the src/BloomE2E launch fixture
+            # before it can run unattended; its specs are excluded by that config's testIgnore.
+            #
+            # It needs neither build: the harness serves the components from its own Vite dev
+            # server, which playwright.config.ts starts as its webServer. So this group depends on
+            # nothing above it, and runs even when the builds failed.
+            - name: Set up component-tester tests
+              id: setup_component_tests
+              if: ${{ !cancelled() && env.RUN_COMPONENT_TESTS == 'true' }}
+              working-directory: src/BloomBrowserUI/react_components/component-tester
+              shell: bash
+              run: |
+                  pnpm install --frozen-lockfile
+                  pnpm exec playwright install chromium
+
+            # Playwright takes its junit path from PLAYWRIGHT_JUNIT_OUTPUT_NAME, not from a CLI
+            # flag: it has no --outputFile (that is vitest's). The path is relative to the working
+            # directory, so it climbs back to the repo root's output/Tests like the other suites.
+            #
+            # --timeout raises the per-test 30s of playwright.config.ts, which is a developer's
+            # number: it assumes a dev server that has already transformed the module graph. Every
+            # CI run starts cold, and the first request for a component pays for transforming that
+            # whole graph. Locally, a cold first run failed 12 tests on `page.goto` timing out
+            # while the warm re-run of the same suite passed 142 in 58 seconds. So give the runner
+            # room rather than reporting a cold start as a broken component. A passing test still
+            # returns as soon as it passes, so this costs a green run nothing.
+            - name: Run component-tester tests
+              id: component_tests
+              if: ${{ !cancelled() && steps.setup_component_tests.outcome == 'success' }}
+              working-directory: src/BloomBrowserUI/react_components/component-tester
+              shell: bash
+              env:
+                  PLAYWRIGHT_JUNIT_OUTPUT_NAME: ../../../../output/Tests/component-tester-junit.xml
+              run: pnpm test --timeout=120000 --reporter=list,junit
+
             # ----- Test reports, one per suite -----
             # Each suite gets its OWN invocation of the publish action, and therefore its own
             # check run on the commit and its own section in the job summary: pass/fail/skip
@@ -512,6 +562,15 @@ jobs:
                   action_fail_on_inconclusive: true
                   files: output/Tests/visual-regression-junit.xml
 
+            - name: Publish component-tester test results
+              if: ${{ !cancelled() && steps.component_tests.outcome != 'skipped' }}
+              uses: EnricoMi/publish-unit-test-result-action/windows@d0a4676d0e0b938bc201470d88276b7c74c712b3 # v2.24.0
+              with:
+                  check_name: "Nightly tests: component-tester (Playwright)"
+                  comment_mode: "off"
+                  action_fail_on_inconclusive: true
+                  files: output/Tests/component-tester-junit.xml
+
             - name: Upload test results
               if: ${{ always() }}
               uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1
@@ -525,4 +584,5 @@ jobs:
                       output/Tests/vitest-junit.xml
                       output/Tests/e2e-junit.xml
                       output/Tests/visual-regression-junit.xml
+                      output/Tests/component-tester-junit.xml
                   retention-days: 14

src/BloomBrowserUI/bookEdit/bookAndPageSettings/StyleAndFontTable.tsx

@@ -46,7 +46,9 @@ export const StyleAndFontTable: React.FunctionComponent<{
 
     function closeDialogAndJumpToPage(pageId: string) {
         props.closeDialog();
-        postString("editView/jumpToPage", pageId);
+        // report: false — the edit tab declines a jump it cannot do (it is mid-save), and that
+        // is not something to raise a problem report about. See EditingModel.JumpToPage.
+        postString("editView/jumpToPage", pageId, false);
     }
 
     return (