Skip to content

Don't create watcher for "other" directory if there isn't one (BL-15838) #7660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JohnThomson wants to merge 1 commit into
base: Version6.3
Choose a base branch
from
+14 −4
Open

Don't create watcher for "other" directory if there isn't one (BL-15838) #7660

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/BloomExe/TeamCollection/FolderTeamCollection.cs
View file
Open in desktop
Copy link

@devin-ai-integration devin-ai-integration bot Feb 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Pre-existing bug: Dictionary key access without existence check

The color palette merging code at lines 689-702 iterates over repoColorPalettes.Keys and directly accesses localColorPalettes[key] at line 693 without verifying the key exists in localColorPalettes. If the repo's colorPalettes.json contains a palette key that the local file doesn't have, this will throw a KeyNotFoundException.

The MergeColorPaletteValues function (FolderTeamCollection.cs:616-632) does handle null/empty values gracefully, but it never gets called if the dictionary access throws first.

This is pre-existing code, not introduced by this PR, but worth noting as it could cause sync failures when palette files diverge.

(Refers to lines 689-702)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
Expand Up @@ -825,10 +825,20 @@ protected internal override void StartMonitoring()
// Begin watching.
_booksWatcher.EnableRaisingEvents = true;

_otherWatcher = new FileSystemWatcherWrapper(Path.Combine(_repoFolderPath, "Other"));
_otherWatcher.NotifyFilter = NotifyFilters.LastWrite;
_otherWatcher.DebounceChanged(OnCollectionFilesChanged, kDebouncePeriodInMs);
_otherWatcher.EnableRaisingEvents = true;
var otherFilesDirPath = Path.Combine(_repoFolderPath, "Other");
// If it doesn't exist we can't watch it. Rather bizarre since we normally create
// it if it doesn't exist as part of syncing. But BL-15838 seems to have been
// caused by not checking. If we can't set it up, unfortunately we won't find
// out immediately if some remote user modifies something in the collection.
// But we should find out on the next startup, and from then on we'll be able to
// monitor it, so I don't think it's very serious.
if (Directory.Exists(otherFilesDirPath))
{
_otherWatcher = new FileSystemWatcherWrapper(otherFilesDirPath);
_otherWatcher.NotifyFilter = NotifyFilters.LastWrite;
_otherWatcher.DebounceChanged(OnCollectionFilesChanged, kDebouncePeriodInMs);
_otherWatcher.EnableRaisingEvents = true;
}
}

private void OnDeleted(object sender, FileSystemEventArgs e)
Expand Down