Conversation
📝 WalkthroughWalkthroughAsset identifiers are migrated from ChangesGuid ID migration, SearchAssetsAsync adoption, and API alignment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
78-80: Use named arguments for ImmichApi calls with multiple null parameters.These calls pass several positional
nullarguments to generated client methods with optional parameters (key,slug,isOwned,isShared,name,edited,size). Using named arguments would make the intent explicit and guard against parameter order changes during client regeneration.Affected calls:
GetAssetInfoAsync(assetId, null, null)→GetAssetInfoAsync(id: assetId, key: null, slug: null)(lines 78, 88)GetAllAlbumsAsync(assetId, null, null, null, null)→GetAllAlbumsAsync(assetId: assetId, id: null, isOwned: null, isShared: null, name: null)(line 80)ViewAssetAsync(null, id, string.Empty, AssetMediaSize.Preview, null)→ViewAssetAsync(edited: null, id: id, key: string.Empty, size: AssetMediaSize.Preview, slug: null)(line 143)PlayAssetVideoAsync(id, null, null)→PlayAssetVideoAsync(id: id, key: null, slug: null)(line 176)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` around lines 78 - 80, Replace positional null arguments with named arguments in ImmichApi method calls to improve clarity and protect against parameter order changes. Update the GetAssetInfoAsync call on line 78 to use named arguments for key and slug parameters, the GetAllAlbumsAsync call on line 80 to use named arguments for id, isOwned, isShared, and name parameters, the ViewAssetAsync call on line 143 to use named arguments for edited, id, key, size, and slug parameters, and the PlayAssetVideoAsync call on line 176 to use named arguments for id, key, and slug parameters. Each call should explicitly name all parameters being passed instead of relying on positional argument order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ImmichFrame.Core/Helpers/AssetHelper.cs`:
- Around line 15-20: The GetExcludedAlbumAssets method in AssetHelper.cs and
LoadAssets method in AlbumAssetsPool.cs only fetch the first page of search
results, causing assets from later pages to be missed. Implement pagination in
both methods by wrapping the search call in a loop that uses Page and Size
parameters set to a batch size of 1000, accumulating results across multiple
pages until a page returns fewer items than the batch size. Follow the same
pagination pattern already implemented in FavoriteAssetsPool, PeopleAssetsPool,
and TagAssetsPool to ensure consistency across the codebase.
In `@ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs`:
- Around line 17-19: The albumAssets query in AlbumAssetsPool is not paginated
and only retrieves the first page of results, causing large albums to have
truncated assets. Implement pagination similar to the FavoriteAssetsPool,
PeopleAssetsPool, and TagAssetsPool implementations by wrapping the
immichApi.SearchAssetsAsync call in a loop that increments the Page parameter on
MetadataSearchDto until all pages are retrieved. Apply the same pagination fix
to AssetHelper.cs where the identical issue exists.
---
Nitpick comments:
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 78-80: Replace positional null arguments with named arguments in
ImmichApi method calls to improve clarity and protect against parameter order
changes. Update the GetAssetInfoAsync call on line 78 to use named arguments for
key and slug parameters, the GetAllAlbumsAsync call on line 80 to use named
arguments for id, isOwned, isShared, and name parameters, the ViewAssetAsync
call on line 143 to use named arguments for edited, id, key, size, and slug
parameters, and the PlayAssetVideoAsync call on line 176 to use named arguments
for id, key, and slug parameters. Each call should explicitly name all
parameters being passed instead of relying on positional argument order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 021382d4-dd64-49fa-b97b-f3c51b252447
📒 Files selected for processing (24)
ImmichFrame.Core.Tests/Logic/Pool/AggregatingAssetPoolTests.csImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/FixtureHelpers.csImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/MultiAssetPoolTests.csImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/QueuingAssetPoolTests.csImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.csImmichFrame.Core/Helpers/AssetHelper.csImmichFrame.Core/Logic/AccountSelection/BloomFilterAssetAccountTracker.csImmichFrame.Core/Logic/AccountSelection/IAssetAccountTracker.csImmichFrame.Core/Logic/AccountSelection/TotalAccountImagesSelectionStrategy.csImmichFrame.Core/Logic/Pool/AlbumAssetsPool.csImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.csImmichFrame.Core/Logic/Pool/MemoryAssetsPool.csImmichFrame.Core/Logic/Pool/PeopleAssetsPool.csImmichFrame.Core/Logic/Pool/TagAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.Core/OpenAPIs/immich-openapi-specs.jsonImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.csImmichFrame.WebApi/Controllers/AssetController.cs
Comment on lines
+15
to
20
| var metadataBody = new MetadataSearchDto { AlbumIds = [albumId] }; | ||
| var searchResponse = await immichApi.SearchAssetsAsync(metadataBody, ct); | ||
| if (searchResponse.Assets != null) | ||
| { | ||
| excludedAlbumAssets.AddRange(albumInfo.Assets); | ||
| excludedAlbumAssets.AddRange(searchResponse.Assets.Items); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Cross-check all exclusion and album search callsites for explicit pagination.
rg -n --type=cs "ExcludedAlbums|AlbumIds" -C2
rg -n --type=cs "SearchAssetsAsync\\(" -C2
rg -n --type=cs "Page\\s*=|Size\\s*="Repository: immichFrame/ImmichFrame
Length of output: 42490
Excluded-album filtering requires pagination to fetch all assets.
The GetExcludedAlbumAssets method in AssetHelper.cs and LoadAssets method in AlbumAssetsPool.cs both fetch only the first page of search results. Albums with more assets than the default page size will have assets on later pages not excluded/included, allowing them to appear incorrectly in results.
Other search-based pools in the codebase (FavoriteAssetsPool, PeopleAssetsPool, TagAssetsPool) all implement pagination with a loop, Page/Size parameters, and batchSize = 1000, stopping when fewer items are returned than the batch size. The same pattern should be applied here.
Suggested fix
- var metadataBody = new MetadataSearchDto { AlbumIds = [albumId] };
- var searchResponse = await immichApi.SearchAssetsAsync(metadataBody, ct);
- if (searchResponse.Assets != null)
- {
- excludedAlbumAssets.AddRange(searchResponse.Assets.Items);
- }
+ const int pageSize = 1000;
+ for (var page = 1; ; page++)
+ {
+ var metadataBody = new MetadataSearchDto
+ {
+ AlbumIds = [albumId],
+ Page = page,
+ Size = pageSize,
+ };
+ var searchResponse = await immichApi.SearchAssetsAsync(metadataBody, ct);
+ var items = searchResponse.Assets?.Items ?? [];
+ if (items.Count == 0) break;
+
+ excludedAlbumAssets.AddRange(items);
+ if (items.Count < pageSize) break;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var metadataBody = new MetadataSearchDto { AlbumIds = [albumId] }; | |
| var searchResponse = await immichApi.SearchAssetsAsync(metadataBody, ct); | |
| if (searchResponse.Assets != null) | |
| { | |
| excludedAlbumAssets.AddRange(albumInfo.Assets); | |
| excludedAlbumAssets.AddRange(searchResponse.Assets.Items); | |
| } | |
| const int pageSize = 1000; | |
| for (var page = 1; ; page++) | |
| { | |
| var metadataBody = new MetadataSearchDto | |
| { | |
| AlbumIds = [albumId], | |
| Page = page, | |
| Size = pageSize, | |
| }; | |
| var searchResponse = await immichApi.SearchAssetsAsync(metadataBody, ct); | |
| var items = searchResponse.Assets?.Items ?? []; | |
| if (items.Count == 0) break; | |
| excludedAlbumAssets.AddRange(items); | |
| if (items.Count < pageSize) break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImmichFrame.Core/Helpers/AssetHelper.cs` around lines 15 - 20, The
GetExcludedAlbumAssets method in AssetHelper.cs and LoadAssets method in
AlbumAssetsPool.cs only fetch the first page of search results, causing assets
from later pages to be missed. Implement pagination in both methods by wrapping
the search call in a loop that uses Page and Size parameters set to a batch size
of 1000, accumulating results across multiple pages until a page returns fewer
items than the batch size. Follow the same pagination pattern already
implemented in FavoriteAssetsPool, PeopleAssetsPool, and TagAssetsPool to ensure
consistency across the codebase.
Comment on lines
+17
to
+19
| var metadataBody = new MetadataSearchDto { AlbumIds = [albumId] }; | ||
| var searchResponse = await immichApi.SearchAssetsAsync(metadataBody, ct); | ||
| albumAssets.AddRange(searchResponse.Assets.Items); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify pagination usage patterns for SearchAssetsAsync in the repo.
rg -n --type=cs "SearchAssetsAsync\\(" -C2
rg -n --type=cs "new MetadataSearchDto" -C2
rg -n --type=cs "Page\\s*=|Size\\s*="Repository: immichFrame/ImmichFrame
Length of output: 29547
Paginate album search results to avoid silently dropping assets.
At Line 18, each album is queried once without Page/Size, so only the first page is loaded. This is inconsistent with other asset pool implementations (FavoriteAssetsPool, PeopleAssetsPool, TagAssetsPool) which use pagination loops. Large albums will have their assets truncated. The same issue exists in AssetHelper.cs.
Suggested fix
- var metadataBody = new MetadataSearchDto { AlbumIds = [albumId] };
- var searchResponse = await immichApi.SearchAssetsAsync(metadataBody, ct);
- albumAssets.AddRange(searchResponse.Assets.Items);
+ const int pageSize = 1000;
+ for (var page = 1; ; page++)
+ {
+ var metadataBody = new MetadataSearchDto
+ {
+ AlbumIds = [albumId],
+ Page = page,
+ Size = pageSize,
+ };
+ var searchResponse = await immichApi.SearchAssetsAsync(metadataBody, ct);
+ var items = searchResponse.Assets?.Items ?? [];
+ if (items.Count == 0) break;
+
+ albumAssets.AddRange(items);
+ if (items.Count < pageSize) break;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var metadataBody = new MetadataSearchDto { AlbumIds = [albumId] }; | |
| var searchResponse = await immichApi.SearchAssetsAsync(metadataBody, ct); | |
| albumAssets.AddRange(searchResponse.Assets.Items); | |
| const int pageSize = 1000; | |
| for (var page = 1; ; page++) | |
| { | |
| var metadataBody = new MetadataSearchDto | |
| { | |
| AlbumIds = [albumId], | |
| Page = page, | |
| Size = pageSize, | |
| }; | |
| var searchResponse = await immichApi.SearchAssetsAsync(metadataBody, ct); | |
| var items = searchResponse.Assets?.Items ?? []; | |
| if (items.Count == 0) break; | |
| albumAssets.AddRange(items); | |
| if (items.Count < pageSize) break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs` around lines 17 - 19, The
albumAssets query in AlbumAssetsPool is not paginated and only retrieves the
first page of results, causing large albums to have truncated assets. Implement
pagination similar to the FavoriteAssetsPool, PeopleAssetsPool, and
TagAssetsPool implementations by wrapping the immichApi.SearchAssetsAsync call
in a loop that increments the Page parameter on MetadataSearchDto until all
pages are retrieved. Apply the same pagination fix to AssetHelper.cs where the
identical issue exists.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Refactor
Tests