From 49eaf0f06b88fb0ed2a25081e1cf0aadc47a5ab6 Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Mon, 10 Aug 2026 11:19:11 -0700 Subject: [PATCH 1/3] Fix BL-16174 publish/bloompub/updatePreview returned Error: Request failed with status code 503 https://issues.bloomlibrary.org/youtrack/issue/BL-16174 Switching from the Edit tab to Publish and asking for a BloomPUB preview sometimes threw "Should not be creating bloom book while not in publish tab" (InvalidOperationException from PublishHelper's constructor), showed the "Bloom had a problem" dialog, left the preview blank, and left the Collection and Edit tabs greyed out until Bloom was restarted. Bloom kept two separate records of "we are in the Publish tab": * WorkspaceTabSelection.ActiveTab, the authoritative one, assigned in WorkspaceView.ChangeTab's PostponedWork; and * the static PublishHelper.InPublishTab, which the book-staging code checks, maintained separately by PublishView.Activate/Deactivate from its SelectedTabChangedEvent subscriber. The second is updated strictly later than the first: after ActiveTab has already changed and after however many other subscribers have run. The reported log proves the two disagreed - PublishApi.MakeBloompubPreview's own guard (ActiveTab != publish) let the request through, so ActiveTab said "publish", while PublishHelper's guard fired, so the flag still said "not publish". Fix: give WorkspaceTabSelection.ActiveTab a setter that updates PublishHelper.InPublishTab in the same assignment, and stop PublishView from setting the flag at all. The two can no longer be observed disagreeing, and it also clears the flag when a new collection's WorkspaceTabSelection initializes to the Collection tab (the static previously survived that). Also stop PublishView's _isActive latch going stale: it is now set before Activate()/Deactivate() rather than after, so a throw inside either cannot leave us believing we are still in the state we just left. That stale belief silently skipped the next Activate(), and with it the SetTabsEnabled(true) that releases the Edit tab's save lock - which is what left the tabs greyed out. Exceptions still propagate; nothing is swallowed. Tests: added src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs (4 tests, covering both directions and the stale-static-across-collections case). Ran the full C# suite through build/agent-dotnet.sh: 3067 passed, 0 failed, 12 skipped. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomExe/Publish/PublishHelper.cs | 7 ++ src/BloomExe/Publish/PublishView.cs | 32 ++++--- .../Workspace/WorkspaceTabSelection.cs | 28 +++++- .../Workspace/WorkspaceTabSelectionTests.cs | 90 +++++++++++++++++++ 4 files changed, 142 insertions(+), 15 deletions(-) create mode 100644 src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs diff --git a/src/BloomExe/Publish/PublishHelper.cs b/src/BloomExe/Publish/PublishHelper.cs index db874eb003be..0d63d4bf5716 100644 --- a/src/BloomExe/Publish/PublishHelper.cs +++ b/src/BloomExe/Publish/PublishHelper.cs @@ -48,6 +48,13 @@ public static void Cancel() _latestInstance = null; } + /// + /// 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). + /// public static bool InPublishTab { get; set; } private OffScreenBrowser _pageChecksBrowser; diff --git a/src/BloomExe/Publish/PublishView.cs b/src/BloomExe/Publish/PublishView.cs index 174fe826466c..68aa7c5def11 100644 --- a/src/BloomExe/Publish/PublishView.cs +++ b/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( diff --git a/src/BloomExe/Workspace/WorkspaceTabSelection.cs b/src/BloomExe/Workspace/WorkspaceTabSelection.cs index 75cdc35b97c2..a32db863568c 100644 --- a/src/BloomExe/Workspace/WorkspaceTabSelection.cs +++ b/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 /// public class WorkspaceTabSelection { - public WorkspaceTab ActiveTab; + private WorkspaceTab _activeTab; + + /// + /// 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. + /// + 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; + } + } } } diff --git a/src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs b/src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs new file mode 100644 index 000000000000..3c582fad0d72 --- /dev/null +++ b/src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs @@ -0,0 +1,90 @@ +using Bloom.Publish; +using Bloom.Workspace; +using NUnit.Framework; + +namespace BloomTests.Workspace +{ + /// + /// 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). + /// + [TestFixture] + 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); + } + + /// + /// Opening a different collection builds a whole new ProjectContext, and so a new + /// WorkspaceTabSelection, but InPublishTab is static and survives. The new selection's + /// initialization must therefore clear it, or we would carry "we are in the publish tab" + /// over into a collection that is sitting on its Collection tab. + /// + [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); + } + } +} From 2ce2afc3a92f9bd4133e02661bb250081b81e9ad Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Mon, 10 Aug 2026 11:41:29 -0700 Subject: [PATCH 2/3] Address Devin's informational flags on the BL-16174 test fixture Two documentation/robustness points Devin raised on https://github.com/BloomBooks/BloomDesktop/pull/8183 (both Informational, no bugs): - The fixture's summary comment implied that constructing a new WorkspaceTabSelection is what clears the stale static after a collection switch. It isn't: what clears it is the new WorkspaceView constructor's `_tabSelection.ActiveTab = WorkspaceTab.collection`, which the test stands in for. Say so, and say that the invariant depends on that line continuing to exist. - Mark the fixture [NonParallelizable]. PublishHelper.InPublishTab is process-wide; Setup/TearDown restoring it is enough while fixtures run one at a time, but stating the constraint means turning parallel execution on later cannot quietly let this fixture and the publish tests perturb each other. Matches the existing use of the attribute in BloomTests/Publish/Rab/RabRealBuildTests.cs. Comments and a test attribute only; no production code touched. Tests: full C# suite green at the parent commit (3067 passed, 0 failed, 12 skipped); WorkspaceTabSelectionTests re-run green (4 passed) after these edits. Co-Authored-By: Claude Opus 5 (1M context) --- .../Workspace/WorkspaceTabSelectionTests.cs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs b/src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs index 3c582fad0d72..2ae36ddf8f56 100644 --- a/src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs +++ b/src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs @@ -12,6 +12,11 @@ namespace BloomTests.Workspace /// "Should not be creating bloom book while not in publish tab" (BL-16174). /// [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; @@ -66,9 +71,12 @@ public void ActiveTab_SetToOtherTab_ClearsInPublishTab(WorkspaceTab tab) /// /// Opening a different collection builds a whole new ProjectContext, and so a new - /// WorkspaceTabSelection, but InPublishTab is static and survives. The new selection's - /// initialization must therefore clear it, or we would carry "we are in the publish tab" - /// over into a collection that is sitting on its Collection tab. + /// 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. /// [Test] public void ActiveTab_NewSelectionInitializedToCollection_ClearsStaleInPublishTab() From e2c4649515f8808dd0ed364217c6fea540929d69 Mon Sep 17 00:00:00 2001 From: Andrew Polk Date: Mon, 10 Aug 2026 11:46:19 -0700 Subject: [PATCH 3/3] Correct what the e2e staging guard's comment claims (BL-16174 follow-up) Devin flagged (informational) that moving PublishHelper.InPublishTab into the ActiveTab setter weakened a guarantee that the comment on StageBookForBloomPubPreviewForTest still asserts. The flag now goes true when the tab becomes active, which is ahead of the SelectedTabChangedEvent subscribers -- so it no longer implies "PublishView.Activate has finished the real publish-tab setup", which is what the comment said it meant. The e2e flow is still correct, but for a different reason than the comment gave: selectTab runs the whole tab switch synchronously inside its own API call, which the caller awaits. Say that, so nobody later reasons from a guarantee the guard no longer provides. Comment only; no behavior change. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomExe/web/controllers/PublishApi.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/BloomExe/web/controllers/PublishApi.cs b/src/BloomExe/web/controllers/PublishApi.cs index 4405064159a9..a3cfbce7375b 100644 --- a/src/BloomExe/web/controllers/PublishApi.cs +++ b/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. "