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)