Fix collection whose name ends with a period breaking Team Collections (BL-16679) by StephenMcConnel · Pull Request #8204 · BloomBooks/BloomDesktop

StephenMcConnel · 2026-08-13T22:41:06Z

Fixes the two problems reported in BL-16679, which both trace to one trigger: the reported collection's name ends with a period. Windows silently drops trailing periods and spaces when it creates a folder, so the folder lost the period while the .bloomCollection file kept it (…نهاية\…نهاية..bloomCollection).

1. The path broke FileSystemWatcher, so the collection wouldn't open

The path Bloom remembered still had the period on the folder component. Ordinary File/Directory APIs hide that — they normalize the path first — but FileSystemWatcher does not: .NET prefixes a path ending in a period or space with \\?\, which turns Windows normalization off, so StartRaisingEvents threw Error reading the … directory. That killed TeamCollection.StartMonitoring; BookCollection watches the collection folder the same way.

MiscUtils.GetPathAsOnDisk normalizes a collection path, applied where a collection enters Bloom (Program.OpenProjectWindow) and before it goes to the MRU list, so a bad path already saved by an older Bloom heals.
CollectionSettings.GetPathForNewSettings no longer creates such a path in the first place.

(An existing guard in TeamCollectionManager aimed at this could never fire — Directory.Exists returns true for the dotted path whenever the real folder exists — and had already been removed.)

2. That failure left Bloom as a zombie process

The exception escaped the ProjectContext constructor, which had already locked the .bloomCollection file. Program assigns _projectContext only after the constructor returns, so nobody could dispose the half-built context: it kept the collection file locked, along with the project-scoped objects it had built — the Shell and its WebView2 browsers, the file watchers. (BloomServer itself is application-level, resolved from the parent container, so it is not the context's to shut down — an earlier version of this description said otherwise.) Bloom could then neither reopen the collection (being used by another process) nor exit — ProgramExit.Exit(), and its 20-second force-quit net, is never reached, so it sat in a windowless Application.Run().

The constructor now disposes itself on failure, and Dispose tolerates a partly-built context.

3. Three places assumed the settings file is named after its folder

False for this collection — and for any collection whose folder was renamed, of which this repo's own dev machines have several.

ProjectContext.GetCollectionSettings already intended to fall back to whatever .bloomCollection is in the folder, but passed the file path where TryGetSettingsFilePath wants the folder, so Directory.EnumerateFiles threw DirectoryNotFoundException instead of repairing anything. This is what aborted the Team Collection startup sync (via OkToEditCollectionSettings), so no books were copied in.
SpreadsheetExportCommand now looks for the settings file that is really in the folder instead of assuming the name. It keeps its old tolerance for a book that isn't inside a collection at all (falling back to default settings) rather than failing.
The collection chooser's default-directory scan silently omitted such collections.

Testing

20 new test cases across MiscUtilsTests, CollectionSettingsTests, ProjectContextTests (new file) and TeamCollectionManagerTests (previously an empty class). The OkToEditCollectionSettings test was written before the fix and confirmed to fail with the exact reported exception. Full C# suite green (3110 passed).

Verified by hand on the real collection: joining the Team Collection now works.

Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16679

Devin review

This change is

…s (BL-16679) Windows silently drops trailing periods and spaces when it creates a folder, so a collection named "Some Name." gets a folder without the period while its settings file keeps it. Two independent defects followed from that. 1. The path Bloom remembered still had the period on the folder component. Ordinary File/Directory APIs hide that, because they normalize the path first, but FileSystemWatcher does not: .NET prefixes a path ending in a period or space with \\?\, which turns Windows normalization off, so StartRaisingEvents threw "Error reading the ... directory". That killed TeamCollection.StartMonitoring, and BookCollection watches the collection folder the same way. Normalize the path where a collection enters Bloom (and before it goes to the MRU list, so an already-saved bad path heals), and stop creating such paths in GetPathForNewSettings. 2. That exception escaped the ProjectContext constructor, which had already locked the .bloomCollection file. Program assigns _projectContext only after the constructor returns, so nobody could dispose the half-built context: it kept the collection file locked, along with BloomServer, its port and any WebView2. Bloom could then neither reopen the collection ("being used by another process") nor exit -- it lingered as a zombie process. The constructor now disposes itself on failure, and Dispose tolerates a partly-built context. Also fixed two places that assumed the settings file is named after its folder, which is false for such a collection (and for any collection whose folder was renamed): - ProjectContext.GetCollectionSettings already meant to fall back to whatever .bloomCollection file is in the folder, but passed the file path where TryGetSettingsFilePath wants the folder, so it threw DirectoryNotFoundException instead of repairing anything. This is what aborted the Team Collection startup sync, so no books were copied in. SpreadsheetExportCommand now uses that repaired lookup too. - The collection chooser's default-directory scan silently omitted such collections. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…lectionZombied

- Correcting the remembered path left the old spelling in the recently-used list, which matches paths as exact strings, so the collection was listed twice in the Open/Create Collections dialog, indefinitely. Remove the old spelling when we replace it. - ProjectContext.GetCollectionSettings, now that it looks in the folder rather than at a path that could never be a folder, reached "new CollectionSettings(missing path)" when a folder holds no .bloomCollection at all -- and that constructor treats a missing file as "make me a new collection here", so we would have silently written a default settings file into the folder (e.g. under a book exported to a spreadsheet from outside any collection). Throw a clear FileNotFoundException instead, which is what this method did, less legibly, before. - The ProjectContext constructor's cleanup comment claimed it shuts down BloomServer and its port. It does not: BloomServer is application-level, resolved from the parent container, so disposing our scope does not touch it. Say what the cleanup really does -- release the collection lock and dispose the project-scoped objects (the Shell and its WebView2 browsers, the file watchers). - The new-collection name check trimmed trailing periods only for its emptiness test, so "templates." passed validation while the folder actually created was the reserved "templates". Judge the name we will really use for all of the checks. - Note in GetPathAsOnDisk's remarks what it does not do (fix case, expand 8.3 names) and that it is a no-op on Linux, where such names are legal and the watcher copes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

- Spreadsheet export of a book that is not inside a collection used to carry on with default settings; routing it through ProjectContext.GetCollectionSettings made it fail instead, because that method now (rightly) refuses to invent a collection. Look for the real settings file directly, and fall back to the derived name -- and so to the old behaviour -- when the folder holds no collection at all. - Healing the remembered path only covered collections opened through OpenCollection. A normal launch opens the most recent collection without touching that list, so the old spelling stayed in it, and now that the folder scan finds the same collection under its real name, the Open/Create dialog listed it twice. Normalize the remembered paths where the dialog reads them, which fixes the display whatever is stored. - Looking inside every folder to find a collection file, rather than testing for one file name, means the scan can now throw: one unreadable or just-deleted folder would cost the user the whole Open/Create dialog. Skip such a folder and log it. - ProjectContext.Dispose claimed to be safe to call twice, but CollectionLock.Unlock was not idempotent: the second call threw inside its own try, which a DEBUG build rethrows. Return early when there is nothing to unlock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Judging the trimmed name for invalid file-name characters lost coverage: tab, newline and the other control characters are both invalid in a file name and removed by Trim(), so a pasted name ending in one was reported as fine and then failed when Bloom tried to create the folder -- which is still built from the raw text. Check the raw text for invalid characters, and keep the trimmed name for the emptiness and "templates" checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Creating a collection no longer asks Windows for a folder name it will silently alter, but renaming one still did: RenameCollection moved the folder to the caller's name and then named the settings file after that same name, so renaming to "Foo." produced the folder Foo holding Foo..bloomCollection -- the mismatch this whole ticket is about, reachable from `Bloom --rename`. The rest of the fix means that state is no longer fatal, but there is no reason to keep creating it. Trim the destination name the same way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

StephenMcConnel · 2026-08-13T23:42:11Z

[Claude Opus 5 from Steve McConnel's machine during preflight] Consulted Devin on 2026-08-13, five rounds, up to commit 6eac1460c.

It found real problems each time, and two of them were regressions from fixes for the earlier ones — worth knowing if you are deciding how much to trust a single pass. Nine findings were mirrored as review threads above, each with a reply saying what happened to it: seven fixed, one assessed as not applicable (the tests it worried about can't run on Linux — both projects target net8.0-windows), and one left open for a developer decision, because it arrived after this run had spent its budget of automatic fix rounds:

Open: the collection-file lock is taken before the settings-path repair.

Devin also raised 13 informational items on the final round; those are not mirrored (low signal), but they were read. CI (pr-automation) is green on every commit, and CodeRabbit is switched off for this repo in .coderabbit.yml, so it is not a silent reviewer.

Note for anyone reading the raw Devin output: each re-review re-reported the earlier rounds' findings alongside the new ones, quoting code that had already changed. Every one was checked against the committed file before being acted on or dismissed.

…lectionZombied

The collection file is locked while a collection is open so its folder can't be renamed or moved out from under us. That lock was taken on the path as handed in, before the code that works out which .bloomCollection is really in the folder -- so in the mismatch case this ticket is about, the lock was aimed at a file that does not exist: CollectionLock.Lock swallows that in a release build, silently losing the protection, and rethrows it in a DEBUG one, which aborts opening the collection before the repair runs. Settle the real path first, then lock it and build everything else from it. The lookup that GetCollectionSettings was doing privately is now a named helper they share, and it still returns the path it was given when there is no collection to find, so callers keep the behaviour they had for that case -- GetCollectionSettings still reports it, and the lock still tolerates it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

StephenMcConnel · 2026-08-17T15:55:01Z

[Claude Opus 5 from Steve McConnel's machine during preflight] Consultation log addendum, up to commit 62aa76cac.

The one finding that had been left open — the collection lock being taken before the settings-path repair — was decided by the developer in favour of fixing it, and is fixed in 62aa76cac; that thread now records the decision and is resolved. All nine mirrored findings are therefore closed.

Devin has no review for this commit. Its job errored immediately, three separate triggers running: the push, a re-run of pr-automation, and loading the review page directly. No error message and no findings pass in any of them, and the failures are marked retryable on their side, so this looks like a Devin-side problem rather than anything about the diff. Its last completed review is 6eac1460c; the only change since is the lock fix it asked for, plus a merge of master. Re-running preflight later would pick up a real result.

CI is green on 62aa76cac, and the C# suite is green at 3128 tests after re-merging master.

… (BL-16679) The trailing-period spelling was only corrected in Program's desktop startup path, so a collection whose name ends with a period still broke when Bloom was driven from the command line -- creating artifacts, bulk upload, font analytics -- because those construct a ProjectContext directly. Nothing noticed the bad spelling on the way in: file APIs open it happily, since Windows normalizes for them. It only surfaced where it did originally, in the FileSystemWatchers that get the collection folder. Normalize in GetRealSettingsPath instead, which is the one place every ProjectContext passes through, so no entry point has to remember to do it. Also: renaming a collection by only adding a trailing period asks for the folder we already have, and since we now trim the destination that came out as "there is already a directory with the new name", which stops Bloom reopening. Treat it as the no-op it is and return the existing settings path. Compared exactly, so changing a name's letter case is still a real rename. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

StephenMcConnel · 2026-08-17T17:28:18Z

[Claude Opus 5 from Steve McConnel's machine during preflight] Consulted Devin on 2026-08-17, up to commit 5a11b5e05. Re-review clean.

The review of 62aa76cac (which had errored on three triggers earlier — transient, it went through on a re-trigger) raised two genuinely new things, both now fixed in 5a11b5e05 and both mirrored above with their outcomes and resolved:

the normalization never reached the command-line entry points — worth reading, it is the sharper form of an informational item from round 1 that I had wrongly judged cosmetic;
renaming a collection to add only a trailing period reported "there is already a directory with the new name".

The review of 5a11b5e05 then found nothing new. It lists eleven findings, but every one carries an older commit in its own head_sha field (62aa76cac, 43ac9799a, dd5d7e66c, c1a7cde4a, or none) — they are carry-forwards of findings already fixed, not observations about this commit. That per-finding head_sha is the field to check: the raw count on the job says nothing.

Eleven findings have been mirrored across the whole review, and all eleven threads are resolved with a documented outcome — nine fixed, one assessed as not applicable (the tests cannot run on Linux; both projects target net8.0-windows), and one that went to the developer for a decision and was then fixed at their direction.

CI green on 5a11b5e05; C# suite 3130 passed, 0 failed.

Three test cases from the developer's working tree: "Foo .", "Foo . . " and "Foo . . ." should all give the folder "Foo". They pass as written -- GetPathForNewSettings trims any run of trailing periods and spaces, not just one kind -- so this pins that down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…lectionZombied

StephenMcConnel · 2026-08-17T19:05:55Z

[Claude Opus 5 from Steve McConnel's machine during preflight] Consulted Devin on 2026-08-17, up to commit 86eb72f6f. Re-review clean.

Nothing new. The job lists eleven findings, but every one carries an older commit in its own head_sha field (or none at all) — they are carry-forwards of findings already fixed, not observations about this commit. Zero carry 86eb72f6f.

This run existed only to close two mechanical gaps that stopped promotion: three test cases sitting uncommitted in the working tree (now 560ca4713 — they pass), and origin/master having moved on (now merged as 86eb72f6f). No code behaviour changed.

CI green on 86eb72f6f; C# suite 3133 passed, 0 failed; all eleven mirrored review threads remain resolved.

StephenMcConnel and others added 3 commits August 13, 2026 16:36

Merge remote-tracking branch 'origin/master' into BL-16679-BadTeamCol…

c1a7cde

…lectionZombied