Keep PublishHelper.InPublishTab in step with the active tab (BL-16174) - #8183
Keep PublishHelper.InPublishTab in step with the active tab (BL-16174)#8183andrew-polk wants to merge 3 commits into
Conversation
…ailed 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) <[email protected]>
Two documentation/robustness points Devin raised on #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) <[email protected]>
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) <[email protected]>
|
[Claude Opus 5 (1M context)] Consulted Devin on 2026-08-10, up to commit Devin ran three times, once per commit on this branch. Every run came back with no bugs and nothing flagged for investigation — the seven items on the final run are all Informational, so there are no findings mirrored as review threads here. Two of the informational points were worth acting on, and did become commits:
The rest were noted and deliberately not acted on; they are in the preflight report for the developer, chiefly the observation that CI ( |
Switching from the Edit tab to Publish and asking for a BloomPUB preview sometimes threw
InvalidOperationException: Should not be creating bloom book while not in publish tabfromPublishHelper's constructor: the "Bloom had a problem" dialog appeared, the preview stayedblank, and the Collection and Edit tabs were left greyed out until Bloom was restarted.
Cause
Bloom kept two records of "we are in the Publish tab", updated at different times:
WorkspaceTabSelection.ActiveTab— the authoritative one, assigned inWorkspaceView.ChangeTab'sPostponedWork.PublishHelper.InPublishTab— the static flag the book-staging code actually checks, maintained separately byPublishView.Activate()/Deactivate()from theSelectedTabChangedEventsubscriber.The second is always updated later — after
ActiveTabhas already changed, and after however manyother subscribers have run. The log attached to the card shows the two disagreeing at the moment of
failure:
PublishApi.MakeBloompubPreview's own guard (ActiveTab != publish→ abort) let the requestthrough, so
ActiveTabsaid "publish";PublishHelper's guard fired, so the flag still said "not publish".The same desync explains the second symptom.
PublishView.Activate()is also what callsSetTabsEnabled(true), the safety net that releases the Edit tab's save lock — a skipped activationloses that too.
Fix
WorkspaceTabSelection.ActiveTabbecomes a property whose setter also setsPublishHelper.InPublishTab. One assignment, no window, one owner. This also clears the flag when a new collection'sWorkspaceTabSelectioninitializes to the Collection tab — the static previously survived a collection switch.PublishViewno longer writes the flag (comments at both sites point at the new owner).PublishView._isActiveis now set beforeActivate()/Deactivate(), so a throw inside either cannot leave the latch stale and silently skip the next activation. Exceptions still propagate; nothing is swallowed.Tests
New
src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs— 4 tests covering both directions andthe stale-static-across-collections case, each with a sanity check on the starting state.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16174
Devin review
This change is