From dccd09b9e71cb76a93ebced428ccf8c04de7186f Mon Sep 17 00:00:00 2001 From: John Thomson Date: Fri, 4 Sep 2026 16:18:56 -0500 Subject: [PATCH] Fix BL-16809 Remove Page sometimes prevented by race condition https://issues.bloomlibrary.org/youtrack/issue/BL-16809 Since BL-16421 the "Really Remove Page?" confirmation is a browser dialog. Like the other edit-tab dialogs it posts editView/setModalState from a React effect, which on the C# side disabled the page list (PageThumbnailList.Enabled) as well as locking the workspace tabs. When the user clicked Remove, the browser sent the removePage command and the "modal closed" notice as two concurrent HTTP requests. Whenever the command was handled first (about 100ms after the click, after HandleContextMenuItemClickedRequest's deliberate delay), the page list was still disabled, so IsContextMenuCommandEnabled returned false and ExecuteContextMenuCommand dropped the command silently. Captured over CDP against 6.5.3018: the unlock was processed roughly 350ms after the command. Disabling the page list from C# dates from when the page list was its own browser and a dialog's backdrop could not cover it. The whole edit tab is in one browser now, so a dialog shown from the workspace root already has a backdrop that blocks the page list (and the tabs). The C# gate could therefore only do harm, so this removes it: SetModalState now only locks the tabs, and PageListController's Enabled setter, PageThumbnailList.Enabled and its two checks (page clicks and context-menu commands) are gone. The same lag could also swallow a page click made right after a dialog closed. Two dialogs were still launched from code running in the page iframe with a direct import of ShowEditViewDialog, so they rendered inside the book pane and their backdrop covered only that pane: Choose Link Target (from the text hyperlink command and the navigation canvas element's "Set Destination") and Book Grid Setup. Both are now exposed on the workspace bundle, following the copyright dialog's pattern, and the page-side callers go through getWorkspaceBundleExports(), so their backdrop covers the whole workspace. The comments on the dialogs that post the flag are updated to say what it now does. Also logs a papercut about the dev launcher timing out while a direct dotnet watch run works. Tests: full C# suite through build/agent-dotnet.ps1 (3319 passed, 0 failed, 13 skipped) and the full vitest suite (784 passed, 5 skipped) pass. Verified live in a dev build: removing a page via the thumbnail context menu deletes it, refreshes the list, saves to disk and navigates to the next page. Co-Authored-By: Claude Fable 5.1 --- PAPERCUTS.md | 5 +++++ .../TopicChooser/TopicChooserDialog.tsx | 5 +---- .../bookEdit/bloomField/BloomField.ts | 5 ++++- .../CopyrightAndLicenseDialog.tsx | 5 +---- src/BloomBrowserUI/bookEdit/js/linkGrid.ts | 5 +++-- .../toolbox/canvas/canvasControlRegistry.ts | 4 +++- .../talkingBook/AdjustTimingsDialog.tsx | 5 +---- src/BloomBrowserUI/bookEdit/workspaceRoot.ts | 20 +++++++++++++++++++ .../pageChooser/PageChooserDialog.tsx | 5 +---- .../LinkTargetChooserDialogLauncher.tsx | 3 +++ .../registrationDialogLauncher.tsx | 2 +- src/BloomExe/Edit/EditingView.cs | 17 +++++++++++----- src/BloomExe/Edit/PageListController.cs | 5 ----- src/BloomExe/Edit/PageThumbnailList.cs | 6 ++---- 14 files changed, 57 insertions(+), 35 deletions(-) diff --git a/PAPERCUTS.md b/PAPERCUTS.md index 2a100668fc7a..28e79b05447f 100644 --- a/PAPERCUTS.md +++ b/PAPERCUTS.md @@ -19,6 +19,11 @@ House rules: --- +## 2026-09-04 — Launcher times out waiting for BLOOM_AUTOMATION_READY while a direct dotnet watch works +- **Cut:** `go.mjs` / `launcherControl.mjs --ensure-running` built Bloom in ~6s, printed `dotnet watch ⌚ Loaded 2 project(s)`, then never saw the ready marker and tore the whole stack down after the 120s `launchTimeoutMs` in `scripts/watchBloomExe.mjs` — three times in a row. Running `dotnet watch run --project src/BloomExe/BloomExe.csproj --non-interactive -- --automation` by hand from the same shell started Bloom and printed `BLOOM_AUTOMATION_READY` within seconds (alongside a running BetaInternal, so it was not the single-instance token). +- **Idea:** Find what differs when `watchBloomExe.mjs` spawns dotnet watch (`--vite-port`/`--label` args, control-port env, stdout piping) and make the launcher print dotnet watch's later output or the Bloom PID's window titles when it gives up, so the failure is diagnosable. Consider making the timeout configurable. +- **Context:** worktree Format-Gear-Positioning-356 at the Version6.5 tip, while fixing BL-16809; hit by Claude. + ## 2026-07-30 — Visual regression suite reports only the first stale image per case - **Cut:** Each case in `src/BloomVisualRegressionTests/index.spec.ts` compares the book preview and then every bloom-player page in sequence, and every comparison throws on failure — so the diff --git a/src/BloomBrowserUI/bookEdit/TopicChooser/TopicChooserDialog.tsx b/src/BloomBrowserUI/bookEdit/TopicChooser/TopicChooserDialog.tsx index 1d15c0c56efd..7844b3d71293 100644 --- a/src/BloomBrowserUI/bookEdit/TopicChooser/TopicChooserDialog.tsx +++ b/src/BloomBrowserUI/bookEdit/TopicChooser/TopicChooserDialog.tsx @@ -54,10 +54,7 @@ export const TopicChooserDialog: React.FunctionComponent< const dialogTitle = useL10n("Choose Topic", "TopicChooser.Title"); - // Tell edit tab to disable everything when the dialog is up. - // (Without this, the page list is not disabled since the modal - // div only exists in the book pane. Once the whole edit tab is inside - // one browser, this would not be necessary.) + // Tell C# to lock the workspace tabs while the dialog is up. React.useEffect(() => { if (propsForBloomDialog.open === undefined) return; diff --git a/src/BloomBrowserUI/bookEdit/bloomField/BloomField.ts b/src/BloomBrowserUI/bookEdit/bloomField/BloomField.ts index 02521d2735b9..59d5e4dd574b 100644 --- a/src/BloomBrowserUI/bookEdit/bloomField/BloomField.ts +++ b/src/BloomBrowserUI/bookEdit/bloomField/BloomField.ts @@ -7,7 +7,7 @@ import BloomMessageBoxSupport from "../../utils/bloomMessageBoxSupport"; import { tryProcessHyperlink } from "./hyperlinks"; import { EditableDivUtils } from "../js/editableDivUtils"; import $ from "jquery"; -import { showLinkTargetChooserDialog } from "../../react_components/LinkTargetChooser/LinkTargetChooserDialogLauncher"; +import { getWorkspaceBundleExports } from "../js/workspaceFrames"; import { getLocalization } from "../../react_components/l10n"; import { kNoIndentClass } from "../textContextMenu/noIndent"; @@ -401,6 +401,9 @@ export default class BloomField { ckeditor.addCommand("setupHyperlink", { exec: function (edt) { + // Shown from the workspace root so its backdrop covers the page list too. + const showLinkTargetChooserDialog = + getWorkspaceBundleExports().showLinkTargetChooserDialog; showLinkTargetChooserDialog("", (url) => { if (!url) return; get("app/selectedBookInfo", (bookInfo) => { diff --git a/src/BloomBrowserUI/bookEdit/copyrightAndLicense/CopyrightAndLicenseDialog.tsx b/src/BloomBrowserUI/bookEdit/copyrightAndLicense/CopyrightAndLicenseDialog.tsx index 1ee421bbd9e9..dfcfeed33081 100644 --- a/src/BloomBrowserUI/bookEdit/copyrightAndLicense/CopyrightAndLicenseDialog.tsx +++ b/src/BloomBrowserUI/bookEdit/copyrightAndLicense/CopyrightAndLicenseDialog.tsx @@ -68,10 +68,7 @@ export const CopyrightAndLicenseDialog: React.FunctionComponent<{ const dialogTitle = useL10n("Copyright and License", "CopyrightAndLicense"); - // Tell edit tab to disable everything when the dialog is up. - // (Without this, the page list is not disabled since the modal - // div only exists in the book pane. Once the whole edit tab is inside - // one browser, this would not be necessary.) + // Tell C# to lock the workspace tabs while the dialog is up. React.useEffect(() => { if (propsForBloomDialog.open === undefined) return; diff --git a/src/BloomBrowserUI/bookEdit/js/linkGrid.ts b/src/BloomBrowserUI/bookEdit/js/linkGrid.ts index 824f23cefc4c..d3d42015ec9c 100644 --- a/src/BloomBrowserUI/bookEdit/js/linkGrid.ts +++ b/src/BloomBrowserUI/bookEdit/js/linkGrid.ts @@ -4,7 +4,7 @@ import WebSocketManager, { IBloomWebSocketEvent, } from "../../utils/WebSocketManager"; import { postJson } from "../../utils/bloomApi"; -import { showBookGridSetupDialog } from "../../react_components/BookGridSetup/BookGridSetupDialog"; +import { getWorkspaceBundleExports } from "./workspaceFrames"; import { Link } from "../../react_components/BookGridSetup/BookLinkTypes"; function getLanguage1Tag(): string { @@ -56,7 +56,8 @@ export function editLinkGrid(linkGrid: HTMLElement) { }; }); - showBookGridSetupDialog( + // Shown from the workspace root so its backdrop covers the page list too. + getWorkspaceBundleExports().showBookGridSetupDialog( currentLinks, // callback if they press OK (links: Link[]) => { diff --git a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlRegistry.ts b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlRegistry.ts index ba74b8321e15..b7866976e671 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlRegistry.ts +++ b/src/BloomBrowserUI/bookEdit/toolbox/canvas/canvasControlRegistry.ts @@ -74,7 +74,6 @@ import { showDialogToChooseSoundFileAsync, } from "../games/GameTool"; import { showTalkingBookTool } from "../talkingBook/showTalkingBookTool"; -import { showLinkTargetChooserDialog } from "../../../react_components/LinkTargetChooser/LinkTargetChooserDialogLauncher"; import { kBloomBlue } from "../../../bloomMaterialUITheme"; import { trackEvent } from "../../../utils/bloomApi"; import { @@ -1149,6 +1148,9 @@ export const controlRegistry: Record = { // element itself (not on any nested image container). const currentUrl = ctx.canvasElement.getAttribute("data-href") ?? ""; + // Shown from the workspace root so its backdrop covers the page list too. + const showLinkTargetChooserDialog = + getWorkspaceBundleExports().showLinkTargetChooserDialog; showLinkTargetChooserDialog(currentUrl, (newUrl) => { if (newUrl) { ctx.canvasElement.setAttribute("data-href", newUrl); diff --git a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/AdjustTimingsDialog.tsx b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/AdjustTimingsDialog.tsx index 31ee121ce124..cfc6ce57c0c0 100644 --- a/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/AdjustTimingsDialog.tsx +++ b/src/BloomBrowserUI/bookEdit/toolbox/talkingBook/AdjustTimingsDialog.tsx @@ -115,10 +115,7 @@ export const AdjustTimingsDialog: React.FunctionComponent<{ ); const [timingsFilePath, setTimingsFilePath] = useState(); - // Tell edit tab to disable everything when the dialog is up. - // (Without this, the page list is not disabled since the modal - // div only exists in the book pane. Once the whole edit tab is inside - // one browser, this would not be necessary.) + // Tell C# to lock the workspace tabs while the dialog is up. React.useEffect(() => { if (propsForBloomDialog.open === undefined) return; postBoolean("editView/setModalState", propsForBloomDialog.open); diff --git a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts index c864174008ae..c17093273fe6 100644 --- a/src/BloomBrowserUI/bookEdit/workspaceRoot.ts +++ b/src/BloomBrowserUI/bookEdit/workspaceRoot.ts @@ -8,6 +8,7 @@ import { hideColorPickerDialog as doHideColorPickerDialog, } from "../react_components/color-picking/colorPickerDialog"; import { postJson } from "../utils/bloomApi"; +import { Link } from "../react_components/BookGridSetup/BookLinkTypes"; import "../modified_libraries/jquery-ui/jquery-ui-1.10.3.custom.min.js"; //for dialog() import $ from "jquery"; @@ -33,6 +34,14 @@ export interface IWorkspaceExports { hideColorPickerDialog(): void; showCopyrightAndLicenseDialog(imageUrl?: string): void; showEditViewTopicChooserDialog(): void; + showLinkTargetChooserDialog( + currentUrl: string, + onSetUrl: (url: string) => void, + ): void; + showBookGridSetupDialog( + currentLinks: Link[], + setLinksCallback: (links: Link[]) => void, + ): void; showAdjustTimingsDialogFromWorkspaceRoot( currentTextBox: HTMLElement, // The split and applyTimingsFile calls both return a list of new timings, @@ -70,6 +79,13 @@ import { getEditablePageBundleExports } from "./js/workspaceFrames"; export { getEditablePageBundleExports }; import { showPageChooserDialog } from "../pageChooser/PageChooserDialog"; export { showPageChooserDialog }; +// These two are launched from code that runs in the page iframe. They must be shown from here, +// the workspace root, so that their modal backdrop covers the whole workspace including the +// page list; rendered in the page iframe the backdrop covers only the book pane (BL-16809). +import { showLinkTargetChooserDialog } from "../react_components/LinkTargetChooser/LinkTargetChooserDialogLauncher"; +export { showLinkTargetChooserDialog }; +import { showBookGridSetupDialog } from "../react_components/BookGridSetup/BookGridSetupDialog"; +export { showBookGridSetupDialog }; import "../lib/errorHandler"; import { showBookSettingsDialog } from "./bookAndPageSettings/BookAndPageSettingsDialog"; @@ -449,6 +465,8 @@ interface WorkspaceBundleApi { getToolboxBundleExports: typeof getToolboxBundleExports; getEditablePageBundleExports: typeof getEditablePageBundleExports; showPageChooserDialog: typeof showPageChooserDialog; + showLinkTargetChooserDialog: typeof showLinkTargetChooserDialog; + showBookGridSetupDialog: typeof showBookGridSetupDialog; showBookSettingsDialog: typeof showBookSettingsDialog; showRegistrationDialog: typeof showRegistrationDialogForEditTab; showAboutDialog: typeof showAboutDialog; @@ -495,6 +513,8 @@ window.workspaceBundle = { getToolboxBundleExports, getEditablePageBundleExports, showPageChooserDialog, + showLinkTargetChooserDialog, + showBookGridSetupDialog, showBookSettingsDialog, showRegistrationDialog: showRegistrationDialogForEditTab, showAboutDialog, diff --git a/src/BloomBrowserUI/pageChooser/PageChooserDialog.tsx b/src/BloomBrowserUI/pageChooser/PageChooserDialog.tsx index 596fdfa97095..b50be645531a 100644 --- a/src/BloomBrowserUI/pageChooser/PageChooserDialog.tsx +++ b/src/BloomBrowserUI/pageChooser/PageChooserDialog.tsx @@ -124,10 +124,7 @@ export const PageChooserDialog: React.FunctionComponent< HTMLDivElement | undefined >(undefined); - // Tell edit tab to disable everything when the dialog is up. - // (Without this, the page list is not disabled since the modal - // div only exists in the book pane. Once the whole edit tab is inside - // one browser, this would not be necessary.) + // Tell C# to lock the workspace tabs while the dialog is up. useEffect(() => { if (open === undefined) return; diff --git a/src/BloomBrowserUI/react_components/LinkTargetChooser/LinkTargetChooserDialogLauncher.tsx b/src/BloomBrowserUI/react_components/LinkTargetChooser/LinkTargetChooserDialogLauncher.tsx index 0995d5b06688..52bf08b1e86e 100644 --- a/src/BloomBrowserUI/react_components/LinkTargetChooser/LinkTargetChooserDialogLauncher.tsx +++ b/src/BloomBrowserUI/react_components/LinkTargetChooser/LinkTargetChooserDialogLauncher.tsx @@ -2,6 +2,9 @@ import * as React from "react"; import { ShowEditViewDialog } from "../../bookEdit/workspaceRoot"; import { LinkTargetChooserDialog } from "./LinkTargetChooserDialog"; +// Call this only from the workspace root (from the page iframe, go through +// getWorkspaceBundleExports().showLinkTargetChooserDialog): ShowEditViewDialog renders in the +// calling frame's document, and only in the root does the dialog's backdrop cover the page list. export const showLinkTargetChooserDialog = ( currentUrl: string, onSetUrl: (url: string) => void, diff --git a/src/BloomBrowserUI/react_components/registration/registrationDialogLauncher.tsx b/src/BloomBrowserUI/react_components/registration/registrationDialogLauncher.tsx index eba301eff105..e83650f23606 100644 --- a/src/BloomBrowserUI/react_components/registration/registrationDialogLauncher.tsx +++ b/src/BloomBrowserUI/react_components/registration/registrationDialogLauncher.tsx @@ -38,7 +38,7 @@ export const RegistrationDialogLauncher: React.FunctionComponent< React.useEffect(() => { if (props.dialogEnvironment?.mode === Mode.Edit) { - // Tell edit tab to disable everything when the dialog is up + // Tell C# to lock the workspace tabs while the dialog is up postBoolean("editView/setModalState", propsForBloomDialog.open); } }, [props.dialogEnvironment?.mode, propsForBloomDialog.open]); diff --git a/src/BloomExe/Edit/EditingView.cs b/src/BloomExe/Edit/EditingView.cs index 0ce1581e4b58..5fbe57cdf7d5 100644 --- a/src/BloomExe/Edit/EditingView.cs +++ b/src/BloomExe/Edit/EditingView.cs @@ -506,7 +506,7 @@ internal void StartNavigationToEditPage(IPage page) _model.SetupServerWithCurrentBookToolboxContents(); WorkspaceView.ReloadWorkspaceRootDocument(); } - SetModalState(false); // ensure _pageListView is enabled (BL-9712). + SetModalState(false); // ensure the tabs are not left locked (BL-9712). #if MEMORYCHECK // Check memory for the benefit of developers. Bloom.Utils.MemoryManagement.CheckMemory( @@ -1545,8 +1545,17 @@ public void Dispose() public string HelpTopicUrl => "/Tasks/Edit_tasks/Edit_tasks_overview.htm"; /// - /// Prevent navigation, e.g. while a dialog box is showing in the browser control + /// Lock workspace navigation (the tabs), e.g. while a dialog box is showing in the browser + /// control. Calls nest: each true must be matched by a false. /// + /// + /// This used to disable the page list as well, because the page list lived in its own + /// browser and a modal dialog's backdrop in the main browser could not cover it. The whole + /// edit tab is in one browser now, so the backdrop already blocks the page list, and the + /// C# gate could only do harm: the browser posted the dialog's "closed" notice and the command + /// the dialog confirmed (e.g. Remove Page) as two concurrent requests, and when the command was + /// handled first it was silently refused (BL-16809). + /// internal void SetModalState(bool isModal) { if (isModal) @@ -1554,9 +1563,7 @@ internal void SetModalState(bool isModal) else _modalDialogDepth = Math.Max(0, _modalDialogDepth - 1); - var isActuallyModal = _modalDialogDepth > 0; - _pageListView.Enabled = !isActuallyModal; - _workspaceView?.SetTabsEnabled(!isActuallyModal); + _workspaceView?.SetTabsEnabled(_modalDialogDepth == 0); } public void ShowAddPageDialog() diff --git a/src/BloomExe/Edit/PageListController.cs b/src/BloomExe/Edit/PageListController.cs index 9b2be121eb49..0311f0245256 100644 --- a/src/BloomExe/Edit/PageListController.cs +++ b/src/BloomExe/Edit/PageListController.cs @@ -105,10 +105,5 @@ public void EmptyThumbnailCache() { _thumbNailList.EmptyThumbnailCache(); } - - public bool Enabled - { - set { _thumbNailList.Enabled = value; } - } } } diff --git a/src/BloomExe/Edit/PageThumbnailList.cs b/src/BloomExe/Edit/PageThumbnailList.cs index 06eb94464aff..3b4d36f7b22d 100644 --- a/src/BloomExe/Edit/PageThumbnailList.cs +++ b/src/BloomExe/Edit/PageThumbnailList.cs @@ -203,8 +203,7 @@ private List UpdateItemsInternal(IEnumerable pages) internal void PageClicked(IPage page) { - if (Enabled) - InvokePageSelectedChanged(page); + InvokePageSelectedChanged(page); } /// @@ -217,7 +216,7 @@ internal void PageClicked(IPage page) /// internal bool IsContextMenuCommandEnabled(IPage page, string commandId) { - if (!Enabled || page == null) + if (page == null) return false; switch (commandId) @@ -272,7 +271,6 @@ internal void ExecuteContextMenuCommand(IPage page, string commandId) } private PageListApi _pageListApi; - internal bool Enabled = true; // This gets invoked by Javascript (via the PageListApi) when it determines that a particular page has been moved. // newIndex is the (zero-based) index that the page is moving to