feat: support selecting photos from tags #537
Conversation
Personally I'd prefer to use tags to select which photos show up on my frame, rather than adding photos to an album.
📝 WalkthroughWalkthroughThis PR adds tag-based asset filtering and display to ImmichFrame. It introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as PooledImmichFrameLogic
participant Pool as TagAssetsPool
participant API as ImmichApi
participant Cache as IApiCache
rect rgba(200, 220, 240, 0.3)
Note over Client,Cache: Asset Loading (when tags configured)
end
Client->>Pool: LoadAssets(cancellationToken)
activate Pool
Note over Pool: For each tag in accountSettings.Tags
rect rgba(240, 220, 200, 0.3)
Note over Pool,API: Pagination Loop
end
loop For each page (batch 1000)
Pool->>API: SearchAssetsAsync(MetadataSearchDto<br/>with TagIds, page, size)
activate API
API-->>Pool: AssetResponseDto[]
deactivate API
loop For each returned asset
alt asset.Tags is null
Pool->>API: GetAssetDetailsAsync(assetId)
activate API
API-->>Pool: Full AssetResponseDto
deactivate API
end
Pool->>Pool: Add to accumulated assets
end
alt totalAssets > batch size
Note over Pool: Continue to next page
else totalAssets ≤ batch size
Note over Pool: Complete
end
end
Pool-->>Client: IEnumerable\<AssetResponseDto\>
deactivate Pool
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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.
| private AssetResponseDto CreateAsset(string id) => new AssetResponseDto { Id = id, Type = AssetTypeEnum.IMAGE, Tags = new List<TagResponseDto>() }; | ||
| private SearchResponseDto CreateSearchResult(List<AssetResponseDto> assets, int total) => | ||
| new SearchResponseDto { Assets = new SearchAssetResponseDto { Items = assets, Total = total } }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add test coverage for the GetAssetInfoAsync code path.
The CreateAsset helper always initializes Tags as an empty list (not null), so the tests never exercise the code path in TagAssetsPool.LoadAssets where asset.Tags == null triggers a call to GetAssetInfoAsync to fetch full asset details. This is a critical path that should be tested.
🔎 Suggested test case for null Tags scenario
Add a test method to verify the GetAssetInfoAsync behavior:
[Test]
public async Task LoadAssets_WhenAssetTagsIsNull_CallsGetAssetInfoAsync()
{
// Arrange
var tagId = Guid.NewGuid();
_mockAccountSettings.SetupGet(s => s.Tags).Returns(new List<Guid> { tagId });
var assetWithNullTags = new AssetResponseDto
{
Id = "asset_1",
Type = AssetTypeEnum.IMAGE,
Tags = null // Null tags to trigger GetAssetInfoAsync
};
var fullAssetInfo = new AssetResponseDto
{
Id = "asset_1",
Type = AssetTypeEnum.IMAGE,
Tags = new List<TagResponseDto> { new TagResponseDto { Id = tagId.ToString() } },
ExifInfo = new ExifResponseDto(),
People = new List<PersonResponseDto>()
};
_mockImmichApi.Setup(api => api.SearchAssetsAsync(It.IsAny<MetadataSearchDto>(), It.IsAny<CancellationToken>()))
.ReturnsAsync(CreateSearchResult(new List<AssetResponseDto> { assetWithNullTags }, 1));
_mockImmichApi.Setup(api => api.GetAssetInfoAsync(new Guid("asset_1"), null, It.IsAny<CancellationToken>()))
.ReturnsAsync(fullAssetInfo);
// Act
var result = (await _tagAssetsPool.TestLoadAssets()).ToList();
// Assert
Assert.That(result.Count, Is.EqualTo(1));
_mockImmichApi.Verify(api => api.GetAssetInfoAsync(new Guid("asset_1"), null, It.IsAny<CancellationToken>()), Times.Once);
Assert.That(result[0].Tags, Is.Not.Null);
Assert.That(result[0].ExifInfo, Is.Not.Null);
Assert.That(result[0].People, Is.Not.Null);
}🤖 Prompt for AI Agents
In ImmichFrame.Core.Tests/Logic/Pool/TagAssetsPoolTests.cs around lines 44-46,
the CreateAsset helper always initializes Tags to an empty list so tests never
exercise the TagAssetsPool.LoadAssets branch that detects asset.Tags == null and
calls GetAssetInfoAsync; add a new test that arranges an AssetResponseDto with
Tags = null, configures _mockImmichApi.SearchAssetsAsync to return that asset,
configures _mockImmichApi.GetAssetInfoAsync to return a full AssetResponseDto
(with Tags, ExifInfo, People populated), invokes the pool loader
(TestLoadAssets), and asserts GetAssetInfoAsync was called once and the returned
asset has non-null Tags, ExifInfo and People.
1 participant
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
✏️ Tip: You can customize this high-level summary in your review settings.