From 40ce746790bdbb569108d9877c0220a2e525127a Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Thu, 6 Aug 2026 22:00:52 -0700 Subject: [PATCH] Make the publish upload fully modal (BL-16654) Uploading a book to BloomLibrary already greyed out the main workspace tabs, but left the publish-tool buttons (PDF & Print, Web, BloomPUB, ...) live. One click tore down the Web screen, which took its Cancel button, its progress log and its eventual "Your Book on BloomLibrary.org" link with it while the upload carried on invisibly; coming back offered an enabled Upload button, so a second concurrent upload of the same book was reachable. Rather than give the Publish tab a second, weaker notion of "busy", report the lock C# already keeps: WorkspaceView.GetTabInfo now sends navigationLocked alongside the tab states, and PublishTabPane vetoes tool switching and greys out the other tools while it is set. Because that is the same flag C# toggles in a try/finally around the upload, the tools unlock when the upload really finishes or is cancelled, not when the browser guesses it has. The Apps tool gets this for free, so its appsBusy state and the onBusyChange callback threaded down through AppPublisherScreen are gone, and the lock now survives that screen remounting mid-action. Refinements that came out of review: The lock is gated on a tool actually being selected. Without that, a lock set while tabIndex is still the "no tool chosen yet" sentinel greys out all six tools and leaves no way to pick one -- reachable because the flag is shared with other subsystems, including the Copyright and License dialog, which posts editView/setModalState ungated and is reachable from Publish > Web. The Web screen also reports "an upload is under way" up to PublishTabPane, OR-ed with C#'s flag, because uploadOneBook() shows Cancel and a progress log before making two API round trips that precede C# taking its lock -- leaving the tools live during a window the user already reads as uploading. OR is the whole point and is commented at both ends: isUploading is untrustworthy as an *unlock* signal (Cancel clears it while C# works on, and so does any error line), but as an extra *lock* term it can only lock more than C# alone would, never less. Those two pre-upload requests now also pass an error callback. If one dies at the transport level there is no reply and no progress message, so nothing would ever clear isUploading; that used to leave just a stale Cancel button, but with the tool strip following the flag it would have disabled the other tools with no obvious way back. SetTabsEnabled stays a single shared flag rather than a count. With the tool switcher locked too, a second publish operation is unreachable while one runs, so the overlap the old RabPublishApi comment worried about can't happen; that comment is updated, and HandleSetModalState now documents that despite its "editView" name it is reached from outside the Edit tab. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomBrowserUI/app/App.tsx | 10 +-- .../publish/Apps/AppPublisherScreen.tsx | 26 ++----- .../LibraryPublish/LibraryPublishScreen.tsx | 8 +- .../LibraryPublish/LibraryPublishSteps.tsx | 78 ++++++++++++++----- .../publish/PublishTab/PublishTabPane.tsx | 57 ++++++++++---- .../react_components/TopBar/TopBar.tsx | 23 +++++- src/BloomExe/Publish/Rab/RabPublishApi.cs | 11 +-- src/BloomExe/Workspace/WorkspaceView.cs | 8 ++ .../web/controllers/EditingViewApi.cs | 13 ++++ 9 files changed, 161 insertions(+), 73 deletions(-) diff --git a/src/BloomBrowserUI/app/App.tsx b/src/BloomBrowserUI/app/App.tsx index dad0f50ad1a2..11b290c7e944 100644 --- a/src/BloomBrowserUI/app/App.tsx +++ b/src/BloomBrowserUI/app/App.tsx @@ -8,12 +8,11 @@ import { defaultWorkspaceTabState, getActiveWorkspaceTab, TopBar, - TabStates, + useWorkspaceTabInfo, WorkspaceTabId, } from "../react_components/TopBar/TopBar"; import { PublishTabPane } from "../publish/PublishTab/PublishTabPane"; import { kPanelBackground } from "../bloomMaterialUITheme"; -import { useWatchApiObject } from "../utils/bloomApi"; import { EditTabPane } from "./EditTabPane"; import { ToastHost } from "../toast/ToastHost"; @@ -27,12 +26,7 @@ export const App: React.FunctionComponent = () => { // Team collection toast click returns to collection tab in WorkspaceView.cs:606. // Publish flow can force jump to edit tab in LibraryPublishApi.cs:517. // Edit-book command switches to edit tab in WorkspaceView.cs:1164. - const state = useWatchApiObject<{ tabStates: TabStates }>( - "workspace/tabs", - defaultWorkspaceTabState, - "workspace", - "tabs", - ); + const state = useWorkspaceTabInfo(); const tabStates = state.tabStates ?? defaultWorkspaceTabState.tabStates; const activeTab = React.useMemo((): WorkspaceTabId => { diff --git a/src/BloomBrowserUI/publish/Apps/AppPublisherScreen.tsx b/src/BloomBrowserUI/publish/Apps/AppPublisherScreen.tsx index 5b4ad4c36442..0e38b3fad057 100644 --- a/src/BloomBrowserUI/publish/Apps/AppPublisherScreen.tsx +++ b/src/BloomBrowserUI/publish/Apps/AppPublisherScreen.tsx @@ -87,7 +87,6 @@ const AppActionButton: React.FunctionComponent<{ // Keep this component mostly declarative. The hook owns websocket/API state so the JSX can stay focused on the workflow. const AppPublisherScreenContents: React.FunctionComponent<{ isActive: boolean; - onBusyChange?: (busy: boolean) => void; }> = (props) => { const screenState = useAppBuilderPublisherScreen(props.isActive); const [showSettingsDialog, setShowSettingsDialog] = React.useState(false); @@ -95,21 +94,10 @@ const AppPublisherScreenContents: React.FunctionComponent<{ React.useState(false); const [showUsbDebuggingHelpDialog, setShowUsbDebuggingHelpDialog] = React.useState(false); - // An effect (not an event handler) is warranted here, even though "notify the parent of a - // change" usually belongs in the handler that caused the change: busyAction has no single - // originating handler. It is set/cleared from several asynchronous sources inside - // useAppBuilderPublisherScreen — the "actionComplete" websocket event, the status-poll - // recovery that reconciles with the backend after a blank/reload, and the action-start call — - // so the only place that observes every transition is a render keyed on the resulting value. - // What we are doing is synchronizing an external system (the publish-tab host, which makes the - // operation modal by blocking the other publish tools while C# blocks the main workspace tabs) - // to that state, which is exactly what effects are for. The cleanup resets it to false so - // leaving or unmounting never leaves the publish tools stuck disabled. - React.useEffect(() => { - props.onBusyChange?.(!!screenState.busyAction); - return () => props.onBusyChange?.(false); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [screenState.busyAction]); + // Note: this screen no longer tells the publish-tab host when it is busy. Blocking the other + // publish tools during a prepare/build/install is now driven by the same C# navigation lock + // that greys out the main workspace tabs (RabPublishApi sets it; PublishTabPane reads it), + // so the two can't disagree, and the lock survives this component remounting mid-action. const prepareTooltip = useL10n( "Create the Reading App Builder project in this collection's Bloom App Data folder.", "PublishTab.Apps.Prepare.TooltipBloomAppData", @@ -731,7 +719,6 @@ const AppPublisherScreenContents: React.FunctionComponent<{ export const AppPublisherScreen: React.FunctionComponent<{ isActive: boolean; - onBusyChange?: (busy: boolean) => void; }> = (props) => { const optionsPanel = ( @@ -770,10 +757,7 @@ export const AppPublisherScreen: React.FunctionComponent<{ bannerDescriptionMarkdown="Create an app that you can install on your Android phone, share with others, and publish on the Google Play Store." optionsPanelContents={optionsPanel} > - + ); diff --git a/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishScreen.tsx b/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishScreen.tsx index 0e77ec21f33a..cfb2f045d211 100644 --- a/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishScreen.tsx +++ b/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishScreen.tsx @@ -1,4 +1,5 @@ import { css } from "@emotion/react"; +import * as React from "react"; import { Typography } from "@mui/material"; import { Link } from "../../react_components/link"; import { @@ -18,10 +19,13 @@ import { PublishTopic } from "../commonPublish/PublishTopic"; export const kWebSocketContext = "libraryPublish"; -export const LibraryPublishScreen = () => { +export const LibraryPublishScreen: React.FunctionComponent<{ + // Passed straight through to LibraryPublishSteps, which owns the upload state. + onUploadingChange?: (uploading: boolean) => void; +}> = (props) => { const mainPanel = ( - + ); diff --git a/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx b/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx index 55e6560139d5..67a543e4f364 100644 --- a/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx +++ b/src/BloomBrowserUI/publish/LibraryPublish/LibraryPublishSteps.tsx @@ -62,7 +62,11 @@ interface IReadonlyBookInfo { const kWebSocketEventId_uploadSuccessful: string = "uploadSuccessful"; const kWebSocketEventId_uploadCanceled: string = "uploadCanceled"; -export const LibraryPublishSteps: React.FunctionComponent = () => { +export const LibraryPublishSteps: React.FunctionComponent<{ + // Called with true once the user commits to an upload and false when it is over. See the + // effect below for why the publish-tab host may only OR this into its lock. + onUploadingChange?: (uploading: boolean) => void; +}> = (props) => { const selectedBookContext = React.useContext(SelectedBookContext); const [bookshelfHasProblem, setBookshelfHasProblem] = useState(false); const { @@ -218,28 +222,62 @@ export const LibraryPublishSteps: React.FunctionComponent = () => { const [conflictIndex, setConflictIndex] = useState(0); const [isUploading, setIsUploading] = useState(false); + // Tell the publish-tab host as soon as the user commits to an upload, so it can lock the + // publish-tool switcher for the whole operation and not just the part C# knows about. C# + // takes its lock inside UploadBookAsync, but by then we have already made two API round + // trips (the subscription check and the "existing copy on server" query), during which the + // screen shows Cancel and a progress log while the tools were still live (BL-16654). + // The host OR-s this with C#'s lock rather than replacing it. That direction matters: this + // flag is not trustworthy as an *unlock* signal — clicking Cancel clears it while C# keeps + // working, and so does any error line in the progress log — but as an extra *lock* term it + // can only ever lock more than C# would, never less, so the unreliability is harmless here + // and C# stays the authority on when things reopen. + // + // Why an effect rather than doing this in the handler that starts the upload: isUploading has + // no single originating handler. It is set in uploadOneBook and cleared from four unrelated + // places — the Cancel button, the uploadSuccessful websocket, an error line in the progress + // box, and the collision dialog's cancel — so a render keyed on the resulting value is the + // only place that observes every transition. What we are doing is synchronizing an external + // system (the publish-tab host) to this state, which is what effects are for. + useEffect(() => { + props.onUploadingChange?.(isUploading); + // Never leave the host locked if this screen goes away mid-upload. + return () => props.onUploadingChange?.(false); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [isUploading]); function uploadOneBook() { setIsUploadComplete(false); setIsUploading(true); - get("libraryPublish/checkSubscriptionMatch", (result) => { - if (result.data.error) { - // The API already sent an error message - return; - } - get( - "libraryPublish/getUploadCollisionInfo?index=" + conflictIndex, - (result) => { - if (result.data.error) { - // The API already sent an error message - return; - } - if (result.data.shouldShow) { - setUploadCollisionInfo(result.data); - showUploadCollisionDialog(); - } else post("libraryPublish/upload"); - }, - ); - }); + // If either pre-upload request dies at the transport level we get no reply and no progress + // message, so nothing else would ever clear isUploading. That used to leave only a stale + // Cancel button, but now it would also keep the other publish tools disabled, so clear it + // here. (An error *reported by* the API still arrives as a progress message and is handled + // by handleUploadError.) + get( + "libraryPublish/checkSubscriptionMatch", + (result) => { + if (result.data.error) { + // The API already sent an error message + return; + } + get( + "libraryPublish/getUploadCollisionInfo?index=" + + conflictIndex, + (result) => { + if (result.data.error) { + // The API already sent an error message + return; + } + if (result.data.shouldShow) { + setUploadCollisionInfo(result.data); + showUploadCollisionDialog(); + } else post("libraryPublish/upload"); + }, + handleUploadError, + ); + }, + handleUploadError, + ); } const changeConflictIndex = (index: number) => { diff --git a/src/BloomBrowserUI/publish/PublishTab/PublishTabPane.tsx b/src/BloomBrowserUI/publish/PublishTab/PublishTabPane.tsx index 72d1572cc5f0..c763008b7d4d 100644 --- a/src/BloomBrowserUI/publish/PublishTab/PublishTabPane.tsx +++ b/src/BloomBrowserUI/publish/PublishTab/PublishTabPane.tsx @@ -30,6 +30,7 @@ import { import { AboutDialogLauncher } from "../../react_components/aboutDialog"; import { RegistrationDialogEventLauncher } from "../../react_components/registration/registrationDialogLauncher"; import { RequiresSubscriptionOverlayWrapper } from "../../react_components/requiresSubscription"; +import { useWorkspaceTabInfo } from "../../react_components/TopBar/TopBar"; export const CheckoutNeededScreen: React.FunctionComponent<{ titleForDisplay: string; @@ -108,9 +109,31 @@ export const PublishTabPane: React.FunctionComponent = () => { const [tabIndex, setTabIndex] = React.useState( kWaitForUserToChooseTabIndex, ); - // True while the Apps tool has a Reading App Builder action running (its Cancel button is - // showing). While busy, switching to another publish tool is blocked so the operation is modal. - const [appsBusy, setAppsBusy] = React.useState(false); + // True while a long-running publish operation has made itself modal by locking navigation: + // a BloomLibrary upload, or one of the Apps tool's Reading App Builder actions. C# owns this + // flag (it is the same one that greys out the main workspace tabs), so the publish tools + // unlock at exactly the moment the operation really finishes or is cancelled — not when the + // browser guesses it has. See BL-16654. + const navigationLocked = useWorkspaceTabInfo().navigationLocked; + // The Web tool tells us directly when the user has committed to an upload, because C# does + // not take its lock until a couple of API round trips later, leaving a window where the + // screen already shows Cancel but the tools were still clickable (BL-16654). + const [uploadUnderway, setUploadUnderway] = React.useState(false); + // OR, never AND. C#'s flag is the authority on when an operation has really finished, and + // uploadUnderway is deliberately only ever an *additional* reason to lock: it is unreliable + // as an unlock signal (it clears the moment Cancel is pressed, and on any error line in the + // progress log) but adding it can only lock more than C# alone would, never less. + // + // Both are then gated on a tool actually showing. The lock exists to stop the user walking + // away from an operation in progress, and none can be in progress while the sentinel "no tool + // chosen yet" panel is up. That gate matters because the C# flag is shared with other + // subsystems — e.g. the Copyright and License dialog, reachable from this tab's own "Missing + // Copyright" link, posts editView/setModalState, which locks. Without it, a lock still set + // while tabIndex is the sentinel would grey out every tool at once and leave the user no way + // to choose one at all. + const publishToolsLocked = + (navigationLocked || uploadUnderway) && + tabIndex !== kWaitForUserToChooseTabIndex; const appBuilderFeatureStatus = useGetFeatureStatus("AppBuilder"); const setup = () => { setTabIndex(kWaitForUserToChooseTabIndex); @@ -242,11 +265,11 @@ export const PublishTabPane: React.FunctionComponent = () => { labelBackgroundColor={kPanelBackground} selectedIndex={tabIndex} onSelect={(newIndex) => { - // While a Reading App Builder action is running (its Cancel button - // is showing), the Apps operation is modal: veto switching to another - // publish tool until it finishes or is cancelled. The main workspace - // tabs are locked from C# (RabPublishApi) to match. - if (appsBusy) { + // While an upload or an Apps action is running (its Cancel button is + // showing), the operation is modal: veto switching to another publish + // tool until it finishes or is cancelled. The main workspace tabs are + // locked from C# by the same flag. + if (publishToolsLocked) { return false; } post("publish/switchingPublishMode"); @@ -311,7 +334,7 @@ export const PublishTabPane: React.FunctionComponent = () => { display: none; } // Doubled class for enough specificity to override the tab color - // rule above, so tools disabled during a modal Apps action read as + // rule above, so tools disabled during a modal operation read as // greyed out (react-tabs already makes them non-clickable). .react-tabs__tab--disabled.react-tabs__tab--disabled { opacity: 0.4; @@ -331,9 +354,14 @@ export const PublishTabPane: React.FunctionComponent = () => { {publishTabs.map((tab, index) => ( { {publishTabInfo.canUpload ? ( - + ) : ( { isActive={ publishTabs[tabIndex]?.id === "apps" } - onBusyChange={setAppsBusy} /> diff --git a/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx b/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx index 9587b3811f83..6fea416fba82 100644 --- a/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx +++ b/src/BloomBrowserUI/react_components/TopBar/TopBar.tsx @@ -22,6 +22,16 @@ export type WorkspaceTabState = "active" | "enabled" | "disabled" | "hidden"; export type TabStates = Record; +// What C# (WorkspaceView.GetTabInfo) tells us about workspace navigation. +export interface IWorkspaceTabInfo { + tabStates: TabStates; + // True while some operation has made itself modal by locking navigation: a BloomLibrary + // upload, a Reading App Builder action, or an Edit-tab modal dialog. Screens with their own + // navigation (notably the Publish tab's switcher between publish tools) use this to lock in + // step with the main tabs. + navigationLocked: boolean; +} + interface ITabDefinition { id: WorkspaceTabId; l10nId: string; @@ -57,21 +67,28 @@ export function getActiveWorkspaceTab(tabStates: TabStates): WorkspaceTabId { ); } -export const defaultWorkspaceTabState: { tabStates: TabStates } = { +export const defaultWorkspaceTabState: IWorkspaceTabInfo = { tabStates: { collection: "active", edit: "hidden", publish: "hidden", }, + navigationLocked: false, }; -export const TopBar: React.FunctionComponent = () => { - const state = useWatchApiObject<{ tabStates: TabStates }>( +// Subscribes to what C# says about workspace navigation, kept in one place because several +// screens in different browser controls need the same answer. +export function useWorkspaceTabInfo(): IWorkspaceTabInfo { + return useWatchApiObject( "workspace/tabs", defaultWorkspaceTabState, "workspace", "tabs", ); +} + +export const TopBar: React.FunctionComponent = () => { + const state = useWorkspaceTabInfo(); const topBarRef = React.useRef(null); const tabStates = state.tabStates ?? defaultWorkspaceTabState.tabStates; diff --git a/src/BloomExe/Publish/Rab/RabPublishApi.cs b/src/BloomExe/Publish/Rab/RabPublishApi.cs index b9b2b8e33b5a..4752fb7296da 100644 --- a/src/BloomExe/Publish/Rab/RabPublishApi.cs +++ b/src/BloomExe/Publish/Rab/RabPublishApi.cs @@ -33,11 +33,12 @@ public RabPublishApi(RabProjectService rabProjectService, PublishView publishVie /// of a prepare/build/install action. While an action runs — i.e. while its Cancel button is /// showing — the operation is modal: the user cannot navigate to another workspace tab until /// it finishes or is cancelled. Mirrors how a BloomLibrary upload locks the tabs (see - /// LibraryPublishApi.SetParentControlsState). The publish-tool switcher on the Publish tab is - /// blocked separately on the React side (PublishTabPane). - /// Note that SetTabsEnabled is a single shared flag, so if an upload and an Apps action ever - /// overlap, whichever finishes first unlocks the tabs for both; making the lock compose across - /// its several independent users is BL-16654. + /// LibraryPublishApi.SetParentControlsState). The switcher between publish tools on the + /// Publish tab follows the same lock: WorkspaceView reports it to the browser as + /// "navigationLocked" and PublishTabPane disables the other tools while it is set (BL-16654). + /// SetTabsEnabled remains a single shared flag rather than a count, deliberately: now that the + /// publish-tool switcher is locked too, the user cannot start a second publish operation while + /// one is running, so two of them can no longer overlap and race to unlock the tabs. /// private void SetWorkspaceTabsEnabled(bool enable) { diff --git a/src/BloomExe/Workspace/WorkspaceView.cs b/src/BloomExe/Workspace/WorkspaceView.cs index cb59263c3418..2bc2ec6e6bf2 100644 --- a/src/BloomExe/Workspace/WorkspaceView.cs +++ b/src/BloomExe/Workspace/WorkspaceView.cs @@ -735,6 +735,14 @@ public dynamic GetTabInfo() tabInfo.tabStates.collection = GetTabStateForUi("collection", activeTabId); tabInfo.tabStates.edit = GetTabStateForUi("edit", activeTabId); tabInfo.tabStates.publish = GetTabStateForUi("publish", activeTabId); + // True while something has locked navigation to make itself modal: a BloomLibrary + // upload, a Reading App Builder action, or an Edit-tab modal dialog. The tabStates + // above already encode this for the main tabs, but the Publish tab also has its own + // switcher between publish tools (in a different browser control, so nothing we do + // here disables it for free). Reporting the lock itself, rather than making the + // Publish tab infer it from the tab states, lets that switcher lock and unlock in + // exact step with the main tabs. See BL-16654. + tabInfo.navigationLocked = !_tabsEnabled; return tabInfo; } diff --git a/src/BloomExe/web/controllers/EditingViewApi.cs b/src/BloomExe/web/controllers/EditingViewApi.cs index 0ead6418cb50..79fce2cea76c 100644 --- a/src/BloomExe/web/controllers/EditingViewApi.cs +++ b/src/BloomExe/web/controllers/EditingViewApi.cs @@ -505,6 +505,19 @@ private void HandleDuplicatePageMany(ApiRequest request) model.DuplicatePageManyTimes((int)requestData.numberOfTimes); } + /// + /// Despite the "editView" name, this is NOT reached only from the Edit tab, and the depth + /// counter it drives (EditingView.SetModalState) locks navigation for the whole workspace. + /// Most dialogs that post here gate it on their mode being Mode.Edit, but the Copyright and + /// License dialog does not, and it is reachable from the Publish tab via Publish > Web's + /// "Missing Copyright" link. So a dialog opened from Publish can lock and unlock the shared + /// navigation flag, which since BL-16654 also greys out the publish-tool switcher. + /// We looked at gating this to the Edit tab and decided against it: locking while a modal + /// dialog is up is the behavior we want wherever the dialog was opened from, and the gate is + /// in the callers rather than here. It is safe today only because that link appears when the + /// copyright is missing, which is itself what stops an upload from being in progress — an + /// invariant worth knowing about if you change either end. + /// public void HandleSetModalState(ApiRequest request) { lock (request)