Make the publish upload modal on the publish-tool switcher too (BL-16654) - #8174
Conversation
|
(Claude Opus 5 from Andrew Polk's machine during preflight) Consulted Devin on 2026-08-06 up to commit Devin was run twice this session, once per commit. Against The re-review of Six informational items in total across the two runs, not mirrored here (low signal). CodeRabbit is switched off in this repo by |
6a97f74 to
ea033c4
Compare
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) <noreply@anthropic.com>
ea033c4 to
40ce746
Compare
|
(Claude Opus 5 from Andrew Polk's machine during preflight) Consulted Devin on 2026-08-06 up to commit Two more rounds since the note above, and the branch has been squashed to a single commit (so earlier commit shas on this PR no longer exist; the review threads survived and are all resolved). Round 3 ( Round 4 ( Worth knowing when reading Devin's page for this PR: it re-emits findings against the very commit that fixes them. Both round-3 findings, and the two findings from earlier rounds, still appear in the round-4 results even though each was already fixed or refuted in the commit analysed. Every finding across all four rounds now has a documented outcome on a resolved thread here — treat those threads, not Devin's page, as the current state. One finding has now been reported four times and refuted three: that dialog bookkeeping can unlock the tools mid-upload. It remains a false positive ( |
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson reviewed 9 files and all commit messages, and resolved 5 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on andrew-polk).
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", this reports the existing lock to the browser:
WorkspaceView.GetTabInfonow sendsnavigationLockedalongside the tab states, andPublishTabPanevetoes tool switching and greys out the other tools while it is set. Because that is the same flag C# already toggles in atry/finallyaround the upload, the publish tools unlock at the moment the upload really finishes or is cancelled — not when the browser guesses it has. The front end's ownisUploadingis cleared as soon as Cancel is clicked, and by any error line in the progress log, both of which would have unlocked too early.The Apps tool gets this for free and now shares one path, so its
appsBusystate and theonBusyChangecallback threaded down throughAppPublisherScreenare gone. As a side effect the lock now survives that screen remounting mid-action.SetTabsEnabledstays a single shared flag rather than becoming a count. That was a deliberate call: with the tool switcher locked as well, the user can no longer start a second publish operation while one is running, so the overlap the oldRabPublishApicomment worried about is unreachable. That comment is updated to say so. (Verified there is no programmatic navigation to the Publish tab — the onlyChangeTab(WorkspaceTab.publish)call site is a user click on the top bar, which is itself refused while the tabs are disabled — soPublishView.Activate()'s unconditional re-enable cannot fire mid-operation.)Deliberately not in scope, per decisions on the card: no tooltip/new localized string on the disabled buttons (matches how Apps behaves today), no re-entrancy guard on the upload endpoint, and no change to ePUB/BloomPUB/PDF preview generation (those abort cleanly when you leave, rather than needing to block).
Not verified in a running Bloom: the greying-out has not been watched during a real in-flight upload.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16654
This change is
Devin review
Devin review