Keep PublishHelper.InPublishTab in step with the active tab (BL-16174)

src/BloomExe/Publish/PublishHelper.cs

@@ -48,6 +48,13 @@ public static void Cancel()
             _latestInstance = null;
         }
 
+        /// <summary>
+        /// A static mirror of WorkspaceTabSelection.ActiveTab == WorkspaceTab.publish, which exists
+        /// only because the code that stages books (and so hits the guard in the constructor above)
+        /// is static and cannot get at the one WorkspaceTabSelection. Its setter is the sole writer:
+        /// do not set this from anywhere else, or the two can drift apart and turn an ordinary
+        /// publish into the exception above (BL-16174).
+        /// </summary>
         public static bool InPublishTab { get; set; }
 
         private OffScreenBrowser _pageChecksBrowser;

src/BloomExe/Publish/PublishView.cs

@@ -52,19 +52,20 @@ BloomWebSocketServer webSocketServer
             //off the tab itself changing, either to us or away from us.
             selectedTabChangedEvent.Subscribe(_ =>
             {
-                if (_tabSelection.ActiveTab == WorkspaceTab.publish)
-                {
-                    if (!_isActive)
-                    {
-                        Activate();
-                        _isActive = true;
-                    }
-                }
-                else if (_isActive)
-                {
+                var shouldBeActive = _tabSelection.ActiveTab == WorkspaceTab.publish;
+                if (shouldBeActive == _isActive)
+                    return;
+                // Record where we are going before doing the work, not after. If some part of
+                // activating or deactivating throws, we must not be left believing we are still in
+                // the state we just left: that stale belief would silently skip the *next*
+                // Activate(), and with it the SetTabsEnabled(true) below that releases the Edit
+                // tab's save lock -- leaving the Collection and Edit tabs greyed out until the user
+                // restarts Bloom. That was half of BL-16174. The exception itself still propagates.
+                _isActive = shouldBeActive;
+                if (shouldBeActive)
+                    Activate();
+                else
                     Deactivate();
-                    _isActive = false;
-                }
             });
 
             //TODO: find a way to call this just once, at the right time:
@@ -80,7 +81,8 @@ private void Deactivate()
             _publishToVideoApi.AbortMakingVideo();
             // TODO-WV2: Can we clear the cache for WV2? Do we need to?
             PublishHelper.Cancel();
-            PublishHelper.InPublishTab = false;
+            // Note: PublishHelper.InPublishTab is not ours to clear; WorkspaceTabSelection.ActiveTab
+            // has already done it. See the comment there (BL-16174).
             _webSocketServer.SendEvent("publish", "switchOutOfPublishTab");
         }
 
@@ -104,7 +106,9 @@ private void Activate()
             // Safety net: any Edit-tab save lock must be complete before we reach Publish,
             // so ensure tab switching is enabled in case the re-enable callback was missed.
             WorkspaceView?.SetTabsEnabled(true);
-            PublishHelper.InPublishTab = true;
+            // Note: PublishHelper.InPublishTab is not ours to set; WorkspaceTabSelection.ActiveTab
+            // has already done it, before any of this event's subscribers ran. See the comment
+            // there (BL-16174).
             var hostForm = GetHostControlForInvoke() as Form;
             PublishEpubApi.ControlForInvoke = hostForm;
             LibraryPublishApi.Model = new BloomLibraryPublishModel(

src/BloomExe/Workspace/WorkspaceTabSelection.cs

@@ -3,6 +3,7 @@
 using System.Linq;
 using System.Text;
 using System.Threading.Tasks;
+using Bloom.Publish;
 
 namespace Bloom.Workspace
 {
@@ -20,6 +21,31 @@ public enum WorkspaceTab
     /// </summary>
     public class WorkspaceTabSelection
     {
-        public WorkspaceTab ActiveTab;
+        private WorkspaceTab _activeTab;
+
+        /// <summary>
+        /// The tab the Workspace is currently showing. This is the authoritative answer to
+        /// "which tab are we on"; everything else that needs to know should either read this or
+        /// be kept in step by this setter.
+        /// </summary>
+        public WorkspaceTab ActiveTab
+        {
+            get => _activeTab;
+            set
+            {
+                _activeTab = value;
+                // PublishHelper refuses to stage a book while we are not in the Publish tab, but it
+                // is reached from static code that has no way to get hold of this object, so it needs
+                // a static mirror of this value. We update it here, as part of the same assignment,
+                // precisely so the two cannot disagree. PublishView used to own it instead, setting it
+                // from its SelectedTabChangedEvent subscriber -- which runs later, after ActiveTab has
+                // already changed and after however many other subscribers. Any gap or hiccup in
+                // between (e.g. an activation that got skipped) left ActiveTab saying "publish" while
+                // the flag still said "not publish", and then an ordinary switch-to-Publish-and-make-a-
+                // BloomPUB-preview died with "Should not be creating bloom book while not in publish
+                // tab". See BL-16174.
+                PublishHelper.InPublishTab = value == WorkspaceTab.publish;
+            }
+        }
     }
 }

src/BloomExe/web/controllers/PublishApi.cs

@@ -778,6 +778,11 @@ public string StageBookForBloomPubPreviewForTest(Book.Book book)
             // put Bloom into the publish tab first (POST workspace/selectTab {tab:"publish"}), which
             // runs all the real publish-tab setup; otherwise we might miss setup that the tab does
             // now or adds later. Fail Fast here (via the guard) if the caller forgot to do that.
+            // Note what the guard does and does not prove: since BL-16174 the flag is set by the
+            // ActiveTab setter, so it goes true when the tab becomes active, ahead of the
+            // SelectedTabChangedEvent subscribers that do the rest of the setup (PublishView.Activate).
+            // What makes the setup complete by the time we get here is that selectTab runs the whole
+            // switch synchronously inside its own API call, which the caller awaits -- not the flag.
             if (!PublishHelper.InPublishTab)
                 throw new InvalidOperationException(
                     "makeBloomPubPreview requires the publish tab to be active. "

src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs

@@ -0,0 +1,98 @@
+using Bloom.Publish;
+using Bloom.Workspace;
+using NUnit.Framework;
+
+namespace BloomTests.Workspace
+{
+    /// <summary>
+    /// PublishHelper.InPublishTab is a static mirror of ActiveTab, needed because the book-staging
+    /// code that consults it is static. These tests pin down that the mirror is updated by the
+    /// ActiveTab setter itself, so nothing can observe the two disagreeing. When they could
+    /// disagree, switching to the Publish tab and asking for a BloomPUB preview sometimes died with
+    /// "Should not be creating bloom book while not in publish tab" (BL-16174).
+    /// </summary>
+    [TestFixture]
+    // These tests read and write PublishHelper.InPublishTab, which is process-wide. Setup/TearDown
+    // put it back, which is enough while fixtures run one at a time, but say so explicitly so that
+    // turning parallel test execution on later cannot quietly let this fixture and the publish
+    // tests perturb each other.
+    [NonParallelizable]
+    public class WorkspaceTabSelectionTests
+    {
+        private bool _originalInPublishTab;
+
+        [SetUp]
+        public void Setup()
+        {
+            // It's a static, shared with the rest of the test run, so put it back afterwards.
+            _originalInPublishTab = PublishHelper.InPublishTab;
+            PublishHelper.InPublishTab = false;
+        }
+
+        [TearDown]
+        public void TearDown()
+        {
+            PublishHelper.InPublishTab = _originalInPublishTab;
+        }
+
+        [Test]
+        public void ActiveTab_SetToPublish_SetsInPublishTab()
+        {
+            var selection = new WorkspaceTabSelection();
+            Assert.That(
+                PublishHelper.InPublishTab,
+                Is.False,
+                "Sanity check: this test is meaningless unless the flag starts out false."
+            );
+
+            selection.ActiveTab = WorkspaceTab.publish;
+
+            Assert.That(selection.ActiveTab, Is.EqualTo(WorkspaceTab.publish));
+            Assert.That(PublishHelper.InPublishTab, Is.True);
+        }
+
+        [TestCase(WorkspaceTab.collection)]
+        [TestCase(WorkspaceTab.edit)]
+        public void ActiveTab_SetToOtherTab_ClearsInPublishTab(WorkspaceTab tab)
+        {
+            var selection = new WorkspaceTabSelection();
+            selection.ActiveTab = WorkspaceTab.publish;
+            Assert.That(
+                PublishHelper.InPublishTab,
+                Is.True,
+                "Sanity check: we should be starting from the publish tab."
+            );
+
+            selection.ActiveTab = tab;
+
+            Assert.That(selection.ActiveTab, Is.EqualTo(tab));
+            Assert.That(PublishHelper.InPublishTab, Is.False);
+        }
+
+        /// <summary>
+        /// Opening a different collection builds a whole new ProjectContext, and so a new
+        /// WorkspaceTabSelection, but InPublishTab is static and survives. Note that merely
+        /// constructing the new WorkspaceTabSelection does not clear it -- what clears it is the
+        /// new WorkspaceView constructor's `_tabSelection.ActiveTab = WorkspaceTab.collection`
+        /// (WorkspaceView.cs), which this test stands in for. So the invariant depends on that
+        /// line continuing to exist; without it we would carry "we are in the publish tab" over
+        /// into a collection that is sitting on its Collection tab.
+        /// </summary>
+        [Test]
+        public void ActiveTab_NewSelectionInitializedToCollection_ClearsStaleInPublishTab()
+        {
+            new WorkspaceTabSelection().ActiveTab = WorkspaceTab.publish;
+            Assert.That(
+                PublishHelper.InPublishTab,
+                Is.True,
+                "Sanity check: we should be starting from the publish tab."
+            );
+
+            // What WorkspaceView's constructor does for the new collection.
+            var newSelection = new WorkspaceTabSelection();
+            newSelection.ActiveTab = WorkspaceTab.collection;
+
+            Assert.That(PublishHelper.InPublishTab, Is.False);
+        }
+    }
+}