feat: Sort memories chronologically #436
Conversation
jpg0
force-pushed
the
feat/sort-memories-chronologically
branch
2 times, most recently
from
a1ce2a5 to
33e2f5d
Compare
|
Anyone know why this was never merged? Now it has conflicts. |
|
No; if you want to merge I'm happy to resolve the conflicts though. |
Sure, if you don't mind. Seems like a nice addition, and sorry for the hassle. |
jpg0
force-pushed
the
feat/sort-memories-chronologically
branch
from
33e2f5d to
d927278
Compare
WalkthroughRefactors pool APIs to return IList, makes CachingApiAssetsPool deterministic and list-based, adds LogicPoolAdapter and PoolConfiguration to compose pools and ImmichApi, updates PooledImmichFrameLogic to inherit LogicPoolAdapter and change its constructor, adjusts DI and tests, and ensures MemoryAssetsPool returns memories in chronological order. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Logic as LogicPoolAdapter / PooledImmichFrameLogic
participant Pool as IAssetPool
participant Api as ImmichApi
participant Webhook as WebhookHelper
rect rgba(227,242,253,0.6)
note over Client,Logic: Retrieve next asset
Client->>Logic: GetNextAsset()
Logic->>Pool: GetAssets(1)
Pool-->>Logic: IList<Asset>
Logic-->>Client: Asset? (first or null)
end
rect rgba(232,245,233,0.6)
note over Client,Logic: Retrieve multiple assets
Client->>Logic: GetAssets()
Logic->>Pool: GetAssets(25)
Pool-->>Logic: IList<Asset>
Logic-->>Client: collection
end
rect rgba(255,243,224,0.6)
note over Client,Logic: Fetch image bytes
Client->>Logic: GetImage(assetId)
Logic->>Api: ViewAssetAsync(id, "", Preview)
Api-->>Logic: HttpResponse (stream + headers)
alt Found
Logic-->>Client: (fileName, contentType, stream)
else Not found
Logic-->>Client: throws AssetNotFoundException
end
end
rect rgba(252,228,236,0.6)
note over Client,Logic: Send webhook (adapter)
Client->>Logic: SendWebhookNotification(notification)
Logic->>Webhook: Send(webhookUrl, notification)
Webhook-->>Logic: Ack
Logic-->>Client: Completed
end
sequenceDiagram
autonumber
participant Caller as Logic (GetAssets)
participant Pool as CachingApiAssetsPool
Caller->>Pool: GetAssets(requested)
alt requested == 0 or none available
Pool-->>Caller: []
else deterministic sequence
Pool->>Pool: compute slice using _next (tail/head)
Pool-->>Caller: IList<Asset> (slice), advance/reset _next
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
14-36: Pagination bug: loop will prematurely stop after the first pageYou compare
total(overall matches) withbatchSize; iftotal > batchSize, the conditiontotal == batchSizeis false and you stop after page 1. UseItems.Count == batchSizeor the API’sNextPageindicator instead.Apply one of these fixes:
Option A — continue while page returns a full batch:
- int total; - do + int returned; + do { var metadataBody = new MetadataSearchDto { Page = page, Size = batchSize, PersonIds = [personId], Type = AssetTypeEnum.IMAGE, WithExif = true, WithPeople = true }; var personInfo = await immichApi.SearchAssetsAsync(metadataBody, ct); - - total = personInfo.Assets.Total; - - personAssets.AddRange(personInfo.Assets.Items); + var items = personInfo.Assets.Items; + returned = items.Count; + personAssets.AddRange(items); page++; - } while (total == batchSize); + } while (returned == batchSize && !ct.IsCancellationRequested);Option B — use NextPage if available:
- int total; - do + int? nextPage; + do { var metadataBody = new MetadataSearchDto { Page = page, Size = batchSize, PersonIds = [personId], Type = AssetTypeEnum.IMAGE, WithExif = true, WithPeople = true }; var personInfo = await immichApi.SearchAssetsAsync(metadataBody, ct); - - total = personInfo.Assets.Total; - + nextPage = personInfo.Assets.NextPage; personAssets.AddRange(personInfo.Assets.Items); - page++; - } while (total == batchSize); + page = nextPage ?? (page + 1); + } while (nextPage.HasValue && !ct.IsCancellationRequested);Also consider de-duplicating across people to avoid the same asset appearing multiple times:
- return personAssets; + return personAssets + .GroupBy(a => a.Id) + .Select(g => g.First()) + .ToList();ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (3)
16-30: Remove redundant initialization logic.The constructor duplicates initialization that already happens in
PoolConfiguration:
- Line 20 assigns
_immichApifrompoolConfiguration.ImmichApi, but line 26 overwrites it with a new instance- Lines 28-29 create new
_apiCacheand_poolinstances, but the base class already usespoolConfiguration.AssetPoolandpoolConfiguration.ApiCacheThis defeats the purpose of introducing
PoolConfigurationand causes unnecessary object creation.Apply this diff to remove redundant initialization:
public PooledImmichFrameLogic(IGeneralSettings generalSettings, IAccountSettings accountSettings, PoolConfiguration poolConfiguration, IHttpClientFactory httpClientFactory) : base(poolConfiguration.AssetPool, poolConfiguration.ImmichApi, generalSettings.Webhook) { _generalSettings = generalSettings; - _immichApi = poolConfiguration.ImmichApi; - - var httpClient = httpClientFactory.CreateClient("ImmichApiAccountClient"); + _immichApi = poolConfiguration.ImmichApi; + _apiCache = poolConfiguration.ApiCache; + _pool = poolConfiguration.AssetPool; AccountSettings = accountSettings; - - httpClient.UseApiKey(accountSettings.ApiKey); - _immichApi = new ImmichApi(accountSettings.ImmichServerUrl, httpClient); - - _apiCache = new ApiCache(RefreshInterval(generalSettings.RefreshAlbumPeopleInterval)); - _pool = BuildPool(accountSettings); }
33-60: Remove obsolete methods.Both
RefreshIntervalandBuildPoolare no longer needed:
RefreshIntervalwas only used in the removed cache initialization (line 28)BuildPoolduplicatesPoolConfiguration.BuildPool(lines 24-46 in PoolConfiguration.cs)Since
PoolConfigurationnow handles pool building, these methods should be removed.Apply this diff:
- private static TimeSpan RefreshInterval(int hours) - => hours > 0 ? TimeSpan.FromHours(hours) : TimeSpan.FromMilliseconds(1); public IAccountSettings AccountSettings { get; } - - - private IAssetPool BuildPool(IAccountSettings accountSettings) - { - if (!accountSettings.ShowFavorites && !accountSettings.ShowMemories && !accountSettings.Albums.Any() && !accountSettings.People.Any()) - { - return new AllAssetsPool(_apiCache, _immichApi, accountSettings); - } - - var pools = new List<IAssetPool>(); - - if (accountSettings.ShowFavorites) - pools.Add(new FavoriteAssetsPool(_apiCache, _immichApi, accountSettings)); - - if (accountSettings.ShowMemories) - pools.Add(new MemoryAssetsPool(_immichApi, accountSettings)); - - if (accountSettings.Albums.Any()) - pools.Add(new AlbumAssetsPool(_apiCache, _immichApi, accountSettings)); - - if (accountSettings.People.Any()) - pools.Add(new PersonAssetsPool(_apiCache, _immichApi, accountSettings)); - - return new MultiAssetPool(pools); - }
62-76: Remove methods that duplicate base class implementation.These methods (
GetNextAsset,GetAssets,GetAssetInfoById,GetAlbumInfoById,GetTotalAssets) are already implemented identically in theLogicPoolAdapterbase class (lines 10-22 in LogicPoolAdapter.cs). After fixing the constructor to usepoolConfiguration.AssetPoolinstead of creating a new_pool, these overrides become redundant.Apply this diff:
public IAccountSettings AccountSettings { get; } - - public async Task<AssetResponseDto?> GetNextAsset() - { - return (await _pool.GetAssets(1)).FirstOrDefault(); - } - - public Task<IEnumerable<AssetResponseDto>> GetAssets() - { - return _pool.GetAssets(25); - } - - public Task<AssetResponseDto> GetAssetInfoById(Guid assetId) => _immichApi.GetAssetInfoAsync(assetId, null); - - public async Task<IEnumerable<AlbumResponseDto>> GetAlbumInfoById(Guid assetId) => await _immichApi.GetAllAlbumsAsync(assetId, null); - - public Task<long> GetTotalAssets() => _pool.GetAssetCount(); public async Task<(string fileName, string ContentType, Stream fileStream)> GetImage(Guid id)
🧹 Nitpick comments (8)
ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj (1)
29-31: Avoid referencing a test project from another test projectReferencing ImmichFrame.Core.Tests from ImmichFrame.WebApi.Tests can cause:
- Duplicate test discovery and analyzer conflicts
- Tight coupling and circular dependency risks
- Longer builds
Prefer:
- Referencing the production project(s) only; and/or
- Extracting shared fixtures/mocks into a TestCommon library referenced by both test projects.
If you need specific helpers from Core.Tests, I can sketch a small ImmichFrame.TestCommon project to host them and update references accordingly.
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
1-1: Minor: file/class naming mismatchFile is PeopleAssetsPool.cs but class is PersonAssetsPool. Consider aligning names for clarity.
ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (2)
15-16: Confirm desired order: ascending vs descending by MemoryAtOrdering with
OrderBy(m => m.MemoryAt)yields oldest memories first (e.g., 3y ago → 2y → 1y). If the intent is “one year ago, two years ago, …”, sort descending:-foreach (var memory in memories.OrderBy(m => m.MemoryAt)) +foreach (var memory in memories.OrderByDescending(m => m.MemoryAt))
8-8: Daily cache: consider UTC to avoid TZ edge casesUsing local time for both the API date and cache expiration can drift around DST/offsets. Consider DateTime.UtcNow and AbsoluteExpiration at UTC midnight to reduce boundary surprises.
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (3)
8-9: Potential race on _next with concurrent GetAssetsGetAssets mutates
_nextwithout synchronization; concurrent calls can skip/duplicate assets. Guard updates and wrap-around with a lock.Apply this diff:
- private int _next; //next asset to return + private int _next; // next asset to return + private readonly object _sync = new();And update GetAssets as below.
13-39: Make GetAssets thread-safe, handle negatives, and simplify slicing
- Protect
_nextupdates- Treat non-positive
requestedas empty- Use Skip instead of TakeLast
- public async Task<IEnumerable<AssetResponseDto>> GetAssets(int requested, CancellationToken ct = default) + public async Task<IEnumerable<AssetResponseDto>> GetAssets(int requested, CancellationToken ct = default) { - if (requested == 0) + if (requested <= 0) { return new List<AssetResponseDto>(); } var all = await AllAssets(ct); if (all.Count < requested) { requested = all.Count; //limit request to what we have } - - var tail = all.TakeLast(all.Count - _next).ToList(); - - if (tail.Count >= requested) - { - _next += requested; - return tail.Take(requested); - } - - // not enough left in tail; need to read head too - var overrun = requested - tail.Count; - _next = overrun; - return tail.Concat(all.Take(overrun)); + + if (requested == 0) + return Array.Empty<AssetResponseDto>(); + + int start, firstCount, secondCount; + lock (_sync) + { + if (_next >= all.Count) + _next %= Math.Max(1, all.Count); + + start = _next; + firstCount = Math.Min(requested, all.Count - start); + secondCount = requested - firstCount; + _next = (start + requested) % (all.Count == 0 ? 1 : all.Count); + } + + var head = all.Skip(start).Take(firstCount); + return secondCount > 0 + ? head.Concat(all.Take(secondCount)) + : head; }
47-73: Null-safety in account filters (optional)Some asset sources may not include ExifInfo. Guard against nulls to avoid NREs.
- if (takenBefore.HasValue) - { - assets = assets.Where(x => x.ExifInfo.DateTimeOriginal <= takenBefore); - } + if (takenBefore.HasValue) + assets = assets.Where(x => x.ExifInfo != null && x.ExifInfo.DateTimeOriginal <= takenBefore); ... - if (takenAfter.HasValue) - { - assets = assets.Where(x => x.ExifInfo.DateTimeOriginal >= takenAfter); - } + if (takenAfter.HasValue) + assets = assets.Where(x => x.ExifInfo != null && x.ExifInfo.DateTimeOriginal >= takenAfter); ... - if (accountSettings.Rating is int rating) - { - assets = assets.Where(x => x.ExifInfo.Rating == rating); - } + if (accountSettings.Rating is int rating) + assets = assets.Where(x => x.ExifInfo != null && x.ExifInfo.Rating == rating);ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (1)
177-193: Catch‑all 200 may mask unintended HTTP callsReturning 200
{}for any other host requests can hide regressions. Prefer failing fast with 404 for unknown paths so tests surface unexpected traffic.- .ReturnsAsync(() => new HttpResponseMessage - { - StatusCode = HttpStatusCode.OK, - Content = new StringContent("{}", System.Text.Encoding.UTF8, "application/json") - }); + .ReturnsAsync(() => new HttpResponseMessage + { + StatusCode = HttpStatusCode.NotFound, + Content = new StringContent("{\"error\":\"unhandled mock path\"}", System.Text.Encoding.UTF8, "application/json") + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs(1 hunks)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs(12 hunks)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs(1 hunks)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs(1 hunks)ImmichFrame.Core/Logic/LogicPoolAdapter.cs(1 hunks)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs(2 hunks)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs(2 hunks)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/PoolConfiguration.cs(1 hunks)ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs(3 hunks)ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs(2 hunks)ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj(1 hunks)ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs(3 hunks)ImmichFrame.WebApi/Program.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.467Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
📚 Learning: 2025-10-09T16:12:49.467Z
Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.467Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.csImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.csImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs
🧬 Code graph analysis (15)
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (2)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (6)
CachingApiAssetsPool(6-76)Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(44-44)
ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (5)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
Task(26-26)AssetResponseDto(41-41)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (1)
Task(32-35)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(44-44)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
Task(8-36)
ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (4)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
Task(26-26)AssetResponseDto(41-41)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(42-42)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
Task(8-39)
ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (3)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (6)
CachingApiAssetsPool(6-76)Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
Task(8-36)ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
Task(10-35)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (4)
ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (1)
Task(32-35)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(42-42)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(44-44)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
Task(9-28)
ImmichFrame.WebApi/Program.cs (2)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (2)
PooledImmichFrameLogic(8-123)PooledImmichFrameLogic(16-30)ImmichFrame.Core/Logic/PoolConfiguration.cs (2)
PoolConfiguration(8-47)PoolConfiguration(14-22)
ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (3)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
CachingApiAssetsPool(6-76)Task(10-11)Task(13-39)Task(41-44)Task(47-73)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
Task(9-28)ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs (2)
List(32-49)List(51-70)
ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs (3)
ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
MemoryAssetsPool(8-36)ImmichFrame.Core/Logic/LogicPoolAdapter.cs (8)
LogicPoolAdapter(8-45)Task(10-11)Task(13-14)Task(16-17)Task(19-20)Task(22-22)Task(24-25)Task(27-44)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (4)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)
ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (4)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (6)
CachingApiAssetsPool(6-76)Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
Task(26-26)AssetResponseDto(41-41)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(42-42)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(44-44)
ImmichFrame.Core/Logic/PoolConfiguration.cs (7)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (2)
IAssetPool(38-60)TimeSpan(33-34)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
AllAssetsPool(6-78)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
FavoriteAssetsPool(6-37)ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
MemoryAssetsPool(8-36)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
AlbumAssetsPool(7-29)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
PersonAssetsPool(6-40)ImmichFrame.Core/Logic/Pool/MultiAssetPool.cs (1)
MultiAssetPool(6-22)
ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (4)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
Task(26-26)AssetResponseDto(41-41)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(42-42)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(44-44)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (4)
ImmichFrame.Core/Logic/PoolConfiguration.cs (1)
IAssetPool(24-46)ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (7)
IAssetPool(38-60)Task(62-65)Task(67-70)Task(72-72)Task(74-74)Task(76-76)Task(78-120)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (2)
Task(32-35)List(62-72)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
Task(9-28)
ImmichFrame.Core/Logic/LogicPoolAdapter.cs (5)
ImmichFrame.Core/Logic/PoolConfiguration.cs (1)
IAssetPool(24-46)ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (4)
IAssetPool(38-60)Task(62-65)Task(67-70)Task(72-72)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
Task(9-28)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core/Helpers/WebhookHelper.cs (1)
WebhookHelper(5-33)
ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (1)
ImmichFrame.WebApi.Tests/Mocks/MockHttpMessageHandler.cs (1)
Task(19-27)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (3)
ImmichFrame.Core/Logic/LogicPoolAdapter.cs (1)
LogicPoolAdapter(8-45)ImmichFrame.Core/Helpers/ApiCache.cs (3)
ApiCache(5-27)ApiCache(10-15)ApiCache(17-20)ImmichFrame.Core/Logic/PoolConfiguration.cs (3)
IAssetPool(24-46)PoolConfiguration(8-47)PoolConfiguration(14-22)
🪛 GitHub Check: test
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs
[warning] 6-6:
Parameter 'IAccountSettings accountSettings' is captured into the state of the enclosing type and its value is also passed to the base constructor. The value might be captured by the base class as well.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (16)
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
6-6: Constructor alignment with base change looks goodDropping ImmichApi from the base ctor and keeping it as a primary-ctor capture here is consistent with the refactor across pools. LGTM.
ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (1)
161-176: Broader asset/thumbnails mock looks goodCovers both /assets/ and /thumbnail endpoints, returning minimal JPEG bytes. This stabilizes the test path.
If the API may hit additional asset-related endpoints (e.g.,
/api/assets/), consider including those patterns explicitly.ImmichFrame.WebApi/Program.cs (1)
63-65: LGTM!The DI setup correctly constructs
PoolConfigurationand passes it toPooledImmichFrameLogic. ActivatorUtilities will resolve the missing dependencies (IGeneralSettingsandIHttpClientFactory) from the service provider.ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (1)
27-29: LGTM!The return type update from
IEnumerable<AssetResponseDto>toIList<AssetResponseDto>aligns with the base class signature changes. The test logic remains unchanged.ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (1)
27-29: LGTM!Consistent with the refactor to return
IList<AssetResponseDto>instead ofIEnumerable<AssetResponseDto>.ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (1)
26-26: LGTM!The return type update is consistent with the broader refactor.
ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
6-8: LGTM!The constructor base call and return type updates align with the refactored
CachingApiAssetsPoolbase class, which no longer acceptsImmichApiin its constructor and expectsIList<AssetResponseDto>return types.ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (2)
7-9: LGTM!The constructor and return type updates are consistent with the base class refactor.
27-27: LGTM!The
.ToList()call is necessary to convert theIEnumerableresult fromWhereExcludestoIList<AssetResponseDto>.ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs (3)
191-216: LGTM!This test effectively verifies that memories are sorted chronologically by reversing the input order and asserting that assets are still returned oldest-first ("2 years ago" before "1 year ago").
218-244: LGTM!Excellent use of
[Repeat(10)]and shuffling to ensure the chronological sorting is deterministic and not accidentally passing due to input order.
246-274: LGTM!This test verifies that chronological ordering is maintained across multiple calls to
GetNextAsset, confirming that the pool's internal iteration state respects the sorted order.ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (2)
25-34: LGTM!The test harness updates from
IEnumerable<AssetResponseDto>toIList<AssetResponseDto>are consistent with the base class refactor.
48-52: LGTM!The mock setup correctly uses the updated
IList<AssetResponseDto>type for the cache delegate.ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
78-120: LGTM!The
GetImageoverride correctly adds local caching on top of the base implementation. The delegation tobase.GetImage(id)at line 106 and the surrounding cache logic are appropriate.ImmichFrame.Core/Logic/LogicPoolAdapter.cs (1)
27-44: Verify and extend image content-type mapping
The GetImage logic assumes only “image/webp” and defaults all other types to JPEG. Confirm whether the Immich API can return formats like PNG or GIF and, if so, map those content-types to their correct extensions.
jpg0
force-pushed
the
feat/sort-memories-chronologically
branch
from
d927278 to
62596c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
34-56: Code duplication: BuildPool logic exists in both classes.The
BuildPoolmethod (lines 34-56) duplicates pool-building logic that already exists inPoolConfiguration.BuildPool(lines 17-46 per context). This violates the DRY principle and creates maintenance burden - any changes to pool construction must be made in two places.Given that
PoolConfigurationis already responsible for building the pool, consider removing this method and relying on the pool frompoolConfiguration.AssetPool.Apply this approach to eliminate duplication:
- private IAssetPool BuildPool(IAccountSettings accountSettings) - { - if (!accountSettings.ShowFavorites && !accountSettings.ShowMemories && !accountSettings.Albums.Any() && !accountSettings.People.Any()) - { - return new AllAssetsPool(_apiCache, _immichApi, accountSettings); - } - - var pools = new List<IAssetPool>(); - - if (accountSettings.ShowFavorites) - pools.Add(new FavoriteAssetsPool(_apiCache, _immichApi, accountSettings)); - - if (accountSettings.ShowMemories) - pools.Add(new MemoryAssetsPool(_immichApi, accountSettings)); - - if (accountSettings.Albums.Any()) - pools.Add(new AlbumAssetsPool(_apiCache, _immichApi, accountSettings)); - - if (accountSettings.People.Any()) - pools.Add(new PersonAssetsPool(_apiCache, _immichApi, accountSettings)); - - return new MultiAssetPool(pools); - }
🧹 Nitpick comments (3)
ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs (2)
25-25: Consider using MockBehavior.Strict for better test safety.Passing
null!for required constructor parameters works but provides minimal mock setup. Consider usingMockBehavior.Strictinstead to catch unexpected calls during test development.Apply this diff:
- _mockImmichApi = new Mock<ImmichApi>(null!, null!); // Base constructor requires ILogger, IHttpClientFactory, IOptions, pass null + _mockImmichApi = new Mock<ImmichApi>(MockBehavior.Strict, null!, null!);
218-244: Consider removing [Repeat(10)] for deterministic chronological ordering.Since
MemoryAssetsPoolnow explicitly sorts byMemoryAt(line 8 in MemoryAssetsPool.cs:OrderBy(m => m.MemoryAt)), the ordering is deterministic. The[Repeat(10)]attribute is unnecessary overhead for a test that will produce the same result every time.Based on learnings: The retrieved learning about
ChronologicalAssetsPoolWrapperusing Fisher-Yates shuffle doesn't apply here, asMemoryAssetsPooluses explicit ordering, not shuffling.Apply this diff:
[Test] - [Repeat(10)] // In case it accidentally passes due to random order public async Task LoadAssetsFromApi_ReturnsAssetsInChronologicalMemoryOrder()ImmichFrame.Core/Logic/PoolConfiguration.cs (1)
10-12: Consider using init-only setters for immutability.The properties already use auto-property syntax (addressing the past review comment), but consider using
{ get; init; }instead of{ get; }to enable initialization syntax while maintaining immutability after construction.Apply this diff:
- public ImmichApi ImmichApi { get; } - public IAssetPool AssetPool { get; } - public ApiCache ApiCache { get; } + public ImmichApi ImmichApi { get; init; } + public IAssetPool AssetPool { get; init; } + public ApiCache ApiCache { get; init; }Then update the constructor:
- ImmichApi = new ImmichApi(accountSettings.ImmichServerUrl, httpClient); - ApiCache = new ApiCache(TimeSpan.FromHours(generalSettings.RefreshAlbumPeopleInterval)); - AssetPool = BuildPool(accountSettings, ApiCache, ImmichApi); + ImmichApi = new ImmichApi(accountSettings.ImmichServerUrl, httpClient), + ApiCache = new ApiCache(TimeSpan.FromHours(generalSettings.RefreshAlbumPeopleInterval)), + AssetPool = BuildPool(accountSettings, ApiCache, ImmichApi)Wait, that's not correct syntax. Actually with
{ get; init; }, the assignment in the constructor body would remain the same. The benefit is that you can also use object initializer syntax if needed. Keep the constructor body as is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs(1 hunks)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs(12 hunks)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs(1 hunks)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs(1 hunks)ImmichFrame.Core/Logic/LogicPoolAdapter.cs(1 hunks)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs(2 hunks)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs(2 hunks)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs(2 hunks)ImmichFrame.Core/Logic/PoolConfiguration.cs(1 hunks)ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs(2 hunks)ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs(2 hunks)ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj(1 hunks)ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs(3 hunks)ImmichFrame.WebApi/Program.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs
- ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs
- ImmichFrame.Core/Logic/LogicPoolAdapter.cs
- ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.467Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
📚 Learning: 2025-10-09T16:12:49.467Z
Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.467Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.csImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs
🧬 Code graph analysis (12)
ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (4)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
Task(26-26)AssetResponseDto(41-41)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(42-42)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
Task(8-39)
ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj (2)
ImmichFrame.Core.Tests/Logic/Pool/MultiAssetPoolTests.cs (1)
ImmichFrame(12-303)ImmichFrame.Core.Tests/Logic/Pool/AggregatingAssetPoolTests.cs (1)
ImmichFrame(6-208)
ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (1)
ImmichFrame.WebApi.Tests/Mocks/MockHttpMessageHandler.cs (1)
Task(19-27)
ImmichFrame.WebApi/Program.cs (3)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (2)
PooledImmichFrameLogic(8-119)PooledImmichFrameLogic(16-26)ImmichFrame.Core/Logic/PoolConfiguration.cs (2)
PoolConfiguration(8-47)PoolConfiguration(14-22)ImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.cs (1)
MultiImmichFrameLogicDelegate(16-28)
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (4)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (6)
CachingApiAssetsPool(6-76)Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(44-44)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
Task(9-28)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
Task(8-36)
ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
CachingApiAssetsPool(6-76)Task(10-11)Task(13-39)Task(41-44)Task(47-73)
ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (2)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (6)
CachingApiAssetsPool(6-76)Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
Task(26-26)AssetResponseDto(41-41)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (3)
ImmichFrame.Core/Logic/LogicPoolAdapter.cs (1)
LogicPoolAdapter(8-45)ImmichFrame.Core/Helpers/ApiCache.cs (3)
ApiCache(5-27)ApiCache(10-15)ApiCache(17-20)ImmichFrame.Core/Logic/PoolConfiguration.cs (3)
IAssetPool(24-46)PoolConfiguration(8-47)PoolConfiguration(14-22)
ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs (3)
ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
MemoryAssetsPool(8-36)ImmichFrame.Core/Logic/LogicPoolAdapter.cs (8)
LogicPoolAdapter(8-45)Task(10-11)Task(13-14)Task(16-17)Task(19-20)Task(22-22)Task(24-25)Task(27-44)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (4)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)
ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (4)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
Task(26-26)AssetResponseDto(41-41)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(44-44)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
Task(8-36)
ImmichFrame.Core/Logic/PoolConfiguration.cs (3)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
IAssetPool(34-56)ImmichFrame.Core/Helpers/HttpClientExtensionMethods.cs (1)
UseApiKey(5-8)ImmichFrame.Core/Logic/Pool/MultiAssetPool.cs (1)
MultiAssetPool(6-22)
ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (3)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
Task(26-26)AssetResponseDto(41-41)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
Task(9-28)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)
🪛 GitHub Check: test
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs
[warning] 6-6:
Parameter 'IAccountSettings accountSettings' is captured into the state of the enclosing type and its value is also passed to the base constructor. The value might be captured by the base class as well.
ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs
[warning] 7-7:
Parameter 'IAccountSettings accountSettings' is captured into the state of the enclosing type and its value is also passed to the base constructor. The value might be captured by the base class as well.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (22)
ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (3)
15-15: LGTM! Chronological ordering successfully implemented.The memories are now correctly sorted by
MemoryAtdate, ensuring they're displayed in chronological order as specified in the PR objectives.
15-15: Excellent! Memories now sorted chronologically.This
.OrderBy(m => m.MemoryAt)directly addresses the PR objective and issue #163 by ensuring memories are processed in chronological order (e.g., one year ago, two years ago) rather than in random order.
10-10: Return type change aligns with broader refactor.The change from
IEnumerable<AssetResponseDto>toIList<AssetResponseDto>is consistent with the codebase-wide migration to concrete list types across all pool implementations.ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (3)
7-7: The static analysis warning is a false positive.The warning about
accountSettingsbeing captured in the primary constructor state and passed to base is expected C# 12 primary constructor behavior. This pattern is intentional and does not indicate an issue.
7-7: Static analysis warning can be ignored.The analyzer flags
accountSettingsbeing passed to both the base constructor and captured by this class. This is intentional and safe—the base class (CachingApiAssetsPool) requires it for applying account filters, while this class uses it for album-specific logic. No actual issue exists.
9-27: Return type and ToList() addition look good.The return type change from
IEnumerable<AssetResponseDto>toIList<AssetResponseDto>with the corresponding.ToList()call aligns with the broader refactor across all pool implementations.ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj (2)
29-29: LGTM! Test project reference enables test utility sharing.Adding a reference to
ImmichFrame.Core.Testsallows this test project to reuse test helpers and utilities, which is a reasonable pattern for reducing test code duplication.
29-29: Project reference addition is appropriate.Adding a reference to
ImmichFrame.Core.Testsenables the Web API test project to leverage shared test utilities and helpers from the core test suite, which is a standard and beneficial practice.ImmichFrame.WebApi/Program.cs (2)
64-65: LGTM! DI factory correctly wired for new architecture.The factory now properly creates
PoolConfigurationand passes it toPooledImmichFrameLogic, supporting the new pool-based orchestration pattern.
64-65: DI wiring correctly updated for new architecture.The factory now creates
PoolConfigurationviaActivatorUtilitiesand passes it toPooledImmichFrameLogic, correctly aligning with the updated constructor signature and the new pool-based orchestration architecture.ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (2)
161-192: LGTM! Enhanced mock coverage prevents unintended network calls.The extended mock setup now handles both
/assets/and/thumbnailendpoints, with a catch-all for any other requests. This defensive approach ensures all HTTP interactions are mocked during tests.
161-192: Enhanced mock coverage improves test robustness.The expanded mocking now handles both
/assets/and/thumbnailendpoints (lines 166-168), plus a catch-all for other endpoints (lines 177-192). This prevents unintended network calls and ensures the tests remain isolated and deterministic, especially important given the new pool orchestration andLogicPoolAdapterimage retrieval paths.ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs (2)
51-69: LGTM! Memory creation logic supports chronological ordering.The implementation correctly computes
memoryAtrelative tomemoryYearand passes the year asDateTimeOriginal, enabling the chronological ordering tests.
191-216: LGTM! Test validates chronological ordering.This test correctly verifies that memories are returned oldest-first regardless of input order. The use of
Reverse()ensures the test isn't passing accidentally.ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (2)
6-6: Static analysis warning can be safely ignored.The warning about
accountSettingsbeing both captured and passed to base is a common pattern in C# when derived classes need to use constructor parameters both locally and in the base class. In this case,accountSettings.Peopleis accessed at line 12, which is the intended design.
8-39: LGTM! Return type change and pagination improvements are correct.The changes correctly:
- Update return type to
IList<AssetResponseDto>for consistency- Replace
totalwithreturnedfor clearer pagination semantics- Add cancellation token check in the loop condition to enable early termination
ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (1)
25-272: LGTM! Test harness consistently updated for IList return type.All changes correctly update the test infrastructure and assertions to work with
IList<AssetResponseDto>instead ofIEnumerable<AssetResponseDto>. The test logic remains sound and comprehensive.ImmichFrame.Core/Logic/PoolConfiguration.cs (2)
14-22: LGTM! Constructor properly initializes all components.The constructor correctly:
- Creates HTTP client from factory (supporting proper disposal and configuration)
- Applies API key to the client
- Initializes ImmichApi, ApiCache, and AssetPool in the correct order
24-46: LGTM! Pool configuration logic correctly consolidated.The
BuildPoolmethod appropriately:
- Returns
AllAssetsPoolwhen no specific filters are enabled- Assembles a
MultiAssetPoolfrom enabled pool types in a consistent order- Correctly passes
apiCacheto pools that need caching (Favorites, Albums, People) whileMemoryAssetsPooluses its own daily cacheThis consolidation improves maintainability by centralizing pool configuration logic.
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
74-116: GetImage refactor looks good.The delegation to
base.GetImage(id)appropriately leverages the base class implementation fromLogicPoolAdapter, eliminating duplication and maintaining consistent image retrieval behavior.ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (1)
27-27: Test helper return type correctly updated.The return type change from
Task<IEnumerable<AssetResponseDto>>toTask<IList<AssetResponseDto>>aligns with the implementation change inFavoriteAssetsPool.LoadAssetsand the broader migration to list-based return types across all pool implementations.ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (1)
27-27: Test helper return type correctly updated.The return type change from
Task<IEnumerable<AssetResponseDto>>toTask<IList<AssetResponseDto>>aligns with the implementation change inPersonAssetsPool.LoadAssetsand maintains consistency with the broader refactor to list-based return types.
|
I guess you will run into the same Problem as i did - there are two random|shuffles after a sorting: |
jpg0
force-pushed
the
feat/sort-memories-chronologically
branch
from
62596c5 to
5569f97
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (1)
177-192: Recommend adding a clarifying comment.The catch-all mock improves test stability by handling unmocked endpoints, but could mask unexpected API calls. Consider adding a comment explaining its purpose to aid future maintenance.
Example comment:
+// Catch-all for any other API calls during test execution that aren't explicitly mocked above. +// This prevents spurious test failures from additional endpoints introduced by PoolConfiguration/LogicPoolAdapter changes. +// Returns minimal valid JSON to satisfy most GET endpoints. _mockHttpMessageHandler.Protected()ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs (1)
25-25: Consider avoiding null-forgiving operators.The use of
null!to suppress null warnings forImmichApiconstructor parameters may hide actual nullability issues. If the constructor truly accepts nullable parameters, consider using explicitnullvalues or creating a test-specific mock setup.Consider this alternative:
- _mockImmichApi = new Mock<ImmichApi>(null!, null!); + _mockImmichApi = new Mock<ImmichApi>(MockBehavior.Strict); + // or create a test helper that provides default parametersImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
16-35: UseAssets.Totalfor pagination to avoid an extra API call
SearchAssetsAsyncreturnsAssets.Total; update the loop in PeopleAssetsPool.cs to include a check likepage*batchSize < totalinstead of relying solely onitems.Count == batchSizeso you don’t fetch an empty final page when the total is an exact multiple ofbatchSize.ImmichFrame.Core/Logic/LogicPoolAdapter.cs (1)
27-44: Simplistic extension inference may cause incorrect file extensions.The content-type detection defaults to "jpeg" for any non-webp image, which could be incorrect for PNG, GIF, SVG, BMP, TIFF, or other image formats. While this may work for most cases, it could cause issues with file caching or client-side rendering expectations.
Consider a more comprehensive mapping:
- var ext = contentType.ToLower() == "image/webp" ? "webp" : "jpeg"; + var ext = contentType.ToLower() switch + { + "image/webp" => "webp", + "image/png" => "png", + "image/gif" => "gif", + "image/svg+xml" => "svg", + "image/bmp" => "bmp", + "image/tiff" => "tiff", + _ => "jpeg" + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs(1 hunks)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs(12 hunks)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs(1 hunks)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs(1 hunks)ImmichFrame.Core/Logic/LogicPoolAdapter.cs(1 hunks)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs(2 hunks)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs(2 hunks)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs(1 hunks)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs(2 hunks)ImmichFrame.Core/Logic/PoolConfiguration.cs(1 hunks)ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs(1 hunks)ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs(2 hunks)ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj(1 hunks)ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs(3 hunks)ImmichFrame.WebApi/Program.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- ImmichFrame.WebApi/Program.cs
- ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj
- ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs
- ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
PR: immichFrame/ImmichFrame#481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.csImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.csImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.csImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs
🧬 Code graph analysis (12)
ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (5)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
Task(26-26)AssetResponseDto(41-41)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (1)
Task(32-35)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(44-44)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
Task(8-36)
ImmichFrame.Core/Logic/LogicPoolAdapter.cs (3)
ImmichFrame.Core/Helpers/WebhookHelper.cs (1)
WebhookHelper(5-33)ImmichFrame.Core/Interfaces/IImmichFrameLogic.cs (1)
IImmichFrameLogic(6-15)ImmichFrame.Core/Logic/Pool/IAssetPool.cs (1)
IAssetPool(5-36)
ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (5)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
Task(26-26)AssetResponseDto(41-41)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (1)
Task(32-35)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(42-42)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
Task(8-39)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (8)
ImmichFrame.Core/Logic/LogicPoolAdapter.cs (1)
LogicPoolAdapter(8-45)ImmichFrame.Core/Logic/PoolConfiguration.cs (2)
PoolConfiguration(8-21)PoolConfiguration(13-20)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
AllAssetsPool(6-78)ImmichFrame.Core/Helpers/ApiCache.cs (3)
ApiCache(5-27)ApiCache(10-15)ApiCache(17-20)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
FavoriteAssetsPool(6-37)ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
MemoryAssetsPool(8-36)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
AlbumAssetsPool(7-29)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
PersonAssetsPool(6-40)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (5)
ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (1)
Task(32-35)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(42-42)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (2)
Task(27-30)AssetResponseDto(44-44)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
Task(9-28)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (5)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (2)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (6)
CachingApiAssetsPool(6-76)Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
Task(8-36)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (6)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
IAssetPool(20-42)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (2)
Task(32-35)List(62-72)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
Task(9-28)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
Task(8-36)ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
Task(10-35)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
Task(8-39)
ImmichFrame.Core/Logic/PoolConfiguration.cs (1)
ImmichFrame.Core/Helpers/HttpClientExtensionMethods.cs (1)
UseApiKey(5-8)
ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (6)
CachingApiAssetsPool(6-76)Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)
ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (2)
ImmichFrame.Core/Logic/LogicPoolAdapter.cs (7)
Task(10-11)Task(13-14)Task(16-17)Task(19-20)Task(22-22)Task(24-25)Task(27-44)ImmichFrame.WebApi.Tests/Mocks/MockHttpMessageHandler.cs (1)
Task(19-27)
ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs (3)
ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
MemoryAssetsPool(8-36)ImmichFrame.Core/Logic/LogicPoolAdapter.cs (8)
LogicPoolAdapter(8-45)Task(10-11)Task(13-14)Task(16-17)Task(19-20)Task(22-22)Task(24-25)Task(27-44)ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (4)
Task(10-11)Task(13-39)Task(41-44)Task(47-73)
ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (4)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (6)
CachingApiAssetsPool(6-76)Task(10-11)Task(13-39)Task(41-44)Task(47-73)Task(75-75)ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
Task(26-26)AssetResponseDto(41-41)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (1)
Task(32-35)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (3)
Task(8-13)Task(15-61)Task(64-76)
🪛 GitHub Check: test
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs
[warning] 6-6:
Parameter 'IAccountSettings accountSettings' is captured into the state of the enclosing type and its value is also passed to the base constructor. The value might be captured by the base class as well.
ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs
[warning] 7-7:
Parameter 'IAccountSettings accountSettings' is captured into the state of the enclosing type and its value is also passed to the base constructor. The value might be captured by the base class as well.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (16)
ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (2)
7-7: LGTM! Required for test infrastructure.The new using directives support HttpClientFactoryOptions configuration (line 36) and interface registrations (lines 83-84).
Also applies to: 12-12
161-175: LGTM! Expanded coverage for asset view requests.Correctly mocks both
/assets/and/thumbnailendpoints to cover different asset retrieval paths used by LogicPoolAdapter.ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
6-6: Static analysis warning about parameter capture.The static analysis tool flags that
accountSettingsis captured both by the class and passed to the base constructor, which may lead to redundant state storage. This is a design choice but could indicate unnecessary duplication.However, examining the base class
CachingApiAssetsPool, it appears the parameter is needed by the base class for filtering logic, so this warning can be safely ignored as a false positive.ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (1)
27-27: LGTM!The return type change from
IEnumerable<AssetResponseDto>toIList<AssetResponseDto>is consistent with the broader pool API refactoring. Tests correctly validate the new interface.ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (1)
27-27: LGTM!The return type change aligns with the pool API refactoring. The test coverage is comprehensive and correctly validates pagination behavior.
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (1)
26-26: LGTM!The return type change is consistent with the pool API refactoring. Tests properly validate the album inclusion/exclusion logic.
ImmichFrame.Core/Logic/PoolConfiguration.cs (1)
8-21: LGTM!The
PoolConfigurationclass provides a clean abstraction for wiring API access and caching. The use of read-only properties ensures immutability after construction, which is a good design choice.ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
7-28: LGTM!The changes align with the pool API refactoring:
- Base constructor simplified by removing
immichApiparameter- Return type changed to
IList<AssetResponseDto>for consistency- Explicit
ToList()materializes the filtered resultThe static analysis warning about
accountSettingsparameter capture is a false positive, as the parameter is correctly used by both the derived class and the base class.ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs (1)
8-36: Verify DailyApiCache expiration behavior.The
MemoryAssetsPooluses aDailyApiCachethat expires at midnight. Ensure this behavior aligns with the expectation that memories should refresh daily (when "Today's memories" change at midnight).If memories should update more frequently (e.g., hourly), the cache expiration may need adjustment.
ImmichFrame.WebApi.Tests/Logic/MemoryAssetsPoolTests.cs (2)
191-244: Excellent test coverage for chronological sorting!The new tests comprehensively verify that memories are returned in chronological order:
LoadAssets_ReturnsAssetsInChronologicalMemoryOrderverifies sorting with reversed inputLoadAssetsFromApi_ReturnsAssetsInChronologicalMemoryOrderuses[Repeat(10)]to guard against flakiness from random orderingLoadAssetsFromApi_ReturnsAssetsInChronologicalMemoryOrderOverMultipleCallsverifies sorting is preserved across multipleGetNextAssetcallsThese tests directly validate the fix for issue #163.
32-49: Improved test helper signature.The change from
CreateSampleAssets(int count, bool withExif, int yearCreated)toCreateSampleAssets(int count, bool withExif, DateTime created)is more precise and makes the test setup clearer. UsingDateTimedirectly eliminates the need to derive dates from year integers.ImmichFrame.Core/Logic/LogicPoolAdapter.cs (2)
8-9: LGTM! Clean adapter implementation.The primary constructor syntax and delegation pattern effectively compose the pool and API layers.
10-14: LGTM! Asset retrieval methods delegate correctly.Both methods properly delegate to the pool, with GetNextAsset correctly using FirstOrDefault() to handle empty results.
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (2)
6-11: LGTM! Constructor signature correctly updated.The removal of the ImmichApi parameter aligns with the refactoring to use PoolConfiguration, and the deterministic iteration via
_nextsupports the PR's objective to ensure consistent sequential access.
15-39: Ensure wrap-around and empty-list cases are tested
- Add or confirm a test where
_next >= all.Count(e.g. exhaust the pool with sequential GetAssets calls, then request again to force wrap-around)- Add or confirm a test where
AllAssetsreturns an empty list for a non-zerorequested(should return an empty result)ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
13-18: LGTM! Constructor properly uses PoolConfiguration.The past review comments flagged duplicate pool/cache initialization, but the current implementation correctly uses
PoolConfigurationto supply both the pool (viaBuildPool) and API instances, eliminating the redundancy. The base class initialization properly delegates toLogicPoolAdapter.
| { | ||
| private readonly Random _random = new(); | ||
| private int _next; //next asset to return |
There was a problem hiding this comment.
Thread safety: Concurrent GetAssets calls will corrupt _next.
The _next field is mutated without synchronization (lines 31, 37), so concurrent calls to GetAssets will race and produce incorrect results or skip assets. If this pool is accessed from multiple threads (e.g., concurrent requests in a web app), the iteration state will be corrupted.
Apply thread-local state or locking:
+ private readonly object _lock = new();
private int _next; //next asset to return
public async Task<IEnumerable<AssetResponseDto>> GetAssets(int requested, CancellationToken ct = default)
{
+ lock (_lock)
+ {
if (requested == 0)
{
return new List<AssetResponseDto>();
}
var all = await AllAssets(ct);
if (all.Count < requested)
{
requested = all.Count; //limit request to what we have
}
var tail = all.TakeLast(all.Count - _next).ToList();
if (tail.Count >= requested)
{
_next += requested;
return tail.Take(requested);
}
// not enough left in tail; need to read head too
var overrun = requested - tail.Count;
_next = overrun;
return tail.Concat(all.Take(overrun));
+ }
}Note: If AllAssets performs async I/O, consider using SemaphoreSlim instead of lock to avoid blocking threads.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs around line 8 (field
_next) and the mutations at lines ~31 and ~37, the mutable field _next is
modified without synchronization causing races when GetAssets is called
concurrently; fix by protecting access to _next with a thread-safe strategy:
either make _next thread-local (e.g., use ThreadLocal<int> or maintain a
per-call index) or serialize updates with a lock (create a private readonly
object _lock and read/update _next inside lock blocks), and if AllAssets or
surrounding logic is asynchronous avoid blocking locks and use an async-aware
semaphore (SemaphoreSlim.WaitAsync/Release) to guard the critical section;
ensure all reads and writes of _next use the chosen synchronization so
concurrent callers cannot corrupt iteration state.
|
|
||
| var memoryAssets = new List<AssetResponseDto>(); | ||
| foreach (var memory in memories) | ||
| foreach (var memory in memories.OrderBy(m => m.MemoryAt)) |
There was a problem hiding this comment.
🧩 Analysis chain
Chronological sorting implemented correctly.
The addition of .OrderBy(m => m.MemoryAt) ensures memories are processed in chronological order (oldest to newest), which addresses the core requirement of issue #163. This will display memories as "1 year ago", "2 years ago", etc., in the correct sequence.
However, there's a critical concern raised in the PR comments: JoeRu noted that sorting may be undone by random/shuffle operations in TotalAccountImagesSelectionStrategy.cs and MultiImmichFrameLogicDelegate.cs. These randomizations could override the sorting done here, making this fix ineffective.
Run the following script to identify potential shuffle operations that might override this sorting:
🏁 Script executed:
#!/bin/bash
# Search for random/shuffle operations in the codebase that might affect asset order
echo "=== Checking TotalAccountImagesSelectionStrategy.cs ==="
rg -nC3 -g 'TotalAccountImagesSelectionStrategy.cs' '\b(Random|Shuffle|OrderBy.*Guid\.NewGuid)\b'
echo "=== Checking MultiImmichFrameLogicDelegate.cs ==="
rg -nC3 -g 'MultiImmichFrameLogicDelegate.cs' '\b(Random|Shuffle|OrderBy.*Guid\.NewGuid)\b'
echo "=== Checking for any shuffle operations in Logic/AccountSelection ==="
rg -nC3 -g '*.cs' --type-add 'cs:*.cs' 'Logic/AccountSelection' -e '\b(Random|Shuffle|OrderBy.*Guid\.NewGuid)\b'
echo "=== Checking for any shuffle operations in PooledImmichFrameLogic ==="
rg -nC3 -g 'PooledImmichFrameLogic.cs' '\b(Random|Shuffle|OrderBy.*Guid\.NewGuid)\b'Length of output: 1953
Sorting is negated by downstream shuffle operations.
- TotalAccountImagesSelectionStrategy.cs (line 63):
assets.Shuffle()overrides the chronological sort. - MultiImmichFrameLogicDelegate.cs (line 34):
.Shuffle()on the assembled assets also discards prior ordering.
Reapply OrderBy(m => m.MemoryAt) after the final shuffle or relocate/remove these shuffles to ensure memories remain in chronological order.
🤖 Prompt for AI Agents
In ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs around line 15, the foreach
over memories.OrderBy(m => m.MemoryAt) is being negated by downstream Shuffle()
calls (TotalAccountImagesSelectionStrategy.cs line 63 and
MultiImmichFrameLogicDelegate.cs line 34); to fix, ensure chronological ordering
is preserved by either removing those downstream Shuffle() calls or reapplying
OrderBy(m => m.MemoryAt) after the final Shuffle (i.e., in the final asset
assembly/return path), so the resulting collection is explicitly sorted by
MemoryAt before being consumed or returned.
| private static IAssetPool BuildPool(IAccountSettings accountSettings, PoolConfiguration poolConfiguration) | ||
| { | ||
| if (!accountSettings.ShowFavorites && !accountSettings.ShowMemories && !accountSettings.Albums.Any() && !accountSettings.People.Any()) | ||
| { | ||
| return new AllAssetsPool(_apiCache, _immichApi, accountSettings); | ||
| return new AllAssetsPool(poolConfiguration.ApiCache, poolConfiguration.ImmichApi, accountSettings); | ||
| } | ||
|
|
||
| var pools = new List<IAssetPool>(); | ||
|
|
||
| if (accountSettings.ShowFavorites) | ||
| pools.Add(new FavoriteAssetsPool(_apiCache, _immichApi, accountSettings)); | ||
| pools.Add(new FavoriteAssetsPool(poolConfiguration.ApiCache, poolConfiguration.ImmichApi, accountSettings)); | ||
|
|
||
| if (accountSettings.ShowMemories) | ||
| pools.Add(new MemoryAssetsPool(_immichApi, accountSettings)); | ||
| pools.Add(new MemoryAssetsPool(poolConfiguration.ImmichApi, accountSettings)); | ||
|
|
||
| if (accountSettings.Albums.Any()) | ||
| pools.Add(new AlbumAssetsPool(_apiCache, _immichApi, accountSettings)); | ||
| pools.Add(new AlbumAssetsPool(poolConfiguration.ApiCache, poolConfiguration.ImmichApi, accountSettings)); | ||
|
|
||
| if (accountSettings.People.Any()) | ||
| pools.Add(new PersonAssetsPool(_apiCache, _immichApi, accountSettings)); | ||
| pools.Add(new PersonAssetsPool(poolConfiguration.ApiCache, poolConfiguration.ImmichApi, accountSettings)); | ||
|
|
||
| return new MultiAssetPool(pools); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify that downstream randomization doesn't undo chronological sorting.
The BuildPool logic correctly uses PoolConfiguration to create pools, including MemoryAssetsPool for chronological memories. However, as noted in the PR comments, JoeRu warned that sorting may be undone by random/shuffle operations in TotalAccountImagesSelectionStrategy.cs and MultiImmichFrameLogicDelegate.cs. These randomizations could override the chronological order established by MemoryAssetsPool.
Run the following script to check for randomization operations that might affect the order:
🏁 Script executed:
#!/bin/bash
# Description: Find shuffle/randomization logic that might override chronological order
# Search for Random usage in selection strategies
rg -n -A10 -B5 'Random|Shuffle|OrderBy.*Guid\.NewGuid' \
--glob='**/TotalAccountImagesSelectionStrategy.cs' \
--glob='**/MultiImmichFrameLogicDelegate.cs'
# Look for places where assets are reordered after pool creation
ast-grep --pattern $'$ASSETS.OrderBy($$$)'
ast-grep --pattern $'$ASSETS.Shuffle($$$)'Length of output: 4581
Preserve chronological order after pooling
MemoryAssetsPool already sorts by MemoryAt, but its output is later randomized by:
- TotalAccountImagesSelectionStrategy.cs:63 (
assets.Shuffle()) - MultiImmichFrameLogicDelegate.cs:34 (
GetAssets().Shuffle())
Move shuffles before sorting or reapply the chronological sort (or skip shuffle for memories) to maintain the intended order.
🤖 Prompt for AI Agents
In ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs around lines 20 to 42, the
logic builds a MultiAssetPool which ends up having assets shuffled elsewhere and
thereby breaks MemoryAssetsPool's chronological sorting by MemoryAt; update the
pooling behavior so the chronological order for memories is preserved by either
(A) preventing shuffles from being applied after pooling for memory-origin
assets (e.g., tag or flag memory assets and skip Shuffle in
TotalAccountImagesSelectionStrategy.cs:63 and
MultiImmichFrameLogicDelegate.cs:34 when assets include memory-origin items), or
(B) reapply sorting by MemoryAt immediately after any shuffle calls (or before
returning from MultiAssetPool) so memories are restored to chronological order;
implement the chosen approach consistently across the shuffle sites so
MemoryAssetsPool output remains chronologically ordered.
4 participants
Ensures that memories are returned chronologically. Tested by called the API with a mock immich server. Includes a few changes to make the implementation testable.
Also ensures that repeated calls iterate through correctly.
fixes #163 (again)
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactor
Tests