Handle null albums, people, and excluded albums #535
Conversation
📝 WalkthroughWalkthroughAdding null-safety checks across pool-related classes to prevent null reference exceptions in account settings collections, with corresponding unit tests verifying edge cases where Albums, ExcludedAlbums, or People are null. Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 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.
1 participant
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.