updates#655
Open
davayd wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughAdds a ChangesMemory Assets Grouped Endpoint
Supporting Config and Docs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs (1)
195-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlease add an integration test for
/api/Asset/Memories.This PR adds a new authenticated route and a nested response shape, but the new coverage still only exercises
/api/Assetand/api/Asset/RandomImageAndInfo. A small test with one empty memory group and one populated group would protect the controller wiring and the empty-group filtering in this slice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs` around lines 195 - 262, Add an integration test in AssetControllerTests for the authenticated /api/Asset/Memories route, since current coverage only exercises GetAssets and GetRandomImageAndInfo. Create a test that mocks the memory response shape with one empty memory group and one populated group, then call the controller through CreateAuthorizedClient and assert the nested response is wired correctly and the empty group is filtered out. Use the existing test patterns in GetAssets_ReturnsVideoDurationFromImmichMetadata and the AssetController endpoint setup to locate the right place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs`:
- Around line 10-16: `GetAssetGroups()` in `MemoryAssetsPool` bypasses the
shared cached read path and calls `LoadAssetGroups()` directly, so grouped
requests never benefit from the `CachingApiAssetsPool` cache. Update the
grouped-read flow to go through a single cached fetch path used by both
`GetAssetGroups()` and `LoadAssets()`, or add caching around the grouped result
so `SearchMemoriesAsync` and the asset-info fetches are not repeated on every
request.
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs`:
- Around line 78-80: GetMemoryAssets currently creates a new MemoryAssetsPool on
every call, which discards any cached state such as DailyApiCache. Update
PooledImmichFrameLogic to hold a dedicated MemoryAssetsPool instance as a field
(initialized once from _immichApi and AccountSettings) and have GetMemoryAssets
reuse that field instead of constructing a fresh pool in the method body.
---
Nitpick comments:
In `@ImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.cs`:
- Around line 195-262: Add an integration test in AssetControllerTests for the
authenticated /api/Asset/Memories route, since current coverage only exercises
GetAssets and GetRandomImageAndInfo. Create a test that mocks the memory
response shape with one empty memory group and one populated group, then call
the controller through CreateAuthorizedClient and assert the nested response is
wired correctly and the empty group is filtered out. Use the existing test
patterns in GetAssets_ReturnsVideoDurationFromImmichMetadata and the
AssetController endpoint setup to locate the right place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 87909de8-e59e-49bd-b967-3f65fce4919e
📒 Files selected for processing (9)
AGENTS.mdImmichFrame.Core/Interfaces/IImmichFrameLogic.csImmichFrame.Core/Logic/AccountSelection/TotalAccountImagesSelectionStrategy.csImmichFrame.Core/Logic/MultiImmichFrameLogicDelegate.csImmichFrame.Core/Logic/Pool/MemoryAssetsPool.csImmichFrame.Core/Logic/PooledImmichFrameLogic.csImmichFrame.WebApi.Tests/Controllers/AssetControllerTests.csImmichFrame.WebApi/Controllers/AssetController.csNuGet.Config
Comment on lines
+10
to
+16
| public async Task<IEnumerable<IEnumerable<AssetResponseDto>>> GetAssetGroups(CancellationToken ct = default) | ||
| => await LoadAssetGroups(ct); | ||
|
|
||
| protected override async Task<IEnumerable<AssetResponseDto>> LoadAssets(CancellationToken ct = default) | ||
| => (await LoadAssetGroups(ct)).SelectMany(assets => assets); | ||
|
|
||
| private async Task<List<List<AssetResponseDto>>> LoadAssetGroups(CancellationToken ct = default) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Share the grouped read path with the daily cache.
GetAssetGroups() goes straight to LoadAssetGroups(), so callers never hit CachingApiAssetsPool's cached load path. On a busy /Memories client this will replay SearchMemoriesAsync and the follow-up asset-info fetches on every request; please cache the grouped result too, or make this method read from a single cached fetch path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImmichFrame.Core/Logic/Pool/MemoryAssetsPool.cs` around lines 10 - 16,
`GetAssetGroups()` in `MemoryAssetsPool` bypasses the shared cached read path
and calls `LoadAssetGroups()` directly, so grouped requests never benefit from
the `CachingApiAssetsPool` cache. Update the grouped-read flow to go through a
single cached fetch path used by both `GetAssetGroups()` and `LoadAssets()`, or
add caching around the grouped result so `SearchMemoriesAsync` and the
asset-info fetches are not repeated on every request.
Comment on lines
+78
to
+80
| public Task<IEnumerable<IEnumerable<AssetResponseDto>>> GetMemoryAssets() | ||
| { | ||
| return new MemoryAssetsPool(_immichApi, AccountSettings).GetAssetGroups(); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid allocating a new MemoryAssetsPool per request.
This throws away DailyApiCache every time GetMemoryAssets() runs, so even a cached pool instance cannot help here. Reuse a dedicated pool field instead of constructing a fresh pool in the method body.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ImmichFrame.Core/Logic/PooledImmichFrameLogic.cs` around lines 78 - 80,
GetMemoryAssets currently creates a new MemoryAssetsPool on every call, which
discards any cached state such as DailyApiCache. Update PooledImmichFrameLogic
to hold a dedicated MemoryAssetsPool instance as a field (initialized once from
_immichApi and AccountSettings) and have GetMemoryAssets reuse that field
instead of constructing a fresh pool in the method body.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests