feat: support selecting photos from tags #537
Conversation
📝 WalkthroughWalkthroughAdds tag-based asset collection and display: new TagAssetsPool fetches assets by configured tag values; account Changes
Sequence Diagram(s)sequenceDiagram
participant Orchestrator as PooledImmichFrameLogic
participant Pool as TagAssetsPool
participant API as ImmichApi
participant Cache as IApiCache
participant Settings as IAccountSettings
rect rgba(220,240,220,0.25)
Note over Orchestrator,Settings: Trigger when account has configured Tags
end
Orchestrator->>Pool: LoadAssets(ct)
activate Pool
Pool->>Settings: read Tags (List<string>)
Pool->>API: GetAllTagsAsync()
activate API
API-->>Pool: all tags (value→id map)
deactivate API
loop per configured tag value
Pool->>API: SearchAssetsAsync(MetadataSearchDto{TagIds, Page, Size=1000, Type=IMAGE, WithExif, WithPeople})
activate API
API-->>Pool: Asset page (+ Total)
deactivate API
loop per asset
alt missing Tags/Exif/People
Pool->>API: GetAssetInfoAsync(assetId)
activate API
API-->>Pool: Full AssetResponseDto
deactivate API
end
Pool->>Cache: store/retrieve (optional)
Pool-->>Pool: dedupe & accumulate
end
end
Pool-->>Orchestrator: aggregated assets
deactivate Pool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
This PR is AI assisted. I've reviewed and tested it locally. |
![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
🧹 Nitpick comments (4)
docker/Settings.example.json (1)
42-43: Consider commenting out one of the ApiKey options for clarity.Both
ApiKeyandApiKeyFileare shown, but the comment on lines 38-39 states "Exactly one of ApiKey or ApiKeyFile must be set." Consider commenting out one option to avoid confusion.📝 Suggested clarification
"ImmichServerUrl": "REQUIRED", "ApiKey": "super-secret-api-key", - "ApiKeyFile": "/path/to/api.key", + // "ApiKeyFile": "/path/to/api.key",ImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs (3)
76-78: Consider more explicit assertion style.The boolean assertions on lines 76-78 and 109 work correctly but would be more explicit with
Is.True:-Assert.That(result.Any(a => a.Id == "t1_p1_0")); +Assert.That(result.Any(a => a.Id == "t1_p1_0"), Is.True);This makes the intent clearer and provides better failure messages.
Also applies to: 109-109
48-133: Consider adding a test for duplicate assets across tags.The
TagAssetsPoolimplementation doesn't deduplicate assets, so if the same asset is tagged with multiple configured tags, it will appear multiple times in the result. Adding a test to document this behavior (whether intentional or not) would be valuable:[Test] public async Task LoadAssets_AssetInMultipleTags_AppearsMultipleTimes() { // Arrange var tag1Id = Guid.NewGuid(); var tag2Id = Guid.NewGuid(); _mockAccountSettings.SetupGet(s => s.Tags).Returns(new List<Guid> { tag1Id, tag2Id }); // Same asset appears in both tags var sharedAsset = CreateAsset("shared_asset"); _mockImmichApi.Setup(api => api.SearchAssetsAsync( It.Is<MetadataSearchDto>(d => d.TagIds.Contains(tag1Id)), It.IsAny<CancellationToken>())) .ReturnsAsync(CreateSearchResult(new List<AssetResponseDto> { sharedAsset }, 1)); _mockImmichApi.Setup(api => api.SearchAssetsAsync( It.Is<MetadataSearchDto>(d => d.TagIds.Contains(tag2Id)), It.IsAny<CancellationToken>())) .ReturnsAsync(CreateSearchResult(new List<AssetResponseDto> { sharedAsset }, 1)); // Act var result = (await _tagAssetsPool.TestLoadAssets()).ToList(); // Assert // Document expected behavior: duplicates or deduplication? Assert.That(result.Count, Is.EqualTo(2)); // or 1 if deduplication is expected Assert.That(result.Count(a => a.Id == "shared_asset"), Is.EqualTo(2)); // or 1 }
1-133: Optional: Consider adding error handling tests.The current test suite focuses on happy path scenarios. Consider adding tests for error conditions such as:
- API failures (SearchAssetsAsync throws exception)
- Cancellation token triggered mid-operation
- GetAssetInfoAsync failures when asset.Tags is null
These tests would ensure the pool handles failures gracefully.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.csImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.Core/Logic/Pool/TagAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.csImmichFrame.WebApi/Models/ClientSettingsDto.csImmichFrame.WebApi/Models/ServerSettings.csdocker/Settings.example.jsondocker/Settings.example.ymldocs/docs/getting-started/configuration.mdimmichFrame.Web/src/lib/components/elements/asset-info.svelteimmichFrame.Web/src/lib/components/elements/image-component.svelteimmichFrame.Web/src/lib/components/elements/image.svelteimmichFrame.Web/src/lib/components/elements/imageoverlay/image-overlay.svelteimmichFrame.Web/src/lib/components/home-page/home-page.svelteimmichFrame.Web/src/lib/immichFrameApi.ts
🧰 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/TagAssetsPoolTests.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.csImmichFrame.Core/Logic/Pool/TagAssetsPool.cs
🧬 Code graph analysis (4)
ImmichFrame.WebApi/Models/ClientSettingsDto.cs (1)
ImmichFrame.WebApi/Controllers/ConfigController.cs (2)
ApiController(7-33)ImmichFrame(5-34)
ImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs (1)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (2)
TagAssetsPool(6-52)Task(8-51)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (3)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (1)
TagAssetsPool(6-52)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
AllAssetsPool(6-78)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
AlbumAssetsPool(7-29)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (4)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (2)
CachingApiAssetsPool(6-55)Task(20-23)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
AllAssetsPool(6-78)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
FavoriteAssetsPool(6-37)ImmichFrame.Core/Logic/Pool/IAssetPool.cs (1)
IAssetPool(5-36)
🔇 Additional comments (36)
immichFrame.Web/src/lib/components/home-page/home-page.svelte (1)
357-357: LGTM! Clean integration of tag display configuration.The
showTagsDescprop is correctly wired from the config store to the ImageComponent, following the same pattern as other display flags.docker/Settings.example.yml (1)
55-56: LGTM! Configuration format is consistent.The Tags configuration follows the same pattern as Albums and People, maintaining consistency across the settings file.
docker/Settings.example.json (1)
59-62: LGTM! Tags configuration matches the established pattern.The Tags array is correctly added to the account settings, maintaining consistency with the Albums and People configuration structure.
docs/docs/getting-started/configuration.md (3)
77-78: LGTM! Clear and consistent documentation.The ShowTagsDesc setting is well-documented and follows the same format as ShowPeopleDesc and ShowAlbumName.
186-186: Good addition of required API permission.The
tag.readpermission is correctly added to the API Key Permissions list, ensuring users grant the necessary access for tag-based filtering.
135-137: Well-documented tag configuration with helpful API guidance.The Tags configuration is clearly explained and includes practical guidance on retrieving tag UUIDs via the Immich API. The API endpoint
GET /api/tagsis correct and returns tag objects with their IDs as documented. The updated section header appropriately reflects the expanded filtering options.Also applies to: 145-149
immichFrame.Web/src/lib/components/elements/image-component.svelte (2)
25-25: LGTM! Prop definition follows established patterns.The
showTagsDescprop is correctly added to the Props interface with a default value oftrue, consistent withshowPeopleDescandshowAlbumName.Also applies to: 44-44
93-93: LGTM! Complete prop propagation across all render paths.The
showTagsDescprop is correctly passed to the Image component in both split-view and default rendering branches, ensuring consistent behavior.Also applies to: 109-109, 127-127
immichFrame.Web/src/lib/components/elements/asset-info.svelte (4)
7-7: LGTM! Proper import and prop definition.The
mdiTagicon is imported andshowTagsDescprop is correctly added to the component's interface.Also applies to: 16-16, 27-27
68-68: LGTM! Tag filtering logic is correct.The
availableTagsderived value correctly filters tags to only include those with names, matching the pattern used foravailablePeople.
71-71: LGTM! Visibility condition properly updated.The outer conditional correctly includes
showTagsDescto control whether the metadata container should be rendered.
103-108: LGTM! Tags display implementation is consistent and correct.The tags description block follows the established pattern used for people and albums:
- Conditional rendering based on
showTagsDescand tag availability- mdiTag icon for visual consistency
- Comma-separated tag names
- Appropriate element ID "tagsdescription"
immichFrame.Web/src/lib/immichFrameApi.ts (1)
201-201: LGTM! API type correctly updated.The
showTagsDescoptional boolean property is correctly added toClientSettingsDto. Since this file is auto-generated (as noted in the header), the change reflects the API schema update appropriately.ImmichFrame.Core/Interfaces/IServerSettings.cs (2)
25-25: LGTM! Interface extension is clean and consistent.The
Tagsproperty addition toIAccountSettingsfollows the same pattern asAlbumsandPeople, usingList<Guid>for tag identifiers.
52-52: LGTM! Boolean flag follows established convention.The
ShowTagsDescproperty addition toIGeneralSettingsis consistent with other display control flags likeShowPeopleDescandShowAlbumName.ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs (1)
37-37: LGTM!The test setup correctly initializes the Tags property with an empty list, consistent with other collection properties like ExcludedAlbums.
immichFrame.Web/src/lib/components/elements/imageoverlay/image-overlay.svelte (3)
14-14: LGTM!The mdiTag icon import is correctly added to support the new Tags overlay item.
29-29: LGTM!The availableTags derived state correctly filters tags by name, mirroring the pattern used for availablePeople on line 28.
83-89: LGTM!The Tags overlay item follows the same pattern as the People and Album sections. The conditional rendering and icon usage are appropriate.
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (2)
38-38: LGTM!The condition correctly includes Tags in the check for whether to use AllAssetsPool. This ensures that when tags are configured, the appropriate specialized pool is used.
57-58: LGTM!TagAssetsPool is correctly instantiated when tags are configured, following the same pattern as PersonAssetsPool and AlbumAssetsPool.
ImmichFrame.WebApi/Models/ServerSettings.cs (2)
53-53: LGTM!The ShowTagsDesc property is correctly added with an appropriate default value of true, consistent with ShowPeopleDesc on line 52.
93-93: LGTM!The Tags property is correctly initialized as an empty list, following the same pattern as the People property on line 92.
ImmichFrame.WebApi/Models/ClientSettingsDto.cs (2)
19-19: LGTM!The ShowTagsDesc property is correctly positioned in the DTO structure, maintaining consistency with related display flags.
50-50: LGTM!The ShowTagsDesc mapping is correctly implemented in the FromGeneralSettings method, following the established pattern.
immichFrame.Web/src/lib/components/elements/image.svelte (3)
18-18: LGTM!The showTagsDesc property is correctly added to the Props interface, maintaining consistency with other display flags like showPeopleDesc.
33-33: LGTM!The showTagsDesc prop is correctly destructured from $props.
183-183: LGTM!The showTagsDesc prop is correctly passed to the AssetInfo component using Svelte's shorthand syntax.
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (4)
23-23: LGTM!The Tags property is correctly added to ServerSettingsV1, following the same pattern as the People property on line 22.
43-43: LGTM!The ShowTagsDesc property is correctly added with an appropriate default value, consistent with ShowPeopleDesc on line 42.
91-91: LGTM!The Tags property is correctly exposed through the AccountSettingsV1Adapter, maintaining consistency with other adapter properties.
118-118: LGTM!The ShowTagsDesc property is correctly exposed through the GeneralSettingsV1Adapter.
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (2)
12-48: Pagination pattern is consistent and intentional.The
while (total == batchSize)condition is used consistently across all asset pool implementations (TagAssetsPool, PeopleAssetsPool, and FavoriteAssetsPool), wheretotalrepresents the count of items returned in the current page. This causes an extra API call when the result count exactly equals the batch size (e.g., exactly 1000 assets), but this is the designed behavior—the loop continues if a page is full, and exits only when a page returns fewer items than requested. Tests confirm this is the expected behavior.
34-43: Consider optimizing asset enrichment for Tags field to avoid potential N+1 query problem.This code may make individual
GetAssetInfoAsynccalls for each asset whereTags == null, potentially resulting in many additional API calls. With a batch size of 1000, if Tags are frequently null, this could create significant performance overhead.However, comparing to similar pools reveals important context:
PersonAssetsPool(in PeopleAssetsPool.cs) calls the sameSearchAssetsAsyncwithWithExif=trueandWithPeople=true, then directly returns assets without enrichment.FavoriteAssetsPoolfollows the same pattern—no enrichment needed.This suggests
SearchAssetsAsyncdoes not populate the Tags field (unlike ExifInfo and People), so the enrichment may be necessary. But consider:
- What is the typical frequency of null Tags in actual data? If rare, the cost is minimal; if common, consider alternatives.
- Could the API be enhanced to support
WithTagsinMetadataSearchDtoto fetch tags in bulk?- Alternatively, batch the enrichment calls or implement intelligent caching to reduce API overhead.
At minimum, add monitoring/logging to track how often enrichment is triggered to validate the performance impact.
ImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs (2)
22-31: LGTM: Clean test wrapper pattern.The TestableTagAssetsPool wrapper appropriately exposes the protected LoadAssets method for testing.
85-133: LGTM: Comprehensive edge case coverage.These test methods effectively cover important scenarios:
- Empty tag configuration with no API calls
- Tags with no assets not affecting other tags' results
- Correct parameter passing to the search API
The test logic and assertions are sound.
ahal
force-pushed
the
ahal/push-oyxrsrzmqrnw
branch
from
7651aad to
9f2c8d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (1)
15-23: Consider logging when configured tags are not found.If a user misconfigures a tag name (typo or tag was deleted), the current code silently ignores it. This could make debugging difficult when expected photos don't appear. Consider logging a warning for unknown tags.
🔎 Suggested improvement
// Find the tag IDs for the configured tag values var tagIds = new List<Guid>(); foreach (var tagValue in accountSettings.Tags) { if (tagValueToIds.TryGetValue(tagValue, out var id)) { tagIds.Add(id); } + else + { + // Consider logging: configured tag "{tagValue}" not found in Immich + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.csImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.Core/Logic/Pool/TagAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.WebApi.Tests/Resources/TestV1.jsonImmichFrame.WebApi.Tests/Resources/TestV2.jsonImmichFrame.WebApi.Tests/Resources/TestV2.ymlImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.csImmichFrame.WebApi/Models/ClientSettingsDto.csImmichFrame.WebApi/Models/ServerSettings.csdocker/Settings.example.jsondocker/Settings.example.ymldocs/docs/getting-started/configuration.mdimmichFrame.Web/src/lib/components/elements/asset-info.svelteimmichFrame.Web/src/lib/components/elements/image-component.svelteimmichFrame.Web/src/lib/components/elements/image.svelteimmichFrame.Web/src/lib/components/elements/imageoverlay/image-overlay.svelteimmichFrame.Web/src/lib/components/home-page/home-page.svelteimmichFrame.Web/src/lib/immichFrameApi.ts
✅ Files skipped from review due to trivial changes (1)
- ImmichFrame.WebApi.Tests/Resources/TestV2.yml
🚧 Files skipped from review as they are similar to previous changes (10)
- immichFrame.Web/src/lib/components/elements/imageoverlay/image-overlay.svelte
- docker/Settings.example.yml
- immichFrame.Web/src/lib/components/home-page/home-page.svelte
- ImmichFrame.WebApi/Models/ServerSettings.cs
- ImmichFrame.Core/Interfaces/IServerSettings.cs
- ImmichFrame.WebApi/Models/ClientSettingsDto.cs
- immichFrame.Web/src/lib/components/elements/asset-info.svelte
- immichFrame.Web/src/lib/components/elements/image-component.svelte
- docker/Settings.example.json
- immichFrame.Web/src/lib/components/elements/image.svelte
🧰 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/Logic/Pool/TagAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs
🧬 Code graph analysis (2)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (5)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (2)
CachingApiAssetsPool(6-55)Task(20-23)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
AllAssetsPool(6-78)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
AlbumAssetsPool(7-29)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
FavoriteAssetsPool(6-37)ImmichFrame.Core/Logic/Pool/IAssetPool.cs (1)
IAssetPool(5-36)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (1)
TagAssetsPool(6-65)
🔇 Additional comments (21)
immichFrame.Web/src/lib/immichFrameApi.ts (1)
201-201: LGTM! The new field follows the established pattern.The addition of
showTagsDescis consistent with other similar fields (showImageDesc,showPeopleDesc) and correctly typed as an optional boolean. The placement is logical and the syntax is correct.Since this is a generated file (as noted in the header), ensure that the underlying OpenAPI specification was updated before regeneration to maintain consistency between the spec and this client code.
ImmichFrame.WebApi.Tests/Resources/TestV1.json (1)
29-31: LGTM!The Tags test data addition is consistent with the existing test structure and follows the same pattern as other array fields like Albums and People.
ImmichFrame.WebApi.Tests/Resources/TestV2.json (1)
59-62: LGTM!The Tags field additions to both accounts are properly structured and use distinct test values for each account, maintaining consistency with the V2 multi-account configuration format.
Also applies to: 83-86
docs/docs/getting-started/configuration.md (3)
77-78: LGTM!The ShowTagsDesc flag and Tags configuration are well-documented and consistent with the existing People and Albums patterns. The use of tag values (strings) instead of UUIDs aligns with the past discussion and improves user experience.
Also applies to: 135-138
149-155: Excellent documentation addressing past concerns.The filtering documentation clearly explains:
- Using the full hierarchical path (
valuefield) rather than just the tag name- Hierarchical structure with forward slashes
- Case-insensitive matching
- Concrete examples
This directly addresses the concern raised in past review comments about users potentially expecting to use
nameinstead ofvalue.
192-192: LGTM!The
tag.readpermission is a necessary addition for the tags feature and is properly documented alongside other required permissions.ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (3)
23-23: LGTM!The Tags property is correctly defined as
List<string>to store tag values (full paths) rather than GUIDs, which aligns with the design decision to improve user experience by avoiding UUID lookups.
43-43: LGTM!The ShowTagsDesc flag defaults to
true, which is consistent with other similar flags like ShowPeopleDesc and ShowAlbumName.
91-91: LGTM!Both adapter properties correctly delegate to the underlying settings object, maintaining consistency with the existing adapter pattern for other settings.
Also applies to: 118-118
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (1)
38-38: LGTM!The extended condition correctly includes Tags in the check for whether to use AllAssetsPool, maintaining consistency with how Albums and People are handled.
ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs (1)
37-37: LGTM!The mock setup for the new
Tagsproperty follows the existing pattern and provides a sensible default (empty list) for tests that don't specifically test tag functionality.ImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs (9)
22-31: LGTM!The
TestableTagAssetsPoolwrapper pattern is a clean approach to expose the protectedLoadAssetsmethod for testing without modifying the production class visibility.
33-42: LGTM!Setup is minimal and follows the existing test patterns. Individual tests appropriately override the
Tagsmock when needed.
44-46: LGTM!The helpers are well-designed for most test cases. The
LoadAssets_EnrichesAssetsWithNullTags_CallsGetAssetInfoAsynctest (lines 157-206) properly creates an asset withTags = nulldirectly, addressing the code path that the past review comment flagged.
48-86: LGTM!Comprehensive test covering multi-tag search with pagination. The test correctly sets up the pagination scenario where page 1 returns exactly
batchSize(1000) items to trigger page 2, and page 2 returns fewer items to terminate the loop.
88-99: LGTM!Good edge case coverage ensuring that when no tags are configured, the method returns empty without making unnecessary API calls.
101-125: LGTM!Good test for ensuring that a tag with no assets doesn't affect the retrieval of assets from other tags.
127-155: LGTM!Good verification of the
MetadataSearchDtoconstruction, ensuring all required parameters are set correctly.
157-206: LGTM!Excellent test that directly addresses the asset enrichment code path when
Tagsis null. This ensures theGetAssetInfoAsyncfallback is properly tested.
208-244: LGTM!Good test coverage for hierarchical tag matching. This ensures that tags are matched by their full
Value(e.g.,"Parent1/Child") rather than just theName, which is crucial for disambiguating tags with the same name in different hierarchies.ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (1)
6-7: LGTM!The class correctly extends
CachingApiAssetsPooland follows the established patterns in the codebase (similar toFavoriteAssetsPool). The primary constructor syntax keeps the code concise.
| // Fetch full asset details to get tag information | ||
| foreach (var asset in tagInfo.Assets.Items) | ||
| { | ||
| if (asset.Tags == null) | ||
| { | ||
| var assetInfo = await immichApi.GetAssetInfoAsync(new Guid(asset.Id), null, ct); | ||
| asset.Tags = assetInfo.Tags; | ||
| asset.ExifInfo = assetInfo.ExifInfo; | ||
| asset.People = assetInfo.People; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the TagAssetsPool.cs file
find . -name "TagAssetsPool.cs" -type f | head -20Repository: immichFrame/ImmichFrame
Length of output: 113
🏁 Script executed:
# Search for MetadataSearchDto usage to understand what data is returned by the API
rg "MetadataSearchDto|GetAssetInfoAsync" --type=cs -B 3 -A 3 | head -100Repository: immichFrame/ImmichFrame
Length of output: 11523
🏁 Script executed:
# Check FavoriteAssetsPool for comparison patterns
find . -name "FavoriteAssetsPool.cs" -type fRepository: immichFrame/ImmichFrame
Length of output: 118
🏁 Script executed:
# Get a comprehensive view of all GetAssetInfoAsync calls in the codebase
rg "GetAssetInfoAsync" --type=cs -B 2 -A 2Repository: immichFrame/ImmichFrame
Length of output: 5512
🏁 Script executed:
# Read TagAssetsPool.cs around the problematic lines
cat -n ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs | sed -n '40,65p'Repository: immichFrame/ImmichFrame
Length of output: 1096
🏁 Script executed:
# Read FavoriteAssetsPool.cs to compare pattern
cat -n ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs | head -100Repository: immichFrame/ImmichFrame
Length of output: 1452
🏁 Script executed:
# Search for SearchAssetsAsync calls in TagAssetsPool to see WithExif/WithPeople parameters
rg "SearchAssetsAsync" ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs -B 5 -A 5Repository: immichFrame/ImmichFrame
Length of output: 467
🏁 Script executed:
# See the full metadataBody construction in TagAssetsPool to check if WithTags is set
cat -n ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs | sed -n '30,50p'Repository: immichFrame/ImmichFrame
Length of output: 911
🏁 Script executed:
# Check the full TagAssetsPool.cs file to understand the context better
cat -n ImmichFrame.Core/Logic/Pool/TagAssetsPool.csRepository: immichFrame/ImmichFrame
Length of output: 2763
🏁 Script executed:
# Search for MetadataSearchDto to see if there's a WithTags property
rg "WithTags" --type=cs | head -20Repository: immichFrame/ImmichFrame
Length of output: 49
N+1 query pattern: sequential GetAssetInfoAsync calls for tag enrichment.
SearchAssetsAsync doesn't support a WithTags parameter, so tags must be fetched separately for each asset. The sequential GetAssetInfoAsync calls inside the loop are necessary to populate the Tags field. While batching or parallelizing these calls with concurrency limits could reduce latency, the current pattern is not a defensive fallback but a required enrichment step given the API constraints.
🤖 Prompt for AI Agents
In ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs around lines 46 to 56, the
current sequential loop calling GetAssetInfoAsync for each asset causes N+1
latency; change this to perform the enrichment calls concurrently with a bounded
degree of parallelism (use SemaphoreSlim or a Parallel.ForEachAsync pattern) so
you kick off multiple GetAssetInfoAsync requests up to a configurable limit,
await their completion (respecting the cancellation token), and assign
Tags/ExifInfo/People back to each asset while preserving any exceptions and
cancellation behavior.
Personally I'd prefer to use tags to select which photos show up on my frame, rather than adding photos to an album.
ahal
force-pushed
the
ahal/push-oyxrsrzmqrnw
branch
from
9f2c8d4 to
4c2529c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
immichFrame.Web/src/lib/components/elements/asset-info.svelte (1)
73-73: Consider extracting visibility logic to improve readability.The conditional expression is growing long with six boolean checks. While functional, extracting this to a derived variable would improve readability.
🔎 Optional refactoring for readability
+ let shouldShowMetadata = $derived( + showPhotoDate || showLocation || showImageDesc || showPeopleDesc || showTagsDesc || showAlbumName + ); + -{#if showPhotoDate || showLocation || showImageDesc || showPeopleDesc || showTagsDesc || showAlbumName} +{#if shouldShowMetadata}ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (2)
15-23: Consider logging when configured tags aren't found.Tag values that don't match any existing tags are silently skipped. While this is consistent with other pool implementations, it could be helpful for users to log a warning when a configured tag value has no match in Immich.
🔎 Optional enhancement: Add logging for unmatched tags
If a logger is available in the constructor, consider adding:
foreach (var tagValue in accountSettings.Tags) { if (tagValueToIds.TryGetValue(tagValue, out var id)) { tagIds.Add(id); } else { // Optional: log warning about unmatched tag _logger?.LogWarning("Configured tag value '{TagValue}' not found in Immich", tagValue); } }
55-61: Optional: Consider parallel enrichment for better performance.The sequential GetAssetInfoAsync calls work correctly but could benefit from bounded parallelism to reduce latency when many assets need enrichment. Given the "Chill" review mode, this is purely optional and can be deferred.
🔎 Optional performance enhancement
For scenarios with many assets requiring enrichment, consider using
Parallel.ForEachAsyncwith a concurrency limit:var assetsNeedingEnrichment = tagInfo.Assets.Items .Where(a => !seenIds.Contains(a.Id) && a.Tags == null) .ToList(); await Parallel.ForEachAsync(assetsNeedingEnrichment, new ParallelOptions { MaxDegreeOfParallelism = 10, CancellationToken = ct }, async (asset, ct) => { var assetInfo = await immichApi.GetAssetInfoAsync(new Guid(asset.Id), null, ct); asset.Tags = assetInfo.Tags; asset.ExifInfo = assetInfo.ExifInfo; asset.People = assetInfo.People; });Then add all enriched assets to the result set.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.csImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.csImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.Core/Logic/Pool/TagAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.WebApi.Tests/Resources/TestV1.jsonImmichFrame.WebApi.Tests/Resources/TestV2.jsonImmichFrame.WebApi.Tests/Resources/TestV2.ymlImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.csImmichFrame.WebApi/Models/ClientSettingsDto.csImmichFrame.WebApi/Models/ServerSettings.csdocker/Settings.example.jsondocker/Settings.example.ymldocs/docs/getting-started/configuration.mdimmichFrame.Web/src/lib/components/elements/asset-info.svelteimmichFrame.Web/src/lib/components/elements/image-component.svelteimmichFrame.Web/src/lib/components/elements/image.svelteimmichFrame.Web/src/lib/components/elements/imageoverlay/image-overlay.svelteimmichFrame.Web/src/lib/components/home-page/home-page.svelteimmichFrame.Web/src/lib/immichFrameApi.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- ImmichFrame.Core/Interfaces/IServerSettings.cs
- docker/Settings.example.yml
- immichFrame.Web/src/lib/immichFrameApi.ts
- docker/Settings.example.json
- ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
- ImmichFrame.WebApi/Models/ClientSettingsDto.cs
- ImmichFrame.WebApi/Models/ServerSettings.cs
- immichFrame.Web/src/lib/components/home-page/home-page.svelte
- ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs
- immichFrame.Web/src/lib/components/elements/image-component.svelte
🧰 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/Logic/Pool/TagAssetsPool.csImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs
🧬 Code graph analysis (2)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (7)
ImmichFrame.Core/Logic/Pool/CachingApiAssetsPool.cs (2)
CachingApiAssetsPool(6-55)Task(20-23)ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (7)
Task(63-66)Task(68-71)Task(73-73)Task(75-75)Task(77-77)Task(79-135)Task(137-138)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
AllAssetsPool(6-78)ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
PersonAssetsPool(6-40)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
AlbumAssetsPool(7-29)ImmichFrame.Core/Logic/Pool/FavoriteAssetsPool.cs (1)
FavoriteAssetsPool(6-37)ImmichFrame.Core/Logic/Pool/IAssetPool.cs (1)
IAssetPool(5-36)
ImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs (1)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (2)
TagAssetsPool(6-73)Task(8-72)
🔇 Additional comments (18)
immichFrame.Web/src/lib/components/elements/imageoverlay/image-overlay.svelte (1)
14-14: LGTM! Clean tag display implementation.The tag support follows the existing patterns consistently:
- Icon import matches other metadata icons
availableTagsderives state identically toavailablePeoplewith proper optional chaining and filtering- UI rendering uses the same
OverlayItemcomponent pattern and placement is logicalAlso applies to: 29-29, 83-89
ImmichFrame.WebApi.Tests/Resources/TestV1.json (1)
29-31: LGTM! Test data properly updated.The Tags field addition follows the same pattern as Albums and People, with appropriate test values.
immichFrame.Web/src/lib/components/elements/asset-info.svelte (4)
7-7: LGTM! Icon import follows existing pattern.The mdiTag icon import is consistent with other metadata icons used in this component.
16-16: LGTM! Prop definition follows established conventions.The showTagsDesc prop mirrors the pattern used for showPeopleDesc and other metadata visibility controls.
Also applies to: 28-28
70-70: LGTM! Derived tags filtering is consistent.The availableTags derivation uses the same filtering pattern as availablePeople (line 69), ensuring only tags with names are displayed.
105-110: LGTM! Tag rendering implementation is consistent.The tags rendering block follows the exact same pattern as the people description block (lines 99-104), ensuring UI consistency. The use of optional chaining and the filter for non-empty tags is appropriate.
ImmichFrame.WebApi.Tests/Resources/TestV2.yml (1)
54-55: LGTM! Test configuration properly updated.The Tags field additions to both accounts follow the existing test data patterns and are placed logically after the People arrays. The account-prefixed naming is consistent with other TestV2 fields.
Also applies to: 72-73
ImmichFrame.WebApi.Tests/Resources/TestV2.json (1)
59-62: LGTM! Test JSON properly updated.The Tags field additions to both account configurations are syntactically correct and consistent with the TestV2.yml counterpart. The account-specific test values follow established naming patterns.
Also applies to: 83-86
ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (5)
23-23: LGTM! Tags property follows established patterns.The Tags property definition is consistent with the existing Albums and People properties, using the same type (List for Tags vs List for entities) and initialization pattern.
43-43: LGTM! ShowTagsDesc default aligns with similar settings.The ShowTagsDesc property follows the same pattern as ShowPeopleDesc (line 42), with a sensible default value of true to match the feature's opt-out behavior.
91-91: LGTM! Adapter properly delegates Tags property.The AccountSettingsV1Adapter correctly exposes the Tags property following the same delegation pattern used for Albums (line 88) and People (line 90).
118-118: LGTM! GeneralSettings adapter properly delegates ShowTagsDesc.The GeneralSettingsV1Adapter correctly exposes ShowTagsDesc following the same delegation pattern as ShowPeopleDesc (line 117).
23-23: Tags integration is correctly implemented. The Tags property from ServerSettingsV1 is properly consumed by TagAssetsPool, which resolves tag values to IDs via a case-insensitive dictionary lookup from the API's GetAllTagsAsync response. The resolved tag IDs are correctly passed to the asset search query, and comprehensive test coverage validates the integration including hierarchical tag support and asset enrichment.immichFrame.Web/src/lib/components/elements/image.svelte (1)
18-18: LGTM! Clean prop addition following established patterns.The
showTagsDescprop is properly typed, destructured, and forwarded to the AssetInfo component, consistent with other visibility control props in this component.Also applies to: 34-34, 185-185
docs/docs/getting-started/configuration.md (1)
77-78: Excellent documentation of the tag-based filtering feature.The documentation clearly explains:
- The ShowTagsDesc display option
- How to configure Tags using full hierarchical paths (values)
- Case-insensitive matching behavior
- The required
tag.readAPI permissionThe examples are helpful and the hierarchical path explanation addresses potential confusion between tag names and values.
Also applies to: 135-138, 149-155, 192-192
ImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs (1)
1-289: Comprehensive test coverage addressing previous gaps.The test suite thoroughly validates TagAssetsPool behavior including:
- Pagination across multiple tags
- Empty configuration handling
- Asset enrichment when Tags is null (lines 157-206)
- Correct MetadataSearchDto parameter construction
- Hierarchical tag value matching
- Deduplication of assets appearing in multiple tags
The test at lines 157-206 specifically addresses the previous review concern about exercising the GetAssetInfoAsync enrichment path.
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (2)
25-25: Well-implemented deduplication logic.The HashSet-based deduplication correctly prevents assets with multiple configured tags from appearing multiple times in the result set. This addresses the concern raised in previous reviews.
Also applies to: 50-53, 63-64
55-61: Asset enrichment logic is correct.The conditional enrichment via GetAssetInfoAsync when
asset.Tags == nullproperly populates Tags, ExifInfo, and People. This is necessary because SearchAssetsAsync doesn't return tag data even though the search is by tag ID.
2 participants
Personally I'd prefer to use tags to select which photos show up on my frame, rather than adding photos to an album.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.