feat: support selecting photos from tags #537
Conversation
📝 WalkthroughWalkthroughAdds tag-based asset collection and display: new TagAssetsPool aggregates assets by configured account 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
Note over Orchestrator,Settings: Trigger when account has configured Tags
Orchestrator->>Pool: LoadAssets(ct)
activate Pool
Pool->>Settings: read Tags
Pool->>Cache: TryGetAllTags()
alt cache miss
Pool->>API: GetAllTagsAsync()
API-->>Pool: all tags
Pool->>Cache: StoreAllTags(...)
else cache hit
Cache-->>Pool: tags
end
loop per configured tag value
Pool->>API: SearchAssetsAsync(MetadataSearchDto{TagIds, Page, Size=1000, Type=IMAGE, WithExif, WithPeople})
API-->>Pool: Asset page (items, total)
loop per asset in page
alt asset lacks Tags
Pool->>API: GetAssetInfoAsync(assetId)
API-->>Pool: AssetResponseDto
end
Pool-->>Pool: dedupe by Id & attach tag
end
end
Pool-->>Orchestrator: aggregated assets
deactivate Pool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (21)
🚧 Files skipped from review as they are similar to previous changes (12)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-09T16:12:49.488ZApplied to files:
🧬 Code graph analysis (2)ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (3)
ImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs (1)
🔇 Additional comments (19)
✏️ Tip: You can disable this entire section by setting 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.
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.
|
No rush, but just in case it wasn't clear, this should be ready for another round of review :) |
JW-CH
left a comment
JW-CH
left a comment
There was a problem hiding this comment.
Could you run make api in the console to generate the swagger.json. Make sure the app is running and do it in another console :) also add the topmost line again. If you don't know what I want I can also do it myself :P
JW-CH
left a comment
JW-CH
left a comment
There was a problem hiding this comment.
A case-insensitive test would be great too. Great job! I'll need to do some more testing after :)
Also, just because I am curious, did you use Claude code to do this? Claude really does a good job but sometimes makes the mistakes I mentioned. (loop over tags etc.)
| continue; | ||
| } | ||
|
|
||
| if (asset.Tags == null) |
There was a problem hiding this comment.
Why is this checked for null? Tags should not be null here if you search like this no?
There was a problem hiding this comment.
I think this is actually a bug in Immich:
immich-app/immich#17361
Even though we're filtering by tag id, the tags aren't being returned in the response of SearchAssetsAsync, so another call is needed to get them.
But I think this was written back when I was using tag ids in the configuraiton. Now that we're specifying the tag value directly, we don't really need to get all the tags and we could just display that. The caveat is that in this current implementation, all tags for an asset are displayed, but if we drop the call to GetAssetInfoAsync, we'd only display the configured tag.
Personally I think this is fine and makes sense, so I'll go ahead and make that change. If you disagree I can revert it back.
There was a problem hiding this comment.
Bug might be the wrong word, seems more like WithTags just isn't implemented yet.
| { | ||
| Page = page, | ||
| Size = batchSize, | ||
| TagIds = [tagId], |
There was a problem hiding this comment.
the search can take multiple tags, why looping over and just take one?
There was a problem hiding this comment.
Just saw that the person pool does not do this too, but you could :)
There was a problem hiding this comment.
Given #537 (comment) I think I now actually do need to loop over them one by one. Otherwise there would be no way to know which tag to display as we aren't calling GetAssetInfoAsync anymore.
I'll publish this change soon and you can decide which one you prefer.
ahal
force-pushed
the
ahal/push-oyxrsrzmqrnw
branch
from
4c2529c to
8842872
Compare
|
Thanks for the great review! I've addressed the comments I could, and guessed what you'd prefer to do about Personally I like this because calling |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs`:
- Line 13: The dictionary tagValueToTag is created with ToDictionary(t =>
t.Value) which is case-sensitive and contradicts the documented case-insensitive
matching; change the creation to use a case-insensitive comparer (e.g.,
allTags.ToDictionary(t => t.Value, StringComparer.OrdinalIgnoreCase)) so lookups
like "vacation" will match "Vacation" (and if duplicate tag values differing
only by case are possible, handle them first—e.g., dedupe or use GroupBy—before
building tagValueToTag).
🧹 Nitpick comments (1)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (1)
17-23: Consider logging unmatched tag values.When a configured tag value doesn't match any tag in Immich, it's silently ignored. While this is consistent with how other pools handle missing albums/people, adding a warning log could help users diagnose configuration issues.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
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.tsopenApi/swagger.json
🚧 Files skipped from review as they are similar to previous changes (7)
- ImmichFrame.WebApi/Models/ClientSettingsDto.cs
- immichFrame.Web/src/lib/components/elements/imageoverlay/image-overlay.svelte
- immichFrame.Web/src/lib/components/elements/image.svelte
- immichFrame.Web/src/lib/immichFrameApi.ts
- immichFrame.Web/src/lib/components/home-page/home-page.svelte
- ImmichFrame.WebApi.Tests/Resources/TestV1.json
- ImmichFrame.WebApi.Tests/Resources/TestV2.yml
🧰 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 (2)
ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (4)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (1)
TagAssetsPool(6-69)ImmichFrame.Core/Logic/Pool/AllAssetsPool.cs (1)
AllAssetsPool(6-78)ImmichFrame.Core/Logic/Pool/AlbumAssetsPool.cs (1)
AlbumAssetsPool(7-29)ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (2)
s(56-56)Test(74-85)
ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs (4)
ImmichFrame.Core.Tests/Logic/Pool/MemoryAssetsPoolTests.cs (3)
List(30-46)List(48-62)SetUp(21-28)ImmichFrame.Core.Tests/Logic/Pool/CachingApiAssetsPoolTests.cs (3)
List(62-72)SetUp(38-60)c(48-51)ImmichFrame.Core.Tests/Logic/Pool/AlbumAssetsPoolTests.cs (1)
SetUp(29-39)ImmichFrame.Core.Tests/Logic/Pool/FavoriteAssetsPoolTests.cs (1)
SetUp(33-40)
🔇 Additional comments (24)
ImmichFrame.Core/Interfaces/IServerSettings.cs (2)
25-25: LGTM!The
Tagsproperty is correctly typed asList<string>to hold tag values (hierarchical paths) rather than GUIDs, which aligns with the agreed-upon approach from the review discussion. The placement afterPeoplemaintains logical grouping of filter criteria.
52-52: LGTM!The
ShowTagsDescproperty follows the established pattern of similar display settings (ShowPeopleDesc,ShowAlbumName).docs/docs/getting-started/configuration.md (4)
77-78: LGTM!Documentation for
ShowTagsDescis clear and follows the established pattern of other display settings.
135-138: LGTM!The examples clearly demonstrate both simple and hierarchical tag values, matching the implementation approach.
149-155: Clear documentation of the tag filtering behavior.The explanation of hierarchical paths, case-insensitive matching, and automatic ID resolution is helpful for users. This addresses the earlier review discussion about using tag values instead of UUIDs.
192-192: LGTM!The
tag.readpermission is correctly documented as required for the new tag functionality.ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (1)
25-64: LGTM!The pagination and deduplication logic is correctly implemented:
seenIdsHashSet prevents duplicate assets across tagsitemsInPage == batchSizecorrectly determines if more pages exist- Each asset gets the matched tag attached since
WithTagsisn't supported by the APIImmichFrame.Core/Logic/PooledImmichFrameLogic.cs (2)
38-38: LGTM!The condition correctly includes
Tags.Any()to ensureAllAssetsPoolis only used when no specific filters (favorites, memories, albums, people, or tags) are configured.
57-58: LGTM!The
TagAssetsPoolintegration follows the established pattern used by other pool types. The pool is correctly added to theMultiAssetPoolwhen tags are configured.docker/Settings.example.json (1)
59-63: LGTM!The example demonstrates both simple (
"Vacation") and hierarchical ("Travel/Europe") tag values, providing clear guidance to users. The placement afterPeopleis consistent with the interface definition.openApi/swagger.json (1)
824-826: LGTM!The
showTagsDescfield addition follows the existing pattern for boolean display flags and is logically grouped withshowPeopleDesc.docker/Settings.example.yml (1)
55-57: LGTM!The Tags example includes both a simple tag (
Vacation) and a hierarchical subtag (Travel/Europe), which demonstrates the supported syntax effectively.ImmichFrame.WebApi.Tests/Resources/TestV2.json (1)
59-62: LGTM!Test data for both accounts follows the established naming convention (
*_TESTsuffix) and placement pattern.Also applies to: 83-86
ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs (1)
37-37: LGTM!The mock setup for
Tagsfollows the established pattern for otherIAccountSettingsproperties and provides an appropriate default empty list for tests not specifically targeting tag functionality.ImmichFrame.WebApi/Models/ServerSettings.cs (2)
53-53: LGTM!The
ShowTagsDescproperty follows the same pattern asShowPeopleDescwith a default oftrue, ensuring tag descriptions are visible by default.
93-93: LGTM!The
Tagsproperty asList<string>correctly supports hierarchical tag values (e.g.,Travel/Europe) and defaults to an empty list for accounts without tag filtering. Interface contract is satisfied — Tags property exists in IServerSettings with matching signature.ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
23-23: LGTM!The new
TagsandShowTagsDescproperties are correctly added toServerSettingsV1and properly exposed through the adapter classes, following the established patterns for similar properties likeAlbums,People, andShowPeopleDesc.Also applies to: 43-43, 91-91, 118-118
immichFrame.Web/src/lib/components/elements/image-component.svelte (1)
25-25: LGTM!The
showTagsDescprop is correctly added to the interface, destructured with a sensible default, and consistently propagated to all threeImagecomponent instances across the split and non-split rendering branches.Also applies to: 44-44, 93-93, 110-110, 129-129
ImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs (4)
14-49: Well-structured test setup.The
TestableTagAssetsPoolwrapper pattern is appropriate for testing protected methods. The setup correctly configures mocks and the helper methods provide clean asset/result creation.
52-90: Good pagination and multi-tag coverage.This test effectively validates that the pool correctly handles pagination across multiple tags and aggregates results from all configured tags.
164-209: Excellent optimization test.This test properly verifies that when
SearchAssetsAsyncreturns assets withTags = nullbut includesExifInfoandPeople, the implementation attaches tags directly rather than making additionalGetAssetInfoAsynccalls. This confirms the optimization is working as intended.
286-328: Deduplication test is well designed.The test correctly validates that assets appearing under multiple tags are deduplicated in the final result, ensuring each asset is only included once regardless of how many configured tags it matches.
immichFrame.Web/src/lib/components/elements/asset-info.svelte (2)
7-7: LGTM!The
mdiTagimport,showTagsDescprop, andavailableTagsderivation follow the established patterns used for people and albums metadata handling.Also applies to: 16-16, 28-28, 70-70
73-73: Tag rendering block is well implemented.The conditional rendering for tags correctly mirrors the pattern used for albums and people, with proper null/empty checks and consistent icon styling.
Also applies to: 105-110
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
ahal
force-pushed
the
ahal/push-oyxrsrzmqrnw
branch
from
8842872 to
e0b20c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/docs/getting-started/configuration.md`:
- Around line 135-138: Update the documentation header that currently reads
"case-insensitive" to accurately state "case-sensitive" so it matches the
implementation and existing docs (e.g., the "Matching is case-sensitive" line
later) and the unit test
TagAssetsPoolTests.cs::LoadAssets_CaseSensitiveTagMatching_OnlyMatchesExactCase
which confirms case-sensitive matching; ensure the Tags example remains the same
but the descriptive phrase is corrected to "case-sensitive".
🧹 Nitpick comments (2)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (2)
15-23: Consider logging when configured tags are not found.If a user misconfigures a tag value (e.g., typo), it silently gets skipped with no indication that something is wrong. This could lead to confusion when expected photos don't appear.
💡 Suggested improvement to add logging for unmatched tags
// Find the tags for the configured tag values var tags = new List<TagResponseDto>(); foreach (var tagValue in accountSettings.Tags) { if (tagValueToTag.TryGetValue(tagValue, out var tag)) { tags.Add(tag); } + else + { + // Consider logging: $"Configured tag '{tagValue}' not found on server {accountSettings.ImmichServerUrl}" + } }
50-60: Minor: Simplify using HashSet.Add return value.
HashSet.Addreturnsfalseif the item already exists, eliminating the need for a separateContainscheck.♻️ Proposed simplification
foreach (var asset in tagInfo.Assets.Items) { - if (seenIds.Contains(asset.Id)) + if (!seenIds.Add(asset.Id)) { continue; } // SearchAssetsAsync does not support a `WithTags` // parameter, so simply set the one that was configured asset.Tags = new List<TagResponseDto> { tag }; - seenIds.Add(asset.Id); tagAssets.Add(asset); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
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.tsopenApi/swagger.json
🚧 Files skipped from review as they are similar to previous changes (8)
- immichFrame.Web/src/lib/immichFrameApi.ts
- ImmichFrame.WebApi.Tests/Resources/TestV2.yml
- immichFrame.Web/src/lib/components/home-page/home-page.svelte
- ImmichFrame.Core/Interfaces/IServerSettings.cs
- ImmichFrame.WebApi.Tests/Resources/TestV1.json
- ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs
- immichFrame.Web/src/lib/components/elements/imageoverlay/image-overlay.svelte
- docker/Settings.example.json
🧰 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/Pool/TagAssetsPool.csImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs
🧬 Code graph analysis (2)
ImmichFrame.WebApi/Models/ClientSettingsDto.cs (2)
ImmichFrame.WebApi/Controllers/ConfigController.cs (2)
ApiController(7-33)ImmichFrame(5-34)ImmichFrame.WebApi/Helpers/SettingsExtensonMethods.cs (1)
IConfigSettable(6-9)
ImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs (2)
ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (2)
TagAssetsPool(6-69)Task(8-68)ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (1)
TestablePersonAssetsPool(22-31)
🔇 Additional comments (20)
ImmichFrame.WebApi.Tests/Resources/TestV2.json (1)
59-62: LGTM!The Tags arrays are correctly added to both test accounts, following the established pattern for account-level configuration in this test fixture. The string-based tag names align with the feature's design for case-insensitive tag matching.
Also applies to: 83-86
immichFrame.Web/src/lib/components/elements/image.svelte (1)
18-18: LGTM!The
showTagsDescprop is correctly added to the Props interface, destructured, and forwarded to theAssetInfochild component. The implementation follows the established pattern for similar display toggle props (showPeopleDesc,showImageDesc, etc.).Also applies to: 34-34, 185-185
immichFrame.Web/src/lib/components/elements/image-component.svelte (2)
25-25: LGTM!The
showTagsDescprop is correctly defined as optional with a sensible default oftrue, consistent with other display toggle props likeshowPeopleDescandshowImageDesc.Also applies to: 44-44
93-93: LGTM!The
showTagsDescprop is correctly propagated to theImagecomponent across all three render branches (split layout first image, split layout second image, and default non-split layout).Also applies to: 110-110, 129-129
immichFrame.Web/src/lib/components/elements/asset-info.svelte (4)
7-7: LGTM!The
mdiTagicon import is appropriately added alongside the other Material Design icons used in this component.
16-16: LGTM!The
showTagsDescprop andavailableTagsderived state are correctly implemented, following the same pattern asshowPeopleDescandavailablePeople.Also applies to: 28-28, 70-70
73-73: LGTM!The outer visibility condition correctly includes
showTagsDescto ensure the metadata container renders when tags should be displayed.
105-110: LGTM!The tags rendering block follows the established pattern from
showPeopleDescexactly—checking both the toggle and the presence of data, using the appropriate icon, and applying consistent styling withinfo-item,info-text, andshort-textclasses.docker/Settings.example.yml (1)
55-57: LGTM!The Tags example appropriately demonstrates both simple tags (
Vacation) and hierarchical subtags (Travel/Europe), which addresses the previous reviewer feedback. The YAML structure is consistent with other list fields in the example configuration.openApi/swagger.json (1)
824-826: LGTM!The
showTagsDescfield is correctly added to theClientSettingsDtoschema, following the same pattern as other display toggle properties (showPeopleDesc,showAlbumName, etc.).ImmichFrame.Core.Tests/Logic/Pool/AllAssetsPoolTests.cs (1)
37-37: LGTM!The mock setup for
Tagsfollows the established pattern used for other list properties (e.g.,ExcludedAlbums), providing a sensible empty default for tests not focused on tag functionality.ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs (1)
23-23: LGTM!The
TagsandShowTagsDescproperties are correctly added toServerSettingsV1with appropriate defaults, and the adapters properly delegate to the underlying settings. The placement follows the existing organizational pattern (filter properties grouped together, display toggles grouped together).Also applies to: 43-43, 91-91, 118-118
ImmichFrame.WebApi/Models/ClientSettingsDto.cs (1)
19-19: LGTM!The
ShowTagsDescproperty and its mapping inFromGeneralSettingscorrectly follow the established pattern for other display toggle properties, ensuring the setting is properly propagated from server configuration to API clients.Also applies to: 50-50
docs/docs/getting-started/configuration.md (1)
77-78: LGTM!The documentation additions for
ShowTagsDesc, theTagsaccount setting, and thetag.readAPI permission are well-structured and provide clear guidance for users. The examples for hierarchical tag paths are helpful.Also applies to: 192-192
ImmichFrame.WebApi/Models/ServerSettings.cs (2)
53-53: LGTM!The
ShowTagsDescproperty follows the existing pattern established byShowPeopleDescandShowAlbumName, with an appropriate default value oftrue.
93-93: LGTM!The
Tagsproperty follows the established pattern used byAlbumsandPeople, usingList<string>for tag values rather thanList<Guid>since tags are matched by their display value. The empty list default is appropriate.ImmichFrame.Core/Logic/Pool/TagAssetsPool.cs (1)
6-68: Overall implementation looks good.The implementation correctly:
- Caches tag lookups per server URL
- Uses case-sensitive matching (aligned with Immich's behavior)
- Deduplicates assets that appear under multiple configured tags
- Avoids the N+1
GetAssetInfoAsyncpattern by directly attaching the configured tag- Handles pagination properly with the batch size check
ImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs (3)
302-309: Note: Shared object reference is intentional for this test.The test correctly uses the same
sharedAssetobject instance for both tag searches. This accurately simulates the real scenario where the Immich API would return the same asset (by ID) for multiple tag queries. The implementation modifiesasset.Tagsin place, so the first encounter sets the tag and subsequent encounters are skipped via deduplication.
14-328: Comprehensive test coverage for TagAssetsPool.The test suite effectively covers:
- Multi-tag pagination behavior
- Edge cases (no tags, empty tag results)
- API parameter validation (TagIds, WithExif, WithPeople)
- Optimization verification (no GetAssetInfoAsync calls)
- Hierarchical tag matching by full value path
- Case-sensitive tag matching (aligned with Immich behavior)
- Cross-tag asset deduplication
40-40: The mock constructor parameters are correct.ImmichApihas a constructor accepting(string url, System.Net.Http.HttpClient httpClient), so passingnull, nulltoMock<ImmichApi>is the appropriate pattern for testing.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
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
e0b20c8 to
9c7c581
Compare
JW-CH
left a comment
JW-CH
left a comment
There was a problem hiding this comment.
Sorry, more changed needed :)
| { | ||
| if (seenIds.Contains(asset.Id)) | ||
| { | ||
| continue; |
There was a problem hiding this comment.
| continue; | |
| tagAssets.First(a => a.Id == asset.Id).Tags.Add(tag); | |
| continue; |
Something like this is needed to actually display all the tags on an item.
| var result = (await _tagAssetsPool.TestLoadAssets()).ToList(); | ||
|
|
||
| // Assert - Should only include the shared asset once, plus the two unique assets | ||
| Assert.That(result.Count, Is.EqualTo(3)); |
There was a problem hiding this comment.
Doing a check like this seems necessary to me: Assert.That(result.First(x => x.Id == sharedAssetId).Tags.Count, Is.EqualTo(2));
In general, I find the Tests quite big and confusing. Could you check them again? Sorry I am also not too familiar with unit tests, I hope this makes sense. The check above just validates, that an Image can have more than just one tag. (See my other review comment)
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.