updates#655
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an authenticated ChangesMemory Assets Feature
Repository Support Files
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AssetController
participant MultiImmichFrameLogicDelegate
participant PooledImmichFrameLogic
participant MemoryAssetsPool
participant TotalAccountImagesSelectionStrategy
Client->>AssetController: GET api/Asset/Memories
AssetController->>MultiImmichFrameLogicDelegate: GetMemoryAssets()
MultiImmichFrameLogicDelegate->>PooledImmichFrameLogic: GetMemoryAssets() per account
PooledImmichFrameLogic->>MemoryAssetsPool: GetAssetGroups()
MemoryAssetsPool-->>PooledImmichFrameLogic: Grouped assets
MultiImmichFrameLogicDelegate->>TotalAccountImagesSelectionStrategy: RecordAssetLocation(account, assetId)
MultiImmichFrameLogicDelegate-->>AssetController: Account-scoped groups
AssetController-->>Client: List<List<AssetResponseDto>>
Possibly related PRs
Suggested labels: 🚥 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
| 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.
| 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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Dockerfile`:
- Line 52: Replace the hardcoded revision in the Docker image metadata with a
build argument: declare the appropriate REVISION argument and use its value for
the org.opencontainers.image.revision label, so each build reports its source
commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| org.opencontainers.image.url="https://github.com/immichFrame/ImmichFrame" \ | ||
| org.opencontainers.image.licenses="GPL-3.0" \ | ||
| org.opencontainers.image.version="v${VERSION}" \ | ||
| org.opencontainers.image.revision="d8bcf4f" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Derive the revision label from the build commit.
d8bcf4f is hardcoded, so every image built from another commit will report an incorrect source revision. Pass the Git SHA through a build argument (for example, ARG REVISION) and use ${REVISION} here.
🤖 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 `@Dockerfile` at line 52, Replace the hardcoded revision in the Docker image
metadata with a build argument: declare the appropriate REVISION argument and
use its value for the org.opencontainers.image.revision label, so each build
reports its source commit.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@templates/immichframe.xml`:
- Line 16: Update the TemplateURL value in the immichframe template to reference
the canonical immichFrame/ImmichFrame branch rather than the contributor’s
dd-edits branch, while preserving the existing raw GitHub template path.
- Around line 20-21: Update the ImmichServerUrl and ApiKey Config entries to use
Required="false", allowing the container to be created or updated with only the
file-based /app/Config configuration. Keep their existing defaults,
descriptions, and masking behavior unchanged.
🪄 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: 9264220d-7776-434a-a3c4-951efa72b05b
📒 Files selected for processing (1)
templates/immichframe.xml
| <Description>Configure ImmichFrame with either a Settings.json/Settings.yml file mounted at /app/Config or the variables below. Do not use both methods: a settings file takes precedence.</Description> | ||
| <Category>MediaApp:Other</Category> | ||
| <WebUI>http://[IP]:[PORT:8080]</WebUI> | ||
| <TemplateURL>https://raw.githubusercontent.com/davayd/ImmichFrame/dd-edits/templates/immichframe.xml</TemplateURL> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the canonical template URL.
TemplateURL points to the contributor’s dd-edits branch, which can be deleted, rebased, or diverge from the released template. Point this at the canonical immichFrame/ImmichFrame branch instead.
🤖 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 `@templates/immichframe.xml` at line 16, Update the TemplateURL value in the
immichframe template to reference the canonical immichFrame/ImmichFrame branch
rather than the contributor’s dd-edits branch, while preserving the existing raw
GitHub template path.
| <Config Name="ImmichServerUrl" Target="ImmichServerUrl" Default="" Mode="" Description="The URL of your Immich server, e.g. https://photos.yourdomain.com or http://192.168.0.100:2283" Type="Variable" Display="always" Required="true" Mask="false"></Config> | ||
| <Config Name="ApiKey" Target="ApiKey" Default="" Mode="" Description="Your Immich API key." Type="Variable" Display="always" Required="true" Mask="true"></Config> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,120p' templates/immichframe.xmlRepository: immichFrame/ImmichFrame
Length of output: 8086
🌐 Web query:
Unraid Docker template Required="true" variable empty default settings file path mount optional config behavior
💡 Result:
In Unraid Docker templates, the behavior of fields marked with Required="true" varies depending on the type of configuration and whether the field is left empty by the user. Environment Variables: When an environment variable is marked Required="true", the Unraid WebGUI typically requires a value to be entered before the container can be created or updated [1]. However, if a user provides an empty string (or leaves it blank in the UI), Unraid may still pass that empty value to the Docker engine [2][3]. This can lead to unexpected behavior if the application inside the container treats an empty string as a valid, non-null override of a default setting [2]. It is a known issue that some templates or deployment tools pass these empty values rather than omitting the variable entirely [2]. Path Mounts (Volumes): For path mappings (defined in the XML as <Config Type="Path"...>), the Required="true" attribute enforces that a host path must be defined [4]. If the host path field is left empty or is not properly populated, the Unraid Docker manager may generate an invalid Docker command (e.g., resulting in "empty section between colons" errors, such as -v '':'/container/path') [3]. This will cause the container creation process to fail because the underlying Docker run command becomes malformed [3]. Best Practices for Optional Configuration: 1. Use Required="false": If a variable or path is truly optional, set Required="false" in the XML template [5]. This signals to the Unraid WebGUI that the field is not mandatory. 2. Template Design: If you are creating a template, avoid hard-coding Required="true" for fields that are intended to be optional [5]. 3. Handling Empty Defaults: If a setting is optional, it is safer to leave the Default attribute empty or provide a sensible fallback value if the application supports it [1][5]. 4. Manual Removal: If a container fails to start due to an empty path or variable, users can often edit the container in "Advanced View" within the Unraid WebGUI to remove the problematic entry or provide a valid path [6][3].
Citations:
- 1: https://selfhosters.net/docker/templating/templating/
- 2: fix: ignore empty environment variables to fix provider selection on Unraid LearningCircuit/local-deep-research#3365
- 3: https://forums.unraid.net/bug-reports/stable-releases/6128-docker-container-wont-create-when-emty-hostpath-is-in-template-r2845/
- 4: https://forums.unraid.net/topic/38619-docker-template-xml-schema/
- 5: https://cookiecode.dev/unraid/unraid-create-custom-docker-template.html
- 6: https://github.com/binhex/documentation/blob/master/docker/faq/unraid.md
🌐 Web query:
Unraid Docker template Required=true environment variable must be entered before create update official docs
💡 Result:
In the context of Unraid Docker templates, setting Required="true" within a configuration entry (e.g., Variable, Port, Path) signifies that the Unraid web interface will treat that field as mandatory [1][2]. When a template developer defines a configuration entry with Required="true", the Unraid web GUI adds a validation requirement to that field [3]. Consequently, when a user attempts to create or update the Docker container via the web GUI, the interface will enforce that the field cannot be left empty [2]. If the field is left blank, the system will typically prevent the container from being created or updated until a value is provided [2][3]. This functionality is primarily used by template authors to ensure that critical configuration parameters—such as API keys, mandatory paths, or specific port mappings—are explicitly set by the end user before the container is launched [4][1]. Key takeaways: - Implementation: Developers define this in the template XML using the attribute Required="true" within a Config node [1]. - User Experience: The Unraid web GUI automatically detects this attribute and marks the field as required, preventing submission of the form if the field is empty [3]. - Purpose: It ensures essential settings are not overlooked during container setup [4]. If you are a user encountering a required field, you must provide a valid value in that field within the Docker container's edit screen before the Apply button will function successfully [5][6]. If you are a template developer, ensure you include this attribute only for parameters that are strictly necessary for the container to function, as overly restrictive requirements can impede user setup [2].
Citations:
- 1: https://cookiecode.dev/unraid/unraid-create-custom-docker-template.html
- 2: https://forums.unraid.net/topic/38619-docker-template-xml-schema/
- 3: https://github.com/unraid/webgui/blob/e0e86710/emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php
- 4: https://selfhosters.net/docker/templating/templating/
- 5: https://github.com/binhex/documentation/blob/master/docker/faq/unraid.md
- 6: https://docs.unraid.net/unraid-os/manual/docker-management/
Make the ImmichServerUrl and ApiKey variables optional
Required="true" makes Unraid treat these as mandatory fields, so the container can’t be created/updated with only /app/Config set. Set both to Required="false" or document that file-based config still needs placeholder values.
🤖 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 `@templates/immichframe.xml` around lines 20 - 21, Update the ImmichServerUrl
and ApiKey Config entries to use Required="false", allowing the container to be
created or updated with only the file-based /app/Config configuration. Keep
their existing defaults, descriptions, and masking behavior unchanged.
1 participant
Summary by CodeRabbit
api/Asset/Memoriesendpoint that returns memory assets in grouped sections.