Configurable asset fetch batch size#662
Closed
Zaida-3dO wants to merge 2 commits into
Closed
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds a new ChangesAssetBatchSize configuration and usage
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 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 `@ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs`:
- Around line 139-141: GeneralSettingsV1Adapter.Validate() is currently empty,
so legacy V1 configs can bypass the AssetBatchSize 1–1000 bounds and pass
invalid values through the adapter. Update the Validate method in
GeneralSettingsV1Adapter to apply the same clamping logic used by
GeneralSettings for AssetBatchSize, or refactor the shared validation into a
common helper that both paths call.
🪄 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: 7bf6efc7-f749-4ef2-97d2-c3c1819cbffb
📒 Files selected for processing (11)
.gitignoreImmichFrame.Core/Interfaces/IServerSettings.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/ServerSettings.csdocker/Settings.example.jsondocker/Settings.example.ymldocs/docs/getting-started/configuration.md
Comment on lines
+139
to
141
| public int AssetBatchSize => _delegate.AssetBatchSize; | ||
|
|
||
| public void Validate() { } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the surrounding code.
git ls-files | rg 'ImmichFrame\.WebApi/(Helpers/Config/ServerSettingsV1\.cs|Models/ServerSettings\.cs|.*PooledImmichFrameLogic.*|.*Settings.*)'
printf '\n--- ServerSettingsV1.cs ---\n'
wc -l ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
cat -n ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs | sed -n '1,220p'
printf '\n--- ServerSettings.cs ---\n'
wc -l ImmichFrame.WebApi/Models/ServerSettings.cs
cat -n ImmichFrame.WebApi/Models/ServerSettings.cs | sed -n '1,260p'
printf '\n--- Search for AssetBatchSize and Validate usages ---\n'
rg -n "AssetBatchSize|Validate\(\)|GetAssets\(" ImmichFrame.WebApi -g '!**/bin/**' -g '!**/obj/**'Repository: immichFrame/ImmichFrame
Length of output: 15652
Clamp AssetBatchSize in the V1 adapter
GeneralSettingsV1Adapter.Validate() is still empty, so legacy V1 configs bypass the 1–1000 clamp and can send invalid sizes to Immich’s /search/random. Add the same clamp here, or share the validation with GeneralSettings.
🤖 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/Helpers/Config/ServerSettingsV1.cs` around lines 139 -
141, GeneralSettingsV1Adapter.Validate() is currently empty, so legacy V1
configs can bypass the AssetBatchSize 1–1000 bounds and pass invalid values
through the adapter. Update the Validate method in GeneralSettingsV1Adapter to
apply the same clamping logic used by GeneralSettings for AssetBatchSize, or
refactor the shared validation into a common helper that both paths call.
Replace the hardcoded batch size of 25 in PooledImmichFrameLogic.GetAssets() with a configurable AssetBatchSize general setting (default 25). - Add AssetBatchSize to IGeneralSettings and GeneralSettings (V2 config) and the legacy V1 flat config (ServerSettingsV1 + adapter) - Clamp to Immich's random-search range of 1-1000 at the point of use in GetAssets(), so every config source (V1 and V2) stays within bounds - Document in configuration.md and the docker example settings files - Update config-loader test resources for the new property
Zaida-3dO
force-pushed
the
feature/configurable-asset-batch-size
branch
from
a48f13a to
d1f0ce3
Compare
Author
|
I just realized that this assumption is false if you use filters
|
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.
Assets are served from Immich via a stateless random search (
SearchRandomAsync). Each batch returns 25 distinct assets, but there is no cross-batch "already shown" tracking — so once the frontend exhausts a batch and refetches, the next random draw can repeat images the viewer just saw. This means that currently, though I have hundreds of images in my album, I still get repetitions every 2 weeks or so.Change summary
Makes the number of assets fetched per request configurable via a new
AssetBatchSizegeneral setting (default 25, clamped to 1–1000), replacing the hardcoded 25 inPooledImmichFrameLogic.GetAssets().Summary by CodeRabbit
General.AssetBatchSizesetting to control how many assets are fetched per request (default: 25), with matching updates to example Docker settings.General.AssetBatchSize.