Honor a minimum Bloom version in collection settings (BL-16690) - #8200
Honor a minimum Bloom version in collection settings (BL-16690)#8200andrew-polk wants to merge 10 commits into
Conversation
https://issues.bloomlibrary.org/youtrack/issue/BL-16690 In preparation for Cloud syncing, honor a MinimumBloomVersion element in the collection settings file. If the collection asks for a newer Bloom than the one running, we refuse to open it and offer the user two ways forward: upgrade Bloom, or open a different collection. There is no UI for setting the flag yet; it has to be added to the .bloomCollection file by hand, as the card specifies. - CollectionSettings reads MinimumBloomVersion, and writes it back out on Save when it is non-empty. The write-back matters: Save() rebuilds the file from scratch, so without it the hand-added flag would silently disappear the first time the user changed anything in Collection Settings. - New MinimumBloomVersionCheck does the gating. It reads just the one element out of the file rather than loading a whole CollectionSettings, since we have to answer the question before committing to build a ProjectContext. A missing, empty, or unparseable value imposes no restriction; locking someone out of their own collection over a typo would be worse than ignoring it. - The gate lives in Program.OpenProjectWindow, which is the single point all the ways of opening a collection funnel through (startup from MRU, the collection chooser, a command-line path, and switching collections mid-session). Every one of those callers already responds to a false return by putting up the collection chooser, so "open a different collection" needed no new plumbing. "Upgrade Bloom" opens the installers page and quits, which is what Bloom already does when a download needs a newer version. We compare major and minor only, matching Bloom's other two version gates (BookStorage's feature requirements and BookDownload's minVersion), so "6.5" is satisfied by any 6.5.x or later. On the card's open question about channels: the channel plays no part, because a version number means the same thing on every channel -- an alpha tester running ahead of the release is correctly let in. No XLF entries; the strings use LocalizationManager.GetString with English fallbacks, per the AGENTS.md allowance for experimental features. Tests: added MinimumBloomVersionCheckTests (20 cases: the version comparison table, reading the element, unparseable/absent files, and that the setting survives a Load/Save round trip). Those 20 pass, as do the 77 existing CollectionSettingsTests. Tests requiring output/browser (BookStorageTests, CollectionModelTests) cannot run in this worktree because the front end has never been built here; they fail on missing template directories both before and after this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
https://issues.bloomlibrary.org/youtrack/issue/BL-16690 Follow-ups to the minimum-version gate, from review: - Report both versions as major.minor, since that is what we actually compare. Saying "requires Bloom 6.5 ... you are running 6.4.900" invites the reader to compare 900 against 5, and echoing a declared "6.5.132" verbatim would claim we enforce a build number we ignore. Verified on screen: the dialog now reads 'The collection "Fake TC Books" requires Bloom 6.6 or greater. You are running Bloom 6.4.' - Put the Help menu's "opens an external website" icon (MUI Launch) on the Upgrade Bloom button, since that button leaves Bloom for the installers page. This meant teaching BloomMessageBox's buttons about an optional icon, on both the C# and Typescript sides; "launch" is the only value so far. It is passed as endIcon, the usual placement on a button for an external-link affordance (the Help menu's leading position is a menu-layout convention, not a semantic one). - Stop consuming StartupScreenManager's one-shot actions when clearing the splash screen out of the way of the dialog. CloseSplashScreen() invokes and then nulls DoLastOfAllAfterClosingSplashScreen, which is what brings the main window to the front; spending it at our gate means the main window never comes to the front when a collection finally does open. Added HideSplashScreenForDialog(), which does exactly what DoStartupAction already does for a task with ShouldHideSplashScreen -- the existing comments there say this case is deliberately different from closing. Also considered and rejected, with the developer, an in-app Velopack upgrade instead of the installers page. The blocker is not the lack of a browser (the dialog is a ReactDialog, so we do have one) but that Velopack reports progress solely through toasts, and ToastHost is mounted only in app/App.tsx, which does not exist before a collection opens. More decisively, Velopack only offers the latest build on the user's own channel, so a Release user blocked by a 6.5 requirement would be told they are up to date and left with no way forward. Tests: 101 pass (the 77 existing CollectionSettingsTests plus 24 in MinimumBloomVersionCheckTests, 4 of them new here for the major.minor reporting). eslint and prettier clean on the changed .tsx. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
https://issues.bloomlibrary.org/youtrack/issue/BL-16690 Move the Launch icon on the "Upgrade Bloom" button from the trailing position to the leading one, matching where the Help menu puts it on its "website" item. Uses BloomButton's own iconBeforeText prop rather than passing MUI's startIcon through, since that is the API BloomButton offers for exactly this. Verified by rendering BloomMessageBox with a throwaway vitest: the upgrade button gets a MuiButton-startIcon containing an svg, has no MuiButton-endIcon, and the button that asked for no icon has neither. The test was removed afterward rather than kept, since it asserts on MUI's internal class names and would be brittle. The C# side is a comment correction only, so the C# tests are unaffected; the compile is clean. They were not re-run because Bloom is currently running and holds the output exe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
https://issues.bloomlibrary.org/youtrack/issue/BL-16690 Two bugs in the new minimum-version gate, found by review (the second was found independently by both the local reviewer and Devin). ReopenProject discarded OpenProjectWindow's return value, so the comment I wrote claiming "every caller already responds to a false return by putting up the collection chooser" was simply untrue. The shell is already closed on that path, so a collection that refuses to open would leave Bloom running with no window at all -- invisible, killable only from Task Manager, and blocking the next launch via the single-instance mutex. This is reachable now that a collection can start refusing mid-session: a Team Collection member on a newer Bloom adds MinimumBloomVersion, it syncs down, and a collection-settings change triggers a reopen. It now falls back to the chooser like the other callers, and the comment is corrected. ReportCollectionNeedsNewerBloom returned void, so a caller could not tell "the user chose to upgrade and we have already called Application.Exit()" from "the user wants a different collection" -- both came back as false. A caller could therefore put the collection chooser up while Bloom was shutting down, starting a fresh modal message loop that could keep the process alive until the 20-second forced-shutdown watchdog fired. It now reports whether we are quitting, and OpenProjectWindow returns true in that case purely to stop callers starting new UI on the way out (commented as such, since it is not a real "opened"). Tests: 101 pass. The full C# suite has ~26 failures in this worktree, all in epub/audio/publish areas untouched by this change, with filesystem/temp-dir error signatures and a failure set that differs between runs -- environmental, not caused by this branch. Front-end suite green (524 passed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
https://issues.bloomlibrary.org/youtrack/issue/BL-16690 RunningBloomVersion fell back to 1.0.0 when the assembly version could not be read. That fallback was worse than no fallback: 1.0.0 fails every declared minimum, so the one case it "handled" would have locked the user out of every collection carrying the flag -- the exact opposite of what the rest of this class takes care to do, which is err toward letting people in. A loaded assembly always has a version, so per AGENTS.md's fail-fast guidance ("if you expect a foo to be defined, just use foo") the guard is gone and the reasoning is recorded in the comment. Raised by Devin as an informational finding. Tests: 101 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| null, | ||
| $"<strong>{header}</strong><br/><br/>{explanation}", | ||
| buttons, | ||
| MessageBoxIcon.Warning |
There was a problem hiding this comment.
[Devin] Bug: Warning symbol is missing from the “needs a newer Bloom” dialog
The dialog asks for a warning symbol, but none appears — it shows as a plain message.
Why: MessageBoxIcon.Warning and MessageBoxIcon.Exclamation are the same underlying value in .NET, and ToString() returns the first-declared name, so what actually reaches the display side is "exclamation". The Typescript only recognises "warning" and "asterisk", so it draws nothing. This is the same trap the existing comment in BloomMessageBox.tsx already notes for Information arriving as "Asterisk".
Confirmed: a screenshot of the running dialog during this preflight shows the text and both buttons but no warning triangle.
This is pre-existing and affects every BloomMessageBox.ShowWarning call in Bloom, not just this new dialog — so fixing it (teaching the Typescript side that "exclamation" means warning) would give warning icons back to warning dialogs app-wide. Because that is a visible change well beyond this card, it has been left for the developer to decide rather than changed here.
| Default = true, | ||
| }, | ||
| }; | ||
| var result = BloomMessageBox.Show( |
There was a problem hiding this comment.
[Devin] Investigate: Message box is a fixed 500x200, and this message is longer than usual
Bloom's message box is a fixed 500x200 with no auto-sizing. This message is a bold heading plus a two-clause sentence, plus two fairly wide buttons — more than most.
Checked during this preflight: with the collection "Fake TC Books" it renders correctly with room to spare. The residual risk is a much longer collection name or a translated message pushing it past the fixed height.
Left as is for now (the size is a property of the shared message box, not of this dialog), but worth a look at long names.
|
[Claude Opus 5] Consulted Devin on 2026-08-13 up to commit It raised one bug and six things to investigate; each is its own review thread above, so they can be answered one at a time. Two are already closed out: the collection staying at the top of the recently-used list is intended (and self-corrects once you open anything else), and the shutdown race after "Upgrade Bloom" was already fixed in The other five are waiting on a developer decision — the missing warning symbol on the dialog, a bare version number like Six further items were informational and are not mirrored here. One of those — a fallback version number that could have locked users out — was fixed in CI is green, and CodeRabbit is switched off for this repo in |
https://issues.bloomlibrary.org/youtrack/issue/BL-16690 Devin pointed out that the minimum-version check only covers the interactive ways of opening a collection; the command-line commands and BulkUploader build a collection directly and walk straight past it. Decision (Andrew): leave it that way, and write down why. Those tools have no user to read a dialog or choose a different collection, so gating them would convert a silent problem into a failed overnight job. The flag as it stands is a message to a person, and people are on the interactive paths. The comment also names the condition that would overturn the decision: if the flag ever has to protect the files themselves rather than warn someone, this is the spot to revisit. Tests: 101 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
https://issues.bloomlibrary.org/youtrack/issue/BL-16690 A regression in my own earlier fix, caught by Devin. To stop the collection chooser being raised during shutdown, OpenProjectWindow reports success when the user picks "Upgrade Bloom". But OpenCollection reads that as "the collection opened" and records it as the most-recently-used one. So a user who chose a too-new collection from the chooser, clicked Upgrade, and then thought better of the download would come back to the blocked collection on the next start instead of the one they had actually been working in. The shutdown case is now marked with a flag, so OpenCollection can tell a real open from "handled, don't start any more UI" and leaves the recently-used list alone. Tests: 101 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
https://issues.bloomlibrary.org/youtrack/issue/BL-16690 The collection folder for these cases was derived from the version string being tested, after trimming. Two of the cases differ only by surrounding whitespace, so both landed on the same folder and wrote over each other's settings file. They pass either way, but only because each writes immediately before it reads; it is exactly the sort of shared state that turns into a confusing intermittent failure later. Each case now names its own collection. Raised by Devin as an informational finding. Tests: 101 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
[Claude Opus 5] Consulted Devin again on 2026-08-13, up to commit Since the previous log, Devin found one genuine regression in a fix made earlier in this same review: reporting "success" on the upgrade path meant a collection Bloom had just refused was recorded as the last-used one, so abandoning the download and restarting landed you back on the blocked collection instead of the one you were working in. Fixed in It also caught two smaller things, both fixed: a fallback version number that would have blocked every flagged collection ( Three findings were assessed and closed with the reasoning on their threads: joining a Team Collection isn't gated (but opening the result is, so the user is still stopped where it matters); the blocked collection staying top of the recently-used list (intended, and self-correcting); and whether other startup tasks can run after we ask Bloom to quit (they can't, because the quit message stops the idle processing that drives them — noted on the thread that this one rests on reasoning rather than an observed run). Three were decided by the developer and recorded on their threads: the bare version number, the Team Collection sync ordering (now an open question for the designer on this card), and the command-line paths (left ungated, with the reasoning written into the code). Two threads remain open, both deliberately: the missing warning symbol and the fixed message-box size. Both turn out to affect dialogs across Bloom rather than just this one, so the developer is deciding them separately rather than having them changed here. CI is green. CodeRabbit is switched off for this repo in |
https://issues.bloomlibrary.org/youtrack/issue/BL-16690 Swap the two buttons and make upgrading the default. It now sits in the rightmost, primary position, drawn filled and holding the initial focus, with "Open a Different Collection" to its left as the secondary choice. Upgrading is the thing we actually want the user to do -- opening something else is the way out for someone who can't upgrade right now -- so the dialog should lead with it rather than making it the quieter option. Tests: 101 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
https://issues.bloomlibrary.org/youtrack/issue/BL-16690 The "Upgrade Bloom" button now opens the site's /downloads page. Note this only changes the button added by this card. The equivalent link behind the "this book needs a newer Bloom" message (web/controllers/AppApi.cs, the app/showDownloadsPage endpoint) still points at /installers, and BookDownload points at /download, so the three are not consistent. Left alone deliberately: changing them would alter messages beyond this card. Tests: 24 pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
superceded by #8206 |
2 participants
Honors a
MinimumBloomVersionelement in a collection's.bloomCollectionfile, in preparation for Cloud syncing. If the collection requires a newer Bloom than the one running, we refuse to open it and offer the user the two choices the card asks for: upgrade Bloom, or open a different collection.There is no UI for setting the flag yet — it has to be added to the file by hand, as the card specifies.
How it works
CollectionSettingsreadsMinimumBloomVersion, and writes it back out onSave()when non-empty. The write-back matters:Save()rebuilds the file from scratch, so without it a hand-added flag would silently disappear the first time the user changed anything in Collection Settings.MinimumBloomVersionCheck(new) does the gating. It reads just the one element rather than loading a wholeCollectionSettings, since we have to answer the question before committing to build aProjectContext. A missing, empty, or unparseable value imposes no restriction — locking someone out of their own collection over a typo would be worse than ignoring it.The gate lives in
Program.OpenProjectWindow, the single point all the ways of opening a collection funnel through (startup from MRU, the collection chooser, a command-line path, switching collections mid-session). Every one of those callers already responds to afalsereturn by putting up the collection chooser, so "open a different collection" needed no new plumbing.HideSplashScreenForDialog(new, inStartupScreenManager) clears the splash out of the way without consuming the one-shotDoLastOfAllAfterClosingSplashScreen, which is what brings the main window to the front.CloseSplashScreen()would have spent it at the gate, so the main window would never come forward when a collection finally opened.Decisions worth flagging for review
Major.minor comparison only, matching Bloom's other two version gates (
BookStorage's feature requirements andBookDownload'sminVersion). So6.5is satisfied by any 6.5.x or later, and a build number in the requirement is ignored. Both versions are reported as major.minor too, so we don't claim to enforce a build number we don't look at.Channel plays no part. A version number means the same thing on every channel, so an alpha tester running ahead of the release is correctly let in. (This was the card's open question.)
"Upgrade Bloom" opens the installers page and quits, rather than doing an in-app Velopack update. Velopack reports progress solely through toasts, and
ToastHostis mounted only inapp/App.tsx, which does not exist before a collection opens. More decisively, Velopack only offers the latest build on the user's own channel, so a Release user blocked by a 6.5 requirement would be told they are up to date and left with no way forward.No XLF entries — the strings use
LocalizationManager.GetStringwith English fallbacks, per theAGENTS.mdallowance for experimental features.Testing
24 new tests in
MinimumBloomVersionCheckTestscover the comparison table, reading the element, unparseable/absent files, that the setting survives aLoad/Saveround trip, and that the requirement is reported as major.minor. Verified in the running app that the dialog reads 'The collection "Fake TC Books" requires Bloom 6.6 or greater. You are running Bloom 6.4.'Note the base branch is Version6.4, not master.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16690
Devin review
This change is