[FEAT] Introduced RequireAllPeople flag to filter assets for people in an AND fashion#626
Conversation
…l-people-flag [FEAT] Introduced RequireAllPeople flag to filter assets for people in an AND fashion
|
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 ChangesRequireAllPeople Filter Mode
Web UI Asset Error Handling and Animation Improvements
MemoryAssetsPool Date Reference
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
12-23:⚠️ Potential issue | 🟠 MajorGuard against empty
Peoplein AND mode to avoid unintended search calls.At Line 21,
RequireAllPeople=truewith an empty people list creates a single empty group and still executesSearchAssetsAsync. Add an earlyCount == 0return to keep filtering behavior safe and predictable.💡 Proposed fix
var people = accountSettings.People; - if (people == null) + if (people == null || people.Count == 0) { return personAssets; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs` around lines 12 - 23, The code builds personIdGroups from accountSettings.People and in AND mode (accountSettings.RequireAllPeople) it can create a single empty group causing SearchAssetsAsync to run; add an early guard after the null check that if accountSettings.RequireAllPeople is true and accountSettings.People.Count == 0 then return personAssets immediately to avoid performing searches for an empty people list; update the logic around personIdGroups, RequireAllPeople, and the call sites that use personIdGroups/SearchAssetsAsync so the empty-case is short-circuited.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs`:
- Around line 12-23: The code builds personIdGroups from accountSettings.People
and in AND mode (accountSettings.RequireAllPeople) it can create a single empty
group causing SearchAssetsAsync to run; add an early guard after the null check
that if accountSettings.RequireAllPeople is true and
accountSettings.People.Count == 0 then return personAssets immediately to avoid
performing searches for an empty people list; update the logic around
personIdGroups, RequireAllPeople, and the call sites that use
personIdGroups/SearchAssetsAsync so the empty-case is short-circuited.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e668ca15-c79d-4a15-8e79-6f1c171e5712
📒 Files selected for processing (10)
ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.csImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.Core/Logic/Pool/PeopleAssetsPool.csImmichFrame.WebApi.Tests/Resources/TestV1.jsonImmichFrame.WebApi.Tests/Resources/TestV2.jsonImmichFrame.WebApi.Tests/Resources/TestV2.ymlImmichFrame.WebApi.Tests/Resources/TestV2_NoGeneral.jsonImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.csImmichFrame.WebApi/Models/ServerSettings.csdocs/docs/getting-started/configuration.md
|
Nice! This LGTM. Can you also add the new setting to the examples please (they are in the docker folder). |
FlorianObermayer
force-pushed
the
Feat/Add-RequireAllPeople-setting-for-AND-based-person-filtering
branch
from
2ba1121 to
7c6dfa2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs (1)
48-52:⚠️ Potential issue | 🟠 MajorPagination condition uses the wrong signal and can drop pages.
Line 52 compares
totaltobatchSize; this usually reflects global match count, not items returned in the current page. For totals > 1000, pagination can stop early after page 1.Proposed fix
- int total; + int itemsInPage; do { var metadataBody = new MetadataSearchDto { Page = page, Size = batchSize, PersonIds = personIds, WithExif = true, WithPeople = true }; if (!accountSettings.ShowVideos) { metadataBody.Type = AssetTypeEnum.IMAGE; } var personInfo = await immichApi.SearchAssetsAsync(metadataBody, ct); - total = personInfo.Assets.Total; + itemsInPage = personInfo.Assets.Items.Count; personAssets.AddRange(personInfo.Assets.Items); page++; - } while (total == batchSize); + } while (itemsInPage == batchSize);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs` around lines 48 - 52, The pagination loop in PeopleAssetsPool (in PeopleAssetsPool.cs) uses the wrong condition—comparing total to batchSize—so pagination can stop early; change the loop to continue while the current page returned a full batch (e.g., while (personInfo.Assets.Items.Count == batchSize) or equivalent) and rely on the returned item count (personInfo.Assets.Items) instead of total; ensure personAssets.AddRange(personInfo.Assets.Items) and page++ remain unchanged and that total is used only for metadata, not the loop condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker/Settings.example.yml`:
- Line 1: The file containing the "General:" YAML header has CRLF endings;
convert that file to LF line endings (e.g., run dos2unix on the file or set your
editor to save with LF, or run git add --renormalize <file>) and recommit so the
"General:" line (and whole file) uses '\n' endings to satisfy YAMLlint/CI
checks. Ensure your local git core.autocrlf or editor settings are updated to
prevent reintroducing CRLF.
In `@ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs`:
- Around line 21-23: If accountSettings.RequireAllPeople is true but the people
collection is empty, the current logic builds a single empty group in
personIdGroups and still performs the search; guard this by checking
people.Any() (or people.Count > 0) before creating personIdGroups and before
executing the search in the PeopleAssetsPool logic: when RequireAllPeople is
true and people is empty, short-circuit to return an empty result (or skip the
search) instead of constructing an empty group; update the code paths that
reference personIdGroups and the search invocation so they only run when people
has elements.
---
Outside diff comments:
In `@ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs`:
- Around line 48-52: The pagination loop in PeopleAssetsPool (in
PeopleAssetsPool.cs) uses the wrong condition—comparing total to batchSize—so
pagination can stop early; change the loop to continue while the current page
returned a full batch (e.g., while (personInfo.Assets.Items.Count == batchSize)
or equivalent) and rely on the returned item count (personInfo.Assets.Items)
instead of total; ensure personAssets.AddRange(personInfo.Assets.Items) and
page++ remain unchanged and that total is used only for metadata, not the loop
condition.
🪄 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: 43019efb-3523-44c6-ad49-f8c07e10d8e1
📒 Files selected for processing (13)
ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.csImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.Core/Logic/Pool/PeopleAssetsPool.csImmichFrame.WebApi.Tests/Resources/TestV1.jsonImmichFrame.WebApi.Tests/Resources/TestV2.jsonImmichFrame.WebApi.Tests/Resources/TestV2.ymlImmichFrame.WebApi.Tests/Resources/TestV2_NoGeneral.jsonImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.csImmichFrame.WebApi/Models/ServerSettings.csdocker/Settings.example.jsondocker/Settings.example.ymldocker/example.envdocs/docs/getting-started/configuration.md
✅ Files skipped from review due to trivial changes (3)
- ImmichFrame.WebApi.Tests/Resources/TestV2.yml
- ImmichFrame.WebApi.Tests/Resources/TestV1.json
- ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/docs/getting-started/configuration.md
- ImmichFrame.Core/Interfaces/IServerSettings.cs
- ImmichFrame.WebApi.Tests/Resources/TestV2_NoGeneral.json
- ImmichFrame.WebApi/Models/ServerSettings.cs
- ImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.cs
- ImmichFrame.WebApi.Tests/Resources/TestV2.json
Just amended the change but apparently I have broken line ending settings. Moved the PR into draft state until those are resolved. |
FlorianObermayer
force-pushed
the
Feat/Add-RequireAllPeople-setting-for-AND-based-person-filtering
branch
from
7c6dfa2 to
d5821fd
Compare
Done! I think there are some line ending differences in this project, though (most cs files are |
FlorianObermayer
force-pushed
the
Feat/Add-RequireAllPeople-setting-for-AND-based-person-filtering
branch
from
6fababe to
9f7722d
Compare
FlorianObermayer
force-pushed
the
Feat/Add-RequireAllPeople-setting-for-AND-based-person-filtering
branch
from
9f7722d to
e72e812
Compare
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.Web/src/lib/components/home-page/home-page.svelte`:
- Around line 202-205: The consecutive error counter is being reset too early in
the handleDone() method at line 204, before the replacement asset has proven to
work. Remove the consecutiveErrorSkips reset from the finally block in
handleDone(). Instead, keep the existing reset in the onVideoPlaying handler for
video slides since that confirms successful playback, and add a separate reset
for image-only slides immediately after they are confirmed as successfully
fetched. This ensures the counter can properly accumulate across a streak of
failures before triggering the tripwire at the greater-than-10 threshold.
🪄 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: 170d815f-4b2b-475a-b7b1-edc79357cde9
📒 Files selected for processing (18)
ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.csImmichFrame.Core/Interfaces/IServerSettings.csImmichFrame.Core/Logic/Pool/MemoryAssetsPool.csImmichFrame.Core/Logic/Pool/PeopleAssetsPool.csImmichFrame.WebApi.Tests/Resources/TestV1.jsonImmichFrame.WebApi.Tests/Resources/TestV2.jsonImmichFrame.WebApi.Tests/Resources/TestV2.ymlImmichFrame.WebApi.Tests/Resources/TestV2_NoGeneral.jsonImmichFrame.WebApi/Helpers/Config/ServerSettingsV1.csImmichFrame.WebApi/Models/ServerSettings.csdocker/Settings.example.jsondocker/Settings.example.ymldocker/example.envdocs/docs/getting-started/configuration.mdimmichFrame.Web/src/lib/components/elements/asset-component.svelteimmichFrame.Web/src/lib/components/elements/asset.svelteimmichFrame.Web/src/lib/components/elements/progress-bar.svelteimmichFrame.Web/src/lib/components/home-page/home-page.svelte
✅ Files skipped from review due to trivial changes (5)
- docker/example.env
- ImmichFrame.WebApi.Tests/Resources/TestV2.json
- ImmichFrame.WebApi.Tests/Resources/TestV2.yml
- ImmichFrame.WebApi.Tests/Resources/TestV1.json
- docker/Settings.example.yml
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/docs/getting-started/configuration.md
- ImmichFrame.WebApi/Models/ServerSettings.cs
- ImmichFrame.Core/Interfaces/IServerSettings.cs
- ImmichFrame.WebApi.Tests/Resources/TestV2_NoGeneral.json
- ImmichFrame.Core/Logic/Pool/PeopleAssetsPool.cs
- docker/Settings.example.json
- ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs
| await assetComponent?.play?.(); | ||
| progressBar.play(); | ||
| consecutiveErrorSkips = 0; | ||
| } finally { |
There was a problem hiding this comment.
Don't clear the consecutive-failure counter before the replacement asset proves healthy.
Line 204 resets consecutiveErrorSkips at the end of every handleDone(), but onAssetError reaches handleDone() for each skip. In a streak of bad videos, the counter becomes 1, the transition runs, then Line 204 immediately zeroes it again before onVideoPlaying ever fires, so the > 10 tripwire never triggers.
💡 Minimal fix
await assetComponent?.play?.();
progressBar.play();
- consecutiveErrorSkips = 0;
+ if (displayingAssets.every((asset) => !isVideoAsset(asset))) {
+ consecutiveErrorSkips = 0;
+ }Reset on a confirmed success path instead: keep the existing reset in onVideoPlaying for video slides, and only reset immediately for image-only slides that were already fetched successfully.
Also applies to: 547-560
🤖 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.Web/src/lib/components/home-page/home-page.svelte` around lines
202 - 205, The consecutive error counter is being reset too early in the
handleDone() method at line 204, before the replacement asset has proven to
work. Remove the consecutiveErrorSkips reset from the finally block in
handleDone(). Instead, keep the existing reset in the onVideoPlaying handler for
video slides since that confirms successful playback, and add a separate reset
for image-only slides immediately after they are confirmed as successfully
fetched. This ensures the counter can properly accumulate across a streak of
failures before triggering the tripwire at the greater-than-10 threshold.
2 participants
This pull request introduces a new
RequireAllPeoplesetting that changes how ImmichFrame filters assets by person.Before: Assets are returned if they feature any of the configured people (OR logic).
After: When
RequireAllPeople: trueis set, only assets featuring all configured people are returned (AND logic).This is useful for finding shared moments — for example, only showing photos where both Person A and Person B appear together.
Usage:
Summary by CodeRabbit
RequireAllPeopleaccount setting. When enabled, images must match all listed people; when disabled (default), images match any listed person.RequireAllPeople.