[FEAT] Introduced RequireAllPeople flag to filter assets for people in an AND fashion#626
Open
FlorianObermayer wants to merge 1 commit intoimmichFrame:mainfrom
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 (13)
✅ Files skipped from review due to trivial changes (6)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdded a per-account Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
Collaborator
|
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
Author
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
Author
Done! I think there are some line ending differences in this project, though (most cs files are |
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
2 participants
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.
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
New Features
Documentation
Tests
Chores