Feat: Add RequireAllPeople setting for AND-based person filtering#609
Open
FlorianObermayer wants to merge 3 commits intoimmichFrame:mainfrom
Open
Feat: Add RequireAllPeople setting for AND-based person filtering#609FlorianObermayer wants to merge 3 commits intoimmichFrame:mainfrom
FlorianObermayer wants to merge 3 commits intoimmichFrame:mainfrom
Conversation
…l-people-flag [FEAT] Introduced RequireAllPeople flag to filter assets for people in an AND fashion
📝 WalkthroughWalkthroughA new per-account Changes
Sequence Diagram(s)sequenceDiagram
participant Pool as PersonAssetsPool
participant Settings as AccountSettings
participant API as MetadataSearchService
participant Storage as Storage/DB
Pool->>Settings: read People[], RequireAllPeople
alt RequireAllPeople == true
Pool->>API: Search(MetadataSearchDto{PersonIds = [all people], Page=1})
else
loop per personId
Pool->>API: Search(MetadataSearchDto{PersonIds = [personId], Page=1})
end
end
API->>Storage: query by PersonIds + pagination
Storage-->>API: page results
API-->>Pool: results (may be multiple pages)
loop while API has next page
Pool->>API: Search(next Page)
API-->>Pool: next page results
end
Pool->>Pool: aggregate results per strategy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
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 |
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 | 🟡 MinorGuard against an empty
Peoplelist in AND mode.With
RequireAllPeopleenabled, an empty list now becomes oneSearchAssetsAsynccall withPersonIds = []. That changes the old “no people => no assets” behavior and can widen the query if Immich treats an empty filter as “unset”. Please short-circuit empty lists before buildingpersonIdGroups.Suggested fix
var people = accountSettings.People; - if (people == null) + if (people == null || !people.Any()) { 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 using accountSettings.RequireAllPeople but doesn't guard against an empty people list, which causes an unintended broad query when RequireAllPeople is true; to fix, check accountSettings.People (the people variable) for null or empty before constructing personIdGroups and short-circuit by returning personAssets if people.Count == 0 when accountSettings.RequireAllPeople is true; adjust the logic around personIdGroups (the ternary using RequireAllPeople) so the empty-list case returns early rather than creating a single empty group, ensuring SearchAssetsAsync is never called with PersonIds = [].
🧹 Nitpick comments (1)
ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs (1)
127-153: Consider adding an edge-case test for single-person AND mode.When
RequireAllPeople=trueand only one person is configured, the behavior should be identical to OR mode. Adding a test for this edge case would document and verify this equivalence.📋 Suggested additional test
[Test] public async Task LoadAssets_RequireAllPeople_SinglePerson_BehavesSameAsOrMode() { // Arrange var personId = Guid.NewGuid(); _mockAccountSettings.SetupGet(s => s.People).Returns(new List<Guid> { personId }); _mockAccountSettings.SetupGet(s => s.RequireAllPeople).Returns(true); var assets = Enumerable.Range(0, 5).Select(i => CreateAsset($"asset_{i}")).ToList(); _mockImmichApi.Setup(api => api.SearchAssetsAsync( It.Is<MetadataSearchDto>(d => d.PersonIds.Contains(personId) && d.PersonIds.Count == 1), It.IsAny<CancellationToken>())) .ReturnsAsync(CreateSearchResult(assets, 5)); // Act var result = (await _personAssetsPool.TestLoadAssets()).ToList(); // Assert Assert.That(result.Count, Is.EqualTo(5)); _mockImmichApi.Verify(api => api.SearchAssetsAsync(It.IsAny<MetadataSearchDto>(), It.IsAny<CancellationToken>()), Times.Once); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs` around lines 127 - 153, Add a new unit test in PersonAssetsPoolTests named LoadAssets_RequireAllPeople_SinglePerson_BehavesSameAsOrMode that sets _mockAccountSettings.People to a single Guid and _mockAccountSettings.RequireAllPeople to true, arranges _mockImmichApi.Setup to expect a MetadataSearchDto containing that single personId (and PersonIds.Count == 1) and to return a sample CreateSearchResult, invokes _personAssetsPool.TestLoadAssets(), and asserts the returned count equals the expected number and that SearchAssetsAsync was called exactly once; this verifies the AND-mode behavior for one person matches OR-mode.
🤖 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 using
accountSettings.RequireAllPeople but doesn't guard against an empty people list,
which causes an unintended broad query when RequireAllPeople is true; to fix,
check accountSettings.People (the people variable) for null or empty before
constructing personIdGroups and short-circuit by returning personAssets if
people.Count == 0 when accountSettings.RequireAllPeople is true; adjust the
logic around personIdGroups (the ternary using RequireAllPeople) so the
empty-list case returns early rather than creating a single empty group,
ensuring SearchAssetsAsync is never called with PersonIds = [].
---
Nitpick comments:
In `@ImmichFrame.Core.Tests/Logic/Pool/PersonAssetsPoolTests.cs`:
- Around line 127-153: Add a new unit test in PersonAssetsPoolTests named
LoadAssets_RequireAllPeople_SinglePerson_BehavesSameAsOrMode that sets
_mockAccountSettings.People to a single Guid and
_mockAccountSettings.RequireAllPeople to true, arranges _mockImmichApi.Setup to
expect a MetadataSearchDto containing that single personId (and PersonIds.Count
== 1) and to return a sample CreateSearchResult, invokes
_personAssetsPool.TestLoadAssets(), and asserts the returned count equals the
expected number and that SearchAssetsAsync was called exactly once; this
verifies the AND-mode behavior for one person matches OR-mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7faac249-635c-485a-b58e-ee27fe4b4c8f
📒 Files selected for processing (9)
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.cs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/docs/getting-started/configuration.md (1)
141-142: Consider clarifying the OR vs AND semantics.The current description is accurate but doesn't clearly explain the key distinction between the default behavior (OR logic) and the enabled behavior (AND logic). Users might not immediately understand when or why to use this setting.
Consider revising to something like:
- # If this is set, all specified people must be present in an image for it to be displayed. - RequireAllPeople: false # boolean + # Controls person filtering logic. When false (default), images featuring ANY of the specified people are displayed (OR logic). + # When true, only images featuring ALL specified people are displayed (AND logic). + RequireAllPeople: false # booleanThis makes the default behavior explicit and highlights the OR vs AND distinction that's central to this feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/getting-started/configuration.md` around lines 141 - 142, Update the description for the RequireAllPeople configuration entry to explicitly state the OR vs AND semantics: explain that when RequireAllPeople is false (default) images are shown if they contain any of the listed people (OR logic), and when true images are shown only if they contain all listed people (AND logic), and include a short example for each case referencing the RequireAllPeople key to make the behavior unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/docs/getting-started/configuration.md`:
- Around line 141-142: Update the description for the RequireAllPeople
configuration entry to explicitly state the OR vs AND semantics: explain that
when RequireAllPeople is false (default) images are shown if they contain any of
the listed people (OR logic), and when true images are shown only if they
contain all listed people (AND logic), and include a short example for each case
referencing the RequireAllPeople key to make the behavior unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 184b95ad-fb07-455c-883e-f3c246bdcb3c
📒 Files selected for processing (1)
docs/docs/getting-started/configuration.md
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.
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