Handle null albums, people, and excluded albums #535
Conversation
📝 WalkthroughWalkthroughThis pull request systematically introduces null-safety checks across the pool logic codebase to prevent null reference exceptions when account settings collections (Albums, ExcludedAlbums, People) are null. Corresponding test cases are added to verify proper handling of these null scenarios. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Hey, @JW-CH - apologies if I'm being pushy, but I also know that it's easy for PR notifications to get lost in the shuffle. Just want to nudge that this PR is ready for review. |
|
I didn't review the tests, but the logic looks good to me. 👍 |
|
Sorry, I lack behind with reviews. Hopefully, I'll find this weekend :) |
JW-CH
left a comment
JW-CH
left a comment
There was a problem hiding this comment.
Can you check the tests again? I assume this is AI assisted/generated. Please verify that those checks are needed. In general, if you want my reivew, please review it yourself first :)
| // Act | ||
| var logic = new PooledImmichFrameLogic(_mockAccountSettings.Object, _mockGeneralSettings.Object, _mockHttpClientFactory.Object); | ||
|
|
||
| // Assert - The pool should be AllAssetsPool, not MultiAssetPool |
There was a problem hiding this comment.
these comments are quite off? can you check this file again?
There was a problem hiding this comment.
Also some of these checks do not really do anything? Checking if a constructor added something successfully does not make much sense to me.
| public void BuildPool_WithMixedSettingsAndNullPeople_DoesNotIncludePersonAssetsPool() | ||
| { | ||
| // Arrange - enable some settings but keep People null | ||
| _mockAccountSettings.SetupGet(s => s.ShowFavorites).Returns(true); |
| var logic = new PooledImmichFrameLogic(_mockAccountSettings.Object, _mockGeneralSettings.Object, _mockHttpClientFactory.Object); | ||
|
|
||
| // Assert | ||
| Assert.That(logic.AccountSettings.People, Is.Null); |
There was a problem hiding this comment.
wouldn't this throw if you access logic.AccountSettings.People if logic.AccountSettings is null? Asking because of the next check a line below.
|
@JW-CH - apologies, I did export the work on the tests to Copilot. I should have looked more closely at the tests before submitting the PR. With that in mind, I'm trying to do some TDD to verify that the tests actually catch errors, and I'm struggling with Moq here. In this case, I've removed the calls to 
We know that it can be If that's the case, I could possibly try to get around it by maybe creating a concrete instance of the settings and, within the scope of the test, create a new instance of I apologize, again, for wasting your time with my AI-generated unit tests. |
No need to apologize, I like reviewing stuff, but this takes time :) I'm glad that you like the project and want to make changes yourself. If you can't get the tests to work, please just remove them. I'd prefer having no tests over AI-gibberish tests. I can also add some myself. I have not the greatest knowledge myself in testing software, that's why I do not have a strong opinion about this. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core.Tests/Logic/PooledImmichFrameLogicTests.cs`:
- Around line 43-55: The test currently never calls BuildPool; update the test
in PooledImmichFrameLogicTests to call the BuildPool method on the _logic
instance (e.g., _logic.BuildPool()) and assert it does not throw; additionally,
if there is a public accessor for pools (e.g., _logic.Pools or similar), assert
that no PersonAssetsPool was added when AccountSettings.People is null (or
assert Pools.Count remains unchanged) to catch regressions where a
PersonAssetsPool is created.
…es of null handling are handled by other tests
JW-CH
left a comment
JW-CH
left a comment
There was a problem hiding this comment.
I have pushed on your PR, I hope you don't mind. If you want to return null in the setup, you are able to do it like so:
_mockAccountSettings.SetupGet(s => s.ExcludedAlbums).Returns((List<Guid>)null);I assume that was what you tried to do with your added property :) Thank you for your PR!
I found that actually didn't work. The reason I had to make the change to a property-driven provisioning of the albums reference is that, if I removed the null checks I added and used the It's not a hill I'll die on, but I do want to stress that the tests, as they exist in |
Good to know! Thank you! I'll have rework the tests sometimes in the future again. Will keep this in mind. For now, I'll think we are ok. |
3 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.