Handle null albums, people, and excluded albums #535
Conversation
📝 WalkthroughWalkthroughAdd null-safety guards for accountSettings collections (Albums, ExcludedAlbums, People) in pool logic and PooledImmichFrameLogic; introduce unit tests covering null and empty collection edge cases to ensure API calls are skipped or assets loaded appropriately. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-10-09T16:12:49.488ZApplied to files:
🧬 Code graph analysis (1)ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (2)
🔇 Additional comments (3)
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: 0
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
65-77: Consider using the local variable for consistency.Line 69 directly uses
accountSettings.ExcludedAlbums, while the calling code at line 53 loads it into a local variable. For consistency with the null-safety pattern used inAlbumAssetsPool.cs(lines 16, 28) andPeopleAssetsPool.cs(line 18), consider either:
- Passing
excludedAlbumsas a parameter to this method, or- Adding a defensive null check at the start of this method
This improves maintainability and prevents potential issues if the method is called from other contexts in the future.
🔎 Proposed refactor
Option 1: Pass the local variable as a parameter
- private async Task<IEnumerable<AssetResponseDto>> GetExcludedAlbumAssets(CancellationToken ct = default) + private async Task<IEnumerable<AssetResponseDto>> GetExcludedAlbumAssets(IEnumerable<Guid> excludedAlbums, CancellationToken ct = default) { var excludedAlbumAssets = new List<AssetResponseDto>(); - foreach (var albumId in accountSettings.ExcludedAlbums) + foreach (var albumId in excludedAlbums) { var albumInfo = await immichApi.GetAlbumInfoAsync(albumId, null, null, ct); excludedAlbumAssets.AddRange(albumInfo.Assets); } return excludedAlbumAssets; }And update the call site:
- var excludedAssetList = await GetExcludedAlbumAssets(ct); + var excludedAssetList = await GetExcludedAlbumAssets(excludedAlbums, ct);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.csImmichFrame.Core.Tests/Logic/PooledImmichFrameLogicTests.csImmichFrame.Core/Logic/Pool/AlbumAssetsPool.csImmichFrame.Core/Logic/Pool/AllAssetsPool.csImmichFrame.Core/Logic/Pool/PeopleAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 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/AllAssetsPoolTests.csImmichFrame.Core.Tests/Logic/PooledImmichFrameLogicTests.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.Core/Logic/Pool/AllAssetsPool.csImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.csImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs
🧬 Code graph analysis (3)
ImmichFrame.Core.Tests/Logic/PooledImmichFrameLogicTests.cs (1)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (2)
PooledImmichFrameLogic(9-144)PooledImmichFrameLogic(17-29)
ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (1)
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
Task(8-45)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (3)
Task(8-13)Task(15-62)Task(65-77)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
Task(9-36)
🔇 Additional comments (8)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
53-59: Good null-safety implementation.The defensive check for null and empty excluded albums prevents unnecessary API calls and potential exceptions. The pattern aligns well with the PR objective.
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
38-63: Excellent null-safety implementation.The introduction of
hasAlbumsandhasPeopleflags with explicit null checks cleanly separates validation from pool construction logic. This pattern preventsArgumentNullExceptionwhen iterating collections in downstream pool classes and makes the pool selection logic more readable.ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
12-18: LGTM - Clean null-safety pattern.The early return when
peopleis null prevents iteration over a null collection, and the local variable is used consistently throughout the method. This aligns well with the similar pattern inAlbumAssetsPool.cs.ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (1)
94-101: Good test coverage for null scenario.This test appropriately verifies that when
Peopleis null, no API calls are made and an empty result is returned. This complements the existing empty-list test and validates the null-safety changes inPeopleAssetsPool.cs.ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs (1)
137-175: Comprehensive test coverage for null and empty excluded albums.Both tests appropriately verify that no album info calls are made when
ExcludedAlbumsis null or empty, validating the optimization introduced in the null-safety changes. The assertions confirm that all assets are returned without exclusion filtering.ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (1)
102-126: Well-structured tests for null album scenarios.Both tests appropriately cover the edge cases:
LoadAssets_NullAlbums_ReturnsEmptyverifies no API calls when Albums is nullLoadAssets_NullExcludedAlbums_ReturnsAlbumAssetsverifies that null ExcludedAlbums doesn't prevent album asset loadingThese tests validate the null-safety implementation in
AlbumAssetsPool.cs.ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
13-33: Exemplary null-safety implementation.The consistent use of local variables (
excludedAlbums,albums) with null checks throughout the method demonstrates excellent defensive programming. This pattern is used consistently for both excluded and included albums, preventing potential null reference exceptions while maintaining clean, readable code.ImmichFrame.Core.Tests/Logic/PooledImmichFrameLogicTests.cs (1)
1-144: Comprehensive test coverage for pool building with null/empty People.The test suite effectively covers various scenarios:
- Null People (lines 48-61, 87-101, 128-143)
- Empty People (lines 68-80)
- Populated People (lines 108-121)
- Mixed settings combinations
The tests verify that pool construction doesn't throw exceptions when
Peopleis null or empty, which directly validates the null-safety changes inPooledImmichFrameLogic.cs. The approach of testing construction success and AccountSettings state is appropriate for validating the fix.
Co-authored-by: Jan <17313367+JW-CH@users.noreply.github.com>
Co-authored-by: Jan <17313367+JW-CH@users.noreply.github.com>
Co-authored-by: Jan <17313367+JW-CH@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
53-53: Remove unused local variable.The
excludedAlbumsvariable is assigned but never used. Line 54 and the subsequent code referenceaccountSettings.ExcludedAlbumsdirectly instead of using this local variable.🔎 Proposed fix
- var excludedAlbums = accountSettings.ExcludedAlbums; if (accountSettings.ExcludedAlbums?.Any() ?? false)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 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
Repo: immichFrame/ImmichFrame PR: 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/Logic/Pool/AllAssetsPool.cs
🔇 Additional comments (1)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
54-54: LGTM! Proper null-safety implementation.The null-conditional check correctly prevents
ArgumentNullExceptionwhenExcludedAlbumsis null, and ensures the exclusion logic only runs when excluded albums are present. This guards both theAny()call and the subsequent iteration inGetExcludedAlbumAssets.
JW-CH
left a comment
JW-CH
left a comment
There was a problem hiding this comment.
Sorry, the previous comments were just a hint on how to change it. This would be leftovers :)
| var assets = await immichApi.SearchRandomAsync(searchDto, ct); | ||
|
|
||
| if (accountSettings.ExcludedAlbums.Any()) | ||
| var excludedAlbums = accountSettings.ExcludedAlbums; |
| private IAssetPool BuildPool(IAccountSettings accountSettings) | ||
| { | ||
| if (!accountSettings.ShowFavorites && !accountSettings.ShowMemories && !accountSettings.Albums.Any() && !accountSettings.People.Any()) | ||
| var albums = accountSettings.Albums; |
| var albums = accountSettings.Albums; | ||
| var hasAlbums = accountSettings.Albums?.Any() ?? false; | ||
|
|
||
| var people = accountSettings.People; |
|
@JW-CH - sorry, I should have previewed these changes in the IDE before committing them - the 'unused variable' warnings would have been more visible. I've also replaced the calls to |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
65-77: Consider aligning with AlbumAssetsPool's null-check pattern.The current implementation is safe because
GetExcludedAlbumAssetsis only called whenexcludedAlbumsCount > 0(line 54), preventing null access at line 69. However,AlbumAssetsPool.csuses a more explicit pattern that captures the collection reference and checks null before iterating:var excludedAlbums = accountSettings.ExcludedAlbums; if (excludedAlbums != null) { foreach (var albumId in excludedAlbums) { // ... } }This pattern is clearer, more defensive against potential concurrent modifications, and maintains consistency across pool implementations.
🔎 Suggested refactor for consistency
private async Task<IEnumerable<AssetResponseDto>> GetExcludedAlbumAssets(CancellationToken ct = default) { var excludedAlbumAssets = new List<AssetResponseDto>(); - - foreach (var albumId in accountSettings.ExcludedAlbums) + + var excludedAlbums = accountSettings.ExcludedAlbums; + if (excludedAlbums != null) { - var albumInfo = await immichApi.GetAlbumInfoAsync(albumId, null, null, ct); - - excludedAlbumAssets.AddRange(albumInfo.Assets); + foreach (var albumId in excludedAlbums) + { + var albumInfo = await immichApi.GetAlbumInfoAsync(albumId, null, null, ct); + excludedAlbumAssets.AddRange(albumInfo.Assets); + } } return excludedAlbumAssets; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 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
Repo: immichFrame/ImmichFrame PR: 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/Logic/Pool/AllAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
🧬 Code graph analysis (2)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (3)
ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (4)
Test(74-85)s(92-92)s(78-78)api(71-71)ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs (4)
s(36-36)s(120-120)api(124-124)Test(111-135)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
AlbumAssetsPool(7-29)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
AlbumAssetsPool(7-37)
🔇 Additional comments (3)
ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
53-54: Null-safety fix looks good.The null-conditional operator and count check correctly prevent ArgumentNullException when
ExcludedAlbumsis null. This addresses the core issue described in the PR.Note: A past review comment suggested using
?.Any() ?? falsepattern instead of?.Count ?? 0. Both approaches work correctly for this use case.ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (2)
38-39: Null-safety flags correctly prevent ArgumentNullException.The use of
?.Count > 0safely handles null collections by leveraging C#'s lifted comparison operators, which returnfalsewhen the nullable operand is null. This correctly addresses the issue described in the PR.Note: A past review comment suggested using
?.Any() ?? falsepattern. While both approaches are functionally equivalent and correct, the current implementation is concise and idiomatic.
41-41: LGTM! Pool construction logic correctly uses null-safe flags.The conditional pool construction now properly handles null or empty collections:
AllAssetsPoolis used when no specific sources are configuredAlbumAssetsPoolandPersonAssetsPoolare only added when their respective collections have itemsThis prevents the ArgumentNullException that occurred when iterating over null collections.
Also applies to: 54-54, 57-57
Sorry for the back and forth. I just did some reading (on a quite outdated article, lol) https://www.tabsoverspaces.com/233649-comparing-speed-of-count-0-and-any But only the numbers should be outdated, not the point this guy is making:
In this case, I quite agree with that and would ignore the lesser performance and value readability higher; it also fits the style of the rest better. What are your thoughts? (By the way, LINQ got better over time and more optimized. I don't think it will be much of a difference, plus the love LINQ got performance-wise in .NET 10 is incredible (I know, Frame is on .NET 8 as of now)) |
My thoughts are it's been a very hot minute since I've written .NET professionally. 😄 I'm fine with |
2 participants
If I provided an empty configuration for albums, people, or excluded albums, the deserialized settings appear to leave the respective
.Albums,.People,and.ExcludedAlbumsproperties asnull. This causes errors like the one below when ImmichFrame tries to iterate over them:This adds
nullawareness around uses of these enumerables to protect against these errors.Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.