diff --git a/Wino.Mail.ViewModels.Tests/Collections/WinoMailCollectionTests.cs b/Wino.Mail.ViewModels.Tests/Collections/WinoMailCollectionTests.cs index 3c13879b..cdad8808 100644 --- a/Wino.Mail.ViewModels.Tests/Collections/WinoMailCollectionTests.cs +++ b/Wino.Mail.ViewModels.Tests/Collections/WinoMailCollectionTests.cs @@ -1,3 +1,7 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading.Tasks; using FluentAssertions; using Wino.Core.Domain.Entities.Mail; using Wino.Core.Domain.Enums; @@ -113,6 +117,57 @@ public class WinoMailCollectionTests threadItems.Should().Contain(item => item.ThreadId == "thread-c" && item.EmailCount == 2); } + [Fact] + public async Task AddRangeAsync_ShouldKeepGroupsAndItemsSortedDuringHighVolumeInitialization() + { + var sut = CreateCollection(); + var baseDate = new DateTime(2026, 4, 6, 12, 0, 0, DateTimeKind.Utc); + + var items = Enumerable.Range(0, 240) + .Select(index => + { + var dayOffset = index % 4; + var minuteOffset = 240 - index; + + return new MailItemViewModel(CreateMailCopy( + threadId: $"single-{index}", + creationDate: baseDate.AddDays(-dayOffset).AddMinutes(-minuteOffset))); + }) + .OrderByDescending(item => item.MailCopy.UniqueId) + .ToList(); + + await sut.AddRangeAsync(items, clearIdCache: true); + + var groups = new List<(DateTime Key, List Items)>(); + foreach (var group in sut.MailItems) + { + var groupItems = new List(); + foreach (var item in group) + { + groupItems.Add(item); + } + + groups.Add(((DateTime)group.Key, groupItems)); + } + + groups.Should().NotBeEmpty(); + + var orderedGroupKeys = groups.Select(group => group.Key).ToList(); + orderedGroupKeys.Should().BeInDescendingOrder(); + + foreach (var group in groups) + { + group.Items.Should().OnlyContain(item => item is MailItemViewModel); + + var creationDates = group.Items + .Cast() + .Select(item => item.MailCopy.CreationDate) + .ToList(); + + creationDates.Should().BeInDescendingOrder(); + } + } + [Fact] public async Task UpdateMailCopy_ShouldMergeExistingSingles_WhenThreadIdChangesToMatch() { @@ -155,6 +210,48 @@ public class WinoMailCollectionTests threadItem.GetContainingIds().Should().BeEquivalentTo([existing.UniqueId, incoming.UniqueId]); } + [Fact] + public async Task AddAsync_ShouldRemainConsistentUnderHighVolumeConcurrentAdds() + { + var sut = CreateCollection(); + var threadCount = 40; + var mailsPerThread = 25; + var baseDate = new DateTime(2026, 4, 6, 12, 0, 0, DateTimeKind.Utc); + + var mails = Enumerable.Range(0, threadCount) + .SelectMany(threadIndex => Enumerable.Range(0, mailsPerThread) + .Select(mailIndex => CreateMailCopy( + threadId: $"thread-{threadIndex}", + creationDate: baseDate.AddMinutes(-(threadIndex * mailsPerThread + mailIndex))))) + .OrderBy(_ => Guid.NewGuid()) + .ToList(); + + await Task.WhenAll(mails.Select(mail => Task.Run(() => sut.AddAsync(mail)))); + + var flattenedMailIds = FlattenMailItems(sut) + .Select(item => item.MailCopy.UniqueId) + .ToList(); + + flattenedMailIds.Should().HaveCount(threadCount * mailsPerThread); + flattenedMailIds.Should().OnlyHaveUniqueItems(); + flattenedMailIds.Should().BeEquivalentTo(mails.Select(mail => mail.UniqueId)); + + var topLevelItems = FlattenItems(sut); + topLevelItems.Should().HaveCount(threadCount); + topLevelItems.Should().OnlyContain(item => item is ThreadMailItemViewModel); + + foreach (var thread in topLevelItems.Cast()) + { + thread.EmailCount.Should().Be(mailsPerThread); + + var expectedIds = mails + .Where(mail => mail.ThreadId == thread.ThreadId) + .Select(mail => mail.UniqueId); + + thread.GetContainingIds().Should().BeEquivalentTo(expectedIds); + } + } + private static WinoMailCollection CreateCollection() => new() { CoreDispatcher = new ImmediateDispatcher() @@ -175,6 +272,28 @@ public class WinoMailCollectionTests return items; } + private static List FlattenMailItems(WinoMailCollection collection) + { + var items = new List(); + + foreach (var group in collection.MailItems) + { + foreach (var item in group) + { + if (item is MailItemViewModel mailItem) + { + items.Add(mailItem); + } + else if (item is ThreadMailItemViewModel threadItem) + { + items.AddRange(threadItem.ThreadEmails); + } + } + } + + return items; + } + private static MailCopy CreateMailCopy(string threadId, DateTime? creationDate = null) => new() { diff --git a/Wino.Mail.ViewModels/Collections/WinoMailCollection.cs b/Wino.Mail.ViewModels/Collections/WinoMailCollection.cs index 2d4f804a..f080a80e 100644 --- a/Wino.Mail.ViewModels/Collections/WinoMailCollection.cs +++ b/Wino.Mail.ViewModels/Collections/WinoMailCollection.cs @@ -2,6 +2,7 @@ using System.Collections.Concurrent; using System.Collections.Generic; using System.Linq; +using System.Threading; using System.Threading.Tasks; using CommunityToolkit.Mvvm.Collections; using CommunityToolkit.Mvvm.ComponentModel; @@ -42,6 +43,7 @@ public class WinoMailCollection : ObservableRecipient, IRecipient _mailItemSource = new ObservableGroupedCollection(); + private readonly SemaphoreSlim _mutationGate = new(1, 1); public ReadOnlyObservableGroupedCollection MailItems { get; } @@ -106,25 +108,28 @@ public class WinoMailCollection : ObservableRecipient, IRecipient + await RunSerializedAsync(async () => { - foreach (var group in _mailItemSource) + await ExecuteUIThread(() => { - foreach (var item in group) + foreach (var group in _mailItemSource) { - if (item is ThreadMailItemViewModel threadItem) + foreach (var item in group) { - threadItem.UnregisterThreadEmailPropertyChangedHandlers(); + if (item is ThreadMailItemViewModel threadItem) + { + threadItem.UnregisterThreadEmailPropertyChangedHandlers(); + } } } - } - _mailItemSource.Clear(); - MailCopyIdHashSet.Clear(); - _threadIdToItemsMap.Clear(); - _itemToGroupMap.Clear(); - _uniqueIdToMailItemMap.Clear(); - _uniqueIdToThreadMap.Clear(); + _mailItemSource.Clear(); + MailCopyIdHashSet.Clear(); + _threadIdToItemsMap.Clear(); + _itemToGroupMap.Clear(); + _uniqueIdToMailItemMap.Clear(); + _uniqueIdToThreadMap.Clear(); + }); }); } @@ -434,7 +439,10 @@ public class WinoMailCollection : ObservableRecipient, IRecipient RunSerializedAsync(() => AddInternalAsync(addedItem)); + + private async Task AddInternalAsync(MailCopy addedItem) { // First check if this is an update to an existing item if (MailCopyIdHashSet.ContainsKey(addedItem.UniqueId)) @@ -504,8 +512,8 @@ public class WinoMailCollection : ObservableRecipient, IRecipient /// Adds multiple emails to the collection. /// - public async Task AddRangeAsync(IEnumerable items, bool clearIdCache) + public Task AddRangeAsync(IEnumerable items, bool clearIdCache) + => RunSerializedAsync(() => AddRangeInternalAsync(items, clearIdCache)); + + private async Task AddRangeInternalAsync(IEnumerable items, bool clearIdCache) { if (clearIdCache) { @@ -717,17 +728,22 @@ public class WinoMailCollection : ObservableRecipient, IRecipient 0) { - // Pre-compute grouping on background thread to reduce UI thread work var groupedItems = await Task.Run(() => itemsToAdd .GroupBy(GetGroupingKey) - .ToDictionary(g => g.Key, g => g.ToList())).ConfigureAwait(false); + .OrderBy(group => group.Key, listComparer) + .Select(group => new + { + Key = group.Key, + Items = group.OrderBy(item => (object)item, listComparer).ToList() + }) + .ToList()).ConfigureAwait(false); await ExecuteUIThread(() => { - foreach (var kvp in groupedItems) + foreach (var groupedItem in groupedItems) { - var groupKey = kvp.Key; - var groupItems = kvp.Value; + var groupKey = groupedItem.Key; + var groupItems = groupedItem.Items; // Update caches first foreach (var item in groupItems) @@ -736,25 +752,14 @@ public class WinoMailCollection : ObservableRecipient, IRecipient(groupKey, groupItems); - _mailItemSource.AddGroup(groupKey, newGroup); + _mailItemSource.InsertItem(groupKey, listComparer, item, listComparer); - // Update item-to-group cache - foreach (var item in groupItems) + var targetGroup = _mailItemSource.FirstGroupByKeyOrDefault(groupKey); + if (targetGroup != null) { - _itemToGroupMap[item] = newGroup; - } - } - else - { - foreach (var item in groupItems) - { - existingGroup.Add(item); - _itemToGroupMap[item] = existingGroup; + _itemToGroupMap[item] = targetGroup; } } } @@ -814,7 +819,7 @@ public class WinoMailCollection : ObservableRecipient, IRecipient + return RunSerializedAsync(() => CoreDispatcher.ExecuteOnUIThread(() => { foreach (var group in _mailItemSource) { @@ -836,7 +841,7 @@ public class WinoMailCollection : ObservableRecipient, IRecipient @@ -845,16 +850,17 @@ public class WinoMailCollection : ObservableRecipient, IRecipientUpdated mail copy. /// public Task UpdateMailCopy(MailCopy updatedMailCopy, MailUpdateSource mailUpdateSource, MailCopyChangeFlags changedProperties = MailCopyChangeFlags.None) - { - var itemContainer = GetMailItemContainer(updatedMailCopy.UniqueId); - - if (itemContainer?.ItemViewModel == null) + => RunSerializedAsync(() => { - return Task.CompletedTask; - } + var itemContainer = GetMailItemContainer(updatedMailCopy.UniqueId); - return UpdateExistingItemAsync(itemContainer, updatedMailCopy, mailUpdateSource, changedProperties); - } + if (itemContainer?.ItemViewModel == null) + { + return Task.CompletedTask; + } + + return UpdateExistingItemAsync(itemContainer, updatedMailCopy, mailUpdateSource, changedProperties); + }); public MailItemViewModel GetFirst() => AllItems.ElementAtOrDefault(0); @@ -921,7 +927,10 @@ public class WinoMailCollection : ObservableRecipient, IRecipient RunSerializedAsync(() => RemoveInternalAsync(removeItem)); + + private async Task RemoveInternalAsync(MailCopy removeItem) { var itemContainer = GetMailItemContainer(removeItem.UniqueId); @@ -1121,6 +1130,20 @@ public class WinoMailCollection : ObservableRecipient, IRecipient CoreDispatcher?.ExecuteOnUIThread(action); + private async Task RunSerializedAsync(Func action) + { + await _mutationGate.WaitAsync().ConfigureAwait(false); + + try + { + await action().ConfigureAwait(false); + } + finally + { + _mutationGate.Release(); + } + } + public void Receive(SelectedItemsChangedMessage message) => _ = NotifySelectionChangesAsync(); private async Task NotifySelectionChangesAsync() diff --git a/Wino.Mail.ViewModels/MailListPageViewModel.cs b/Wino.Mail.ViewModels/MailListPageViewModel.cs index 17380f1a..ab274ff0 100644 --- a/Wino.Mail.ViewModels/MailListPageViewModel.cs +++ b/Wino.Mail.ViewModels/MailListPageViewModel.cs @@ -764,11 +764,17 @@ public partial class MailListPageViewModel : MailBaseViewModel, if (addedMail.AssignedAccount == null || addedMail.AssignedFolder == null) return; + bool hasLock = false; + try { + await listManipulationSemepahore.WaitAsync(); + hasLock = true; + if (ActiveFolder == null) return; - // At least one of the accounts we are listing must match with the account of the added mail. + // Re-evaluate folder membership after acquiring the semaphore so an add that was queued + // behind a folder re-initialization cannot land in the newly selected folder by mistake. if (!ActiveFolder.HandlingFolders.Any(a => a.MailAccountId == addedMail.AssignedAccount.Id)) return; // Fix for draft duplication: When a draft is created for reply/forward, it's first added as local draft. @@ -838,8 +844,6 @@ public partial class MailListPageViewModel : MailBaseViewModel, if (!IsMailMatchingLocalSearch(addedMail)) return; } - await listManipulationSemepahore.WaitAsync(); - // AddAsync already handles UI threading internally, no need to wrap it await MailCollection.AddAsync(addedMail); @@ -851,7 +855,10 @@ public partial class MailListPageViewModel : MailBaseViewModel, catch { } finally { - listManipulationSemepahore.Release(); + if (hasLock) + { + listManipulationSemepahore.Release(); + } } }